Messages in this thread | | | From | Jerome Brunet <> | Subject | Re: [PATCH 2/2] pwm: meson: support meson A1 SoC family | Date | Tue, 23 Apr 2024 19:35:15 +0200 |
| |
On Tue 23 Apr 2024 at 19:10, George Stark <gnstark@salutedevices.com> wrote:
> From: George Stark <gnstark@sberdevices.ru> > > Add a compatible string and configuration for the meson A1 SoC family > PWM. Additionally, provide an external clock initialization helper > specifically designed for these PWM IPs. > > Signed-off-by: George Stark <gnstark@sberdevices.ru> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> > --- > drivers/pwm/pwm-meson.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index ea96c5973488..529a541ba7b6 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -462,6 +462,33 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip) > return meson_pwm_init_clocks_meson8b(chip, mux_parent_data); > } > > +static int meson_pwm_init_channels_ext_clock(struct pwm_chip *chip)
That kind on naming (ext) is almost sure to clash with whatever comes next. Just use the name of the first SoC using the method, a1 for instance.
> +{ > + struct device *dev = pwmchip_parent(chip); > + struct meson_pwm *meson = to_meson_pwm(chip); > + struct meson_pwm_channel *channels = meson->channels; > + struct clk_bulk_data *clks = NULL; > + unsigned int i; > + int res; > + > + res = devm_clk_bulk_get_all(dev, &clks); > + if (res < 0) { > + dev_err(dev, "can't get device clocks\n"); > + return res; > + }
I don't think allocating the 'clk_bulk_data *clks' is necessary or safe. We know exactly how many clocks we expect, there is no need for a get all.
> + > + if (res != MESON_NUM_PWMS) { > + dev_err(dev, "clock count must be %d, got %d\n", > + MESON_NUM_PWMS, res); > + return -EINVAL; > + }
.. and this only catches the problem after the fact.
It is probably convinient but not necessary.
> + > + for (i = 0; i < MESON_NUM_PWMS; i++) > + channels[i].clk = clks[i].clk;
channels[i].clk could be assigned directly of_clk_get() using clock indexes. No extra allocation needed.
> + > + 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, > @@ -500,11 +527,19 @@ 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_meson_ext_clock_data = { > + .channels_init = meson_pwm_init_channels_ext_clock, > +}; > + > static const struct of_device_id meson_pwm_matches[] = { > { > .compatible = "amlogic,meson8-pwm-v2", > .data = &pwm_meson8_v2_data > }, > + { > + .compatible = "amlogic,meson-a1-pwm", > + .data = &pwm_meson_ext_clock_data > + }, > /* The following compatibles are obsolete */ > { > .compatible = "amlogic,meson8b-pwm",
-- Jerome
| |