| 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:
<CH2PR03MB5285363BCF71D480471AB49F82772@CH2PR03MB5285.namprd03.prod.outlook.com>
Date: Tue, 1 Oct 2024 14:29:41 +0000
From: "Artamonovs, Arturs" <Arturs.Artamonovs@...log.com>
To: Linus Walleij <linus.walleij@...aro.org>
CC: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Greg Malysa <greg.malysa@...esys.com>,
Philipp Zabel
<p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski
<krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"Agarwal, Utsav"
<Utsav.Agarwal@...log.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Bartosz Golaszewski <brgl@...ev.pl>,
Thomas
Gleixner <tglx@...utronix.de>,
Andi Shyti <andi.shyti@...nel.org>,
Greg
Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby
<jirislaby@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Olof Johansson
<olof@...om.net>, "soc@...nel.org" <soc@...nel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>,
"linux-clk@...r.kernel.org"
<linux-clk@...r.kernel.org>,
"linux-gpio@...r.kernel.org"
<linux-gpio@...r.kernel.org>,
"linux-i2c@...r.kernel.org"
<linux-i2c@...r.kernel.org>,
"linux-serial@...r.kernel.org"
<linux-serial@...r.kernel.org>,
Linux Factory <adsp-linux@...log.com>,
Nathan
Barrett-Morrison <nathan.morrison@...esys.com>
Subject: RE: [PATCH 09/21] gpio: add driver for ADI ADSP-SC5xx platform
Thanks for feedback Linus, will address that in next patch version
> -----Original Message-----
> From: Linus Walleij <linus.walleij@...aro.org>
> Sent: Tuesday, October 1, 2024 1:45 PM
> To: Artamonovs, Arturs <Arturs.Artamonovs@...log.com>
> Cc: Catalin Marinas <catalin.marinas@....com>; Will Deacon <will@...nel.org>;
> Greg Malysa <greg.malysa@...esys.com>; Philipp Zabel
> <p.zabel@...gutronix.de>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski
> <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Agarwal, Utsav
> <Utsav.Agarwal@...log.com>; Michael Turquette <mturquette@...libre.com>;
> Stephen Boyd <sboyd@...nel.org>; Bartosz Golaszewski <brgl@...ev.pl>;
> Thomas Gleixner <tglx@...utronix.de>; Andi Shyti <andi.shyti@...nel.org>; Greg
> Kroah-Hartman <gregkh@...uxfoundation.org>; Jiri Slaby <jirislaby@...nel.org>;
> Arnd Bergmann <arnd@...db.de>; Olof Johansson <olof@...om.net>;
> soc@...nel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org; linux-clk@...r.kernel.org;
> linux-gpio@...r.kernel.org; linux-i2c@...r.kernel.org; linux-
> serial@...r.kernel.org; Linux Factory <adsp-linux@...log.com>; Nathan Barrett-
> Morrison <nathan.morrison@...esys.com>
> Subject: Re: [PATCH 09/21] gpio: add driver for ADI ADSP-SC5xx platform
>
> [External]
>
> Hi Arturs,
>
> thanks for your patch!
>
> On Thu, Sep 12, 2024 at 8:20 PM Arturs Artamonovs via B4 Relay
> <devnull+arturs.artamonovs.analog.com@...nel.org> wrote:
>
> > From: Arturs Artamonovs <arturs.artamonovs@...log.com>
> >
> > Add ADSP-SC5xx GPIO driver.
> > - Support all GPIO ports
> > - Each gpio support seperate PINT interrupt controller
> >
> > Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@...log.com>
> > Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> > Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> > Co-developed-by: Greg Malysa <greg.malysa@...esys.com>
> > Signed-off-by: Greg Malysa <greg.malysa@...esys.com>
>
> (...)
>
> > +config GPIO_ADI_ADSP_PORT
> > + bool "ADI ADSP PORT GPIO driver"
> > + depends on OF_GPIO
> > + select GPIO_GENERIC
>
> If you select this then you need to use it in the idiomatic way.
>
> +#include <linux/soc/adi/adsp-gpio-port.h>
>
> Drop this, just bring the contents into this file all register defines
> etc.
>
> +#include "gpiolib.h"
>
> No way, do this:
> #include <linux/gpio/driver.h>
>
> > +static int adsp_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> > +
> > + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR);
>
> Ah these __adsp_gpio_writew/readw things are too idiomatic. Just
> use the base and common writew() please.
>
> > + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET);
>
> Interrupt enable in the direction function? No thanks, poke the
> interrupt registers in your irqchip if you make one (you currently
> do not) in this case I'd say just disable all interrupts in probe()
> using something like writew(base + ADSP_PORT_REG_INEN_SET, 0xffff)
> and be done with it.
>
> > +static int adsp_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> > +
> > + return !!(__adsp_gpio_readw(port, ADSP_PORT_REG_DATA) & BIT(offset));
> > +}
>
> This becomes a reimplemenation of generic GPIO.
>
> > +static int adsp_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> > + irq_hw_number_t irq = offset + port->irq_offset;
> > + int map = irq_find_mapping(port->irq_domain, irq);
> > +
> > + if (map)
> > + return map;
> > +
> > + return irq_create_mapping(port->irq_domain, irq);
> > +}
>
> This irqdomain in the "port" looks weird.
>
> Implement the irqchip in the GPIO driver instead.
>
> If the domain *has* to be external to the GPIO driver then
> you need to use hierarchical irqdomains.
>
> > +static int adsp_gpio_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct adsp_gpio_port *gpio;
> > + int ret;
> > +
> > + gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + gpio->regs = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(gpio->regs))
> > + return PTR_ERR(gpio->regs);
>
> So you have gpio->regs which is the base.
>
> > + gpio->dev = dev;
> > +
> > + ret = adsp_attach_pint_to_gpio(gpio);
> > + if (ret)
> > + dev_err_probe(gpio->dev, ret, "error attaching interupt to gpio
> pin\n");
> > +
> > + spin_lock_init(&gpio->lock);
> > +
> > + gpio->gpio.label = "adsp-gpio";
> > + gpio->gpio.direction_input = adsp_gpio_direction_input;
> > + gpio->gpio.direction_output = adsp_gpio_direction_output;
> > + gpio->gpio.get = adsp_gpio_get_value;
> > + gpio->gpio.set = adsp_gpio_set_value;
> > + gpio->gpio.to_irq = adsp_gpio_to_irq;
> > + gpio->gpio.request = gpiochip_generic_request;
> > + gpio->gpio.free = gpiochip_generic_free;
> > + gpio->gpio.ngpio = ADSP_PORT_NGPIO;
> > + gpio->gpio.parent = dev;
> > + gpio->gpio.base = -1;
> > + return devm_gpiochip_add_data(dev, &gpio->gpio, gpio);
>
> Look in e.g. drivers/gpio/gpio-ftgpio010.c for an example of
> how to use generic GPIO (with an irqchip!). It will be something like:
>
> ret = bgpio_init(&g->gc, dev, 2,
> gpio->regs + ADSP_PORT_REG_DATA,
> gpio->regs + ADSP_PORT_REG_DATA_SET,
> gpio->regs + ADSP_PORT_REG_DATA_CLEAR,
> gpio->regs + ADSP_PORT_REG_DIR_SET,
> gpio->regs + ADSP_PORT_REG_DIR_CLEAR,
> 0);
> if (ret) {
> dev_err(dev, "unable to init generic GPIO\n");
> goto dis_clk;
> }
> g->gc.label = dev_name(dev);
> g->gc.base = -1;
> g->gc.parent = dev;
> g->gc.owner = THIS_MODULE;
> /* ngpio is set by bgpio_init() */
>
> You can augment the generic driver instance with an extra config function
> to set the special open drain bits.
>
> Yours,
> Linus Walleij
Powered by blists - more mailing lists