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 = <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. Other than that it looks fine. Yours, Linus Walleij