[RFC] wifi: wilc1000: fix reset line assert/deassert polarity

  When using a wilc1000 chip over a spi bus, users can optionally define a
reset gpio and a chip enable gpio. The reset line of wilc1000 is active
low, so to hold the chip in reset, a low (physical) value must be applied.

The corresponding device tree binding documentation was introduced by
commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
properties") and correctly indicates that the reset line is an active-low
signal. However, the corresponding driver part, brought by commit
ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
misusing the gpiod APIs and apply an inverted logic when powering up/down
the chip (for example, setting the reset line to a logic "1" during power
up, which in fact asserts the reset line when device tree describes the
reset line as GPIO_ACTIVE_LOW). As a consequence, any platform currently
using the driver in SPI mode must use a faulty reset line description in
device tree, or else chip will be maintained in reset and will not even
allow to bring up the chip.

Fix reset line usage by inverting back the gpiod APIs usage, setting the
reset line to the logic value "0" when powering the chip, and the logic
value "1" when powering off the chip.

Fixes: ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver")
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
This issue was detected because I struggled a bit to setup a WILC-over-SPI
setup, and eventually realized that it was due to chip being hold in reset.

This patch, if accepted, will force any WILC-over-SPI user to update its
device tree description: any platform currently working correctly in this
setup likely have a wrong GPIO_ACTIVE_HIGH used on the reset line device
tree description, contrary to what the documentation says. I am not sure
whether this is tolerable ? If not, what would be the proper way to fix
this ? Make the driver manually parse this flag and somehow make it able
to deal with both versions (so basically: ignoring the setting) ? Just live
with it, and possibly document the issue somewhere ?
---
 drivers/net/wireless/microchip/wilc1000/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


---
base-commit: 246a8c611ace197f43ecc6ea4936c6ca363b8aaa
change-id: 20240119-wilc_1000_reset_line-393270fc474e

Best regards,
  

On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
> When using a wilc1000 chip over a spi bus, users can optionally define a
> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
> low, so to hold the chip in reset, a low (physical) value must be applied.
> 
> The corresponding device tree binding documentation was introduced by
> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
> properties") and correctly indicates that the reset line is an active-low
> signal. However, the corresponding driver part, brought by commit
> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
> misusing the gpiod APIs and apply an inverted logic when powering up/down
> the chip (for example, setting the reset line to a logic "1" during power
> up, which in fact asserts the reset line when device tree describes the
> reset line as GPIO_ACTIVE_LOW).

Note that commit ec031ac4792c is doing the right thing in regards to an
ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.

It was later on that commit fcf690b0 flipped the RESET line polarity to treat it
as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as you noted, it
introduced in inconsistency with the binding documentation.

On our platform, we never merged commit fcf690b0 and hence our DTS already
defines the RESET pin as GPIO_ACTIVE_LOW.  So, I don't have any issues at all
with your patch! :-)

  --david
                                                                              gi
  

On 2/13/24 17:42, David Mosberger-Tang wrote:
> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
>> When using a wilc1000 chip over a spi bus, users can optionally define a
>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
>> low, so to hold the chip in reset, a low (physical) value must be applied.
>>
>> The corresponding device tree binding documentation was introduced by
>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
>> properties") and correctly indicates that the reset line is an active-low
>> signal. However, the corresponding driver part, brought by commit
>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
>> misusing the gpiod APIs and apply an inverted logic when powering up/down
>> the chip (for example, setting the reset line to a logic "1" during power
>> up, which in fact asserts the reset line when device tree describes the
>> reset line as GPIO_ACTIVE_LOW).
> 
> Note that commit ec031ac4792c is doing the right thing in regards to an
> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.
> 
> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it
> as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as you noted, it
> introduced in inconsistency with the binding documentation.

Ah, you are right, and I was wrong citing your GPIOs patch as faulty
(git-blaming too fast !), thanks for the clarification. I missed this patch from
Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing
proper device tree configuration and then submitted this flip ?

> On our platform, we never merged commit fcf690b0 and hence our DTS already
> defines the RESET pin as GPIO_ACTIVE_LOW.  So, I don't have any issues at all
> with your patch! :-)

So in the end, the patch should be about a mere revert. I will update
accordingly when relevant, but before that I'll wait for some feedback about the
potential issue of this patch (forcing users to udpate faulty devicetree)

Thanks,
Alexis
  

Hi,

