[PATCH 0/5] pwm: atmel-tcb: Some driver maintenance

[PATCH 0/5] pwm: atmel-tcb: Some driver maintenance

[Uwe Kleine-König]

Hello,

here come some improvements to the pwm-atmel-tcb driver.

There are still a few opportunities to improve the driver. For example
.duty shouldn't be relevant for atmel_tcb_pwm_disable(). Also the driver
could be converted from of_clk_get_by_name to devm_clk_get() (and then
also devm_pwmchip_add()). Further more I think all members of
atmel_tcb_pwm_device could be dropped as they are all only used in
.apply() after they were assigned earlier in the same function; similar
to how I removed the polarity member. Maybe someone with the hardware
wants to chime in?

Best regards
Uwe

Uwe Kleine-König (5):
  pwm: atmel-tcb: Harmonize resource allocation order
  pwm: atmel-tcb: Fix resource freeing in error path and remove
  pwm: atmel-tcb: Put per-channel data into driver data
  pwm: atmel-tcb: Unroll atmel_tcb_pwm_set_polarity() into only caller
  pwm: atmel-tcb: Don't track polarity in driver data

 drivers/pwm/pwm-atmel-tcb.c | 120 ++++++++++++++----------------------
 1 file changed, 47 insertions(+), 73 deletions(-)

base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.39.2

[PATCH 1/5] pwm: atmel-tcb: Harmonize resource allocation order

[Uwe Kleine-König]

Allocate driver data as first resource in the probe function. This way it
can be used during allocation of the other resources (instead of assigning
these to local variables first and update driver data only when it's
allocated). Also as driver data is allocated using a devm function this
should happen first to have the order of freeing resources in the error
path and the remove function in reverse.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-tcb.c | 49 +++++++++++++++----------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 4a116dc44f6e..613dd1810fb5 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -422,13 +422,14 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 	struct atmel_tcb_pwm_chip *tcbpwm;
 	const struct atmel_tcb_config *config;
 	struct device_node *np = pdev->dev.of_node;
-	struct regmap *regmap;
-	struct clk *clk, *gclk = NULL;
-	struct clk *slow_clk;
 	char clk_name[] = "t0_clk";
 	int err;
 	int channel;
 
+	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
+	if (tcbpwm == NULL)
+		return -ENOMEM;
+
 	err = of_property_read_u32(np, "reg", &channel);
 	if (err < 0) {
 		dev_err(&pdev->dev,
@@ -437,47 +438,37 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	regmap = syscon_node_to_regmap(np->parent);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
+	tcbpwm->regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(tcbpwm->regmap))
+		return PTR_ERR(tcbpwm->regmap);
 
-	slow_clk = of_clk_get_by_name(np->parent, "slow_clk");
-	if (IS_ERR(slow_clk))
-		return PTR_ERR(slow_clk);
+	tcbpwm->slow_clk = of_clk_get_by_name(np->parent, "slow_clk");
+	if (IS_ERR(tcbpwm->slow_clk))
+		return PTR_ERR(tcbpwm->slow_clk);
 
 	clk_name[1] += channel;
-	clk = of_clk_get_by_name(np->parent, clk_name);
-	if (IS_ERR(clk))
-		clk = of_clk_get_by_name(np->parent, "t0_clk");
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
+	tcbpwm->clk = of_clk_get_by_name(np->parent, clk_name);
+	if (IS_ERR(tcbpwm->clk))
+		tcbpwm->clk = of_clk_get_by_name(np->parent, "t0_clk");
+	if (IS_ERR(tcbpwm->clk))
+		return PTR_ERR(tcbpwm->clk);
 
 	match = of_match_node(atmel_tcb_of_match, np->parent);
 	config = match->data;
 
 	if (config->has_gclk) {
-		gclk = of_clk_get_by_name(np->parent, "gclk");
-		if (IS_ERR(gclk))
-			return PTR_ERR(gclk);
-	}
-
-	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
-	if (tcbpwm == NULL) {
-		err = -ENOMEM;
-		goto err_slow_clk;
+		tcbpwm->gclk = of_clk_get_by_name(np->parent, "gclk");
+		if (IS_ERR(tcbpwm->gclk))
+			return PTR_ERR(tcbpwm->gclk);
 	}
 
 	tcbpwm->chip.dev = &pdev->dev;
 	tcbpwm->chip.ops = &atmel_tcb_pwm_ops;
 	tcbpwm->chip.npwm = NPWM;
 	tcbpwm->channel = channel;
-	tcbpwm->regmap = regmap;
-	tcbpwm->clk = clk;
-	tcbpwm->gclk = gclk;
-	tcbpwm->slow_clk = slow_clk;
 	tcbpwm->width = config->counter_width;
 
-	err = clk_prepare_enable(slow_clk);
+	err = clk_prepare_enable(tcbpwm->slow_clk);
 	if (err)
 		goto err_slow_clk;
 
@@ -495,7 +486,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 	clk_disable_unprepare(tcbpwm->slow_clk);
 
 err_slow_clk:
-	clk_put(slow_clk);
+	clk_put(tcbpwm->slow_clk);
 
 	return err;
 }
-- 
2.39.2

[PATCH 2/5] pwm: atmel-tcb: Fix resource freeing in error path and remove

[Uwe Kleine-König]

Several resources were not freed in the error path and the remove
function. Add the forgotten items.

Fixes: 34cbcd72588f ("pwm: atmel-tcb: Add sama5d2 support")
Fixes: 061f8572a31c ("pwm: atmel-tcb: Switch to new binding")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-tcb.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 613dd1810fb5..2826fc216d29 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -450,16 +450,20 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 	tcbpwm->clk = of_clk_get_by_name(np->parent, clk_name);
 	if (IS_ERR(tcbpwm->clk))
 		tcbpwm->clk = of_clk_get_by_name(np->parent, "t0_clk");
-	if (IS_ERR(tcbpwm->clk))
-		return PTR_ERR(tcbpwm->clk);
+	if (IS_ERR(tcbpwm->clk)) {
+		err = PTR_ERR(tcbpwm->clk);
+		goto err_slow_clk;
+	}
 
 	match = of_match_node(atmel_tcb_of_match, np->parent);
 	config = match->data;
 
 	if (config->has_gclk) {
 		tcbpwm->gclk = of_clk_get_by_name(np->parent, "gclk");
-		if (IS_ERR(tcbpwm->gclk))
-			return PTR_ERR(tcbpwm->gclk);
+		if (IS_ERR(tcbpwm->gclk)) {
+			err = PTR_ERR(tcbpwm->gclk);
+			goto err_clk;
+		}
 	}
 
 	tcbpwm->chip.dev = &pdev->dev;
@@ -470,7 +474,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 
 	err = clk_prepare_enable(tcbpwm->slow_clk);
 	if (err)
-		goto err_slow_clk;
+		goto err_gclk;
 
 	spin_lock_init(&tcbpwm->lock);
 
@@ -485,6 +489,12 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 err_disable_clk:
 	clk_disable_unprepare(tcbpwm->slow_clk);
 
+err_gclk:
+	clk_put(tcbpwm->gclk);
+
+err_clk:
+	clk_put(tcbpwm->clk);
+
 err_slow_clk:
 	clk_put(tcbpwm->slow_clk);
 
@@ -498,8 +508,9 @@ static void atmel_tcb_pwm_remove(struct platform_device *pdev)
 	pwmchip_remove(&tcbpwm->chip);
 
 	clk_disable_unprepare(tcbpwm->slow_clk);
-	clk_put(tcbpwm->slow_clk);
+	clk_put(tcbpwm->gclk);
 	clk_put(tcbpwm->clk);
