[PATCH v5 0/7] pinctrl: intel: Enable PWM optional feature

[PATCH v5 0/7] pinctrl: intel: Enable PWM optional feature

[Andy Shevchenko]

This is a continuation of the previously applied PWM LPSS cleanup series.
Now, we would like to enable PWM optional feature that may be embedded
into Intel pin control IPs (starting from Sky Lake platforms).

I would like to route this via Intel pin control tree with issuing
an immutable branch for both PINCTRL and PWM subsystems, but I'm
open for other suggestions.

Hans, I dared to leave your Rb tags, however the patches are slightly
differ, because of the Uwe's suggestion on how to handle the missing
headers. I hope you are okay with that. If not, please comment what
must be amended then.

Uwe, the patches 3 and 6 still need your blessing.

Changelog v5:
- added more tags (Uwe)
- avoid moving struct pwm_lpss_chip in patch 3 (Uwe)
- simplified error checking in intel_pinctrl_probe_pwm() (Uwe)

Changelog v4:
- added patch "Rename pwm_lpss_probe() --> devm_pwm_lpss_probe()"

Changelog v3:
- added tags (Uwe, Linus, Thierry)
- fixed some spelling issues in the commit messages
- changed a paragraph in the commit message of the patch 3 (Uwe)
- replaced -ENODEV check with IS_REACHABLE() in the patch 6 (Uwe)

Changelog v2:
- added tag (Mika)
- added base-commit to the series, to make sure LKP can test it

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Andy Shevchenko (7):
  pwm: Add a stub for devm_pwmchip_add()
  pwm: lpss: Rename MAX_PWMS --> LPSS_MAX_PWMS
  pwm: lpss: Include headers we are the direct user of
  pwm: lpss: Allow other drivers to enable PWM LPSS
  pwm: lpss: Rename pwm_lpss_probe() --> devm_pwm_lpss_probe()
  pwm: lpss: Add devm_pwm_lpss_probe() stub
  pinctrl: intel: Enumerate PWM device when community has a capability

 drivers/pinctrl/intel/pinctrl-intel.c      | 29 ++++++++++++++
 drivers/pwm/pwm-lpss-pci.c                 |  2 +-
 drivers/pwm/pwm-lpss-platform.c            |  2 +-
 drivers/pwm/pwm-lpss.c                     |  8 ++--
 drivers/pwm/pwm-lpss.h                     | 26 ++-----------
 include/linux/platform_data/x86/pwm-lpss.h | 44 ++++++++++++++++++++++
 include/linux/pwm.h                        |  5 +++
 7 files changed, 88 insertions(+), 28 deletions(-)
 create mode 100644 include/linux/platform_data/x86/pwm-lpss.h


base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
-- 
2.35.1

[PATCH v5 1/7] pwm: Add a stub for devm_pwmchip_add()

[Andy Shevchenko]

The devm_pwmchip_add() can be called by a module that optionally
instantiates PWM chip. In the case of CONFIG_PWM=n, the compilation
can't be performed. Hence, add a necessary stub.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/pwm.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d70c6e5a839d..bba492eea96c 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -478,6 +478,11 @@ static inline int pwmchip_remove(struct pwm_chip *chip)
 	return -EINVAL;
 }
 
+static inline int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
+{
+	return -EINVAL;
+}
+
 static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 						       unsigned int index,
 						       const char *label)
-- 
2.35.1

[PATCH v5 2/7] pwm: lpss: Rename MAX_PWMS --> LPSS_MAX_PWMS

[Andy Shevchenko]

The MAX_PWMS definition is already being used by the PWM core.
Using the same name in the certain driver confuses people
and potentially can clash with it.

Hence, rename it by adding LPSS prefix.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss.c | 2 +-
 drivers/pwm/pwm-lpss.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index accdef5dd58e..b8739cd2c235 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -252,7 +252,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 	int i, ret;
 	u32 ctrl;
 
-	if (WARN_ON(info->npwm > MAX_PWMS))
+	if (WARN_ON(info->npwm > LPSS_MAX_PWMS))
 		return ERR_PTR(-ENODEV);
 
 	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 8e82eb5a7e00..2c746c51b883 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,7 +13,7 @@
 #include <linux/device.h>
 #include <linux/pwm.h>
 
-#define MAX_PWMS			4
+#define LPSS_MAX_PWMS			4
 
 struct pwm_lpss_chip {
 	struct pwm_chip chip;
-- 
2.35.1

[PATCH v5 3/7] pwm: lpss: Include headers we are the direct user of

[Andy Shevchenko]

For the sake of integrity, include headers we are the direct
user of.

Replace the inclusion of device.h by a forward declaration
of struct device plus a (cheaper) of types.h as device.h is
an expensive include (measured in compiler effort).

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 2c746c51b883..4ce6daa46ca8 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -10,8 +10,10 @@
 #ifndef __PWM_LPSS_H
 #define __PWM_LPSS_H
 
-#include <linux/device.h>
 #include <linux/pwm.h>
+#include <linux/types.h>
+
+struct device;
 
 #define LPSS_MAX_PWMS			4
 
-- 
2.35.1

[PATCH v5 4/7] pwm: lpss: Allow other drivers to enable PWM LPSS

[Andy Shevchenko]

The PWM LPSS device can be embedded in another device.
In order to enable it, allow that drivers to probe
a corresponding device.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss.h                        | 22 +--------------
 .../linux/platform_data/x86}/pwm-lpss.h       | 28 ++++---------------
 2 files changed, 6 insertions(+), 44 deletions(-)
 copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (53%)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 4ce6daa46ca8..bf841250385f 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,7 +13,7 @@
 #include <linux/pwm.h>
 #include <linux/types.h>
 
-struct device;
+#include <linux/platform_data/x86/pwm-lpss.h>
 
 #define LPSS_MAX_PWMS			4
 
@@ -23,29 +23,9 @@ struct pwm_lpss_chip {
 	const struct pwm_lpss_boardinfo *info;
 };
 
-struct pwm_lpss_boardinfo {
-	unsigned long clk_rate;
-	unsigned int npwm;
-	unsigned long base_unit_bits;
-	/*
-	 * Some versions of the IP may stuck in the state machine if enable
-	 * bit is not set, and hence update bit will show busy status till
-	 * the reset. For the rest it may be otherwise.
-	 */
-	bool bypass;
-	/*
-	 * On some devices the _PS0/_PS3 AML code of the GPU (GFX0) device
-	 * messes with the PWM0 controllers state,
-	 */
-	bool other_devices_aml_touches_pwm_regs;
-};
-
 extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
 
-struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
-				     const struct pwm_lpss_boardinfo *info);
-
 #endif	/* __PWM_LPSS_H */
diff --git a/drivers/pwm/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
similarity index 53%
copy from drivers/pwm/pwm-lpss.h
copy to include/linux/platform_data/x86/pwm-lpss.h
index 4ce6daa46ca8..296bd837ddbb 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -1,27 +1,14 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Intel Low Power Subsystem PWM controller driver
- *
- * Copyright (C) 2014, Intel Corporation
- *
- * Derived from the original pwm-lpss.c
- */
+/* Intel Low Power Subsystem PWM controller driver */
 
-#ifndef __PWM_LPSS_H
-#define __PWM_LPSS_H
+#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
+#define __PLATFORM_DATA_X86_PWM_LPSS_H
 
-#include <linux/pwm.h>
 #include <linux/types.h>
 
 struct device;
 
-#define LPSS_MAX_PWMS			4
-
-struct pwm_lpss_chip {
-	struct pwm_chip chip;
-	void __iomem *regs;
-	const struct pwm_lpss_boardinfo *info;
-};
+struct pwm_lpss_chip;
 
 struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
@@ -40,12 +27,7 @@ struct pwm_lpss_boardinfo {
 	bool other_devices_aml_touches_pwm_regs;
 };
 
-extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
-
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
 
-#endif	/* __PWM_LPSS_H */
+#endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.35.1

[PATCH v5 5/7] pwm: lpss: Rename pwm_lpss_probe() --> devm_pwm_lpss_probe()

[Andy Shevchenko]

The pwm_lpss_probe() uses managed resources. Show this to
the users explicitly by adding devm prefix to its name.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss-pci.c                 | 2 +-
 drivers/pwm/pwm-lpss-platform.c            | 2 +-
 drivers/pwm/pwm-lpss.c                     | 6 +++---
 include/linux/platform_data/x86/pwm-lpss.h | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index 98413d364338..b4134bee2863 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -30,7 +30,7 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 		return err;
 
 	info = (struct pwm_lpss_boardinfo *)id->driver_data;
