[PATCH v2 0/3] acpi_lpss/pwm: Fix i915 pwm_get issues

[PATCH v2 0/3] acpi_lpss/pwm: Fix i915 pwm_get issues

[Hans de Goede]

Hi All,

Here is another attempt at fixing i915 not finding the lpss_pwm
device (fixed by calling pwm_table_add from apci_lpss.c) as well
as i915 not being capable of pwm_get returning -EPROBE_DEFER.

I must sat I rather like this version (vs forcing pwm-lpss* to be
builtin), so I hope we can move forward to this.

As the acpi/lpss changes (patch 3/3) now rely on some core pwm
changes (the first 2 patches) I suggest simply merging all 3 patches
through the pwm tree. Rafael if that is ok with you, can we have
your Acked-by please ?

Regards,

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

[PATCH v2 1/3] pwm/core: Don't hold pwm_lookup_lock longer then necessary

[Hans de Goede]

There is no need to hold pwm_lookup_lock after we're done with
looping over pwm_lookup_list, so release it earlier.

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

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 172ef82..0d3ef29 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -757,9 +757,9 @@ void pwm_remove_table(struct pwm_lookup *table, size_t num)
  */
 struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 {
-	struct pwm_device *pwm = ERR_PTR(-EPROBE_DEFER);
 	const char *dev_id = dev ? dev_name(dev) : NULL;
-	struct pwm_chip *chip = NULL;
+	struct pwm_device *pwm;
+	struct pwm_chip *chip;
 	unsigned int best = 0;
 	struct pwm_lookup *p, *chosen = NULL;
 	unsigned int match;
@@ -817,24 +817,22 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 		}
 	}
 
-	if (!chosen) {
-		pwm = ERR_PTR(-ENODEV);
-		goto out;
-	}
+	mutex_unlock(&pwm_lookup_lock);
+
+	if (!chosen)
+		return ERR_PTR(-ENODEV);
 
 	chip = pwmchip_find_by_name(chosen->provider);
 	if (!chip)
-		goto out;
+		return ERR_PTR(-EPROBE_DEFER);
 
 	pwm = pwm_request_from_chip(chip, chosen->index, con_id ?: dev_id);
 	if (IS_ERR(pwm))
-		goto out;
+		return pwm;
 
 	pwm->args.period = chosen->period;
 	pwm->args.polarity = chosen->polarity;
 
-out:
-	mutex_unlock(&pwm_lookup_lock);
 	return pwm;
 }
 EXPORT_SYMBOL_GPL(pwm_get);
-- 
2.9.3

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

[PATCH v2 2/3] pwm/core: Try to get the module from pwm_get

[Hans de Goede]

Add a module_name string to the pwm_lookup struct and if specified
and pwmchip_find_by_name() does not find the pwmchip try calling
request_module with the specified name.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/core.c  |  4 ++++
 include/linux/pwm.h | 11 +++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 0d3ef29..c418a7a 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -823,6 +823,10 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 		return ERR_PTR(-ENODEV);
 
 	chip = pwmchip_find_by_name(chosen->provider);
+	if (!chip && chosen->module_name) {
+		request_module(chosen->module_name);
+		chip = pwmchip_find_by_name(chosen->provider);
+	}
 	if (!chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2c6c511..40ab8b6 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -613,18 +613,25 @@ struct pwm_lookup {
 	const char *con_id;
 	unsigned int period;
 	enum pwm_polarity polarity;
+	const char *module_name; /* Optional may be NULL */
 };
 
-#define PWM_LOOKUP(_provider, _index, _dev_id, _con_id, _period, _polarity) \
+#define PWM_LOOKUP_MODNAME(_provider, _index, _dev_id, _con_id, _period, \
+			   _polarity, _module_name) \
 	{						\
 		.provider = _provider,			\
 		.index = _index,			\
 		.dev_id = _dev_id,			\
 		.con_id = _con_id,			\
 		.period = _period,			\
-		.polarity = _polarity			\
+		.polarity = _polarity,			\
+		.module_name = _module_name		\
 	}
 
+#define PWM_LOOKUP(_provider, _index, _dev_id, _con_id, _period, _polarity) \
+	PWM_LOOKUP_MODNAME(_provider, _index, _dev_id, _con_id, _period, \
+			   _polarity, NULL)
+
 #if IS_ENABLED(CONFIG_PWM)
 void pwm_add_table(struct pwm_lookup *table, size_t num);
 void pwm_remove_table(struct pwm_lookup *table, size_t num);
-- 
2.9.3

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

Re: [PATCH v2 2/3] pwm/core: Try to get the module from pwm_get

[Thierry Reding]

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

