Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 24 Jan 2017 21:39:28 +0200 | Subject | Re: [PATCH] platform/x86: Add IMS/ZII SCU driver |
| |
On Sat, Jan 14, 2017 at 1:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 01/13/2017 08:38 AM, Andy Shevchenko wrote: >> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> 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
| |