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
