Unused synth_immediate method

[Okash Khawaja]

Hi,

On Mon, Feb 20, 2017 at 12:57 PM, Samuel Thibault
<samuel.thibault at ens-lyon.org> wrote:
> Hello,
>
> Okash Khawaja, on lun. 20 févr. 2017 12:23:45 +0000, wrote:
>> > On 19 Feb 2017, at 23:06, Samuel Thibault <samuel.thibault at ens-lyon.org> wrote:
>> >
>> > Okash Khawaja, on dim. 19 févr. 2017 10:44:36 +0000, wrote:
>> >> Some drivers like apollo and bns only assign synth_immediate method in
>> >> their spk_synth structs but don't seem to use it. For those drivers,
>> >> can it be set to NULL?
>> >
>> > I guess this method could be useful for some screen reading features,
>> > just not used yet.
>>
>> I see. Now because they are assigned but not used, they may cause confusion. For example, spk_synth_immediate uses outb.
>
> Oh, I didn't realize this.
>
>> When migrating a synth to ttyio, if synth_immediate is left assigned, it may look like that synth isn't fully migrated yet.
>
> Indeed, and that can only lead to problems later.
>
>> We can leave as it is although I would prefer it to be set to NULL
>> until it is actually used.
>
> Indeed.
>
> Ideally however we'd have a ttyio variant to assign here. It shouldn't
> be hard, something like this (untested):
>
> const char *spk_ttyio_synth_immediate(struct spk_synth *synth, const char *buff)
> {
>         u_char ch;
>
>         while ((ch = *buff)) {
>                 if (ch == '\n')
>                         ch = synth->procspeech;
>                 if (tty_write_room(speakup_tty) < 1 || !spk_ttyio_out(ch))
>                         return buff;
>                 buff++;
>         }
>         return NULL;
> }

I've already done this for speakup_acntsa.

>
> And the existing spk_synth_immediate should be renamed to
> spk_serial_synth_immediate to make it clear it uses I/O ports.

Sure, and I'll also move it into serialio.c from your comments below.
>
> Thinking about this, spk_ttyio_out should check the result of the write
> operation, something like this:

Good point. Will update the code.
>
> int spk_ttyio_out(const char ch)
> {
>         if (synth->alive && speakup_tty && speakup_tty->ops->write) {
>                 int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
>                 if (ret == 0)
>                         /* No room */
>                         return 0;
>                 if (ret < 0) {
>                         pr_warn("%s: I/O error, deactivating speakup\n", synth->long_name);
>                         /* No synth any more, so nobody will restart TTYs, and we thus
>                          * need to do it ourselves.  Now that there is no synth we can
>                          * let application flood anyway
>                          */
>                         synth->alive = 0;
>                         speakup_start_ttys();
>                         return 0;
>                 }
>                 return 1;
>         }
>         return 0;
> }
>
> BTW, I can see that spk_serial_synth_probe uses outb too, it should be
> moved to serialio.c too. In the end, we should have a situation where
> only serialio.c functions use in[bwl]/out[bwl], and these functions be
> prefixed with spk_serial_, so that it's easy to know which code parts
> still use them.

Cool, will send in the patch set.

Okash