| Message ID | 20240613-s4-pwm-v8-1-b5bd0a768282@amlogic.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | Add support for Amlogic S4 PWM | expand |
If there's another series you can fix log messages and start it from low-case letter as in the rest of the file. Either way Reviewed-by: George Stark <gnstark@salutedevices.com> On 6/13/24 14:46, Kelvin Zhang via B4 Relay wrote: > From: Junyi Zhao <junyi.zhao@amlogic.com> > > Add support for Amlogic S4 PWM. > > Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com> > Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com> > --- > drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index b2f97dfb01bb..98e6c1533312 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -460,6 +460,37 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip) > return meson_pwm_init_clocks_meson8b(chip, mux_parent_data); > } > > +static void meson_pwm_s4_put_clk(void *data) > +{ > + struct clk *clk = data; > + > + clk_put(clk); > +} > + > +static int meson_pwm_init_channels_s4(struct pwm_chip *chip) > +{ > + struct device *dev = pwmchip_parent(chip); > + struct device_node *np = dev->of_node; > + struct meson_pwm *meson = to_meson_pwm(chip); > + int i, ret; > + > + for (i = 0; i < MESON_NUM_PWMS; i++) { > + meson->channels[i].clk = of_clk_get(np, i); > + if (IS_ERR(meson->channels[i].clk)) > + return dev_err_probe(dev, > + PTR_ERR(meson->channels[i].clk), > + "Failed to get clk\n"); > + > + ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, > + meson->channels[i].clk); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to add clk_put action\n"); > + } > + > + return 0; > +} > + > static const struct meson_pwm_data pwm_meson8b_data = { > .parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" }, > .channels_init = meson_pwm_init_channels_meson8b_legacy, > @@ -498,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = { > .channels_init = meson_pwm_init_channels_meson8b_v2, > }; > > +static const struct meson_pwm_data pwm_s4_data = { > + .channels_init = meson_pwm_init_channels_s4, > +}; > + > static const struct of_device_id meson_pwm_matches[] = { > { > .compatible = "amlogic,meson8-pwm-v2", > @@ -536,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = { > .compatible = "amlogic,meson-g12a-ao-pwm-cd", > .data = &pwm_g12a_ao_cd_data > }, > + { > + .compatible = "amlogic,meson-s4-pwm", > + .data = &pwm_s4_data > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, meson_pwm_matches); >
On Thu 13 Jun 2024 at 19:46, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote: > From: Junyi Zhao <junyi.zhao@amlogic.com> > > Add support for Amlogic S4 PWM. > > Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com> > Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com> > --- > drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index b2f97dfb01bb..98e6c1533312 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -460,6 +460,37 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip) > return meson_pwm_init_clocks_meson8b(chip, mux_parent_data); > } > > +static void meson_pwm_s4_put_clk(void *data) > +{ > + struct clk *clk = data; > + > + clk_put(clk); > +} > + > +static int meson_pwm_init_channels_s4(struct pwm_chip *chip) > +{ > + struct device *dev = pwmchip_parent(chip); > + struct device_node *np = dev->of_node; > + struct meson_pwm *meson = to_meson_pwm(chip); > + int i, ret; > + > + for (i = 0; i < MESON_NUM_PWMS; i++) { > + meson->channels[i].clk = of_clk_get(np, i); > + if (IS_ERR(meson->channels[i].clk)) > + return dev_err_probe(dev, > + PTR_ERR(meson->channels[i].clk), > + "Failed to get clk\n"); here it is ok but .. > + > + ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, > + meson->channels[i].clk); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to add clk_put action\n"); ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful. dev_err() if you must print something. Just "return ret;" would be fine by me > + } > + > + return 0; > +} > + > static const struct meson_pwm_data pwm_meson8b_data = { > .parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" }, > .channels_init = meson_pwm_init_channels_meson8b_legacy, > @@ -498,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = { > .channels_init = meson_pwm_init_channels_meson8b_v2, > }; > > +static const struct meson_pwm_data pwm_s4_data = { > + .channels_init = meson_pwm_init_channels_s4, > +}; > + > static const struct of_device_id meson_pwm_matches[] = { > { > .compatible = "amlogic,meson8-pwm-v2", > @@ -536,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = { > .compatible = "amlogic,meson-g12a-ao-pwm-cd", > .data = &pwm_g12a_ao_cd_data > }, > + { > + .compatible = "amlogic,meson-s4-pwm", > + .data = &pwm_s4_data > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, meson_pwm_matches);
Hello, On Thu, Jun 13, 2024 at 05:54:31PM +0200, Jerome Brunet wrote: > > + for (i = 0; i < MESON_NUM_PWMS; i++) { > > + meson->channels[i].clk = of_clk_get(np, i); > > + if (IS_ERR(meson->channels[i].clk)) > > + return dev_err_probe(dev, > > + PTR_ERR(meson->channels[i].clk), > > + "Failed to get clk\n"); > > here it is ok but .. > > > + > > + ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, > > + meson->channels[i].clk); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to add clk_put action\n"); > > ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful. > dev_err() if you must print something. Just "return ret;" would be fine > by me I don't agree. dev_err_probe() has several purposes. Only one of them is handling -EPROBE_DEFER. See also commit 532888a59505da2a3fbb4abac6adad381cedb374. So yes, please use dev_err_probe() also to handle devm_add_action_or_reset(). Best regards Uwe
On Thu 13 Jun 2024 at 22:46, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > Hello, > > On Thu, Jun 13, 2024 at 05:54:31PM +0200, Jerome Brunet wrote: >> > + for (i = 0; i < MESON_NUM_PWMS; i++) { >> > + meson->channels[i].clk = of_clk_get(np, i); >> > + if (IS_ERR(meson->channels[i].clk)) >> > + return dev_err_probe(dev, >> > + PTR_ERR(meson->channels[i].clk), >> > + "Failed to get clk\n"); >> >> here it is ok but .. >> >> > + >> > + ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, >> > + meson->channels[i].clk); >> > + if (ret) >> > + return dev_err_probe(dev, ret, >> > + "Failed to add clk_put action\n"); >> >> ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful. >> dev_err() if you must print something. Just "return ret;" would be fine >> by me > > I don't agree. dev_err_probe() has several purposes. Only one of them is > handling -EPROBE_DEFER. See also commit > 532888a59505da2a3fbb4abac6adad381cedb374. I was stuck on the -EPROBE_DEFER usage. Thanks for the heads up > > So yes, please use dev_err_probe() also to handle > devm_add_action_or_reset(). My point here is also that devm_add_action_or_reset() can only fail on memory allocation, like (devm_)kzalloc. Looking around the kernel, we tend to not add messages for that and just return the error code, presumably because those same 'out of memory' messages would proliferate everywhere. > > Best regards > Uwe
Hello Jerôme, On Fri, Jun 14, 2024 at 09:24:28AM +0200, Jerome Brunet wrote: > My point here is also that devm_add_action_or_reset() can only fail on > memory allocation, like (devm_)kzalloc. Looking around the kernel, we > tend to not add messages for that and just return the error code, > presumably because those same 'out of memory' messages would proliferate > everywhere. I took your first message in this thread as opportunity to resend a patch improving the situation here. See https://lore.kernel.org/lkml/3d1e308d45cddf67749522ca42d83f5b4f0b9634.1718311756.git.u.kleine-koenig@baylibre.com/ Best regards Uwe
On 2024/6/14 15:24, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Thu 13 Jun 2024 at 22:46, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > >> Hello, >> >> On Thu, Jun 13, 2024 at 05:54:31PM +0200, Jerome Brunet wrote: >>>> + for (i = 0; i < MESON_NUM_PWMS; i++) { >>>> + meson->channels[i].clk = of_clk_get(np, i); >>>> + if (IS_ERR(meson->channels[i].clk)) >>>> + return dev_err_probe(dev, >>>> + PTR_ERR(meson->channels[i].clk), >>>> + "Failed to get clk\n"); >>> >>> here it is ok but .. >>> >>>> + >>>> + ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, >>>> + meson->channels[i].clk); >>>> + if (ret) >>>> + return dev_err_probe(dev, ret, >>>> + "Failed to add clk_put action\n"); >>> >>> ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful. >>> dev_err() if you must print something. Just "return ret;" would be fine >>> by me >> >> I don't agree. dev_err_probe() has several purposes. Only one of them is >> handling -EPROBE_DEFER. See also commit >> 532888a59505da2a3fbb4abac6adad381cedb374. > > I was stuck on the -EPROBE_DEFER usage. Thanks for the heads up > >> >> So yes, please use dev_err_probe() also to handle >> devm_add_action_or_reset(). > > My point here is also that devm_add_action_or_reset() can only fail on > memory allocation, like (devm_)kzalloc. Looking around the kernel, we > tend to not add messages for that and just return the error code, > presumably because those same 'out of memory' messages would proliferate > everywhere. > >> >> Best regards >> Uwe > > -- > Jerome Hi Uwe, I didnt get the clear point. So, if we need "return ret" directly? or keep "dev_err_probe()" to print? -- Best regards Junyi
Hello, On Mon, Jun 17, 2024 at 04:44:13PM +0800, Junyi Zhao wrote: > > > So yes, please use dev_err_probe() also to handle > > > devm_add_action_or_reset(). > > > > My point here is also that devm_add_action_or_reset() can only fail on > > memory allocation, like (devm_)kzalloc. Looking around the kernel, we > > tend to not add messages for that and just return the error code, > > presumably because those same 'out of memory' messages would proliferate > > everywhere. > > Hi Uwe, I didnt get the clear point. > So, if we need "return ret" directly? or keep "dev_err_probe()" to print? Please keep the dev_err_probe(). There is a problem with that approach (as Jerome pointed out), but that is about to be addressed in driver core code. Best regards Uwe
On 2024/6/17 22:11, Uwe Kleine-König wrote: > Hello, > > On Mon, Jun 17, 2024 at 04:44:13PM +0800, Junyi Zhao wrote: >>>> So yes, please use dev_err_probe() also to handle >>>> devm_add_action_or_reset(). >>> My point here is also that devm_add_action_or_reset() can only fail on >>> memory allocation, like (devm_)kzalloc. Looking around the kernel, we >>> tend to not add messages for that and just return the error code, >>> presumably because those same 'out of memory' messages would proliferate >>> everywhere. >> Hi Uwe, I didnt get the clear point. >> So, if we need "return ret" directly? or keep "dev_err_probe()" to print? > Please keep the dev_err_probe(). There is a problem with that approach > (as Jerome pointed out), but that is about to be addressed in driver > core code. > Hi Uwe, For this patchset, is there anything that needs improvement? Thanks! > Best regards > Uwe
Hello Kelvin, On Tue, Jun 25, 2024 at 05:33:22PM +0800, Kelvin Zhang wrote: > On 2024/6/17 22:11, Uwe Kleine-König wrote: > > On Mon, Jun 17, 2024 at 04:44:13PM +0800, Junyi Zhao wrote: > > > > > So yes, please use dev_err_probe() also to handle > > > > > devm_add_action_or_reset(). > > > > My point here is also that devm_add_action_or_reset() can only fail on > > > > memory allocation, like (devm_)kzalloc. Looking around the kernel, we > > > > tend to not add messages for that and just return the error code, > > > > presumably because those same 'out of memory' messages would proliferate > > > > everywhere. > > > Hi Uwe, I didnt get the clear point. > > > So, if we need "return ret" directly? or keep "dev_err_probe()" to print? > > Please keep the dev_err_probe(). There is a problem with that approach > > (as Jerome pointed out), but that is about to be addressed in driver > > core code. FTR, this is done in https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=2f3cfd2f4b7cf3026fe6b9b2a5320cc18f4c184e > For this patchset, is there anything that needs improvement? It's on my (not particularily short) todo list to review this patch. I'm aware there are several people waiting for that one. I intend to work on this one later this week. Best regards Uwe
Hello, On Thu, Jun 13, 2024 at 07:46:35PM +0800, Kelvin Zhang via B4 Relay wrote: > From: Junyi Zhao <junyi.zhao@amlogic.com> > > Add support for Amlogic S4 PWM. > > Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com> > Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com> Applied this patch with George Stark's Reviewed-by: to https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next . You signed your patch using gpg, that's fine. However I failed to find your key to verify that signature. I suggest you to upload your key to https://keys.openpgp.org/ (note you have to register your email addresses there to make this useful) and read through https://korg.docs.kernel.org/pgpkeys.html#submitting-keys-to-the-keyring . Best regards Uwe
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index b2f97dfb01bb..98e6c1533312 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -460,6 +460,37 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip) return meson_pwm_init_clocks_meson8b(chip, mux_parent_data); } +static void meson_pwm_s4_put_clk(void *data) +{ + struct clk *clk = data; + + clk_put(clk); +} + +static int meson_pwm_init_channels_s4(struct pwm_chip *chip) +{ + struct device *dev = pwmchip_parent(chip); + struct device_node *np = dev->of_node; + struct meson_pwm *meson = to_meson_pwm(chip); + int i, ret; + + for (i = 0; i < MESON_NUM_PWMS; i++) { + meson->channels[i].clk = of_clk_get(np, i); + if (IS_ERR(meson->channels[i].clk)) + return dev_err_probe(dev, + PTR_ERR(meson->channels[i].clk), + "Failed to get clk\n"); + + ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, + meson->channels[i].clk); + if (ret) + return dev_err_probe(dev, ret, + "Failed to add clk_put action\n"); + } + + return 0; +} + static const struct meson_pwm_data pwm_meson8b_data = { .parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" }, .channels_init = meson_pwm_init_channels_meson8b_legacy, @@ -498,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = { .channels_init = meson_pwm_init_channels_meson8b_v2, }; +static const struct meson_pwm_data pwm_s4_data = { + .channels_init = meson_pwm_init_channels_s4, +}; + static const struct of_device_id meson_pwm_matches[] = { { .compatible = "amlogic,meson8-pwm-v2", @@ -536,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = { .compatible = "amlogic,meson-g12a-ao-pwm-cd", .data = &pwm_g12a_ao_cd_data }, + { + .compatible = "amlogic,meson-s4-pwm", + .data = &pwm_s4_data + }, {}, }; MODULE_DEVICE_TABLE(of, meson_pwm_matches);