On 2/13/24 09:58, Alexis Lothoré wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 2/13/24 17:42, David Mosberger-Tang wrote:
>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
>>> When using a wilc1000 chip over a spi bus, users can optionally define a
>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
>>> low, so to hold the chip in reset, a low (physical) value must be applied.
>>>
>>> The corresponding device tree binding documentation was introduced by
>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
>>> properties") and correctly indicates that the reset line is an active-low
>>> signal. However, the corresponding driver part, brought by commit
>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
>>> misusing the gpiod APIs and apply an inverted logic when powering up/down
>>> the chip (for example, setting the reset line to a logic "1" during power
>>> up, which in fact asserts the reset line when device tree describes the
>>> reset line as GPIO_ACTIVE_LOW).
>>
>> Note that commit ec031ac4792c is doing the right thing in regards to an
>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.
>>
>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it
>> as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as you noted, it
>> introduced in inconsistency with the binding documentation.
> 
> Ah, you are right, and I was wrong citing your GPIOs patch as faulty
> (git-blaming too fast !), thanks for the clarification. I missed this patch from
> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing
> proper device tree configuration and then submitted this flip ?

Indeed, it was done to align the code as per the DT entry suggested in
WILC1000/3000 porting guide[1 -page 18], which is already used by most
of the existing users. This change has impact on the users who are using
DT entry from porting guide. One approach is to retain the current code
and document this if needed.

1.
https://ww1.microchip.com/downloads/en/DeviceDoc/ATWILC1000-ATWILC3000-ATWILC-Devices-Linux-Porting-Guide-User-Guide-DS70005329C.pdf


Regards,
Ajay
  

(Adding devicetree list for comments)

<Ajay.Kathat@microchip.com> writes:

> On 2/13/24 09:58, Alexis Lothoré wrote:
>> 
>> On 2/13/24 17:42, David Mosberger-Tang wrote:
>>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
>>>> When using a wilc1000 chip over a spi bus, users can optionally define a
>>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
>>>> low, so to hold the chip in reset, a low (physical) value must be applied.
>>>>
>>>> The corresponding device tree binding documentation was introduced by
>>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
>>>> properties") and correctly indicates that the reset line is an active-low
>>>> signal. However, the corresponding driver part, brought by commit
>>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
>>>> misusing the gpiod APIs and apply an inverted logic when powering up/down
>>>> the chip (for example, setting the reset line to a logic "1" during power
>>>> up, which in fact asserts the reset line when device tree describes the
>>>> reset line as GPIO_ACTIVE_LOW).
>>>
>>> Note that commit ec031ac4792c is doing the right thing in regards to an
>>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.
>>>
>>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it
>>> as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as you noted, it
>>> introduced in inconsistency with the binding documentation.
>> 
>> Ah, you are right, and I was wrong citing your GPIOs patch as faulty
>> (git-blaming too fast !), thanks for the clarification. I missed this patch from
>> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing
>> proper device tree configuration and then submitted this flip ?
>
> Indeed, it was done to align the code as per the DT entry suggested in
> WILC1000/3000 porting guide[1 -page 18], which is already used by most
> of the existing users. This change has impact on the users who are using
> DT entry from porting guide. One approach is to retain the current code
> and document this if needed.

So if I'm understanding the situation correctly Microchip's porting
guide[1] doesn't match with kernel.org documentation[2]? I'm not the
expert here but from my point of view the issue is clear: the code needs
to follow kernel.org documentation[2], not external documentation.

I'll add devicetree list so hopefully people there can comment also,
full patch available in [3].

Alexis, if there are no more comments I'm in favor submitting the revert
you mentioned.


