[PATCH v2 0/6] pinctrl: intel: Enable PWM optional feature

[PATCH v2 0/6] 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 slighly
differ, because of the Uwe's suggestion on how to handle the missing
headers. I hope you is okay with that. If not, please comment what
must be ammended then.

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 (6):
  pwm: Add a stub for devm_pwmchip_add()
  pwm: lpss: Rename MAX_PWMS --> LPSS_MAX_PWMS
  pwm: lpss: Include headers we are direct user of
  pwm: lpss: Allow other drivers to enable PWM LPSS
  pwm: lpss: Add pwm_lpss_probe() stub
  pinctrl: intel: Enumerate PWM device when community has a capabilitty

 drivers/pinctrl/intel/pinctrl-intel.c         | 29 +++++++++++++
 drivers/pwm/pwm-lpss.c                        |  2 +-
 drivers/pwm/pwm-lpss.h                        | 34 ++++-----------
 .../linux/platform_data/x86}/pwm-lpss.h       | 41 ++++++++-----------
 include/linux/pwm.h                           |  5 +++
 5 files changed, 61 insertions(+), 50 deletions(-)
 copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (51%)


base-commit: 3886bc3523db24814c98c57d74fe66d7a21bf40b
-- 
2.35.1

[PATCH v2 1/6] pwm: Add a stub for devm_pwmchip_add()

[Andy Shevchenko]

devm_pwmchip_add() can be called by a module that optionally
instantiates PWM chip. In 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>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 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 v2 2/6] pwm: lpss: Rename MAX_PWMS --> LPSS_MAX_PWMS

[Andy Shevchenko]

The MAX_PWMS definition is already being used by 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>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 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 v2 3/6] pwm: lpss: Include headers we are direct user of

[Andy Shevchenko]

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

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.

Replace device.h with a forward declaration in order to improve
the compilation time due to reducing overhead of device.h parsing
with entire train of dependencies.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 v2 4/6] 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>
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 v2 5/6] pwm: lpss: Add pwm_lpss_probe() stub

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 296bd837ddbb..c868b396ed2c 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 *pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
+#else
+static inline
+struct pwm_lpss_chip *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 v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Andy Shevchenko]

Some of the Communities may have PWM capability. In such cases,
enumerate 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>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 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 6e630e87fed6..6b685ff7041f 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -24,6 +24,8 @@
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 
+#include <linux/platform_data/x86/pwm-lpss.h>
+
 #include "../core.h"
 #include "pinctrl-intel.h"
 
@@ -49,6 +51,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
@@ -1502,6 +1506,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;
+
+	pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
+	if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
+		return PTR_ERR(pwm);
+
+	return 0;
+}
+
 static int intel_pinctrl_probe(struct platform_device *pdev,
 			       const struct intel_pinctrl_soc_data *soc_data)
 {
@@ -1588,6 +1613,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 v2 2/6] pwm: lpss: Rename MAX_PWMS --> LPSS_MAX_PWMS

[Uwe Kleine-König]

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

On Tue, Nov 08, 2022 at 04:22:22PM +0200, Andy Shevchenko wrote:
> The MAX_PWMS definition is already being used by 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>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

Thanks
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 v2 0/6] pinctrl: intel: Enable PWM optional feature

[Linus Walleij]

On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

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

I'm fine with this approach if it works for Uwe.

Yours,
Linus Walleij

Re: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Linus Walleij]

On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Some of the Communities may have PWM capability. In such cases,
> enumerate 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>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

So:

> +#include <linux/platform_data/x86/pwm-lpss.h>
(...)
> +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;
> +
> +       pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> +       if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
> +               return PTR_ERR(pwm);

This is alike a boardfile embedded into the pin control driver.

It's a bit backwards since we usually go the other direction, i.e. probe
a PWM driver or whatever and then that driver request its resources
that are assigned from e.g. DT or ACPI, and in this case that would
mean it request its pin control handle and the pins get set up.

I guess I can be convinced that this hack is the lesser evil :D

What is it in the platform that makes this kind of hacks necessary?
Inconsistent description in ACPI or is the PWM device simply
missing from the DSDT (or whatever is the right form in ACPI)
in already shipped devices that need it?

Or is it simply impossible to describe the PWM device in ACPI?

Yours,
Linus Walleij

Re: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote:
> On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > +       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;
> > +
> > +       pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> > +       if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
> > +               return PTR_ERR(pwm);
> 
> This is alike a boardfile embedded into the pin control driver.

Correct.

