Re: [PATCH 20/32] riscv: Add Kendryte K210 FPIOA pinctrl driver

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On Sun, 2020-11-29 at 22:33 +0100, Linus Walleij wrote:
> On Tue, Nov 24, 2020 at 9:53 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> > On 2020/11/24 17:43, Linus Walleij wrote:
> 
> > > Would also be nice if the maintainer could add some comments?
> > 
> > What do you mean ? I do not understand. scripts/get_maintainer.pl indicates that
> > you are the maintainer of the pinctrl drivers subsystem.
> 
> Sorry I thought we had a RISCV driver already, and we don't
> so this is cool.
> 
> > Do you mean adding an
> > entry to the MAINTAINER file for this driver ? I can do that and put my self as
> > maintainer. Or do you mean you would like a comment from Palmer (riscv arch
> > maintainer) ?
> 
> That would be nice. Whoever will enthusiastically review patches to
> this driver and make sure it works and get modernized should ideally
> be listed as maintainer. I suggest you list yourself.

OK, no problem. Will do.

> The only input I want from the RISCV arch maintainer would
> be on this code:
> 
> +/*
> + * Most devices on the K210 SoC depend on pin mapping changes to initialize
> + * correctly. So initialize this driver early as part of the post core
> + * initialization.
> + */
> +static int __init k210_fpioa_init(void)
> +{
> +       return platform_driver_register(&k210_fpioa_driver);
> +}
> +postcore_initcall(k210_fpioa_init);
> 
> This is a bit nasty and we do not recommend it. But I will accept it
> if the arch maintainer claims it is necessary. What happens if you
> just make it initialize at driver level?

I checked again and nothing bad happens. Declaring the driver with
builtin_platform_driver() works just fine. I will change back to using this
instead of the postcore_initcall().

Note that I had done the same for the k210 sysctl and reset drivers too to get
these drivers to initialize early and avoid an annoying error messages when the
dwapb GPIO driver is probed (the error being "Cannot get reset descriptor" from
dwapb_get_reset()). But that is not a real error as the error code is
EPROBE_DEFER, which leads to the gpio driver initialization being retried
later, after the reset controller initialization. This annoying boot false
error message can be fixed simply with something like this:

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 2a9046c0fb16..4a801e83919b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -617,8 +617,10 @@ static int dwapb_get_reset(struct dwapb_gpio *gpio)
 
        gpio->rst = devm_reset_control_get_optional_shared(gpio->dev, NULL);
        if (IS_ERR(gpio->rst)) {
-               dev_err(gpio->dev, "Cannot get reset descriptor\n");
-               return PTR_ERR(gpio->rst);
+               err = PTR_ERR(gpio->rst);
+               if (err != -EPROBE_DEFER)
+                       dev_err(gpio->dev, "Cannot get reset descriptor\n");
+               return err;
        }

and all drivers can now use builtin_platform_driver() declaration with a clean
boot log. If you are OK with the above patch, I will send it.

Thanks !

> 
> Yours,
> Linus Walleij

-- 
Damien Le Moal
Western Digital