diff mbox series

[v4,7/9] serial: sc16is7xx: fix regression with GPIO configuration

Message ID 20230529140711.896830-8-hugo@hugovil.com
State New
Headers show
Series serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand

Commit Message

Hugo Villeneuve May 29, 2023, 2:07 p.m. UTC
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

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 | 82 +++++++++++++++++++++++++---------
 1 file changed, 62 insertions(+), 20 deletions(-)

Comments

Ilpo Järvinen May 29, 2023, 4:10 p.m. UTC | #1
On Mon, 29 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.
> 
> 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
> 
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Andy Shevchenko May 29, 2023, 10:38 p.m. UTC | #2
Mon, May 29, 2023 at 10:07:09AM -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

...

> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")

Don't forget to refer to the dependency patches form this series.
(I forgot how it should be done, IIRC the documentation about stable kernel
patches can shed a light on this.)


...

> +	switch (mctrl_mask) {
> +	case 0:
> +		s->gpio_valid_mask = 0xFF;

GENMASK()

> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> +		s->gpio_valid_mask = 0x0F;

GENMASK()

> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> +		s->gpio_valid_mask = 0xF0;

GENMASK()

> +		break;
> +	default:
> +		break;
> +	}

...

> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u) {
> +			if (u >= devtype->nr_uart)
> +				continue;
> +
> +			/* Use GPIO lines as modem control lines */
> +			if (u == 0)
> +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +			else if (u == 1)
> +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +		}

Can we use device properties, please?

If you think about backporting to the earlier kernels (w/o properties in use in
this driver), perhaps an additional followup for that?
Greg Kroah-Hartman May 30, 2023, 10:25 a.m. UTC | #3
On Mon, May 29, 2023 at 10:07:09AM -0400, 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.
> 
> 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
> 
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")

So you are marking this as a "bugfix" and yet, it is at the end of a
much larger series of patches.  Does this fix require all of them?  If
so, it's not really relevant for stable kernels, right?  Or is it?

I'm confused, what should I, as a maintainer, do here?  Take just this
one fix for 6.4-final, and the rest for 6.5-rc1?  And add a proper cc:
stable@ tag?  Or queue them all up for 6.4-final?  Or all for 6.5-rc1?
Or something else?

What would you want to see if you were in my position here to help make
your life easier?

thanks,

greg k-h
Hugo Villeneuve May 30, 2023, 3:36 p.m. UTC | #4
On Tue, 30 May 2023 01:38:17 +0300
andy.shevchenko@gmail.com wrote:

> Mon, May 29, 2023 at 10:07:09AM -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
> 
> ...
> 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> 
> Don't forget to refer to the dependency patches form this series.
> (I forgot how it should be done, IIRC the documentation about stable kernel
> patches can shed a light on this.)

Hi,
I will look into it.


> ...
> 
> > +	switch (mctrl_mask) {
> > +	case 0:
> > +		s->gpio_valid_mask = 0xFF;
> 
> GENMASK()
> 
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> > +		s->gpio_valid_mask = 0x0F;
> 
> GENMASK()
> 
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> > +		s->gpio_valid_mask = 0xF0;
> 
> GENMASK()

Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.


> > +		break;
> > +	default:
> > +		break;
> > +	}
> 
> ...
> 
> > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > +					 prop, p, u) {
> > +			if (u >= devtype->nr_uart)
> > +				continue;
> > +
> > +			/* Use GPIO lines as modem control lines */
> > +			if (u == 0)
> > +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +			else if (u == 1)
> > +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +		}
> 
> Can we use device properties, please?

I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?

> If you think about backporting to the earlier kernels (w/o properties in use in
> this driver), perhaps an additional followup for that?

