| Message ID | 20230601201844.3739926-1-hugo@hugovil.com |
|---|---|
| Headers | show |
| Series | serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand |
On Fri, 2 Jun 2023 00:30:14 +0300 andy.shevchenko@gmail.com wrote: > Thu, Jun 01, 2023 at 04:18:40PM -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. > > > > Allow to specify GPIO or modem control line function in the device > > tree, and for each of the ports (A or B). > > > > Do so by using the new device-tree property named > > "modem-control-line-ports" (property added in separate patch). > > > > When registering GPIO chip controller, mask-out GPIO pins declared as > > modem control lines according to this new "modem-control-line-ports" > > 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 > > Almost good, a few remarks and if addressed as suggested, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Thank you! > > ... > > > + if (!s->gpio_valid_mask) > > I would use == 0, but it's up to you. Both will work equally. Hi, done. > > + return 0; > > ... > > > +static int sc16is7xx_setup_mctrl_ports(struct device *dev) > > Not sure why int if you always return an unsigned value. > Otherwise return an error code when it's no defined mask > and check it in the caller. Changed return type to u8. > > +{ > > + struct sc16is7xx_port *s = dev_get_drvdata(dev); > > + int i; > > + int ret; > > + int count; > > + u32 mctrl_port[2]; > > + u8 mctrl_mask = 0; > > I would return 0 directly in the first two cases and split an assignment closer > to the first user. > > > + count = device_property_count_u32(dev, "nxp,modem-control-line-ports"); > > + if (count < 0 || count > ARRAY_SIZE(mctrl_port)) > > + return mctrl_mask; > > return 0; Done. > > + ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports", > > + mctrl_port, count); > > + if (ret) > > + return mctrl_mask; > > return 0; Done. > mctrl_mask = 0; Done. > > + for (i = 0; i < count; i++) { > > + /* Use GPIO lines as modem control lines */ > > + if (mctrl_port[i] == 0) > > + mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > > + else if (mctrl_port[i] == 1) > > + mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > > + } > > > + if (!mctrl_mask) > > + return mctrl_mask; > > Maybe positive one? > if (mctrl_mask) > regmap_update_bits(...); I used negative to save on indentation, but it also fits by converting it to positive, so done. > > + regmap_update_bits(s->regmap, > > + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, > > + SC16IS7XX_IOCONTROL_MODEM_A_BIT | > > + SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask); > > + > > + return mctrl_mask; > > +} > > ... > > > unsigned long freq = 0, *pfreq = dev_get_platdata(dev); > > unsigned int val; > > + u8 mctrl_mask = 0; > > This assignment is redundant, so you simply can define it > > > u32 uartclk = 0; > > u8 mctrl_mask; Done. I will send a V7 soon with your Reviewed-by tag. Thank you, Hugo. > > int i, ret; > > struct sc16is7xx_port *s; > > -- > With Best Regards, > Andy Shevchenko
On Thu, 1 Jun 2023 20:41:40 -0400 Hugo Villeneuve <hugo@hugovil.com> wrote: > On Fri, 2 Jun 2023 00:30:14 +0300 > andy.shevchenko@gmail.com wrote: > > > Thu, Jun 01, 2023 at 04:18:40PM -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. > > > > > > Allow to specify GPIO or modem control line function in the device > > > tree, and for each of the ports (A or B). > > > > > > Do so by using the new device-tree property named > > > "modem-control-line-ports" (property added in separate patch). > > > > > > When registering GPIO chip controller, mask-out GPIO pins declared as > > > modem control lines according to this new "modem-control-line-ports" > > > 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 > > > > Almost good, a few remarks and if addressed as suggested, > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Thank you! > > > > ... > > > > > + if (!s->gpio_valid_mask) > > > > I would use == 0, but it's up to you. Both will work equally. > > Hi, > done. > > > > + return 0; > > > > ... > > > > > +static int sc16is7xx_setup_mctrl_ports(struct device *dev) > > > > Not sure why int if you always return an unsigned value. > > Otherwise return an error code when it's no defined mask > > and check it in the caller. > > Changed return type to u8. > > > > > +{ > > > + struct sc16is7xx_port *s = dev_get_drvdata(dev); > > > + int i; > > > + int ret; > > > + int count; > > > + u32 mctrl_port[2]; > > > + u8 mctrl_mask = 0; > > > > I would return 0 directly in the first two cases and split an assignment closer > > to the first user. > > > > > + count = device_property_count_u32(dev, "nxp,modem-control-line-ports"); > > > + if (count < 0 || count > ARRAY_SIZE(mctrl_port)) > > > + return mctrl_mask; > > > > return 0; > > Done. > > > > > + ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports", > > > + mctrl_port, count); > > > + if (ret) > > > + return mctrl_mask; > > > > return 0; > > Done. > > > > mctrl_mask = 0; > > Done. > > > > > + for (i = 0; i < count; i++) { > > > + /* Use GPIO lines as modem control lines */ > > > + if (mctrl_port[i] == 0) > > > + mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; > > > + else if (mctrl_port[i] == 1) > > > + mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; > > > + } > > > > > + if (!mctrl_mask) > > > + return mctrl_mask; > > > > Maybe positive one? > > if (mctrl_mask) > > regmap_update_bits(...); > > I used negative to save on indentation, but it also fits by converting it to positive, so done. > > > > > + regmap_update_bits(s->regmap, > > > + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, > > > + SC16IS7XX_IOCONTROL_MODEM_A_BIT | > > > + SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask); > > > + > > > + return mctrl_mask; > > > +} > > > > ... > > > > > unsigned long freq = 0, *pfreq = dev_get_platdata(dev); > > > unsigned int val; > > > + u8 mctrl_mask = 0; > > > > This assignment is redundant, so you simply can define it > > > > > u32 uartclk = 0; > > > > u8 mctrl_mask; > > Done. > > I will send a V7 soon with your Reviewed-by tag. Hi Andy, Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:). Hugo.
On Fri, Jun 2, 2023 at 4:25 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > On Thu, 1 Jun 2023 20:41:40 -0400 > Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Fri, 2 Jun 2023 00:30:14 +0300 > > andy.shevchenko@gmail.com wrote: > > > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti: ... > > > Maybe positive one? > > > if (mctrl_mask) > > > regmap_update_bits(...); > > > > I used negative to save on indentation, but it also fits by converting it to positive, so done. I understand, but in this case it is slightly more weird to have negative conditional and in either case return the value of the local variable. ... > Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:). They look fine to me, but Greg is the maintainer, he decides if it's really okay or not.
On Fri, 2 Jun 2023 15:58:43 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 2, 2023 at 4:25 AM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Thu, 1 Jun 2023 20:41:40 -0400 > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Fri, 2 Jun 2023 00:30:14 +0300 > > > andy.shevchenko@gmail.com wrote: > > > > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti: > > ... > > > > > Maybe positive one? > > > > if (mctrl_mask) > > > > regmap_update_bits(...); > > > > > > I used negative to save on indentation, but it also fits by converting it to positive, so done. > > I understand, but in this case it is slightly more weird to have > negative conditional and in either case return the value of the local > variable. Yes. > > Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:). > > They look fine to me, but Greg is the maintainer, he decides if it's > really okay or not. Then I will submit V7 now. Thank you, Hugo.
From: Hugo Villeneuve <hvilleneuve@dimonoff.com> Hello, this patch series mainly fixes a GPIO regression and improve RS485 flags and properties detection from DT. It now also includes various small fixes and improvements that were previously sent as separate patches, but that made testing everything difficult. Patch 1 fixes an issue with init of first port during probing. Patch 2 fixes an issue when debugging IOcontrol register, but it is also necessary for patch "fix regression with GPIO configuration" to work. Patch 3 is a refactor of GPIO registration code in preparation for patch 5. Patches 4 and 5 fix a GPIO regression by (re)allowing to choose GPIO function for GPIO pins shared with modem status lines. Patch 6 fixes a bug with the output value when first setting the GPIO direction. Patch 7 allows to read common rs485 device-tree flags and properties. Patch 8 introduces a delay after a reset operation to respect datasheet timing recommandations. Patch 9 improves comments about chip variants. I have tested the changes on a custom board with two SC16IS752 DUART using a Variscite IMX8MN NANO SOM. Thank you. Link: [v1] https://lkml.org/lkml/2023/5/17/967 [v1] https://lkml.org/lkml/2023/5/17/777 [v1] https://lkml.org/lkml/2023/5/17/780 [v1] https://lkml.org/lkml/2023/5/17/785 [v1] https://lkml.org/lkml/2023/5/17/1311 [v2] https://lkml.org/lkml/2023/5/18/516 [v3] https://lkml.org/lkml/2023/5/25/7 [v4] https://lkml.org/lkml/2023/5/29/656 Changes for V3: - Integrated all patches into single serie to facilitate debugging and tests. - Reduce number of exported GPIOs depending on new property nxp,modem-control-line-ports - Added additional example in DT bindings Changes for V4: - Increase reset post delay to relax scheduler. - Put comments patches at the end. - Remove Fixes tag for patch "mark IOCONTROL register as volatile". - Improve commit messages after reviews. - Fix coding style issues after reviews. - Change GPIO registration to always register the maximum number of GPIOs supported by the chip, but maks-out GPIOs declared as modem control lines. - Add patch to refactor GPIO registration. - Remove patch "serial: sc16is7xx: fix syntax error in comments". - Remove patch "add dump registers function" Changes for V5: - Change patch order to facilitate stable backport(s). - Change duplicate device addresses in DT binding examples. - Use GENMASK for bit masks. - Replace of_property_for_each_u32() with device_property_read_u32_array - Add "Cc: stable..." tags Changes for V6: - Fix compilation bug introduced by patch 3 Hugo Villeneuve (9): serial: sc16is7xx: fix broken port 0 uart init serial: sc16is7xx: mark IOCONTROL register as volatile serial: sc16is7xx: refactor GPIO controller registration dt-bindings: sc16is7xx: Add property to change GPIO function serial: sc16is7xx: fix regression with GPIO configuration serial: sc16is7xx: fix bug when first setting GPIO direction serial: sc16is7xx: add call to get rs485 DT flags and properties serial: sc16is7xx: add post reset delay serial: sc16is7xx: improve comments about variants .../bindings/serial/nxp,sc16is7xx.txt | 46 +++++ drivers/tty/serial/sc16is7xx.c | 168 +++++++++++++----- 2 files changed, 174 insertions(+), 40 deletions(-) base-commit: 929ed21dfdb6ee94391db51c9eedb63314ef6847