Re: [PATCH 2/6] gpio: move hogs into GPIO core

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 05, 2026 at 10:51:27AM +0100, Bartosz Golaszewski wrote:
> Refactor line hogging code by moving the parts duplicated in
> gpiolib-acpi-core.c and gpiolib-of.c into gpiolib.c, leaving just the
> OF-specific bits in the latter.
> 
> This makes fwnode the primary API for setting up hogs and allows to use
> software nodes in addition to ACPI and OF nodes.

...

> +int gpiochip_add_hog(struct gpio_chip *gc, struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *gc_node = dev_fwnode(&gc->gpiodev->dev);
> +	struct of_phandle_args gpiospec;
> +	enum gpiod_flags dflags;
> +	struct gpio_desc *desc;
> +	unsigned long lflags;
> +	const char *name;
> +	int ret, argc;
> +	u32 gpios[3]; /* We support up to three-cell bindings. */
> +	u32 cells;
> +
> +	lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> +	dflags = GPIOD_ASIS;
> +	name = NULL;
> +
> +	argc = fwnode_property_count_u32(fwnode, "gpios");
> +	if (argc < 0)
> +		return ret;

As LKP noticed this perhaps needs to be changed to

		return argc;

> +	if (argc > 3)
> +		return -EINVAL;
> +
> +	ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios, argc);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (is_of_node(fwnode)) {
> +		/*
> +		 * OF-nodes need some additional special handling for
> +		 * translating of devicetree flags.
> +		 */
> +		ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
> +		if (ret)
> +			return ret;

> +		if (cells && argc != cells)
> +			return -EINVAL;

Hmm... So, when cells is 0 we don't care about argc not being 0?

> +		memset(&gpiospec, 0, sizeof(gpiospec));
> +		gpiospec.np = to_of_node(fwnode);
> +		gpiospec.args_count = argc;
> +		memcpy(&gpiospec.args, gpios, argc * sizeof(u32));
> +
> +		ret = of_gpiochip_get_lflags(gc, &gpiospec, &lflags);

I prefer to see less OF things here, id est we may use struct
fwnode_reference_args.

> +		if (ret)
> +			return ret;
> +	} else {
> +		/*
> +		 * GPIO_ACTIVE_LOW is currently the only lookup flag
> +		 * supported for non-OF firmware nodes.
> +		 */
> +		if (gpios[1])
> +			lflags |= GPIO_ACTIVE_LOW;
> +	}
> +
> +	if (fwnode_property_present(fwnode, "input"))
> +		dflags |= GPIOD_IN;
> +	else if (fwnode_property_present(fwnode, "output-low"))
> +		dflags |= GPIOD_OUT_LOW;
> +	else if (fwnode_property_present(fwnode, "output-high"))
> +		dflags |= GPIOD_OUT_HIGH;
> +	else
> +		return -EINVAL;
> +
> +	fwnode_property_read_string(fwnode, "line-name", &name);
> +
> +	desc = gpiochip_get_desc(gc, gpios[0]);
> +	if (IS_ERR(desc))
> +		return PTR_ERR(desc);

> +	ret = gpiod_hog(desc, name, lflags, dflags);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Can be

	return gpiod_hog(desc, name, lflags, dflags);

> +}

-- 
With Best Regards,
Andy Shevchenko