[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACRpkdagPYfSq91dXOTJ9d8VjCMMG2AN3L9-Qxdw3C8tki09EQ@mail.gmail.com>
Date: Sun, 28 Jan 2024 00:33:26 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Alex Soo <yuklin.soo@...rfivetech.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Hal Feng <hal.feng@...rfivetech.com>,
Ley Foon 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.
> 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.
> +/* 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
> +#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.
> +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.
> +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
> +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?
> + 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.
> + 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.
> + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio);
StarFive JH8100 (be precise)
Yours,
Linus Walleij
Powered by blists - more mailing lists