Re: [PATCH] gpiolib: of: Use correct fwnode for DT-probed chips

[Thierry Reding]

On Wed, Nov 16, 2022 at 12:38:24PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 16, 2022 at 11:26:34AM +0100, Thierry Reding wrote:
> > On Tue, Nov 15, 2022 at 12:18:00PM +0100, Marijn Suijten wrote:
> > > On 2022-11-14 16:15:25, Brian Masney wrote:
> > > > On Fri, Nov 11, 2022 at 12:37:32PM +0100, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding@xxxxxxxxxx>
> > > > > 
> > > > > The OF node store in chip->fwnode is used to explicitly override the FW
> > > > > node for a GPIO chip. For chips that use the default FW node (i.e. that
> > > > > of their parent device), this will be NULL and cause the chip not to be
> > > > > fully registered.
> > > > > 
> > > > > Instead, use the GPIO device's FW node, which is set to either the node
> > > > > of the parent device or the explicit override in chip->fwnode.
> > > > > 
> > > > > Fixes: 8afe82550240 ("gpiolib: of: Prepare of_gpiochip_add() / of_gpiochip_remove() for fwnode")
> > > > > Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > > > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > > > 
> > > > Reviewed-by: Brian Masney <bmasney@xxxxxxxxxx>
> > > > Tested-by: Brian Masney <bmasney@xxxxxxxxxx>
> > > > 
> > > > I separately sent a similar type of patch to fix the same issue today:
> > > > https://lore.kernel.org/linux-arm-msm/20221114202943.2389489-1-bmasney@xxxxxxxxxx/T/#u
> > > 
> > > For completeness, your linked patch fixes a synchronous external abort
> > > on multiple Qualcomm platforms pointed out in [1].  This patch however
> > > does not, are you sure they fix the exact same issue?
> 
> Yes, they fix the same issue.
> 
> > > [1]: https://lore.kernel.org/linux-arm-msm/20221115110800.35gl3j43lmbxm3jb@xxxxxxxxxxxxxx/
> > 
> > Can you check if the below fixes the MSM issue that you're seeing
> > (applied on top of my earlier patch, though with Brian's reverted
> > temporarily)?
> 
> I don't know why we would need this. Brian's patch (already applied into
> GPIO tree) is correct, no? (Moreover, it makes yours unneeded, but I'm fine
> with having it anyway.)

I've explained this in the reply to Brian's patch, but I don't think we
want to use gc->fwnode other than initially to override the fwnode that
the GPIO chip uses. It might even be worth turning it into a parameter
to gpiochip_add() to avoid the ambiguity we have right now where we
store the same fwnode in two different places and end up using either
depending on whoever wrote the patch and what mood they were in. There
really should only be one place to store this pointer to avoid this sort
of ambiguity.

Thierry

Attachment: signature.asc
Description: PGP signature