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