Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 18 Jun 2017 16:35:21 +0300 | Subject | Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t |
| |
On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote: > This patch adds functionality to validate and convert either a device > name or 'ser' member of synth into dev_t. Subsequent patch in this set > will call it to convert user-specified device into device number. For > device name, this patch does some basic sanity checks on the string > passed in. It currently supports ttyS*, ttyUSB* and, for selected > synths, lp*. > > The patch also introduces a string member variable named 'dev_name' to > struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc - > which needs conversion to dev_t.
Looks fine. Few minor comments below, and take my Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com> > > --- > drivers/staging/speakup/spk_priv.h | 2 + > drivers/staging/speakup/spk_ttyio.c | 46 ++++++++++++++++++++++++++++++++++++ > drivers/staging/speakup/spk_types.h | 1 > 3 files changed, 49 insertions(+) > > --- a/drivers/staging/speakup/spk_priv.h > +++ b/drivers/staging/speakup/spk_priv.h > @@ -40,6 +40,8 @@ > > #define KT_SPKUP 15 > #define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */ > +#define SYNTH_DEFAULT_DEV "ttyS0" > +#define SYNTH_DEFAULT_SER 0 > > const struct old_serial_port *spk_serial_init(int index); > void spk_stop_serial_interrupt(void); > --- a/drivers/staging/speakup/spk_ttyio.c > +++ b/drivers/staging/speakup/spk_ttyio.c > @@ -7,6 +7,10 @@ > #include "spk_types.h" > #include "spk_priv.h" > > +#define DEV_PREFIX_LP "lp" > +
> +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
static ?
> + > struct spk_ldisc_data { > char buf; > struct semaphore sem; > @@ -16,6 +20,48 @@ struct spk_ldisc_data { > static struct spk_synth *spk_ttyio_synth; > static struct tty_struct *speakup_tty; > > +int ser_to_dev(int ser, dev_t *dev_no) > +{ > + if (ser < 0 || ser > (255 - 64)) {
> + pr_err("speakup: Invalid ser param. \ > + Must be between 0 and 191 inclusive.\n");
Just make it one line.
> + > + return -EINVAL; > + } > + > + *dev_no = MKDEV(4, (64 + ser)); > + return 0; > +} > + > +static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no) > +{ > + /* use ser only when dev is not specified */ > + if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) || synth->ser == SYNTH_DEFAULT_SER) { > + /* for /dev/lp* check if synth is supported */ > + if (strncmp(synth->dev_name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
> + int i; > + > + for (i = 0; i < ARRAY_SIZE(lp_supported); i++) { > + if (strcmp(synth->name, lp_supported[i]) == 0) > + break; > + } > + > + if (i >= ARRAY_SIZE(lp_supported)) {
match_string()
> + pr_err("speakup: lp* is only supported on:");
> + for (i = 0; i < ARRAY_SIZE(lp_supported); i++) > + pr_cont(" %s", lp_supported[i]); > + pr_cont("\n");
pr_cont() is not the best idea, though I think it will be rare cases when it might be broken in pieces.
> + > + return -ENOTSUPP; > + } > + } > + > + return tty_dev_name_to_number(synth->dev_name, dev_no); > + } > + > + return ser_to_dev(synth->ser, dev_no); > +} > + > static int spk_ttyio_ldisc_open(struct tty_struct *tty) > { > struct spk_ldisc_data *ldisc_data; > --- a/drivers/staging/speakup/spk_types.h > +++ b/drivers/staging/speakup/spk_types.h > @@ -169,6 +169,7 @@ struct spk_synth { > int jiffies; > int full; > int ser;
> + char *dev_name;
const ?
> short flags; > short startup; > const int checkval; /* for validating a proper synth module */ >
-- With Best Regards, Andy Shevchenko
| |