[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

This helps to convert drivers from the legacy API to pwm_apply_state without
having to make the aware of the configured polarity (and in some cases even
period).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d5199b507d79..017d1e13e29d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -522,6 +522,15 @@ static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 }
 #endif
 
+static inline void pwm_get_state_default(const struct pwm_device *pwm,
+					 struct pwm_state *state)
+{
+	state->period = pwm->args.period;
+	state->duty_cycle = 0;
+	state->polarity = pwm->args.polarity;
+	state->enabled = false;
+}
+
 static inline void pwm_apply_args(struct pwm_device *pwm)
 {
 	struct pwm_state state = { };
@@ -547,9 +556,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
 	 * pwm_apply_args().
 	 */
 
-	state.enabled = false;
-	state.polarity = pwm->args.polarity;
-	state.period = pwm->args.period;
+	pwm_get_state_default(pwm, &state);
 
 	pwm_apply_state(pwm, &state);
 }
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

This helps to convert drivers from the legacy API to pwm_apply_state without
having to make the aware of the configured polarity (and in some cases even
period).

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d5199b507d79..017d1e13e29d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -522,6 +522,15 @@ static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 }
 #endif
 
+static inline void pwm_get_state_default(const struct pwm_device *pwm,
+					 struct pwm_state *state)
+{
+	state->period = pwm->args.period;
+	state->duty_cycle = 0;
+	state->polarity = pwm->args.polarity;
+	state->enabled = false;
+}
+
 static inline void pwm_apply_args(struct pwm_device *pwm)
 {
 	struct pwm_state state = { };
@@ -547,9 +556,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
 	 * pwm_apply_args().
 	 */
 
-	state.enabled = false;
-	state.polarity = pwm->args.polarity;
-	state.period = pwm->args.period;
+	pwm_get_state_default(pwm, &state);
 
 	pwm_apply_state(pwm, &state);
 }
-- 
2.19.1

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

The legacy PWM API should be removed in the long run, so convert a user
to the atomic PWM API.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 7f5a18fa305b..5c4459f9a5f7 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = {
 		   PWM_POLARITY_NORMAL),
 };
 
+static struct pwm_state lcd_pwm_state;
 static struct pwm_device *lcd_pwm;
 
 static void rx1950_lcd_power(int enable)
@@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
 
 		/* GPB1->OUTPUT, GPB1->0 */
 		gpio_direction_output(S3C2410_GPB(1), 0);
-		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
-		pwm_disable(lcd_pwm);
+		lcd_pwm_state.duty_cycle = 0;
+		lcd_pwm_state.enabled = false;
+		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
 
 		/* GPC0->0, GPC10->0 */
 		gpio_direction_output(S3C2410_GPC(0), 0);
 		gpio_direction_output(S3C2410_GPC(10), 0);
 	} else {
-		pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
-		pwm_enable(lcd_pwm);
+		lcd_pwm_state.duty_cycle = LCD_PWM_DUTY;
+		lcd_pwm_state.enabled = true;
+		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
 
 		gpio_direction_output(S3C2410_GPC(0), 1);
 		gpio_direction_output(S3C2410_GPC(5), 1);
@@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
 		return PTR_ERR(lcd_pwm);
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(lcd_pwm);
+	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
+	lcd_pwm_state.period = LCD_PWM_PERIOD;
 
 	rx1950_lcd_power(1);
 	rx1950_bl_power(1);
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

The legacy PWM API should be removed in the long run, so convert a user
to the atomic PWM API.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 7f5a18fa305b..5c4459f9a5f7 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = {
 		   PWM_POLARITY_NORMAL),
 };
 
+static struct pwm_state lcd_pwm_state;
 static struct pwm_device *lcd_pwm;
 
 static void rx1950_lcd_power(int enable)
@@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
 
 		/* GPB1->OUTPUT, GPB1->0 */
 		gpio_direction_output(S3C2410_GPB(1), 0);