On Sun, Jan 22, 2017 at 05:14:08PM +0100, Hans de Goede wrote:
> Add a module_name string to the pwm_lookup struct and if specified
> and pwmchip_find_by_name() does not find the pwmchip try calling
> request_module with the specified name.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/core.c  |  4 ++++
>  include/linux/pwm.h | 11 +++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 0d3ef29..c418a7a 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -823,6 +823,10 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>  		return ERR_PTR(-ENODEV);
>  
>  	chip = pwmchip_find_by_name(chosen->provider);
> +	if (!chip && chosen->module_name) {
> +		request_module(chosen->module_name);

How about checking for an error code here? It's kind of pointless to try
again if the module didn't load in the first place. No need to respin, I
can make that change as I apply.

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 2/3] pwm/core: Try to get the module from pwm_get

[Hans de Goede]

Hi,

On 30-01-17 09:17, Thierry Reding wrote:
> On Sun, Jan 22, 2017 at 05:14:08PM +0100, Hans de Goede wrote:
>> Add a module_name string to the pwm_lookup struct and if specified
>> and pwmchip_find_by_name() does not find the pwmchip try calling
>> request_module with the specified name.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/pwm/core.c  |  4 ++++
>>  include/linux/pwm.h | 11 +++++++++--
>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 0d3ef29..c418a7a 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -823,6 +823,10 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>>  		return ERR_PTR(-ENODEV);
>>
>>  	chip = pwmchip_find_by_name(chosen->provider);
>> +	if (!chip && chosen->module_name) {
>> +		request_module(chosen->module_name);
>
> How about checking for an error code here? It's kind of pointless to try
> again if the module didn't load in the first place. No need to respin, I
> can make that change as I apply.

Sure, that is fine.

Regards,

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

[PATCH v2 3/3] acpi: lpss: call pwm_add_table() for bsw PWM device

[Hans de Goede]

On x86 we do not have devicetree to link the PWM controller and
the display controller together. So someone needs to call
pwm_add_table() to create the link, so that the i915 driver's
pwm_get(dev, "pwm_backlight") call returns the lpss' pwm0.

The PWM subsystem does not want to have pwm_add_table() calls
directly in PWM drivers (this leads to probe ordering issues),
so lets do it here since the acpi-lpss code is always builtin.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
---
Changes in v2:
-Set new pwm_lookup module_name field in the table
---
 drivers/acpi/acpi_lpss.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 8ea836c..794be64 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -20,6 +20,7 @@
 #include <linux/platform_data/clk-lpss.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/pwm.h>
 #include <linux/delay.h>
 
 #include "internal.h"
@@ -154,6 +155,17 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
 	writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
 }
 
