Re: [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC

[andy . shevchenko]

Tue, May 30, 2023 at 09:59:59AM +0200, Jerome Neanne kirjoitti:

First of all, I have a bit of déjà vu that I have given already some comments
that left neither answered nor addressed.

> Add support for TPS65219 PMICs GPIO interface.
> 
> 3 GPIO pins:
> - GPIO0 only is IO but input mode reserved for MULTI_DEVICE_ENABLE usage
> - GPIO1 and GPIO2 are Output only and referred as GPO1 and GPO2 in spec
> 
> GPIO0 is statically configured as input or output prior to Linux boot.
> it is used for MULTI_DEVICE_ENABLE function.
> This setting is statically configured by NVM.
> GPIO0 can't be used as a generic GPIO (specification Table 8-34).
> It's either a GPO when MULTI_DEVICE_EN=0 or a GPI when MULTI_DEVICE_EN=1.
> 
> Datasheet describes specific usage for non standard GPIO.
> Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf

Can you convert this to be a Datasheet tag? Currently even Link is *not* a tag
because there must be no blank lines in the tag block.

> Co-developed-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jerome Neanne <jneanne@xxxxxxxxxxxx>

...

> +	help
> +	  Select this option to enable GPIO driver for the TPS65219 chip family.
> +	  GPIO0 is statically configured as input or output prior to Linux boot.
> +	  It is used for MULTI_DEVICE_ENABLE function.
> +	  This setting is statically configured by NVM.
> +	  GPIO0 can't be used as a generic GPIO.
> +	  It's either a GPO when MULTI_DEVICE_EN=0 or a GPI when MULTI_DEVICE_EN=1.
> +
> +	  This driver can also be built as a module.
> +	  If so, the module will be called gpio_tps65219.

Random indentation. Can you use as much room as available on each line, please?

> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO driver for TI TPS65219 PMICs
> + *
> + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/tps65219.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

...

> +static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> +	struct device *dev = gpio->tps->dev;
> +	int ret, val;
> +
> +	if (offset != TPS65219_GPIO0_IDX) {
> +		dev_err(dev, "GPIO%d is output only, cannot get\n", offset);

> +		return -EOPNOTSUPP;

This seems blind following the checkpatch false warning. The checkpatch does
not know about subsystem details, i.e. GPIOLIB uses ENOTSUPP in the callbacks.
The userspace won't see that as GPIOLIB takes care of translating it when
needed.

> +	}
> +
> +	ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK));
> +	dev_warn(dev, "GPIO%d = %d, MULTI_DEVICE_ENABLE, not a standard GPIO\n", offset, ret);
> +
> +	/* depends on NVM config return error if dir output else the GPIO0 status bit */

	/*
	 * Depending on NVM config, return an error if direction is output,
	 * otherwise the GPIO0 status bit.
	 */

> +	if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT)
> +		return -EOPNOTSUPP;

Fix the error code.

> +	return ret;
> +}

...

> +static int tps65219_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;
> +
> +	/*
> +	 * Documentation is stating that GPIO0 direction must not be changed in Linux:
> +	 * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE,
> +	 * Should only be changed in INITIALIZE state (prior to ON Request).
> +	 * Set statically by NVM, changing direction in application can cause a hang.
> +	 * Below can be used for test purpose only:
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_GPIO)) {
> +		int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG,
> +					     TPS65219_GPIO0_DIR_MASK, direction);
> +		if (ret) {
> +			dev_err(dev,
> +				"GPIO DEBUG enabled: Fail to change direction to %u for GPIO%d.\n",
> +				direction, offset);
> +			return ret;
> +		}
> +	}
> +
> +	dev_err(dev,
> +		"GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
> +		 offset, direction);
> +
> +	return -EOPNOTSUPP;

Ditto.

> +}

...

> +static int tps65219_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> +	struct device *dev = gpio->tps->dev;
> +
> +	if (offset != TPS65219_GPIO0_IDX) {
> +		dev_err(dev, "GPIO%d is output only, cannot change to input\n", offset);
> +		return -EOPNOTSUPP;

Ditto.

> +	}
> +
> +	if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_IN)
> +		return 0;
> +
> +	return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_IN);
> +}

-- 
With Best Regards,
Andy Shevchenko