-		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
-		pwm_disable(lcd_pwm);
+		lcd_pwm_state.duty_cycle = 0;
+		lcd_pwm_state.enabled = false;
+		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
 
 		/* GPC0->0, GPC10->0 */
 		gpio_direction_output(S3C2410_GPC(0), 0);
 		gpio_direction_output(S3C2410_GPC(10), 0);
 	} else {
-		pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
-		pwm_enable(lcd_pwm);
+		lcd_pwm_state.duty_cycle = LCD_PWM_DUTY;
+		lcd_pwm_state.enabled = true;
+		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
 
 		gpio_direction_output(S3C2410_GPC(0), 1);
 		gpio_direction_output(S3C2410_GPC(5), 1);
@@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
 		return PTR_ERR(lcd_pwm);
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(lcd_pwm);
+	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
+	lcd_pwm_state.period = LCD_PWM_PERIOD;
 
 	rx1950_lcd_power(1);
 	rx1950_bl_power(1);
-- 
2.19.1

[PATCH 3/4] bus: ts-nbus: convert to atomic PWM API

[Uwe Kleine-König]

The legacy PWM API should be removed in the long run, so convert a user
to the atomic PWM API.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/bus/ts-nbus.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
index 073fd9011154..51f5ed870ad1 100644
--- a/drivers/bus/ts-nbus.c
+++ b/drivers/bus/ts-nbus.c
@@ -282,7 +282,7 @@ EXPORT_SYMBOL_GPL(ts_nbus_write);
 static int ts_nbus_probe(struct platform_device *pdev)
 {
 	struct pwm_device *pwm;
-	struct pwm_args pargs;
+	struct pwm_state pstate;
 	struct device *dev = &pdev->dev;
 	struct ts_nbus *ts_nbus;
 	int ret;
@@ -305,25 +305,19 @@ static int ts_nbus_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pwm_get_args(pwm, &pargs);
-	if (!pargs.period) {
+	pwm_get_state_default(pwm, &pstate);
+	if (!pstate.period) {
 		dev_err(&pdev->dev, "invalid PWM period\n");
 		return -EINVAL;
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(pwm);
-	ret = pwm_config(pwm, pargs.period, pargs.period);
+	/* We can start the FPGA and populate the peripherals now. */
+	pstate.enabled = true;
+	pstate.duty_cycle = pstate.period;
+	ret = pwm_apply_state(pwm, &pstate);
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * we can now start the FPGA and populate the peripherals.
-	 */
-	pwm_enable(pwm);
 	ts_nbus->pwm = pwm;
 
 	/*
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] bus: ts-nbus: convert to atomic PWM API

[Uwe Kleine-König]

The legacy PWM API should be removed in the long run, so convert a user
to the atomic PWM API.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/bus/ts-nbus.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
index 073fd9011154..51f5ed870ad1 100644
--- a/drivers/bus/ts-nbus.c
+++ b/drivers/bus/ts-nbus.c
@@ -282,7 +282,7 @@ EXPORT_SYMBOL_GPL(ts_nbus_write);
 static int ts_nbus_probe(struct platform_device *pdev)
 {
 	struct pwm_device *pwm;
-	struct pwm_args pargs;
+	struct pwm_state pstate;
 	struct device *dev = &pdev->dev;
 	struct ts_nbus *ts_nbus;
 	int ret;
@@ -305,25 +305,19 @@ static int ts_nbus_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pwm_get_args(pwm, &pargs);
-	if (!pargs.period) {
+	pwm_get_state_default(pwm, &pstate);
+	if (!pstate.period) {
 		dev_err(&pdev->dev, "invalid PWM period\n");
 		return -EINVAL;
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(pwm);
-	ret = pwm_config(pwm, pargs.period, pargs.period);
+	/* We can start the FPGA and populate the peripherals now. */
+	pstate.enabled = true;
+	pstate.duty_cycle = pstate.period;
+	ret = pwm_apply_state(pwm, &pstate);
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * we can now start the FPGA and populate the peripherals.
-	 */
-	pwm_enable(pwm);
 	ts_nbus->pwm = pwm;
 
 	/*
-- 
2.19.1

[PATCH 4/4] bus: ts-nbus: weaken driver dependency to allow broader compile testing

[Uwe Kleine-König]

There is nothing specific for the i.MX28 SoC in the ts-nbus driver and
it compiles just fine with an ARM/s3c24xx configuration.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/bus/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 1851112ccc29..bf1f43b847e9 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -149,7 +149,7 @@ config TI_SYSC
 
 config TS_NBUS
 	tristate "Technologic Systems NBUS Driver"
-	depends on SOC_IMX28
+	depends on SOC_IMX28 || COMPILE_TEST
 	depends on OF_GPIO && PWM
 	help
 	  Driver for the Technologic Systems NBUS which is used to interface
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] bus: ts-nbus: weaken driver dependency to allow broader compile testing

[Uwe Kleine-König]

There is nothing specific for the i.MX28 SoC in the ts-nbus driver and
it compiles just fine with an ARM/s3c24xx configuration.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/bus/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 1851112ccc29..bf1f43b847e9 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -149,7 +149,7 @@ config TI_SYSC
 
 config TS_NBUS
 	tristate "Technologic Systems NBUS Driver"
-	depends on SOC_IMX28
+	depends on SOC_IMX28 || COMPILE_TEST
 	depends on OF_GPIO && PWM
 	help
 	  Driver for the Technologic Systems NBUS which is used to interface
-- 
2.19.1

Re: [PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Krzysztof Kozlowski]

On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
I see this depends on first patch so I assume this will go through pwm tree.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Krzysztof Kozlowski]

On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
>
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
>
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
I see this depends on first patch so I assume this will go through pwm tree.

Best regards,
Krzysztof

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

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

On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> This helps to convert drivers from the legacy API to pwm_apply_state without
> having to make the aware of the configured polarity (and in some cases even
> period).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/pwm.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

You're going to have to at least Cc me on patches 2 & 3 so that I can
see how you intend this to be used.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> This helps to convert drivers from the legacy API to pwm_apply_state without
> having to make the aware of the configured polarity (and in some cases even
> period).
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/pwm.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

You're going to have to at least Cc me on patches 2 & 3 so that I can
see how you intend this to be used.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181029/66f34692/attachment.sig>

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello Thierry,

On Mon, Oct 29, 2018 at 12:33:37PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > This helps to convert drivers from the legacy API to pwm_apply_state without
> > having to make the aware of the configured polarity (and in some cases even
> > period).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  include/linux/pwm.h | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> You're going to have to at least Cc me on patches 2 & 3 so that I can
> see how you intend this to be used.

you were in To: for all four patches:

  patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
  patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
  patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
  patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello Thierry,

On Mon, Oct 29, 2018 at 12:33:37PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> > This helps to convert drivers from the legacy API to pwm_apply_state without
> > having to make the aware of the configured polarity (and in some cases even
> > period).
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> >  include/linux/pwm.h | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> You're going to have to at least Cc me on patches 2 & 3 so that I can
> see how you intend this to be used.

you were in To: for all four patches:

  patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
  patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
  patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
  patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello,

On Sat, Nov 03, 2018 at 03:25:45PM +0100, Uwe Kleine-König wrote:
> On Mon, Oct 29, 2018 at 12:33:37PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > having to make the aware of the configured polarity (and in some cases even
> > > period).
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  include/linux/pwm.h | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > You're going to have to at least Cc me on patches 2 & 3 so that I can
> > see how you intend this to be used.
> 
> you were in To: for all four patches:
> 
>   patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
>   patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
>   patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
>   patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

Did you find those in your mailbox in the meantime? If google just
failed to put them in your inbox I can send you a bounce of 2 and 3 if
you want to. Just tell me (in irc if you want to).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello,

On Sat, Nov 03, 2018 at 03:25:45PM +0100, Uwe Kleine-K?nig wrote:
> On Mon, Oct 29, 2018 at 12:33:37PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > having to make the aware of the configured polarity (and in some cases even
> > > period).
> > > 
> > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  include/linux/pwm.h | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > You're going to have to at least Cc me on patches 2 & 3 so that I can
> > see how you intend this to be used.
> 
> you were in To: for all four patches:
> 
>   patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
>   patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
>   patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
>   patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

Did you find those in your mailbox in the meantime? If google just
failed to put them in your inbox I can send you a bounce of 2 and 3 if
you want to. Just tell me (in irc if you want to).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

On Mon, Oct 29, 2018 at 11:46:15AM +0100, Krzysztof Kozlowski wrote:
> On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > The legacy PWM API should be removed in the long run, so convert a user
> > to the atomic PWM API.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> I see this depends on first patch so I assume this will go through pwm tree.

Right, I assume so, too. Unfortunately I didn't hear anything back from
Thierry about this because there seems to be a mail reception problem on
his end.

@Thierry: Could you find the problem? It would be great to get some
feedback from you regarding these patches. There are more pwm users that
could benefit from this change, but I first want some feedback about
this before investing more time here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

On Mon, Oct 29, 2018 at 11:46:15AM +0100, Krzysztof Kozlowski wrote:
> On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > The legacy PWM API should be removed in the long run, so convert a user
> > to the atomic PWM API.
> >
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> I see this depends on first patch so I assume this will go through pwm tree.

Right, I assume so, too. Unfortunately I didn't hear anything back from
Thierry about this because there seems to be a mail reception problem on
his end.

@Thierry: Could you find the problem? It would be great to get some
feedback from you regarding these patches. There are more pwm users that
could benefit from this change, but I first want some feedback about
this before investing more time here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Thierry Reding]

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

On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
> index 7f5a18fa305b..5c4459f9a5f7 100644
> --- a/arch/arm/mach-s3c24xx/mach-rx1950.c
> +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
> @@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = {
>  		   PWM_POLARITY_NORMAL),
>  };
>  
> +static struct pwm_state lcd_pwm_state;

You shouldn't need this. The whole point of the atomic API is that the
PWM carries its state, so the proper way to make a change is to query
the current state, make any required modifications and then apply the
new state.

>  static struct pwm_device *lcd_pwm;
>  
>  static void rx1950_lcd_power(int enable)
> @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
>  
>  		/* GPB1->OUTPUT, GPB1->0 */
>  		gpio_direction_output(S3C2410_GPB(1), 0);
> -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> -		pwm_disable(lcd_pwm);
> +		lcd_pwm_state.duty_cycle = 0;
> +		lcd_pwm_state.enabled = false;
> +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);

The correct way to do this would be:

	struct pwm_state state;

	pwm_get_state(lcd_pwm, &state);
	state.enabled = false;
	state.duty_cycle = 0;
	pwm_apply_state(lcd_pwm, &state);

>  
>  		/* GPC0->0, GPC10->0 */
>  		gpio_direction_output(S3C2410_GPC(0), 0);
>  		gpio_direction_output(S3C2410_GPC(10), 0);
>  	} else {
> -		pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
> -		pwm_enable(lcd_pwm);
> +		lcd_pwm_state.duty_cycle = LCD_PWM_DUTY;
> +		lcd_pwm_state.enabled = true;
> +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);

Similarily here.

>  
>  		gpio_direction_output(S3C2410_GPC(0), 1);
>  		gpio_direction_output(S3C2410_GPC(5), 1);
> @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
>  		return PTR_ERR(lcd_pwm);
>  	}
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to
> -	 * the atomic PWM API.
> -	 */
> -	pwm_apply_args(lcd_pwm);
> +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> +	lcd_pwm_state.period = LCD_PWM_PERIOD;

This is wrong, though it's probably because the comment is also
confusing. There should be nothing wrong with using pwm_apply_args() in
this case.

While at it, I think the conversion should also be include replacing the
call to pwm_request() by pwm_get(). There's already a PWM lookup table
in the RX1950 board code, so pwm_get() would be able to use that. Note
that for some reason that table contains a period that is different from
LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
the information other than duty cycle should be coming from either DT or
a lookup table.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Thierry Reding]

