[Intel-gfx] [PATCH 1/2] pwm: lpss: Make builtin and add lookup-table so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 05-12-16 11:59, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 09:18:03AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 05-12-16 08:46, Thierry Reding wrote:
>>> On Fri, Dec 02, 2016 at 11:17:35AM +0100, Hans de Goede wrote:
>>>> The primary consumer of the lpss pwm is the i915 kms driver, but
>>>> currently that driver cannot get the pwm because i915 platforms are
>>>> not using devicetree and pwm-lpss does not call pwm_add_table.
>>>>
>>>> Another problem is that i915 does not support get_pwm returning
>>>> -EPROBE_DEFER and i915's init is very complex and this is almost
>>>> impossible to fix.
>>>>
>>>> This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so
>>>> that when the i915 driver loads the lpss pwm will be available avoiding
>>>> the -EPROBE_DEFER issue. Note that this is identical to how the same
>>>> problem was solved for the pwm-crc driver.
>>>>
>>>> Being builtin also allows calling pwm_add_table directly from the
>>>> pwm-lpss code, otherwise the pwm_add_table call would need to be put
>>>> somewhere else to ensure it happens before i915 calls pwm_get,
>>>> even if i915 would support -EPROBE_DEFER.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>>  drivers/pwm/Kconfig    | 12 +++---------
>>>>  drivers/pwm/pwm-lpss.c | 11 +++++++++++
>>>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> This is completely backwards. You're putting board-specific bits into a
>>> generic driver.
>>
>> This is not really board specific I'm advertising that the goal of
>> the pwm is to be used to control a backlight.
>
> pwm_add_table() is a board-specific API. Documentation/pwm.txt says that
> "board setup code" should be using pwm_add_table(). Using it from within
> the provider is completely the opposite.

The problem here really is that there is no such thing as
board setup code on x86 + EFI/ACPI, that is supposedly all
handled by the EFI/ACPI code there.

>> Before typing this reply I've been thinking about another place
>> to put the pwm_add_table call put I cannot come up with any.
>
> I suggested drivers/platform/x86. A bunch of code in there is doing
> exactly the kind of board/platform setup stuff that you're trying to do
> here.

All drivers under drivers/platform/x86 bind to something, be it
an ACPI interface or an actual platform device. In the case of the
pwm-lpss we have an actual platform or pci device and a driver binding
to it, that is the only common code path I see where I can add the
pwm_add_table.

Sure I can put a built-in bit of code under drivers/platform/x86
which checks from its module_init() that there is an pwm-lpss controller
present (either listed under ACPI or through PCI) and then calls
pwm_add_table, but seems silly. Note as said this then must be
built-in, because if it is a module nothing will trigger the
loading of the module, unless I add duplicate MODULE_DEVICE_TABLE
tables in there with the code under drivers/pwm/pwm-lpss.

TL;DR: the problem is that something needs to trigger / activate
the code doing the pwm_add_table() and AFAICT we have no other
trigger then the presence of the pwm-lpss device, at which point
the pwm_lpss_probe function becomes the best place to do the
pwm_add_table call.

Regards,

Hans





>
>>
>> drivers/acpi/acpi_lpss.c comes to mind, but that would only work
>> with the pwm device in acpi mode and not with it in pci mode.
>>
>> Another candidate would be to put the pwm_add_table call in the
>> i915 driver itself, when the vbt says we need to use an external
>> pwm and the plaform is cherrytrail, but it does not know if the
>> pwm device is in pci or acpi modes which requires a different
>> provider entry in the table.
>
> i915 isn't a good location for that either. This should really be driven
> by code external to any generic drivers. It's exactly the kind of glue
> that platform or board setup code is used for.
>
> If you look at drivers/platform/x86/intel_oaktrail.c, it does very
> similar things. If this is specific to Cherry Trail, I'm sure you can
> find ways to identify the platform as such and drive it in a similar way
> from drivers/platform/x86/intel_cherrytrail.c.
>
>> Besides that having the pwm in the table will not do anything
>> unless the i915 driver does a pwm_get, so this does not gain us
>> anything.
>
> It will keep the driver generic and put the board code where it belongs.
> I call that very much of a gain.
>
>> Maybe we need to rename the con_id from "pwm_backlight" to
>> "pwm_lpss0", that way it is very clear which pwm any pwm_get calls
>> are getting (and lpss-pwm.c is obviously the correct place to
>> do the add_table), and then we can teach the i915 code to look
>> for "pwm_lpss0" based on vbt info ?
>
> pwm_backlight is the much better consumer name because by your own words
> that's exactly what the PWM is used for. Obfuscating this by turning the
> name into something unrecognizable such as pwm_lpss0 isn't going to
> change any of the above.
>
> Thierry
>