[PATCH 1/4] pwm: berlin: use consistent naming for variables

[PATCH 1/4] pwm: berlin: use consistent naming for variables

[Uwe Kleine-König]

A struct berlin_pwm_chip* is now always called "bpc" (instead of "pwm"
which is usually used for struct pwm_device* or "chip" which is usually
used for struct pwm_chip*). The struct pwm_device* variables were named
"pwm_dev" or "pwm"; they is now always called "pwm".

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

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index acb6fbc3cc32..0310d28408ed 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -56,17 +56,17 @@ static inline struct berlin_pwm_chip *to_berlin_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct berlin_pwm_chip, chip);
 }
 
-static inline u32 berlin_pwm_readl(struct berlin_pwm_chip *chip,
+static inline u32 berlin_pwm_readl(struct berlin_pwm_chip *bpc,
 				   unsigned int channel, unsigned long offset)
 {
-	return readl_relaxed(chip->base + channel * 0x10 + offset);
+	return readl_relaxed(bpc->base + channel * 0x10 + offset);
 }
 
-static inline void berlin_pwm_writel(struct berlin_pwm_chip *chip,
+static inline void berlin_pwm_writel(struct berlin_pwm_chip *bpc,
 				     unsigned int channel, u32 value,
 				     unsigned long offset)
 {
-	writel_relaxed(value, chip->base + channel * 0x10 + offset);
+	writel_relaxed(value, bpc->base + channel * 0x10 + offset);
 }
 
 static int berlin_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -87,15 +87,15 @@ static void berlin_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	kfree(channel);
 }
 
-static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev,
+static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			     int duty_ns, int period_ns)
 {
-	struct berlin_pwm_chip *pwm = to_berlin_pwm_chip(chip);
+	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	bool prescale_4096 = false;
 	u32 value, duty, period;
 	u64 cycles;
 
-	cycles = clk_get_rate(pwm->clk);
+	cycles = clk_get_rate(bpc->clk);
 	cycles *= period_ns;
 	do_div(cycles, NSEC_PER_SEC);
 
@@ -112,59 +112,59 @@ static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev,
 	do_div(cycles, period_ns);
 	duty = cycles;
 
-	value = berlin_pwm_readl(pwm, pwm_dev->hwpwm, BERLIN_PWM_CONTROL);
+	value = berlin_pwm_readl(bpc, pwm->hwpwm, BERLIN_PWM_CONTROL);
 	if (prescale_4096)
 		value |= BERLIN_PWM_PRESCALE_4096;
 	else
 		value &= ~BERLIN_PWM_PRESCALE_4096;
-	berlin_pwm_writel(pwm, pwm_dev->hwpwm, value, BERLIN_PWM_CONTROL);
+	berlin_pwm_writel(bpc, pwm->hwpwm, value, BERLIN_PWM_CONTROL);
 
-	berlin_pwm_writel(pwm, pwm_dev->hwpwm, duty, BERLIN_PWM_DUTY);
-	berlin_pwm_writel(pwm, pwm_dev->hwpwm, period, BERLIN_PWM_TCNT);
+	berlin_pwm_writel(bpc, pwm->hwpwm, duty, BERLIN_PWM_DUTY);
+	berlin_pwm_writel(bpc, pwm->hwpwm, period, BERLIN_PWM_TCNT);
 
 	return 0;
 }
 
 static int berlin_pwm_set_polarity(struct pwm_chip *chip,
-				   struct pwm_device *pwm_dev,
+				   struct pwm_device *pwm,
 				   enum pwm_polarity polarity)
 {
-	struct berlin_pwm_chip *pwm = to_berlin_pwm_chip(chip);
+	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	u32 value;
 
-	value = berlin_pwm_readl(pwm, pwm_dev->hwpwm, BERLIN_PWM_CONTROL);
+	value = berlin_pwm_readl(bpc, pwm->hwpwm, BERLIN_PWM_CONTROL);
 
 	if (polarity == PWM_POLARITY_NORMAL)
 		value &= ~BERLIN_PWM_INVERT_POLARITY;
 	else
 		value |= BERLIN_PWM_INVERT_POLARITY;
 
-	berlin_pwm_writel(pwm, pwm_dev->hwpwm, value, BERLIN_PWM_CONTROL);
+	berlin_pwm_writel(bpc, pwm->hwpwm, value, BERLIN_PWM_CONTROL);
 
 	return 0;
 }
 
