by Henning Schild

[permalink] [raw]

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

Am Tue, 19 Jul 2022 11:39:44 +0200
schrieb Bartosz Golaszewski <[email protected]>:

> On Wed, Jul 13, 2022 at 12:39 PM Henning Schild
> <[email protected]> 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.

I am not an expert either, just looked at several similar drivers for
inspiration.

In the next round i will propose an "adding the chip" to an existing
driver. That will greatly reduce the amount of code needed. And we do
not have to copy the pattern, we just reuse the existing one.

> > 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. :)

I know ;). One of the reasons i am here with "my" simatic stuff.

Henning

> 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 <[email protected]>");
> > > > +MODULE_LICENSE("GPL"); --
> > > > 2.35.1
> > > >
> >