[patch 6/7] staging: speakup: add send_xchar, tiocmset and input functionality for tty

[Samuel Thibault]

Hello,

Okash Khawaja, on jeu. 13 avril 2017 18:41:34 +0100, wrote:
> +static struct spk_synth *spk_ttyio_synth;
> +/* TODO: make this part of synth and remove the global */

Mmm, rather the other way round: add a synth pointer inside the
spk_ldisc_data structure, so you can get the synth from 

> +static int spk_ttyio_receive_buf2(struct tty_struct *tty,


> -		pr_err("Error registering line discipline.\n");
> +		pr_err("speakup: Error registering line discipline.\n");

Do not mix with unrelated changes :)

> +static unsigned char ttyio_in(int timeout)
> +{
> +	struct spk_ldisc_data *ldisc_data =
> +		(struct spk_ldisc_data *)speakup_tty->disc_data;
> +	char rv;
> +
> +	if (!down_timeout(&ldisc_data->sem, usecs_to_jiffies(timeout))) {

Err, this should actually be if (down_timeout(...) != 0) (error case),
shouldn't it?

> +		pr_warn("spk_ttyio: timeout (%d)  while waiting for input\n",
> +				timeout);
> +		return 0xff;
> +	}
> +
> +	rv = ldisc_data->buf;
> +	/* Make sure we have read buf before we set buf_free to let
> +	 * the producer overwrite it */
> +	mb();
> +	ldisc_data->buf_free = true;
> +	tty_schedule_flip(speakup_tty->port);

Here, document that tty_schedule_flip lets the tty push more characters.

> +	up(&ldisc_data->sem);

Err, no, it's spk_ttyio_receive_buf2 (the producer) which does the up(),
the consumer shouldn't do any up()!  This is really a resource-counting
semaphore, not a mutex!

> Index: linux-staging/drivers/staging/speakup/spk_priv.h
> ===================================================================
> --- linux-staging.orig/drivers/staging/speakup/spk_priv.h
> +++ linux-staging/drivers/staging/speakup/spk_priv.h
> @@ -39,6 +39,7 @@
>  #endif
>  
>  #define KT_SPKUP 15
> +#define SPK_SYNTH_TIMEOUT 100000

Please document the unit (us)

Samuel