[PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

The primary consumer of the lpss pwm is the i915 kms driver,
the i915 driver does not support get_pwm returning -EPROBE_DEFER and
its init is very complex making 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, which is used by the i915
driver on other platforms.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
Changes in v2:
-Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
---
 drivers/pwm/Kconfig | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index f92dd41..12a6cf8 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -249,28 +249,22 @@ config PWM_LPC32XX
 	  will be called pwm-lpc32xx.
 
 config PWM_LPSS
-	tristate
+	bool
 
 config PWM_LPSS_PCI
-	tristate "Intel LPSS PWM PCI driver"
+	bool "Intel LPSS PWM PCI driver"
 	depends on X86 && PCI
 	select PWM_LPSS
 	help
 	  The PCI driver for Intel Low Power Subsystem PWM controller.
 
-	  To compile this driver as a module, choose M here: the module
-	  will be called pwm-lpss-pci.
-
 config PWM_LPSS_PLATFORM
-	tristate "Intel LPSS PWM platform driver"
+	bool "Intel LPSS PWM platform driver"
 	depends on X86 && ACPI
 	select PWM_LPSS
 	help
 	  The platform driver for Intel Low Power Subsystem PWM controller.
 
-	  To compile this driver as a module, choose M here: the module
-	  will be called pwm-lpss-platform.
-
 config PWM_MESON
 	tristate "Amlogic Meson PWM driver"
 	depends on ARCH_MESON
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1442 bytes --]

On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
> The primary consumer of the lpss pwm is the i915 kms driver,
> the i915 driver does not support get_pwm returning -EPROBE_DEFER and
> its init is very complex making 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, which is used by the i915
> driver on other platforms.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
> Changes in v2:
> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
> ---
>  drivers/pwm/Kconfig | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)

For the record I think this is completely wrong and i915 should be
taught how to deal with -EPROBE_DEFER. We've gone through a lot of
pain to clean up this kind of init-level ordering on other devices
and the result is, in my opinion, a *lot* better than what we had
before. It'd be shame to see i915 backpedal on that.

That said, if everyone else thinks that it really can't be done and
this workaround is the best way forward, I'll just shut up about it
and stop caring.

Mika, Andy, any objections to this?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2274 bytes --]

On Fri, Jan 20, 2017 at 08:03:33AM +0100, Thierry Reding wrote:
> On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
> > The primary consumer of the lpss pwm is the i915 kms driver,
> > the i915 driver does not support get_pwm returning -EPROBE_DEFER and
> > its init is very complex making 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, which is used by the i915
> > driver on other platforms.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> > Changes in v2:
> > -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
> > ---
> >  drivers/pwm/Kconfig | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> For the record I think this is completely wrong and i915 should be
> taught how to deal with -EPROBE_DEFER. We've gone through a lot of
> pain to clean up this kind of init-level ordering on other devices
> and the result is, in my opinion, a *lot* better than what we had
> before. It'd be shame to see i915 backpedal on that.

Looking into i915 a little, I don't see why handling -EPROBE_DEFER would
be very complicated. The call stack looks somewhat like this:

	i915_pci_probe()
	i915_driver_load()
	i915_load_modeset_init()
	intel_modeset_init()
	intel_setup_outputs()
	intel_dsi_init()
	intel_panel_setup_backlight()
	pwm_setup_backlight()

In the above, intel_modeset_init() is the last one that propagates
errors, but its caller, i915_load_modeset_init() properly handles
failure from other function calls. Also, pwm_setup_backlight() can
return errors to intel_panel_setup_backlight(), which will in turn
propagate them to intel_dsi_init().

So I'd think that in order to properly handle -EPROBE_DEFER you'd only
need to propagate errors back up this way:

	intel_panel_setup_backlight()
	intel_dsi_init()
	intel_setup_outputs()
	intel_modeset_init()

