RE: [RFC PATCH v2 2/6] pinctrl: starfive: jh8100: add main and sys_east driver

[Yuklin Soo <yuklin.soo@xxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: Thursday, February 29, 2024 9:03 PM
> To: Yuklin Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>; Hal Feng
> <hal.feng@xxxxxxxxxxxxxxxx>; Leyfoon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>;
> Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>; Emil Renner Berthing
> <kernel@xxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> Drew Fustini <drew@xxxxxxxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> riscv@xxxxxxxxxxxxxxxxxxx; Paul Walmsley <paul.walmsley@xxxxxxxxxx>; Palmer
> Dabbelt <palmer@xxxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx>
> Subject: Re: [RFC PATCH v2 2/6] pinctrl: starfive: jh8100: add main and sys_east
> driver
> 
> Thanks Alex,
> 
> this new version is much improved!
> 
> On Tue, Feb 20, 2024 at 7:43 AM Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx> wrote:
> 
> > Add JH8100 pinctrl main and sys_east domain driver.
> >
> > Signed-off-by: Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> 
> This commit message should at least explain what we are adding here, that it's
> a core driver that will be used by all the domains, what the SoC is etc etc.

Will update the commit message of the main driver and sys_east domain subdriver to
to inform what SoC they are running on, and to explain that the main driver provides
common APIs to all the domain subdrivers to perform their respective tasks, and how
the main driver and domain drivers work together.

> 
> > +       select GPIOLIB_IRQCHIP
> (...)
> > +#include "../core.h"
> > +#include "../pinmux.h"
> > +#include "../pinconf.h"
> 
> Do you really need to poke around in the internals like this?
> 
> Please explain for each cross-include *why* you need to do this.

For subdrivers: “aon”, “sys-east”, “sys-west”, and ”sys-gmac” :

The cross-include “../pinmux.h” and “../pinconf.h” are redundant.

The cross-include “../core.h” provides reference to functions:
int pinctrl_force_sleep(struct pinctrl_dev *pctldev);
int pinctrl_force_default(struct pinctrl_dev *pctldev);

Remove all cross-include “../core.h”, "../pinmux.h" and "../pinconf.h" from the subdrivers,
and move "#include "../core.h" to driver header file “pinctrl-starfive-jh8100.h”.

> 
> > +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh8100.c
> (...)
> > +#include <linux/of_gpio.h>
> 
> Never use this include. It is legacy and you should not be using it. Use
> <linux/gpio/consumer.h> solely. See comments below.

The header <linux/of_gpio.h> is removed and use only the gpiod interfaces in <linux/gpio/consumer.h>.

> 
> > +#include <linux/pinctrl/consumer.h>
> 
> Why?
> 
> > +#include "../core.h"
> > +#include "../pinctrl-utils.h"
> > +#include "../pinmux.h"
> > +#include "../pinconf.h"
> 
> Again all this. Explain for each one exactly why you need this.

Cross-includes:

 “../core.h” --

provides definitions for “struct pinctrl_dev” and “struct group_desc”.

also, provides reference to functions:

pinctrl_generic_get_group_count
pinctrl_generic_get_group_name
pinctrl_generic_get_group_pins
pinctrl_generic_get_group
pinctrl_generic_add_group

Suggest to move “../core.h” to the driver header file “pinctrl-starfive-jh8100.h”

“../pinctrl-utils.h” -- 

provides reference to function:

pinctrl_utils_free_map

Suggest removing “../pinctrl-utils.h” and add the “pinctrl_utils_free_map” 
function prototype to the driver header file “pinctrl-starfive-jh8100.h”.

“../pinmux.h” – 

provides reference to functions:

pinmux_generic_get_function_count
pinmux_generic_get_function_name
pinmux_generic_get_function_groups
pinmux_generic_add_function

Suggest removing “../pinmux.h” and add the above “pinmux_generic_*” 
function prototypes to the driver header file “pinctrl-starfive-jh8100.h”.

“../pinconf.h” – 

provides reference to function: 

pinconf_generic_parse_dt_config

Suggest removing “../pinconf.h” and add the above “pinconf_generic_parse_dt_config”
function prototype to driver header file “pinctrl-starfive-jh8100.h”.

> 
> > +static int jh8100_gpio_irq_setup(struct device *dev, struct
> > +jh8100_pinctrl *sfp) {
> > +       struct device_node *np = dev->of_node;
> > +       struct gpio_irq_chip *girq = &sfp->gc.irq;
> > +       struct gpio_desc *gpiod;
> > +       struct irq_desc *desc;
> > +       irq_hw_number_t hwirq;
> > +       int i, ret;
> > +       int dir;
> > +
> > +       if (!girq->domain) {
> > +               sfp->irq_domain = irq_domain_add_linear(np, sfp->gc.ngpio,
> > +                                                       &irq_domain_simple_ops,
> > +                                                       sfp);
> 
> When would this happen? I don't quite get it.
> 
> It looks like a probe order issue or something.

This condition is unlikely to happen. Will remove this if statement.

> 
> > +       } else {
> > +               sfp->irq_domain = girq->domain;
> > +       }
> > +
> > +       if (!sfp->irq_domain) {
> > +               dev_err(dev, "Couldn't allocate IRQ domain\n");
> > +               return -ENXIO;
> > +       }
> > +
> > +       for (i = 0; i < sfp->gc.ngpio; i++) {
> > +               int virq = irq_create_mapping(sfp->irq_domain, i);
> > +
> > +               irq_set_chip_and_handler(virq, &jh8100_irq_chip,
> > +                                        handle_edge_irq);
> > +               irq_set_chip_data(virq, &sfp->gc);
> > +       }
> 
> This duplicates core gpiolib irqchip handling, which you select using select
> GPIOLIB_IRQCHIP.
> 
> Can you please look into just using the gpiolib irqchip handling?

Yes, will use the gpiolib irqchip handling. Please see the implementation in
function gpiochip_wakeup_irq_setup() in drivers/gpio/gpiolib.c.

> 
> > +       sfp->wakeup_gpio = of_get_named_gpio(np, "wakeup-gpios", 0);
> 
> No using <linux/of_gpio.h> please.
> 
> Use just <linux/gpio/consumer.h> and something like:
> 
> struct gpio_desc *wakeup;
> wakeup = devm_gpiod_get_optional(dev, "wakeup", GPIOD_IN);

Will use only descriptor-based interface to handle the wakeup_gpio since the
integer-based interface is obsolete. Please see the implementation in
function gpiochip_wakeup_irq_setup() in drivers/gpio/gpiolib.c.

> 
> > +       if (gpio_is_valid(sfp->wakeup_gpio)) {
> > +               hwirq = pin_to_hwirq(sfp);
> > +               sfp->wakeup_irq = irq_find_mapping(sfp->irq_domain, hwirq);
> > +               desc = irq_to_desc(sfp->wakeup_irq);
> 
> if (wakeup) {
>      irq = gpiod_to_irq(wakeup);
>      .. convert to irq descriptor etc...
> 
> Actually: is this wakeup handling something we would like to add to the gpiolib
> irqchip so everyone can reuse it?
> In gpiochip_add_irqchip()?
> At least give it a thought.

I have removed the function jh8100_gpio_irq_setup() and the GPIO wakeup irq 
enablement in probe function from the main driver.
The function gpiochip_wakeup_irq_setup() is created in gpiolib core
"drivers/gpio/gpiolib.c" as a replacement of the codes removed from the main
driver. This function is a wakeup handling that automatically set up the gpio wakeup
irq if user had the “wakeup-source” and “wakeup-gpios” properties defined in their
device tree.
Initially, I tried to call gpiochip_wakeup_irq_setup() from function gpiochip_add_irqchip(),
but gpiod_to_irq() returns -517 (EPROBE_DEFER) when convert hwirq to virq. 
Eventually, this function is called at the end of function gpiochip_add_data_with_key().
The testing showed that when the GPIO logic level is toggled, the interrupt triggered
can wake up the system from sleep.

> 
> Yours,
> Linus Walleij