Re: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Nuno,

GPIO-related review as requested! Thanks for your patch!

On Fri, Nov 24, 2023 at 3:18 PM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:

> +config SENSORS_LTC4282
> +       tristate "Analog Devices LTC4282"
> +       depends on I2C
> +       select REGMAP_I2C

select GPIOLIB

potentially also

select GPIO_REGMAP, see below.

> +struct ltc4282_gpio {
> +       const char * const *funcs;
> +       u32 out_reg;
> +       u32 out_mask;
> +       u32 in_reg;
> +       u32 in_mask;
> +       bool active_high;
> +       u8 n_funcs;
> +};

So pretty simple dedicated bits.

> +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct ltc4282_state *st = gpiochip_get_data(chip);
> +
> +       /* we can only control this for GPIO_1 */
> +       if (offset != LTC4282_GPIO_1)
> +               return 0;
> +
> +       return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG,
> +                              LTC4282_GPIO_1_CONFIG_MASK);
> +}
> +
> +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int offset,
> +                                  int val)
> +{
> +       struct ltc4282_state *st = gpiochip_get_data(chip);
> +       const struct ltc4282_gpio *gpio = &ltc4282_gpios[offset];
> +
> +       guard(mutex)(&st->lock);
> +       /*
> +        * Explicitly setting the pin as output can only be done for GPIO_1. For
> +        * the other pins we just pull the line down or high-z.
> +        */
> +       if (offset == LTC4282_GPIO_1) {
> +               int ret;
> +
> +               ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG,
> +                                        LTC4282_GPIO_1_CONFIG_MASK,
> +                                        FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, 2));
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /*
> +        * GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down
> +        * the line
> +        */
> +       if (!gpio->active_high)
> +               val = !val;
> +
> +       return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask,
> +                                 field_prep(gpio->out_mask, val));
> +}
> +
> +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                            int val)
> +{
> +       struct ltc4282_state *st = gpiochip_get_data(chip);
> +       const struct ltc4282_gpio *gpio = &ltc4282_gpios[offset];
> +
> +       if (!gpio->active_high)
> +               val = !val;
> +
> +       regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask,
> +                          field_prep(gpio->out_mask, val));
> +}
> +
> +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct ltc4282_state *st = gpiochip_get_data(chip);
> +       const struct ltc4282_gpio *gpio = &ltc4282_gpios[offset];
> +       int ret;
> +       u32 val;
> +
> +       ret = regmap_read(st->map, gpio->in_reg, &val);
> +       if (ret)
> +               return ret;
> +
> +       return !!(val & gpio->in_mask);
> +}
> +
> +static int ltc4282_gpio_valid_mask(struct gpio_chip *chip,
> +                                  unsigned long *valid_mask,
> +                                  unsigned int ngpios)
> +{
> +       struct ltc4282_state *st = gpiochip_get_data(chip);
> +
> +       *valid_mask = st->valid_mask;
> +       return 0;
> +}

Some of this looks like it could use GPIO_REGMAP, look into other
drivers using these helpers such as
drivers/gpio/gpio-ds4520.c and see how small it becomes.

It may or may not help you. But take a look.

Other than that it looks fine.

Yours,
Linus Walleij