Message-ID: <Y2jnGVKDmGvK94AV@smile.fi.intel.com>
Date:   Mon, 7 Nov 2022 13:08:09 +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 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:

...

> > > > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > > > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > > > 
> > > > > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > > > > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > > > 
> > > > Okay, can we have an example for something like reset-gpios? Because from
> > > > the above I can't easily get what label is and how in the `gpioinfo` tool
> > > > the requested line will look like.
> > > 
> > > The label is something unrelated to gpio. The example was supposed to
> > > match gpio-keys binding found in
> > > Documentation/devicetree/bindings/input/gpio-keys.yaml
> > 
> > Yes, but what would be output of `gpioinfo` for the above  example and
> > if GPIO is named properly (with con_id)?
> 
> Same as if I am using device tree, or ACPI, etc. I am not changing how
> labeling is done, so whatever rules were before adding swnode support
> they will be used with swnodes.
> 
> With the hack patch to gpio-keys.c below and device using the following
> DT fragment I see the following from gpioinfo:
> 
>         gpio_keys: gpio-keys {
>                 status = "okay";
> 
>                 compatible = "gpio-keys";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pen_eject>;
> 
>                 pen_insert: pen-insert {
>                         label = "Pen Insert";
>                         /* Insert = low, eject = high */
>                         /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */
>                         linux,code = <SW_PEN_INSERTED>;
>                         linux,input-type = <EV_SW>;
>                         wakeup-event-action = <EV_ACT_DEASSERTED>;
>                         wakeup-source;
>                 };
>         };
> 
> Just "gpios" (con_id == NULL):
> 
>         line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]
> 
> With "key-gpios" (con_id == "key") it is exactly the same:
> 
>         line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]
> 
> Ah, I guess you wonder how it will look like if we do not pass this
> "label" into devm_fwnode_gpiod_get() and instead use NULL?
> 
> 	line  18: "PEN_EJECT_OD" "?" input active-low [used]
> 
> If the driver used gpiod_get() or similar it would either have the
> "con_id" label or device name (produced with dev_name(dev) if con_id is
> NULL. Still, not changes from using swnodes compared to ACPI or DT.

Yes, can you add a summary of above to the commit message?

> > > > > 	{ }
> > > > > };

...

> > > > > +	/*
> > > > > +	 * 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.

...

> > > > > +	/*
> > > > > +	 * 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?

> 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.

Doing it in a hackish way by allowing priority inversion in _some_ APIs
is no go in my opinion.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists