| 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:
<ZQ0PR01MB130267FBE0BFB694C2554605F650A@ZQ0PR01MB1302.CHNPR01.prod.partner.outlook.cn>
Date: Tue, 20 Feb 2024 05:55:42 +0000
From: Yuklin Soo <yuklin.soo@...rfivetech.com>
To: Linus Walleij <linus.walleij@...aro.org>
CC: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Hal Feng
<hal.feng@...rfivetech.com>, Leyfoon Tan <leyfoon.tan@...rfivetech.com>,
Jianlong Huang <jianlong.huang@...rfivetech.com>, Emil Renner Berthing
<kernel@...il.dk>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Drew
Fustini <drew@...gleboard.org>, "linux-gpio@...r.kernel.org"
<linux-gpio@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-riscv@...ts.infradead.org"
<linux-riscv@...ts.infradead.org>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: RE: [RFC PATCH 2/6] pinctrl: starfive: jh8100: add pinctrl driver for
sys_east domain
> -----Original Message-----
> From: Linus Walleij <linus.walleij@...aro.org>
> Sent: Sunday, January 28, 2024 7:33 AM
> To: Yuklin Soo <yuklin.soo@...rfivetech.com>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>; Hal Feng
> <hal.feng@...rfivetech.com>; Leyfoon Tan <leyfoon.tan@...rfivetech.com>;
> Jianlong Huang <jianlong.huang@...rfivetech.com>; Emil Renner Berthing
> <kernel@...il.dk>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley <conor+dt@...nel.org>;
> Drew Fustini <drew@...gleboard.org>; linux-gpio@...r.kernel.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org; linux-
> riscv@...ts.infradead.org; Paul Walmsley <paul.walmsley@...ive.com>;
> Palmer Dabbelt <palmer@...belt.com>; Albert Ou
> <aou@...s.berkeley.edu>
> Subject: Re: [RFC PATCH 2/6] pinctrl: starfive: jh8100: add pinctrl driver for
> sys_east domain
>
> Hi Alex,
>
> thanks for your patch!
>
> On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@...rfivetech.com>
> wrote:
>
> > Add pinctrl driver for sys_east domain.
>
> This commit message is wrong, it also contains the main driver for jh8100.
> Please add some proper subject and commit message.
Will change the commit log to "add main and sys_east driver" to indicate the commit of both main and sys-east driver.
>
> > Signed-off-by: Alex Soo <yuklin.soo@...rfivetech.com>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan@...rfivetech.com>
> (...)
> > +#define pin_to_hwirq(sfp) (((sfp)->wakeup_gpio) - ((sfp)->gc.base))
>
> Please do not reference gc.base like this, it is a gpio internal detail.
>
> Also, turn this into a static inline function, the macro is hard to read.
The pin_to_hwirq macro will be converted to to a static inline function to hide gpio
internal detail, and for easier code readability.
>
> > +/* pad control bits */
> > +#define JH8100_PADCFG_POS BIT(7)
> > +#define JH8100_PADCFG_SMT BIT(6)
> > +#define JH8100_PADCFG_SLEW BIT(5)
> > +#define JH8100_PADCFG_PD BIT(4)
> > +#define JH8100_PADCFG_PU BIT(3)
> > +#define JH8100_PADCFG_BIAS (JH8100_PADCFG_PD |
> JH8100_PADCFG_PU)
>
> JH8100_PADCFG_BIAS_MASK
Will change to "JH8100_PADCFG_BIAS_MASK" in next version.
>
> > +#define JH8100_PADCFG_DS_MASK GENMASK(2, 1)
> > +#define JH8100_PADCFG_DS_2MA (0U << 1)
> > +#define JH8100_PADCFG_DS_4MA BIT(1)
> > +#define JH8100_PADCFG_DS_8MA (2U << 1)
> > +#define JH8100_PADCFG_DS_12MA (3U << 1)
>
> Please use (1U << 1) for 4MA, this looks weird otherwise.
Will change to "(1U << 1)" for 4MA in next version.
>
> > +static const struct pinconf_ops jh8100_pinconf_ops = {
> > + .pin_config_get = jh8100_pinconf_get,
> > + .pin_config_group_get = jh8100_pinconf_group_get,
> > + .pin_config_group_set = jh8100_pinconf_group_set,
> > + .pin_config_dbg_show = jh8100_pinconf_dbg_show,
> > + .is_generic = true,
> > +};
> > +
> > +static int jh8100_gpio_request(struct gpio_chip *gc, unsigned int
> > +gpio) {
> > + return pinctrl_gpio_request(gc, gpio); }
> > +
> > +static void jh8100_gpio_free(struct gpio_chip *gc, unsigned int gpio)
> > +{
> > + pinctrl_gpio_free(gc, gpio);
> > +}
>
> Skip one level of indirection, just add pinctrl_gpio_request/free directly into
> the vtable.
This will be fixed in next version.
>
> > +static int jh8100_gpio_set_config(struct gpio_chip *gc,
> > + unsigned int gpio, unsigned long
> > +config) {
> > + struct jh8100_pinctrl *sfp = container_of(gc,
> > + struct jh8100_pinctrl, gc);
> > + u32 arg = pinconf_to_config_argument(config);
>
> Please don't reimplement .set_config, just call into the pinctrl backend using
>
> .set_config = gpiochip_generic_config
Will replace "jh8100_gpio_set_config" by "gpiochip_generic_config" in next version.
>
> > +static int jh8100_gpio_add_pin_ranges(struct gpio_chip *gc) {
> > + struct jh8100_pinctrl *sfp = container_of(gc,
> > + struct jh8100_pinctrl, gc);
> > +
> > + sfp->gpios.name = sfp->gc.label;
> > + sfp->gpios.base = sfp->gc.base;
> > + sfp->gpios.pin_base = 0;
> > + sfp->gpios.npins = sfp->gc.ngpio;
> > + sfp->gpios.gc = &sfp->gc;
> > + pinctrl_add_gpio_range(sfp->pctl, &sfp->gpios);
> > + return 0;
> > +}
>
> Why are you not putting the ranges into the device tree where the GPIO core
> will add them for you?
Will remove the jh8100_gpio_add_pin_ranges function and use gpio-ranges in device tree to
provide information for GPIO core to add pin range for each pin controller.
>
> > + if (info->irq_reg) {
> > + jh8100_irq_chip.name = sfp->gc.label;
>
> That's not immutable. The struct should be const.
> You have to use .irq_print_chip in the irq_chip.
The struct irq_chip will be used as a constant data structure.
Will add an irq_print_chip function to display irqchip name to user space.
>
> > + gpio_irq_chip_set_chip(&sfp->gc.irq,
> > + &jh8100_irq_chip);
>
> Use the convention:
>
> struct gpio_irq_chip *girq;
>
> girq = &chip->irq;
> gpio_irq_chip_set_chip(girq, &nmk_irq_chip);
>
> ... and use girq-> in the rest of the assignments.
girq will be used to represent GPIO interrupt controller.
>
> > + dev_info(dev, "StarFive GPIO chip registered %d
> > + GPIOs\n", sfp->gc.ngpio);
>
> StarFive JH8100 (be precise)
"StarFive GPIO" will be changed to "StarFive JH8100 GPIO".
>
> Yours,
> Linus Walleij
Powered by blists - more mailing lists