Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Wed, Dec 4, 2013 at 1:23 AM, Bruno Randolf <br1@einfach.org> wrote:

> This patch adds support for the GPIOs found on the SMSC "Super I/O" chips
> SCH311x.
>
> The chip detection and I/O functions are copied from sch311x_wdt.c
>
> Signed-off-by: Bruno Randolf <br1@einfach.org>
>
> ---
> Notes:
> - All GPIOs are now supported, driver data is runtime allocated
>   and I tried to address all comments as far as possible.
> - Still a platform device is registered, similar to gpio-f7188x.c

So I have a problem with this design pattern so I need to ask
Matthew Garret about this:

- Driver *always* performs some port-mapped I/O probe on
  some random ports as a compulsory initcall.

- No other hardware discovery.

- If detecting magic, registers a platform device.

- Then handles this device by also providing a device
  driver for it.

- All of this is placed in one file.

Matthew is this how discovery and registration of x86 platform
drivers for port-mapped devices should work or are we doing
something weird here?

Since earlier we have:

drivers/gpio/gpio-f7188x.c - exactly the same pattern

drivers/gpio/gpio-it8761e.c - doesn't even bother to create a platform
  device, goes on and creates a gpio_chip without any device

drivers/gpio/gpio-sch.c - port-mapped but platform device is
  created by an MFD which is probed from PCI (seems fine).

drivers/gpio/gpio-ts5500.c - aha, created from
  arch/x86/platform/ts5500/ts5500.c, a "board file".

This is a bit heterogeneous, but I can't claim to understand how
x86 want to register its devices so need some input here.

Maybe this is all the right way to do things, but I want to
be hammered down by some convincing arguments first.

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Matthew Garrett]

On Tue, Dec 10, 2013 at 12:51:30PM +0100, Linus Walleij wrote:

> Matthew is this how discovery and registration of x86 platform
> drivers for port-mapped devices should work or are we doing
> something weird here?

This is pretty awful, but typical. LPC devices *should* be present in 
ACPI to some extent, but often you'll just find (at best) a reference to 
a superIO chip and no child devices. Do we have an example DSDT for a 
device with this part?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Vivien Didelot]

Hi Linus,

You wrote:
> > This patch adds support for the GPIOs found on the SMSC "Super I/O"
> > chips
> > SCH311x.
> >
> > The chip detection and I/O functions are copied from sch311x_wdt.c
> >
> > Signed-off-by: Bruno Randolf <br1@einfach.org>
> >
> > ---
> > Notes:
> > - All GPIOs are now supported, driver data is runtime allocated
> >   and I tried to address all comments as far as possible.
> > - Still a platform device is registered, similar to gpio-f7188x.c
> 
> So I have a problem with this design pattern so I need to ask
> Matthew Garret about this:
> 
> - Driver *always* performs some port-mapped I/O probe on
>   some random ports as a compulsory initcall.
> 
> - No other hardware discovery.
> 
> - If detecting magic, registers a platform device.
> 
> - Then handles this device by also providing a device
>   driver for it.
> 
> - All of this is placed in one file.
> 
> Matthew is this how discovery and registration of x86 platform
> drivers for port-mapped devices should work or are we doing
> something weird here?
> 
> Since earlier we have:
> 
> drivers/gpio/gpio-f7188x.c - exactly the same pattern
> 
> drivers/gpio/gpio-it8761e.c - doesn't even bother to create a
> platform
>   device, goes on and creates a gpio_chip without any device
> 
> drivers/gpio/gpio-sch.c - port-mapped but platform device is
>   created by an MFD which is probed from PCI (seems fine).
> 
> drivers/gpio/gpio-ts5500.c - aha, created from
>   arch/x86/platform/ts5500/ts5500.c, a "board file".

Because of the lack of mechanism such as DMI, there's no safe way to verify the
machine at the GPIO driver level. However, the board code (which registers the
platform device) does a safe check by looking for some magic in the RAM.

Would it be safer if we make GPIO_TS5500 depends on TS5500?

> This is a bit heterogeneous, but I can't claim to understand how
> x86 want to register its devices so need some input here.
> 
> Maybe this is all the right way to do things, but I want to
> be hammered down by some convincing arguments first.

Best,
Vivien

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Wed, Dec 11, 2013 at 5:05 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
>[Me]
>> drivers/gpio/gpio-ts5500.c - aha, created from
>>   arch/x86/platform/ts5500/ts5500.c, a "board file".
>
> Because of the lack of mechanism such as DMI, there's no safe way to verify the
> machine at the GPIO driver level. However, the board code (which registers the
> platform device) does a safe check by looking for some magic in the RAM.
>
> Would it be safer if we make GPIO_TS5500 depends on TS5500?

I don't know actually. How does it work with these x86 things?
Is it so that the system comes up and then you modprobe this driver?

In that case, do you bring up a fully generic kernel or do you have
to have it compiled with CONFIG_TS5500 set to 'y' and is that
done of a say, typical distro kernel?

If we don't know if it will be modprobed or not on a certain system
we should keep it as is, but of the distro kernels all set
CONFIG_TS5500 to 'n' then we should add a dependency
like that.

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Vivien Didelot]

Hi Linus,

You wrote:
> >> drivers/gpio/gpio-ts5500.c - aha, created from
> >>   arch/x86/platform/ts5500/ts5500.c, a "board file".
> >
> > Because of the lack of mechanism such as DMI, there's no safe way
> > to verify the
> > machine at the GPIO driver level. However, the board code (which
> > registers the
> > platform device) does a safe check by looking for some magic in the
> > RAM.
> >
> > Would it be safer if we make GPIO_TS5500 depends on TS5500?
> 
> I don't know actually. How does it work with these x86 things?
> Is it so that the system comes up and then you modprobe this driver?

With TS5500 selected, the board comes up, get probed reading the RAM
(from the board code), then registers this GPIO platform device.

> In that case, do you bring up a fully generic kernel or do you have
> to have it compiled with CONFIG_TS5500 set to 'y' and is that
> done of a say, typical distro kernel?

GPIO_TS5500 cannot probe the DIO blocks by itself. It needs a board
code to fill and register the corresponding platform device structures.

So a generic x86 kernel will work on the board, but we need TS5500 set
to 'y' in order to have the GPIOs.

> If we don't know if it will be modprobed or not on a certain system
> we should keep it as is, but of the distro kernels all set
> CONFIG_TS5500 to 'n' then we should add a dependency
> like that.

As it is an embedded platform, I don't think CONFIG_TS5500 is enabled
on distro kernels.

I hope it's a bit clearer. Let me know what is your advice on this.

Best,
Vivien

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Mon, Dec 16, 2013 at 3:46 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> [Me]
>> If we don't know if it will be modprobed or not on a certain system
>> we should keep it as is, but of the distro kernels all set
>> CONFIG_TS5500 to 'n' then we should add a dependency
>> like that.
>
> As it is an embedded platform, I don't think CONFIG_TS5500 is enabled
> on distro kernels.
>
> I hope it's a bit clearer. Let me know what is your advice on this.

Hm I think right now we should not fix what ain't broken and leave
it as it is. The module is not on any distros I've seen so they're
already avoiding to shit it it seems.

Yours,
Linus Walleij