linux-kernel - Re: [PATCH v2 6/7] platform: x86: Add intel_skl

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]

Date:   Tue, 19 Jan 2021 00:11:40 +0000
From:   Daniel Scally <djrscally@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, devel@...ica.org,
        rjw@...ysocki.net, lenb@...nel.org, andy@...nel.org,
        mika.westerberg@...ux.intel.com, linus.walleij@...aro.org,
        bgolaszewski@...libre.com, wsa@...nel.org, lee.jones@...aro.org,
        hdegoede@...hat.com, mgross@...ux.intel.com,
        robert.moore@...el.com, erik.kaneda@...el.com,
        sakari.ailus@...ux.intel.com, kieran.bingham@...asonboard.com
Subject: Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver

Hi Andy, Laurent

On 18/01/2021 21:19, Daniel Scally wrote:
>>>> +static const struct clk_ops skl_int3472_clock_ops = {
>>>> +	.prepare = skl_int3472_clk_prepare,
>>>> +	.unprepare = skl_int3472_clk_unprepare,
>>>> +	.enable = skl_int3472_clk_enable,
>>>> +	.disable = skl_int3472_clk_disable,
>>>> +};
>> Yeah, sounds like reinventing clk-gpio.c.
>>
>> static const struct clk_ops clk_gpio_gate_ops = {
>> 	.enable = clk_gpio_gate_enable,
>> 	.disable = clk_gpio_gate_disable,
>> 	.is_enabled = clk_gpio_gate_is_enabled,
>> };
>>
>> (Or is it mux? It has support there as well.
>>
> Hmm, yeah, this looks like it would work actually. So I think I'd need to:
>
>
> 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver
>
> 2. Register a platform device to bind to the clk-gpio driver
>
> 3. Register a gpio lookup table so that the clk-gpio driver can find the
> gpio in question using gpiod_get()
>
>
> And that looks like it will work; I'll try it.

I'm more and more confident that this will work, but it has some
knock-on effects:

The both clk and regulator gpio driver expects to be able to fetch the
GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of
course, so we need to add another GPIO lookup table so those drivers can
see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will
be so we can set the .dev_id member of a gpiod_lookup_table to that
value, but that isn't set until _after_ the pdev is registered (because
it has to figure out the id, we can't manually set the IDs because there
could be more than one instance of int3472-discrete bound to multiple
PMIC devices, and we don't know which id the current one should have).
Finally, we can't wait until the device is registered because it
immediately probes, can't find the GPIO and then fails probe.

It's similar problem that causes us to need the i2c-acpi name format
macros, but complicated by the dynamic ID part of dev_name(&pdev->dev)

Solving it is a bit of a sticky one; perhaps something like moving the
dev_set_name() part of platform_device_add() [1] to its own function,
that's called in both platform_device_alloc() and
platform_device_register(). That way it would be available before the
device itself was registered, meaning we could create the lookup table
before it probes the driver.

(also, Laurent, if we did it this way we wouldn't be able to also handle
the led-indicator GPIO here without some fairly major rework)

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L563