| Message ID | 20240605-s4-pwm-v7-1-e822b271d7b0@amlogic.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Amlogic S4 PWM | expand |
Hello Kelvin, Junyi On 6/5/24 05:44, 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 | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index b2f97dfb01bb..4f01847525b2 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -460,6 +460,34 @@ 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)) { > + ret = PTR_ERR(meson->channels[i].clk); > + dev_err_probe(dev, ret, "Failed to get clk\n"); > + return ret; > + } > + devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, > + meson->channels[i].clk); devm_add_action_or_reset() result should be checked > + } > + > + return 0; > +} You can rewrite it a bit to always have a single allocation for devm node: static void meson_pwm_s4_put_clk(void *data) { struct meson_pwm *meson = data; int i; for (i = 0; i < MESON_NUM_PWMS; i++) clk_put(meson->channels[i].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; ret = devm_add_action(dev, meson_pwm_s4_put_clk, meson); if (ret) return 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)) { ret = PTR_ERR(meson->channels[i].clk); dev_err_probe(dev, ret, "Failed to get clk\n"); return ret; } } return 0; }
On Wed 05 Jun 2024 at 15:12, George Stark <gnstark@salutedevices.com> wrote: > Hello Kelvin, Junyi > > On 6/5/24 05:44, 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 | 36 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >> index b2f97dfb01bb..4f01847525b2 100644 >> --- a/drivers/pwm/pwm-meson.c >> +++ b/drivers/pwm/pwm-meson.c >> @@ -460,6 +460,34 @@ 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)) { >> + ret = PTR_ERR(meson->channels[i].clk); >> + dev_err_probe(dev, ret, "Failed to get clk\n"); >> + return ret; >> + } >> + devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, >> + meson->channels[i].clk); > > devm_add_action_or_reset() result should be checked > >> + } >> + >> + return 0; >> +} > > You can rewrite it a bit to always have a single allocation for devm node: > > static void meson_pwm_s4_put_clk(void *data) > { > struct meson_pwm *meson = data; > int i; > > for (i = 0; i < MESON_NUM_PWMS; i++) > clk_put(meson->channels[i].clk); > } This has already been discussed on v6. This make the code un-necessarily complex and potentially put clock that have not even been claimed. > > 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; > > ret = devm_add_action(dev, meson_pwm_s4_put_clk, meson); > if (ret) > return 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)) { > ret = PTR_ERR(meson->channels[i].clk); > dev_err_probe(dev, ret, "Failed to get clk\n"); > return ret; > } > } > > return 0; > }
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index b2f97dfb01bb..4f01847525b2 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -460,6 +460,34 @@ 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)) { + ret = PTR_ERR(meson->channels[i].clk); + dev_err_probe(dev, ret, "Failed to get clk\n"); + return ret; + } + devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, + meson->channels[i].clk); + } + + 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 +526,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 +568,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);