I am not sure what you mean by this?
Hugo Villeneuve May 30, 2023, 4:25 p.m. UTC | #5
On Tue, 30 May 2023 11:25:53 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, May 29, 2023 at 10:07:09AM -0400, 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.
> > 
> > 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
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> 
> So you are marking this as a "bugfix" and yet, it is at the end of a
> much larger series of patches.  Does this fix require all of them?  If
> so, it's not really relevant for stable kernels, right?  Or is it?

Like I said to Andy, I will re-order the patches so that "bugfix" patches are first. See new order below.

> I'm confused, what should I, as a maintainer, do here?  Take just this
> one fix for 6.4-final, and the rest for 6.5-rc1?  And add a proper cc:
> stable@ tag?  Or queue them all up for 6.4-final?  Or all for 6.5-rc1?
> Or something else?
> 
> What would you want to see if you were in my position here to help make
> your life easier?

From what I understand from https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst,
here is the new proposed patches order as well as what I plan to do in the commit message:

2f0f23e598df serial: sc16is7xx: fix broken port 0 uart init
  I will add tag "Cc: <stable@vger.kernel.org>"
  This patch is a prerequiste of "fix regression with GPIO configuration".

f292951c521e serial: sc16is7xx: mark IOCONTROL register as volatile
  I will add tag "Cc: <stable@vger.kernel.org>"
  This patch is a prerequiste of "fix regression with GPIO configuration".
  This patch has no "Fixes:" tag because it doesn't fix a previous bug, but Lech Perczak reported that it was required for 
  patch "fix regression with GPIO configuration" to work.

78930d607121 serial: sc16is7xx: refactor GPIO controller registration
  This patch is a prerequiste of "fix regression with GPIO configuration".
  It was done separately to ease the review process, but from a stable kernel backport, maybe it would be best to integrate it directly into "fix regression with GPIO configuration"?
  If not, should I add tag "Cc: <stable@vger.kernel.org>"?

f7ba105873d7 dt-bindings: sc16is7xx: Add property to change GPIO function
  This patch is a prerequiste of "fix regression with GPIO configuration".
  I will add tag "Cc: <stable@vger.kernel.org>"
  Should I add a tag "Fixes: " like I did in patch "fix regression with GPIO configuration"?

f2238e8f69b0 serial: sc16is7xx: fix regression with GPIO configuration
  I will add tags:
    Cc: <stable@vger.kernel.org> 2f0f23e5 serial: sc16is7xx: fix broken port 0 uart init
    Cc: <stable@vger.kernel.org> f292951c serial: sc16is7xx: mark IOCONTROL register as volatile
    Cc: <stable@vger.kernel.org> 78930d60 serial: sc16is7xx: refactor GPIO controller registration
    Cc: <stable@vger.kernel.org> f7ba1058 dt-bindings: sc16is7xx: Add property to change GPIO function
    Cc: <stable@vger.kernel.org>

2d98ab070b70 serial: sc16is7xx: fix bug when first setting GPIO direction
  This is a standalone bugfix
  I will add tag "Cc: <stable@vger.kernel.org>"

658e39d9073e serial: sc16is7xx: add call to get rs485 DT flags and properties
  Enhancement

588aac544e00 serial: sc16is7xx: add post reset delay
  Enhancement

5bb1b45bca81 serial: sc16is7xx: improve comments about variants
  Comments enhancements

Please tell me if it makes sense and if some tags are wrong/missing.

Hugo.
Andy Shevchenko May 30, 2023, 9:56 p.m. UTC | #6
On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 30 May 2023 01:38:17 +0300
> andy.shevchenko@gmail.com wrote:
> > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:

...

> > GENMASK()
>
> Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.

GENMASK() was introduced to increase code robustness:
1) to make sure the bits mentioned are correct
2) to check the bit boundary.

...

> > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > +                                    prop, p, u) {
> > > +                   if (u >= devtype->nr_uart)
> > > +                           continue;
> > > +
> > > +                   /* Use GPIO lines as modem control lines */
> > > +                   if (u == 0)
> > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > +                   else if (u == 1)
> > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > +           }
> >
> > Can we use device properties, please?
>
> I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?

