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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Veg_A+1a9-bHDBEAieXy+7wgLRakg1yGKO=PUO_HyVoOg@mail.gmail.com>
Date:   Tue, 24 Jan 2017 21:39:28 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Platform Driver <platform-driver-x86@...r.kernel.org>,
        Chris Healy <cphealy@...il.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Darren Hart <dvhart@...radead.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: Add IMS/ZII SCU driver

On Sat, Jan 14, 2017 at 1:15 AM, Florian Fainelli <f.fainelli@...il.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@...il.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