+	clk_put(tcbpwm->slow_clk);
 }
 
 static const struct of_device_id atmel_tcb_pwm_dt_ids[] = {
-- 
2.39.2

[PATCH 3/5] pwm: atmel-tcb: Put per-channel data into driver data

[Uwe Kleine-König]

This simplifies the code, reduces the number of memory allocations and
pointer dereferences.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-tcb.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 2826fc216d29..ae274bd7907d 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -57,7 +57,7 @@ struct atmel_tcb_pwm_chip {
 	struct clk *clk;
 	struct clk *gclk;
 	struct clk *slow_clk;
-	struct atmel_tcb_pwm_device *pwms[NPWM];
+	struct atmel_tcb_pwm_device pwms[NPWM];
 	struct atmel_tcb_channel bkup;
 };
 
@@ -73,7 +73,7 @@ static int atmel_tcb_pwm_set_polarity(struct pwm_chip *chip,
 				      enum pwm_polarity polarity)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
+	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 
 	tcbpwm->polarity = polarity;
 
@@ -84,19 +84,13 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
 				 struct pwm_device *pwm)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm;
+	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	unsigned cmr;
 	int ret;
 
-	tcbpwm = devm_kzalloc(chip->dev, sizeof(*tcbpwm), GFP_KERNEL);
-	if (!tcbpwm)
-		return -ENOMEM;
-
 	ret = clk_prepare_enable(tcbpwmc->clk);
-	if (ret) {
-		devm_kfree(chip->dev, tcbpwm);
+	if (ret)
 		return ret;
-	}
 
 	tcbpwm->polarity = PWM_POLARITY_NORMAL;
 	tcbpwm->duty = 0;
@@ -131,25 +125,20 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
 	regmap_write(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), cmr);
 	spin_unlock(&tcbpwmc->lock);
 
-	tcbpwmc->pwms[pwm->hwpwm] = tcbpwm;
-
 	return 0;
 }
 
 static void atmel_tcb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
 
 	clk_disable_unprepare(tcbpwmc->clk);
-	tcbpwmc->pwms[pwm->hwpwm] = NULL;
-	devm_kfree(chip->dev, tcbpwm);
 }
 
 static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
+	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	unsigned cmr;
 	enum pwm_polarity polarity = tcbpwm->polarity;
 
@@ -206,7 +195,7 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
+	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	u32 cmr;
 	enum pwm_polarity polarity = tcbpwm->polarity;
 
@@ -291,7 +280,7 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 				int duty_ns, int period_ns)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
+	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	struct atmel_tcb_pwm_device *atcbpwm = NULL;
 	int i = 0;
 	int slowclk = 0;
@@ -338,9 +327,9 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	period = div_u64(period_ns, min);
 
 	if (pwm->hwpwm == 0)
-		atcbpwm = tcbpwmc->pwms[1];
+		atcbpwm = &tcbpwmc->pwms[1];
 	else
-		atcbpwm = tcbpwmc->pwms[0];
+		atcbpwm = &tcbpwmc->pwms[0];
 
 	/*
 	 * PWM devices provided by the TCB driver are grouped by 2.
-- 
2.39.2

[PATCH 4/5] pwm: atmel-tcb: Unroll atmel_tcb_pwm_set_polarity() into only caller

[Uwe Kleine-König]

atmel_tcb_pwm_set_polarity() is only called once and effectively wraps
an assignment only. Replace the function call by the respective
assignment.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-tcb.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index ae274bd7907d..32a60d7f8ed2 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -68,18 +68,6 @@ static inline struct atmel_tcb_pwm_chip *to_tcb_chip(struct pwm_chip *chip)
 	return container_of(chip, struct atmel_tcb_pwm_chip, chip);
 }
 
-static int atmel_tcb_pwm_set_polarity(struct pwm_chip *chip,
-				      struct pwm_device *pwm,
-				      enum pwm_polarity polarity)
-{
-	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
-
-	tcbpwm->polarity = polarity;
-
-	return 0;
-}
-
 static int atmel_tcb_pwm_request(struct pwm_chip *chip,
 				 struct pwm_device *pwm)
 {
@@ -357,11 +345,12 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			       const struct pwm_state *state)
 {
+	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
+	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	int duty_cycle, period;
 	int ret;
 
-	/* This function only sets a flag in driver data */
-	atmel_tcb_pwm_set_polarity(chip, pwm, state->polarity);
+	tcbpwm->polarity = state->polarity;
 
 	if (!state->enabled) {
 		atmel_tcb_pwm_disable(chip, pwm);
-- 
2.39.2

[PATCH 5/5] pwm: atmel-tcb: Don't track polarity in driver data

[Uwe Kleine-König]

struct atmel_tcb_pwm_device::polarity is only used in atmel_tcb_pwm_enable
and atmel_tcb_pwm_disable(). These functions are only called by
atmel_tcb_pwm_apply() after the member variable was assigned to
state->polarity. So the value assigned in atmel_tcb_pwm_request() is
never used and the member can be dropped from struct atmel_tcb_pwm_device.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-tcb.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 32a60d7f8ed2..30c966238e41 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -34,7 +34,6 @@
 				 ATMEL_TC_BEEVT | ATMEL_TC_BSWTRG)
 
 struct atmel_tcb_pwm_device {
-	enum pwm_polarity polarity;	/* PWM polarity */
 	unsigned div;			/* PWM clock divider */
 	unsigned duty;			/* PWM duty expressed in clk cycles */
 	unsigned period;		/* PWM period expressed in clk cycles */
@@ -80,7 +79,6 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
 	if (ret)
 		return ret;
 
-	tcbpwm->polarity = PWM_POLARITY_NORMAL;
 	tcbpwm->duty = 0;
 	tcbpwm->period = 0;
 	tcbpwm->div = 0;
@@ -123,12 +121,12 @@ static void atmel_tcb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable_unprepare(tcbpwmc->clk);
 }
 
