Re: [PATCH v4 7/7] gpio: add pinctrl based generic gpio driver

[Andy Shevchenko]

On Tue, Mar 17, 2026 at 05:41:02PM +0300, Dan Carpenter wrote:

> The ARM SCMI pinctrl protocol allows GPIO access.  Instead of creating
> a new SCMI gpio driver, this driver is a generic GPIO driver that uses

GPIO

> standard pinctrl interfaces.

...

> +config GPIO_BY_PINCTRL
> +	tristate "GPIO support based on a pure pin control backend"
> +	depends on GPIOLIB
> +	help
> +	  Select this option to support GPIO devices based solely on pin
> +	  control.  This is used to do GPIO over the ARM SCMI protocol.

This is not enough to understand (as discussion in previous round showed).
Can we have added a Documentation/driver-api/gpio/... (to the existing one
or a new one)?

...

> +// Copyright (C) 2023 Linaro Inc.

2026 (as well)?

...

+ errno.h // -ENOMEM, et cetera

> +#include <linux/gpio/driver.h>

> +#include <linux/list.h>

?! Cargo cult?

+ mod_devicetable.h // of_device_id

> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>

+ blank line.

> +#include "gpiolib.h"

...

> +struct pin_control_gpio_priv {
> +	struct gpio_chip chip;
> +};

Unneeded, you can use struct gpio_chip directly. No?

...

> +static int pin_control_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	unsigned long config;
> +	bool in, out;
> +	int ret;
> +
> +	config = PIN_CONFIG_INPUT_ENABLE;
> +	ret = pinctrl_gpio_get_config(gc, offset, &config);
> +	if (ret)
> +		return ret;
> +	in = config;
> +
> +	config = PIN_CONFIG_OUTPUT_ENABLE;
> +	ret = pinctrl_gpio_get_config(gc, offset, &config);
> +	if (ret)
> +		return ret;
> +	out = config;

> +	/* Consistency check - in theory both can be enabled! */
> +	if (in && !out)
> +		return GPIO_LINE_DIRECTION_IN;
> +	if (!in && out)
> +		return GPIO_LINE_DIRECTION_OUT;
> +
> +	return -EINVAL;

When both are enabled it's out direction, so the entire piece
can be simplified to

	if (out)
		return GPIO_LINE_DIRECTION_OUT;

	if (in)
		return GPIO_LINE_DIRECTION_IN;

	return ...something...; ideally it should be HiZ.

So, even more simplified will be just

	if (out)
		return GPIO_LINE_DIRECTION_OUT;
	else
		return GPIO_LINE_DIRECTION_IN;

> +}

...

> +	ret = devm_gpiochip_add_data(dev, chip, priv);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);

Not used.

> +	return 0;

Hence, the entire piece is just

	return devm_gpiochip_add_data(dev, chip, priv);

-- 
With Best Regards,
Andy Shevchenko