Message-ID: <Y2kucGtw/t9v0245@google.com>
Date:   Mon, 7 Nov 2022 08:12:32 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.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 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:
> 
> ...
> 
> > > > > > 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.

OK, I guess include/linux/gpio/property.h ?

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

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

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists