| Message ID | 20230808171931.944154-72-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
On Tue, Aug 08, 2023 at 07:19:01PM +0200, Uwe Kleine-König wrote: > struct pwm_chip::dev is about to change. To not have to touch this > driver in the same commit as struct pwm_chip::dev, store a pointer to > the parent device in driver data. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-ab8500.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) You're basically adding a parent device to all driver-private data structures in this and the following patches, so why not keep in in struct pwm_chip and simply rename chip->dev to chip->parent? As Andy has commented, this would eventually allow the PWM core to take care of certain things like runtime PM, or even only for stuff like using the parent device name in info/debug/error messages. Also, you could then just make this a single large patch that renames dev to parent in one go rather than making this large set even larger with this kind of trivial changes. Thierry > > diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c > index f64f3fd251e7..663fdfe90bb6 100644 > --- a/drivers/pwm/pwm-ab8500.c > +++ b/drivers/pwm/pwm-ab8500.c > @@ -24,6 +24,7 @@ > #define AB8500_PWM_CLKRATE 9600000 > > struct ab8500_pwm_chip { > + struct device *parent; > unsigned int hwid; > }; > > @@ -91,7 +92,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > * when disabled. > */ > if (!state->enabled || duty_steps == 0) { > - ret = abx500_mask_and_set_register_interruptible(chip->dev, > + ret = abx500_mask_and_set_register_interruptible(ab8500->parent, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > 1 << ab8500->hwid, 0); > > @@ -111,18 +112,18 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > reg = AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2); > > - ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC, > + ret = abx500_set_register_interruptible(ab8500->parent, AB8500_MISC, > reg, lower_val); > if (ret < 0) > return ret; > > - ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC, > + ret = abx500_set_register_interruptible(ab8500->parent, AB8500_MISC, > (reg + 1), higher_val); > if (ret < 0) > return ret; > > /* enable */ > - ret = abx500_mask_and_set_register_interruptible(chip->dev, > + ret = abx500_mask_and_set_register_interruptible(ab8500->parent, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > 1 << ab8500->hwid, 1 << ab8500->hwid); > > @@ -137,7 +138,7 @@ static int ab8500_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip); > unsigned int div, duty_steps; > > - ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC, > + ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC, > AB8500_PWM_OUT_CTRL7_REG, > &ctrl7); > if (ret) > @@ -150,13 +151,13 @@ static int ab8500_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > return 0; > } > > - ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC, > + ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC, > AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2), > &lower_val); > if (ret) > return ret; > > - ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC, > + ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC, > AB8500_PWM_OUT_CTRL2_REG + (ab8500->hwid * 2), > &higher_val); > if (ret) > @@ -196,6 +197,7 @@ static int ab8500_pwm_probe(struct platform_device *pdev) > ab8500 = pwmchip_priv(chip); > > chip->ops = &ab8500_pwm_ops; > + ab8500->parent = &pdev->dev; > ab8500->hwid = pdev->id - 1; > > err = devm_pwmchip_add(&pdev->dev, chip); > -- > 2.40.1 >
Hello, On Fri, Oct 06, 2023 at 11:41:51AM +0200, Thierry Reding wrote: > On Tue, Aug 08, 2023 at 07:19:01PM +0200, Uwe Kleine-König wrote: > > struct pwm_chip::dev is about to change. To not have to touch this > > driver in the same commit as struct pwm_chip::dev, store a pointer to > > the parent device in driver data. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/pwm-ab8500.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > You're basically adding a parent device to all driver-private data > structures in this and the following patches, so why not keep in in > struct pwm_chip and simply rename chip->dev to chip->parent? > > As Andy has commented, this would eventually allow the PWM core to > take care of certain things like runtime PM, or even only for stuff > like using the parent device name in info/debug/error messages. > > Also, you could then just make this a single large patch that renames > dev to parent in one go rather than making this large set even larger > with this kind of trivial changes. The idea here is (again) that I don't have to touch all drivers in the commit that changes struct pwm_chip. In the end there is such a parent pointer (pwmchip.dev.parent). Would you prefer a macro (say) pwmchip_parentdev that can be defined as #define pwmchip_parentdev(chip) (chip)->dev at the beginning and then be changed to #define pwmchip_parentdev(chip) (chip)->dev.parent in the right moment? That's the best idea I have here. Best regards Uwe
On Fri, Oct 06, 2023 at 01:09:51PM +0200, Uwe Kleine-König wrote: > Hello, > > On Fri, Oct 06, 2023 at 11:41:51AM +0200, Thierry Reding wrote: > > On Tue, Aug 08, 2023 at 07:19:01PM +0200, Uwe Kleine-König wrote: > > > struct pwm_chip::dev is about to change. To not have to touch this > > > driver in the same commit as struct pwm_chip::dev, store a pointer to > > > the parent device in driver data. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/pwm-ab8500.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > You're basically adding a parent device to all driver-private data > > structures in this and the following patches, so why not keep in in > > struct pwm_chip and simply rename chip->dev to chip->parent? > > > > As Andy has commented, this would eventually allow the PWM core to > > take care of certain things like runtime PM, or even only for stuff > > like using the parent device name in info/debug/error messages. > > > > Also, you could then just make this a single large patch that renames > > dev to parent in one go rather than making this large set even larger > > with this kind of trivial changes. > > The idea here is (again) that I don't have to touch all drivers in the > commit that changes struct pwm_chip. > > In the end there is such a parent pointer (pwmchip.dev.parent). Would > you prefer a macro (say) pwmchip_parentdev that can be defined as > > #define pwmchip_parentdev(chip) (chip)->dev > > at the beginning and then be changed to > > #define pwmchip_parentdev(chip) (chip)->dev.parent > > in the right moment? That's the best idea I have here. No, that's not necessarily better. So I think either we live with the duplicated parent pointer (which is the same whether we keep it in the core structure or the driver-private structure), or we just change all the occurrences at once when the new parent is introduced. I personally much prefer a single big patch with a lot of small one-line fixups over 30 patches that exist purely for the purpose of making patch 31 smaller. Thierry
Hello Thierry, On Fri, Oct 06, 2023 at 01:20:18PM +0200, Thierry Reding wrote: > On Fri, Oct 06, 2023 at 01:09:51PM +0200, Uwe Kleine-König wrote: > > On Fri, Oct 06, 2023 at 11:41:51AM +0200, Thierry Reding wrote: > > > Also, you could then just make this a single large patch that renames > > > dev to parent in one go rather than making this large set even larger > > > with this kind of trivial changes. > > > > The idea here is (again) that I don't have to touch all drivers in the > > commit that changes struct pwm_chip. > > > > In the end there is such a parent pointer (pwmchip.dev.parent). Would > > you prefer a macro (say) pwmchip_parentdev that can be defined as > > > > #define pwmchip_parentdev(chip) (chip)->dev > > > > at the beginning and then be changed to > > > > #define pwmchip_parentdev(chip) (chip)->dev.parent > > > > in the right moment? That's the best idea I have here. > > No, that's not necessarily better. So I think either we live with the > duplicated parent pointer (which is the same whether we keep it in the > core structure or the driver-private structure), or we just change all > the occurrences at once when the new parent is introduced. I personally > much prefer a single big patch with a lot of small one-line fixups over > 30 patches that exist purely for the purpose of making patch 31 smaller. I find that a surprising attitude. We're quite different here, I'd even ask a contributor who sends a such a change in a single patch to split it. I'll rework this part of the series to the above described macro, which opens the opportunity to squash it all into a single patch in the end without those (admittedly ugly) added extra pointers in driver private data. Best regards Uwe
diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c index f64f3fd251e7..663fdfe90bb6 100644 --- a/drivers/pwm/pwm-ab8500.c +++ b/drivers/pwm/pwm-ab8500.c @@ -24,6 +24,7 @@ #define AB8500_PWM_CLKRATE 9600000 struct ab8500_pwm_chip { + struct device *parent; unsigned int hwid; }; @@ -91,7 +92,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, * when disabled. */ if (!state->enabled || duty_steps == 0) { - ret = abx500_mask_and_set_register_interruptible(chip->dev, + ret = abx500_mask_and_set_register_interruptible(ab8500->parent, AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, 1 << ab8500->hwid, 0); @@ -111,18 +112,18 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, reg = AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2); - ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC, + ret = abx500_set_register_interruptible(ab8500->parent, AB8500_MISC, reg, lower_val); if (ret < 0) return ret; - ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC, + ret = abx500_set_register_interruptible(ab8500->parent, AB8500_MISC, (reg + 1), higher_val); if (ret < 0) return ret; /* enable */ - ret = abx500_mask_and_set_register_interruptible(chip->dev, + ret = abx500_mask_and_set_register_interruptible(ab8500->parent, AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, 1 << ab8500->hwid, 1 << ab8500->hwid); @@ -137,7 +138,7 @@ static int ab8500_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip); unsigned int div, duty_steps; - ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC, + ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, &ctrl7); if (ret) @@ -150,13 +151,13 @@ static int ab8500_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, return 0; } - ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC, + ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC, AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2), &lower_val); if (ret) return ret; - ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC, + ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC, AB8500_PWM_OUT_CTRL2_REG + (ab8500->hwid * 2), &higher_val); if (ret) @@ -196,6 +197,7 @@ static int ab8500_pwm_probe(struct platform_device *pdev) ab8500 = pwmchip_priv(chip); chip->ops = &ab8500_pwm_ops; + ab8500->parent = &pdev->dev; ab8500->hwid = pdev->id - 1; err = devm_pwmchip_add(&pdev->dev, chip);
struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, store a pointer to the parent device in driver data. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-ab8500.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)