Re: [PATCH v3 3/6] hwmon: pmbus: adm1266: Add support for GPIOs

From: Guenter Roeck
Date: Fri May 29 2020 - 14:36:35 EST

Next message: Jason Gunthorpe: "Re: [PATCH] RDMA/core: Fix several reference count leaks."
Previous message: Arnd Bergmann: "Re: [PATCH v4 0/2] soc: ti: add k3 platforms chipid module driver"
In reply to: alexandru.tachici: "[PATCH v3 3/6] hwmon: pmbus: adm1266: Add support for GPIOs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 5/29/20 6:05 AM, alexandru.tachici@xxxxxxxxxx wrote:
> From: Alexandru Tachici <alexandru.tachici@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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 a7ef048a9a5c..190170300ef1 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -6,7 +6,11 @@
> * Copyright 2020 Analog Devices Inc.
> */
>
> +#include <linux/bitfield.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>
> @@ -14,16 +18,243 @@
>
> #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)
> +
> +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;
> +};
> +
> +#if IS_ENABLED(CONFIG_GPIOLIB)

GPIOLIB is bool. IS_ENABLED suggests tristate, which won't happen. Just use #ifdef.

> +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[PMBUS_BLOCK_MAX + 1];

Unnecessarily large. SMBUS_BLOCK_MAX is sufficient here.

> + 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);
> + 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);
> + else

else after return is unnecessary.

> + 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 gpio_data[PMBUS_BLOCK_MAX + 1];
> + u8 pdio_data[PMBUS_BLOCK_MAX + 1];

This is quite some strain on the stack. I would suggest to use only
one buffer and handle gpio completely first, then pdio.

> + unsigned long gpio_status;
> + unsigned long pdio_status;
> + unsigned int gpio_nr;
> + int ret;
> +
> + ret = i2c_smbus_read_block_data(data->client, ADM1266_GPIO_STATUS,
> + gpio_data);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_block_data(data->client, ADM1266_PDIO_STATUS,
> + pdio_data);
> + if (ret < 0)
> + return ret;
> +
> + gpio_status = gpio_data[0] + (gpio_data[1] << 8);
> + pdio_status = pdio_data[0] + (pdio_data[1] << 8);
> + *bits = 0;
> + for_each_set_bit(gpio_nr, mask, ADM1266_GPIO_NR) {
> + if (test_bit(adm1266_gpio_mapping[gpio_nr][1], &gpio_status))
> + set_bit(gpio_nr, bits);
> + }
> +
> + for_each_set_bit_from(gpio_nr, mask,
> + ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
> + if (test_bit(gpio_nr - ADM1266_GPIO_NR, &pdio_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 write_buf[PMBUS_BLOCK_MAX + 1];

We know how much data we are going to write. Allocating more is a waste.
Given that it is only going to be one byte, you might as well just point
to it directly or use an u8 variable and point to that.

> + u8 read_buf[PMBUS_BLOCK_MAX + 1];
> + unsigned long gpio_config;
> + unsigned long pdio_config;
> + unsigned long pin_cfg;
> + int ret;
> + int i;
> +
> + for (i = 0; i < ADM1266_GPIO_NR; i++) {
> + write_buf[0] = adm1266_gpio_mapping[i][1];
> + ret = pmbus_block_wr(data->client, ADM1266_GPIO_CONFIG, 1,
> + write_buf, read_buf);
> + if (ret < 0)
> + dev_err(&data->client->dev, "GPIOs scan failed(%d).\n",
> + ret);
> +
So there was an error ...

> + gpio_config = read_buf[0];

... and then happily report (random) data anyway ?

Besides, we are having logging noise again, this time on top of
the logging noise in pmbus_block_wr().

> + 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_buf[0] = 0xFF;
> + ret = pmbus_block_wr(data->client, ADM1266_PDIO_CONFIG, 1, write_buf,
> + read_buf);
> + if (ret < 0)
> + dev_err(&data->client->dev, "PDIOs scan failed(%d).\n", ret);
> +

Again: error followed by reporting (non-received) data anyway.
On top of that, 'ret' is not compared against ADM1266_PDIO_NR.
If less data is received than expected, the resulting output
will be random.

> + 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 inline int adm1266_config_gpio(struct adm1266_data *data)

inline is unnecessary. Let the compiler do its job.

> +{
> + 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);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> +
> + ret = adm1266_config_gpio(data);
> + if (ret < 0)
> + return ret;
>
> + info = &data->info;
> info->pages = 17;
> info->format[PSC_VOLTAGE_OUT] = linear;
> funcs = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
>

Next message: Jason Gunthorpe: "Re: [PATCH] RDMA/core: Fix several reference count leaks."
Previous message: Arnd Bergmann: "Re: [PATCH v4 0/2] soc: ti: add k3 platforms chipid module driver"
In reply to: alexandru.tachici: "[PATCH v3 3/6] hwmon: pmbus: adm1266: Add support for GPIOs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]