-static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
+				  enum pwm_polarity polarity)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
 	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	unsigned cmr;
-	enum pwm_polarity polarity = tcbpwm->polarity;
 
 	/*
 	 * If duty is 0 the timer will be stopped and we have to
@@ -180,12 +178,12 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_unlock(&tcbpwmc->lock);
 }
 
-static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+				enum pwm_polarity polarity)
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
 	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	u32 cmr;
-	enum pwm_polarity polarity = tcbpwm->polarity;
 
 	/*
 	 * If duty is 0 the timer will be stopped and we have to
@@ -345,15 +343,11 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			       const struct pwm_state *state)
 {
-	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
-	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
 	int duty_cycle, period;
 	int ret;
 
-	tcbpwm->polarity = state->polarity;
-
 	if (!state->enabled) {
-		atmel_tcb_pwm_disable(chip, pwm);
+		atmel_tcb_pwm_disable(chip, pwm, state->polarity);
 		return 0;
 	}
 
@@ -364,7 +358,7 @@ static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (ret)
 		return ret;
 
-	return atmel_tcb_pwm_enable(chip, pwm);
+	return atmel_tcb_pwm_enable(chip, pwm, state->polarity);
 }
 
 static const struct pwm_ops atmel_tcb_pwm_ops = {
-- 
2.39.2

Re: [PATCH 5/5] pwm: atmel-tcb: Don't track polarity in driver data

[claudiu beznea]

On 19.07.2023 22:20, Uwe Kleine-König wrote:
> struct atmel_tcb_pwm_device::polarity is only used in atmel_tcb_pwm_enable
> and atmel_tcb_pwm_disable(). These functions are only called by
> atmel_tcb_pwm_apply() after the member variable was assigned to
> state->polarity. So the value assigned in atmel_tcb_pwm_request() is
> never used and the member can be dropped from struct atmel_tcb_pwm_device.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pwm/pwm-atmel-tcb.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index 32a60d7f8ed2..30c966238e41 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -34,7 +34,6 @@
>   				 ATMEL_TC_BEEVT | ATMEL_TC_BSWTRG)
>   
>   struct atmel_tcb_pwm_device {
> -	enum pwm_polarity polarity;	/* PWM polarity */
>   	unsigned div;			/* PWM clock divider */
>   	unsigned duty;			/* PWM duty expressed in clk cycles */
>   	unsigned period;		/* PWM period expressed in clk cycles */
> @@ -80,7 +79,6 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>   	if (ret)
>   		return ret;
>   
> -	tcbpwm->polarity = PWM_POLARITY_NORMAL;
>   	tcbpwm->duty = 0;
>   	tcbpwm->period = 0;
>   	tcbpwm->div = 0;
> @@ -123,12 +121,12 @@ static void atmel_tcb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>   	clk_disable_unprepare(tcbpwmc->clk);
>   }
>   
> -static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  enum pwm_polarity polarity)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
>   	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	unsigned cmr;
> -	enum pwm_polarity polarity = tcbpwm->polarity;
>   
>   	/*
>   	 * If duty is 0 the timer will be stopped and we have to
> @@ -180,12 +178,12 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>   	spin_unlock(&tcbpwmc->lock);
>   }
>   
> -static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
>   	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	u32 cmr;
> -	enum pwm_polarity polarity = tcbpwm->polarity;
>   
>   	/*
>   	 * If duty is 0 the timer will be stopped and we have to
> @@ -345,15 +343,11 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   			       const struct pwm_state *state)
>   {
> -	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	int duty_cycle, period;
>   	int ret;
>   
> -	tcbpwm->polarity = state->polarity;
> -
>   	if (!state->enabled) {
> -		atmel_tcb_pwm_disable(chip, pwm);
> +		atmel_tcb_pwm_disable(chip, pwm, state->polarity);
>   		return 0;
>   	}
>   
> @@ -364,7 +358,7 @@ static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   	if (ret)
>   		return ret;
>   
> -	return atmel_tcb_pwm_enable(chip, pwm);
> +	return atmel_tcb_pwm_enable(chip, pwm, state->polarity);
>   }
>   
>   static const struct pwm_ops atmel_tcb_pwm_ops = {

Re: [PATCH 4/5] pwm: atmel-tcb: Unroll atmel_tcb_pwm_set_polarity() into only caller

[claudiu beznea]

On 19.07.2023 22:20, Uwe Kleine-König wrote:
> atmel_tcb_pwm_set_polarity() is only called once and effectively wraps
> an assignment only. Replace the function call by the respective
> assignment.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pwm/pwm-atmel-tcb.c | 17 +++--------------
>   1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index ae274bd7907d..32a60d7f8ed2 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -68,18 +68,6 @@ static inline struct atmel_tcb_pwm_chip *to_tcb_chip(struct pwm_chip *chip)
>   	return container_of(chip, struct atmel_tcb_pwm_chip, chip);
>   }
>   
> -static int atmel_tcb_pwm_set_polarity(struct pwm_chip *chip,
> -				      struct pwm_device *pwm,
> -				      enum pwm_polarity polarity)
> -{
> -	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
> -
> -	tcbpwm->polarity = polarity;
> -
> -	return 0;
> -}
> -
>   static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>   				 struct pwm_device *pwm)
>   {
> @@ -357,11 +345,12 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   			       const struct pwm_state *state)
>   {
> +	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> +	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	int duty_cycle, period;
>   	int ret;
>   
> -	/* This function only sets a flag in driver data */
> -	atmel_tcb_pwm_set_polarity(chip, pwm, state->polarity);
> +	tcbpwm->polarity = state->polarity;
>   
>   	if (!state->enabled) {
>   		atmel_tcb_pwm_disable(chip, pwm);

Re: [PATCH 3/5] pwm: atmel-tcb: Put per-channel data into driver data

[claudiu beznea]

On 19.07.2023 22:20, Uwe Kleine-König wrote:
> This simplifies the code, reduces the number of memory allocations and
> pointer dereferences.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pwm/pwm-atmel-tcb.c | 29 +++++++++--------------------
>   1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index 2826fc216d29..ae274bd7907d 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -57,7 +57,7 @@ struct atmel_tcb_pwm_chip {
>   	struct clk *clk;
>   	struct clk *gclk;
>   	struct clk *slow_clk;
> -	struct atmel_tcb_pwm_device *pwms[NPWM];
> +	struct atmel_tcb_pwm_device pwms[NPWM];
>   	struct atmel_tcb_channel bkup;
>   };
>   
> @@ -73,7 +73,7 @@ static int atmel_tcb_pwm_set_polarity(struct pwm_chip *chip,
>   				      enum pwm_polarity polarity)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
> +	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   
>   	tcbpwm->polarity = polarity;
>   
> @@ -84,19 +84,13 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>   				 struct pwm_device *pwm)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm;
> +	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	unsigned cmr;
>   	int ret;
>   
> -	tcbpwm = devm_kzalloc(chip->dev, sizeof(*tcbpwm), GFP_KERNEL);
> -	if (!tcbpwm)
> -		return -ENOMEM;
> -
>   	ret = clk_prepare_enable(tcbpwmc->clk);
> -	if (ret) {
> -		devm_kfree(chip->dev, tcbpwm);
> +	if (ret)
>   		return ret;
> -	}
>   
>   	tcbpwm->polarity = PWM_POLARITY_NORMAL;
>   	tcbpwm->duty = 0;
> @@ -131,25 +125,20 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>   	regmap_write(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), cmr);
>   	spin_unlock(&tcbpwmc->lock);
>   
> -	tcbpwmc->pwms[pwm->hwpwm] = tcbpwm;
> -
>   	return 0;
>   }
>   
>   static void atmel_tcb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
>   
>   	clk_disable_unprepare(tcbpwmc->clk);
> -	tcbpwmc->pwms[pwm->hwpwm] = NULL;
> -	devm_kfree(chip->dev, tcbpwm);
>   }
>   
>   static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
> +	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	unsigned cmr;
>   	enum pwm_polarity polarity = tcbpwm->polarity;
>   
> @@ -206,7 +195,7 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>   static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
> +	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	u32 cmr;
>   	enum pwm_polarity polarity = tcbpwm->polarity;
>   
> @@ -291,7 +280,7 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   				int duty_ns, int period_ns)
>   {
>   	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> -	struct atmel_tcb_pwm_device *tcbpwm = tcbpwmc->pwms[pwm->hwpwm];
> +	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
>   	struct atmel_tcb_pwm_device *atcbpwm = NULL;
>   	int i = 0;
>   	int slowclk = 0;
> @@ -338,9 +327,9 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	period = div_u64(period_ns, min);
>   
>   	if (pwm->hwpwm == 0)
> -		atcbpwm = tcbpwmc->pwms[1];
> +		atcbpwm = &tcbpwmc->pwms[1];
>   	else
> -		atcbpwm = tcbpwmc->pwms[0];
> +		atcbpwm = &tcbpwmc->pwms[0];
>   
>   	/*
>   	 * PWM devices provided by the TCB driver are grouped by 2.

Re: [PATCH 2/5] pwm: atmel-tcb: Fix resource freeing in error path and remove

[claudiu beznea]

On 19.07.2023 22:20, Uwe Kleine-König wrote:
> Several resources were not freed in the error path and the remove
> function. Add the forgotten items.
> 
> Fixes: 34cbcd72588f ("pwm: atmel-tcb: Add sama5d2 support")
> Fixes: 061f8572a31c ("pwm: atmel-tcb: Switch to new binding")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pwm/pwm-atmel-tcb.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index 613dd1810fb5..2826fc216d29 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -450,16 +450,20 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   	tcbpwm->clk = of_clk_get_by_name(np->parent, clk_name);
>   	if (IS_ERR(tcbpwm->clk))
>   		tcbpwm->clk = of_clk_get_by_name(np->parent, "t0_clk");
> -	if (IS_ERR(tcbpwm->clk))
> -		return PTR_ERR(tcbpwm->clk);
> +	if (IS_ERR(tcbpwm->clk)) {
> +		err = PTR_ERR(tcbpwm->clk);
> +		goto err_slow_clk;
> +	}
>   
>   	match = of_match_node(atmel_tcb_of_match, np->parent);
>   	config = match->data;
>   
>   	if (config->has_gclk) {
>   		tcbpwm->gclk = of_clk_get_by_name(np->parent, "gclk");
> -		if (IS_ERR(tcbpwm->gclk))
> -			return PTR_ERR(tcbpwm->gclk);
> +		if (IS_ERR(tcbpwm->gclk)) {
> +			err = PTR_ERR(tcbpwm->gclk);
> +			goto err_clk;
> +		}
>   	}
>   
>   	tcbpwm->chip.dev = &pdev->dev;
> @@ -470,7 +474,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   
>   	err = clk_prepare_enable(tcbpwm->slow_clk);
>   	if (err)
> -		goto err_slow_clk;
> +		goto err_gclk;
>   
>   	spin_lock_init(&tcbpwm->lock);
>   
> @@ -485,6 +489,12 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   err_disable_clk:
>   	clk_disable_unprepare(tcbpwm->slow_clk);
>   
> +err_gclk:
> +	clk_put(tcbpwm->gclk);
> +
> +err_clk:
> +	clk_put(tcbpwm->clk);
> +
>   err_slow_clk:
>   	clk_put(tcbpwm->slow_clk);
>   
> @@ -498,8 +508,9 @@ static void atmel_tcb_pwm_remove(struct platform_device *pdev)
>   	pwmchip_remove(&tcbpwm->chip);
>   
>   	clk_disable_unprepare(tcbpwm->slow_clk);
> -	clk_put(tcbpwm->slow_clk);
> +	clk_put(tcbpwm->gclk);
>   	clk_put(tcbpwm->clk);
> +	clk_put(tcbpwm->slow_clk);
>   }
>   
>   static const struct of_device_id atmel_tcb_pwm_dt_ids[] = {

Re: [PATCH 1/5] pwm: atmel-tcb: Harmonize resource allocation order

[claudiu beznea]

On 19.07.2023 22:20, Uwe Kleine-König wrote:
> Allocate driver data as first resource in the probe function. This way it
> can be used during allocation of the other resources (instead of assigning
> these to local variables first and update driver data only when it's
> allocated). Also as driver data is allocated using a devm function this
> should happen first to have the order of freeing resources in the error
> path and the remove function in reverse.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/pwm/pwm-atmel-tcb.c | 49 +++++++++++++++----------------------
>   1 file changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index 4a116dc44f6e..613dd1810fb5 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -422,13 +422,14 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   	struct atmel_tcb_pwm_chip *tcbpwm;
>   	const struct atmel_tcb_config *config;
>   	struct device_node *np = pdev->dev.of_node;
> -	struct regmap *regmap;
> -	struct clk *clk, *gclk = NULL;
> -	struct clk *slow_clk;
>   	char clk_name[] = "t0_clk";
>   	int err;
>   	int channel;
>   
> +	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> +	if (tcbpwm == NULL)

I know this was previously but maybe we can change it like this now:
	if (!tcbpwm)

this is how is done in most of the memory alloc failure checks (AFAICT).

> +		return -ENOMEM;
> +
>   	err = of_property_read_u32(np, "reg", &channel);
>   	if (err < 0) {
>   		dev_err(&pdev->dev,
> @@ -437,47 +438,37 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   		return err;
>   	}
>   
> -	regmap = syscon_node_to_regmap(np->parent);
> -	if (IS_ERR(regmap))
> -		return PTR_ERR(regmap);
> +	tcbpwm->regmap = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(tcbpwm->regmap))
> +		return PTR_ERR(tcbpwm->regmap);
>   
> -	slow_clk = of_clk_get_by_name(np->parent, "slow_clk");
> -	if (IS_ERR(slow_clk))
> -		return PTR_ERR(slow_clk);
> +	tcbpwm->slow_clk = of_clk_get_by_name(np->parent, "slow_clk");
> +	if (IS_ERR(tcbpwm->slow_clk))
> +		return PTR_ERR(tcbpwm->slow_clk);
>   
>   	clk_name[1] += channel;
> -	clk = of_clk_get_by_name(np->parent, clk_name);
> -	if (IS_ERR(clk))
> -		clk = of_clk_get_by_name(np->parent, "t0_clk");
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> +	tcbpwm->clk = of_clk_get_by_name(np->parent, clk_name);
> +	if (IS_ERR(tcbpwm->clk))
> +		tcbpwm->clk = of_clk_get_by_name(np->parent, "t0_clk");
> +	if (IS_ERR(tcbpwm->clk))
> +		return PTR_ERR(tcbpwm->clk);
>   
>   	match = of_match_node(atmel_tcb_of_match, np->parent);
>   	config = match->data;
>   
>   	if (config->has_gclk) {
> -		gclk = of_clk_get_by_name(np->parent, "gclk");
> -		if (IS_ERR(gclk))
> -			return PTR_ERR(gclk);
> -	}
> -
> -	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> -	if (tcbpwm == NULL) {
> -		err = -ENOMEM;
> -		goto err_slow_clk;
> +		tcbpwm->gclk = of_clk_get_by_name(np->parent, "gclk");
> +		if (IS_ERR(tcbpwm->gclk))
> +			return PTR_ERR(tcbpwm->gclk);
>   	}
>   
>   	tcbpwm->chip.dev = &pdev->dev;
>   	tcbpwm->chip.ops = &atmel_tcb_pwm_ops;
>   	tcbpwm->chip.npwm = NPWM;
>   	tcbpwm->channel = channel;
> -	tcbpwm->regmap = regmap;
> -	tcbpwm->clk = clk;
> -	tcbpwm->gclk = gclk;
> -	tcbpwm->slow_clk = slow_clk;
>   	tcbpwm->width = config->counter_width;
>   
> -	err = clk_prepare_enable(slow_clk);
> +	err = clk_prepare_enable(tcbpwm->slow_clk);
>   	if (err)
>   		goto err_slow_clk;
>   
> @@ -495,7 +486,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   	clk_disable_unprepare(tcbpwm->slow_clk);
>   
>   err_slow_clk:
> -	clk_put(slow_clk);
> +	clk_put(tcbpwm->slow_clk);
>   
>   	return err;
>   }

Re: [PATCH 1/5] pwm: atmel-tcb: Harmonize resource allocation order

[Uwe Kleine-König]

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

Hello claudiu,

On Thu, Jul 27, 2023 at 09:00:28AM +0300, claudiu beznea wrote:
> On 19.07.2023 22:20, Uwe Kleine-König wrote:
> > +	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> > +	if (tcbpwm == NULL)
> 
> I know this was previously but maybe we can change it like this now:
> 	if (!tcbpwm)
> 
> this is how is done in most of the memory alloc failure checks (AFAICT).
> 
> > ...
> > -	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> > -	if (tcbpwm == NULL) {

I don't care much. If and when I resend I will change to !tcbpwm.

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 0/5] pwm: atmel-tcb: Some driver maintenance

[Thierry Reding]

On Wed, 19 Jul 2023 21:20:08 +0200, Uwe Kleine-König wrote:
> here come some improvements to the pwm-atmel-tcb driver.
> 
> There are still a few opportunities to improve the driver. For example
> .duty shouldn't be relevant for atmel_tcb_pwm_disable(). Also the driver
> could be converted from of_clk_get_by_name to devm_clk_get() (and then
> also devm_pwmchip_add()). Further more I think all members of
> atmel_tcb_pwm_device could be dropped as they are all only used in
> .apply() after they were assigned earlier in the same function; similar
> to how I removed the polarity member. Maybe someone with the hardware
> wants to chime in?
> 
> [...]

Applied, thanks!

[1/5] pwm: atmel-tcb: Harmonize resource allocation order
      commit: 0323e8fedd1ef25342cf7abf3a2024f5670362b8
[2/5] pwm: atmel-tcb: Fix resource freeing in error path and remove
      commit: c11622324c023415fb69196c5fc3782d2b8cced0
[3/5] pwm: atmel-tcb: Put per-channel data into driver data
      commit: 78dca23bd6706dd6a3cdb5c0052f48794b4d2bed
[4/5] pwm: atmel-tcb: Unroll atmel_tcb_pwm_set_polarity() into only caller
      commit: 9a6ac822a2153d583b0da95b8693e954b5f4203a
[5/5] pwm: atmel-tcb: Don't track polarity in driver data
      commit: 28a1dadc49e2902d0a7a2e8c699a15f93b1b6f40

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>