Yes, thank you!

> > If you think about backporting to the earlier kernels (w/o properties in use in
> > this driver), perhaps an additional followup for that?
>
> I am not sure what you mean by this?

If the device property API was not yet available for this fix being
backported to the old enough kernel we have to use old OF stuff. In
that case the device property conversion needs to be done in a
separate change.
Hugo Villeneuve May 31, 2023, 1:57 p.m. UTC | #7
On Wed, 31 May 2023 00:56:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 30 May 2023 01:38:17 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:
> 
> ...
> 
> > > GENMASK()
> >
> > Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.
> 
> GENMASK() was introduced to increase code robustness:
> 1) to make sure the bits mentioned are correct
> 2) to check the bit boundary.
> 
> ...
> 
> > > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > > +                                    prop, p, u) {
> > > > +                   if (u >= devtype->nr_uart)
> > > > +                           continue;
> > > > +
> > > > +                   /* Use GPIO lines as modem control lines */
> > > > +                   if (u == 0)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > > +                   else if (u == 1)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > > +           }
> > >
> > > Can we use device properties, please?
> >
> > I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?
> 
> Yes, thank you!
> 
> > > If you think about backporting to the earlier kernels (w/o properties in use in
> > > this driver), perhaps an additional followup for that?
> >
> > I am not sure what you mean by this?
> 
> If the device property API was not yet available for this fix being
> backported to the old enough kernel we have to use old OF stuff. In
> that case the device property conversion needs to be done in a
> separate change.

Hi,.
ok, now I see.

Thank you,
Hugo.
Hugo Villeneuve May 31, 2023, 11:57 p.m. UTC | #8
On Wed, 31 May 2023 00:56:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 30 May 2023 01:38:17 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:
> 
> ...
> 
> > > GENMASK()
> >
> > Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.
> 
> GENMASK() was introduced to increase code robustness:
> 1) to make sure the bits mentioned are correct
> 2) to check the bit boundary.
> 
> ...
> 
> > > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > > +                                    prop, p, u) {
> > > > +                   if (u >= devtype->nr_uart)
> > > > +                           continue;
> > > > +
> > > > +                   /* Use GPIO lines as modem control lines */
> > > > +                   if (u == 0)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > > +                   else if (u == 1)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > > +           }
> > >
> > > Can we use device properties, please?
> >
> > I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?
> 
> Yes, thank you!

Hi Andy,
now that I am using the device property API, I think I no longer have the need to test for "if (dev->of_node)" before reading the new property "nxp,modem-control-line-ports"?

If that is the case, I will leave the test "if (dev->of_node)" only for the "irda-mode-ports" property.

The pseudo code woulk look like this:

	if (dev->of_node) {
		struct property *prop;
		const __be32 *p;
		u32 u;

		of_property_for_each_u32(dev->of_node, "irda-mode-ports",
					 prop, p, u)
			if (u < devtype->nr_uart)
				s->p[u].irda_mode = true;
	}

        /* Read "nxp,modem-control-line-ports" using device property API. */
	sc16is7xx_setup_mctrl_ports(dev);

Thank you,
Hugo.


> > > If you think about backporting to the earlier kernels (w/o properties in use in
> > > this driver), perhaps an additional followup for that?
> >
> > I am not sure what you mean by this?
> 
> If the device property API was not yet available for this fix being
> backported to the old enough kernel we have to use old OF stuff. In
> that case the device property conversion needs to be done in a
> separate change.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>
Andy Shevchenko June 1, 2023, 9:24 a.m. UTC | #9
On Thu, Jun 1, 2023 at 2:57 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Wed, 31 May 2023 00:56:57 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Tue, 30 May 2023 01:38:17 +0300
> > > andy.shevchenko@gmail.com wrote:
> > > > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:

...

> > > > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > > > +                                    prop, p, u) {
> > > > > +                   if (u >= devtype->nr_uart)
> > > > > +                           continue;
> > > > > +
> > > > > +                   /* Use GPIO lines as modem control lines */
> > > > > +                   if (u == 0)
> > > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > > > +                   else if (u == 1)
> > > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > > > +           }
> > > >
> > > > Can we use device properties, please?
> > >
> > > I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?
> >
> > Yes, thank you!
>
> Hi Andy,
> now that I am using the device property API, I think I no longer have the need to test for "if (dev->of_node)" before reading the new property "nxp,modem-control-line-ports"?
>
> If that is the case, I will leave the test "if (dev->of_node)" only for the "irda-mode-ports" property.
>
> The pseudo code woulk look like this:
>
>         if (dev->of_node) {
>                 struct property *prop;
>                 const __be32 *p;
>                 u32 u;
>
>                 of_property_for_each_u32(dev->of_node, "irda-mode-ports",
>                                          prop, p, u)
>                         if (u < devtype->nr_uart)
>                                 s->p[u].irda_mode = true;
>         }
>
>         /* Read "nxp,modem-control-line-ports" using device property API. */
>         sc16is7xx_setup_mctrl_ports(dev);

Looks good to me, thank you for following the advice!
diff mbox series

Patch

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7a993add3f04..34739b31b44b 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -236,7 +236,8 @@ 
 
 /* IOControl register bits (Only 750/760) */
 #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;
+	unsigned long			gpio_valid_mask;
 #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)
@@ -1359,16 +1356,45 @@  static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
-static int sc16is7xx_setup_gpio_chip(struct device *dev)
+static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
+					  unsigned long *valid_mask,
+					  unsigned int ngpios)
+{
+	struct sc16is7xx_port *s = gpiochip_get_data(chip);
+
+	*valid_mask = s->gpio_valid_mask;
+
+	return 0;
+}
+
+static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(dev);
 
 	if (!s->devtype->nr_gpio)
 		return 0;
 
+	switch (mctrl_mask) {
+	case 0:
+		s->gpio_valid_mask = 0xFF;
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
+		s->gpio_valid_mask = 0x0F;
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
+		s->gpio_valid_mask = 0xF0;
+		break;
+	default:
+		break;
+	}
+
+	if (!s->gpio_valid_mask)
+		return 0;
+
 	s->gpio.owner		 = THIS_MODULE;
 	s->gpio.parent		 = dev;
 	s->gpio.label		 = dev_name(dev);
+	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
 	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
 	s->gpio.get		 = sc16is7xx_gpio_get;
 	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
@@ -1392,6 +1418,7 @@  static int sc16is7xx_probe(struct device *dev,
 {
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
 	unsigned int val;
+	u8 mctrl_mask = 0;
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
@@ -1493,12 +1520,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);
@@ -1534,6 +1555,27 @@  static int sc16is7xx_probe(struct device *dev,
 					 prop, p, u)
 			if (u < devtype->nr_uart)
 				s->p[u].irda_mode = true;
+
+		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
+					 prop, p, u) {
+			if (u >= devtype->nr_uart)
+				continue;
+
+			/* Use GPIO lines as modem control lines */
+			if (u == 0)
+				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+			else if (u == 1)
+				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+		}
+
+		if (mctrl_mask) {
+			regmap_update_bits(
+				s->regmap,
+				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+				SC16IS7XX_IOCONTROL_MODEM_B_BIT,
+				mctrl_mask);
+		}
 	}
 
 #ifdef CONFIG_GPIOLIB
@@ -1562,7 +1604,7 @@  static int sc16is7xx_probe(struct device *dev,
 		return 0;
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 
 out_thread:
@@ -1588,7 +1630,7 @@  static void sc16is7xx_remove(struct device *dev)
 	int i;
 
 #ifdef CONFIG_GPIOLIB
-	if (s->devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 #endif