+/* BSW PWM used for backlight control by the i915 driver */
+static struct pwm_lookup bsw_pwm_lookup[] = {
+	PWM_LOOKUP_MODNAME("80862288:00", 0, "0000:00:02.0", "pwm_backlight",
+			   0, PWM_POLARITY_NORMAL, "pwm-lpss-platform"),
+};
+
+static void bsw_pwm_setup(struct lpss_private_data *pdata)
+{
+	pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
+}
+
 static const struct lpss_device_desc lpt_dev_desc = {
 	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
 	.prv_offset = 0x800,
@@ -191,6 +203,7 @@ static const struct lpss_device_desc byt_pwm_dev_desc = {
 
 static const struct lpss_device_desc bsw_pwm_dev_desc = {
 	.flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY,
+	.setup = bsw_pwm_setup,
 };
 
 static const struct lpss_device_desc byt_uart_dev_desc = {
-- 
2.9.3

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

Re: [PATCH v2 3/3] acpi: lpss: call pwm_add_table() for bsw PWM device

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On Sun, Jan 22, 2017 at 05:14:09PM +0100, Hans de Goede wrote:
> On x86 we do not have devicetree to link the PWM controller and
> the display controller together. So someone needs to call
> pwm_add_table() to create the link, so that the i915 driver's
> pwm_get(dev, "pwm_backlight") call returns the lpss' pwm0.
> 
> The PWM subsystem does not want to have pwm_add_table() calls
> directly in PWM drivers (this leads to probe ordering issues),
> so lets do it here since the acpi-lpss code is always builtin.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> ---
> Changes in v2:
> -Set new pwm_lookup module_name field in the table
> ---
>  drivers/acpi/acpi_lpss.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Rafael, do you mind if I take this through the PWM tree to resolve the
build-time dependency?

Thanks,
Thierry

> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 8ea836c..794be64 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_data/clk-lpss.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
>  #include <linux/delay.h>
>  
>  #include "internal.h"
> @@ -154,6 +155,17 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
>  	writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
>  }
>  
> +/* BSW PWM used for backlight control by the i915 driver */
> +static struct pwm_lookup bsw_pwm_lookup[] = {
> +	PWM_LOOKUP_MODNAME("80862288:00", 0, "0000:00:02.0", "pwm_backlight",
> +			   0, PWM_POLARITY_NORMAL, "pwm-lpss-platform"),
> +};
> +
> +static void bsw_pwm_setup(struct lpss_private_data *pdata)
> +{
> +	pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
> +}
> +
>  static const struct lpss_device_desc lpt_dev_desc = {
>  	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
>  	.prv_offset = 0x800,
> @@ -191,6 +203,7 @@ static const struct lpss_device_desc byt_pwm_dev_desc = {
>  
>  static const struct lpss_device_desc bsw_pwm_dev_desc = {
>  	.flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY,
> +	.setup = bsw_pwm_setup,
>  };
>  
>  static const struct lpss_device_desc byt_uart_dev_desc = {
> -- 
> 2.9.3
> 

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

Re: [PATCH v2 3/3] acpi: lpss: call pwm_add_table() for bsw PWM device

[Rafael J. Wysocki]

On Mon, Jan 30, 2017 at 9:18 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Sun, Jan 22, 2017 at 05:14:09PM +0100, Hans de Goede wrote:
>> On x86 we do not have devicetree to link the PWM controller and
>> the display controller together. So someone needs to call
>> pwm_add_table() to create the link, so that the i915 driver's
>> pwm_get(dev, "pwm_backlight") call returns the lpss' pwm0.
>>
>> The PWM subsystem does not want to have pwm_add_table() calls
>> directly in PWM drivers (this leads to probe ordering issues),
>> so lets do it here since the acpi-lpss code is always builtin.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Thierry Reding <thierry.reding@gmail.com>
>> ---
>> Changes in v2:
>> -Set new pwm_lookup module_name field in the table
>> ---
>>  drivers/acpi/acpi_lpss.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>
> Rafael, do you mind if I take this through the PWM tree to resolve the
> build-time dependency?

No, I don't, please go ahead, unless Mika/Andy see any problems in it.

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

Re: [PATCH v2 3/3] acpi: lpss: call pwm_add_table() for bsw PWM device

[Mika Westerberg]

On Mon, Jan 30, 2017 at 09:28:09AM +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 30, 2017 at 9:18 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Sun, Jan 22, 2017 at 05:14:09PM +0100, Hans de Goede wrote:
> >> On x86 we do not have devicetree to link the PWM controller and
> >> the display controller together. So someone needs to call
> >> pwm_add_table() to create the link, so that the i915 driver's
> >> pwm_get(dev, "pwm_backlight") call returns the lpss' pwm0.
> >>
> >> The PWM subsystem does not want to have pwm_add_table() calls
> >> directly in PWM drivers (this leads to probe ordering issues),
> >> so lets do it here since the acpi-lpss code is always builtin.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> >> ---
> >> Changes in v2:
> >> -Set new pwm_lookup module_name field in the table
> >> ---
> >>  drivers/acpi/acpi_lpss.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >
> > Rafael, do you mind if I take this through the PWM tree to resolve the
> > build-time dependency?
> 
> No, I don't, please go ahead, unless Mika/Andy see any problems in it.

No problems from my side - looks good.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] acpi: lpss: call pwm_add_table() for bsw PWM device

[Thierry Reding]

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

On Mon, Jan 30, 2017 at 11:33:10AM +0200, Mika Westerberg wrote:
> On Mon, Jan 30, 2017 at 09:28:09AM +0100, Rafael J. Wysocki wrote:
> > On Mon, Jan 30, 2017 at 9:18 AM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > > On Sun, Jan 22, 2017 at 05:14:09PM +0100, Hans de Goede wrote:
> > >> On x86 we do not have devicetree to link the PWM controller and
> > >> the display controller together. So someone needs to call
> > >> pwm_add_table() to create the link, so that the i915 driver's
> > >> pwm_get(dev, "pwm_backlight") call returns the lpss' pwm0.
> > >>
> > >> The PWM subsystem does not want to have pwm_add_table() calls
> > >> directly in PWM drivers (this leads to probe ordering issues),
> > >> so lets do it here since the acpi-lpss code is always builtin.
> > >>
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> > >> ---
> > >> Changes in v2:
> > >> -Set new pwm_lookup module_name field in the table
> > >> ---
> > >>  drivers/acpi/acpi_lpss.c | 13 +++++++++++++
> > >>  1 file changed, 13 insertions(+)
> > >
> > > Rafael, do you mind if I take this through the PWM tree to resolve the
> > > build-time dependency?
> > 
> > No, I don't, please go ahead, unless Mika/Andy see any problems in it.
> 
> No problems from my side - looks good.

I've pulled this into a separate branch that also contains the
prerequisites, so if you ever need this in ACPI, I can provide a stable
branch or tag.

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