Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753002AbdFRNfY (ORCPT ); Sun, 18 Jun 2017 09:35:24 -0400 Received: from mail-qt0-f196.google.com ([209.85.216.196]:36814 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752102AbdFRNfW (ORCPT ); Sun, 18 Jun 2017 09:35:22 -0400 MIME-Version: 1.0 In-Reply-To: <20170618093536.021961426@gmail.com> References: <20170618085825.601359240@gmail.com> <20170618093536.021961426@gmail.com> From: Andy Shevchenko Date: Sun, 18 Jun 2017 16:35:21 +0300 Message-ID: Subject: Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t To: Okash Khawaja Cc: Greg Kroah-Hartman , Jiri Slaby , Samuel Thibault , "linux-kernel@vger.kernel.org" , William Hubbs , Chris Brannon , Kirk Reiser , speakup@linux-speakup.org, devel@driverdev.osuosl.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4147 Lines: 135 On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja 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 > > Signed-off-by: Okash Khawaja > > --- > 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