Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Jun 2020 14:35:57 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     alexandru.tachici@analog.com
Cc:     linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v4 3/7] hwmon: pmbus: adm1266: Add support for GPIOs
Message-ID: <20200624213557.GA74535@roeck-us.net>
References: <20200623173659.41358-1-alexandru.tachici@analog.com>
 <20200623173659.41358-4-alexandru.tachici@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200623173659.41358-4-alexandru.tachici@analog.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 23, 2020 at 08:36:55PM +0300, alexandru.tachici@analog.com wrote:
> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Adm1266 exposes 9 GPIOs and 16 PDIOs which are currently read-only. They
> are controlled by the internal sequencing engine.
> 
> This patch makes adm1266 driver expose GPIOs and PDIOs to user-space
> using GPIO provider kernel api.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  drivers/hwmon/pmbus/adm1266.c | 233 +++++++++++++++++++++++++++++++++-
>  1 file changed, 232 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 381d89a8569f..76bf2c78e737 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -6,8 +6,12 @@
>   * Copyright 2020 Analog Devices Inc.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/crc8.h>
> +#include <linux/debugfs.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
> +#include <linux/i2c-smbus.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -15,10 +19,35 @@
>  
>  #include "pmbus.h"
>  
> +#define ADM1266_PDIO_CONFIG	0xD4
> +#define ADM1266_GPIO_CONFIG	0xE1
> +#define ADM1266_PDIO_STATUS	0xE9
> +#define ADM1266_GPIO_STATUS	0xEA
> +
> +/* ADM1266 GPIO defines */
> +#define ADM1266_GPIO_NR			9
> +#define ADM1266_GPIO_FUNCTIONS(x)	FIELD_GET(BIT(0), x)
> +#define ADM1266_GPIO_INPUT_EN(x)	FIELD_GET(BIT(2), x)
> +#define ADM1266_GPIO_OUTPUT_EN(x)	FIELD_GET(BIT(3), x)
> +#define ADM1266_GPIO_OPEN_DRAIN(x)	FIELD_GET(BIT(4), x)
> +
> +/* ADM1266 PDIO defines */
> +#define ADM1266_PDIO_NR			16
> +#define ADM1266_PDIO_PIN_CFG(x)		FIELD_GET(GENMASK(15, 13), x)
> +#define ADM1266_PDIO_GLITCH_FILT(x)	FIELD_GET(GENMASK(12, 9), x)
> +#define ADM1266_PDIO_OUT_CFG(x)		FIELD_GET(GENMASK(2, 0), x)
> +
>  #define ADM1266_PMBUS_BLOCK_MAX		255
>  
>  DECLARE_CRC8_TABLE(pmbus_crc_table);
>  
> +struct adm1266_data {
> +	struct pmbus_driver_info info;
> +	struct gpio_chip gc;
> +	const char *gpio_names[ADM1266_GPIO_NR + ADM1266_PDIO_NR];
> +	struct i2c_client *client;
> +};
> +
>  /* Different from Block Read as it sends data and waits for the slave to

Please use standard multi-line comment (yes, I know, part of previous patch).

>   * return a value dependent on that data. The protocol is simply a Write Block
>   * followed by a Read Block without the Read-Block command field and the
> @@ -91,18 +120,220 @@ int pmbus_block_xfer(struct i2c_client *client, u8 cmd, u8 w_len,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_GPIOLIB

It is customary to use "select GPIOLIB" in Kconfig. I would suggest to do
that here as well.

> +static const unsigned int adm1266_gpio_mapping[ADM1266_GPIO_NR][2] = {
> +	{1, 0},
> +	{2, 1},
> +	{3, 2},
> +	{4, 8},
> +	{5, 9},
> +	{6, 10},
> +	{7, 11},
> +	{8, 6},
> +	{9, 7},
> +};
> +
> +static const char *adm1266_names[ADM1266_GPIO_NR + ADM1266_PDIO_NR] = {
> +	"GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5", "GPIO6", "GPIO7", "GPIO8",
> +	"GPIO9", "PDIO1", "PDIO2", "PDIO3", "PDIO4", "PDIO5", "PDIO6",
> +	"PDIO7", "PDIO8", "PDIO9", "PDIO10", "PDIO11", "PDIO12", "PDIO13",
> +	"PDIO14", "PDIO15", "PDIO16",
> +};
> +
> +static int adm1266_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct adm1266_data *data = gpiochip_get_data(chip);
> +	u8 read_buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	unsigned long pins_status;
> +	unsigned int pmbus_cmd;
> +	int ret;
> +
> +	if (offset < ADM1266_GPIO_NR)
> +		pmbus_cmd = ADM1266_GPIO_STATUS;
> +	else
> +		pmbus_cmd = ADM1266_PDIO_STATUS;
> +
> +	ret = i2c_smbus_read_block_data(data->client, pmbus_cmd,
> +					read_buf);

Maximum line length is now 100 columns.

> +	if (ret < 0)
> +		return ret;
> +
> +	pins_status = read_buf[0] + (read_buf[1] << 8);
> +	if (offset < ADM1266_GPIO_NR)
> +		return test_bit(adm1266_gpio_mapping[offset][1], &pins_status);
> +
> +	return test_bit(offset - ADM1266_GPIO_NR, &pins_status);
> +}
> +
> +static int adm1266_gpio_get_multiple(struct gpio_chip *chip,
> +				     unsigned long *mask,
> +				     unsigned long *bits)
> +{
> +	struct adm1266_data *data = gpiochip_get_data(chip);
> +	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1];
> +	unsigned long status;
> +	unsigned int gpio_nr;
> +	int ret;
> +
> +	ret = i2c_smbus_read_block_data(data->client, ADM1266_GPIO_STATUS,
> +					read_buf);

Line length again. Applies everywhere.

> +	if (ret < 0)
> +		return ret;
> +
> +	status = read_buf[0] + (read_buf[1] << 8);
> +
> +	*bits = 0;
> +	for_each_set_bit(gpio_nr, mask, ADM1266_GPIO_NR) {
> +		if (test_bit(adm1266_gpio_mapping[gpio_nr][1], &status))
> +			set_bit(gpio_nr, bits);
> +	}
> +
> +	ret = i2c_smbus_read_block_data(data->client, ADM1266_PDIO_STATUS,
> +					read_buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	status = read_buf[0] + (read_buf[1] << 8);
> +
> +	*bits = 0;
> +	for_each_set_bit_from(gpio_nr, mask,
> +			      ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
> +		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
> +			set_bit(gpio_nr, bits);
> +	}
> +
> +	return 0;
> +}
> +
> +static void adm1266_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> +	struct adm1266_data *data = gpiochip_get_data(chip);
> +	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1];
> +	unsigned long gpio_config;
> +	unsigned long pdio_config;
> +	unsigned long pin_cfg;
> +	u8 write_cmd;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < ADM1266_GPIO_NR; i++) {
> +		write_cmd = adm1266_gpio_mapping[i][1];
> +		ret = pmbus_block_xfer(data->client, ADM1266_GPIO_CONFIG, 1,
> +				       &write_cmd, read_buf);
> +		if (ret != 2)
> +			return;
> +
> +		gpio_config = read_buf[0];
> +		seq_puts(s, adm1266_names[i]);
> +
> +		seq_puts(s, " ( ");
> +		if (!ADM1266_GPIO_FUNCTIONS(gpio_config)) {
> +			seq_puts(s, "high-Z )\n");
> +			continue;
> +		}
> +		if (ADM1266_GPIO_INPUT_EN(gpio_config))
> +			seq_puts(s, "input ");
> +		if (ADM1266_GPIO_OUTPUT_EN(gpio_config))
> +			seq_puts(s, "output ");
> +		if (ADM1266_GPIO_OPEN_DRAIN(gpio_config))
> +			seq_puts(s, "open-drain )\n");
> +		else
> +			seq_puts(s, "push-pull )\n");
> +	}
> +
> +	write_cmd = 0xFF;
> +	ret = pmbus_block_xfer(data->client, ADM1266_PDIO_CONFIG, 1, &write_cmd,
> +			       read_buf);
> +	if (ret != 32)
> +		return;
> +
> +	for (i = 0; i < ADM1266_PDIO_NR; i++) {
> +		seq_puts(s, adm1266_names[ADM1266_GPIO_NR + i]);
> +
> +		pdio_config = read_buf[2 * i];
> +		pdio_config += (read_buf[2 * i + 1] << 8);
> +		pin_cfg = ADM1266_PDIO_PIN_CFG(pdio_config);
> +
> +		seq_puts(s, " ( ");
> +		if (!pin_cfg || pin_cfg > 5) {
> +			seq_puts(s, "high-Z )\n");
> +			continue;
> +		}
> +
> +		if (pin_cfg & BIT(0))
> +			seq_puts(s, "output ");
> +
> +		if (pin_cfg & BIT(1))
> +			seq_puts(s, "input ");
> +
> +		seq_puts(s, ")\n");
> +	}
> +}
> +
> +static int adm1266_config_gpio(struct adm1266_data *data)
> +{
> +	const char *name = dev_name(&data->client->dev);
> +	char *gpio_name;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(data->gpio_names); i++) {
> +		gpio_name = devm_kasprintf(&data->client->dev, GFP_KERNEL,
> +					   "adm1266-%x-%s", data->client->addr,
> +					   adm1266_names[i]);
> +		if (!gpio_name)
> +			return -ENOMEM;
> +
> +		data->gpio_names[i] = gpio_name;
> +	}
> +
> +	data->gc.label = name;
> +	data->gc.parent = &data->client->dev;
> +	data->gc.owner = THIS_MODULE;
> +	data->gc.base = -1;
> +	data->gc.names = data->gpio_names;
> +	data->gc.ngpio = ARRAY_SIZE(data->gpio_names);
> +	data->gc.get = adm1266_gpio_get;
> +	data->gc.get_multiple = adm1266_gpio_get_multiple;
> +	data->gc.dbg_show = adm1266_gpio_dbg_show;
> +
> +	ret = devm_gpiochip_add_data(&data->client->dev, &data->gc, data);
> +	if (ret)
> +		dev_err(&data->client->dev, "GPIO registering failed (%d)\n",
> +			ret);
> +
> +	return ret;
> +}
> +#else
> +static int adm1266_config_gpio(struct adm1266_data *data)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int adm1266_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
>  	struct pmbus_driver_info *info;
> +	struct adm1266_data *data;
>  	u32 funcs;
> +	int ret;
>  	int i;
>  
> -	info = devm_kzalloc(&client->dev, sizeof(struct pmbus_driver_info),
> +	data = devm_kzalloc(&client->dev, sizeof(struct adm1266_data),
>  			    GFP_KERNEL);

It might be a bit cleaner to create struct adm1266_data in the first patch
of the series, even if it only includes the "info" field. This would reduce
the size of the diffs here, as it would not be necessary to change this
code.

> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	ret = adm1266_config_gpio(data);
> +	if (ret < 0)
> +		return ret;
>  
>  	crc8_populate_msb(pmbus_crc_table, 0x7);
>  
> +	info = &data->info;
>  	info->pages = 17;
>  	info->format[PSC_VOLTAGE_OUT] = linear;
>  	funcs = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;