* Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity [not found] ` <aac398e4-d870-4ba2-8877-b98afecb8d1b@microchip.com> @ 2024-02-16 16:01 ` Kalle Valo 2024-02-16 16:54 ` Conor Dooley 0 siblings, 1 reply; 5+ messages in thread From: Kalle Valo @ 2024-02-16 16:01 UTC (permalink / raw) To: Ajay.Kathat Cc: alexis.lothore, davidm, linux-wireless, claudiu.beznea, thomas.petazzoni, linux-kernel, devicetree (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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity 2024-02-16 16:01 ` [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity Kalle Valo @ 2024-02-16 16:54 ` Conor Dooley 2024-02-16 16:55 ` Conor Dooley 0 siblings, 1 reply; 5+ messages in thread From: Conor Dooley @ 2024-02-16 16:54 UTC (permalink / raw) To: Kalle Valo Cc: Ajay.Kathat, alexis.lothore, davidm, linux-wireless, claudiu.beznea, thomas.petazzoni, linux-kernel, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity 2024-02-16 16:54 ` Conor Dooley @ 2024-02-16 16:55 ` Conor Dooley 2024-02-16 18:07 ` Kalle Valo 0 siblings, 1 reply; 5+ messages in thread From: Conor Dooley @ 2024-02-16 16:55 UTC (permalink / raw) To: Kalle Valo Cc: Ajay.Kathat, alexis.lothore, davidm, linux-wireless, claudiu.beznea, thomas.petazzoni, linux-kernel, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity 2024-02-16 16:55 ` Conor Dooley @ 2024-02-16 18:07 ` Kalle Valo 2024-02-17 0:10 ` Ajay.Kathat 0 siblings, 1 reply; 5+ messages in thread From: Kalle Valo @ 2024-02-16 18:07 UTC (permalink / raw) To: Conor Dooley Cc: Ajay.Kathat, alexis.lothore, davidm, linux-wireless, claudiu.beznea, thomas.petazzoni, linux-kernel, devicetree 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity 2024-02-16 18:07 ` Kalle Valo @ 2024-02-17 0:10 ` Ajay.Kathat 0 siblings, 0 replies; 5+ messages in thread From: Ajay.Kathat @ 2024-02-17 0:10 UTC (permalink / raw) To: kvalo, conor Cc: alexis.lothore, davidm, linux-wireless, claudiu.beznea, thomas.petazzoni, linux-kernel, devicetree 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-17 0:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com>
[not found] ` <2ff1c701f3443e1c612a81f4077b0280850f57c6.camel@egauge.net>
[not found] ` <081bce96-f485-414c-8051-e1c14271f8cc@bootlin.com>
[not found] ` <aac398e4-d870-4ba2-8877-b98afecb8d1b@microchip.com>
2024-02-16 16:01 ` [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity Kalle Valo
2024-02-16 16:54 ` Conor Dooley
2024-02-16 16:55 ` Conor Dooley
2024-02-16 18:07 ` Kalle Valo
2024-02-17 0:10 ` Ajay.Kathat
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).