That seems to me to be far from "almost impossible".

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 20-01-17 08:18, Thierry Reding wrote:
> On Fri, Jan 20, 2017 at 08:03:33AM +0100, Thierry Reding wrote:
>> On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
>>> The primary consumer of the lpss pwm is the i915 kms driver,
>>> the i915 driver does not support get_pwm returning -EPROBE_DEFER and
>>> its init is very complex making 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, which is used by the i915
>>> driver on other platforms.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>> Changes in v2:
>>> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
>>> ---
>>>  drivers/pwm/Kconfig | 12 +++---------
>>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> For the record I think this is completely wrong and i915 should be
>> taught how to deal with -EPROBE_DEFER. We've gone through a lot of
>> pain to clean up this kind of init-level ordering on other devices
>> and the result is, in my opinion, a *lot* better than what we had
>> before. It'd be shame to see i915 backpedal on that.
>
> Looking into i915 a little, I don't see why handling -EPROBE_DEFER would
> be very complicated. The call stack looks somewhat like this:
>
> 	i915_pci_probe()
> 	i915_driver_load()
> 	i915_load_modeset_init()
> 	intel_modeset_init()
> 	intel_setup_outputs()
> 	intel_dsi_init()
> 	intel_panel_setup_backlight()
> 	pwm_setup_backlight()
>
> In the above, intel_modeset_init() is the last one that propagates
> errors, but its caller, i915_load_modeset_init() properly handles
> failure from other function calls. Also, pwm_setup_backlight() can
> return errors to intel_panel_setup_backlight(), which will in turn
> propagate them to intel_dsi_init().
>
> So I'd think that in order to properly handle -EPROBE_DEFER you'd only
> need to propagate errors back up this way:
>
> 	intel_panel_setup_backlight()
> 	intel_dsi_init()
> 	intel_setup_outputs()
> 	intel_modeset_init()
>
> That seems to me to be far from "almost impossible".

I will let the i915 devs answer this.

IIRC a lot of setup is already done at this point, including some changes
to the hw and we really do not want to change the init-sequence, or
repeat parts of it.

I myself was actually thinking about solving this by figuring out if the
pwm is necessary and getting it early on, but the figuring out bit is non
trivial.

Anyways as said I will let the i915 devs answer this.

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Jani Nikula]

On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
>> The primary consumer of the lpss pwm is the i915 kms driver,
>> the i915 driver does not support get_pwm returning -EPROBE_DEFER and
>> its init is very complex making 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, which is used by the i915
>> driver on other platforms.
>> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> Changes in v2:
>> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
>> ---
>>  drivers/pwm/Kconfig | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> For the record I think this is completely wrong and i915 should be
> taught how to deal with -EPROBE_DEFER. We've gone through a lot of
> pain to clean up this kind of init-level ordering on other devices
> and the result is, in my opinion, a *lot* better than what we had
> before. It'd be shame to see i915 backpedal on that.
>
> That said, if everyone else thinks that it really can't be done and
> this workaround is the best way forward, I'll just shut up about it
> and stop caring.

Superficially, it is, of course, easy to agree we should handle deferred
probing.

i915 is a complex driver for complex hardware. We require a ton of
initialization before we even get to the point we realize we might need
the PWM. Naturally, we'd need to gracefully tear all that down for
-EPROBE_DEFER handling. And we've been slowly working towards this;
we've even added injected probe failures in CI to test this. But we're
not there yet. This patch seems like a rather simple workaround for the
time being.

There are two other related things I wonder about.

I see module reloading mostly as a developer feature. I don't think I'm
alone in that. You just don't recommend anyone doing module reloads in a
production environment. However, deferred probing is in some ways more
demanding than module reload, because it needs to gracefully handle
partial probes. Yet that is the solution of choice for init
ordering. Makes you wonder.

Another thing that comes up a lot with graphics is that people really do
appreciate any crappy degraded image over a black screen. If the PWM
never shows up, all the external screens will be black in addition to
the embedded display. We're always torn between failing fast
vs. plunging on despite failures.

---

That said, I suppose there could be an alternative to handling pwm_get()
failures at probe. We could just go on with our init, but schedule a
retry later. Perhaps a bit hacky, but it would address both of the
concerns above. Again, this patch seems a simple workaround in the mean
time.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5398 bytes --]

On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
> On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
> >> The primary consumer of the lpss pwm is the i915 kms driver,
> >> the i915 driver does not support get_pwm returning -EPROBE_DEFER and
> >> its init is very complex making 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, which is used by the i915
> >> driver on other platforms.
> >> 
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> Acked-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >> Changes in v2:
> >> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
> >> ---
> >>  drivers/pwm/Kconfig | 12 +++---------
> >>  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > For the record I think this is completely wrong and i915 should be
> > taught how to deal with -EPROBE_DEFER. We've gone through a lot of
> > pain to clean up this kind of init-level ordering on other devices
> > and the result is, in my opinion, a *lot* better than what we had
> > before. It'd be shame to see i915 backpedal on that.
> >
> > That said, if everyone else thinks that it really can't be done and
> > this workaround is the best way forward, I'll just shut up about it
> > and stop caring.
> 
> Superficially, it is, of course, easy to agree we should handle deferred
> probing.
> 
> i915 is a complex driver for complex hardware. We require a ton of
> initialization before we even get to the point we realize we might need
> the PWM. Naturally, we'd need to gracefully tear all that down for
> -EPROBE_DEFER handling. And we've been slowly working towards this;
> we've even added injected probe failures in CI to test this. But we're
> not there yet. This patch seems like a rather simple workaround for the
> time being.
> 
> There are two other related things I wonder about.
> 
> I see module reloading mostly as a developer feature. I don't think I'm
> alone in that. You just don't recommend anyone doing module reloads in a
> production environment. However, deferred probing is in some ways more
> demanding than module reload, because it needs to gracefully handle
> partial probes. Yet that is the solution of choice for init
> ordering. Makes you wonder.

