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

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

[Kalle Valo]

(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/

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3893 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4207 bytes --]

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.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Kalle Valo]

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!

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Ajay.Kathat]

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