-	lpwm = pwm_lpss_probe(&pdev->dev, pcim_iomap_table(pdev)[0], info);
+	lpwm = devm_pwm_lpss_probe(&pdev->dev, pcim_iomap_table(pdev)[0], info);
 	if (IS_ERR(lpwm))
 		return PTR_ERR(lpwm);
 
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index c48c6f2b2cd8..f350607e28bd 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -31,7 +31,7 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	lpwm = pwm_lpss_probe(&pdev->dev, base, info);
+	lpwm = devm_pwm_lpss_probe(&pdev->dev, base, info);
 	if (IS_ERR(lpwm))
 		return PTR_ERR(lpwm);
 
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index b8739cd2c235..bb740346b699 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -244,8 +244,8 @@ static const struct pwm_ops pwm_lpss_ops = {
 	.owner = THIS_MODULE,
 };
 
-struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
-				     const struct pwm_lpss_boardinfo *info)
+struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
+					  const struct pwm_lpss_boardinfo *info)
 {
 	struct pwm_lpss_chip *lpwm;
 	unsigned long c;
@@ -284,7 +284,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 
 	return lpwm;
 }
-EXPORT_SYMBOL_GPL(pwm_lpss_probe);
+EXPORT_SYMBOL_GPL(devm_pwm_lpss_probe);
 
 MODULE_DESCRIPTION("PWM driver for Intel LPSS");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
index 296bd837ddbb..c852fe24fe2a 100644
--- a/include/linux/platform_data/x86/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -27,7 +27,7 @@ struct pwm_lpss_boardinfo {
 	bool other_devices_aml_touches_pwm_regs;
 };
 
-struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
-				     const struct pwm_lpss_boardinfo *info);
+struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
+					  const struct pwm_lpss_boardinfo *info);
 
 #endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.35.1

[PATCH v5 6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

[Andy Shevchenko]

In case the PWM LPSS module is not provided, allow users to be
compiled with the help of the devm_pwm_lpss_probe() stub.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 include/linux/platform_data/x86/pwm-lpss.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
index c852fe24fe2a..6ef21b8baec7 100644
--- a/include/linux/platform_data/x86/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -4,6 +4,8 @@
 #ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
 #define __PLATFORM_DATA_X86_PWM_LPSS_H
 
+#include <linux/err.h>
+#include <linux/kconfig.h>
 #include <linux/types.h>
 
 struct device;
@@ -27,7 +29,16 @@ struct pwm_lpss_boardinfo {
 	bool other_devices_aml_touches_pwm_regs;
 };
 
+#if IS_REACHABLE(CONFIG_PWM_LPSS)
 struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
 					  const struct pwm_lpss_boardinfo *info);
+#else
+static inline
+struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
+					  const struct pwm_lpss_boardinfo *info)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif	/* CONFIG_PWM_LPSS */
 
 #endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.35.1

[PATCH v5 7/7] pinctrl: intel: Enumerate PWM device when community has a capability

[Andy Shevchenko]

Some of the Communities may have PWM capability. In such cases,
enumerate the PWM device via respective driver. A user is still
responsible for setting correct pin muxing for the line that
needs to output the signal.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 29 +++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 52ecd66ce357..d74c8b650aa7 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -21,6 +21,8 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
 
+#include <linux/platform_data/x86/pwm-lpss.h>
+
 #include "../core.h"
 #include "pinctrl-intel.h"
 
@@ -46,6 +48,8 @@
 #define PADOWN_MASK(p)			(GENMASK(3, 0) << PADOWN_SHIFT(p))
 #define PADOWN_GPP(p)			((p) / 8)
 
+#define PWMC				0x204
+
 /* Offset from pad_regs */
 #define PADCFG0				0x000
 #define PADCFG0_RXEVCFG_SHIFT		25
@@ -1499,6 +1503,27 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
 	return 0;
 }
 