Well, there have been proposals in the past to get rid of deferred
probing by replacing it with something more formal, but it's a fairly
difficult issue to solve. While deferred probing is indeed a rather
heavy-weight solution, it's one that's proven to work well enough for
most of the world.

Also gracefully handling partial probes is something you need in most
cases anyway. Typically the easiest solution is to make sure to run all
the code that could possibly fail as early as possible, like Hans had
suggested, so that you need to unwind as little as possible.

> Another thing that comes up a lot with graphics is that people really do
> appreciate any crappy degraded image over a black screen. If the PWM
> never shows up, all the external screens will be black in addition to
> the embedded display. We're always torn between failing fast
> vs. plunging on despite failures.

Yes, I see how deferred probe would get in the way here. To be fair,
deferred probing was never meant to solve this kind of use-case. One
of the reasons why it is so heavy-weight is that drivers can usually
not continue without the resources they're trying to get.

In this particular case, however, only a very small subset of the driver
relies on the PWM, so it's more of an optional, nice to have, resource
rather than an essential one.

> That said, I suppose there could be an alternative to handling pwm_get()
> failures at probe. We could just go on with our init, but schedule a
> retry later. Perhaps a bit hacky, but it would address both of the
> concerns above. Again, this patch seems a simple workaround in the mean
> time.

I don't think that's hacky at all. In fact I think it's a really nice
solution for this particular case and could probably be a good fit for
other use-cases as well.

As for adding a simple workaround in the meantime, is that really
necessary? This doesn't really fix any bugs, right? Its just that new
hardware may not work properly, isn't it? I'm somewhat reluctant to
temporarily add this workaround because I know Paul Gortmaker will
immediately send out a patch to make the driver use builtin_{pci,
platform}_driver and all of a sudden we've got a bunch of things to
untangle because of a "simple workaround".

Hans, do you think you could find some time to at least investigate
whether or not Jani's proposal above would be a viable option that
wouldn't take ages to implement? If it's excessively complicated to
do, I'm okay with this patch, though you may want to do the module_*
to builtin_* conversion while at it to save Paul the extra work. And
maybe add a comment somewhere that this is meant to be a temporary
workaround until i915 can deal with this more nicely?

Thanks,
Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 20-01-17 09:56, Thierry Reding wrote:
> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
>> On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
>>>> The primary consumer of the lpss pwm is the i915 kms driver,
>>>> the i915 driver does not support get_pwm returning -EPROBE_DEFER and
>>>> its init is very complex making 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, which is used by the i915
>>>> driver on other platforms.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>>> ---
>>>> Changes in v2:
>>>> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
>>>> ---
>>>>  drivers/pwm/Kconfig | 12 +++---------
>>>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>>
>>> For the record I think this is completely wrong and i915 should be
>>> taught how to deal with -EPROBE_DEFER. We've gone through a lot of
>>> pain to clean up this kind of init-level ordering on other devices
>>> and the result is, in my opinion, a *lot* better than what we had
>>> before. It'd be shame to see i915 backpedal on that.
>>>
>>> That said, if everyone else thinks that it really can't be done and
>>> this workaround is the best way forward, I'll just shut up about it
>>> and stop caring.
>>
>> Superficially, it is, of course, easy to agree we should handle deferred
>> probing.
>>
>> i915 is a complex driver for complex hardware. We require a ton of
>> initialization before we even get to the point we realize we might need
>> the PWM. Naturally, we'd need to gracefully tear all that down for
>> -EPROBE_DEFER handling. And we've been slowly working towards this;
>> we've even added injected probe failures in CI to test this. But we're
>> not there yet. This patch seems like a rather simple workaround for the
>> time being.
>>
>> There are two other related things I wonder about.
>>
>> I see module reloading mostly as a developer feature. I don't think I'm
>> alone in that. You just don't recommend anyone doing module reloads in a
>> production environment. However, deferred probing is in some ways more
>> demanding than module reload, because it needs to gracefully handle
>> partial probes. Yet that is the solution of choice for init
>> ordering. Makes you wonder.
>
> Well, there have been proposals in the past to get rid of deferred
> probing by replacing it with something more formal, but it's a fairly
> difficult issue to solve. While deferred probing is indeed a rather
> heavy-weight solution, it's one that's proven to work well enough for
> most of the world.
>
> Also gracefully handling partial probes is something you need in most
> cases anyway. Typically the easiest solution is to make sure to run all
> the code that could possibly fail as early as possible, like Hans had
> suggested, so that you need to unwind as little as possible.
>
>> Another thing that comes up a lot with graphics is that people really do
>> appreciate any crappy degraded image over a black screen. If the PWM
>> never shows up, all the external screens will be black in addition to
>> the embedded display. We're always torn between failing fast
>> vs. plunging on despite failures.
>
> Yes, I see how deferred probe would get in the way here. To be fair,
> deferred probing was never meant to solve this kind of use-case. One
> of the reasons why it is so heavy-weight is that drivers can usually
> not continue without the resources they're trying to get.
>
> In this particular case, however, only a very small subset of the driver
> relies on the PWM, so it's more of an optional, nice to have, resource
> rather than an essential one.
>
>> That said, I suppose there could be an alternative to handling pwm_get()
>> failures at probe. We could just go on with our init, but schedule a
>> retry later. Perhaps a bit hacky, but it would address both of the
>> concerns above. Again, this patch seems a simple workaround in the mean
>> time.
>
> I don't think that's hacky at all. In fact I think it's a really nice
> solution for this particular case and could probably be a good fit for
> other use-cases as well.
>
> As for adding a simple workaround in the meantime, is that really
> necessary? This doesn't really fix any bugs, right?

