Messages in this thread | | | From | andy.shevchenko@gmail ... | Date | Thu, 25 May 2023 14:19:52 +0300 | Subject | Re: [PATCH v3 08/11] serial: sc16is7xx: fix regression with GPIO configuration |
| |
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 */ > + }
-- With Best Regards, Andy Shevchenko
| |