Re: [PATCH 6/6] gpiolib: add support for software nodes

[Dmitry Torokhov]

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.

> 
> > > > 	{ }
> > > > };
> 
> ...
> 
> > > > +	/*
> > > > +	 * 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?

> 
> ...
> 
> > > > +		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
> > > > +			__func__, prop_name, fwnode, idx);
> > > 
> > > __func__ is not needed. Dynamic Debug can automatically add it.
> > > Since you have an fwnode, use that as a marker.
> > 
> > I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can
> > guess the function from other log messages we emit, but does it hurt
> > having it?
> 
> I think it's redundant. You can modify message itself to improve its
> uniqueness.

¯\_(ツ)_/¯ I think we are moving into extreme bikeshedding direction
here.

> 
> ...
> 
> > > > +	/*
> > > > +	 * This is not very efficient, but GPIO lists usually have only
> > > > +	 * 1 or 2 entries.
> > > > +	 */
> > > > +	count = 0;
> > > > +	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
> > > > +						  0, count, &args) == 0)
> > > 
> > > I would put it into for loop (and looking into property.h I think propname
> > > is fine variable name):
> > > 
> > > 	for (count = 0; ; count++) {
> > > 		if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args))
> > > 			break;
> > > 	}
> > 
> > OK on name, but I like explicit counting with the "while" loop as it
> > shows the purpose of the code.
> 
> OK, let's see how it will look like with the proper dropped reference.
> 
> > > Btw, what about reference counting? Do we need to care about it?
> > 
> > Yes, indeed, we need to drop the reference, thank you for noticing!
> 
> ...
> 
> > > > +	/*
> > > > +	 * 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 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.

Thanks.

-- 
Dmitry


diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 22a91db645b8f..5fe51c5baa6bb 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -30,6 +30,17 @@
 #include <linux/spinlock.h>
 #include <dt-bindings/input/gpio-keys.h>
 
+#include <linux/property.h>
+#include <linux/gpio/machine.h>
+const struct software_node gpio_bank_node = {
+	.name = "pinctrl_paris",
+};
+
+const struct property_entry pen_insert_props[] = {
+	PROPERTY_ENTRY_REF("key-gpios", &gpio_bank_node, 18, GPIO_ACTIVE_LOW),
+	{ }
+};
+
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
@@ -519,8 +530,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	spin_lock_init(&bdata->lock);
 
 	if (child) {
+		if (!strcmp(fwnode_get_name(child), "pen-insert"))
+			child->secondary = fwnode_create_software_node(pen_insert_props, NULL);
+
 		bdata->gpiod = devm_fwnode_gpiod_get(dev, child,
-						     NULL, GPIOD_IN, desc);
+						     "key", GPIOD_IN, desc);
 		if (IS_ERR(bdata->gpiod)) {
 			error = PTR_ERR(bdata->gpiod);
 			if (error == -ENOENT) {
@@ -1056,14 +1070,18 @@ static struct platform_driver gpio_keys_device_driver = {
 	}
 };
 
+
+
 static int __init gpio_keys_init(void)
 {
+	software_node_register(&gpio_bank_node);
 	return platform_driver_register(&gpio_keys_device_driver);
 }
 
 static void __exit gpio_keys_exit(void)
 {
 	platform_driver_unregister(&gpio_keys_device_driver);
+	software_node_unregister(&gpio_bank_node);
 }
 
 late_initcall(gpio_keys_init);