[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y2uNDmRefzPvUu3P@smile.fi.intel.com>
Date: Wed, 9 Nov 2022 13:20:46 +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 v2 6/6] gpiolib: add support for software nodes
On Tue, Nov 08, 2022 at 04:26:51PM -0800, 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)
> - a new PROPERTY_ENTRY_GPIO() macro is supplied to ensure the above
> - name of the software node representing gpiochip must match label of
> the gpiochip, as we use it to locate gpiochip structure at runtime
>
> The following illustrates use of software nodes to describe a "System"
> button that is currently specified via use of gpio_keys_platform_data
> in arch/mips/alchemy/board-mtx1.c. It follows bindings specified in
> Documentation/devicetree/bindings/input/gpio-keys.yaml.
>
> static const struct software_node mxt1_gpiochip2_node = {
> .name = "alchemy-gpio2",
> };
>
> static const struct property_entry mtx1_gpio_button_props[] = {
> PROPERTY_ENTRY_U32("linux,code", BTN_0),
> PROPERTY_ENTRY_STRING("label", "System button"),
> PROPERTY_ENTRY_GPIO("gpios", &mxt1_gpiochip2_node, 7, GPIO_ACTIVE_LOW),
> { }
> };
>
> Similarly, arch/arm/mach-tegra/board-paz00.c can be converted to:
>
> static const struct software_node tegra_gpiochip_node = {
> .name = "tegra-gpio",
> };
>
> static struct property_entry wifi_rfkill_prop[] __initdata = {
> PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
> PROPERTY_ENTRY_STRING("type", "wlan"),
> PROPERTY_ENTRY_GPIO("reset-gpios",
> &tegra_gpiochip_node, 25, GPIO_ACTIVE_HIGH);
> PROPERTY_ENTRY_GPIO("shutdown-gpios",
> &tegra_gpiochip_node, 85, GPIO_ACTIVE_HIGH);
> { },
> };
>
> static struct platform_device wifi_rfkill_device = {
> .name = "rfkill_gpio",
> .id = -1,
> };
>
> ...
>
> software_node_register(&tegra_gpiochip_node);
> device_create_managed_software_node(&wifi_rfkill_device.dev,
> wifi_rfkill_prop, NULL);
...
> +static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
> +{
> + const struct software_node *chip_node;
> + struct gpio_chip *chip;
> +
> + chip_node = to_software_node(fwnode);
> + if (!chip_node || !chip_node->name)
> + return ERR_PTR(-EINVAL);
> + chip = gpiochip_find((void *)chip_node->name,
> + swnode_gpiochip_match_name);
One line?
> + if (!chip)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + return chip;
As below you can use Elvis here as well, up to you.
return chip ?: ERR_PTR(...);
> +}
...
> + desc = gpiochip_get_desc(chip, args.args[0]);
> + *flags = args.args[1]; /* We expect native GPIO flags */
> +
> + pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> + __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));
%pe ?
> + return desc;
...
> + while (fwnode_property_get_reference_args(fwnode, propname, NULL,
> + 0, count, &args) == 0) {
I would move 0 to the previous line.
> + fwnode_handle_put(args.fwnode);
> + count++;
> + }
...
> int gpiod_count(struct device *dev, const char *con_id)
> {
> - const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> - int count = -ENOENT;
> + struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
Why dropping const?
> + int count;
Why this change is needed?
> 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;
...
> +#include <dt-bindings/gpio/gpio.h>
Not sure why we have this here.
> +#include <linux/property.h>
> +
> +#define PROPERTY_ENTRY_GPIO(_name_, _chip_node_, _idx_, _flags_) \
> + PROPERTY_ENTRY_REF(_name_, _chip_node_, _idx_, _flags_)
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists