| 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: <D71ZOCDV6CTL.G3BF0PKHLUTU@bootlin.com>
Date: Tue, 14 Jan 2025 18:57:45 +0100
From: "Mathieu Dubois-Briand" <mathieu.dubois-briand@...tlin.com>
To: "Linus Walleij" <linus.walleij@...aro.org>
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
On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote:
> 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/
>
Thanks, I was not aware of that. I tested it and I should be able to get
rid of a lot of code using GPIO_REGMAP.
My main concern so far is with the request()/free() functions, as I
believe I will not be able to define them as callback anymore.
I also saw the equivalent REGMAP_IRQ, but I'm not sure I will be able to
use it, as I believe I would need to have registers identifying the
exact GPIO source of the IRQ.
> > +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.
Just so remove any doubt, all this does is request the pin for the
exclusive use of this driver, preventing the PWM or rotary encoder
drivers to use it. There is no hardware configuration done here.
Yet I agree that this does look a bit like some pin muxing.
>
> Yours,
> Linus Walleij
Thanks for your review.
--
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists