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