[1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATWILC1000-ATWILC3000-ATWILC-Devices-Linux-Porting-Guide-User-Guide-DS70005329C.pdf

[2] Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml

[3] https://patchwork.kernel.org/project/linux-wireless/patch/20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com/
  

On Fri, Feb 16, 2024 at 06:01:52PM +0200, Kalle Valo wrote:
> (Adding devicetree list for comments)
> 
> <Ajay.Kathat@microchip.com> writes:
> 
> > On 2/13/24 09:58, Alexis Lothoré wrote:
> >> 
> >> On 2/13/24 17:42, David Mosberger-Tang wrote:
> >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
> >>>> When using a wilc1000 chip over a spi bus, users can optionally define a
> >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
> >>>> low, so to hold the chip in reset, a low (physical) value must be applied.
> >>>>
> >>>> The corresponding device tree binding documentation was introduced by
> >>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
> >>>> properties") and correctly indicates that the reset line is an active-low
> >>>> signal. However, the corresponding driver part, brought by commit
> >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
> >>>> misusing the gpiod APIs and apply an inverted logic when powering up/down
> >>>> the chip (for example, setting the reset line to a logic "1" during power
> >>>> up, which in fact asserts the reset line when device tree describes the
> >>>> reset line as GPIO_ACTIVE_LOW).
> >>>
> >>> Note that commit ec031ac4792c is doing the right thing in regards to an
> >>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.
> >>>
> >>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it
> >>> as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as you noted, it
> >>> introduced in inconsistency with the binding documentation.
> >> 
> >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty
> >> (git-blaming too fast !), thanks for the clarification. I missed this patch from
> >> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing
> >> proper device tree configuration and then submitted this flip ?
> >
> > Indeed, it was done to align the code as per the DT entry suggested in
> > WILC1000/3000 porting guide[1 -page 18], which is already used by most
> > of the existing users. This change has impact on the users who are using
> > DT entry from porting guide. One approach is to retain the current code
> > and document this if needed.
> 
> So if I'm understanding the situation correctly Microchip's porting
> guide[1] doesn't match with kernel.org documentation[2]? I'm not the
> expert here but from my point of view the issue is clear: the code needs
> to follow kernel.org documentation[2], not external documentation.

My point of view would definitely be that drivers in the mainline kernel
absolutely should respect the ABI defined in the dt-binding. What a vendor
decides to do in their own tree I suppose is their problem, but I would
advocate that vendor kernels would also respect the ABI from mainline.

Looking a bit more closely at the porting guide, it contains other
properties that are not present in the dt-binding - undocumented
compatibles and a different enable gpio property for example.
I guess it (and the vendor version of the driver) never got updated when
wilc1000 supported landed in mainline?

> I'll add devicetree list so hopefully people there can comment also,
> full patch available in [3].
> 
> Alexis, if there are no more comments I'm in favor submitting the revert
> you mentioned.

From a dt-bindings point of view, the aforementioned revert seems
correct and would be
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Getting off my dt-binding maintainer high-horse, linux4microchip is going
be updating to a 6.6 based kernel in the coming weeks - maybe that's a
good time to update the vendor kernel wilc drivers (and therefore the
porting guide?) to match the properties used by mainline Ajay?

Cheers,
Conor.
  

On Fri, Feb 16, 2024 at 04:54:29PM +0000, Conor Dooley wrote:
> On Fri, Feb 16, 2024 at 06:01:52PM +0200, Kalle Valo wrote:
> > (Adding devicetree list for comments)
> > 
> > <Ajay.Kathat@microchip.com> writes:
> > 
> > > On 2/13/24 09:58, Alexis Lothoré wrote:
> > >> 
> > >> On 2/13/24 17:42, David Mosberger-Tang wrote:
> > >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
> > >>>> When using a wilc1000 chip over a spi bus, users can optionally define a
> > >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
> > >>>> low, so to hold the chip in reset, a low (physical) value must be applied.
> > >>>>
> > >>>> The corresponding device tree binding documentation was introduced by
> > >>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
> > >>>> properties") and correctly indicates that the reset line is an active-low
> > >>>> signal. However, the corresponding driver part, brought by commit
> > >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
> > >>>> misusing the gpiod APIs and apply an inverted logic when powering up/down
> > >>>> the chip (for example, setting the reset line to a logic "1" during power
> > >>>> up, which in fact asserts the reset line when device tree describes the
> > >>>> reset line as GPIO_ACTIVE_LOW).
> > >>>
> > >>> Note that commit ec031ac4792c is doing the right thing in regards to an
> > >>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.
> > >>>
> > >>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it
> > >>> as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as you noted, it
> > >>> introduced in inconsistency with the binding documentation.
> > >> 
> > >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty
> > >> (git-blaming too fast !), thanks for the clarification. I missed this patch from
> > >> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing
> > >> proper device tree configuration and then submitted this flip ?
> > >
> > > Indeed, it was done to align the code as per the DT entry suggested in
> > > WILC1000/3000 porting guide[1 -page 18], which is already used by most
> > > of the existing users. This change has impact on the users who are using
> > > DT entry from porting guide. One approach is to retain the current code
> > > and document this if needed.
> > 
> > So if I'm understanding the situation correctly Microchip's porting
> > guide[1] doesn't match with kernel.org documentation[2]? I'm not the
> > expert here but from my point of view the issue is clear: the code needs
> > to follow kernel.org documentation[2], not external documentation.
> 
> My point of view would definitely be that drivers in the mainline kernel
> absolutely should respect the ABI defined in the dt-binding. What a vendor
> decides to do in their own tree I suppose is their problem, but I would
> advocate that vendor kernels would also respect the ABI from mainline.
> 
> Looking a bit more closely at the porting guide, it contains other
> properties that are not present in the dt-binding - undocumented
> compatibles and a different enable gpio property for example.
> I guess it (and the vendor version of the driver) never got updated when
> wilc1000 supported landed in mainline?
> 
> > I'll add devicetree list so hopefully people there can comment also,
> > full patch available in [3].
> > 
> > Alexis, if there are no more comments I'm in favor submitting the revert
> > you mentioned.
> 
> From a dt-bindings point of view, the aforementioned revert seems
> correct and would be
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Maybe an R-b is more suitable here, too used to acking trivial patches
that are dt related..

> 
> Getting off my dt-binding maintainer high-horse, linux4microchip is going
> be updating to a 6.6 based kernel in the coming weeks - maybe that's a
> good time to update the vendor kernel wilc drivers (and therefore the
> porting guide?) to match the properties used by mainline Ajay?
> 
> Cheers,
> Conor.
  

Conor Dooley <conor@kernel.org> writes:

>> > So if I'm understanding the situation correctly Microchip's porting
>> > guide[1] doesn't match with kernel.org documentation[2]? I'm not the
>> > expert here but from my point of view the issue is clear: the code needs
>> > to follow kernel.org documentation[2], not external documentation.
>> 
>> My point of view would definitely be that drivers in the mainline kernel
>> absolutely should respect the ABI defined in the dt-binding. What a vendor
>> decides to do in their own tree I suppose is their problem, but I would
>> advocate that vendor kernels would also respect the ABI from mainline.
>> 
>> Looking a bit more closely at the porting guide, it contains other
>> properties that are not present in the dt-binding - undocumented
>> compatibles and a different enable gpio property for example.
>> I guess it (and the vendor version of the driver) never got updated when
>> wilc1000 supported landed in mainline?
>> 
>> > I'll add devicetree list so hopefully people there can comment also,
>> > full patch available in [3].
>> > 
>> > Alexis, if there are no more comments I'm in favor submitting the revert
>> > you mentioned.
>> 
>> From a dt-bindings point of view, the aforementioned revert seems
>> correct and would be
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> Maybe an R-b is more suitable here, too used to acking trivial patches
> that are dt related..

On the contrary, I think Acked-by is the right thing here and makes it
easier for Alexis and me. Thanks!
  

On 2/16/24 11:07, Kalle Valo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Conor Dooley <conor@kernel.org> writes:
> 
>>>> So if I'm understanding the situation correctly Microchip's porting
>>>> guide[1] doesn't match with kernel.org documentation[2]? I'm not the
>>>> expert here but from my point of view the issue is clear: the code needs
>>>> to follow kernel.org documentation[2], not external documentation.
>>>
>>> My point of view would definitely be that drivers in the mainline kernel
>>> absolutely should respect the ABI defined in the dt-binding. What a vendor
>>> decides to do in their own tree I suppose is their problem, but I would
>>> advocate that vendor kernels would also respect the ABI from mainline.
>>>
>>> Looking a bit more closely at the porting guide, it contains other
>>> properties that are not present in the dt-binding - undocumented
>>> compatibles and a different enable gpio property for example.
>>> I guess it (and the vendor version of the driver) never got updated when
>>> wilc1000 supported landed in mainline?
>>>
>>>> I'll add devicetree list so hopefully people there can comment also,
>>>> full patch available in [3].
>>>>
>>>> Alexis, if there are no more comments I'm in favor submitting the revert
>>>> you mentioned.
>>>
>>> From a dt-bindings point of view, the aforementioned revert seems
>>> correct and would be
>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>
>> Maybe an R-b is more suitable here, too used to acking trivial patches
>> that are dt related..
> 
> On the contrary, I think Acked-by is the right thing here and makes it
> easier for Alexis and me. Thanks!

Acked-by: Ajay Singh <ajay.kathat@microchip.com>

Agree, we can go ahead with this patch to make the code inline with
kernel.org documentation. I don't think any change is required in
dt-binding definition after this patch. However external documentation
update is needed as Conor has also pointed out,  I will be taking care
of it.

Regards,
Ajay