On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-K?nig wrote:
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
> index 7f5a18fa305b..5c4459f9a5f7 100644
> --- a/arch/arm/mach-s3c24xx/mach-rx1950.c
> +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
> @@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = {
>  		   PWM_POLARITY_NORMAL),
>  };
>  
> +static struct pwm_state lcd_pwm_state;

You shouldn't need this. The whole point of the atomic API is that the
PWM carries its state, so the proper way to make a change is to query
the current state, make any required modifications and then apply the
new state.

>  static struct pwm_device *lcd_pwm;
>  
>  static void rx1950_lcd_power(int enable)
> @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
>  
>  		/* GPB1->OUTPUT, GPB1->0 */
>  		gpio_direction_output(S3C2410_GPB(1), 0);
> -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> -		pwm_disable(lcd_pwm);
> +		lcd_pwm_state.duty_cycle = 0;
> +		lcd_pwm_state.enabled = false;
> +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);

The correct way to do this would be:

	struct pwm_state state;

	pwm_get_state(lcd_pwm, &state);
	state.enabled = false;
	state.duty_cycle = 0;
	pwm_apply_state(lcd_pwm, &state);

>  
>  		/* GPC0->0, GPC10->0 */
>  		gpio_direction_output(S3C2410_GPC(0), 0);
>  		gpio_direction_output(S3C2410_GPC(10), 0);
>  	} else {
> -		pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
> -		pwm_enable(lcd_pwm);
> +		lcd_pwm_state.duty_cycle = LCD_PWM_DUTY;
> +		lcd_pwm_state.enabled = true;
> +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);

Similarily here.

>  
>  		gpio_direction_output(S3C2410_GPC(0), 1);
>  		gpio_direction_output(S3C2410_GPC(5), 1);
> @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
>  		return PTR_ERR(lcd_pwm);
>  	}
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to
> -	 * the atomic PWM API.
> -	 */
> -	pwm_apply_args(lcd_pwm);
> +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> +	lcd_pwm_state.period = LCD_PWM_PERIOD;

This is wrong, though it's probably because the comment is also
confusing. There should be nothing wrong with using pwm_apply_args() in
this case.

While at it, I think the conversion should also be include replacing the
call to pwm_request() by pwm_get(). There's already a PWM lookup table
in the RX1950 board code, so pwm_get() would be able to use that. Note
that for some reason that table contains a period that is different from
LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
the information other than duty cycle should be coming from either DT or
a lookup table.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181114/016e5051/attachment.sig>

Re: [PATCH 3/4] bus: ts-nbus: convert to atomic PWM API

[Thierry Reding]

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

On Fri, Oct 26, 2018 at 08:41:57PM +0200, Uwe Kleine-König wrote:
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/bus/ts-nbus.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
> index 073fd9011154..51f5ed870ad1 100644
> --- a/drivers/bus/ts-nbus.c
> +++ b/drivers/bus/ts-nbus.c
> @@ -282,7 +282,7 @@ EXPORT_SYMBOL_GPL(ts_nbus_write);
>  static int ts_nbus_probe(struct platform_device *pdev)
>  {
>  	struct pwm_device *pwm;
> -	struct pwm_args pargs;
> +	struct pwm_state pstate;
>  	struct device *dev = &pdev->dev;
>  	struct ts_nbus *ts_nbus;
>  	int ret;
> @@ -305,25 +305,19 @@ static int ts_nbus_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	pwm_get_args(pwm, &pargs);
> -	if (!pargs.period) {
> +	pwm_get_state_default(pwm, &pstate);
> +	if (!pstate.period) {
>  		dev_err(&pdev->dev, "invalid PWM period\n");
>  		return -EINVAL;
>  	}
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to
> -	 * the atomic PWM API.
> -	 */
> -	pwm_apply_args(pwm);
> -	ret = pwm_config(pwm, pargs.period, pargs.period);
> +	/* We can start the FPGA and populate the peripherals now. */
> +	pstate.enabled = true;
> +	pstate.duty_cycle = pstate.period;
> +	ret = pwm_apply_state(pwm, &pstate);
>  	if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * we can now start the FPGA and populate the peripherals.
> -	 */
> -	pwm_enable(pwm);
>  	ts_nbus->pwm = pwm;
>  
>  	/*

This looks suspiciously like the PWM is actually powering this bus. It
would've been better to model that as a PWM driven regulator rather than
an PWM consumer.

Either way I don't see why pwm_get_state_default() would be useful here.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] bus: ts-nbus: convert to atomic PWM API

[Thierry Reding]

On Fri, Oct 26, 2018 at 08:41:57PM +0200, Uwe Kleine-K?nig wrote:
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/bus/ts-nbus.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
> index 073fd9011154..51f5ed870ad1 100644
> --- a/drivers/bus/ts-nbus.c
> +++ b/drivers/bus/ts-nbus.c
> @@ -282,7 +282,7 @@ EXPORT_SYMBOL_GPL(ts_nbus_write);
>  static int ts_nbus_probe(struct platform_device *pdev)
>  {
>  	struct pwm_device *pwm;
> -	struct pwm_args pargs;
> +	struct pwm_state pstate;
>  	struct device *dev = &pdev->dev;
>  	struct ts_nbus *ts_nbus;
>  	int ret;
> @@ -305,25 +305,19 @@ static int ts_nbus_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	pwm_get_args(pwm, &pargs);
> -	if (!pargs.period) {
> +	pwm_get_state_default(pwm, &pstate);
> +	if (!pstate.period) {
>  		dev_err(&pdev->dev, "invalid PWM period\n");
>  		return -EINVAL;
>  	}
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to
> -	 * the atomic PWM API.
> -	 */
> -	pwm_apply_args(pwm);
> -	ret = pwm_config(pwm, pargs.period, pargs.period);
> +	/* We can start the FPGA and populate the peripherals now. */
> +	pstate.enabled = true;
> +	pstate.duty_cycle = pstate.period;
> +	ret = pwm_apply_state(pwm, &pstate);
>  	if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * we can now start the FPGA and populate the peripherals.
> -	 */
> -	pwm_enable(pwm);
>  	ts_nbus->pwm = pwm;
>  
>  	/*

This looks suspiciously like the PWM is actually powering this bus. It
would've been better to model that as a PWM driven regulator rather than
an PWM consumer.

Either way I don't see why pwm_get_state_default() would be useful here.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181114/6e41b905/attachment.sig>

Re: [PATCH 4/4] bus: ts-nbus: weaken driver dependency to allow broader compile testing

[Thierry Reding]

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

On Fri, Oct 26, 2018 at 08:41:58PM +0200, Uwe Kleine-König wrote:
> There is nothing specific for the i.MX28 SoC in the ts-nbus driver and
> it compiles just fine with an ARM/s3c24xx configuration.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/bus/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks like Sebastien's email no longer exists. Seeing that this driver
was merged a year ago and doesn't have any in-kernel users I wonder if
there's any use in keeping it.

Arnd, this looks completely dead at this point and it doesn't seem like
this ever had any users, should we remove it?

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] bus: ts-nbus: weaken driver dependency to allow broader compile testing

[Thierry Reding]

On Fri, Oct 26, 2018 at 08:41:58PM +0200, Uwe Kleine-K?nig wrote:
> There is nothing specific for the i.MX28 SoC in the ts-nbus driver and
> it compiles just fine with an ARM/s3c24xx configuration.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/bus/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks like Sebastien's email no longer exists. Seeing that this driver
was merged a year ago and doesn't have any in-kernel users I wonder if
there's any use in keeping it.

Arnd, this looks completely dead at this point and it doesn't seem like
this ever had any users, should we remove it?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181114/ca9804ba/attachment.sig>

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

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

On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> This helps to convert drivers from the legacy API to pwm_apply_state without
> having to make the aware of the configured polarity (and in some cases even
> period).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/pwm.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to
be able to get at the default state, which would be the state as it was
"configured" in DT or the PWM lookup table so that you can modify that
state (well, really just setting the duty-cycle) before you actually
send the state to the hardware.

So the difference to pwm_apply_args() is that you don't want the driver
to specifically program a duty cycle of 0 before setting the actual duty
cycle that you want to set.

Is that about right?

Isn't that exactly what pwm_init_state() already does? pwm-backlight
uses it for exactly the same purpose that you seem to be using it in
RX1950 backlight control.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> This helps to convert drivers from the legacy API to pwm_apply_state without
> having to make the aware of the configured polarity (and in some cases even
> period).
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/pwm.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to
be able to get at the default state, which would be the state as it was
"configured" in DT or the PWM lookup table so that you can modify that
state (well, really just setting the duty-cycle) before you actually
send the state to the hardware.

So the difference to pwm_apply_args() is that you don't want the driver
to specifically program a duty cycle of 0 before setting the actual duty
cycle that you want to set.

Is that about right?

Isn't that exactly what pwm_init_state() already does? pwm-backlight
uses it for exactly the same purpose that you seem to be using it in
RX1950 backlight control.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181114/493f7c6e/attachment-0001.sig>

Re: [PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

Hello Thierry,

On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> > +static struct pwm_state lcd_pwm_state;
> 
> You shouldn't need this. The whole point of the atomic API is that the
> PWM carries its state, so the proper way to make a change is to query
> the current state, make any required modifications and then apply the
> new state.

I thought the whole point of the atomic API is to be atomic :-)

> >  static struct pwm_device *lcd_pwm;
> >  
> >  static void rx1950_lcd_power(int enable)
> > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> >  
> >  		/* GPB1->OUTPUT, GPB1->0 */
> >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > -		pwm_disable(lcd_pwm);
> > +		lcd_pwm_state.duty_cycle = 0;
> > +		lcd_pwm_state.enabled = false;
> > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> 
> The correct way to do this would be:
> 
> 	struct pwm_state state;
> 
> 	pwm_get_state(lcd_pwm, &state);
> 	state.enabled = false;
> 	state.duty_cycle = 0;
> 	pwm_apply_state(lcd_pwm, &state);

The difference here is that with my approach the rx1950 driver caches
the intended pwm_state saving a few cycles for repeatedly copying the
pwm cache to a stack variable at the cost of some memory. I like my
approach a little better but I'm not willing to argue about that one and
can give in.

> >  		gpio_direction_output(S3C2410_GPC(0), 1);
> >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> >  		return PTR_ERR(lcd_pwm);
> >  	}
> >  
> > -	/*
> > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > -	 * the atomic PWM API.
> > -	 */
> > -	pwm_apply_args(lcd_pwm);
> > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> 
> This is wrong, though it's probably because the comment is also
> confusing. There should be nothing wrong with using pwm_apply_args() in
> this case.

In my eyes it is wrong because it results in a call to the backend
driver's apply callback to get the default setting just to fix the
configuration and apply that in the code to follow.

> While at it, I think the conversion should also be include replacing the
> call to pwm_request() by pwm_get(). There's already a PWM lookup table
> in the RX1950 board code, so pwm_get() would be able to use that. Note
> that for some reason that table contains a period that is different from
> LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> the information other than duty cycle should be coming from either DT or
> a lookup table.

Yeah, I noticed that, too. It's not entirely clear to me how to do that
yet. So I thought to care about getting rid of the legacy usage of
pwm_config and pwm_apply_args first.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

Hello Thierry,

On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-K?nig wrote:
> > +static struct pwm_state lcd_pwm_state;
> 
> You shouldn't need this. The whole point of the atomic API is that the
> PWM carries its state, so the proper way to make a change is to query
> the current state, make any required modifications and then apply the
> new state.

I thought the whole point of the atomic API is to be atomic :-)

> >  static struct pwm_device *lcd_pwm;
> >  
> >  static void rx1950_lcd_power(int enable)
> > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> >  
> >  		/* GPB1->OUTPUT, GPB1->0 */
> >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > -		pwm_disable(lcd_pwm);
> > +		lcd_pwm_state.duty_cycle = 0;
> > +		lcd_pwm_state.enabled = false;
> > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> 
> The correct way to do this would be:
> 
> 	struct pwm_state state;
> 
> 	pwm_get_state(lcd_pwm, &state);
> 	state.enabled = false;
> 	state.duty_cycle = 0;
> 	pwm_apply_state(lcd_pwm, &state);

The difference here is that with my approach the rx1950 driver caches
the intended pwm_state saving a few cycles for repeatedly copying the
pwm cache to a stack variable at the cost of some memory. I like my
approach a little better but I'm not willing to argue about that one and
can give in.

> >  		gpio_direction_output(S3C2410_GPC(0), 1);
> >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> >  		return PTR_ERR(lcd_pwm);
> >  	}
> >  
> > -	/*
> > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > -	 * the atomic PWM API.
> > -	 */
> > -	pwm_apply_args(lcd_pwm);
> > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> 
> This is wrong, though it's probably because the comment is also
> confusing. There should be nothing wrong with using pwm_apply_args() in
> this case.

In my eyes it is wrong because it results in a call to the backend
driver's apply callback to get the default setting just to fix the
configuration and apply that in the code to follow.

> While at it, I think the conversion should also be include replacing the
> call to pwm_request() by pwm_get(). There's already a PWM lookup table
> in the RX1950 board code, so pwm_get() would be able to use that. Note
> that for some reason that table contains a period that is different from
> LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> the information other than duty cycle should be coming from either DT or
> a lookup table.

Yeah, I noticed that, too. It's not entirely clear to me how to do that
yet. So I thought to care about getting rid of the legacy usage of
pwm_config and pwm_apply_args first.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello Thierry,

On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > This helps to convert drivers from the legacy API to pwm_apply_state without
> > having to make the aware of the configured polarity (and in some cases even
> > period).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  include/linux/pwm.h | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> So if I understand your intention correctly here you want consumers to
> be able to get at the default state, which would be the state as it was
> "configured" in DT or the PWM lookup table so that you can modify that
> state (well, really just setting the duty-cycle) before you actually
> send the state to the hardware.
> 
> So the difference to pwm_apply_args() is that you don't want the driver
> to specifically program a duty cycle of 0 before setting the actual duty
> cycle that you want to set.
> 
> Is that about right?

Yes, and a resulting advantage is that the apply callback is only called
once.
 
> Isn't that exactly what pwm_init_state() already does? pwm-backlight
> uses it for exactly the same purpose that you seem to be using it in
> RX1950 backlight control.

I didn't use pwm_init_state because that doesn't give a helpful value
for state->enabled. (I think it is: if the driver provides a get_state
callback it is whatever that one set, otherwise it's off.)
Also the name pwm_init_state isn't that good IMHO. Just from the name
I'd expect it to be something like

	memset(state, 0, sizeof(&state));

instead of something more informed that depends on hardware state and/or
data provided by a device tree.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello Thierry,

On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> > This helps to convert drivers from the legacy API to pwm_apply_state without
> > having to make the aware of the configured polarity (and in some cases even
> > period).
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> >  include/linux/pwm.h | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> So if I understand your intention correctly here you want consumers to
> be able to get at the default state, which would be the state as it was
> "configured" in DT or the PWM lookup table so that you can modify that
> state (well, really just setting the duty-cycle) before you actually
> send the state to the hardware.
> 
> So the difference to pwm_apply_args() is that you don't want the driver
> to specifically program a duty cycle of 0 before setting the actual duty
> cycle that you want to set.
> 
> Is that about right?

Yes, and a resulting advantage is that the apply callback is only called
once.
 
> Isn't that exactly what pwm_init_state() already does? pwm-backlight
> uses it for exactly the same purpose that you seem to be using it in
> RX1950 backlight control.

I didn't use pwm_init_state because that doesn't give a helpful value
for state->enabled. (I think it is: if the driver provides a get_state
callback it is whatever that one set, otherwise it's off.)
Also the name pwm_init_state isn't that good IMHO. Just from the name
I'd expect it to be something like

	memset(state, 0, sizeof(&state));

instead of something more informed that depends on hardware state and/or
data provided by a device tree.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Thierry Reding]

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

On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> > > +static struct pwm_state lcd_pwm_state;
> > 
> > You shouldn't need this. The whole point of the atomic API is that the
> > PWM carries its state, so the proper way to make a change is to query
> > the current state, make any required modifications and then apply the
> > new state.
> 
> I thought the whole point of the atomic API is to be atomic :-)
> 
> > >  static struct pwm_device *lcd_pwm;
> > >  
> > >  static void rx1950_lcd_power(int enable)
> > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> > >  
> > >  		/* GPB1->OUTPUT, GPB1->0 */
> > >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > > -		pwm_disable(lcd_pwm);
> > > +		lcd_pwm_state.duty_cycle = 0;
> > > +		lcd_pwm_state.enabled = false;
> > > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> > 
> > The correct way to do this would be:
> > 
> > 	struct pwm_state state;
> > 
> > 	pwm_get_state(lcd_pwm, &state);
> > 	state.enabled = false;
> > 	state.duty_cycle = 0;
> > 	pwm_apply_state(lcd_pwm, &state);
> 
> The difference here is that with my approach the rx1950 driver caches
> the intended pwm_state saving a few cycles for repeatedly copying the
> pwm cache to a stack variable at the cost of some memory. I like my
> approach a little better but I'm not willing to argue about that one and
> can give in.