> It's a bit backwards since we usually go the other direction, i.e. probe
> a PWM driver or whatever and then that driver request its resources
> that are assigned from e.g. DT or ACPI, and in this case that would
> mean it request its pin control handle and the pins get set up.
> 
> I guess I can be convinced that this hack is the lesser evil :D
> 
> What is it in the platform that makes this kind of hacks necessary?

The PWM capability is discoverable by the looking for it in the pin
control IP MMIO, it's not a separate device, but a sibling (child?)
of the pin control, that's not a separate entity.

Moreover, not every pin control _community_ has that capability (capabilities
are on the Community level and depends on ACPI representation of the
communities themself - single device or device per community - the PWM may or
may not be easily attached.

What you are proposing is to invent at least two additional properties or so
for the pin control device description and then to support old platforms,
create a board file somewhere else, which will go through all pin control
devices, checks the capabilities, then embeds the properties via properties
(Either embedded into DSDT, if done in BIOS, or swnodes).

Do I get you right?

If so, in my opinion it's way more ugly and overkill that the current
approach.

> Inconsistent description in ACPI or is the PWM device simply
> missing from the DSDT (or whatever is the right form in ACPI)
> in already shipped devices that need it?

Right.

> Or is it simply impossible to describe the PWM device in ACPI?

It's a dynamic feature and existing firmwares are carved in stone.
It might be possible to move the above mentioned uglification to
the BIOS. But the horizon of that is 5+ years in case I am able
to convince program managers for it (TBH, I don't believe it's
feasible, since "Windows works!" mantra, they are not engineers).

That said, I agree that this looks not nice, but that's all what
Mika and me can come up with to make all this as little ugly and
intrusive as possible.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Linus Walleij]

On Wed, Nov 9, 2022 at 10:56 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote:

> > I guess I can be convinced that this hack is the lesser evil :D
> >
> > What is it in the platform that makes this kind of hacks necessary?
>
> The PWM capability is discoverable by the looking for it in the pin
> control IP MMIO, it's not a separate device, but a sibling (child?)
> of the pin control, that's not a separate entity.

OK I get it.

> Moreover, not every pin control _community_ has that capability (capabilities
> are on the Community level and depends on ACPI representation of the
> communities themself - single device or device per community - the PWM may or
> may not be easily attached.

OK I think I understand it a bit, if ACPI thinks about the PWM
as "some feature of the community" then that is how it is, we have
this bad fit between device tree and Linux internals at times as well,
then spawning a device from another one is the way to go, we need
to consider the option that it is Linux that is weird at times, not the
HW description.

> What you are proposing is to invent at least two additional properties or so
> for the pin control device description and then to support old platforms,
> create a board file somewhere else, which will go through all pin control
> devices, checks the capabilities, then embeds the properties via properties
> (Either embedded into DSDT, if done in BIOS, or swnodes).
>
> Do I get you right?
>
> If so, in my opinion it's way more ugly and overkill that the current
> approach.

No I just wanted to understand things better. This small hack in the
pin controller is way better than a bigger and widespread hack
somewhere else.

> That said, I agree that this looks not nice, but that's all what
> Mika and me can come up with to make all this as little ugly and
> intrusive as possible.

I can live with it, rough consensus and running code.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 11:08:04AM +0100, Linus Walleij wrote:
> On Wed, Nov 9, 2022 at 10:56 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote:

...

> > > I guess I can be convinced that this hack is the lesser evil :D
> > >
> > > What is it in the platform that makes this kind of hacks necessary?
> >
> > The PWM capability is discoverable by the looking for it in the pin
> > control IP MMIO, it's not a separate device, but a sibling (child?)
> > of the pin control, that's not a separate entity.
> 
> OK I get it.
> 
> > Moreover, not every pin control _community_ has that capability (capabilities
> > are on the Community level and depends on ACPI representation of the
> > communities themself - single device or device per community - the PWM may or
> > may not be easily attached.
> 
> OK I think I understand it a bit, if ACPI thinks about the PWM
> as "some feature of the community" then that is how it is, we have
> this bad fit between device tree and Linux internals at times as well,
> then spawning a device from another one is the way to go, we need
> to consider the option that it is Linux that is weird at times, not the
> HW description.

The problem here is not the impossibility to do the things. The problem is
that things are done and validated on a Windows system. After that it close
to impossible to update the firmware or perform any architectural changes.

OTOH, announcing the separate device out of the existing MMIO space doesn't
sound right from the software point of view that should follow the hardware
representation.

Ideally, this should be an adaptive MFD-like device, but it makes things
even more complicated than has been discussed already. (Note, that some
of the pin control drivers are enumerated as platform devices, and that
code should also be taken into account)

...

> > That said, I agree that this looks not nice, but that's all what
> > Mika and me can come up with to make all this as little ugly and
> > intrusive as possible.
> 
> I can live with it, rough consensus and running code.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/6] pinctrl: intel: Enable PWM optional feature

[Thierry Reding]

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

On Tue, Nov 08, 2022 at 04:22:20PM +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.

I don't have any objections for this to go through the Intel tree as
long as Uwe is happy with this. Most of this is just reworking existing
things and the stub additions look good to me, so:

Acked-by: Thierry Reding <thierry.reding@gmail.com>

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

Re: [PATCH v2 0/6] pinctrl: intel: Enable PWM optional feature

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 06:40:02PM +0100, Thierry Reding wrote:
> On Tue, Nov 08, 2022 at 04:22:20PM +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.
> 
> I don't have any objections for this to go through the Intel tree as
> long as Uwe is happy with this.

So far Uwe acknowledged patch 2 only, hopefully he will have time to go
thru the rest.

> Most of this is just reworking existing
> things and the stub additions look good to me, so:
> 
> Acked-by: Thierry Reding <thierry.reding@gmail.com>

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] pwm: Add a stub for devm_pwmchip_add()

