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

[PATCH v4 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.

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      | 32 ++++++++++++++++
 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                     | 34 ++++-------------
 include/linux/platform_data/x86/pwm-lpss.h | 44 ++++++++++++++++++++++
 include/linux/pwm.h                        |  5 +++
 7 files changed, 95 insertions(+), 32 deletions(-)
 create mode 100644 include/linux/platform_data/x86/pwm-lpss.h


base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
-- 
2.35.1

[PATCH v4 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 v4 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 v4 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).

While at it, move the struct pwm_lpss_chip to be after
the struct pwm_lpss_boardinfo as the former uses pointer
to the latter.

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 | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 2c746c51b883..4561d229b27d 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -10,16 +10,12 @@
 #ifndef __PWM_LPSS_H
 #define __PWM_LPSS_H
 
-#include <linux/device.h>
 #include <linux/pwm.h>
+#include <linux/types.h>
 
-#define LPSS_MAX_PWMS			4
+struct device;
 
-struct pwm_lpss_chip {
-	struct pwm_chip chip;
-	void __iomem *regs;
-	const struct pwm_lpss_boardinfo *info;
-};
+#define LPSS_MAX_PWMS			4
 
 struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
@@ -43,6 +39,12 @@ 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 {
+	struct pwm_chip chip;
+	void __iomem *regs;
+	const struct pwm_lpss_boardinfo *info;
+};
+
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
 
-- 
2.35.1

[PATCH v4 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>
---
 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 4561d229b27d..b721532c6c3c 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,27 +13,10 @@
 #include <linux/pwm.h>
 #include <linux/types.h>
 
-struct device;
+#include <linux/platform_data/x86/pwm-lpss.h>
 
 #define LPSS_MAX_PWMS			4
 
-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;
@@ -45,7 +28,4 @@ struct pwm_lpss_chip {
 	const struct pwm_lpss_boardinfo *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 4561d229b27d..296bd837ddbb 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -1,21 +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_lpss_boardinfo {
 	unsigned long clk_rate;
@@ -34,18 +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 {
-	struct pwm_chip chip;
-	void __iomem *regs;
-	const struct pwm_lpss_boardinfo *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 v4 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>
---
 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 v4 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 v4 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. 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>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 52ecd66ce357..d61c22e9d531 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,30 @@ 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);
+	if (IS_ERR(pwm))
+		return PTR_ERR(pwm);
+
+	return 0;
+}
+
 static int intel_pinctrl_probe(struct platform_device *pdev,
 			       const struct intel_pinctrl_soc_data *soc_data)
 {
@@ -1584,6 +1612,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 v4 0/7] pinctrl: intel: Enable PWM optional feature

[Andy Shevchenko]

On Mon, Nov 14, 2022 at 06:55:38PM +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, it would be nice if you have a chance to look at this one more time.
If it is okay, I can push it to an immutable branch and send out for all
stakeholders.

> 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      | 32 ++++++++++++++++
>  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                     | 34 ++++-------------
>  include/linux/platform_data/x86/pwm-lpss.h | 44 ++++++++++++++++++++++
>  include/linux/pwm.h                        |  5 +++
>  7 files changed, 95 insertions(+), 32 deletions(-)
>  create mode 100644 include/linux/platform_data/x86/pwm-lpss.h
> 
> 
> base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
> -- 
> 2.35.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Uwe Kleine-König]

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

On Mon, Nov 14, 2022 at 06:55:41PM +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).
> 
> While at it, move the struct pwm_lpss_chip to be after
> the struct pwm_lpss_boardinfo as the former uses pointer
> to the latter.

I stand by my feedback that this change is irrelevant in the end. If you
drop it here, the patch gets a bit nicer and in the end the difference
is just:

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index b721532c6c3c..bf841250385f 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -17,15 +17,15 @@
 
 #define LPSS_MAX_PWMS			4
 
-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 {
 	struct pwm_chip chip;
 	void __iomem *regs;
 	const struct pwm_lpss_boardinfo *info;
 };
 
+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;
+
 #endif	/* __PWM_LPSS_H */

which is quite ok to leave as is.

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 v4 4/7] pwm: lpss: Allow other drivers to enable PWM LPSS

