Message-ID: <20130701162849.GF10726@kw.sim.vm.gnt>
Date:	Mon, 1 Jul 2013 18:28:50 +0200
From:	Simon Guinot <simon.guinot@...uanux.org>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Grant Likely <grant.likely@...retlab.ca>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Vincent Donnefort <vdonnefort@...il.com>
Subject: Re: [PATCH] gpio: add GPIO support for F71882FG and F71889F

On Sun, Jun 30, 2013 at 01:35:00AM +0200, Linus Walleij wrote:

Hi Linus,

> On Wed, Jun 26, 2013 at 1:56 PM, Simon Guinot <simon.guinot@...uanux.org> wrote:
> 
> > This patch adds support for the GPIOs found on the Fintek Super-I/O
> > chips F71882FG and F71889F.
> >
> > Signed-off-by: Simon Guinot <simon.guinot@...uanux.org>
> 
> Please be more elaborate in the commit message. What kind of beast
> is a Super-I/O chip? Which architecture is this? Some SoC?
> ISA card? etc. References are made to ACPI so I'm just half-guessing...

OK.

> 
> > +++ b/drivers/gpio/gpio-f7188x.c
> 
> > +#define gpio_oe_reg(base) (base + 0)
> > +#define gpio_od_reg(base) (base + 1)
> > +#define gpio_st_reg(base) (base + 2)
> > +#define gpio_de_reg(base) (base + 3)
> 
> What are these four things?
> 
> Output enable, open drain, ...?

I realize I don't have picked self-explanatory macro names :)

oe: output enable
od: output data
st: pin status
de: driver enable

I will fix that for the next version.

> 
> > +static int __init
> > +f7188x_gpio_device_add(const struct f7188x_sio *sio)
> > +{
> > +       int err;
> > +
> > +       f7188x_gpio_pdev = platform_device_alloc(DRVNAME, -1);
> > +       if (!f7188x_gpio_pdev)
> > +               return -ENOMEM;
> > +
> > +       err = platform_device_add_data(f7188x_gpio_pdev,
> > +                                      sio, sizeof(*sio));
> > +       if (err) {
> > +               pr_err(DRVNAME "Platform data allocation failed\n");
> > +               goto err;
> > +       }
> > +
> > +       err = platform_device_add(f7188x_gpio_pdev);
> > +       if (err) {
> > +               pr_err(DRVNAME "Device addition failed\n");
> > +               goto err;
> > +       }
> > +
> > +       return 0;
> > +
> > +err:
> > +       platform_device_put(f7188x_gpio_pdev);
> > +
> > +       return err;
> > +}
> 
> You need to explain with some comment here what is happening
> here. You auto-probe then spawn some devices?

Yes, that's what we are doing here. I will add some comments to explain
that.

> 
> > +static struct platform_driver f7188x_gpio_driver = {
> > +       .driver = {
> > +               .owner  = THIS_MODULE,
> > +               .name   = DRVNAME,
> > +       },
> > +       .probe          = f7188x_gpio_probe,
> > +       .remove         = f7188x_gpio_remove,
> > +};
> > +
> > +static int __init f7188x_gpio_init(void)
> > +{
> > +       int err;
> > +       struct f7188x_sio sio;
> > +
> > +       if (f7188x_find(0x2e, &sio) &&
> > +           f7188x_find(0x4e, &sio))
> > +               return -ENODEV;
> 
> This looks like the life on the ISA-bus. Is that not dangerous?

I guess this looks like the ISA bus because this super-I/O uses the LPC
bus which is ISA-compatible. At my knowledge, reading this I/O ports
and trying to match the vendor and device IDs is the only way to
identify the super-I/O chip. For example, have a look at the drivers
f71882fg (hwmon) and f71808e_wdt (watchdog). Both are using the same
identification mechanism.

> 
> > +
> > +       err = platform_driver_register(&f7188x_gpio_driver);
> > +       if (!err) {
> > +               err = f7188x_gpio_device_add(&sio);
> > +               if (err)
> > +                       platform_driver_unregister(&f7188x_gpio_driver);
> > +       }
> > +
> > +       return err;
> > +}
> > +subsys_initcall(f7188x_gpio_init);
> 
> And this is called unconditionally. Don't you get hints from
> ACPI (given you include that header) as to whether this needs
> to be registered or not?

At my limited knowledge, ACPI is not helpful here. This also points out
an another problem. The ACPI header is not used by this driver... I will
remove it for the next version.

> 
> It looks quite backwards. Isn't there *any* way to tell if the
> system actually has this thing?

This super-I/O is embedded on some LaCie x86-based NAS boards. The GPIOs
are used to control the LEDs and to power up/down the hard disks. All
this platform devices are not very common on x86 boards. They can't be
detected dynamically. One have to know they are available in order to
register their definitions. For x86, the arch/x86/platform/ directory
seems to be the right place to do that.

In a very same way, maybe we could handle the super-I/O GPIO chip as a
true platform device ? If a board needs it, then a static platform
device declaration should be added in the board setup file. This solves
the probe concern about sending inb/outb commands over a port maybe
owned by an unexpected device. As a drawback, the driver is no longer
usable standalone. Some extra platform code is required.

Let me know your advice.

Regards,

Simon

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists