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:
> > Now that static device properties understand notion of child nodes and
> > references, let's teach gpiolib to handle them:
> >
> > - GPIOs are represented as a references to software nodes representing
> > gpiochip
> > - references must have 2 arguments - GPIO number within the chip and
> > GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc).
> > - name of the software node representing gpiochip must match label of
> > the gpiochip, as we use it to locate gpiochip structure at runtime.
> >
> > const struct software_node gpio_bank_b_node = {
> > .name = "B",
> > };
> >
> > 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
>
> > { }
> > };
>
> ...
>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
>
> It seems you are using much more that these ones.
Yeah, you are right.
>
> ...
>
> > + char prop_name[32]; /* 32 is max size of property name */
>
> Why is it not defined then?
Not sure. 32 is the limit used throughout gpiolib (see the main
gpiolib.c, gpiolib-acpi.c and gpiolib-of.c). We could add a private
gpiolib define. Or we could dynamically allocate strings if we belive
this is an issue.
I'd like to do it as a followup if we decide this needs changing.
>
> ...
>
> > + /*
> > + * Note we do not need to try both -gpios and -gpio suffixes,
> > + * as, unlike OF and ACPI, we can fix software nodes to conform
> > + * to the proper binding.
> > + */
>
> True, but for the sake of consistency between providers perhaps it makes sense
> to check that as well. Dunno, up to Linus and Bart to decide.
I hear you, however we had to have this fallback for OF and ACPI because
of concerns of separate DT/firmware and keeping compatibility. Here we
do not have this problem, so I think we should require properly named
properties.
>
> ...
>
> > + /*
> > + * 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.
>
> ...
>
> > + 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?
>
> ...
>
> > + chip = gpiochip_find((void *)chip_node->name,
> > + swnode_gpiochip_match_name);
>
> One line?
>
> ...
>
> > + pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > + __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc));
>
> Same as above.
>
> ...
>
> > + char prop_name[32];
>
> > + if (con_id)
> > + snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id);
> > + else
> > + strscpy(prop_name, "gpios", sizeof(prop_name));
>
> I saw this code, please deduplicate.
OK.
>
> ...
>
> > + /*
> > + * 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.
>
> Btw, what about reference counting? Do we need to care about it?
Yes, indeed, we need to drop the reference, thank you for noticing!
>
> > + return count ? count : -ENOENT;
>
> Elvis would work as well.
>
> return count ?: -ENOENT;
OK, I like Elvis.
>
>
> ...
>
> > +struct fwnode_handle;
>
> struct gpio_desc;
>
> > +
> > +struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> > + const char *con_id, unsigned int idx,
> > + unsigned long *flags);
> > +int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id);
>
> ...
>
> > + /*
> > + * 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.
>
> > + */
>
> > + if (is_software_node(fwnode->secondary)) {
>
> With the previous comments it would become
>
> if (fwnode && is_...)
Right, I prefer if we could trust that fwnode passed here is not NULL.
>
> > + dev_dbg(consumer,
> > + "using secondary software node for GPIO lookup\n");
> > + desc = swnode_find_gpio(fwnode->secondary,
> > + con_id, idx, lookupflags);
> > + if (!gpiod_not_found(desc))
> > + return desc;
> > + }
>
> ...
>
> > int gpiod_count(struct device *dev, const char *con_id)
> > {
> > + struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> > + int count;
> > +
> > + /*
> > + * First look up GPIO in the secondary software node in case
> > + * it was used to store updated properties.
> > + */
>
> Same question as above.
>
> > + if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) {
> > + count = swnode_gpio_count(fwnode->secondary, con_id);
> > + if (count > 0)
> > + return count;
> > + }
> >
> > if (is_of_node(fwnode))
> > count = of_gpio_get_count(dev, con_id);
> > else if (is_acpi_node(fwnode))
> > count = acpi_gpio_count(dev, con_id);
> > + else if (is_software_node(fwnode))
> > + count = swnode_gpio_count(fwnode, con_id);
> > + else
> > + count = -ENOENT;
> >
> > if (count < 0)
> > count = platform_gpio_count(dev, con_id);
>
Thanks for the review!
--
Dmitry