It fixes the bug of not being able to control the backlight on many
cherrytrail devices.

> Its just that new
> hardware may not work properly, isn't it? I'm somewhat reluctant to
> temporarily add this workaround because I know Paul Gortmaker will
> immediately send out a patch to make the driver use builtin_{pci,
> platform}_driver and all of a sudden we've got a bunch of things to
> untangle because of a "simple workaround".
>
> Hans, do you think you could find some time to at least investigate
> whether or not Jani's proposal above would be a viable option that
> wouldn't take ages to implement?

The whole backlight situation on x86 is much more complicated then
it has any right to be I'm afraid. If the i915 driver does not register
a backlight interface right away, then the acpi-video driver will
register a backlight interface instead (*), which may or may not work
and if userspace manages to try and use that interface before the
i915 driver gets around to register its own interface the firmware
may mess up some i915 or pwm_lpss register settings in such a way
that the backlight will later never work, flicker, be inverted,
whatever.

*) Which will get unregistered again when the i915 driver does
register its own backlight later, yes I kid you not.

The whole firmware interface to the backlight stuff on x86 is
a horrendous mess (I know I've spend a significant amount of time
the last couple of years making it mostly work and adding dmi based
quirks where necessary).

 > If it's excessively complicated to do,

It is probably doable, but I'm very much against trick with this
because of what I've written above. Ah this also brings up another
problem with the i915 driver init order. On systems without an
i915 vbios opregion, the acpi-video bus driver will immediately
enumerate that "bus" and register e.g. backlight device(s) based
on acpi tables. But when an i915 vbios opregion is present,
the acpi-video bus modules' init function will just return 0 and
expects the i915 driver to call its probe function later, after
the i915 opregion's init function has run, because otherwise
bad things may happen.

I've not yet checked if this call to acpi_video's probe function
happens before or after we try to get the pwm, but if it happens
before, undoing that is also a problem ...

 > I'm okay with this patch, though you may want to do the module_*
> to builtin_* conversion while at it to save Paul the extra work. And
> maybe add a comment somewhere that this is meant to be a temporary
> workaround until i915 can deal with this more nicely?

I'm fine with doing a v3 with a comment, how about putting that comment
right at all the module* stuff and explain there that that is to
stay as the builtin only status is meant to be temporary ?

Regards,

Hans

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Andy Shevchenko]

On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote:
> I'm fine with doing a v3 with a comment, how about putting that
> comment
> right at all the module* stuff and explain there that that is to
> stay as the builtin only status is meant to be temporary ?

Can we do other way around? I mean that either i915 selects PWM_LPSS to
be built-in, or uses request_module() call?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 20-01-17 10:55, Andy Shevchenko wrote:
> On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote:
>> I'm fine with doing a v3 with a comment, how about putting that
>> comment
>> right at all the module* stuff and explain there that that is to
>> stay as the builtin only status is meant to be temporary ?
>
> Can we do other way around? I mean that either i915 selects PWM_LPSS to
> be built-in, or uses request_module() call?

PWM_LPSS needs to be built-in if enabled, a stripped down kernel
for non cherrytrail hardware does not need it ...

Also (and esp for request_module) this means building knowledge
into the i915 driver about which pwm hardware there is on which
boards which is undesirable.

Thierry, this does give me an idea though, what if we extend
the info passed to pwm_add_table with a module-name and
make pwm_get call request_module() ?

