| Message ID | 20230525040324.3773741-9-hugo@hugovil.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand |
Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > changed the function of the GPIOs pins to act as modem control > lines without any possibility of selecting GPIO function. > > As a consequence, applications that depends on GPIO lines configured > by default as GPIO pins no longer work as expected. > > Also, the change to select modem control lines function was done only > for channel A of dual UART variants (752/762). This was not documented > in the log message. > This new patch allows to specify GPIO or modem control line function > in the device tree, and for each of the ports (A or B). Imperative mood as stated in documentation, please. Like "Allow to specify...". > This is done by using the new device-tree property named > "modem-control-line-ports" (property added in separate patch). > > We also now reduce the number of exported GPIOs according to the > modem-status-line-port DT property. > > Boards that need to have GPIOS configured as modem control lines > should add that property to their device tree. Here is a list of > boards using the sc16is7xx driver in their device tree and that may > need to be modified: > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > mips/boot/dts/ingenic/cu1830-neo.dts > mips/boot/dts/ingenic/cu1000-neo.dts ... > +#ifdef CONFIG_GPIOLIB I'm wondering if we can avoid adding new ifdefferies... > + s->gpio_configured = devtype->nr_gpio; The name of the variable is a bit vague WRT its content. Shouldn't be as simple as the rvalue, i.e. s->nr_gpio? > +#endif /* CONFIG_GPIOLIB */ ... > + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports", > + prop, p, u) The driver so far is agnostic to property provider. Please keep it that way, i.e. no of_ APIs. > + if (u < devtype->nr_uart) { Hmm... What other can it be? > + /* Use GPIO lines as modem control lines */ > + if (u == 0) > + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > + else if (u == 1) > + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > + > +#ifdef CONFIG_GPIOLIB > + if (s->gpio_configured >= > + SC16IS7XX_GPIOS_PER_BANK) On one line it will be better to read. Esp. taking into account the above remark. > + s->gpio_configured -= > + SC16IS7XX_GPIOS_PER_BANK; Ditto. > +#endif /* CONFIG_GPIOLIB */ > + }
On Thu, 25 May 2023, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > changed the function of the GPIOs pins to act as modem control > lines without any possibility of selecting GPIO function. > > As a consequence, applications that depends on GPIO lines configured > by default as GPIO pins no longer work as expected. > > Also, the change to select modem control lines function was done only > for channel A of dual UART variants (752/762). This was not documented > in the log message. > > This new patch allows to specify GPIO or modem control line function > in the device tree, and for each of the ports (A or B). > > This is done by using the new device-tree property named > "modem-control-line-ports" (property added in separate patch). > > We also now reduce the number of exported GPIOs according to the > modem-status-line-port DT property. > > Boards that need to have GPIOS configured as modem control lines > should add that property to their device tree. Here is a list of > boards using the sc16is7xx driver in their device tree and that may > need to be modified: > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > mips/boot/dts/ingenic/cu1830-neo.dts > mips/boot/dts/ingenic/cu1000-neo.dts > > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines") > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > --- > drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 21 deletions(-) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index a5d8af0f6da0..97ec532a0a19 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -236,7 +236,8 @@ > > /* IOControl register bits (Only 75x/76x) */ > #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */ > -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */ > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */ > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */ > #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */ > > /* EFCR register bits */ > @@ -301,12 +302,12 @@ > /* Misc definitions */ > #define SC16IS7XX_FIFO_SIZE (64) > #define SC16IS7XX_REG_SHIFT 2 > +#define SC16IS7XX_GPIOS_PER_BANK 4 > > struct sc16is7xx_devtype { > char name[10]; > int nr_gpio; > int nr_uart; > - int has_mctrl; > }; > > #define SC16IS7XX_RECONF_MD (1 << 0) > @@ -336,6 +337,7 @@ struct sc16is7xx_port { > struct clk *clk; > #ifdef CONFIG_GPIOLIB > struct gpio_chip gpio; > + int gpio_configured; > #endif > unsigned char buf[SC16IS7XX_FIFO_SIZE]; > struct kthread_worker kworker; > @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = { > .name = "SC16IS74X", > .nr_gpio = 0, > .nr_uart = 1, > - .has_mctrl = 0, > }; > > static const struct sc16is7xx_devtype sc16is750_devtype = { > .name = "SC16IS750", > - .nr_gpio = 4, > + .nr_gpio = 8, > .nr_uart = 1, > - .has_mctrl = 1, > }; > > static const struct sc16is7xx_devtype sc16is752_devtype = { > .name = "SC16IS752", > - .nr_gpio = 0, > + .nr_gpio = 8, > .nr_uart = 2, > - .has_mctrl = 1, > }; > > static const struct sc16is7xx_devtype sc16is760_devtype = { > .name = "SC16IS760", > - .nr_gpio = 4, > + .nr_gpio = 8, > .nr_uart = 1, > - .has_mctrl = 1, > }; > > static const struct sc16is7xx_devtype sc16is762_devtype = { > .name = "SC16IS762", > - .nr_gpio = 0, > + .nr_gpio = 8, > .nr_uart = 2, > - .has_mctrl = 1, > }; > > static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev, > return -ENOMEM; > } > > +#ifdef CONFIG_GPIOLIB > + s->gpio_configured = devtype->nr_gpio; > +#endif /* CONFIG_GPIOLIB */ > + > /* Always ask for fixed clock rate from a property. */ > device_property_read_u32(dev, "clock-frequency", &uartclk); > > @@ -1473,12 +1474,6 @@ static int sc16is7xx_probe(struct device *dev, > SC16IS7XX_EFCR_RXDISABLE_BIT | > SC16IS7XX_EFCR_TXDISABLE_BIT); > > - /* Use GPIO lines as modem status registers */ > - if (devtype->has_mctrl) > - sc16is7xx_port_write(&s->p[i].port, > - SC16IS7XX_IOCONTROL_REG, > - SC16IS7XX_IOCONTROL_MODEM_BIT); > - > /* Initialize kthread work structs */ > kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc); > kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc); > @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev, > prop, p, u) > if (u < devtype->nr_uart) > s->p[u].irda_mode = true; > + > + val = 0; > + > + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports", > + prop, p, u) > + if (u < devtype->nr_uart) { Reverse logic + use continue. It will help with the indentation levels. > + /* Use GPIO lines as modem control lines */ > + if (u == 0) > + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > + else if (u == 1) > + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > + > +#ifdef CONFIG_GPIOLIB > + if (s->gpio_configured >= > + SC16IS7XX_GPIOS_PER_BANK) > + s->gpio_configured -= > + SC16IS7XX_GPIOS_PER_BANK; > +#endif /* CONFIG_GPIOLIB */ > + } Please use braces for of_property_for_each_u32 block. > + > + if (val) > + regmap_update_bits( > + s->regmap, > + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, > + SC16IS7XX_IOCONTROL_MODEM_A_BIT | > + SC16IS7XX_IOCONTROL_MODEM_B_BIT, val); > } > > #ifdef CONFIG_GPIOLIB > - if (devtype->nr_gpio) { > + dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured); > + > + if (s->gpio_configured) { > /* Setup GPIO controller */ > s->gpio.owner = THIS_MODULE; > s->gpio.parent = dev; > @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev, > s->gpio.direction_output = sc16is7xx_gpio_direction_output; > s->gpio.set = sc16is7xx_gpio_set; > s->gpio.base = -1; > - s->gpio.ngpio = devtype->nr_gpio; > + s->gpio.ngpio = s->gpio_configured; > s->gpio.can_sleep = 1; > ret = gpiochip_add_data(&s->gpio, s); > if (ret) > @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev, > return 0; > > #ifdef CONFIG_GPIOLIB > - if (devtype->nr_gpio) > + if (s->gpio_configured) > gpiochip_remove(&s->gpio); > > out_thread: > @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev) > int i; > > #ifdef CONFIG_GPIOLIB > - if (s->devtype->nr_gpio) > + if (s->gpio_configured) > gpiochip_remove(&s->gpio); > #endif > >
On Thu, 25 May 2023 14:19:52 +0300 andy.shevchenko@gmail.com wrote: > Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > changed the function of the GPIOs pins to act as modem control > > lines without any possibility of selecting GPIO function. > > > > As a consequence, applications that depends on GPIO lines configured > > by default as GPIO pins no longer work as expected. > > > > Also, the change to select modem control lines function was done only > > for channel A of dual UART variants (752/762). This was not documented > > in the log message. > > > This new patch allows to specify GPIO or modem control line function > > in the device tree, and for each of the ports (A or B). > > Imperative mood as stated in documentation, please. > Like "Allow to specify...". > > > This is done by using the new device-tree property named > > "modem-control-line-ports" (property added in separate patch). > > > > We also now reduce the number of exported GPIOs according to the > > modem-status-line-port DT property. Just noticed a mistake: s/modem-status-line-port/modem-control-line-ports > > > > Boards that need to have GPIOS configured as modem control lines > > should add that property to their device tree. Here is a list of > > boards using the sc16is7xx driver in their device tree and that may > > need to be modified: > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > mips/boot/dts/ingenic/cu1830-neo.dts > > mips/boot/dts/ingenic/cu1000-neo.dts > > ... > > > +#ifdef CONFIG_GPIOLIB > > I'm wondering if we can avoid adding new ifdefferies... I am simply following waht was already done in the existing driver. Are you suggesting that we need to remove all these #defines? If not, what exactly do you suggest? > > + s->gpio_configured = devtype->nr_gpio; > > The name of the variable is a bit vague WRT its content. > Shouldn't be as simple as the rvalue, i.e. s->nr_gpio? Maybe the name could be improved (and/or comments). devtype->nr_gpio is the maximum "theoretical" number of GPIOs supported by the chip. s->gpio_configured is the number of GPIOs that are configured or requested according to the presence (or not) of the modem-control-line-ports property. I wanted to avoid using the same name to avoid potential confusion. Maybe devtype->nr_gpio could be renamed to devtype->nr_gpio_max and s->gpio_configured to s->nr_gpio_requested or s->nr_gpio_configured? > > +#endif /* CONFIG_GPIOLIB */ > > ... > > > + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports", > > + prop, p, u) > > The driver so far is agnostic to property provider. Please keep it that way, > i.e. no of_ APIs. The driver, before my patches, was already using the exact same function of_property_for_each_u32() to process the irda-mode-ports property, so I don't understand your comment. But what do you suggest instead of of_property_for_each_u32()? And do we need to change it also for processing the irda-mode-ports property? > > + if (u < devtype->nr_uart) { > > Hmm... What other can it be? Again, this is similar to the handling of the irda-mode-ports property. But I am not sure I understand your question/concern? I think this check is important, because if someone puts the following property in a DT: nxp,modem-control-line-ports = <0 1>; but the variant only supports 1 port, then the check is usefull, no? > > > + /* Use GPIO lines as modem control lines */ > > + if (u == 0) > > + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > > + else if (u == 1) > > + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > > + > > +#ifdef CONFIG_GPIOLIB > > + if (s->gpio_configured >= > > + SC16IS7XX_GPIOS_PER_BANK) > > On one line it will be better to read. Esp. taking into account the above remark. Fixed. > > + s->gpio_configured -= > > + SC16IS7XX_GPIOS_PER_BANK; > > Ditto. Fixed. > > > +#endif /* CONFIG_GPIOLIB */ > > + } > > -- > With Best Regards, > Andy Shevchenko > > >
On Thu, 25 May 2023 15:03:41 +0300 (EEST) Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > On Thu, 25 May 2023, Hugo Villeneuve wrote: > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > changed the function of the GPIOs pins to act as modem control > > lines without any possibility of selecting GPIO function. > > > > As a consequence, applications that depends on GPIO lines configured > > by default as GPIO pins no longer work as expected. > > > > Also, the change to select modem control lines function was done only > > for channel A of dual UART variants (752/762). This was not documented > > in the log message. > > > > This new patch allows to specify GPIO or modem control line function > > in the device tree, and for each of the ports (A or B). > > > > This is done by using the new device-tree property named > > "modem-control-line-ports" (property added in separate patch). > > > > We also now reduce the number of exported GPIOs according to the > > modem-status-line-port DT property. > > > > Boards that need to have GPIOS configured as modem control lines > > should add that property to their device tree. Here is a list of > > boards using the sc16is7xx driver in their device tree and that may > > need to be modified: > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > mips/boot/dts/ingenic/cu1830-neo.dts > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > --- > > drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++----------- > > 1 file changed, 44 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index a5d8af0f6da0..97ec532a0a19 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -236,7 +236,8 @@ > > > > /* IOControl register bits (Only 75x/76x) */ > > #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */ > > -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */ > > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */ > > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */ > > #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */ > > > > /* EFCR register bits */ > > @@ -301,12 +302,12 @@ > > /* Misc definitions */ > > #define SC16IS7XX_FIFO_SIZE (64) > > #define SC16IS7XX_REG_SHIFT 2 > > +#define SC16IS7XX_GPIOS_PER_BANK 4 > > > > struct sc16is7xx_devtype { > > char name[10]; > > int nr_gpio; > > int nr_uart; > > - int has_mctrl; > > }; > > > > #define SC16IS7XX_RECONF_MD (1 << 0) > > @@ -336,6 +337,7 @@ struct sc16is7xx_port { > > struct clk *clk; > > #ifdef CONFIG_GPIOLIB > > struct gpio_chip gpio; > > + int gpio_configured; > > #endif > > unsigned char buf[SC16IS7XX_FIFO_SIZE]; > > struct kthread_worker kworker; > > @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = { > > .name = "SC16IS74X", > > .nr_gpio = 0, > > .nr_uart = 1, > > - .has_mctrl = 0, > > }; > > > > static const struct sc16is7xx_devtype sc16is750_devtype = { > > .name = "SC16IS750", > > - .nr_gpio = 4, > > + .nr_gpio = 8, > > .nr_uart = 1, > > - .has_mctrl = 1, > > }; > > > > static const struct sc16is7xx_devtype sc16is752_devtype = { > > .name = "SC16IS752", > > - .nr_gpio = 0, > > + .nr_gpio = 8, > > .nr_uart = 2, > > - .has_mctrl = 1, > > }; > > > > static const struct sc16is7xx_devtype sc16is760_devtype = { > > .name = "SC16IS760", > > - .nr_gpio = 4, > > + .nr_gpio = 8, > > .nr_uart = 1, > > - .has_mctrl = 1, > > }; > > > > static const struct sc16is7xx_devtype sc16is762_devtype = { > > .name = "SC16IS762", > > - .nr_gpio = 0, > > + .nr_gpio = 8, > > .nr_uart = 2, > > - .has_mctrl = 1, > > }; > > > > static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > > @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev, > > return -ENOMEM; > > } > > > > +#ifdef CONFIG_GPIOLIB > > + s->gpio_configured = devtype->nr_gpio; > > +#endif /* CONFIG_GPIOLIB */ > > + > > /* Always ask for fixed clock rate from a property. */ > > device_property_read_u32(dev, "clock-frequency", &uartclk); > > > > @@ -1473,12 +1474,6 @@ static int sc16is7xx_probe(struct device *dev, > > SC16IS7XX_EFCR_RXDISABLE_BIT | > > SC16IS7XX_EFCR_TXDISABLE_BIT); > > > > - /* Use GPIO lines as modem status registers */ > > - if (devtype->has_mctrl) > > - sc16is7xx_port_write(&s->p[i].port, > > - SC16IS7XX_IOCONTROL_REG, > > - SC16IS7XX_IOCONTROL_MODEM_BIT); > > - > > /* Initialize kthread work structs */ > > kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc); > > kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc); > > @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev, > > prop, p, u) > > if (u < devtype->nr_uart) > > s->p[u].irda_mode = true; > > + > > + val = 0; > > + > > + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports", > > + prop, p, u) > > + if (u < devtype->nr_uart) { > > Reverse logic + use continue. It will help with the indentation levels. Good suggestion, done. > > + /* Use GPIO lines as modem control lines */ > > + if (u == 0) > > + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > > + else if (u == 1) > > + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > > + > > +#ifdef CONFIG_GPIOLIB > > + if (s->gpio_configured >= > > + SC16IS7XX_GPIOS_PER_BANK) > > + s->gpio_configured -= > > + SC16IS7XX_GPIOS_PER_BANK; > > +#endif /* CONFIG_GPIOLIB */ > > + } > > Please use braces for of_property_for_each_u32 block. Done > > + > > + if (val) > > + regmap_update_bits( > > + s->regmap, > > + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, > > + SC16IS7XX_IOCONTROL_MODEM_A_BIT | > > + SC16IS7XX_IOCONTROL_MODEM_B_BIT, val); > > } > > > > #ifdef CONFIG_GPIOLIB > > - if (devtype->nr_gpio) { > > + dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured); > > + > > + if (s->gpio_configured) { > > /* Setup GPIO controller */ > > s->gpio.owner = THIS_MODULE; > > s->gpio.parent = dev; > > @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev, > > s->gpio.direction_output = sc16is7xx_gpio_direction_output; > > s->gpio.set = sc16is7xx_gpio_set; > > s->gpio.base = -1; > > - s->gpio.ngpio = devtype->nr_gpio; > > + s->gpio.ngpio = s->gpio_configured; > > s->gpio.can_sleep = 1; > > ret = gpiochip_add_data(&s->gpio, s); > > if (ret) > > @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev, > > return 0; > > > > #ifdef CONFIG_GPIOLIB > > - if (devtype->nr_gpio) > > + if (s->gpio_configured) > > gpiochip_remove(&s->gpio); > > > > out_thread: > > @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev) > > int i; > > > > #ifdef CONFIG_GPIOLIB > > - if (s->devtype->nr_gpio) > > + if (s->gpio_configured) > > gpiochip_remove(&s->gpio); > > #endif > > > > > > -- > i. > >
Thu, May 25, 2023 at 11:02:55AM -0400, Hugo Villeneuve kirjoitti: > On Thu, 25 May 2023 14:19:52 +0300 > andy.shevchenko@gmail.com wrote: > > Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti: ... > > I'm wondering if we can avoid adding new ifdefferies... > > I am simply following waht was already done in the existing driver. > > Are you suggesting that we need to remove all these #defines? If not, what > exactly do you suggest? I was wondering and have nothing to suggest here. It seems a burden we have to cope with for now. > > > + s->gpio_configured = devtype->nr_gpio; > > > > The name of the variable is a bit vague WRT its content. > > Shouldn't be as simple as the rvalue, i.e. s->nr_gpio? > > Maybe the name could be improved (and/or comments). > > devtype->nr_gpio is the maximum "theoretical" number of GPIOs supported by > the chip. > > s->gpio_configured is the number of GPIOs that are configured or requested > according to the presence (or not) of the modem-control-line-ports property. > > I wanted to avoid using the same name to avoid potential confusion. > > Maybe devtype->nr_gpio could be renamed to devtype->nr_gpio_max and > s->gpio_configured to s->nr_gpio_requested or s->nr_gpio_configured? Maybe, but first try the approach with valid mask being involved. It may be that we won't need this variable at all. ... > > > + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports", > > > + prop, p, u) > > > > The driver so far is agnostic to property provider. Please keep it that way, > > i.e. no of_ APIs. > > The driver, before my patches, was already using the exact same function > of_property_for_each_u32() to process the irda-mode-ports property, so I > don't understand your comment. This is unfortunate. I missed that one, but i don't care about IrDA so much. > But what do you suggest instead of of_property_for_each_u32()? And do we need > to change it also for processing the irda-mode-ports property? device_property_read_u32_array(). Independently on the IrDA case, this one is more important and would have consequences if we avoid agnostic APIs. ... > > > + if (u < devtype->nr_uart) { > > > > Hmm... What other can it be? > > Again, this is similar to the handling of the irda-mode-ports property. > > But I am not sure I understand your question/concern? > > I think this check is important, because if someone puts the following > property in a DT: > > nxp,modem-control-line-ports = <0 1>; > > but the variant only supports 1 port, then the check is usefull, no? But you have below checks for u value. Wouldn't be enough? > > > + /* Use GPIO lines as modem control lines */ > > > + if (u == 0) > > > + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > > > + else if (u == 1) > > > + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > > > +
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index a5d8af0f6da0..97ec532a0a19 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -236,7 +236,8 @@ /* IOControl register bits (Only 75x/76x) */ #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */ -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */ +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */ +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */ #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */ /* EFCR register bits */ @@ -301,12 +302,12 @@ /* Misc definitions */ #define SC16IS7XX_FIFO_SIZE (64) #define SC16IS7XX_REG_SHIFT 2 +#define SC16IS7XX_GPIOS_PER_BANK 4 struct sc16is7xx_devtype { char name[10]; int nr_gpio; int nr_uart; - int has_mctrl; }; #define SC16IS7XX_RECONF_MD (1 << 0) @@ -336,6 +337,7 @@ struct sc16is7xx_port { struct clk *clk; #ifdef CONFIG_GPIOLIB struct gpio_chip gpio; + int gpio_configured; #endif unsigned char buf[SC16IS7XX_FIFO_SIZE]; struct kthread_worker kworker; @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = { .name = "SC16IS74X", .nr_gpio = 0, .nr_uart = 1, - .has_mctrl = 0, }; static const struct sc16is7xx_devtype sc16is750_devtype = { .name = "SC16IS750", - .nr_gpio = 4, + .nr_gpio = 8, .nr_uart = 1, - .has_mctrl = 1, }; static const struct sc16is7xx_devtype sc16is752_devtype = { .name = "SC16IS752", - .nr_gpio = 0, + .nr_gpio = 8, .nr_uart = 2, - .has_mctrl = 1, }; static const struct sc16is7xx_devtype sc16is760_devtype = { .name = "SC16IS760", - .nr_gpio = 4, + .nr_gpio = 8, .nr_uart = 1, - .has_mctrl = 1, }; static const struct sc16is7xx_devtype sc16is762_devtype = { .name = "SC16IS762", - .nr_gpio = 0, + .nr_gpio = 8, .nr_uart = 2, - .has_mctrl = 1, }; static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev, return -ENOMEM; } +#ifdef CONFIG_GPIOLIB + s->gpio_configured = devtype->nr_gpio; +#endif /* CONFIG_GPIOLIB */ + /* Always ask for fixed clock rate from a property. */ device_property_read_u32(dev, "clock-frequency", &uartclk); @@ -1473,12 +1474,6 @@ static int sc16is7xx_probe(struct device *dev, SC16IS7XX_EFCR_RXDISABLE_BIT | SC16IS7XX_EFCR_TXDISABLE_BIT); - /* Use GPIO lines as modem status registers */ - if (devtype->has_mctrl) - sc16is7xx_port_write(&s->p[i].port, - SC16IS7XX_IOCONTROL_REG, - SC16IS7XX_IOCONTROL_MODEM_BIT); - /* Initialize kthread work structs */ kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc); kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc); @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev, prop, p, u) if (u < devtype->nr_uart) s->p[u].irda_mode = true; + + val = 0; + + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports", + prop, p, u) + if (u < devtype->nr_uart) { + /* Use GPIO lines as modem control lines */ + if (u == 0) + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; + else if (u == 1) + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; + +#ifdef CONFIG_GPIOLIB + if (s->gpio_configured >= + SC16IS7XX_GPIOS_PER_BANK) + s->gpio_configured -= + SC16IS7XX_GPIOS_PER_BANK; +#endif /* CONFIG_GPIOLIB */ + } + + if (val) + regmap_update_bits( + s->regmap, + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, + SC16IS7XX_IOCONTROL_MODEM_A_BIT | + SC16IS7XX_IOCONTROL_MODEM_B_BIT, val); } #ifdef CONFIG_GPIOLIB - if (devtype->nr_gpio) { + dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured); + + if (s->gpio_configured) { /* Setup GPIO controller */ s->gpio.owner = THIS_MODULE; s->gpio.parent = dev; @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev, s->gpio.direction_output = sc16is7xx_gpio_direction_output; s->gpio.set = sc16is7xx_gpio_set; s->gpio.base = -1; - s->gpio.ngpio = devtype->nr_gpio; + s->gpio.ngpio = s->gpio_configured; s->gpio.can_sleep = 1; ret = gpiochip_add_data(&s->gpio, s); if (ret) @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev, return 0; #ifdef CONFIG_GPIOLIB - if (devtype->nr_gpio) + if (s->gpio_configured) gpiochip_remove(&s->gpio); out_thread: @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev) int i; #ifdef CONFIG_GPIOLIB - if (s->devtype->nr_gpio) + if (s->gpio_configured) gpiochip_remove(&s->gpio); #endif