[Uwe Kleine-König]

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

Hello 

On Tue, Nov 08, 2022 at 04:22:21PM +0200, Andy Shevchenko wrote:
> devm_pwmchip_add() can be called by a module that optionally
> instantiates PWM chip. In 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>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  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;
> +}
> +

I'm a bit surprised to see this returning -EINVAL and not -ENOSYS. But
that's in line with the other stubs, so:

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

[Uwe Kleine-König]

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

Hello,

On Tue, Nov 08, 2022 at 04:22:23PM +0200, Andy Shevchenko wrote:
> For the sake of integrity, include headers we are direct user of.
> 
> 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.

That part is fine.

> Replace device.h with a forward declaration in order to improve
> the compilation time due to reducing overhead of device.h parsing
> with entire train of dependencies.

Together with "For the sake of integrity, include headers we are direct
user of." this makes an a bit schizophrenic impression on me. You add
<linux/types.h> because the file is a direct user of it, but you drop
<linux/device.h> despite being a direct user.

If you adapt the reasoning to something like:

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

I could better live with it. I would even split this into two patches
then. (i.e. move struct pwm_lpss_chip vs the include and forward change)

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

[Uwe Kleine-König]

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

On Tue, Nov 08, 2022 at 04:22:24PM +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.

"probe" isn't the right term here. The other driver only creates the
device.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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;
> -};
> -

Now that pwm_lpss_boardinfo lives in a different file, this makes the
move of pwm_lpss_chip in patch 3 somewhat redundant.

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

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

[Uwe Kleine-König]

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

On Thu, Nov 10, 2022 at 08:28:10AM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 08, 2022 at 04:22:24PM +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.
> 
> "probe" isn't the right term here. The other driver only creates the
> device.

Ah, I didn't look carefully enough for this judgement. My (wrong)
expectation was, that the containing device creates a platform-device
with .parent=$self and then drivers/pwm/pwm-lpss-platform.c does all the
necessary stuff automatically.

Hmm, I can live with that, so I drop this concern.

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 v2 5/6] pwm: lpss: Add pwm_lpss_probe() stub

[Uwe Kleine-König]

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