-static int berlin_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm_dev)
+static int berlin_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct berlin_pwm_chip *pwm = to_berlin_pwm_chip(chip);
+	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	u32 value;
 
-	value = berlin_pwm_readl(pwm, pwm_dev->hwpwm, BERLIN_PWM_EN);
+	value = berlin_pwm_readl(bpc, pwm->hwpwm, BERLIN_PWM_EN);
 	value |= BERLIN_PWM_ENABLE;
-	berlin_pwm_writel(pwm, pwm_dev->hwpwm, value, BERLIN_PWM_EN);
+	berlin_pwm_writel(bpc, pwm->hwpwm, value, BERLIN_PWM_EN);
 
 	return 0;
 }
 
 static void berlin_pwm_disable(struct pwm_chip *chip,
-			       struct pwm_device *pwm_dev)
+			       struct pwm_device *pwm)
 {
-	struct berlin_pwm_chip *pwm = to_berlin_pwm_chip(chip);
+	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	u32 value;
 
-	value = berlin_pwm_readl(pwm, pwm_dev->hwpwm, BERLIN_PWM_EN);
+	value = berlin_pwm_readl(bpc, pwm->hwpwm, BERLIN_PWM_EN);
 	value &= ~BERLIN_PWM_ENABLE;
-	berlin_pwm_writel(pwm, pwm_dev->hwpwm, value, BERLIN_PWM_EN);
+	berlin_pwm_writel(bpc, pwm->hwpwm, value, BERLIN_PWM_EN);
 }
 
 static const struct pwm_ops berlin_pwm_ops = {
@@ -185,50 +185,50 @@ MODULE_DEVICE_TABLE(of, berlin_pwm_match);
 
 static int berlin_pwm_probe(struct platform_device *pdev)
 {
-	struct berlin_pwm_chip *pwm;
+	struct berlin_pwm_chip *bpc;
 	int ret;
 
-	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
-	if (!pwm)
+	bpc = devm_kzalloc(&pdev->dev, sizeof(*bpc), GFP_KERNEL);
+	if (!bpc)
 		return -ENOMEM;
 
-	pwm->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(pwm->base))
-		return PTR_ERR(pwm->base);
+	bpc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(bpc->base))
+		return PTR_ERR(bpc->base);
 
-	pwm->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pwm->clk))
-		return PTR_ERR(pwm->clk);
+	bpc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(bpc->clk))
+		return PTR_ERR(bpc->clk);
 
-	ret = clk_prepare_enable(pwm->clk);
+	ret = clk_prepare_enable(bpc->clk);
 	if (ret)
 		return ret;
 
-	pwm->chip.dev = &pdev->dev;
-	pwm->chip.ops = &berlin_pwm_ops;
-	pwm->chip.npwm = 4;
-	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
-	pwm->chip.of_pwm_n_cells = 3;
+	bpc->chip.dev = &pdev->dev;
+	bpc->chip.ops = &berlin_pwm_ops;
+	bpc->chip.npwm = 4;
+	bpc->chip.of_xlate = of_pwm_xlate_with_flags;
+	bpc->chip.of_pwm_n_cells = 3;
 
-	ret = pwmchip_add(&pwm->chip);
+	ret = pwmchip_add(&bpc->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
-		clk_disable_unprepare(pwm->clk);
+		clk_disable_unprepare(bpc->clk);
 		return ret;
 	}
 
