[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 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.

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.

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.

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.

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 ?

Regards,

Hans



> And no, this is not the same as pwm-crc because the board-specific bits
> went into the MFD driver. I don't like that very much either, but there
> is documentation that suggests that the Crystal Cove PMIC is used on a
> single platform, whereas there's no such evidence for LPSS.
>
> Why can't you just add something to drivers/platform/x86 to register the
> table? You could base that on some kind of identifier (ACPI, DMI, ...)
> to only do so if necessary.
>
> I could probably live with making this builtin, but your justification
> sounds like a bit of a lame excuse rather than a good technical reason.
>
> Thierry
>