[PATCH v2 1/7] docs: firmware-guide: ACPI: Add a PWM example

[PATCH v2 1/7] docs: firmware-guide: ACPI: Add a PWM example

[Andy Shevchenko]

When PWM support for ACPI has been added into the kernel, it missed
the documentation update. Hence update documentation here.

Fixes: 4a6ef8e37c4d ("pwm: Add support referencing PWMs from ACPI")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: updated example to use 600 ms instead of 600 us (looks saner)
 .../firmware-guide/acpi/enumeration.rst       | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/enumeration.rst b/Documentation/firmware-guide/acpi/enumeration.rst
index 9f0d5c854fa4..f588663ba906 100644
--- a/Documentation/firmware-guide/acpi/enumeration.rst
+++ b/Documentation/firmware-guide/acpi/enumeration.rst
@@ -258,6 +258,38 @@ input driver::
 		.id_table	= mpu3050_ids,
 	};
 
+Reference to PWM device
+=======================
+
+Sometimes a device can be a consumer of PWM channel. Obviously OS would like
+to know which one. To provide this mapping the special property has been
+introduced, i.e.::
+
+    Device (DEV)
+    {
+        Name (_DSD, Package ()
+        {
+            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+            Package () {
+                Package () { "compatible", Package () { "pwm-leds" } },
+                Package () { "label", "alarm-led" },
+                Package () { "pwms",
+                    Package () {
+                        "\\_SB.PCI0.PWM",  // <PWM device reference>
+                        0,                 // <PWM index>
+                        600000000,         // <PWM period>
+                        0,                 // <PWM flags>
+                    }
+                }
+            }
+
+        })
+        ...
+
+In the above example the PWM-based LED driver references to the PWM channel 0
+of \_SB.PCI0.PWM device with initial period setting equal to 600 ms (note that
+value is given in nanoseconds).
+
 GPIO support
 ============
 
-- 
2.30.2

[PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Andy Shevchenko]

It makes little sense to make PWM flags optional since in case
of multi-channel consumer the flags can be optional only for
the last listed channel. Thus always require PWM flags to be
provided.

Fixes: 4a6ef8e37c4d ("pwm: Add support referencing PWMs from ACPI")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no change
 drivers/pwm/core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c165c5822703..25f7b3370672 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -852,8 +852,10 @@ static struct pwm_chip *device_to_pwmchip(struct device *dev)
  *
  * This is analogous to of_pwm_get() except con_id is not yet supported.
  * ACPI entries must look like
- * Package () {"pwms", Package ()
- *     { <PWM device reference>, <PWM index>, <PWM period> [, <PWM flags>]}}
+ * Package () { "pwms", Package () {
+ *		<PWM device reference>, <PWM index>, <PWM period>, <PWM flags>,
+ *	}
+ * }
  *
  * Returns: A pointer to the requested PWM device or an ERR_PTR()-encoded
  * error code on failure.
@@ -877,7 +879,7 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode)
 	if (!acpi)
 		return ERR_PTR(-EINVAL);
 
-	if (args.nargs < 2)
+	if (args.nargs < 3)
 		return ERR_PTR(-EPROTO);
 
 	chip = device_to_pwmchip(&acpi->dev);
@@ -891,7 +893,7 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode)
 	pwm->args.period = args.args[1];
 	pwm->args.polarity = PWM_POLARITY_NORMAL;
 
