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 = <c4282_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 = <c4282_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 = <c4282_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á