Received-SPF: pass (google.com: domain of linux-kernel+bounces-41460-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20231221083622.3445726-1-yuklin.soo@starfivetech.com> <20231221083622.3445726-3-yuklin.soo@starfivetech.com>
In-Reply-To: <20231221083622.3445726-3-yuklin.soo@starfivetech.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Sun, 28 Jan 2024 00:33:26 +0100
Message-ID: <CACRpkdagPYfSq91dXOTJ9d8VjCMMG2AN3L9-Qxdw3C8tki09EQ@mail.gmail.com>
Subject: Re: [RFC PATCH 2/6] pinctrl: starfive: jh8100: add pinctrl driver for
 sys_east domain
To: Alex Soo <yuklin.soo@starfivetech.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>, Hal Feng <hal.feng@starfivetech.com>, 
	Ley Foon Tan <leyfoon.tan@starfivetech.com>, 
	Jianlong Huang <jianlong.huang@starfivetech.com>, Emil Renner Berthing <kernel@esmil.dk>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Drew Fustini <drew@beagleboard.org>, linux-gpio@vger.kernel.org, 
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-riscv@lists.infradead.org, Paul Walmsley <paul.walmsley@sifive.com>, 
	Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Alex,

thanks for your patch!

On Thu, Dec 21, 2023 at 9:36=E2=80=AFAM Alex Soo <yuklin.soo@starfivetech.c=
om> 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@starfivetech.com>
> Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.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 =3D {
> +       .pin_config_get         =3D jh8100_pinconf_get,
> +       .pin_config_group_get   =3D jh8100_pinconf_group_get,
> +       .pin_config_group_set   =3D jh8100_pinconf_group_set,
> +       .pin_config_dbg_show    =3D jh8100_pinconf_dbg_show,
> +       .is_generic             =3D 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 =3D container_of(gc,
> +                       struct jh8100_pinctrl, gc);
> +       u32 arg =3D pinconf_to_config_argument(config);

Please don't reimplement .set_config, just call into the pinctrl
backend using

set_config =3D gpiochip_generic_config

> +static int jh8100_gpio_add_pin_ranges(struct gpio_chip *gc)
> +{
> +       struct jh8100_pinctrl *sfp =3D container_of(gc,
> +                       struct jh8100_pinctrl, gc);
> +
> +       sfp->gpios.name =3D sfp->gc.label;
> +       sfp->gpios.base =3D sfp->gc.base;
> +       sfp->gpios.pin_base =3D 0;
> +       sfp->gpios.npins =3D sfp->gc.ngpio;
> +       sfp->gpios.gc =3D &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 =3D 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 =3D &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