Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Linus Walleij]

Hi Mathieu,

thanks for your patch!

On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
<mathieu.dubois-briand@xxxxxxxxxxx> wrote:

> Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
>
> Two sets of GPIOs are provided by the device:
> - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities.
>   These GPIOs also provide interrupts on input changes.
> - Up to 6 GPOs, on unused keypad columns pins.
>
> Co-developed-by: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@xxxxxxxxxxx>
(...)
> +#include <linux/gpio/consumer.h>

Why?

My most generic feedback is if you have looked at using
select GPIO_REGMAP for this driver?

The regmap utility library is very helpful, look how other driver
selecting GPIO_REGMAP gets default implementations
from the library just git grep GPIO_REGMAP drivers/gpio/

> +static void max7360_gpio_set_value(struct gpio_chip *gc,
> +                                  unsigned int pin, int state)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {

OK some custom stuff...

> +               int off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
> +
> +               ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PORTS,
> +                                       BIT(off), state ? BIT(off) : 0);

Fairly standard.

> +       } else {
> +               ret = regmap_write(max7360_gpio->regmap,
> +                                  MAX7360_REG_PWMBASE + pin, state ? 0xFF : 0);
> +       }

Some custom stuff.

> +static int max7360_gpio_get_value(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       unsigned int val;
> +       int off;
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {
> +               off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
> +
> +               ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_PORTS, &val);
> +       } else {
> +               off = pin;
> +               ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val);
> +       }
> +
> +       if (ret) {
> +               dev_err(max7360_gpio->dev, "failed to read gpio-%d", pin);
> +               return ret;
> +       }
> +
> +       return !!(val & BIT(off));
> +}

Looks like stock template regmap-gpio.

> +static int max7360_gpio_get_direction(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       unsigned int val;
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return GPIO_LINE_DIRECTION_OUT;
> +
> +       ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, &val);
> +       if (ret) {
> +               dev_err(max7360_gpio->dev, "failed to read gpio-%d direction",
> +                       pin);
> +               return ret;
> +       }
> +
> +       if (val & BIT(pin))
> +               return GPIO_LINE_DIRECTION_OUT;
> +
> +       return GPIO_LINE_DIRECTION_IN;
> +}

Dito.

> +static int max7360_gpio_direction_input(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return -EIO;
> +
> +       ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL,
> +                               BIT(pin), 0);
> +       if (ret) {
> +               dev_err(max7360_gpio->dev, "failed to set gpio-%d direction",
> +                       pin);
> +               return ret;
> +       }
> +
> +       return 0;
> +}

Dito.

> +static int max7360_gpio_direction_output(struct gpio_chip *gc, unsigned int pin,
> +                                        int state)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
> +               ret = regmap_write_bits(max7360_gpio->regmap,
> +                                       MAX7360_REG_GPIOCTRL, BIT(pin),
> +                                       BIT(pin));
> +               if (ret) {
> +                       dev_err(max7360_gpio->dev,
> +                               "failed to set gpio-%d direction", pin);
> +                       return ret;
> +               }
> +       }
> +
> +       max7360_gpio_set_value(gc, pin, state);
> +
> +       return 0;
> +}

Dito.

> +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +
> +       /*
> +        * GPOs on COL pins (keypad columns) can always be requested: this
> +        * driver has full access to them, up to the number set in chip.ngpio.
> +        * GPIOs on PORT pins are shared with the PWM and rotary encoder
> +        * drivers: they have to be requested from the MFD driver.
> +        */
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return 0;
> +
> +       return max7360_port_pin_request(max7360_gpio->dev->parent, pin, true);
> +}
> +
> +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return;
> +
> +       max7360_port_pin_request(max7360_gpio->dev->parent, pin, false);
> +}

The pin request looks a bit like a custom pin control implementation...

But I think it's fine, pin control can be a bit heavy to implement on simple
devices, but if there is elaborate muxing and config going on, pin control
should be used.

Yours,
Linus Walleij