Date:   Mon, 7 Nov 2022 22:59:39 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        linux-acpi@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] gpiolib: add support for software nodes

On Mon, Nov 07, 2022 at 08:12:32AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 07, 2022 at 01:08:09PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote:
> > > On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> > > > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:

...

> > > > > > > +	/*
> > > > > > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > > > > > +	 * polarity arguments, hence nargs is set to 2.
> > > > > > > +	 */
> > > > > > 
> > > > > > Maybe instead you can provide a custom macro wrapper that will check the number
> > > > > > of arguments at compile time?
> > > > > 
> > > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> > > > > that enforces needed arguments.
> > > > 
> > > > Yes, that's what I meant.
> > > 
> > > Where do you think it should go? Not sure if I want to pollute
> > > property.h, I guess linux/gpio/matchine.h will need to include
> > > property.h?
> > 
> > Good question. I was thinking more of a separate header for that,
> > because adding property.h adds tons of stuff which might be not
> > needed otherwise.
> 
> OK, I guess include/linux/gpio/property.h ?

Why not?

...

> > > > > > > +	/*
> > > > > > > +	 * First look up GPIO in the secondary software node in case
> > > > > > > +	 * it was used to store updated properties.
> > > > > > 
> > > > > > Why this is done first? We don't try secondary before we have checked primary.
> > > > > 
> > > > > I believe we should check secondary first, so that secondaries can be
> > > > > used not only to add missing properties, but also to override existing
> > > > > ones in case they are incorrect.
> > > > 
> > > > It contradicts all code we have in the kernel regarding the use of software
> > > > nodes, you need very strong argument to justify that.
> > > > 
> > > > Personally I think this must be fixed.
> > > 
> > > I agree, the rest of the code should be fixed ;) I'll put it on my TODO
> > > list.
> > 
> > I'm not sure what "rest of the code" you are referring to. The core part of
> > device property APIs?
> 
> Yes.

> > > I gave my argument above already: swnodes should not only be useful to
> > > add missing properties, but also allow fixing up existing ones. If I
> > > implemented what you are suggesting then I would not be able to create
> > > this concise example and would need to model entire DT node for GPIO
> > > keys.
> > 
> > Why do you need that in the first place? We should not use swnodes as primary
> > source of the information. The auxiliary one is fine. "Fixing" via priority
> > inversion in current model is not good thing to have.
> > 
> > If you really need that you have to first do the following:
> > - convert fwnode to be a list node
> > - unembed it from struct device (leaving only head of list there
> > - update all necessary APIs respectively
> > 
> > In such implementation list_add() / list_add_tail() will define a priority
> > and you may have stack of properties.
> 
> Hmm, that will complicate things quite a bit. I wonder why you think
> that using swnodes to fix up the "bad" firmware data is not desirable.

I put there "in current model". It means that for secondary-primary and only
them that design doesn't allow to create quirks.

> Swnodes are controlled by the kernel and thus we can potentially allow
> users tweak them from usersoace. There is a desire to allow easier
> access to various driver's parameters - see for example Hans patches to
> Goodix and Silead where he adds code that intercepts reading of device
> properties and instead gets data form module parameter - I would like to
> have such facility in more general way.
> 
> https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/

How can you guarantee that flip-flopping priority of reading properties doesn't
break things?

Moreover, what problem we are trying to hack up? The DT should be fixed in DT.
ACPI? In ACPI properties are not that common, and even that, we shouldn't unleash
vendors to make all possible abuse-like mistakes in ACPI, that's why I do not think
that allowing property quirks is a good idea at all.

> > Doing it in a hackish way by allowing priority inversion in _some_ APIs
> > is no go in my opinion.
> 
> Yes, I agree that we want to have all APIs behave in the similar way. It
> occurred to me that the topic of handling secondary node is actually
> separate from swnode hanlding, so I will remove it from this patch and I
> can start a separate thread/patches for it after we land this series.

As long as the priority is out of the picture I'm fine with this series,

Because as I said the current design seems (I wasn't the author, so I
might be mistaken) had been made with "we trust the firmware" in mind.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists