Re: [PATCH] platform/x86: Add IMS/ZII SCU driver

[Florian Fainelli]

On 01/24/2017 11:39 AM, Andy Shevchenko wrote:
>> Are you concerned with storage, or what motivates the preference for
>> bitfields vs. bools?
> 
> Just matter of style. Up to you.
> 
>>>> +       {                       /* bit 5 */
>>>> +        .name = "SD6:a:error",
>>>> +        .gpio = SCU_SD_ERROR_6_GPIO,
>>>> +        .active_low = 1,
>>>> +        .default_trigger = "none",
>>>> +        .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>>> +       }
>>>> +};
>>>
>>> Hmm... Can you utilize device tree for that?
>>
>> Not really an option here
> 
> Why not?

The platform is several years old, it is currently deployed with a boot
loader that does not support passing a Device Tree blob pointer in one
of the special e820 regions, I looked into that before, but I am not
proficient enough with grub's source code to make that happen nor do I
think this is going to be worth the trouble for these specific platforms.

Last I worked on x86 and Device Tree on CE4100 (2013 or so) there was
quite a lot of ad-hoc and not so generic code early FDT/OF
initialization code that had to be provided which AFAIR would hinder the
multiplatform capability of the kernel on x86 if there was a second DT
enabled platform that would show up. I did started that route, and it
did not look pretty, also, I would have to make *all* drivers used by
this platform (kempld and friends) to become DT aware, and make sure
that they do propagate of_node/child of_node correctly, major pain.

Finally, writing a complete Device Tree for this platform is a major
pain because a lot of the peripherals are being a PCI host bridge and
some of them are several levels deep (SPI over I2C expander over CPLD
over...).

> 
>>> Or built-in device properties?
>>
>> Not clear what you mean by that, can you expand?
> 
> include/linux/properties.h (platform data in a form of DT resource provider)
> 
>>>> +static void pca_leds_register(struct device *parent,
>>>> +                             struct scu_data *data)
>>>> +{
>>>> +       data->leds_pdev[0] =
>>>> +               platform_device_register_data(parent, "leds-gpio", 1,
>>>> +                                             &pca_gpio_led_info1,
>>>> +                                             sizeof(pca_gpio_led_info1));
>>>> +       data->leds_pdev[1] =
>>>> +               platform_device_register_data(parent, "leds-gpio", 2,
>>>> +                                             &pca_gpio_led_info2,
>>>> +                                             sizeof(pca_gpio_led_info2));
>>>> +       data->leds_pdev[2] =
>>>> +               platform_device_register_data(parent, "leds-gpio", 3,
>>>> +                                             &pca_gpio_led_info3,
>>>> +                                             sizeof(pca_gpio_led_info3));
>>>> +}
>>>
>>> It really sounds like MFD to me.
>>
>> It's more of a board description of attached peripherals (all of them),
>> more than a multi-function device, the whole module is by nature, "multi
>> function" since it has a bunch of different I/Os and on-module peripherals.
> 
> So, there is neither ACPI, nor DT provider for such resources?

Nope.

> We used to have a hell of a time to handle board files for Intel MID
> platforms under arch/x86/platform/intel-mid (formerly known as
> mrst.c), so, I don't like the idea of board files.

I understand the pain, but we also have to work with the tools we have,
and neither DT nor ACPI are such tools at the moment for this platform.

Thanks
-- 
Florian