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

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Wed, 2023-11-29 at 15:49 +0100, Linus Walleij wrote:
> 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
> 

Hmm alright, the only reason why I didn't do this is because gpiochip is an optional
feature for the driver. So I have an '!IS_ENABLED(CONFOG_GPIOLIB)' guard in the
beginning of the function. But yeah, will just do this. Odds are that gpio is already
enabled anyways.

> 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.
> 

Ok, will look at it.

> Other than that it looks fine.
> 

Cool, I actually thought that having the direction + get/set stuff would be weird
given the fact that we can only PULL_LOW or HIGH_Z the pins.

Thanks!
- Nuno Sá