* [PATCH] pwm: ab8500: Fix register offset calculation to not depend on probe order
@ 2021-07-05 16:55 Uwe Kleine-König
2021-07-05 22:35 ` Linus Walleij
0 siblings, 1 reply; 2+ messages in thread
From: Uwe Kleine-König @ 2021-07-05 16:55 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Linus Walleij
Cc: Arun Murthy, Arnd Bergmann, linux-pwm, kernel
The assumption that lead to commit 5e5da1e9fbee ("pwm: ab8500:
Explicitly allocate pwm chip base dynamically") was wrong: The
pwm-ab8500 devices are not directly instantiated from device tree, but
from the ab8500 mfd driver. So the pdev->id isn't -1, but a number
between 1 and 3. Now that pwmchip ids are always allocated dynamically,
this cannot easily be reverted.
Introduce a new member in the driver data struct that tracks the
hardware id and use this to calculate the register offset.
Side-note: Using chip->base to calculate the offset was never robust
because if there was already a PWM with id 1 at the time ab8500-pwm.1
was probed, the associated pwmchip would get assigned chip->base = 2 (or
something bigger).
Fixes: 5e5da1e9fbee ("pwm: ab8500: Explicitly allocate pwm chip base dynamically")
Fixes: 6173f8f4ed9c ("pwm: Move AB8500 PWM driver to PWM framework")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-ab8500.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c
index e2a26d9da25b..281f74a1c50b 100644
--- a/drivers/pwm/pwm-ab8500.c
+++ b/drivers/pwm/pwm-ab8500.c
@@ -22,14 +22,21 @@
struct ab8500_pwm_chip {
struct pwm_chip chip;
+ unsigned int hwid;
};
+static struct ab8500_pwm_chip *ab8500_pwm_from_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct ab8500_pwm_chip, chip);
+}
+
static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
int ret;
u8 reg;
unsigned int higher_val, lower_val;
+ struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip);
if (state->polarity != PWM_POLARITY_NORMAL)
return -EINVAL;
@@ -37,7 +44,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (!state->enabled) {
ret = abx500_mask_and_set_register_interruptible(chip->dev,
AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG,
- 1 << (chip->base - 1), 0);
+ 1 << ab8500->hwid, 0);
if (ret < 0)
dev_err(chip->dev, "%s: Failed to disable PWM, Error %d\n",
@@ -56,7 +63,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
*/
higher_val = ((state->duty_cycle & 0x0300) >> 8);
- reg = AB8500_PWM_OUT_CTRL1_REG + ((chip->base - 1) * 2);
+ reg = AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2);
ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC,
reg, (u8)lower_val);
@@ -70,7 +77,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
ret = abx500_mask_and_set_register_interruptible(chip->dev,
AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG,
- 1 << (chip->base - 1), 1 << (chip->base - 1));
+ 1 << ab8500->hwid, 1 << ab8500->hwid);
if (ret < 0)
dev_err(chip->dev, "%s: Failed to enable PWM, Error %d\n",
pwm->label, ret);
@@ -88,6 +95,9 @@ static int ab8500_pwm_probe(struct platform_device *pdev)
struct ab8500_pwm_chip *ab8500;
int err;
+ if (pdev->id < 1 || pdev->id > 31)
+ return dev_err_probe(&pdev->dev, EINVAL, "Invalid device id %d\n", pdev->id);
+
/*
* Nothing to be done in probe, this is required to get the
* device which is required for ab8500 read and write
@@ -99,6 +109,7 @@ static int ab8500_pwm_probe(struct platform_device *pdev)
ab8500->chip.dev = &pdev->dev;
ab8500->chip.ops = &ab8500_pwm_ops;
ab8500->chip.npwm = 1;
+ ab8500->hwid = pdev->id - 1;
err = pwmchip_add(&ab8500->chip);
if (err < 0)
base-commit: 96e45e5202377da39d086ec19f8934ebcc5b0fd6
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] pwm: ab8500: Fix register offset calculation to not depend on probe order
2021-07-05 16:55 [PATCH] pwm: ab8500: Fix register offset calculation to not depend on probe order Uwe Kleine-König
@ 2021-07-05 22:35 ` Linus Walleij
0 siblings, 0 replies; 2+ messages in thread
From: Linus Walleij @ 2021-07-05 22:35 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thierry Reding, Lee Jones, Arun Murthy, Arnd Bergmann, linux-pwm,
Sascha Hauer
On Mon, Jul 5, 2021 at 6:55 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The assumption that lead to commit 5e5da1e9fbee ("pwm: ab8500:
> Explicitly allocate pwm chip base dynamically") was wrong: The
> pwm-ab8500 devices are not directly instantiated from device tree, but
> from the ab8500 mfd driver. So the pdev->id isn't -1, but a number
> between 1 and 3. Now that pwmchip ids are always allocated dynamically,
> this cannot easily be reverted.
>
> Introduce a new member in the driver data struct that tracks the
> hardware id and use this to calculate the register offset.
>
> Side-note: Using chip->base to calculate the offset was never robust
> because if there was already a PWM with id 1 at the time ab8500-pwm.1
> was probed, the associated pwmchip would get assigned chip->base = 2 (or
> something bigger).
>
> Fixes: 5e5da1e9fbee ("pwm: ab8500: Explicitly allocate pwm chip base dynamically")
> Fixes: 6173f8f4ed9c ("pwm: Move AB8500 PWM driver to PWM framework")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
This looks good to me so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
It seems we may have a vibrator motor connected to this on one
of our reference designs, so I am trying to figure out if I can test
this properly.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-07-05 22:35 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05 16:55 [PATCH] pwm: ab8500: Fix register offset calculation to not depend on probe order Uwe Kleine-König
2021-07-05 22:35 ` Linus Walleij
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).