-	if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED)
+	if (args.args[2] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
 #endif
 
-- 
2.30.2

[PATCH v2 3/7] pwm: core: Convert to use fwnode for matching

[Andy Shevchenko]

When we traverse the list of the registered PWM controllers,
use fwnode to match. This will help for further cleanup.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no change
 drivers/pwm/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 25f7b3370672..338d8ee369db 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -691,14 +691,14 @@ int pwm_adjust_config(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_adjust_config);
 
-static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
+static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode)
 {
 	struct pwm_chip *chip;
 
 	mutex_lock(&pwm_lock);
 
 	list_for_each_entry(chip, &pwm_chips, list)
-		if (chip->dev && chip->dev->of_node == np) {
+		if (chip->dev && dev_fwnode(chip->dev) == fwnode) {
 			mutex_unlock(&pwm_lock);
 			return chip;
 		}
@@ -777,7 +777,7 @@ struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
 		return ERR_PTR(err);
 	}
 
-	pc = of_node_to_pwmchip(args.np);
+	pc = fwnode_to_pwmchip(of_fwnode_handle(args.np));
 	if (IS_ERR(pc)) {
 		if (PTR_ERR(pc) != -EPROBE_DEFER)
 			pr_err("%s(): PWM chip not found\n", __func__);
-- 
2.30.2

[PATCH v2 4/7] pwm: core: Reuse fwnode_to_pwmchip() in ACPI case

[Andy Shevchenko]

In ACPI case we may use matching by fwnode as provided via
fwnode_to_pwmchip(). This makes device_to_pwmchip() not needed
anymore.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no change
 drivers/pwm/core.c | 31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 338d8ee369db..2223a9b6644b 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -819,28 +819,6 @@ struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(of_pwm_get);
 
-#if IS_ENABLED(CONFIG_ACPI)
-static struct pwm_chip *device_to_pwmchip(struct device *dev)
-{
-	struct pwm_chip *chip;
-
-	mutex_lock(&pwm_lock);
-
-	list_for_each_entry(chip, &pwm_chips, list) {
-		struct acpi_device *adev = ACPI_COMPANION(chip->dev);
-
-		if ((chip->dev == dev) || (adev && &adev->dev == dev)) {
-			mutex_unlock(&pwm_lock);
-			return chip;
-		}
-	}
-
-	mutex_unlock(&pwm_lock);
-
-	return ERR_PTR(-EPROBE_DEFER);
-}
-#endif
-
 /**
  * acpi_pwm_get() - request a PWM via parsing "pwms" property in ACPI
  * @fwnode: firmware node to get the "pwm" property from
@@ -863,9 +841,7 @@ static struct pwm_chip *device_to_pwmchip(struct device *dev)
 static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode)
 {
 	struct pwm_device *pwm = ERR_PTR(-ENODEV);
-#if IS_ENABLED(CONFIG_ACPI)
 	struct fwnode_reference_args args;
-	struct acpi_device *acpi;
 	struct pwm_chip *chip;
 	int ret;
 
@@ -875,14 +851,10 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode)
 	if (ret < 0)
 		return ERR_PTR(ret);
 
-	acpi = to_acpi_device_node(args.fwnode);
-	if (!acpi)
-		return ERR_PTR(-EINVAL);
-
 	if (args.nargs < 3)
 		return ERR_PTR(-EPROTO);
 
-	chip = device_to_pwmchip(&acpi->dev);
+	chip = fwnode_to_pwmchip(args.fwnode);
 	if (IS_ERR(chip))
 		return ERR_CAST(chip);
 
@@ -895,7 +867,6 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode)
 
 	if (args.args[2] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
-#endif
 
 	return pwm;
 }
-- 
2.30.2

[PATCH v2 5/7] pwm: core: Unify fwnode checks in the module

[Andy Shevchenko]

Historically we have two different approaches on how to check type of fwnode.
Unify them using the latest and greatest fwnode related APIs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: corrected property name in kernel doc
 drivers/pwm/core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2223a9b6644b..a27d7c2aa6fe 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -821,7 +821,7 @@ EXPORT_SYMBOL_GPL(of_pwm_get);
 
 /**
  * acpi_pwm_get() - request a PWM via parsing "pwms" property in ACPI
- * @fwnode: firmware node to get the "pwm" property from
+ * @fwnode: firmware node to get the "pwms" property from
  *
  * Returns the PWM device parsed from the fwnode and index specified in the
  * "pwms" property or a negative error-code on failure.
@@ -838,7 +838,7 @@ EXPORT_SYMBOL_GPL(of_pwm_get);
  * Returns: A pointer to the requested PWM device or an ERR_PTR()-encoded
  * error code on failure.
  */
-static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode)
+static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode)
 {
 	struct pwm_device *pwm = ERR_PTR(-ENODEV);
 	struct fwnode_reference_args args;
@@ -922,6 +922,7 @@ void pwm_remove_table(struct pwm_lookup *table, size_t num)
  */
 struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 {
+	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct pwm_device *pwm;
 	struct pwm_chip *chip;
@@ -932,12 +933,12 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 	int err;
 
 	/* look up via DT first */
-	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
-		return of_pwm_get(dev, dev->of_node, con_id);
+	if (is_of_node(fwnode))
+		return of_pwm_get(dev, to_of_node(fwnode), con_id);
 
 	/* then lookup via ACPI */
-	if (dev && is_acpi_node(dev->fwnode)) {
-		pwm = acpi_pwm_get(dev->fwnode);
+	if (is_acpi_node(fwnode)) {
+		pwm = acpi_pwm_get(fwnode);
 		if (!IS_ERR(pwm) || PTR_ERR(pwm) != -ENOENT)
 			return pwm;
 	}
-- 
2.30.2

[PATCH v2 6/7] pwm: core: Remove unused devm_pwm_put()

[Andy Shevchenko]

There are no users and seems no will come of the devm_pwm_put().
Remove the function.

While at it, slightly update documentation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 .../driver-api/driver-model/devres.rst        |  3 ++-
 Documentation/driver-api/pwm.rst              |  3 ++-
 drivers/pwm/core.c                            | 25 -------------------
 include/linux/pwm.h                           |  5 ----
 4 files changed, 4 insertions(+), 32 deletions(-)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index b2645870ef7e..cc1e0138ba9f 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -410,7 +410,8 @@ POWER
 
 PWM
   devm_pwm_get()
-  devm_pwm_put()
+  devm_of_pwm_get()
+  devm_fwnode_pwm_get()
 
 REGULATOR
   devm_regulator_bulk_get()
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index a7ca4f58305a..251e3f7be230 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -40,7 +40,8 @@ after usage with pwm_free().
 
 New users should use the pwm_get() function and pass to it the consumer
 device or a consumer name. pwm_put() is used to free the PWM device. Managed
-variants of these functions, devm_pwm_get() and devm_pwm_put(), also exist.
+variants of the getter, devm_pwm_get(), devm_of_pwm_get(),
+devm_fwnode_pwm_get(), also exist.
 
 After being requested, a PWM has to be configured using::
 
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a27d7c2aa6fe..6d4410bd9793 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1166,31 +1166,6 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
 
-static int devm_pwm_match(struct device *dev, void *res, void *data)
-{
-	struct pwm_device **p = res;
-
-	if (WARN_ON(!p || !*p))
-		return 0;
-
-	return *p == data;
-}
-
-/**
- * devm_pwm_put() - resource managed pwm_put()
- * @dev: device for PWM consumer
- * @pwm: PWM device
- *
- * Release a PWM previously allocated using devm_pwm_get(). Calling this
- * function is usually not needed because devm-allocated resources are
- * automatically released on driver detach.
- */
-void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
-{
-	WARN_ON(devres_release(dev, devm_pwm_release, devm_pwm_match, pwm));
-}
-EXPORT_SYMBOL_GPL(devm_pwm_put);
-
 #ifdef CONFIG_DEBUG_FS
 static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 {
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 5bb90af4997e..1eaded57c666 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -417,7 +417,6 @@ struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
 struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 				       struct fwnode_handle *fwnode,
 				       const char *con_id);
-void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
 #else
 static inline struct pwm_device *pwm_request(int pwm_id, const char *label)
 {
@@ -524,10 +523,6 @@ devm_fwnode_pwm_get(struct device *dev, struct fwnode_handle *fwnode,
 {
 	return ERR_PTR(-ENODEV);
 }
-
-static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
-{
-}
 #endif
 
 static inline void pwm_apply_args(struct pwm_device *pwm)
-- 
2.30.2

[PATCH v2 7/7] pwm: core: Simplify some devm_*pwm*() functions

[Andy Shevchenko]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/pwm/core.c | 60 +++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6d4410bd9793..9f643414676b 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1059,9 +1059,9 @@ void pwm_put(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_put);
 
-static void devm_pwm_release(struct device *dev, void *res)
+static void devm_pwm_release(void *pwm)
 {
-	pwm_put(*(struct pwm_device **)res);
+	pwm_put(pwm);
 }
 
 /**
@@ -1077,19 +1077,16 @@ static void devm_pwm_release(struct device *dev, void *res)
  */
 struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id)
 {
-	struct pwm_device **ptr, *pwm;
-
-	ptr = devres_alloc(devm_pwm_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct pwm_device *pwm;
+	int ret;
 
 	pwm = pwm_get(dev, con_id);
-	if (!IS_ERR(pwm)) {
-		*ptr = pwm;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (IS_ERR(pwm))
+		return pwm;
+
+	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return pwm;
 }
@@ -1110,19 +1107,16 @@ EXPORT_SYMBOL_GPL(devm_pwm_get);
 struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
 				   const char *con_id)
 {
-	struct pwm_device **ptr, *pwm;
-
-	ptr = devres_alloc(devm_pwm_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct pwm_device *pwm;
+	int ret;
 
 	pwm = of_pwm_get(dev, np, con_id);
-	if (!IS_ERR(pwm)) {
-		*ptr = pwm;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (IS_ERR(pwm))
+		return pwm;
+
+	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return pwm;
 }
@@ -1144,23 +1138,19 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 				       struct fwnode_handle *fwnode,
 				       const char *con_id)
 {
-	struct pwm_device **ptr, *pwm = ERR_PTR(-ENODEV);
-
-	ptr = devres_alloc(devm_pwm_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct pwm_device *pwm = ERR_PTR(-ENODEV);
+	int ret;
 
 	if (is_of_node(fwnode))
 		pwm = of_pwm_get(dev, to_of_node(fwnode), con_id);
 	else if (is_acpi_node(fwnode))
 		pwm = acpi_pwm_get(fwnode);
+	if (IS_ERR(pwm))
+		return pwm;
 
-	if (!IS_ERR(pwm)) {
-		*ptr = pwm;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return pwm;
 }
-- 
2.30.2

Re: [PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Uwe Kleine-König]

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

Hello Andy,

On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> It makes little sense to make PWM flags optional since in case
> of multi-channel consumer the flags can be optional only for
> the last listed channel.

I think the same holds true for dt references.

> Thus always require PWM flags to be provided.

I'm not sure I want to follow that conclusion. Most consumers only need
a single PWM and then not providing flags is fine, isn't it? Or does
this change fix an error?

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/7] pwm: core: Remove unused devm_pwm_put()

[Uwe Kleine-König]

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

Hello,

On Mon, May 31, 2021 at 10:49:46PM +0300, Andy Shevchenko wrote:
> There are no users and seems no will come of the devm_pwm_put().
> Remove the function.
> 
> While at it, slightly update documentation.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  .../driver-api/driver-model/devres.rst        |  3 ++-
>  Documentation/driver-api/pwm.rst              |  3 ++-
>  drivers/pwm/core.c                            | 25 -------------------
>  include/linux/pwm.h                           |  5 ----
>  4 files changed, 4 insertions(+), 32 deletions(-)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index b2645870ef7e..cc1e0138ba9f 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -410,7 +410,8 @@ POWER
>  
>  PWM
>    devm_pwm_get()
> -  devm_pwm_put()
> +  devm_of_pwm_get()
> +  devm_fwnode_pwm_get()
>  
>  REGULATOR
>    devm_regulator_bulk_get()
> diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
> index a7ca4f58305a..251e3f7be230 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -40,7 +40,8 @@ after usage with pwm_free().
>  
>  New users should use the pwm_get() function and pass to it the consumer
>  device or a consumer name. pwm_put() is used to free the PWM device. Managed
> -variants of these functions, devm_pwm_get() and devm_pwm_put(), also exist.
> +variants of the getter, devm_pwm_get(), devm_of_pwm_get(),
> +devm_fwnode_pwm_get(), also exist.
>  
>  After being requested, a PWM has to be configured using::
>  
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index a27d7c2aa6fe..6d4410bd9793 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -1166,31 +1166,6 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
>  
> -static int devm_pwm_match(struct device *dev, void *res, void *data)
> -{
> -	struct pwm_device **p = res;
> -
> -	if (WARN_ON(!p || !*p))
> -		return 0;
> -
> -	return *p == data;
> -}
> -
> -/**
> - * devm_pwm_put() - resource managed pwm_put()
> - * @dev: device for PWM consumer
> - * @pwm: PWM device
> - *
> - * Release a PWM previously allocated using devm_pwm_get(). Calling this
> - * function is usually not needed because devm-allocated resources are
> - * automatically released on driver detach.
> - */
> -void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
> -{
> -	WARN_ON(devres_release(dev, devm_pwm_release, devm_pwm_match, pwm));
> -}
> -EXPORT_SYMBOL_GPL(devm_pwm_put);
> -
>  #ifdef CONFIG_DEBUG_FS
>  static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  {
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 5bb90af4997e..1eaded57c666 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -417,7 +417,6 @@ struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
>  struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
>  				       struct fwnode_handle *fwnode,
>  				       const char *con_id);
> -void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
>  #else
>  static inline struct pwm_device *pwm_request(int pwm_id, const char *label)
>  {
> @@ -524,10 +523,6 @@ devm_fwnode_pwm_get(struct device *dev, struct fwnode_handle *fwnode,
>  {
>  	return ERR_PTR(-ENODEV);
>  }
> -
> -static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
> -{
> -}
>  #endif
>  
>  static inline void pwm_apply_args(struct pwm_device *pwm)

Nice.

Reviewed-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 7/7] pwm: core: Simplify some devm_*pwm*() functions

[Uwe Kleine-König]

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

Hello Andy,

On Mon, May 31, 2021 at 10:49:47PM +0300, Andy Shevchenko wrote:
> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  drivers/pwm/core.c | 60 +++++++++++++++++++---------------------------
>  1 file changed, 25 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6d4410bd9793..9f643414676b 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -1059,9 +1059,9 @@ void pwm_put(struct pwm_device *pwm)
>  }
>  EXPORT_SYMBOL_GPL(pwm_put);
>  
> -static void devm_pwm_release(struct device *dev, void *res)
> +static void devm_pwm_release(void *pwm)
>  {
> -	pwm_put(*(struct pwm_device **)res);
> +	pwm_put(pwm);
>  }
>  
>  /**
> @@ -1077,19 +1077,16 @@ static void devm_pwm_release(struct device *dev, void *res)
>   */
>  struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id)
>  {
> -	struct pwm_device **ptr, *pwm;
> -
> -	ptr = devres_alloc(devm_pwm_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct pwm_device *pwm;
> +	int ret;
>  
>  	pwm = pwm_get(dev, con_id);
> -	if (!IS_ERR(pwm)) {
> -		*ptr = pwm;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> -	}
> +	if (IS_ERR(pwm))
> +		return pwm;
> +
> +	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return pwm;
>  }
> @@ -1110,19 +1107,16 @@ EXPORT_SYMBOL_GPL(devm_pwm_get);
>  struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
>  				   const char *con_id)
>  {
> -	struct pwm_device **ptr, *pwm;
> -
> -	ptr = devres_alloc(devm_pwm_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct pwm_device *pwm;
> +	int ret;
>  
>  	pwm = of_pwm_get(dev, np, con_id);
> -	if (!IS_ERR(pwm)) {
> -		*ptr = pwm;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> -	}
> +	if (IS_ERR(pwm))
> +		return pwm;
> +
> +	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return pwm;
>  }
> @@ -1144,23 +1138,19 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
>  				       struct fwnode_handle *fwnode,
>  				       const char *con_id)
>  {
> -	struct pwm_device **ptr, *pwm = ERR_PTR(-ENODEV);
> -
> -	ptr = devres_alloc(devm_pwm_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct pwm_device *pwm = ERR_PTR(-ENODEV);
> +	int ret;
>  
>  	if (is_of_node(fwnode))
>  		pwm = of_pwm_get(dev, to_of_node(fwnode), con_id);
>  	else if (is_acpi_node(fwnode))
>  		pwm = acpi_pwm_get(fwnode);
> +	if (IS_ERR(pwm))
> +		return pwm;
>  
> -	if (!IS_ERR(pwm)) {
> -		*ptr = pwm;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> -	}
> +	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
> +	if (ret)
> +		return ERR_PTR(ret);
>  

Another nice one:

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 2/7] pwm: core: Always require PWM flags to be provided

[Andy Shevchenko]

On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> Hello Andy,
> 
> On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > It makes little sense to make PWM flags optional since in case
> > of multi-channel consumer the flags can be optional only for
> > the last listed channel.
> 
> I think the same holds true for dt references.

Can you elaborate this? I haven't got what you are talking about, not a DT
expert here.

> > Thus always require PWM flags to be provided.
> 
> I'm not sure I want to follow that conclusion. Most consumers only need
> a single PWM and then not providing flags is fine, isn't it? Or does
> this change fix an error?

The current ACPI DSD implementation in the Linux kernel doesn't allow to use
variadic arguments in the list of tuples.

So, the current code states that we may use (x, y, z) or (x, y, z, t).
That's true as long as that is the last tuple in the array (*). However,
the case (x1, y1, z1, x2, y2, z2, t2) is not supported. It can be either
(x1, y1, z1, t1, x2, y2, z2) or (x1, y1, z1, t1, x2, y2, z2, t2).
But this makes a little sense and Linux APIs in use and first ABI that uses
that API (GPIO) has never been designed for a such. What is allowed is to skip
tuple in a form of NULL:ifying, like (0, x2, y2, z2, t2).

*) Yes, the comments said that PWM supports only a single reference, but this
may be expanded in the future and this patch allows to do that (it's not a fix
per se, but rather a good clarification to the APIs/ABIs).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Uwe Kleine-König]

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

Hi Andy,

On Mon, Jun 07, 2021 at 12:02:37PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> > Hello Andy,
> > 
> > On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > > It makes little sense to make PWM flags optional since in case
> > > of multi-channel consumer the flags can be optional only for
> > > the last listed channel.
> > 
> > I think the same holds true for dt references.
> 
> Can you elaborate this? I haven't got what you are talking about, not a DT
> expert here.

Ah no, I mixed that up. While the function that parses the phandle is
flexible, for each pwm controller the number of arguments is fixed, so

	pwms = <&pwm1 100000 &pwm2 100000 &pwm3 1000000>;

cannot be interpreted as 3-argument references to two PWMs. This is
different to ACPI (I guess, not an ACPI expert here :-) because &pwm1
"knows" if it needs 1 or 2 additional parameters (#pwm-cells).

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 2/7] pwm: core: Always require PWM flags to be provided

[Andy Shevchenko]

On Mon, Jun 07, 2021 at 11:53:24AM +0200, Uwe Kleine-König wrote:
> On Mon, Jun 07, 2021 at 12:02:37PM +0300, Andy Shevchenko wrote:
> > On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > > > It makes little sense to make PWM flags optional since in case
> > > > of multi-channel consumer the flags can be optional only for
> > > > the last listed channel.
> > > 
> > > I think the same holds true for dt references.
> > 
> > Can you elaborate this? I haven't got what you are talking about, not a DT
> > expert here.
> 
> Ah no, I mixed that up. While the function that parses the phandle is
> flexible, for each pwm controller the number of arguments is fixed, so
> 
> 	pwms = <&pwm1 100000 &pwm2 100000 &pwm3 1000000>;
> 
> cannot be interpreted as 3-argument references to two PWMs. This is
> different to ACPI (I guess, not an ACPI expert here :-) because &pwm1
> "knows" if it needs 1 or 2 additional parameters (#pwm-cells).

It's not about ACPI, it's about "the ACPI glue layer in Linux kernel".
Used API is a part of it and it does allow only two cases, either NULL entry
(by having 0 as an argument) or full-length supplied tuple (in case of PWM it's
3, so, means 4 parameters.

Let's consider examples:

(0, 0, x3, y3, z3, t3) // NULL, NULL, PWM3
(x1, y1, z1, t1, 0, x3, y3, z3, t3) // PWM1, NULL, PWM3

So, making last parameter "flexible" will work only for the last tuple in the
array.

Read this [1] for further information.

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/acpi/property.c#L629

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Andy Shevchenko]

On Mon, Jun 07, 2021 at 01:15:20PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 07, 2021 at 11:53:24AM +0200, Uwe Kleine-König wrote:
> > On Mon, Jun 07, 2021 at 12:02:37PM +0300, Andy Shevchenko wrote:
> > > On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > > > > It makes little sense to make PWM flags optional since in case
> > > > > of multi-channel consumer the flags can be optional only for
> > > > > the last listed channel.
> > > > 
> > > > I think the same holds true for dt references.
> > > 
> > > Can you elaborate this? I haven't got what you are talking about, not a DT
> > > expert here.
> > 
> > Ah no, I mixed that up. While the function that parses the phandle is
> > flexible, for each pwm controller the number of arguments is fixed, so
> > 
> > 	pwms = <&pwm1 100000 &pwm2 100000 &pwm3 1000000>;
> > 
> > cannot be interpreted as 3-argument references to two PWMs. This is
> > different to ACPI (I guess, not an ACPI expert here :-) because &pwm1
> > "knows" if it needs 1 or 2 additional parameters (#pwm-cells).
> 
> It's not about ACPI, it's about "the ACPI glue layer in Linux kernel".
> Used API is a part of it and it does allow only two cases, either NULL entry
> (by having 0 as an argument) or full-length supplied tuple (in case of PWM it's
> 3, so, means 4 parameters.
> 
> Let's consider examples:
> 
> (0, 0, x3, y3, z3, t3) // NULL, NULL, PWM3
> (x1, y1, z1, t1, 0, x3, y3, z3, t3) // PWM1, NULL, PWM3
> 
> So, making last parameter "flexible" will work only for the last tuple in the
> array.
> 
> Read this [1] for further information.
> 
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/acpi/property.c#L629

Hmm... I have read the actual implementation and it seems it's possible to have
flexible array, so this patch needs to be reconsidered.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Andy Shevchenko]

On Mon, Jun 07, 2021 at 01:21:01PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 07, 2021 at 01:15:20PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 07, 2021 at 11:53:24AM +0200, Uwe Kleine-König wrote:
> > > On Mon, Jun 07, 2021 at 12:02:37PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> > > > > On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > > > > > It makes little sense to make PWM flags optional since in case
> > > > > > of multi-channel consumer the flags can be optional only for
> > > > > > the last listed channel.
> > > > > 
> > > > > I think the same holds true for dt references.
> > > > 
> > > > Can you elaborate this? I haven't got what you are talking about, not a DT
> > > > expert here.
> > > 
> > > Ah no, I mixed that up. While the function that parses the phandle is
> > > flexible, for each pwm controller the number of arguments is fixed, so
> > > 
> > > 	pwms = <&pwm1 100000 &pwm2 100000 &pwm3 1000000>;
> > > 
> > > cannot be interpreted as 3-argument references to two PWMs. This is
> > > different to ACPI (I guess, not an ACPI expert here :-) because &pwm1
> > > "knows" if it needs 1 or 2 additional parameters (#pwm-cells).
> > 
> > It's not about ACPI, it's about "the ACPI glue layer in Linux kernel".
> > Used API is a part of it and it does allow only two cases, either NULL entry
> > (by having 0 as an argument) or full-length supplied tuple (in case of PWM it's
> > 3, so, means 4 parameters.
> > 
> > Let's consider examples:
> > 
> > (0, 0, x3, y3, z3, t3) // NULL, NULL, PWM3
> > (x1, y1, z1, t1, 0, x3, y3, z3, t3) // PWM1, NULL, PWM3
> > 
> > So, making last parameter "flexible" will work only for the last tuple in the
> > array.
> > 
> > Read this [1] for further information.
> > 
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/acpi/property.c#L629
> 
> Hmm... I have read the actual implementation and it seems it's possible to have
> flexible array, so this patch needs to be reconsidered.

I was thinking more about it and what we have here is positional-dependent
arguments. Either way we might end up in the same situation (when we need to
parse arguments based on their positions, rather than always have them being
present). So, while I won't change documentation example (to be more stricter
there), I will drop this change.

Also, the PWM initial state doesn't include duty cycle. Any explanations why is
that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Uwe Kleine-König]

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

Hello Andy,

On Mon, Jun 07, 2021 at 02:49:01PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 07, 2021 at 01:21:01PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 07, 2021 at 01:15:20PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 07, 2021 at 11:53:24AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Jun 07, 2021 at 12:02:37PM +0300, Andy Shevchenko wrote:
> > > > > On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> > > > > > On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > > > > > > It makes little sense to make PWM flags optional since in case
> > > > > > > of multi-channel consumer the flags can be optional only for
> > > > > > > the last listed channel.
> > > > > > 
> > > > > > I think the same holds true for dt references.
> > > > > 
> > > > > Can you elaborate this? I haven't got what you are talking about, not a DT
> > > > > expert here.
> > > > 
> > > > Ah no, I mixed that up. While the function that parses the phandle is
> > > > flexible, for each pwm controller the number of arguments is fixed, so
> > > > 
> > > > 	pwms = <&pwm1 100000 &pwm2 100000 &pwm3 1000000>;
> > > > 
> > > > cannot be interpreted as 3-argument references to two PWMs. This is
> > > > different to ACPI (I guess, not an ACPI expert here :-) because &pwm1
> > > > "knows" if it needs 1 or 2 additional parameters (#pwm-cells).
> > > 
> > > It's not about ACPI, it's about "the ACPI glue layer in Linux kernel".
> > > Used API is a part of it and it does allow only two cases, either NULL entry
> > > (by having 0 as an argument) or full-length supplied tuple (in case of PWM it's
> > > 3, so, means 4 parameters.
> > > 
> > > Let's consider examples:
> > > 
> > > (0, 0, x3, y3, z3, t3) // NULL, NULL, PWM3
> > > (x1, y1, z1, t1, 0, x3, y3, z3, t3) // PWM1, NULL, PWM3
> > > 
> > > So, making last parameter "flexible" will work only for the last tuple in the
> > > array.
> > > 
> > > Read this [1] for further information.
> > > 
> > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/acpi/property.c#L629
> > 
> > Hmm... I have read the actual implementation and it seems it's possible to have
> > flexible array, so this patch needs to be reconsidered.
> 
> I was thinking more about it and what we have here is positional-dependent
> arguments. Either way we might end up in the same situation (when we need to
> parse arguments based on their positions, rather than always have them being
> present). So, while I won't change documentation example (to be more stricter
> there), I will drop this change.
> 
> Also, the PWM initial state doesn't include duty cycle. Any explanations why is
> that?

This isn't technically the initial state. It's a hint to the consumer
which period to pick. The duty-cycle is usually variable, but if I
designed the binding today I would not include the period in the pwm
handle. But to discuss this is moot---the binding is as it 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 --]