by Yuklin Soo

[permalink] [raw]

Subject: RE: [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC

> -----Original Message-----
> From: Linus Walleij <[email protected]>
> Sent: Saturday, December 23, 2023 1:59 AM
> To: Yuklin Soo <[email protected]>
> Cc: Bartosz Golaszewski <[email protected]>; Hal Feng
> <[email protected]>; Leyfoon Tan <[email protected]>;
> Jianlong Huang <[email protected]>; Emil Renner Berthing
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Drew Fustini <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Paul Walmsley <[email protected]>;
> Palmer Dabbelt <[email protected]>; Albert Ou
> <[email protected]>
> Subject: Re: [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC
>
> Hi Alex,
>
> thanks for your patch!
>
> On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <[email protected]>
> wrote:
>
> > pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain
> > pinctrl: starfive: jh8100: add pinctrl driver for sys_west domain
> > pinctrl: starfive: jh8100: add pinctrl driver for sys_gmac domain
> > pinctrl: starfive: jh8100: add pinctrl driver for AON domain
>
> To my eye it looks like a lot of code is duplicated between the four subdrivers.
>
> The pattern from other pin controllers is to create a file with all the common
> code and then subdrivers for each sub-pincontroller that have their own
> probe but calls into the library.
>
> C.f.
> drivers/pinctrl/qcom/pinctrl-apq8064.c:
>
> static int apq8064_pinctrl_probe(struct platform_device *pdev) {
> return msm_pinctrl_probe(pdev, &apq8064_pinctrl); }
>
> And that function is in drivers/pinctrl/qcom/pinctrl-msm.c
> and you find great inspiration in the qcom Kconfig and Makefile and
> drivers/pinctrl/qcom/pinctrl-msm.h
> that you can copypaste to pull this off.
>
> Maybe you should start with a patch that extract the common stuff from the
> existing jh7100/jh7110 drivers and then reuse that for jh8100?

The duplicated codes in the subdrivers have been moved to the main driver.
The subdrivers will call those functions from the main driver.
The test results are successful and will submit in the next version for review.

>
> Yours,
> Linus Walleij

2024-02-20 05:56:07

by Yuklin Soo

[permalink] [raw]

Subject: RE: [RFC PATCH 2/6] pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain

> -----Original Message-----
> From: Linus Walleij <[email protected]>
> Sent: Sunday, January 28, 2024 7:33 AM
> To: Yuklin Soo <[email protected]>
> Cc: Bartosz Golaszewski <[email protected]>; Hal Feng
> <[email protected]>; Leyfoon Tan <[email protected]>;
> Jianlong Huang <[email protected]>; Emil Renner Berthing
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Drew Fustini <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Paul Walmsley <[email protected]>;
> Palmer Dabbelt <[email protected]>; Albert Ou
> <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> > Reviewed-by: Ley Foon Tan <[email protected]>
> (...)
> > +#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