[PATCH 0/5] pwm: Some improvements around .of_xlate()

[PATCH 0/5] pwm: Some improvements around .of_xlate()

[Uwe Kleine-König]

Hello,

the first patch is a fix for an out-of-bounds access and so should
probably go in quickly. The other changes are merge window material.

Best regards
Uwe

Uwe Kleine-König (5):
  pwm: Fix out-of-bounds access in of_pwm_single_xlate()
  pwm: Drop useless member .of_pwm_n_cells of struct pwm_chip
  pwm: Let the of_xlate callbacks accept references without period
  pwm: clps711x: Drop custom .of_xlate() callback
  pwm: Drop duplicate check against chip->npwm in
    of_pwm_xlate_with_flags()

 drivers/gpu/drm/bridge/ti-sn65dsi86.c |  1 -
 drivers/pwm/core.c                    | 45 +++++++--------------------
 drivers/pwm/pwm-clps711x.c            | 11 -------
 drivers/pwm/pwm-cros-ec.c             |  1 -
 drivers/pwm/pwm-pxa.c                 |  4 +--
 include/linux/pwm.h                   |  2 --
 6 files changed, 13 insertions(+), 51 deletions(-)


base-commit: d73f444d06fb8a42a5c0623453f3ea1fe9880229
-- 
2.43.0

[PATCH 2/5] pwm: Drop useless member .of_pwm_n_cells of struct pwm_chip

[Uwe Kleine-König]

Apart from the two of_xlate implementations this member is write-only.
In the of_xlate functions of_pwm_xlate_with_flags() and
of_pwm_single_xlate() it's more sensible to check for args->args_count
because this is what is actually used in the device tree.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c |  1 -
 drivers/pwm/core.c                    | 22 +++-------------------
 drivers/pwm/pwm-clps711x.c            |  1 -
 drivers/pwm/pwm-cros-ec.c             |  1 -
 drivers/pwm/pwm-pxa.c                 |  4 +---
 include/linux/pwm.h                   |  2 --
 6 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c45c07840f64..02d9449956e5 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1591,7 +1591,6 @@ static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 	pdata->pchip.ops = &ti_sn_pwm_ops;
 	pdata->pchip.npwm = 1;
 	pdata->pchip.of_xlate = of_pwm_single_xlate;
-	pdata->pchip.of_pwm_n_cells = 1;
 
 	return pwmchip_add(&pdata->pchip);
 }
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f2728ee787d7..31f210872a07 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -107,9 +107,6 @@ of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *arg
 {
 	struct pwm_device *pwm;
 
-	if (chip->of_pwm_n_cells < 2)
-		return ERR_PTR(-EINVAL);
-
 	/* flags in the third cell are optional */
 	if (args->args_count < 2)
 		return ERR_PTR(-EINVAL);
@@ -124,10 +121,8 @@ of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *arg
 	pwm->args.period = args->args[1];
 	pwm->args.polarity = PWM_POLARITY_NORMAL;
 
-	if (chip->of_pwm_n_cells >= 3) {
-		if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
-			pwm->args.polarity = PWM_POLARITY_INVERSED;
-	}
+	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
+		pwm->args.polarity = PWM_POLARITY_INVERSED;
 
 	return pwm;
 }
@@ -138,9 +133,6 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (chip->of_pwm_n_cells < 1)
-		return ERR_PTR(-EINVAL);
-
 	/* validate that one cell is specified, optionally with flags */
 	if (args->args_count != 1 && args->args_count != 2)
 		return ERR_PTR(-EINVAL);
@@ -164,16 +156,8 @@ static void of_pwmchip_add(struct pwm_chip *chip)
 	if (!chip->dev || !chip->dev->of_node)
 		return;
 
-	if (!chip->of_xlate) {
-		u32 pwm_cells;
-
-		if (of_property_read_u32(chip->dev->of_node, "#pwm-cells",
-					 &pwm_cells))
-			pwm_cells = 2;
-
+	if (!chip->of_xlate)
 		chip->of_xlate = of_pwm_xlate_with_flags;
-		chip->of_pwm_n_cells = pwm_cells;
-	}
 
 	of_node_get(chip->dev->of_node);
 }
diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
index 42179b3f7ec3..06562d4bb963 100644
--- a/drivers/pwm/pwm-clps711x.c
+++ b/drivers/pwm/pwm-clps711x.c
@@ -103,7 +103,6 @@ static int clps711x_pwm_probe(struct platform_device *pdev)
 	priv->chip.dev = &pdev->dev;
 	priv->chip.npwm = 2;
 	priv->chip.of_xlate = clps711x_pwm_xlate;