-	platform_set_drvdata(pdev, pwm);
+	platform_set_drvdata(pdev, bpc);
 
 	return 0;
 }
 
 static int berlin_pwm_remove(struct platform_device *pdev)
 {
-	struct berlin_pwm_chip *pwm = platform_get_drvdata(pdev);
+	struct berlin_pwm_chip *bpc = platform_get_drvdata(pdev);
 	int ret;
 
-	ret = pwmchip_remove(&pwm->chip);
-	clk_disable_unprepare(pwm->clk);
+	ret = pwmchip_remove(&bpc->chip);
+	clk_disable_unprepare(bpc->clk);
 
 	return ret;
 }
@@ -236,48 +236,48 @@ static int berlin_pwm_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int berlin_pwm_suspend(struct device *dev)
 {
-	struct berlin_pwm_chip *pwm = dev_get_drvdata(dev);
+	struct berlin_pwm_chip *bpc = dev_get_drvdata(dev);
 	unsigned int i;
 
-	for (i = 0; i < pwm->chip.npwm; i++) {
+	for (i = 0; i < bpc->chip.npwm; i++) {
 		struct berlin_pwm_channel *channel;
 
-		channel = pwm_get_chip_data(&pwm->chip.pwms[i]);
+		channel = pwm_get_chip_data(&bpc->chip.pwms[i]);
 		if (!channel)
 			continue;
 
-		channel->enable = berlin_pwm_readl(pwm, i, BERLIN_PWM_ENABLE);
-		channel->ctrl = berlin_pwm_readl(pwm, i, BERLIN_PWM_CONTROL);
-		channel->duty = berlin_pwm_readl(pwm, i, BERLIN_PWM_DUTY);
-		channel->tcnt = berlin_pwm_readl(pwm, i, BERLIN_PWM_TCNT);
+		channel->enable = berlin_pwm_readl(bpc, i, BERLIN_PWM_ENABLE);
+		channel->ctrl = berlin_pwm_readl(bpc, i, BERLIN_PWM_CONTROL);
+		channel->duty = berlin_pwm_readl(bpc, i, BERLIN_PWM_DUTY);
+		channel->tcnt = berlin_pwm_readl(bpc, i, BERLIN_PWM_TCNT);
 	}
 
-	clk_disable_unprepare(pwm->clk);
+	clk_disable_unprepare(bpc->clk);
 
 	return 0;
 }
 
 static int berlin_pwm_resume(struct device *dev)
 {
-	struct berlin_pwm_chip *pwm = dev_get_drvdata(dev);
+	struct berlin_pwm_chip *bpc = dev_get_drvdata(dev);
 	unsigned int i;
 	int ret;
 
-	ret = clk_prepare_enable(pwm->clk);
+	ret = clk_prepare_enable(bpc->clk);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < pwm->chip.npwm; i++) {
+	for (i = 0; i < bpc->chip.npwm; i++) {
 		struct berlin_pwm_channel *channel;
 
-		channel = pwm_get_chip_data(&pwm->chip.pwms[i]);
+		channel = pwm_get_chip_data(&bpc->chip.pwms[i]);
 		if (!channel)
 			continue;
 
-		berlin_pwm_writel(pwm, i, channel->ctrl, BERLIN_PWM_CONTROL);
-		berlin_pwm_writel(pwm, i, channel->duty, BERLIN_PWM_DUTY);
-		berlin_pwm_writel(pwm, i, channel->tcnt, BERLIN_PWM_TCNT);
-		berlin_pwm_writel(pwm, i, channel->enable, BERLIN_PWM_ENABLE);
+		berlin_pwm_writel(bpc, i, channel->ctrl, BERLIN_PWM_CONTROL);
+		berlin_pwm_writel(bpc, i, channel->duty, BERLIN_PWM_DUTY);
+		berlin_pwm_writel(bpc, i, channel->tcnt, BERLIN_PWM_TCNT);
+		berlin_pwm_writel(bpc, i, channel->enable, BERLIN_PWM_ENABLE);
 	}
 
 	return 0;

base-commit: a6efb35019d00f483a0e5f188747723371d659fe
-- 
2.30.2

[PATCH 2/4] pwm: berlin: Put channel config into driver data

[Uwe Kleine-König]

Instead of allocating extra data in .request() provide the needed memory
in struct berlin_pwm_chip. This reduces the number of allocations. A side
effect is that on suspend and resume the state for all four channels is
always saved and restored. This is easier (and probably quicker) than
looking up the matching pwm_device and checking its PWMF_REQUESTED bit.

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

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 0310d28408ed..894ea5869d42 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -38,6 +38,8 @@
 #define BERLIN_PWM_TCNT			0xc
 #define  BERLIN_PWM_MAX_TCNT		65535
 
+#define BERLIN_PWM_NUMPWMS		4
+
 struct berlin_pwm_channel {
 	u32 enable;
 	u32 ctrl;
@@ -49,6 +51,7 @@ struct berlin_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
 	void __iomem *base;
+	struct berlin_pwm_channel channel[BERLIN_PWM_NUMPWMS];
 };
 
 static inline struct berlin_pwm_chip *to_berlin_pwm_chip(struct pwm_chip *chip)
@@ -69,24 +72,6 @@ static inline void berlin_pwm_writel(struct berlin_pwm_chip *bpc,
 	writel_relaxed(value, bpc->base + channel * 0x10 + offset);
 }
 
-static int berlin_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct berlin_pwm_channel *channel;
-
-	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
-	if (!channel)
-		return -ENOMEM;
-
-	return pwm_set_chip_data(pwm, channel);
-}
-
-static void berlin_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct berlin_pwm_channel *channel = pwm_get_chip_data(pwm);
-
-	kfree(channel);
-}
-
 static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			     int duty_ns, int period_ns)
 {
@@ -168,8 +153,6 @@ static void berlin_pwm_disable(struct pwm_chip *chip,
 }
 
 static const struct pwm_ops berlin_pwm_ops = {
-	.request = berlin_pwm_request,
-	.free = berlin_pwm_free,
 	.config = berlin_pwm_config,
 	.set_polarity = berlin_pwm_set_polarity,
 	.enable = berlin_pwm_enable,
@@ -206,7 +189,7 @@ static int berlin_pwm_probe(struct platform_device *pdev)
 
 	bpc->chip.dev = &pdev->dev;
 	bpc->chip.ops = &berlin_pwm_ops;
-	bpc->chip.npwm = 4;
+	bpc->chip.npwm = BERLIN_PWM_NUMPWMS;
 	bpc->chip.of_xlate = of_pwm_xlate_with_flags;
 	bpc->chip.of_pwm_n_cells = 3;
 
@@ -240,11 +223,7 @@ static int berlin_pwm_suspend(struct device *dev)
 	unsigned int i;
 
 	for (i = 0; i < bpc->chip.npwm; i++) {
-		struct berlin_pwm_channel *channel;
-
-		channel = pwm_get_chip_data(&bpc->chip.pwms[i]);
-		if (!channel)
-			continue;
+		struct berlin_pwm_channel *channel = &bpc->channel[i];
 
 		channel->enable = berlin_pwm_readl(bpc, i, BERLIN_PWM_ENABLE);
 		channel->ctrl = berlin_pwm_readl(bpc, i, BERLIN_PWM_CONTROL);
@@ -268,11 +247,7 @@ static int berlin_pwm_resume(struct device *dev)
 		return ret;
 
 	for (i = 0; i < bpc->chip.npwm; i++) {
-		struct berlin_pwm_channel *channel;
-
-		channel = pwm_get_chip_data(&bpc->chip.pwms[i]);
-		if (!channel)
-			continue;
+		struct berlin_pwm_channel *channel = &bpc->channel[i];
 
 		berlin_pwm_writel(bpc, i, channel->ctrl, BERLIN_PWM_CONTROL);
 		berlin_pwm_writel(bpc, i, channel->duty, BERLIN_PWM_DUTY);
-- 
2.30.2

[PATCH 3/4] pwm: berlin: Implement .apply() callback

[Uwe Kleine-König]

To eventually get rid of all legacy drivers convert this driver to the
modern world implementing .apply(). This just pushes down a slightly
optimized variant of how legacy drivers are handled in the core.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-berlin.c | 43 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 894ea5869d42..4e838a973c97 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -73,7 +73,7 @@ static inline void berlin_pwm_writel(struct berlin_pwm_chip *bpc,
 }
 
 static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			     int duty_ns, int period_ns)
+			     u64 duty_ns, u64 period_ns)
 {
 	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	bool prescale_4096 = false;
@@ -152,11 +152,44 @@ static void berlin_pwm_disable(struct pwm_chip *chip,
 	berlin_pwm_writel(bpc, pwm->hwpwm, value, BERLIN_PWM_EN);
 }
 
+static int berlin_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	int err;
+	bool enabled = pwm->state.enabled;
+
+	if (state->polarity != pwm->state.polarity) {
+		if (enabled) {
+			berlin_pwm_disable(chip, pwm);
+			enabled = false;
+		}
+
+		err = berlin_pwm_set_polarity(chip, pwm, state->polarity);
+		if (err)
+			return err;
+	}
+
+	if (!state->enabled) {
+		if (enabled)
+			berlin_pwm_disable(chip, pwm);
+		return 0;
+	}
+
+	if (state->period != pwm->state.period ||
+	    state->duty_cycle != pwm->state.duty_cycle) {
+		err = berlin_pwm_config(chip, pwm, state->duty_cycle, state->period);
+		if (err)
+			return err;
+	}
+
+	if (!enabled)
+		return berlin_pwm_enable(chip, pwm);
+
+	return 0;
+}
+
 static const struct pwm_ops berlin_pwm_ops = {
-	.config = berlin_pwm_config,
-	.set_polarity = berlin_pwm_set_polarity,
-	.enable = berlin_pwm_enable,
-	.disable = berlin_pwm_disable,
+	.apply = berlin_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.30.2

[PATCH 4/4] pwm: berlin: Don't check the return code of pwmchip_remove()

[Uwe Kleine-König]

pwmchip_remove() always returns 0. Don't use the value to make it
possible to eventually change the function to return void. This is a
good thing as pwmchip_remove() is usually called from a remove function
(mostly for platform devices) and their return value is ignored by the
device core anyhow.

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

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 4e838a973c97..605e91d2f6b9 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -241,12 +241,12 @@ static int berlin_pwm_probe(struct platform_device *pdev)
 static int berlin_pwm_remove(struct platform_device *pdev)
 {
 	struct berlin_pwm_chip *bpc = platform_get_drvdata(pdev);
-	int ret;
 
-	ret = pwmchip_remove(&bpc->chip);
+	pwmchip_remove(&bpc->chip);
+
 	clk_disable_unprepare(bpc->clk);
 
-	return ret;
+	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.30.2

Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data

[Uwe Kleine-König]

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

Hello Thierry,

On Tue, May 04, 2021 at 03:25:35PM +0200, Uwe Kleine-König wrote:
> Instead of allocating extra data in .request() provide the needed memory
> in struct berlin_pwm_chip. This reduces the number of allocations. A side
> effect is that on suspend and resume the state for all four channels is
> always saved and restored. This is easier (and probably quicker) than
> looking up the matching pwm_device and checking its PWMF_REQUESTED bit.

I noticed you applied the other three patches in this series, but
skipped this one and marked it as rejected.

Please point out what you don't like about this patch instead of just
dropping it without comment.

Best regards
Uwe

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

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

Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data

[Uwe Kleine-König]

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

Hello Thierry,

On Wed, Jun 30, 2021 at 08:18:04AM +0200, Uwe Kleine-König wrote:
> On Tue, May 04, 2021 at 03:25:35PM +0200, Uwe Kleine-König wrote:
> > Instead of allocating extra data in .request() provide the needed memory
> > in struct berlin_pwm_chip. This reduces the number of allocations. A side
> > effect is that on suspend and resume the state for all four channels is
> > always saved and restored. This is easier (and probably quicker) than
> > looking up the matching pwm_device and checking its PWMF_REQUESTED bit.
> 
> I noticed you applied the other three patches in this series, but
> skipped this one and marked it as rejected.
> 
> Please point out what you don't like about this patch instead of just
> dropping it without comment.

Any news on this? I still consider the patch good and would like to know
your objections.

Best regards
Uwe


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

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

Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data

[Uwe Kleine-König]

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

Hello Thierry,

On Thu, Sep 09, 2021 at 04:16:03PM +0200, Uwe Kleine-König wrote:
> On Wed, Jun 30, 2021 at 08:18:04AM +0200, Uwe Kleine-König wrote:
> > On Tue, May 04, 2021 at 03:25:35PM +0200, Uwe Kleine-König wrote:
> > > Instead of allocating extra data in .request() provide the needed memory
> > > in struct berlin_pwm_chip. This reduces the number of allocations. A side
> > > effect is that on suspend and resume the state for all four channels is
> > > always saved and restored. This is easier (and probably quicker) than
> > > looking up the matching pwm_device and checking its PWMF_REQUESTED bit.
> > 
> > I noticed you applied the other three patches in this series, but
> > skipped this one and marked it as rejected.
> > 
> > Please point out what you don't like about this patch instead of just
> > dropping it without comment.
> 
> Any news on this? I still consider the patch good and would like to know
> your objections.

This patch is still in my private working copy and I still don't know
why you rejected it. :-\

Best regards
Uwe

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

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

Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data

[Thierry Reding]

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

On Tue, May 04, 2021 at 03:25:35PM +0200, Uwe Kleine-König wrote:
> Instead of allocating extra data in .request() provide the needed memory
> in struct berlin_pwm_chip. This reduces the number of allocations. A side
> effect is that on suspend and resume the state for all four channels is
> always saved and restored. This is easier (and probably quicker) than
> looking up the matching pwm_device and checking its PWMF_REQUESTED bit.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-berlin.c | 37 ++++++-------------------------------
>  1 file changed, 6 insertions(+), 31 deletions(-)

This doesn't look like a worthwhile change to me. The per-PWM channel
data was originally introduced to support exactly this type of use-case,
so I don't see why we shouldn't keep using it here.

Thierry

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

Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data

[Uwe Kleine-König]

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

On Mon, Nov 08, 2021 at 01:09:42PM +0100, Thierry Reding wrote:
> On Tue, May 04, 2021 at 03:25:35PM +0200, Uwe Kleine-König wrote:
> > Instead of allocating extra data in .request() provide the needed memory
> > in struct berlin_pwm_chip. This reduces the number of allocations. A side
> > effect is that on suspend and resume the state for all four channels is
> > always saved and restored. This is easier (and probably quicker) than
> > looking up the matching pwm_device and checking its PWMF_REQUESTED bit.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-berlin.c | 37 ++++++-------------------------------
> >  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> This doesn't look like a worthwhile change to me. The per-PWM channel
> data was originally introduced to support exactly this type of use-case,
> so I don't see why we shouldn't keep using it here.

My reasons to not use the per-channel data are:

 - up to 5 smaller mallocs instead of a single bigger one. A single
   allocation is more effective (AFAIK) regarding memory management
   overhead, memory management overhead, and cache locality

 - bad naming, this isn't per-chip data as the function name
   "pwm_set_chip_data" suggests, but per-channel ("pwm"?) data.

 - maybe subjectively the data model is easier to understand if all
   required data (clk, channel state etc) is in a single data structure

 - With an arm allmodconfig bloat-o-meter reports for my patch:

	add/remove: 0/2 grow/shrink: 0/2 up/down: 0/-376 (-376)
	Function                                     old     new   delta
	berlin_pwm_free                               44       -     -44
	berlin_pwm_suspend                           364     256    -108
	berlin_pwm_resume                            444     332    -112
	berlin_pwm_request                           112       -    -112
	Total: Before=3712, After=3336, chg -10.13%

   So code size (and probably also run time) is improved.

If "back then we considered per-channel data a good idea, so let's use
it" is really your best argument to keep the code as is, I ask you to
reconsider that.

I checked the other drivers that make use of pwm_set_chip_data, and some
of them have a really strange usage pattern. Will prepare patches for
these and let you judge if these are worthwile.

Best regards
Uwe

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

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