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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jan 14, 2017 at 1:15 AM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> On 01/13/2017 08:38 AM, Andy Shevchenko wrote:
>> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>>> This patch adds support for the IMS (now Zodiac Inflight Innovations)
>>> SCU Generation 1/2/3 platform driver. This driver registers all the
>>> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
>>> GPIO expanders, Kontrom CPLD/I2C master, and more.

>> I'm going to review this later, though few of comments.

Sorry, it takes a bit longer. Below my answers.

>> Does it sound like MFD driver + individual drivers?
>
> My understanding of a valid MFD candidate driver is that is partitions a
> shared resource space into individual resources that can all be managed
> by their respective driver. The KemPLD driver is already a MFD driver,
> here, this is more of a superset of all of that and an aggregate
> x86-based module that has a number of on-board peripherals.

So, what is common here to make mandatory to keep all those in one module then?

>>> +       bool eeprom_accessible;
>>> +       bool eeprom_valid;
>>
>> unsigned int flag1:1;
>> unsigned int flag2:1;
>>
>> ?
>
> 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?

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

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux