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.