* [PATCH 1/4] pwm: berlin: use consistent naming for variables
@ 2021-05-04 13:25 Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 2/4] pwm: berlin: Put channel config into driver data Uwe Kleine-König
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-05-04 13:25 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel, Thomas Hebb, Jisheng Zhang
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/4] pwm: berlin: Put channel config into driver data
2021-05-04 13:25 [PATCH 1/4] pwm: berlin: use consistent naming for variables Uwe Kleine-König
@ 2021-05-04 13:25 ` Uwe Kleine-König
2021-06-30 6:18 ` Uwe Kleine-König
2021-11-08 12:09 ` Thierry Reding
2021-05-04 13:25 ` [PATCH 3/4] pwm: berlin: Implement .apply() callback Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 4/4] pwm: berlin: Don't check the return code of pwmchip_remove() Uwe Kleine-König
2 siblings, 2 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-05-04 13:25 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel, Thomas Hebb, Jisheng Zhang
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data
2021-05-04 13:25 ` [PATCH 2/4] pwm: berlin: Put channel config into driver data Uwe Kleine-König
@ 2021-06-30 6:18 ` Uwe Kleine-König
2021-09-09 14:16 ` Uwe Kleine-König
2021-11-08 12:09 ` Thierry Reding
1 sibling, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2021-06-30 6:18 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: Jisheng Zhang, linux-pwm, Thomas Hebb, kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data
2021-06-30 6:18 ` Uwe Kleine-König
@ 2021-09-09 14:16 ` Uwe Kleine-König
2021-11-08 11:04 ` Uwe Kleine-König
0 siblings, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2021-09-09 14:16 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: Jisheng Zhang, linux-pwm, Thomas Hebb, kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data
2021-09-09 14:16 ` Uwe Kleine-König
@ 2021-11-08 11:04 ` Uwe Kleine-König
0 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-11-08 11:04 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: Jisheng Zhang, linux-pwm, Thomas Hebb, kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data
2021-05-04 13:25 ` [PATCH 2/4] pwm: berlin: Put channel config into driver data Uwe Kleine-König
2021-06-30 6:18 ` Uwe Kleine-König
@ 2021-11-08 12:09 ` Thierry Reding
2021-11-08 13:08 ` Uwe Kleine-König
1 sibling, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2021-11-08 12:09 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lee Jones, linux-pwm, kernel, Thomas Hebb, Jisheng Zhang
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data
2021-11-08 12:09 ` Thierry Reding
@ 2021-11-08 13:08 ` Uwe Kleine-König
0 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-11-08 13:08 UTC (permalink / raw)
To: Thierry Reding; +Cc: Jisheng Zhang, linux-pwm, Thomas Hebb, Lee Jones, kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] pwm: berlin: Implement .apply() callback
2021-05-04 13:25 [PATCH 1/4] pwm: berlin: use consistent naming for variables Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 2/4] pwm: berlin: Put channel config into driver data Uwe Kleine-König
@ 2021-05-04 13:25 ` Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 4/4] pwm: berlin: Don't check the return code of pwmchip_remove() Uwe Kleine-König
2 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-05-04 13:25 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel, Thomas Hebb, Jisheng Zhang
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/4] pwm: berlin: Don't check the return code of pwmchip_remove()
2021-05-04 13:25 [PATCH 1/4] pwm: berlin: use consistent naming for variables Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 2/4] pwm: berlin: Put channel config into driver data Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 3/4] pwm: berlin: Implement .apply() callback Uwe Kleine-König
@ 2021-05-04 13:25 ` Uwe Kleine-König
2 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-05-04 13:25 UTC (permalink / raw)
To: Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel, Thomas Hebb, Jisheng Zhang
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-08 13:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 13:25 [PATCH 1/4] pwm: berlin: use consistent naming for variables Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 2/4] pwm: berlin: Put channel config into driver data Uwe Kleine-König
2021-06-30 6:18 ` Uwe Kleine-König
2021-09-09 14:16 ` Uwe Kleine-König
2021-11-08 11:04 ` Uwe Kleine-König
2021-11-08 12:09 ` Thierry Reding
2021-11-08 13:08 ` Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 3/4] pwm: berlin: Implement .apply() callback Uwe Kleine-König
2021-05-04 13:25 ` [PATCH 4/4] pwm: berlin: Don't check the return code of pwmchip_remove() Uwe Kleine-König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).