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

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

[Hans de Goede]

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(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index bf01288..cda31ea 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -240,28 +240,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
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 72c0bce..b4d8835 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -161,6 +161,12 @@ static const struct pwm_ops pwm_lpss_ops = {
 	.owner = THIS_MODULE,
 };
 
+/* PWM consumed by the Intel GFX */
+static struct pwm_lookup pwm_lpss_lookup[] = {
+	PWM_LOOKUP("pwm-lpss", 0, "0000:00:02.0", "pwm_backlight", 0,
+		   PWM_POLARITY_NORMAL),
+};
+
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 				     const struct pwm_lpss_boardinfo *info)
 {
@@ -193,12 +199,17 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 		return ERR_PTR(ret);
 	}
 
+	/* Add lookup table for pwm_backlight */
+	pwm_lpss_lookup[0].provider = dev_name(dev);
+	pwm_add_table(pwm_lpss_lookup, ARRAY_SIZE(pwm_lpss_lookup));
+
 	return lpwm;
 }
 EXPORT_SYMBOL_GPL(pwm_lpss_probe);
 
 int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
 {
+	pwm_remove_table(pwm_lpss_lookup, ARRAY_SIZE(pwm_lpss_lookup));
 	return pwmchip_remove(&lpwm->chip);
 }
 EXPORT_SYMBOL_GPL(pwm_lpss_remove);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] pwm: lpss: Add get_state callback

[Hans de Goede]

Add a get_state callback so that the initial state correctly reflects
the actual hardware state.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index b4d8835..f5603c0 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -135,6 +135,33 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+	unsigned long base_unit_range, freq;
+	unsigned long long base_unit, on_time_div;
+	u32 ctrl;
+
+	base_unit_range = BIT(lpwm->info->base_unit_bits);
+
+	ctrl = pwm_lpss_read(pwm);
+	on_time_div = ctrl & PWM_ON_TIME_DIV_MASK;
+	base_unit = (ctrl >> PWM_BASE_UNIT_SHIFT) & (base_unit_range - 1);
+
+	freq = base_unit * lpwm->info->clk_rate / base_unit_range;
+	if (freq == 0)
+		freq = 1;
+
+	state->period = NSEC_PER_SEC / freq;
+	state->duty_cycle = state->period * on_time_div / 255ULL;
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = (ctrl & PWM_ENABLE) ? true : false;
+
+	if (state->enabled)
+		pm_runtime_get_sync(chip->dev);
+}
+
 static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	pm_runtime_get_sync(chip->dev);
@@ -156,6 +183,7 @@ static void pwm_lpss_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static const struct pwm_ops pwm_lpss_ops = {
 	.config = pwm_lpss_config,
+	.get_state = pwm_lpss_get_state,
 	.enable = pwm_lpss_enable,
 	.disable = pwm_lpss_disable,
 	.owner = THIS_MODULE,
-- 
2.9.3

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

[Jani Nikula]

On Fri, 02 Dec 2016, Hans de Goede <hdegoede@redhat.com> 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.

Arguably this solution was worse for pwm-crc than pwm-lpss here, because
bool CONFIG_PWM_CRC depends on INTEL_SOC_PMIC which depends on I2C being
built-in. This one doesn't have all that bad dependencies.

FWIW, both patches are

Acked-by: Jani Nikula <jani.nikula@intel.com>



>
> 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(-)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index bf01288..cda31ea 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -240,28 +240,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
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 72c0bce..b4d8835 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -161,6 +161,12 @@ static const struct pwm_ops pwm_lpss_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +/* PWM consumed by the Intel GFX */
> +static struct pwm_lookup pwm_lpss_lookup[] = {
> +	PWM_LOOKUP("pwm-lpss", 0, "0000:00:02.0", "pwm_backlight", 0,
> +		   PWM_POLARITY_NORMAL),
> +};
> +
>  struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
>  				     const struct pwm_lpss_boardinfo *info)
>  {
> @@ -193,12 +199,17 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
>  		return ERR_PTR(ret);
>  	}
>  
> +	/* Add lookup table for pwm_backlight */
> +	pwm_lpss_lookup[0].provider = dev_name(dev);
> +	pwm_add_table(pwm_lpss_lookup, ARRAY_SIZE(pwm_lpss_lookup));
> +
>  	return lpwm;
>  }
>  EXPORT_SYMBOL_GPL(pwm_lpss_probe);
>  
>  int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>  {
> +	pwm_remove_table(pwm_lpss_lookup, ARRAY_SIZE(pwm_lpss_lookup));
>  	return pwmchip_remove(&lpwm->chip);
>  }
>  EXPORT_SYMBOL_GPL(pwm_lpss_remove);

-- 
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 2/2] pwm: lpss: Add get_state callback

[Thierry Reding]

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

On Fri, Dec 02, 2016 at 11:17:36AM +0100, Hans de Goede wrote:
> Add a get_state callback so that the initial state correctly reflects
> the actual hardware state.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Please don't mix legacy with atomic PWM. There's a little bit of mixing
in the core for transitional purposes, but if you want to deal with PWM
state, please also replace the ->enable(), ->disable() and ->config()
implementations with ->apply().

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 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 1/2] pwm: lpss: Make builtin and add lookup-table so that i915 can find the pwm_backlight

[Thierry Reding]

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

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.

And no, this is not the same as pwm-crc because the board-specific bits
went into the MFD driver. I don't like that very much either, but there
is documentation that suggests that the Crystal Cove PMIC is used on a
single platform, whereas there's no such evidence for LPSS.

Why can't you just add something to drivers/platform/x86 to register the
table? You could base that on some kind of identifier (ACPI, DMI, ...)
to only do so if necessary.

I could probably live with making this builtin, but your justification
sounds like a bit of a lame excuse rather than a good technical reason.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 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 1/2] pwm: lpss: Make builtin and add lookup-table so that i915 can find the pwm_backlight

[Hans de Goede]

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.

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.

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.

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.

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 ?

Regards,

Hans



> And no, this is not the same as pwm-crc because the board-specific bits
> went into the MFD driver. I don't like that very much either, but there
> is documentation that suggests that the Crystal Cove PMIC is used on a
> single platform, whereas there's no such evidence for LPSS.
>
> Why can't you just add something to drivers/platform/x86 to register the
> table? You could base that on some kind of identifier (ACPI, DMI, ...)
> to only do so if necessary.
>
> I could probably live with making this builtin, but your justification
> sounds like a bit of a lame excuse rather than a good technical reason.
>
> Thierry
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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

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.

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

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 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 1/2] pwm: lpss: Make builtin and add lookup-table so that i915 can find the pwm_backlight

[Hans de Goede]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Hans de Goede]

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