| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y2VVA2Wp1IWoJf3m@smile.fi.intel.com>
Date: Fri, 4 Nov 2022 20:08:03 +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 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.
> { }
> };
...
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
It seems you are using much more that these ones.
...
> + char prop_name[32]; /* 32 is max size of property name */
Why is it not defined then?
...
> + /*
> + * 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.
...
> + /*
> + * 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?
...
> + 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.
...
> + 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.
...
> + /*
> + * 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;
}
Btw, what about reference counting? Do we need to care about it?
> + return count ? count : -ENOENT;
Elvis would work as well.
return count ?: -ENOENT;
...
> +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.
> + */
> + if (is_software_node(fwnode->secondary)) {
With the previous comments it would become
if (fwnode && is_...)
> + 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);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists