[PATCH v2 0/3] pwm: get rid of pwmchip_add_with_polarity()

[PATCH v2 0/3] pwm: get rid of pwmchip_add_with_polarity()

[Uwe Kleine-König]

Hello,

in the (implicit) v1 of this series (starting with Message-Id:
20201205165146.3866846-1-uwe@kleine-koenig.org) I forget to include the
change to drivers/pwm/core.c. This is fixed here.

IMHO It would be good to cook this in next for some time to make it (a
bit more) sure it doesn't introduce a regression.

Best regards
Uwe

Uwe Kleine-König (3):
  pwm: bcm-kona: Use pwmchip_add() instead of
    pwmchip_add_with_polarity()
  pwm: atmel-hlcdc: Use pwmchip_add() instead of
    pwmchip_add_with_polarity()
  pwm: Drop function pwmchip_add_with_polarity()

 drivers/pwm/core.c            | 25 +++----------------------
 drivers/pwm/pwm-atmel-hlcdc.c |  2 +-
 drivers/pwm/pwm-bcm-kona.c    |  2 +-
 include/linux/pwm.h           |  2 --
 4 files changed, 5 insertions(+), 26 deletions(-)

-- 
2.29.2

[PATCH v2 1/3] pwm: bcm-kona: Use pwmchip_add() instead of pwmchip_add_with_polarity()

[Uwe Kleine-König]

The only side effect of this change is that pwm->state.polarity is
initialized to PWM_POLARITY_NORMAL instead of PWM_POLARITY_INVERSED.
However all other members of pwm->state are uninitialized and consumers
are expected to provide the right polarity (either by setting it explicitly
or by using a helper like pwm_init_state() that overwrites .polarity
anyhow with a value independent of the initial value).

The eventual goal is to remove pwmchip_add_with_polarity() and so simplify
the data flow in the PWM core.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/pwm-bcm-kona.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 16c5898b934a..40ecfb2bb632 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -303,7 +303,7 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 
 	clk_disable_unprepare(kp->clk);
 
-	ret = pwmchip_add_with_polarity(&kp->chip, PWM_POLARITY_INVERSED);
+	ret = pwmchip_add(&kp->chip);
 	if (ret < 0)
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
 
-- 
2.29.2

Re: [PATCH v2 1/3] pwm: bcm-kona: Use pwmchip_add() instead of pwmchip_add_with_polarity()

[Thierry Reding]

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

On Mon, Dec 07, 2020 at 02:45:54PM +0100, Uwe Kleine-König wrote:
> The only side effect of this change is that pwm->state.polarity is
> initialized to PWM_POLARITY_NORMAL instead of PWM_POLARITY_INVERSED.
> However all other members of pwm->state are uninitialized and consumers
> are expected to provide the right polarity (either by setting it explicitly
> or by using a helper like pwm_init_state() that overwrites .polarity
> anyhow with a value independent of the initial value).
> 
> The eventual goal is to remove pwmchip_add_with_polarity() and so simplify
> the data flow in the PWM core.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I was initially reluctant to apply this because the purpose of this API
was to mark chips as deviating from the default polarity. That is to say
that this chip's default is to produce an inversed PWM. However, I see
that the driver already deals with that properly during ->apply(), so
there is no reason for this to stay around.

The details are now a bit blurry, but I think initially the idea had
been to let the core handle this in some way (e.g. by rejecting a
polarity setting if ->set_polarity() wasn't implemented and the polarity
was not INVERSED for chips like this), but that's not happening in
practice.

Going forward this should all be dealt with at the driver level. Drivers
that support both polarities should just program them appropriately and
drivers that support only one should flat out refuse to set the other
one. One potential issue is for drivers that will use "normal" polarity
by default, but will work fine with "inverse" polarity (and,
correspondingly, an inverted duty-cycle/period ratio). I guess for those
they could always try with "normal" polarity and if that fails retry
with an inverted polarity and invert duty-cycle/period ratio.

Applied, thanks.

Thierry

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

[PATCH v2 2/3] pwm: atmel-hlcdc: Use pwmchip_add() instead of pwmchip_add_with_polarity()

[Uwe Kleine-König]

The only side effect of this change is that pwm->state.polarity is
initialized to PWM_POLARITY_NORMAL instead of PWM_POLARITY_INVERSED.
However all other members of pwm->state are uninitialized and consumers
are expected to provide the right polarity (either by setting it explicitly
or by using a helper like pwm_init_state() that overwrites .polarity
anyhow with a value independent of the initial value).

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index dcbc0489dfd4..4551aa2c484c 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -270,7 +270,7 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 	chip->chip.of_xlate = of_pwm_xlate_with_flags;
 	chip->chip.of_pwm_n_cells = 3;
 
