Re: [PATCH] gpio: winbond: add driver

[Linus Walleij]

On Thu, Dec 21, 2017 at 12:13 AM, Maciej S. Szmigiero
<mail@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On 20.12.2017 13:06, Linus Walleij wrote:
>> On Tue, Dec 19, 2017 at 12:07 AM, Maciej S. Szmigiero
>> <mail@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>> On 18.12.2017 22:22, Linus Walleij wrote:
>>
> (..)
>>>>> +static void winbond_gpio_warn_conflict(unsigned int idx, const char *otherdev)
>>>>> +{
>>>>> +       pr_warn(WB_GPIO_DRIVER_NAME
>>>>> +               ": enabled GPIO%u share pins with active %s\n", idx + 1,
>>>>> +               otherdev);
>>>>> +}
>>>>
>>>> I don't understand this otherdev business, is it pin multiplexing?
>>>
>>> Yes, some GPIO pins are shared with some other devices and functions.
>>>
>>> But this is a x86 Super I/O chip so we don't really have a general
>>> ability to detect which should be configured how (and a BIOS might not
>>> have configured pins that it didn't care about correctly).
>>
>> I see. That is unfortunate, users having to provide module parameters
>> for this is just so 1990ies, not your fault.
>>
>> I'm just wondering if there is something, anything you can grab onto
>> to detect the type of system you're running on and configure accordingly.
>> Like SMBIOS magic. Or subsystem type on the PCI devices. It's
>> scary to have to insert by hand.
>>
>> Am I guessing right when assuming this is not really a slot-in card
>> but more a, say, internal bridge om some misc machines?
>>
>> I'm just thinking that somewhere on this machine there must be some
>> entity that knows what we're running on.
>
> This driver was developed and tested on a Axiomtek CAPA800 board, which
> has a GPIO header allowing easy access to these Super I/O pins.
> A manual for this board specifically documents poking at hardcoded SIO
> I/O space as the way to read / write them, so yes, it is very much a
> 1990's style of operation.
> BTW: DMI data for this board is full of "To Be Filled By O.E.M." as far
> as I can see.

Let's loop in the x86 platform drivers maintainer Andy Shevchenko and
check what he has to say about these beasts.

>> Maybe I misunderstand the usecase: is this an off-the-shelf product
>> support thing (like, to get LEDs on the case of a laptop or read magic
>> switches on a workstation or so) or is it intended for random hacking and
>> special-purpose machines?
>
> Well, currently this driver has just a one "hard" user of CAPA800 board
> (where the GPIO signals themselves are a part of the platform but it is
> left unspecified what is their intended use).
> But since SIO GPIO lines are often used internally to implement switching
> of various motherboard functions (I know motherboards with other SIO
> models use them for LED control, main / backup BIOS swapping, etc.) I
> think random hacking and reverse engineering of motherboard signals is
> also a sensible use case for this driver.

OK

> For things like LED control and input switches reading, where these
> are a permanent part of some off-the-shelf platform, I think a specific
> driver that registers with LED and input layer should be used instead.

Agreed.

>>>>> +                         "push-pull" :
>>>>> +                         "open drain");
>>>>
>>>> If your hardware supports open drain then implement .set_config() for
>>>> it in the gpio_chip so you can use the hardware open drain with the driver.
>>>
>>> The problem here is that the device doesn't support per-pin
>>> output driver configuration, only per-GPIO device (8 pins)
>>> configuration.
>>
>> Hm. That is well supported by pin control while GPIO does not support
>> it since it is line-oriented. Pin control on the other hand, supports
>> group-wise multiplexing and config.
>>
>> I don't know how much you looked into pin control. It might be a bit
>> heavy for an ISA-type device as this appears to be.
>
> Yes, I think pin control is more a thing for complicated SoCs than for
> a few Super I/O chip signals (especially that nothing on any x86
> platform that I have seems to make use of it).

Ah, I bet you will get one sooner or later. All the later x86 Chromebooks
use drivers/pinctrl/intel/* it's their model for future deeply embedded
x86 SoC things as far as I can tell.

> Besides, it would need adding a support for requesting the proper pins
> on boards with this chip to, for example, the 8250 serial driver.

Actually the device core grabs the pin control handles right before
probing the device so in many cases for just default muxing no
code is needed. Dmitry Tarnyagin once pointed out we should do
it that way. (drivers/base/pinctrl.c)

But if you don't need it, no need to worry.

>> But it falls back to the question of autodetect, naturally. This set of
>> parameters is OK if hacking around like this is necessary.
>
> Since random hacking and reverse engineering will probably represent
> the most common use case for this driver I think they should stay.

OK

Yours,
Linus Walleij