That seems like a good solution and we could then also make
PWM_CRC a tristate again (after someone with hw access has tested
this).

Regards,

Hans



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1551 bytes --]

On Fri, Jan 20, 2017 at 11:18:29AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 20-01-17 10:55, Andy Shevchenko wrote:
> > On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote:
> > > I'm fine with doing a v3 with a comment, how about putting that
> > > comment
> > > right at all the module* stuff and explain there that that is to
> > > stay as the builtin only status is meant to be temporary ?
> > 
> > Can we do other way around? I mean that either i915 selects PWM_LPSS to
> > be built-in, or uses request_module() call?
> 
> PWM_LPSS needs to be built-in if enabled, a stripped down kernel
> for non cherrytrail hardware does not need it ...
> 
> Also (and esp for request_module) this means building knowledge
> into the i915 driver about which pwm hardware there is on which
> boards which is undesirable.
> 
> Thierry, this does give me an idea though, what if we extend
> the info passed to pwm_add_table with a module-name and
> make pwm_get call request_module() ?

I'm not sure that's even necessary. request_module() forwards the string
you pass to it to the userspace helper, so you can pass things like the
modalias to it. I suspect that for ACPI the modalias could be trivially
derived from the provider name already in the table.

Or maybe it couldn't. We don't have information about the type of device
and the name doesn't match, or at least it would be much more difficult
to extend the same method to other types of devices. Module name doesn't
sound too bad, as long as it's optional.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 20-01-17 11:42, Thierry Reding wrote:
> On Fri, Jan 20, 2017 at 11:18:29AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 20-01-17 10:55, Andy Shevchenko wrote:
>>> On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote:
>>>> I'm fine with doing a v3 with a comment, how about putting that
>>>> comment
>>>> right at all the module* stuff and explain there that that is to
>>>> stay as the builtin only status is meant to be temporary ?
>>>
>>> Can we do other way around? I mean that either i915 selects PWM_LPSS to
>>> be built-in, or uses request_module() call?
>>
>> PWM_LPSS needs to be built-in if enabled, a stripped down kernel
>> for non cherrytrail hardware does not need it ...
>>
>> Also (and esp for request_module) this means building knowledge
>> into the i915 driver about which pwm hardware there is on which
>> boards which is undesirable.
>>
>> Thierry, this does give me an idea though, what if we extend
>> the info passed to pwm_add_table with a module-name and
>> make pwm_get call request_module() ?
>
> I'm not sure that's even necessary. request_module() forwards the string
> you pass to it to the userspace helper, so you can pass things like the
> modalias to it. I suspect that for ACPI the modalias could be trivially
> derived from the provider name already in the table.

As you can see in the patch-set I've just send I've chosen to go with the
module_name in pwm_lookup. You're right that simply doing:

	module_request("acpi:%s", chosen->provider);

Would work, but I don't like having the "acpi:%s" bit in the pwm-core,
esp. not since someone will them come along and add support for i2c.
pci, etc. So just adding a module_name field seems saner.

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 8796 bytes --]

