| Message ID | 20231222111658.832167-5-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: meson: dt-bindings fixup | expand |
On Fri, Dec 22, 2023 at 12:16:52PM +0100, Jerome Brunet wrote: > Use struct device data to carry the information data around, instead > of embedded the pwm structure in it and using container_of() > > Doing so works just as well and makes it a little easier to add setup > callback depending on the DT compatible. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index ef50c337f444..15c44185d784 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -101,7 +101,6 @@ struct meson_pwm_data { > }; > > struct meson_pwm { > - struct pwm_chip chip; > const struct meson_pwm_data *data; > struct meson_pwm_channel channels[MESON_NUM_PWMS]; > void __iomem *base; > @@ -114,7 +113,7 @@ struct meson_pwm { > > static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip) > { > - return container_of(chip, struct meson_pwm, chip); > + return dev_get_drvdata(chip->dev); > } > > static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > @@ -146,6 +145,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; > + struct device *dev = pwm->chip->dev; > unsigned int cnt, duty_cnt; > unsigned long fin_freq; > u64 duty, period, freq; > @@ -168,19 +168,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > > fin_freq = clk_round_rate(channel->clk, freq); > if (fin_freq == 0) { > - dev_err(meson->chip.dev, "invalid source clock frequency\n"); > + dev_err(dev, "invalid source clock frequency\n"); > return -EINVAL; > } > > - dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); > + dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq); > > cnt = div_u64(fin_freq * period, NSEC_PER_SEC); > if (cnt > 0xffff) { > - dev_err(meson->chip.dev, "unable to get period cnt\n"); > + dev_err(dev, "unable to get period cnt\n"); > return -EINVAL; > } > > - dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt); > + dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt); > > if (duty == period) { > channel->hi = cnt; > @@ -191,7 +191,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > } else { > duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC); > > - dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt); > + dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt); > > channel->hi = duty_cnt; > channel->lo = cnt - duty_cnt; > @@ -214,7 +214,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) > > err = clk_set_rate(channel->clk, channel->rate); > if (err) > - dev_err(meson->chip.dev, "setting clock rate failed\n"); > + dev_err(pwm->chip->dev, "setting clock rate failed\n"); > > spin_lock_irqsave(&meson->lock, flags); > > @@ -425,10 +425,10 @@ static const struct of_device_id meson_pwm_matches[] = { > }; > MODULE_DEVICE_TABLE(of, meson_pwm_matches); > > -static int meson_pwm_init_channels(struct meson_pwm *meson) > +static int meson_pwm_init_channels(struct device *dev) > { > struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; > - struct device *dev = meson->chip.dev; > + struct meson_pwm *meson = dev_get_drvdata(dev); > unsigned int i; > char name[255]; > int err; > @@ -438,7 +438,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > mux_parent_data[i].name = meson->data->parent_names[i]; > } > > - for (i = 0; i < meson->chip.npwm; i++) { > + for (i = 0; i < MESON_NUM_PWMS; i++) { > struct meson_pwm_channel *channel = &meson->channels[i]; > struct clk_parent_data div_parent = {}, gate_parent = {}; > struct clk_init_data init = {}; > @@ -519,28 +519,35 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > static int meson_pwm_probe(struct platform_device *pdev) > { > struct meson_pwm *meson; > + struct pwm_chip *chip; > int err; > > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL); > if (!meson) > return -ENOMEM; > > + platform_set_drvdata(pdev, meson); > + > meson->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(meson->base)) > return PTR_ERR(meson->base); > > spin_lock_init(&meson->lock); > - meson->chip.dev = &pdev->dev; > - meson->chip.ops = &meson_pwm_ops; > - meson->chip.npwm = MESON_NUM_PWMS; > + chip->dev = &pdev->dev; > + chip->ops = &meson_pwm_ops; > + chip->npwm = MESON_NUM_PWMS; > > meson->data = of_device_get_match_data(&pdev->dev); > > - err = meson_pwm_init_channels(meson); > + err = meson_pwm_init_channels(&pdev->dev); > if (err < 0) > return err; > > - err = devm_pwmchip_add(&pdev->dev, &meson->chip); > + err = devm_pwmchip_add(&pdev->dev, chip); > if (err < 0) > return dev_err_probe(&pdev->dev, err, > "failed to register PWM chip\n"); Parts of this change overlap with plans I have for this driver. I reworked the series a bit now, also affecting the meson driver, the previous submission is available at https://lore.kernel.org/linux-pwm/bf6f7c6253041f60ee8f35b5c9c9e8d595332fb0.1706182805.git.u.kleine-koenig@pengutronix.de I don't see the nice benefit of this patch yet, but I assume this will become clearer when I check the next patch. Best regards Uwe
On Mon 05 Feb 2024 at 18:12, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > [[PGP Signed Part:Undecided]] > On Fri, Dec 22, 2023 at 12:16:52PM +0100, Jerome Brunet wrote: >> Use struct device data to carry the information data around, instead >> of embedded the pwm structure in it and using container_of() >> >> Doing so works just as well and makes it a little easier to add setup >> callback depending on the DT compatible. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++---------------- >> 1 file changed, 23 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >> index ef50c337f444..15c44185d784 100644 >> --- a/drivers/pwm/pwm-meson.c >> +++ b/drivers/pwm/pwm-meson.c >> @@ -101,7 +101,6 @@ struct meson_pwm_data { >> }; >> >> struct meson_pwm { >> - struct pwm_chip chip; >> const struct meson_pwm_data *data; >> struct meson_pwm_channel channels[MESON_NUM_PWMS]; >> void __iomem *base; >> @@ -114,7 +113,7 @@ struct meson_pwm { >> >> static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip) >> { >> - return container_of(chip, struct meson_pwm, chip); >> + return dev_get_drvdata(chip->dev); >> } >> >> static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) >> @@ -146,6 +145,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> const struct pwm_state *state) >> { >> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >> + struct device *dev = pwm->chip->dev; >> unsigned int cnt, duty_cnt; >> unsigned long fin_freq; >> u64 duty, period, freq; >> @@ -168,19 +168,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> >> fin_freq = clk_round_rate(channel->clk, freq); >> if (fin_freq == 0) { >> - dev_err(meson->chip.dev, "invalid source clock frequency\n"); >> + dev_err(dev, "invalid source clock frequency\n"); >> return -EINVAL; >> } >> >> - dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >> + dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq); >> >> cnt = div_u64(fin_freq * period, NSEC_PER_SEC); >> if (cnt > 0xffff) { >> - dev_err(meson->chip.dev, "unable to get period cnt\n"); >> + dev_err(dev, "unable to get period cnt\n"); >> return -EINVAL; >> } >> >> - dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt); >> + dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt); >> >> if (duty == period) { >> channel->hi = cnt; >> @@ -191,7 +191,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> } else { >> duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC); >> >> - dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt); >> + dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt); >> >> channel->hi = duty_cnt; >> channel->lo = cnt - duty_cnt; >> @@ -214,7 +214,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) >> >> err = clk_set_rate(channel->clk, channel->rate); >> if (err) >> - dev_err(meson->chip.dev, "setting clock rate failed\n"); >> + dev_err(pwm->chip->dev, "setting clock rate failed\n"); >> >> spin_lock_irqsave(&meson->lock, flags); >> >> @@ -425,10 +425,10 @@ static const struct of_device_id meson_pwm_matches[] = { >> }; >> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >> >> -static int meson_pwm_init_channels(struct meson_pwm *meson) >> +static int meson_pwm_init_channels(struct device *dev) >> { >> struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; >> - struct device *dev = meson->chip.dev; >> + struct meson_pwm *meson = dev_get_drvdata(dev); >> unsigned int i; >> char name[255]; >> int err; >> @@ -438,7 +438,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >> mux_parent_data[i].name = meson->data->parent_names[i]; >> } >> >> - for (i = 0; i < meson->chip.npwm; i++) { >> + for (i = 0; i < MESON_NUM_PWMS; i++) { >> struct meson_pwm_channel *channel = &meson->channels[i]; >> struct clk_parent_data div_parent = {}, gate_parent = {}; >> struct clk_init_data init = {}; >> @@ -519,28 +519,35 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >> static int meson_pwm_probe(struct platform_device *pdev) >> { >> struct meson_pwm *meson; >> + struct pwm_chip *chip; >> int err; >> >> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); >> + if (!chip) >> + return -ENOMEM; >> + >> meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL); >> if (!meson) >> return -ENOMEM; >> >> + platform_set_drvdata(pdev, meson); >> + >> meson->base = devm_platform_ioremap_resource(pdev, 0); >> if (IS_ERR(meson->base)) >> return PTR_ERR(meson->base); >> >> spin_lock_init(&meson->lock); >> - meson->chip.dev = &pdev->dev; >> - meson->chip.ops = &meson_pwm_ops; >> - meson->chip.npwm = MESON_NUM_PWMS; >> + chip->dev = &pdev->dev; >> + chip->ops = &meson_pwm_ops; >> + chip->npwm = MESON_NUM_PWMS; >> >> meson->data = of_device_get_match_data(&pdev->dev); >> >> - err = meson_pwm_init_channels(meson); >> + err = meson_pwm_init_channels(&pdev->dev); >> if (err < 0) >> return err; >> >> - err = devm_pwmchip_add(&pdev->dev, &meson->chip); >> + err = devm_pwmchip_add(&pdev->dev, chip); >> if (err < 0) >> return dev_err_probe(&pdev->dev, err, >> "failed to register PWM chip\n"); > > Parts of this change overlap with plans I have for this driver. I It does overlap indeed. I'll drop this one while rebasing > reworked the series a bit now, also affecting the meson driver, the > previous submission is available at > https://lore.kernel.org/linux-pwm/bf6f7c6253041f60ee8f35b5c9c9e8d595332fb0.1706182805.git.u.kleine-koenig@pengutronix.de > > I don't see the nice benefit of this patch yet, but I assume this will > become clearer when I check the next patch. > > Best regards > Uwe
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index ef50c337f444..15c44185d784 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -101,7 +101,6 @@ struct meson_pwm_data { }; struct meson_pwm { - struct pwm_chip chip; const struct meson_pwm_data *data; struct meson_pwm_channel channels[MESON_NUM_PWMS]; void __iomem *base; @@ -114,7 +113,7 @@ struct meson_pwm { static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip) { - return container_of(chip, struct meson_pwm, chip); + return dev_get_drvdata(chip->dev); } static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) @@ -146,6 +145,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, const struct pwm_state *state) { struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; + struct device *dev = pwm->chip->dev; unsigned int cnt, duty_cnt; unsigned long fin_freq; u64 duty, period, freq; @@ -168,19 +168,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, fin_freq = clk_round_rate(channel->clk, freq); if (fin_freq == 0) { - dev_err(meson->chip.dev, "invalid source clock frequency\n"); + dev_err(dev, "invalid source clock frequency\n"); return -EINVAL; } - dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); + dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq); cnt = div_u64(fin_freq * period, NSEC_PER_SEC); if (cnt > 0xffff) { - dev_err(meson->chip.dev, "unable to get period cnt\n"); + dev_err(dev, "unable to get period cnt\n"); return -EINVAL; } - dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt); + dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt); if (duty == period) { channel->hi = cnt; @@ -191,7 +191,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, } else { duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC); - dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt); + dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt); channel->hi = duty_cnt; channel->lo = cnt - duty_cnt; @@ -214,7 +214,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) err = clk_set_rate(channel->clk, channel->rate); if (err) - dev_err(meson->chip.dev, "setting clock rate failed\n"); + dev_err(pwm->chip->dev, "setting clock rate failed\n"); spin_lock_irqsave(&meson->lock, flags); @@ -425,10 +425,10 @@ static const struct of_device_id meson_pwm_matches[] = { }; MODULE_DEVICE_TABLE(of, meson_pwm_matches); -static int meson_pwm_init_channels(struct meson_pwm *meson) +static int meson_pwm_init_channels(struct device *dev) { struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; - struct device *dev = meson->chip.dev; + struct meson_pwm *meson = dev_get_drvdata(dev); unsigned int i; char name[255]; int err; @@ -438,7 +438,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) mux_parent_data[i].name = meson->data->parent_names[i]; } - for (i = 0; i < meson->chip.npwm; i++) { + for (i = 0; i < MESON_NUM_PWMS; i++) { struct meson_pwm_channel *channel = &meson->channels[i]; struct clk_parent_data div_parent = {}, gate_parent = {}; struct clk_init_data init = {}; @@ -519,28 +519,35 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) static int meson_pwm_probe(struct platform_device *pdev) { struct meson_pwm *meson; + struct pwm_chip *chip; int err; + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL); if (!meson) return -ENOMEM; + platform_set_drvdata(pdev, meson); + meson->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(meson->base)) return PTR_ERR(meson->base); spin_lock_init(&meson->lock); - meson->chip.dev = &pdev->dev; - meson->chip.ops = &meson_pwm_ops; - meson->chip.npwm = MESON_NUM_PWMS; + chip->dev = &pdev->dev; + chip->ops = &meson_pwm_ops; + chip->npwm = MESON_NUM_PWMS; meson->data = of_device_get_match_data(&pdev->dev); - err = meson_pwm_init_channels(meson); + err = meson_pwm_init_channels(&pdev->dev); if (err < 0) return err; - err = devm_pwmchip_add(&pdev->dev, &meson->chip); + err = devm_pwmchip_add(&pdev->dev, chip); if (err < 0) return dev_err_probe(&pdev->dev, err, "failed to register PWM chip\n");
Use struct device data to carry the information data around, instead of embedded the pwm structure in it and using container_of() Doing so works just as well and makes it a little easier to add setup callback depending on the DT compatible. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)