-	priv->chip.of_pwm_n_cells = 1;
 
 	spin_lock_init(&priv->lock);
 
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 5fe303b8656d..339cedf3a7b1 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -279,7 +279,6 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
 	chip->dev = dev;
 	chip->ops = &cros_ec_pwm_ops;
 	chip->of_xlate = cros_ec_pwm_xlate;
-	chip->of_pwm_n_cells = 1;
 
 	if (ec_pwm->use_pwm_type) {
 		chip->npwm = CROS_EC_PWM_DT_COUNT;
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 76685f926c75..61b74fa1d348 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -180,10 +180,8 @@ static int pwm_probe(struct platform_device *pdev)
 	pc->chip.ops = &pxa_pwm_ops;
 	pc->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
-	if (IS_ENABLED(CONFIG_OF)) {
+	if (IS_ENABLED(CONFIG_OF))
 		pc->chip.of_xlate = of_pwm_single_xlate;
-		pc->chip.of_pwm_n_cells = 1;
-	}
 
 	pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pc->mmio_base))
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index fcc2c4496f73..8ffe9ae7a23a 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -271,7 +271,6 @@ struct pwm_ops {
  * @id: unique number of this PWM chip
  * @npwm: number of PWMs controlled by this chip
  * @of_xlate: request a PWM device given a device tree PWM specifier
- * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
  * @atomic: can the driver's ->apply() be called in atomic context
  * @pwms: array of PWM devices allocated by the framework
  */
@@ -284,7 +283,6 @@ struct pwm_chip {
 
 	struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
 					const struct of_phandle_args *args);
-	unsigned int of_pwm_n_cells;
 	bool atomic;
 
 	/* only used internally by the PWM framework */
-- 
2.43.0

Re: [PATCH 2/5] pwm: Drop useless member .of_pwm_n_cells of struct pwm_chip

[Doug Anderson]

Hi,

On Tue, Jan 9, 2024 at 1:35 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Apart from the two of_xlate implementations this member is write-only.
> In the of_xlate functions of_pwm_xlate_with_flags() and
> of_pwm_single_xlate() it's more sensible to check for args->args_count
> because this is what is actually used in the device tree.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c |  1 -
>  drivers/pwm/core.c                    | 22 +++-------------------
>  drivers/pwm/pwm-clps711x.c            |  1 -
>  drivers/pwm/pwm-cros-ec.c             |  1 -
>  drivers/pwm/pwm-pxa.c                 |  4 +---
>  include/linux/pwm.h                   |  2 --
>  6 files changed, 4 insertions(+), 27 deletions(-)

I haven't done massive thinking about this, but it seems reasonable to
me. I remember being confused about why we needed some of these extra
checks ages ago when I looked at this code, so getting rid of them
makes sense to me.

I've been involved with both the ti-sn65dsi86.c and the pwm-cros-ec.c
code and both looks fine to me.

I'm an official reviewer for ti-sn65dsi86.c and I'm fairly happy with
this tag for it:

Acked-by: Douglas Anderson <dianders@chromium.org>

...and I think it would be fine to go through the PWM tree. If one of
the senior drm-misc maintainers disagrees with me, however, then you
should listen to them rather than me.

-Doug

Re: [PATCH 0/5] pwm: Some improvements around .of_xlate()

[Uwe Kleine-König]

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

Hello,

On Tue, Jan 09, 2024 at 10:34:30PM +0100, Uwe Kleine-König wrote:
> the first patch is a fix for an out-of-bounds access and so should
> probably go in quickly. The other changes are merge window material.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (5):
>   pwm: Fix out-of-bounds access in of_pwm_single_xlate()
>   pwm: Drop useless member .of_pwm_n_cells of struct pwm_chip
>   pwm: Let the of_xlate callbacks accept references without period
>   pwm: clps711x: Drop custom .of_xlate() callback
>   pwm: Drop duplicate check against chip->npwm in
>     of_pwm_xlate_with_flags()
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c |  1 -
>  drivers/pwm/core.c                    | 45 +++++++--------------------
>  drivers/pwm/pwm-clps711x.c            | 11 -------
>  drivers/pwm/pwm-cros-ec.c             |  1 -
>  drivers/pwm/pwm-pxa.c                 |  4 +--
>  include/linux/pwm.h                   |  2 --
>  6 files changed, 13 insertions(+), 51 deletions(-)

The first patch is already contained in v6.8-rc1, I applied the
remaining 4 patches to my repo at

	https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next

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