On Fri, Jan 20, 2017 at 10:48:52AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 20-01-17 09:56, Thierry Reding wrote:
> > On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
> > > On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote:
> > > > > The primary consumer of the lpss pwm is the i915 kms driver,
> > > > > the i915 driver does not support get_pwm returning -EPROBE_DEFER and
> > > > > its init is very complex making 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, which is used by the i915
> > > > > driver on other platforms.
> > > > > 
> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > Acked-by: Jani Nikula <jani.nikula@intel.com>
> > > > > ---
> > > > > Changes in v2:
> > > > > -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver)
> > > > > ---
> > > > >  drivers/pwm/Kconfig | 12 +++---------
> > > > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > > > 
> > > > For the record I think this is completely wrong and i915 should be
> > > > taught how to deal with -EPROBE_DEFER. We've gone through a lot of
> > > > pain to clean up this kind of init-level ordering on other devices
> > > > and the result is, in my opinion, a *lot* better than what we had
> > > > before. It'd be shame to see i915 backpedal on that.
> > > > 
> > > > That said, if everyone else thinks that it really can't be done and
> > > > this workaround is the best way forward, I'll just shut up about it
> > > > and stop caring.
> > > 
> > > Superficially, it is, of course, easy to agree we should handle deferred
> > > probing.
> > > 
> > > i915 is a complex driver for complex hardware. We require a ton of
> > > initialization before we even get to the point we realize we might need
> > > the PWM. Naturally, we'd need to gracefully tear all that down for
> > > -EPROBE_DEFER handling. And we've been slowly working towards this;
> > > we've even added injected probe failures in CI to test this. But we're
> > > not there yet. This patch seems like a rather simple workaround for the
> > > time being.
> > > 
> > > There are two other related things I wonder about.
> > > 
> > > I see module reloading mostly as a developer feature. I don't think I'm
> > > alone in that. You just don't recommend anyone doing module reloads in a
> > > production environment. However, deferred probing is in some ways more
> > > demanding than module reload, because it needs to gracefully handle
> > > partial probes. Yet that is the solution of choice for init
> > > ordering. Makes you wonder.
> > 
> > Well, there have been proposals in the past to get rid of deferred
> > probing by replacing it with something more formal, but it's a fairly
> > difficult issue to solve. While deferred probing is indeed a rather
> > heavy-weight solution, it's one that's proven to work well enough for
> > most of the world.
> > 
> > Also gracefully handling partial probes is something you need in most
> > cases anyway. Typically the easiest solution is to make sure to run all
> > the code that could possibly fail as early as possible, like Hans had
> > suggested, so that you need to unwind as little as possible.
> > 
> > > Another thing that comes up a lot with graphics is that people really do
> > > appreciate any crappy degraded image over a black screen. If the PWM
> > > never shows up, all the external screens will be black in addition to
> > > the embedded display. We're always torn between failing fast
> > > vs. plunging on despite failures.
> > 
> > Yes, I see how deferred probe would get in the way here. To be fair,
> > deferred probing was never meant to solve this kind of use-case. One
> > of the reasons why it is so heavy-weight is that drivers can usually
> > not continue without the resources they're trying to get.
> > 
> > In this particular case, however, only a very small subset of the driver
> > relies on the PWM, so it's more of an optional, nice to have, resource
> > rather than an essential one.
> > 
> > > That said, I suppose there could be an alternative to handling pwm_get()
> > > failures at probe. We could just go on with our init, but schedule a
> > > retry later. Perhaps a bit hacky, but it would address both of the
> > > concerns above. Again, this patch seems a simple workaround in the mean
> > > time.
> > 
> > I don't think that's hacky at all. In fact I think it's a really nice
> > solution for this particular case and could probably be a good fit for
> > other use-cases as well.
> > 
> > As for adding a simple workaround in the meantime, is that really
> > necessary? This doesn't really fix any bugs, right?
> 
> It fixes the bug of not being able to control the backlight on many
> cherrytrail devices.

Okay, but it's not a regression because this clearly can't have worked
before, either. What I'm trying to say is that a workaround is much more
acceptable if it fixes a regression. For new features we try to do
things properly, even if they are complicated.

> > Its just that new
> > hardware may not work properly, isn't it? I'm somewhat reluctant to
> > temporarily add this workaround because I know Paul Gortmaker will
> > immediately send out a patch to make the driver use builtin_{pci,
> > platform}_driver and all of a sudden we've got a bunch of things to
> > untangle because of a "simple workaround".
> > 
> > Hans, do you think you could find some time to at least investigate
> > whether or not Jani's proposal above would be a viable option that
> > wouldn't take ages to implement?
> 
> The whole backlight situation on x86 is much more complicated then
> it has any right to be I'm afraid. If the i915 driver does not register
> a backlight interface right away, then the acpi-video driver will
> register a backlight interface instead (*), which may or may not work
> and if userspace manages to try and use that interface before the
> i915 driver gets around to register its own interface the firmware
> may mess up some i915 or pwm_lpss register settings in such a way
> that the backlight will later never work, flicker, be inverted,
> whatever.
> 
> *) Which will get unregistered again when the i915 driver does
> register its own backlight later, yes I kid you not.
> 
> The whole firmware interface to the backlight stuff on x86 is
> a horrendous mess (I know I've spend a significant amount of time
> the last couple of years making it mostly work and adding dmi based
> quirks where necessary).

I don't understand why people keep saying that ARM is messy...

> > If it's excessively complicated to do,
> 
> It is probably doable, but I'm very much against trick with this
> because of what I've written above. Ah this also brings up another
> problem with the i915 driver init order. On systems without an
> i915 vbios opregion, the acpi-video bus driver will immediately
> enumerate that "bus" and register e.g. backlight device(s) based
> on acpi tables. But when an i915 vbios opregion is present,
> the acpi-video bus modules' init function will just return 0 and
> expects the i915 driver to call its probe function later, after
> the i915 opregion's init function has run, because otherwise
> bad things may happen.
> 
> I've not yet checked if this call to acpi_video's probe function
> happens before or after we try to get the pwm, but if it happens
> before, undoing that is also a problem ...
> 
> > I'm okay with this patch, though you may want to do the module_*
> > to builtin_* conversion while at it to save Paul the extra work. And
> > maybe add a comment somewhere that this is meant to be a temporary
> > workaround until i915 can deal with this more nicely?
> 
> I'm fine with doing a v3 with a comment, how about putting that comment
> right at all the module* stuff and explain there that that is to
> stay as the builtin only status is meant to be temporary ?

