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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 05-12-16 14:23, Hans de Goede wrote:
> 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@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.

ping ?

Can I please get an answer to the above. I'm happy to put the
pwm_add_table call somewhere-else if that is what it takes to
get these fixes merged, but I don't see any obvious other place
to put this. So can you please tell me where to put the
pwm_add_table call, if not in pwm-lpss.c ? Note as said before
it should be triggered by the acpi-ids used to bind
the pwm-lpss driver, which to me really makes the pwm-lpss
driver the best place to do it.

Regards,

Hans