On Tue, Nov 08, 2022 at 04:22:25PM +0200, Andy Shevchenko wrote:
> In case the PWM LPSS module is not provided, allow users to be
> compiled with a help of a pwm_lpss_probe() stub.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 296bd837ddbb..c868b396ed2c 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 *pwm_lpss_probe(struct device *dev, void __iomem *base,
>  				     const struct pwm_lpss_boardinfo *info);
> +#else
> +static inline
> +struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> +				     const struct pwm_lpss_boardinfo *info)
> +{
> +	return ERR_PTR(-ENODEV);

Would it be more consistent to return the same value as the pwmchip_add
stub does?

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

[Uwe Kleine-König]

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

On Tue, Nov 08, 2022 at 04:22:26PM +0200, Andy Shevchenko wrote:
> Some of the Communities may have PWM capability. In such cases,
> enumerate 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>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  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 6e630e87fed6..6b685ff7041f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -24,6 +24,8 @@
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/pinmux.h>
>  
> +#include <linux/platform_data/x86/pwm-lpss.h>
> +
>  #include "../core.h"
>  #include "pinctrl-intel.h"
>  
> @@ -49,6 +51,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
> @@ -1502,6 +1506,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;
> +
> +	pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> +	if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
> +		return PTR_ERR(pwm);

Linus and Andy already agreed that this patch is ugly. I wonder if this
here would be a bit less ugly if you do:

	if (IS_REACHABLE(...)) {
		pwm = pwm_lpss_probe(...);
		...


	}

and drop the check PTR_ERR(pwm) != -ENODEV (which might have a different
semantic than "the pwm driver isn't available").

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

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 9:22 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Nov 08, 2022 at 04:22:23PM +0200, Andy Shevchenko wrote:
> > For the sake of integrity, include headers we are direct user of.
> >
> > 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.
>
> That part is fine.
>
> > Replace device.h with a forward declaration in order to improve
> > the compilation time due to reducing overhead of device.h parsing
> > with entire train of dependencies.
>
> Together with "For the sake of integrity, include headers we are direct
> user of." this makes an a bit schizophrenic impression on me. You add
> <linux/types.h> because the file is a direct user of it, but you drop
> <linux/device.h> despite being a direct user.

But we don't use device.h.

> If you adapt the reasoning to something like:
>
> Replace the inclusion of <linux/device.h> by a forward declaration of
> struct device plus a (cheaper) #include of <linux/types.h> as
> <linux/device.h> is an expensive include (measured in compiler effort).

Fine with me, thanks for the draft.

> I could better live with it. I would even split this into two patches
> then. (i.e. move struct pwm_lpss_chip vs the include and forward change)

I think for this small change for a driver that hasn't been modified
often it's fine to have them in one. But tell me if you are insisting
on a split, I can do that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] pwm: Add a stub for devm_pwmchip_add()

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 9:07 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Nov 08, 2022 at 04:22:21PM +0200, Andy Shevchenko wrote:

...

> > +static inline int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
> > +{
> > +     return -EINVAL;
> > +}
> > +
>
> I'm a bit surprised to see this returning -EINVAL and not -ENOSYS. But
> that's in line with the other stubs, so:

Exactly my thoughts when I created such a change.

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

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] pwm: lpss: Allow other drivers to enable PWM LPSS

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Nov 08, 2022 at 04:22:24PM +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.

...

> Now that pwm_lpss_boardinfo lives in a different file, this makes the
> move of pwm_lpss_chip in patch 3 somewhat redundant.

But they are independent changes. At each stage (after each patch) we
should have a finished step, right? Not touching that makes me feel
that the step is half-baked. If you insist I can drop that move from
the other patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] pwm: lpss: Add pwm_lpss_probe() stub

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 9:38 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Nov 08, 2022 at 04:22:25PM +0200, Andy Shevchenko wrote:
> > In case the PWM LPSS module is not provided, allow users to be
> > compiled with a help of a pwm_lpss_probe() stub.

...

> > +static inline
> > +struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> > +                                  const struct pwm_lpss_boardinfo *info)
> > +{
> > +     return ERR_PTR(-ENODEV);
>
> Would it be more consistent to return the same value as the pwmchip_add
> stub does?

Then I will lose the ability to distinguish between absent driver (or
device) and actual error during the probing of it. Any suggestions on
how to do that better?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 9:45 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Nov 08, 2022 at 04:22:26PM +0200, Andy Shevchenko wrote:
> > Some of the Communities may have PWM capability. In such cases,
> > enumerate PWM device via respective driver. User is still responsible
> > for setting correct pin muxing for the line that needs to output the
> > signal.

...

> > +     pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> > +     if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
> > +             return PTR_ERR(pwm);
>
> Linus and Andy already agreed that this patch is ugly. I wonder if this
> here would be a bit less ugly if you do:
>
>         if (IS_REACHABLE(...)) {
>                 pwm = pwm_lpss_probe(...);
>                 ...
>
>
>         }
>
> and drop the check PTR_ERR(pwm) != -ENODEV (which might have a different
> semantic than "the pwm driver isn't available").

I will think about it (in such case the comment against the previous
patch makes more sense to me).

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] pwm: lpss: Include headers we are direct user of

[Uwe Kleine-König]

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

On Thu, Nov 10, 2022 at 11:53:59AM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 9:22 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Nov 08, 2022 at 04:22:23PM +0200, Andy Shevchenko wrote:
> > > For the sake of integrity, include headers we are direct user of.
> > >
> > > 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.
> >
> > That part is fine.
> >
> > > Replace device.h with a forward declaration in order to improve
> > > the compilation time due to reducing overhead of device.h parsing
> > > with entire train of dependencies.
> >
> > Together with "For the sake of integrity, include headers we are direct
> > user of." this makes an a bit schizophrenic impression on me. You add
> > <linux/types.h> because the file is a direct user of it, but you drop
> > <linux/device.h> despite being a direct user.
> 
> But we don't use device.h.