-	ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED);
+	ret = pwmchip_add(&chip->chip);
 	if (ret) {
 		clk_disable_unprepare(hlcdc->periph_clk);
 		return ret;
-- 
2.29.2

Re: [PATCH v2 2/3] pwm: atmel-hlcdc: Use pwmchip_add() instead of pwmchip_add_with_polarity()

[Thierry Reding]

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

On Mon, Dec 07, 2020 at 02:45:55PM +0100, Uwe Kleine-König wrote:
> The only side effect of this change is that pwm->state.polarity is
> initialized to PWM_POLARITY_NORMAL instead of PWM_POLARITY_INVERSED.
> However all other members of pwm->state are uninitialized and consumers
> are expected to provide the right polarity (either by setting it explicitly
> or by using a helper like pwm_init_state() that overwrites .polarity
> anyhow with a value independent of the initial value).
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

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

[PATCH v2 3/3] pwm: Drop function pwmchip_add_with_polarity()

[Uwe Kleine-König]

pwmchip_add() only calls pwmchip_add_with_polarity() and nothing else. All
other users of pwmchip_add_with_polarity() are gone. So drop
pwmchip_add_with_polarity() and move the code instead to pwmchip_add().

The initial assignment to pwm->state.polarity is dropped. In every correct
usage of the PWM API this value is overwritten later anyhow.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/core.c  | 25 +++----------------------
 include/linux/pwm.h |  2 --
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1f16f5365d3c..78611487d887 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -260,18 +260,15 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
 }
 
 /**
- * pwmchip_add_with_polarity() - register a new PWM chip
+ * pwmchip_add() - register a new PWM chip
  * @chip: the PWM chip to add
- * @polarity: initial polarity of PWM channels
  *
  * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used. The initial polarity for all channels is specified by the
- * @polarity parameter.
+ * will be used.
  *
  * Returns: 0 on success or a negative error code on failure.
  */
-int pwmchip_add_with_polarity(struct pwm_chip *chip,
-			      enum pwm_polarity polarity)
+int pwmchip_add(struct pwm_chip *chip)
 {
 	struct pwm_device *pwm;
 	unsigned int i;
@@ -303,7 +300,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->chip = chip;
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
-		pwm->state.polarity = polarity;
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -326,21 +322,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(pwmchip_add_with_polarity);
-
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used. The initial polarity for all channels is normal.
- *
- * Returns: 0 on success or a negative error code on failure.
- */
-int pwmchip_add(struct pwm_chip *chip)
-{
-	return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
-}
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
 /**
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e4d84d4db293..8f4eefd129aa 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -392,8 +392,6 @@ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 int pwm_set_chip_data(struct pwm_device *pwm, void *data);
 void *pwm_get_chip_data(struct pwm_device *pwm);
 
-int pwmchip_add_with_polarity(struct pwm_chip *chip,
-			      enum pwm_polarity polarity);
 int pwmchip_add(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
 struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
-- 
2.29.2

Re: [PATCH v2 3/3] pwm: Drop function pwmchip_add_with_polarity()

[Thierry Reding]

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

On Mon, Dec 07, 2020 at 02:45:56PM +0100, Uwe Kleine-König wrote:
> pwmchip_add() only calls pwmchip_add_with_polarity() and nothing else. All
> other users of pwmchip_add_with_polarity() are gone. So drop
> pwmchip_add_with_polarity() and move the code instead to pwmchip_add().
> 
> The initial assignment to pwm->state.polarity is dropped. In every correct
> usage of the PWM API this value is overwritten later anyhow.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/pwm/core.c  | 25 +++----------------------
>  include/linux/pwm.h |  2 --
>  2 files changed, 3 insertions(+), 24 deletions(-)

There was a conflict between this and patch "pwm: Always allocate PWM
chip base ID dynamically", but it was fairly trivial to resolve. Let me
know if you think I didn't resolve it correctly.

Thierry

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

Re: [PATCH v2 3/3] pwm: Drop function pwmchip_add_with_polarity()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1075 bytes --]

Hi Thierry,

On 3/22/21 12:32 PM, Thierry Reding wrote:
> On Mon, Dec 07, 2020 at 02:45:56PM +0100, Uwe Kleine-König wrote:
>> pwmchip_add() only calls pwmchip_add_with_polarity() and nothing else. All
>> other users of pwmchip_add_with_polarity() are gone. So drop
>> pwmchip_add_with_polarity() and move the code instead to pwmchip_add().
>>
>> The initial assignment to pwm->state.polarity is dropped. In every correct
>> usage of the PWM API this value is overwritten later anyhow.
>>
>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>> ---
>>   drivers/pwm/core.c  | 25 +++----------------------
>>   include/linux/pwm.h |  2 --
>>   2 files changed, 3 insertions(+), 24 deletions(-)
> 
> There was a conflict between this and patch "pwm: Always allocate PWM
> chip base ID dynamically", but it was fairly trivial to resolve. Let me
> know if you think I didn't resolve it correctly.

You did it in the same way I did it on my next branch. (Actually I had 
the patch order reversed, but the result is the same.)

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]