+static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl,
+				   struct intel_community *community)
+{
+	static const struct pwm_lpss_boardinfo info = {
+		.clk_rate = 19200000,
+		.npwm = 1,
+		.base_unit_bits = 22,
+		.bypass = true,
+	};
+	struct pwm_lpss_chip *pwm;
+
+	if (!(community->features & PINCTRL_FEATURE_PWM))
+		return 0;
+
+	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
+		return 0;
+
+	pwm = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
+	return PTR_ERR_OR_ZERO(pwm);
+}
+
 static int intel_pinctrl_probe(struct platform_device *pdev,
 			       const struct intel_pinctrl_soc_data *soc_data)
 {
@@ -1584,6 +1609,10 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 			ret = intel_pinctrl_add_padgroups_by_size(pctrl, community);
 		if (ret)
 			return ret;
+
+		ret = intel_pinctrl_probe_pwm(pctrl, community);
+		if (ret)
+			return ret;
 	}
 
 	irq = platform_get_irq(pdev, 0);
-- 
2.35.1

Re: [PATCH v5 0/7] pinctrl: intel: Enable PWM optional feature

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 01:07:59PM +0200, Andy Shevchenko wrote:
> 
> This is a continuation of the previously applied PWM LPSS cleanup series.
> Now, we would like to enable PWM optional feature that may be embedded
> into Intel pin control IPs (starting from Sky Lake platforms).
> 
> I would like to route this via Intel pin control tree with issuing
> an immutable branch for both PINCTRL and PWM subsystems, but I'm
> open for other suggestions.
> 
> Hans, I dared to leave your Rb tags, however the patches are slightly
> differ, because of the Uwe's suggestion on how to handle the missing
> headers. I hope you are okay with that. If not, please comment what
> must be amended then.
> 
> Uwe, the patches 3 and 6 still need your blessing.

Uwe, do you think they are ready to go?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/7] pwm: lpss: Include headers we are the direct user of

[Uwe Kleine-König]

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

On Thu, Nov 17, 2022 at 01:08:02PM +0200, Andy Shevchenko wrote:
> For the sake of integrity, include headers we are the direct
> user of.
> 
> Replace the inclusion of device.h by a forward declaration
> of struct device plus a (cheaper) of types.h as device.h is
> an expensive include (measured in compiler effort).
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v5 3/7] pwm: lpss: Include headers we are the direct user of

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 02:39:33PM +0100, Uwe Kleine-König wrote:
> On Thu, Nov 17, 2022 at 01:08:02PM +0200, Andy Shevchenko wrote:
> > For the sake of integrity, include headers we are the direct
> > user of.
> > 
> > Replace the inclusion of device.h by a forward declaration
> > of struct device plus a (cheaper) of types.h as device.h is
> > an expensive include (measured in compiler effort).

...

> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thank you!

Patch 6 is still in question.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

[Uwe Kleine-König]

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

On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:
> In case the PWM LPSS module is not provided, allow users to be
> compiled with the help of the devm_pwm_lpss_probe() stub.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  include/linux/platform_data/x86/pwm-lpss.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
> index c852fe24fe2a..6ef21b8baec7 100644
> --- a/include/linux/platform_data/x86/pwm-lpss.h
> +++ b/include/linux/platform_data/x86/pwm-lpss.h
> @@ -4,6 +4,8 @@
>  #ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
>  #define __PLATFORM_DATA_X86_PWM_LPSS_H
>  
> +#include <linux/err.h>
> +#include <linux/kconfig.h>
>  #include <linux/types.h>
>  
>  struct device;
> @@ -27,7 +29,16 @@ struct pwm_lpss_boardinfo {
>  	bool other_devices_aml_touches_pwm_regs;
>  };
>  
> +#if IS_REACHABLE(CONFIG_PWM_LPSS)
>  struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
>  					  const struct pwm_lpss_boardinfo *info);
> +#else
> +static inline
> +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> +					  const struct pwm_lpss_boardinfo *info)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +#endif	/* CONFIG_PWM_LPSS */

Hmm, this is actually never used, because if
!IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
7) has:

	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
		return 0;

before devm_pwm_lpss_probe() is called.

Not sure if it's safe to just drop this patch. The return value is
neither -ENOSYS (which I would expect for a stub function like that) nor
-EINVAL (which for reasons unknown to me is used in the stub for
pwmchip_add()).

I would have a better feeling with -ENOSYS in your stub, but I don't
feel really strong here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v5 6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 05:47:03PM +0100, Uwe Kleine-König wrote:
> On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:
> > In case the PWM LPSS module is not provided, allow users to be
> > compiled with the help of the devm_pwm_lpss_probe() stub.

