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 >