Given the above and the extent of whackiness involved here, do you
really think the state will be temporary? If it isn't then there's
no need to add a comment that we'll never get rid of again.

I'll leave it up to you whether or not you want to add the comment
or convert to builtin_*, but if you do the former, right near the
module_* seems fine. I'll do my best to fend off Paul until you've
fixed the remaining root causes. =)

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Mika Westerberg]

On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
> That said, I suppose there could be an alternative to handling pwm_get()
> failures at probe. We could just go on with our init, but schedule a
> retry later. Perhaps a bit hacky, but it would address both of the
> concerns above. Again, this patch seems a simple workaround in the mean
> time.

Not sure if this works or how hacky it is, but can't you
request_module() before you start looking up for the pwm?

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Jani Nikula]

On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
>> That said, I suppose there could be an alternative to handling pwm_get()
>> failures at probe. We could just go on with our init, but schedule a
>> retry later. Perhaps a bit hacky, but it would address both of the
>> concerns above. Again, this patch seems a simple workaround in the mean
>> time.
>
> Not sure if this works or how hacky it is, but can't you
> request_module() before you start looking up for the pwm?

I eyeballed this a little, and noticed:

drivers/acpi/acpi_lpss.c:

static struct pwm_lookup bsw_pwm_lookup[] = {
	PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0",
			       "pwm_backlight", 0, PWM_POLARITY_NORMAL,
			       "pwm-lpss-platform"),
};

drivers/mfd/intel_soc_pmic_core.c:

static struct pwm_lookup crc_pwm_lookup[] = {
	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL),
};

Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module
exactly? pwm_get() does an automatic request_module(), if the module is
given.

And will this still be enough?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 08-03-17 10:40, Jani Nikula wrote:
> On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
>>> That said, I suppose there could be an alternative to handling pwm_get()
>>> failures at probe. We could just go on with our init, but schedule a
>>> retry later. Perhaps a bit hacky, but it would address both of the
>>> concerns above. Again, this patch seems a simple workaround in the mean
>>> time.
>>
>> Not sure if this works or how hacky it is, but can't you
>> request_module() before you start looking up for the pwm?
>
> I eyeballed this a little, and noticed:
>
> drivers/acpi/acpi_lpss.c:
>
> static struct pwm_lookup bsw_pwm_lookup[] = {
> 	PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0",
> 			       "pwm_backlight", 0, PWM_POLARITY_NORMAL,
> 			       "pwm-lpss-platform"),
> };
>
> drivers/mfd/intel_soc_pmic_core.c:
>
> static struct pwm_lookup crc_pwm_lookup[] = {
> 	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL),
> };
>
> Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module
> exactly? pwm_get() does an automatic request_module(), if the module is
> given.
>
> And will this still be enough?

I was thinking about this a couple of days ago, unfortunately the
situation with pwm_crc is more complicated as that is part of
an i2c mfd device, so both the i2c-adapter driver and the mfd driver
(intel_soc_pmic_crc) need to be builtin currently the mfd driver
cannot be modular, but the i2c-adapter driver can (and on most
Linux distribution kernels is) configured to be modular.

So just doing request module for pwm-crc is not going to help
since the i2c-adapter driver may not yet have loaded / initialized.

All in all we really need to find a way to figure out if we will need
to do a pwm_get earlier on during i915 initialization (by moving the
VBT parsing to earlier, or at least part of it) and do the pwm_get
before we do anything i-reversible and if it fails then return
-EPROBE_DEFER. Then we can make pwm_crc modular as well as all of
intel_soc_pmic* as it really should be.

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Jani Nikula]

