Re: [PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Bartosz Golaszewski]

On Wed, Jul 13, 2022 at 12:39 PM Henning Schild
<henning.schild@xxxxxxxxxxx> wrote:
>

[snip]

> > > +
> > > +static struct platform_driver nct6116d_gpio_driver = {
> > > +       .driver = {
> > > +               .name   = KBUILD_MODNAME,
> > > +       },
> > > +       .probe          = nct6116d_gpio_probe,
> > > +};
> > > +
> > > +static int __init nct6116d_gpio_init(void)
> > > +{
> > > +       struct nct6116d_sio sio;
> > > +       int err;
> > > +
> > > +       if (nct6116d_find(0x2e, &sio) &&
> > > +           nct6116d_find(0x4e, &sio))
> > > +               return -ENODEV;
> > > +
> > > +       err = platform_driver_register(&nct6116d_gpio_driver);
> > > +       if (!err) {
> > > +               err = nct6116d_gpio_device_add(&sio);
> > > +               if (err)
> > > +
> > > platform_driver_unregister(&nct6116d_gpio_driver);
> > > +       }
> > > +
> > > +       return err;
> > > +}
> >
> > I'm confused - have we not discussed removing this part?
>
> Ah, i thought the problem was the use of subsys_initcall because the
> comment was under that line.
>
> To he honest i do not know all the details since i really just received
> that driver.
>
> What is happening here is that some init code goes and probes well
> known ports to discover which chip might be connected there. Looking at
> hwmon, watchdog and similar gpios ... that is the established pattern
> for Super IOs.

I just thought that you would use the simatic driver you mentioned to
trigger the creation of the devices upon some event. This is what I
got from your previous email. But that's fine - if there's a repeating
pattern of doing it this way, then I won't object. I'm not an expert
on Super IO kernel drivers.

> Not DT or ACPI bindings. Something has to load that module to make it
> init, where it will go and enumerate/poke around.
>
> While i could likely put a platform_device_register_simple("driver",
> 0x42, "chip42") into the simatic platform, then the driver would be
> pretty useless when not having ACPI (for there Super IOs in general!).
> There already are hwmon and watchdog drivers for exactly that chip.
>
> watchdog/w83627hf_wdt.c
> hwmon/nct6775*
>
> All are based on someone has to "know" and "modprobe", which will cause
> "finding"
>
> The pattern we have here seems all copied from gpio/gpio-f7188x.c,
> another super similar driver is gpio/gpio-winbond.c (which is param
> based and not at all reusable in other drivers).
>
> Looking at hwmon or watchdog, Nuvoton/Fintek/Windbond are sometimes
> called a family. But the driver landscape in the kernel is very spread
> and a lot of copied around code. I did not even look into tty/serial or
> whatever other functions these Super I/Os offer.
>
> Looking at the way Super IO chips are driven in the kernel, it seems
> all about writing a driver for each sub-function (uart, hwmon, watchdog
> ... and gpio). Where even very similar chips gets new drivers instead of
> making existing drivers more generic.
>
> I am just observing this and proposing a "similar copy", which i did
> not even write myself. At some point it might be better to rewrite all
> that and make Super I/Os platforms that discover the chip once and then
> register all the many devices they have. Where ressources are properly
> reserved and not that really weird "superio_enter" with
> "request_muxed_region(base, 2, DRVNAME)" which can be found all over
> the place. But hey that allows a very smooth mix of in-tree and
> out-of-tree.
>

A note on that: the kernel community explicitly has zero regard for
out-of-tree code. :)

Bart

> When reviewing this driver i suggest to measure it against i.e. f7188
> or winbond and maybe others.
>
> Say i would manage to make the nct6116d chip supported by f7188, would
> that be acceptable? I would have the "init pattern" i need without
> copying it. But i would add a Nuvoton chip to a Fintek driver ... might
> be the same family ... no clue.
>
> Henning
>
> > Bart
> >
> > > +
> > > +static void __exit nct6116d_gpio_exit(void)
> > > +{
> > > +       platform_device_unregister(nct6116d_gpio_pdev);
> > > +       platform_driver_unregister(&nct6116d_gpio_driver);
> > > +}
> > > +module_init(nct6116d_gpio_init);
> > > +module_exit(nct6116d_gpio_exit);
> > > +
> > > +MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips
> > > NCT5104D, NCT6106D, NCT6116D, NCT6122D");
> > > +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@xxxxxxxxx>");
> > > +MODULE_LICENSE("GPL"); --
> > > 2.35.1
> > >
>