Message-ID: <31aab2cd-a21e-537b-d1e2-dc8c1be62225@maciej.szmigiero.name>
Date:   Thu, 21 Dec 2017 00:13:01 +0100
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     William Breathitt Gray <vilhelm.gray@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-gpio@...r.kernel.org
Subject: Re: [PATCH] gpio: winbond: add driver

On 20.12.2017 13:06, Linus Walleij wrote:
> On Tue, Dec 19, 2017 at 12:07 AM, Maciej S. Szmigiero
> <mail@...iej.szmigiero.name> 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.

See also my answer under the next paragraph below.

>> So the only practical way is to have an user tell us which GPIO devices
>> he wants to use and then enable only these (maybe stealing pins from
>> other devices).
> 
> That is fine, it's the system as a whole that counts.
> 
> 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.

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.

>>>> +       else
>>>> +               pr_notice(WB_GPIO_DRIVER_NAME ": GPIO%u pins are %s\n", idx + 1,
>>>> +                         winbond_sio_reg_btest(base, output_reg,
>>>> +                                               output_pp_bit) ?
>>>> +                         "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).
Besides, it would need adding a support for requesting the proper pins
on boards with this chip to, for example, the 8250 serial driver.

>>> winbond_gpio_configure_uartb()?
>>
>> UARTB is a device that the first GPIO device conflicts with.
>> Should it give a name to this GPIO device configuration function?
>>
>> Datasheet calls this device just "GPIO1" (the driver uses zero-based
>> index internally, however).
> 
> Hm I don't know really. Up to you, at least it should be evident
> from code and comments what is going on.

OK.

>>> winbond_gpio_configure_i2cgpio()?
>>
>> Same issue, should a fact that I2C conflicts with two pins of this GPIO
>> device give its configuration function a "I2C" name?
> 
> I see. Well whatever makes most sense I guess.

OK.

>>> So figure out some way to not have 6 similar functions but parameterize
>>> the functions somehow with some struct or so?
>>
>> Please note that all these 6 functions share only 3 lines of a
>> boilerplate code:
>>>> +       if (!(gpios & BIT(x)))
>>>> +               return;
>>
>> and:
>>>> +       winbond_gpio_configure_common(base, x, WB_SIO_DEV_FOO,
>>>> +                                     WB_SIO_BAR,
>>>> +                                     WB_SIO_XXX,
>>>> +                                     WB_SIO_YYY,
>>>> +                                     WB_SIO_ZZZ);
>>
>> (Okay, that's technically a more than one line, but only due to line
>> wrapping).
> 
> 
> You might not see the forest because a lot of trees standing in your way :)
> 
> I mean a deeper type of table:
> 
> struct winbond_gpio_port {
>     const char *conflicts;
>     u8 selector;
>     u8 testreg;
>     u8 testval;
>     ...
> };
> 
> static const struct winbond_gpio_port ports[] = {
>     {
>         .conflicts = "UARTB",
>         .selector = WB_SIO_DEV_UARTB,
>         .testreg = WB_SIO_UARTB_REG_ENABLE,
>         .testval = WB_SIO_UARTB_ENABLE_ON,
>         ...
>     },
> };
> 
> (...)
> for (i = 0; i < ARRAY_SIZE(ports); i++) {
>    struct winbond_gpio_port *port = &ports[i];
> 
>    winbond_sio_select_logical(bas, port->selector);
>    if (winbond_sio_reg_btest(bas, port->testreg, port->testval))
>       winbond_gpio_warn_conflict(i, port->conflicts);
> };
> 
> Just follow this pattern and also include the registers and values etc
> for winbond_gpio_config_common() etc and I am pretty sure you can
> cut it down to a neat table.

I understand now what you had in mind - will do it this way then (it
looks like this will also solve the function naming problem above).

>> Rest of the code is unique for a particular GPIO device and
>> the code common for all these devices is already in
>> winbond_gpio_configure_common().
>>
>> Naturally, it can be made into a one, parametrized function, but at a
>> cost of adding a few additional "if" blocks inside - won't that make it
>> less readable than separate ones?
> 
> I'm not sure you see the option of encoding all magic values and register
> offsets into the table?

Yes, it is clear now.

>>>> +module_param(gpios, byte, 0444);
>>>> +MODULE_PARM_DESC(gpios,
>>>> +                "bitmask of GPIO ports to enable (bit 0 - GPIO1, bit 1 - GPIO2, etc.");
>>>> +
>>
>> This sets which GPIO devices (ports) we enable.
>>
>>>> +module_param(ppgpios, byte, 0444);
>>>> +MODULE_PARM_DESC(ppgpios,
>>>> +                "bitmask of GPIO ports to set to push-pull mode (bit 0 - GPIO1, bit 1 - GPIO2, etc.");
>>>> +
>>>> +module_param(odgpios, byte, 0444);
>>>> +MODULE_PARM_DESC(odgpios,
>>>> +                "bitmask of GPIO ports to set to open drain mode (bit 0 - GPIO1, bit 1 - GPIO2, etc.");
>>
>> These two above configure which GPIO devices (ports) we enable.
>> It can't be a one bitmask since we need three values per port: push-pull,
>> open-drain and keep as-is.
>>
>>>> +module_param(pledgpio, bool, 0644);
>>>> +MODULE_PARM_DESC(pledgpio,
>>>> +                "enable changing value of GPIO2.0 bit (Power LED), default no.");
>>>> +
>>>> +module_param(beepgpio, bool, 0644);
>>>> +MODULE_PARM_DESC(beepgpio,
>>>> +                "enable changing value of GPIO2.1 bit (BEEP), default no.");
>>
>> These two control a basic PC functionality that I don't think we should
>> allow tinkering with by default (it is very likely that the firmware
>> owns these pins).
> 
> At least all the text above should be comments in the code so it is
> crystal clear how to use the parameters for anyone daring enough?

Will add these comments to the code.

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

> Yours,
> Linus Walleij
> 

Best regards,
Maciej Szmigiero

Powered by blists - more mailing lists