...

> > +static inline
> > +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> > +					  const struct pwm_lpss_boardinfo *info)
> > +{
> > +	return ERR_PTR(-ENODEV);
> > +}
> > +#endif	/* CONFIG_PWM_LPSS */
> 
> Hmm, this is actually never used, because if
> !IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
> 7) has:
> 
> 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
> 		return 0;
> 
> before devm_pwm_lpss_probe() is called.
> 
> Not sure if it's safe to just drop this patch.

How is it supposed to be compiled and linked then?

>	The return value is
> neither -ENOSYS (which I would expect for a stub function like that)

This is not a generic library registration / addition of the device.
I don't see how ENOSYS and esp. EINVAL fits here.

>	nor
> -EINVAL (which for reasons unknown to me is used in the stub for
> pwmchip_add()).

This I explained already that _add() != _probe() semantically, I do not see the
link between their error codes.

> I would have a better feeling with -ENOSYS in your stub, but I don't
> feel really strong here.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

[Uwe Kleine-König]

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

Hello Andy,

On Tue, Nov 22, 2022 at 07:35:38PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 05:47:03PM +0100, Uwe Kleine-König wrote:
> > On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:
> > > In case the PWM LPSS module is not provided, allow users to be
> > > compiled with the help of the devm_pwm_lpss_probe() stub.
> 
> ...
> 
> > > +static inline
> > > +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> > > +					  const struct pwm_lpss_boardinfo *info)
> > > +{
> > > +	return ERR_PTR(-ENODEV);
> > > +}
> > > +#endif	/* CONFIG_PWM_LPSS */
> > 
> > Hmm, this is actually never used, because if
> > !IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
> > 7) has:
> > 
> > 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
> > 		return 0;
> > 
> > before devm_pwm_lpss_probe() is called.
> > 
> > Not sure if it's safe to just drop this patch.
> 
> How is it supposed to be compiled and linked then?

The compiler optimizes everything away after that return 0 and so
doesn't need that symbol at all.

I just tested compiling your series without patch #6, x86_64 allmodconfig + PWM=n.

nm doesn't report the need for devm_pwm_lpss_probe in
drivers/pinctrl/intel/pinctrl-intel.o.

The build isn't done yet, but I don't expect surprises.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v5 6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 07:14:44PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 22, 2022 at 07:35:38PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 22, 2022 at 05:47:03PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:

...

> > > > +static inline
> > > > +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> > > > +					  const struct pwm_lpss_boardinfo *info)
> > > > +{
> > > > +	return ERR_PTR(-ENODEV);
> > > > +}
> > > > +#endif	/* CONFIG_PWM_LPSS */
> > > 
> > > Hmm, this is actually never used, because if
> > > !IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
> > > 7) has:
> > > 
> > > 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
> > > 		return 0;
> > > 
> > > before devm_pwm_lpss_probe() is called.
> > > 
> > > Not sure if it's safe to just drop this patch.
> > 
> > How is it supposed to be compiled and linked then?
> 
> The compiler optimizes everything away after that return 0 and so
> doesn't need that symbol at all.
> 
> I just tested compiling your series without patch #6, x86_64 allmodconfig + PWM=n.
> 
> nm doesn't report the need for devm_pwm_lpss_probe in
> drivers/pinctrl/intel/pinctrl-intel.o.

Sounds like you are right. I tried a brief test with m/m, y/m, m/y, and m/n
variants between PINCTRL_INTEL and PWM_LPSS and all were built fine.

That said, I will drop this patch and send a PR with the rest.

Thank you for thorough review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

[Uwe Kleine-König]

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

Hello Andy,

On Tue, Nov 22, 2022 at 08:32:53PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 07:14:44PM +0100, Uwe Kleine-König wrote:
> > I just tested compiling your series without patch #6, x86_64 allmodconfig + PWM=n.
> > 
> > nm doesn't report the need for devm_pwm_lpss_probe in
> > drivers/pinctrl/intel/pinctrl-intel.o.
> 
> Sounds like you are right. I tried a brief test with m/m, y/m, m/y, and m/n
> variants between PINCTRL_INTEL and PWM_LPSS and all were built fine.
> 
> That said, I will drop this patch and send a PR with the rest.

Sounds good.

> Thank you for thorough review!

Thank you for the interesting thread. I like our conversations.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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