It's a redundant copy of the PWM's internal state and not guaranteed to
remain in sync with the PWM hardware state. You also require a global
variable, which is usually a bad idea. And we're not repeatedly copying
data. We occasionally do. These operations are typically only executed
once or twice per boot, so hardly anything that needs to be optimized
for speed.

> > >  		gpio_direction_output(S3C2410_GPC(0), 1);
> > >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> > >  		return PTR_ERR(lcd_pwm);
> > >  	}
> > >  
> > > -	/*
> > > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > > -	 * the atomic PWM API.
> > > -	 */
> > > -	pwm_apply_args(lcd_pwm);
> > > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> > 
> > This is wrong, though it's probably because the comment is also
> > confusing. There should be nothing wrong with using pwm_apply_args() in
> > this case.
> 
> In my eyes it is wrong because it results in a call to the backend
> driver's apply callback to get the default setting just to fix the
> configuration and apply that in the code to follow.

Okay, I see. But there's already pwm_init_state() which pretty much does
what you do here, right?

Thinking about it some more, I wonder if we shouldn't be more consistent
about state handling here. So the reason why pwm_apply_args() exists is
to program the PWM with a known state if the PWM doesn't support
hardware readout. Since most drivers don't support hardware readout, we
need this to at some specific point synchronize the hardware and
internal states.

In retrospect I'm not sure that's necessary, because there's more and
more evidence that we don't want to touch a PWM configuration until a
consumer explicitly says so. There's also the slight problem that the
pwm_apply_args() is really only necessary if the PWM driver doesn't
support hardware readout, but that's not something that consumers are
aware of or should worry about.

In light of that I think perhaps a better solution would be to basically
apply the PWM arguments to the internal state at request time. We can't
do it earlier because we don't know the arguments before the PWM is
requested. So I think at request time we could do something like this:

	if (chip->ops->get_state)
		chip->ops->get_state(chip, pwm, &pwm->state);
	else
		pwm_init_state(pwm, &pwm->state);

That way drivers always get either the current state or the "default"
state that was configured via platform-specific means (DT or lookup).

> > While at it, I think the conversion should also be include replacing the
> > call to pwm_request() by pwm_get(). There's already a PWM lookup table
> > in the RX1950 board code, so pwm_get() would be able to use that. Note
> > that for some reason that table contains a period that is different from
> > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> > the information other than duty cycle should be coming from either DT or
> > a lookup table.
> 
> Yeah, I noticed that, too. It's not entirely clear to me how to do that
> yet. So I thought to care about getting rid of the legacy usage of
> pwm_config and pwm_apply_args first.

So I think I got the backlight PWM confused with the LCD PWM. The former
is what we have in the PWM lookup table and which controls the backlight
brightness. The latter is what we don't have in the table and which is
requested using the legacy pwm_request() function. That also explains
why there's a difference between the period in the PWM lookup table for
the backlight PWM and the LCD_PWM_PERIOD macro.

Adding support for pwm_get() should be as simple ad adding an entry to
rx1950_pwm_lookup[], such as this:

	PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960,
		   PWM_POLARITY_NORMAL),

Then in rx1950_backlight_init(), which is passed the device structure
that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can
be called like this:

	lcd_pwm = pwm_get(dev, "lcd");

The matching code should then be able to find the second entry as the
best match and return the correct one.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Thierry Reding]

On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-K?nig wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-K?nig wrote:
> > > +static struct pwm_state lcd_pwm_state;
> > 
> > You shouldn't need this. The whole point of the atomic API is that the
> > PWM carries its state, so the proper way to make a change is to query
> > the current state, make any required modifications and then apply the
> > new state.
> 
> I thought the whole point of the atomic API is to be atomic :-)
> 
> > >  static struct pwm_device *lcd_pwm;
> > >  
> > >  static void rx1950_lcd_power(int enable)
> > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> > >  
> > >  		/* GPB1->OUTPUT, GPB1->0 */
> > >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > > -		pwm_disable(lcd_pwm);
> > > +		lcd_pwm_state.duty_cycle = 0;
> > > +		lcd_pwm_state.enabled = false;
> > > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> > 
> > The correct way to do this would be:
> > 
> > 	struct pwm_state state;
> > 
> > 	pwm_get_state(lcd_pwm, &state);
> > 	state.enabled = false;
> > 	state.duty_cycle = 0;
> > 	pwm_apply_state(lcd_pwm, &state);
> 
> The difference here is that with my approach the rx1950 driver caches
> the intended pwm_state saving a few cycles for repeatedly copying the
> pwm cache to a stack variable at the cost of some memory. I like my
> approach a little better but I'm not willing to argue about that one and
> can give in.

It's a redundant copy of the PWM's internal state and not guaranteed to
remain in sync with the PWM hardware state. You also require a global
variable, which is usually a bad idea. And we're not repeatedly copying
data. We occasionally do. These operations are typically only executed
once or twice per boot, so hardly anything that needs to be optimized
for speed.

> > >  		gpio_direction_output(S3C2410_GPC(0), 1);
> > >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> > >  		return PTR_ERR(lcd_pwm);
> > >  	}
> > >  
> > > -	/*
> > > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > > -	 * the atomic PWM API.
> > > -	 */
> > > -	pwm_apply_args(lcd_pwm);
> > > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> > 
> > This is wrong, though it's probably because the comment is also
> > confusing. There should be nothing wrong with using pwm_apply_args() in
> > this case.
> 
> In my eyes it is wrong because it results in a call to the backend
> driver's apply callback to get the default setting just to fix the
> configuration and apply that in the code to follow.

Okay, I see. But there's already pwm_init_state() which pretty much does
what you do here, right?

Thinking about it some more, I wonder if we shouldn't be more consistent
about state handling here. So the reason why pwm_apply_args() exists is
to program the PWM with a known state if the PWM doesn't support
hardware readout. Since most drivers don't support hardware readout, we
need this to at some specific point synchronize the hardware and
internal states.

In retrospect I'm not sure that's necessary, because there's more and
more evidence that we don't want to touch a PWM configuration until a
consumer explicitly says so. There's also the slight problem that the
pwm_apply_args() is really only necessary if the PWM driver doesn't
support hardware readout, but that's not something that consumers are
aware of or should worry about.

In light of that I think perhaps a better solution would be to basically
apply the PWM arguments to the internal state at request time. We can't
do it earlier because we don't know the arguments before the PWM is
requested. So I think at request time we could do something like this:

	if (chip->ops->get_state)
		chip->ops->get_state(chip, pwm, &pwm->state);
	else
		pwm_init_state(pwm, &pwm->state);

That way drivers always get either the current state or the "default"
state that was configured via platform-specific means (DT or lookup).