[Uwe Kleine-König]

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

On Mon, Nov 14, 2022 at 06:55:42PM +0200, Andy Shevchenko wrote:
> 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>

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 v4 5/7] pwm: lpss: Rename pwm_lpss_probe() --> devm_pwm_lpss_probe()

[Uwe Kleine-König]

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

On Mon, Nov 14, 2022 at 06:55:43PM +0200, Andy Shevchenko wrote:
> 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>

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 v4 3/7] pwm: lpss: Include headers we are the direct user of

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 09:50:27AM +0100, Uwe Kleine-König wrote:
> On Mon, Nov 14, 2022 at 06:55:41PM +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).
> > 
> > While at it, move the struct pwm_lpss_chip to be after
> > the struct pwm_lpss_boardinfo as the former uses pointer
> > to the latter.
> 
> I stand by my feedback that this change is irrelevant in the end. If you
> drop it here, the patch gets a bit nicer.

OK, since you are insisting, I will modify this in v5.

-- 
With Best Regards,
Andy Shevchenko

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

[Uwe Kleine-König]

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

Hello,

On Mon, Nov 14, 2022 at 06:55:45PM +0200, Andy Shevchenko wrote:
> Some of the Communities may have PWM capability. In such cases,

Is "Communities" is proper name in this context? If not, I'd not
capitalize it.

> enumerate the PWM device via respective driver. User is still

s/User/A user/ ?

> 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>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 32 +++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 52ecd66ce357..d61c22e9d531 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,30 @@ 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);
> +	if (IS_ERR(pwm))
> +		return PTR_ERR(pwm);
> +
> +	return 0;

The last 3 codelines can be replaced by

	return PTR_ERR_OR_ZERO(pwm);

(but I know it's subjective if you like that or not, so I won't insist;
see also b784c77075023e1a71bc06e6b4f711acb99e9c73).

> +}
> +
>  static int intel_pinctrl_probe(struct platform_device *pdev,
>  			       const struct intel_pinctrl_soc_data *soc_data)
>  {
> @@ -1584,6 +1612,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);

intel_pinctrl_add_padgroups_by_size() doesn't need cleanup in the error
path, so this hunk is fine.

All in all this is all very minor, so:

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

even if you keep the patch as is.

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 v4 7/7] pinctrl: intel: Enumerate PWM device when community has a capability

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 10:06:05AM +0100, Uwe Kleine-König wrote:
> On Mon, Nov 14, 2022 at 06:55:45PM +0200, Andy Shevchenko wrote:
> > Some of the Communities may have PWM capability. In such cases,
> 
> Is "Communities" is proper name in this context? If not, I'd not
> capitalize it.

Intel pin control is a set of so called Communities, which are divided by
groups of pins. (There is an intermediate division, but it doesn't affect
software anyhow, so I haven't mentioned it).

> > enumerate the PWM device via respective driver. User is still
> 
> s/User/A user/ ?

OK!

> > responsible for setting correct pin muxing for the line that
> > needs to output the signal.

...

> > +	pwm = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> > +	if (IS_ERR(pwm))
> > +		return PTR_ERR(pwm);
> > +
> > +	return 0;
> 
> The last 3 codelines can be replaced by
> 
> 	return PTR_ERR_OR_ZERO(pwm);
> 
> (but I know it's subjective if you like that or not, so I won't insist;
> see also b784c77075023e1a71bc06e6b4f711acb99e9c73).

Yes, it used to be like that in some of my previous attempts
(maybe not public), but have been changed due to an additional
error check which is gone, so it can be reinstantiated now.

...

> All in all this is all very minor, so:
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> even if you keep the patch as is.

Thank you, I will amend as you suggested.

-- 
With Best Regards,
Andy Shevchenko