linux-kernel - Re: [PATCH v6 2/2] gpio: tps65219: Add support for TI TPS65214 PMIC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <780d9307-4c47-4772-b527-bfd94486b931@wanadoo.fr>
Date: Thu, 3 Jul 2025 21:05:40 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Shree Ramamoorthy <s-ramamoorthy@...com>, aaro.koskinen@....fi,
 andreas@...nade.info, khilman@...libre.com, rogerq@...nel.org,
 tony@...mide.com, linus.walleij@...aro.org, brgl@...ev.pl,
 linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org
Cc: m-leonard@...com, praneeth@...com, jcormier@...ticallink.com
Subject: Re: [PATCH v6 2/2] gpio: tps65219: Add support for TI TPS65214 PMIC

Le 03/07/2025 à 20:07, Shree Ramamoorthy a écrit :
> Add support for the TI TPS65214 PMIC with the addition of an id_table,
> separate TPS65214 template_chip, and device-specific _change_direction
> functions.
> 
> - Use platform_get_device_id() to assign dev-specific information.
> - Use different change_direction() functions since TPS65214's GPIO
>    configuration bits are changeable during device operation through bit
>    GPIO_CONFIG in GENERAL_CONFIG register.
> - Remove MODULE_ALIAS since it is now generated by MODULE_DEVICE_TABLE.
> 
> Reviewed-by: Jonathan Cormier <jcormier@...ticallink.com>
> Tested-by: Jonathan Cormier <jcormier@...ticallink.com>
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@...com>
> ---

...

> +static int tps65214_gpio_change_direction(struct gpio_chip *gc, unsigned int offset,
> +					  unsigned int direction)
> +{
> +	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> +	struct device *dev = gpio->tps->dev;
> +	int val, ret;
> +
> +	/* Verified if GPIO or GPO in parent function

Nitpick: should the /* be on a separate line?

> +	 * Masked value: 0 = GPIO, 1 = VSEL
> +	 */
> +	ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = !!(val & BIT(TPS65219_GPIO0_DIR_MASK));
> +	if (ret)
> +		dev_err(dev, "GPIO%d configured as VSEL, not GPIO\n", offset);
> +
> +	ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_GENERAL_CONFIG,
> +				 TPS65214_GPIO0_DIR_MASK, direction);
> +	if (ret)
> +		dev_err(dev,
> +			"Fail to change direction to %u for GPIO%d.\n",

Nitpick: keep it on the previous line?

> +			direction, offset);
> +
> +	return ret;
> +}

...

> +static const struct gpio_chip tps65214_template_chip = {
> +	.label			= "tps65214-gpio",
> +	.owner			= THIS_MODULE,
> +	.get_direction		= tps65214_gpio_get_direction,
> +	.direction_input	= tps65219_gpio_direction_input,
> +	.direction_output	= tps65219_gpio_direction_output,
> +	.get			= tps65219_gpio_get,
> +	.set_rv			= tps65219_gpio_set,
> +	.base			= -1,
> +	.ngpio			= 2,
> +	.can_sleep		= true,
> +};
> +
>   static const struct gpio_chip tps65219_template_chip = {
>   	.label			= "tps65219-gpio",
>   	.owner			= THIS_MODULE,
> @@ -154,7 +218,7 @@ static const struct gpio_chip tps65219_template_chip = {
>   	.direction_input	= tps65219_gpio_direction_input,
>   	.direction_output	= tps65219_gpio_direction_output,
>   	.get			= tps65219_gpio_get,
> -	.set			= tps65219_gpio_set,
> +	.set_rv			= tps65219_gpio_set,

Is this correct? Does it even compile?
tps65219_gpio_set() returns void and .set_rv() expects a return value.

(same for tps65214_template_chip above)

>   	.base			= -1,
>   	.ngpio			= 3,
>   	.can_sleep		= true,

...

CJ