> > While at it, I think the conversion should also be include replacing the
> > call to pwm_request() by pwm_get(). There's already a PWM lookup table
> > in the RX1950 board code, so pwm_get() would be able to use that. Note
> > that for some reason that table contains a period that is different from
> > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> > the information other than duty cycle should be coming from either DT or
> > a lookup table.
> 
> Yeah, I noticed that, too. It's not entirely clear to me how to do that
> yet. So I thought to care about getting rid of the legacy usage of
> pwm_config and pwm_apply_args first.

So I think I got the backlight PWM confused with the LCD PWM. The former
is what we have in the PWM lookup table and which controls the backlight
brightness. The latter is what we don't have in the table and which is
requested using the legacy pwm_request() function. That also explains
why there's a difference between the period in the PWM lookup table for
the backlight PWM and the LCD_PWM_PERIOD macro.

Adding support for pwm_get() should be as simple ad adding an entry to
rx1950_pwm_lookup[], such as this:

	PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960,
		   PWM_POLARITY_NORMAL),

Then in rx1950_backlight_init(), which is passed the device structure
that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can
be called like this:

	lcd_pwm = pwm_get(dev, "lcd");

The matching code should then be able to find the second entry as the
best match and return the correct one.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181115/e4101d15/attachment.sig>

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

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