What is the canonical header to provide struct device?

> > If you adapt the reasoning to something like:
> >
> > Replace the inclusion of <linux/device.h> by a forward declaration of
> > struct device plus a (cheaper) #include of <linux/types.h> as
> > <linux/device.h> is an expensive include (measured in compiler effort).
> 
> Fine with me, thanks for the draft.
> 
> > I could better live with it. I would even split this into two patches
> > then. (i.e. move struct pwm_lpss_chip vs the include and forward change)
> 
> I think for this small change for a driver that hasn't been modified
> often it's fine to have them in one. But tell me if you are insisting
> on a split, I can do that.

I don't insist.

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

[Uwe Kleine-König]

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

On Thu, Nov 10, 2022 at 11:58:53AM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Nov 08, 2022 at 04:22:24PM +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.
> 
> ...
> 
> > Now that pwm_lpss_boardinfo lives in a different file, this makes the
> > move of pwm_lpss_chip in patch 3 somewhat redundant.
> 
> But they are independent changes. At each stage (after each patch) we
> should have a finished step, right? Not touching that makes me feel
> that the step is half-baked. If you insist I can drop that move from
> the other patch.

Given that the move is something you do just en passant in the earlier
patch , I consider my suggestion cleaner. I'd call that 0.5 * insist.

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

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 12:20 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Nov 10, 2022 at 11:53:59AM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 10, 2022 at 9:22 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Nov 08, 2022 at 04:22:23PM +0200, Andy Shevchenko wrote:

...

> > > > Replace device.h with a forward declaration in order to improve
> > > > the compilation time due to reducing overhead of device.h parsing
> > > > with entire train of dependencies.
> > >
> > > Together with "For the sake of integrity, include headers we are direct
> > > user of." this makes an a bit schizophrenic impression on me. You add
> > > <linux/types.h> because the file is a direct user of it, but you drop
> > > <linux/device.h> despite being a direct user.
> >
> > But we don't use device.h.
>
> What is the canonical header to provide struct device?

But we don't use the struct device here. We use _pointer_ to a struct device.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] pwm: lpss: Allow other drivers to enable PWM LPSS

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 11:23:17AM +0100, Uwe Kleine-König wrote:
> On Thu, Nov 10, 2022 at 11:58:53AM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Nov 08, 2022 at 04:22:24PM +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.

...

> > > Now that pwm_lpss_boardinfo lives in a different file, this makes the
> > > move of pwm_lpss_chip in patch 3 somewhat redundant.
> > 
> > But they are independent changes. At each stage (after each patch) we
> > should have a finished step, right? Not touching that makes me feel
> > that the step is half-baked. If you insist I can drop that move from
> > the other patch.
> 
> Given that the move is something you do just en passant in the earlier
> patch , I consider my suggestion cleaner. I'd call that 0.5 * insist.

I have looked again and I have noticed that in the current state we have

	sturct pwm_lpss_chip {
		...
	};

	struct pwm_lpss_boardinfo {
		...
	};

	extern struct pwm_lpss_boardinfo ...;


In the proposed change (with move included) it becomes

	#include <...>

	extern struct pwm_lpss_boardinfo ...;

	sturct pwm_lpss_chip {
		...
	};

and without

	#include <...>

	sturct pwm_lpss_chip {
		...
	};

	extern struct pwm_lpss_boardinfo ...;

And I consider that my way is slightly better in terms of ordering.
That said, I will leave it as is for v3. We may continue discussing
it further there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] pwm: lpss: Add pwm_lpss_probe() stub

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 12:01:50PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 9:38 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Nov 08, 2022 at 04:22:25PM +0200, Andy Shevchenko wrote:
> > > In case the PWM LPSS module is not provided, allow users to be
> > > compiled with a help of a pwm_lpss_probe() stub.

...

> > > +static inline
> > > +struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> > > +                                  const struct pwm_lpss_boardinfo *info)
> > > +{
> > > +     return ERR_PTR(-ENODEV);
> >
> > Would it be more consistent to return the same value as the pwmchip_add
> > stub does?
> 
> Then I will lose the ability to distinguish between absent driver (or
> device) and actual error during the probing of it. Any suggestions on
> how to do that better?

Independently on the above, I think that _probe() != _chip_add() semantically
and having the same, and we know weird, return code doesn't make it anyhow
better. I believe that -ENODEV is the best fit here.

That said, I leave it as is for v3 and we may continue discussing it there.

-- 
With Best Regards,
Andy Shevchenko