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