On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > having to make the aware of the configured polarity (and in some cases even
> > > period).
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  include/linux/pwm.h | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > So if I understand your intention correctly here you want consumers to
> > be able to get at the default state, which would be the state as it was
> > "configured" in DT or the PWM lookup table so that you can modify that
> > state (well, really just setting the duty-cycle) before you actually
> > send the state to the hardware.
> > 
> > So the difference to pwm_apply_args() is that you don't want the driver
> > to specifically program a duty cycle of 0 before setting the actual duty
> > cycle that you want to set.
> > 
> > Is that about right?
> 
> Yes, and a resulting advantage is that the apply callback is only called
> once.
>  
> > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > uses it for exactly the same purpose that you seem to be using it in
> > RX1950 backlight control.
> 
> I didn't use pwm_init_state because that doesn't give a helpful value
> for state->enabled. (I think it is: if the driver provides a get_state
> callback it is whatever that one set, otherwise it's off.)

That's correct. It's pretty much the same thing that you have, except
you explicitly set enable = false. But that's not really important
because consumers are supposed to explicitly override that anyway.

> Also the name pwm_init_state isn't that good IMHO. Just from the name
> I'd expect it to be something like
> 
> 	memset(state, 0, sizeof(&state));
> 
> instead of something more informed that depends on hardware state and/or
> data provided by a device tree.

Be that as it may, the kerneldoc is pretty explicit about what it does
and so is the code. Not liking the name shouldn't be an excuse for
duplicating functionality.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-K?nig wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > having to make the aware of the configured polarity (and in some cases even
> > > period).
> > > 
> > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  include/linux/pwm.h | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > So if I understand your intention correctly here you want consumers to
> > be able to get at the default state, which would be the state as it was
> > "configured" in DT or the PWM lookup table so that you can modify that
> > state (well, really just setting the duty-cycle) before you actually
> > send the state to the hardware.
> > 
> > So the difference to pwm_apply_args() is that you don't want the driver
> > to specifically program a duty cycle of 0 before setting the actual duty
> > cycle that you want to set.
> > 
> > Is that about right?
> 
> Yes, and a resulting advantage is that the apply callback is only called
> once.
>  
> > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > uses it for exactly the same purpose that you seem to be using it in
> > RX1950 backlight control.
> 
> I didn't use pwm_init_state because that doesn't give a helpful value
> for state->enabled. (I think it is: if the driver provides a get_state
> callback it is whatever that one set, otherwise it's off.)

That's correct. It's pretty much the same thing that you have, except
you explicitly set enable = false. But that's not really important
because consumers are supposed to explicitly override that anyway.

> Also the name pwm_init_state isn't that good IMHO. Just from the name
> I'd expect it to be something like
> 
> 	memset(state, 0, sizeof(&state));
> 
> instead of something more informed that depends on hardware state and/or
> data provided by a device tree.

Be that as it may, the kerneldoc is pretty explicit about what it does
and so is the code. Not liking the name shouldn't be an excuse for
duplicating functionality.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181115/775d5c3a/attachment.sig>

Re: [PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

Hello Thierry,

On Thu, Nov 15, 2018 at 05:15:16PM +0100, Thierry Reding wrote:
> On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-König wrote:
> > On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> > > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> > > > +static struct pwm_state lcd_pwm_state;
> > > >  static struct pwm_device *lcd_pwm;
> > > >  
> > > >  static void rx1950_lcd_power(int enable)
> > > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> > > >  
> > > >  		/* GPB1->OUTPUT, GPB1->0 */
> > > >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > > > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > > > -		pwm_disable(lcd_pwm);
> > > > +		lcd_pwm_state.duty_cycle = 0;
> > > > +		lcd_pwm_state.enabled = false;
> > > > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> > > 
> > > The correct way to do this would be:
> > > 
> > > 	struct pwm_state state;
> > > 
> > > 	pwm_get_state(lcd_pwm, &state);
> > > 	state.enabled = false;
> > > 	state.duty_cycle = 0;
> > > 	pwm_apply_state(lcd_pwm, &state);
> > 
> > The difference here is that with my approach the rx1950 driver caches
> > the intended pwm_state saving a few cycles for repeatedly copying the
> > pwm cache to a stack variable at the cost of some memory. I like my
> > approach a little better but I'm not willing to argue about that one and
> > can give in.
> 
> It's a redundant copy of the PWM's internal state

That the pwm keeps a copy of this is an implementation detail of the pwm
framework that shouldn't matter to its consumers.

> and not guaranteed to remain in sync with the PWM hardware state.

What should happen that the driver's pwm_state and the hardware state
diverge?

> You also require a global variable, which is usually a bad idea.

Right, but that's attributable to the fact, that the struct pwm_device
is a global variable, too.

> And we're not repeatedly copying data. We occasionally do. These
> operations are typically only executed once or twice per boot, so
> hardly anything that needs to be optimized for speed.
> 
> > > >  		gpio_direction_output(S3C2410_GPC(0), 1);
> > > >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> > > >  		return PTR_ERR(lcd_pwm);
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > > > -	 * the atomic PWM API.
> > > > -	 */
> > > > -	pwm_apply_args(lcd_pwm);
> > > > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > > > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> > > 
> > > This is wrong, though it's probably because the comment is also
> > > confusing. There should be nothing wrong with using pwm_apply_args() in
> > > this case.
> > 
> > In my eyes it is wrong because it results in a call to the backend
> > driver's apply callback to get the default setting just to fix the
> > configuration and apply that in the code to follow.
> 
> Okay, I see. But there's already pwm_init_state() which pretty much does
> what you do here, right?
> 
> Thinking about it some more, I wonder if we shouldn't be more consistent
> about state handling here. So the reason why pwm_apply_args() exists is
> to program the PWM with a known state if the PWM doesn't support
> hardware readout. Since most drivers don't support hardware readout, we
> need this to at some specific point synchronize the hardware and
> internal states.
> 
> In retrospect I'm not sure that's necessary, because there's more and
> more evidence that we don't want to touch a PWM configuration until a
> consumer explicitly says so. There's also the slight problem that the
> pwm_apply_args() is really only necessary if the PWM driver doesn't
> support hardware readout, but that's not something that consumers are
> aware of or should worry about.

I consider your words as a light in the tunnel. Maybe we will reach a
conclusion eventually.

> In light of that I think perhaps a better solution would be to basically
> apply the PWM arguments to the internal state at request time. We can't
> do it earlier because we don't know the arguments before the PWM is
> requested. So I think at request time we could do something like this:
> 
> 	if (chip->ops->get_state)
> 		chip->ops->get_state(chip, pwm, &pwm->state);
> 	else
> 		pwm_init_state(pwm, &pwm->state);
> 
> That way drivers always get either the current state or the "default"
> state that was configured via platform-specific means (DT or lookup).

I think a gap in this reasoning is that if the DT dictates a period of
say 1 ms and the hardware is configured to something else, the consumer
should still work with the 1 ms value.

> > > While at it, I think the conversion should also be include replacing the
> > > call to pwm_request() by pwm_get(). There's already a PWM lookup table
> > > in the RX1950 board code, so pwm_get() would be able to use that. Note
> > > that for some reason that table contains a period that is different from
> > > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> > > the information other than duty cycle should be coming from either DT or
> > > a lookup table.
> > 
> > Yeah, I noticed that, too. It's not entirely clear to me how to do that
> > yet. So I thought to care about getting rid of the legacy usage of
> > pwm_config and pwm_apply_args first.
> 
> So I think I got the backlight PWM confused with the LCD PWM. The former
> is what we have in the PWM lookup table and which controls the backlight
> brightness. The latter is what we don't have in the table and which is
> requested using the legacy pwm_request() function. That also explains
> why there's a difference between the period in the PWM lookup table for
> the backlight PWM and the LCD_PWM_PERIOD macro.
> 
> Adding support for pwm_get() should be as simple ad adding an entry to
> rx1950_pwm_lookup[], such as this:
> 
> 	PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960,
> 		   PWM_POLARITY_NORMAL),
> 
> Then in rx1950_backlight_init(), which is passed the device structure
> that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can
> be called like this:
> 
> 	lcd_pwm = pwm_get(dev, "lcd");
> 
> The matching code should then be able to find the second entry as the
> best match and return the correct one.

Does this lookup belong to the board file or to a more general place
near the provider of the "samsung-pwm" devices?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

[Uwe Kleine-König]

Hello Thierry,

On Thu, Nov 15, 2018 at 05:15:16PM +0100, Thierry Reding wrote:
> On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> > > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-K?nig wrote:
> > > > +static struct pwm_state lcd_pwm_state;
> > > >  static struct pwm_device *lcd_pwm;
> > > >  
> > > >  static void rx1950_lcd_power(int enable)
> > > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> > > >  
> > > >  		/* GPB1->OUTPUT, GPB1->0 */
> > > >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > > > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > > > -		pwm_disable(lcd_pwm);
> > > > +		lcd_pwm_state.duty_cycle = 0;
> > > > +		lcd_pwm_state.enabled = false;
> > > > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> > > 
> > > The correct way to do this would be:
> > > 
> > > 	struct pwm_state state;
> > > 
> > > 	pwm_get_state(lcd_pwm, &state);
> > > 	state.enabled = false;
> > > 	state.duty_cycle = 0;
> > > 	pwm_apply_state(lcd_pwm, &state);
> > 
> > The difference here is that with my approach the rx1950 driver caches
> > the intended pwm_state saving a few cycles for repeatedly copying the
> > pwm cache to a stack variable at the cost of some memory. I like my
> > approach a little better but I'm not willing to argue about that one and
> > can give in.
> 
> It's a redundant copy of the PWM's internal state

That the pwm keeps a copy of this is an implementation detail of the pwm
framework that shouldn't matter to its consumers.

> and not guaranteed to remain in sync with the PWM hardware state.

What should happen that the driver's pwm_state and the hardware state
diverge?

> You also require a global variable, which is usually a bad idea.

Right, but that's attributable to the fact, that the struct pwm_device
is a global variable, too.

> And we're not repeatedly copying data. We occasionally do. These
> operations are typically only executed once or twice per boot, so
> hardly anything that needs to be optimized for speed.
> 
> > > >  		gpio_direction_output(S3C2410_GPC(0), 1);
> > > >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> > > >  		return PTR_ERR(lcd_pwm);
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > > > -	 * the atomic PWM API.
> > > > -	 */
> > > > -	pwm_apply_args(lcd_pwm);
> > > > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > > > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> > > 
> > > This is wrong, though it's probably because the comment is also
> > > confusing. There should be nothing wrong with using pwm_apply_args() in
> > > this case.
> > 
> > In my eyes it is wrong because it results in a call to the backend
> > driver's apply callback to get the default setting just to fix the
> > configuration and apply that in the code to follow.
> 
> Okay, I see. But there's already pwm_init_state() which pretty much does
> what you do here, right?
> 
> Thinking about it some more, I wonder if we shouldn't be more consistent
> about state handling here. So the reason why pwm_apply_args() exists is
> to program the PWM with a known state if the PWM doesn't support
> hardware readout. Since most drivers don't support hardware readout, we
> need this to at some specific point synchronize the hardware and
> internal states.
> 
> In retrospect I'm not sure that's necessary, because there's more and
> more evidence that we don't want to touch a PWM configuration until a
> consumer explicitly says so. There's also the slight problem that the
> pwm_apply_args() is really only necessary if the PWM driver doesn't
> support hardware readout, but that's not something that consumers are
> aware of or should worry about.

I consider your words as a light in the tunnel. Maybe we will reach a
conclusion eventually.

> In light of that I think perhaps a better solution would be to basically
> apply the PWM arguments to the internal state at request time. We can't
> do it earlier because we don't know the arguments before the PWM is
> requested. So I think at request time we could do something like this:
> 
> 	if (chip->ops->get_state)
> 		chip->ops->get_state(chip, pwm, &pwm->state);
> 	else
> 		pwm_init_state(pwm, &pwm->state);
> 
> That way drivers always get either the current state or the "default"
> state that was configured via platform-specific means (DT or lookup).

I think a gap in this reasoning is that if the DT dictates a period of
say 1 ms and the hardware is configured to something else, the consumer
should still work with the 1 ms value.

> > > While at it, I think the conversion should also be include replacing the
> > > call to pwm_request() by pwm_get(). There's already a PWM lookup table
> > > in the RX1950 board code, so pwm_get() would be able to use that. Note
> > > that for some reason that table contains a period that is different from
> > > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> > > the information other than duty cycle should be coming from either DT or
> > > a lookup table.
> > 
> > Yeah, I noticed that, too. It's not entirely clear to me how to do that
> > yet. So I thought to care about getting rid of the legacy usage of
> > pwm_config and pwm_apply_args first.
> 
> So I think I got the backlight PWM confused with the LCD PWM. The former
> is what we have in the PWM lookup table and which controls the backlight
> brightness. The latter is what we don't have in the table and which is
> requested using the legacy pwm_request() function. That also explains
> why there's a difference between the period in the PWM lookup table for
> the backlight PWM and the LCD_PWM_PERIOD macro.
> 
> Adding support for pwm_get() should be as simple ad adding an entry to
> rx1950_pwm_lookup[], such as this:
> 
> 	PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960,
> 		   PWM_POLARITY_NORMAL),
> 
> Then in rx1950_backlight_init(), which is passed the device structure
> that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can
> be called like this:
> 
> 	lcd_pwm = pwm_get(dev, "lcd");
> 
> The matching code should then be able to find the second entry as the
> best match and return the correct one.

Does this lookup belong to the board file or to a more general place
near the provider of the "samsung-pwm" devices?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello Thierry,

On Thu, Nov 15, 2018 at 05:21:59PM +0100, Thierry Reding wrote:
> On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-König wrote:
> > On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > > having to make the aware of the configured polarity (and in some cases even
> > > > period).
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  include/linux/pwm.h | 13 ++++++++++---
> > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > So if I understand your intention correctly here you want consumers to
> > > be able to get at the default state, which would be the state as it was
> > > "configured" in DT or the PWM lookup table so that you can modify that
> > > state (well, really just setting the duty-cycle) before you actually
> > > send the state to the hardware.
> > > 
> > > So the difference to pwm_apply_args() is that you don't want the driver
> > > to specifically program a duty cycle of 0 before setting the actual duty
> > > cycle that you want to set.
> > > 
> > > Is that about right?
> > 
> > Yes, and a resulting advantage is that the apply callback is only called
> > once.
> >  
> > > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > > uses it for exactly the same purpose that you seem to be using it in
> > > RX1950 backlight control.
> > 
> > I didn't use pwm_init_state because that doesn't give a helpful value
> > for state->enabled. (I think it is: if the driver provides a get_state
> > callback it is whatever that one set, otherwise it's off.)
> 
> That's correct. It's pretty much the same thing that you have, except
> you explicitly set enable = false. But that's not really important
> because consumers are supposed to explicitly override that anyway.

If we hardcode to enable=false the call to pwm_get_state could be
dropped, too. Then the two functions are really semantically identical.
> 
> > Also the name pwm_init_state isn't that good IMHO. Just from the name
> > I'd expect it to be something like
> > 
> > 	memset(state, 0, sizeof(&state));
> > 
> > instead of something more informed that depends on hardware state and/or
> > data provided by a device tree.
> 
> Be that as it may, the kerneldoc is pretty explicit about what it does
> and so is the code. Not liking the name shouldn't be an excuse for
> duplicating functionality.

With your suggestion to move the initialisation of ->state into the
core, maybe this function can go away from the public API such that the
naming doesn't really matter (and can be adapted easily).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Uwe Kleine-König]

Hello Thierry,

On Thu, Nov 15, 2018 at 05:21:59PM +0100, Thierry Reding wrote:
> On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> > > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > > having to make the aware of the configured polarity (and in some cases even
> > > > period).
> > > > 
> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  include/linux/pwm.h | 13 ++++++++++---
> > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > So if I understand your intention correctly here you want consumers to
> > > be able to get at the default state, which would be the state as it was
> > > "configured" in DT or the PWM lookup table so that you can modify that
> > > state (well, really just setting the duty-cycle) before you actually
> > > send the state to the hardware.
> > > 
> > > So the difference to pwm_apply_args() is that you don't want the driver
> > > to specifically program a duty cycle of 0 before setting the actual duty
> > > cycle that you want to set.
> > > 
> > > Is that about right?
> > 
> > Yes, and a resulting advantage is that the apply callback is only called
> > once.
> >  
> > > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > > uses it for exactly the same purpose that you seem to be using it in
> > > RX1950 backlight control.
> > 
> > I didn't use pwm_init_state because that doesn't give a helpful value
> > for state->enabled. (I think it is: if the driver provides a get_state
> > callback it is whatever that one set, otherwise it's off.)
> 
> That's correct. It's pretty much the same thing that you have, except
> you explicitly set enable = false. But that's not really important
> because consumers are supposed to explicitly override that anyway.

If we hardcode to enable=false the call to pwm_get_state could be
dropped, too. Then the two functions are really semantically identical.
> 
> > Also the name pwm_init_state isn't that good IMHO. Just from the name
> > I'd expect it to be something like
> > 
> > 	memset(state, 0, sizeof(&state));
> > 
> > instead of something more informed that depends on hardware state and/or
> > data provided by a device tree.
> 
> Be that as it may, the kerneldoc is pretty explicit about what it does
> and so is the code. Not liking the name shouldn't be an excuse for
> duplicating functionality.

With your suggestion to move the initialisation of ->state into the
core, maybe this function can go away from the public API such that the
naming doesn't really matter (and can be adapted easily).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

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

On Thu, Nov 15, 2018 at 10:05:35PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Nov 15, 2018 at 05:21:59PM +0100, Thierry Reding wrote:
> > On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-König wrote:
> > > On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > > > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > > > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > > > having to make the aware of the configured polarity (and in some cases even
> > > > > period).
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  include/linux/pwm.h | 13 ++++++++++---
> > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > So if I understand your intention correctly here you want consumers to
> > > > be able to get at the default state, which would be the state as it was
> > > > "configured" in DT or the PWM lookup table so that you can modify that
> > > > state (well, really just setting the duty-cycle) before you actually
> > > > send the state to the hardware.
> > > > 
> > > > So the difference to pwm_apply_args() is that you don't want the driver
> > > > to specifically program a duty cycle of 0 before setting the actual duty
> > > > cycle that you want to set.
> > > > 
> > > > Is that about right?
> > > 
> > > Yes, and a resulting advantage is that the apply callback is only called
> > > once.
> > >  
> > > > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > > > uses it for exactly the same purpose that you seem to be using it in
> > > > RX1950 backlight control.
> > > 
> > > I didn't use pwm_init_state because that doesn't give a helpful value
> > > for state->enabled. (I think it is: if the driver provides a get_state
> > > callback it is whatever that one set, otherwise it's off.)
> > 
> > That's correct. It's pretty much the same thing that you have, except
> > you explicitly set enable = false. But that's not really important
> > because consumers are supposed to explicitly override that anyway.
> 
> If we hardcode to enable=false the call to pwm_get_state could be
> dropped, too. Then the two functions are really semantically identical.
> > 
> > > Also the name pwm_init_state isn't that good IMHO. Just from the name
> > > I'd expect it to be something like
> > > 
> > > 	memset(state, 0, sizeof(&state));
> > > 
> > > instead of something more informed that depends on hardware state and/or
> > > data provided by a device tree.
> > 
> > Be that as it may, the kerneldoc is pretty explicit about what it does
> > and so is the code. Not liking the name shouldn't be an excuse for
> > duplicating functionality.
> 
> With your suggestion to move the initialisation of ->state into the
> core, maybe this function can go away from the public API such that the
> naming doesn't really matter (and can be adapted easily).

Yeah, I think it would be best to move it into core.c and make it static
in that case. There should be no need for anyone to create any special
state, they can just use pwm_get_state() instead.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

[Thierry Reding]

On Thu, Nov 15, 2018 at 10:05:35PM +0100, Uwe Kleine-K?nig wrote:
> Hello Thierry,
> 
> On Thu, Nov 15, 2018 at 05:21:59PM +0100, Thierry Reding wrote:
> > On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-K?nig wrote:
> > > On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > > > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-K?nig wrote:
> > > > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > > > having to make the aware of the configured polarity (and in some cases even
> > > > > period).
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  include/linux/pwm.h | 13 ++++++++++---
> > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > So if I understand your intention correctly here you want consumers to
> > > > be able to get at the default state, which would be the state as it was
> > > > "configured" in DT or the PWM lookup table so that you can modify that
> > > > state (well, really just setting the duty-cycle) before you actually
> > > > send the state to the hardware.
> > > > 
> > > > So the difference to pwm_apply_args() is that you don't want the driver
> > > > to specifically program a duty cycle of 0 before setting the actual duty
> > > > cycle that you want to set.
> > > > 
> > > > Is that about right?
> > > 
> > > Yes, and a resulting advantage is that the apply callback is only called
> > > once.
> > >  
> > > > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > > > uses it for exactly the same purpose that you seem to be using it in
> > > > RX1950 backlight control.
> > > 
> > > I didn't use pwm_init_state because that doesn't give a helpful value
> > > for state->enabled. (I think it is: if the driver provides a get_state
> > > callback it is whatever that one set, otherwise it's off.)
> > 
> > That's correct. It's pretty much the same thing that you have, except
> > you explicitly set enable = false. But that's not really important
> > because consumers are supposed to explicitly override that anyway.
> 
> If we hardcode to enable=false the call to pwm_get_state could be
> dropped, too. Then the two functions are really semantically identical.
> > 
> > > Also the name pwm_init_state isn't that good IMHO. Just from the name
> > > I'd expect it to be something like
> > > 
> > > 	memset(state, 0, sizeof(&state));
> > > 
> > > instead of something more informed that depends on hardware state and/or
> > > data provided by a device tree.
> > 
> > Be that as it may, the kerneldoc is pretty explicit about what it does
> > and so is the code. Not liking the name shouldn't be an excuse for
> > duplicating functionality.
> 
> With your suggestion to move the initialisation of ->state into the
> core, maybe this function can go away from the public API such that the
> naming doesn't really matter (and can be adapted easily).

Yeah, I think it would be best to move it into core.c and make it static
in that case. There should be no need for anyone to create any special
state, they can just use pwm_get_state() instead.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181116/621c9ecc/attachment.sig>