On Wed, 08 Mar 2017, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08-03-17 10:40, Jani Nikula wrote:
>> On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>>> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
>>>> That said, I suppose there could be an alternative to handling pwm_get()
>>>> failures at probe. We could just go on with our init, but schedule a
>>>> retry later. Perhaps a bit hacky, but it would address both of the
>>>> concerns above. Again, this patch seems a simple workaround in the mean
>>>> time.
>>>
>>> Not sure if this works or how hacky it is, but can't you
>>> request_module() before you start looking up for the pwm?
>>
>> I eyeballed this a little, and noticed:
>>
>> drivers/acpi/acpi_lpss.c:
>>
>> static struct pwm_lookup bsw_pwm_lookup[] = {
>> 	PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0",
>> 			       "pwm_backlight", 0, PWM_POLARITY_NORMAL,
>> 			       "pwm-lpss-platform"),
>> };
>>
>> drivers/mfd/intel_soc_pmic_core.c:
>>
>> static struct pwm_lookup crc_pwm_lookup[] = {
>> 	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL),
>> };
>>
>> Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module
>> exactly? pwm_get() does an automatic request_module(), if the module is
>> given.
>>
>> And will this still be enough?
>
> I was thinking about this a couple of days ago, unfortunately the
> situation with pwm_crc is more complicated as that is part of
> an i2c mfd device, so both the i2c-adapter driver and the mfd driver
> (intel_soc_pmic_crc) need to be builtin currently the mfd driver
> cannot be modular, but the i2c-adapter driver can (and on most
> Linux distribution kernels is) configured to be modular.
>
> So just doing request module for pwm-crc is not going to help
> since the i2c-adapter driver may not yet have loaded / initialized.
>
> All in all we really need to find a way to figure out if we will need
> to do a pwm_get earlier on during i915 initialization (by moving the
> VBT parsing to earlier, or at least part of it) and do the pwm_get
> before we do anything i-reversible and if it fails then return
> -EPROBE_DEFER. Then we can make pwm_crc modular as well as all of
> intel_soc_pmic* as it really should be.

The other alternatives are:

1) Handle defers using a workqueue within i915. It's a bit tedious, but
I didn't spot any show stoppers with the approach. We'd register a
non-functional backlight interface until the pwm_get() succeeds.

2) Rip out pwm backlight from i915 altogether, and turn it into a
separate platform backlight that uses pwm. I think there's ready
infrastructure for that. It's not without problems, though, as then we
lose control over the sequence in which backlight gets enabled/disabled.

BR,
Jani.

>
> Regards,
>
> Hans

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] pwm: lpss: Make builtin so that i915 can find the pwm_backlight

[Hans de Goede]

Hi,

On 08-03-17 11:15, Jani Nikula wrote:
> On Wed, 08 Mar 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 08-03-17 10:40, Jani Nikula wrote:
>>> On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>>>> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote:
>>>>> That said, I suppose there could be an alternative to handling pwm_get()
>>>>> failures at probe. We could just go on with our init, but schedule a
>>>>> retry later. Perhaps a bit hacky, but it would address both of the
>>>>> concerns above. Again, this patch seems a simple workaround in the mean
>>>>> time.
>>>>
>>>> Not sure if this works or how hacky it is, but can't you
>>>> request_module() before you start looking up for the pwm?
>>>
>>> I eyeballed this a little, and noticed:
>>>
>>> drivers/acpi/acpi_lpss.c:
>>>
>>> static struct pwm_lookup bsw_pwm_lookup[] = {
>>> 	PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0",
>>> 			       "pwm_backlight", 0, PWM_POLARITY_NORMAL,
>>> 			       "pwm-lpss-platform"),
>>> };
>>>
>>> drivers/mfd/intel_soc_pmic_core.c:
>>>
>>> static struct pwm_lookup crc_pwm_lookup[] = {
>>> 	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL),
>>> };
>>>
>>> Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module
>>> exactly? pwm_get() does an automatic request_module(), if the module is
>>> given.
>>>
>>> And will this still be enough?
>>
>> I was thinking about this a couple of days ago, unfortunately the
>> situation with pwm_crc is more complicated as that is part of
>> an i2c mfd device, so both the i2c-adapter driver and the mfd driver
>> (intel_soc_pmic_crc) need to be builtin currently the mfd driver
>> cannot be modular, but the i2c-adapter driver can (and on most
>> Linux distribution kernels is) configured to be modular.
>>
>> So just doing request module for pwm-crc is not going to help
>> since the i2c-adapter driver may not yet have loaded / initialized.
>>
>> All in all we really need to find a way to figure out if we will need
>> to do a pwm_get earlier on during i915 initialization (by moving the
>> VBT parsing to earlier, or at least part of it) and do the pwm_get
>> before we do anything i-reversible and if it fails then return
>> -EPROBE_DEFER. Then we can make pwm_crc modular as well as all of
>> intel_soc_pmic* as it really should be.
>
> The other alternatives are:
>
> 1) Handle defers using a workqueue within i915. It's a bit tedious, but
> I didn't spot any show stoppers with the approach. We'd register a
> non-functional backlight interface until the pwm_get() succeeds.

Ack, although I would prefer for i915 just to properly at deferred
probing handling instead of this hack. But if doing the VBT parsing
earlier so we know in advance if we will need the pwm or not turns
out to be really tricky we could go with this instead.

> 2) Rip out pwm backlight from i915 altogether, and turn it into a
> separate platform backlight that uses pwm. I think there's ready
> infrastructure for that. It's not without problems, though, as then we
> lose control over the sequence in which backlight gets enabled/disabled.

Yeah, not a fan of this.

Regards,

Hans