| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdb5rmUK06uW3M2Lsy4Wam8JvrjmGM83cJa-V3LZwTX9dg@mail.gmail.com>
Date: Tue, 14 Jan 2025 15:33:55 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Kamel Bouhara <kamel.bouhara@...tlin.com>, Bartosz Golaszewski <brgl@...ev.pl>,
Dmitry Torokhov <dmitry.torokhov@...il.com>, Uwe Kleine-König <ukleinek@...nel.org>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-input@...r.kernel.org,
linux-pwm@...r.kernel.org, Grégory Clement <gregory.clement@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support
Hi Mathieu,
thanks for your patch!
On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
<mathieu.dubois-briand@...tlin.com> 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@...tlin.com>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@...tlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
(...)
> +#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
Powered by blists - more mailing lists