| Message ID | 20231129134004.3642121-5-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: meson: dt-bindings fixup | expand |
HI, On 29/11/2023 14:40, Jerome Brunet wrote: > Introduce a new compatible support in the Amlogic PWM driver. > > The PWM HW is actually the same for all SoCs supported so far. > A specific compatible is needed only because the clock sources > of the PWMs are hard-coded in the driver. > > It is better to have the clock source described in DT but this > changes the bindings so a new compatible must be introduced. > > When all supported platform have migrated to the new compatible, > support for the legacy ones may be removed from the driver. > > Adding a callback to setup the clock will also make it easier > to add support for the new PWM HW found in a1, s4, c3 and t7 SoC > families. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/pwm/pwm-meson.c | 224 ++++++++++++++++++++++++---------------- > 1 file changed, 133 insertions(+), 91 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 5cbd65cae28a..d5d745a651d3 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -95,6 +95,7 @@ struct meson_pwm_channel { > > struct meson_pwm_data { > const char * const *parent_names; > + int (*channels_init)(struct device *dev); > }; > > struct meson_pwm { > @@ -333,95 +334,6 @@ static const struct pwm_ops meson_pwm_ops = { > .get_state = meson_pwm_get_state, > }; > > -static const char * const pwm_meson8b_parent_names[] = { > - "xtal", NULL, "fclk_div4", "fclk_div3" > -}; > - > -static const struct meson_pwm_data pwm_meson8b_data = { > - .parent_names = pwm_meson8b_parent_names, > -}; > - > -/* > - * Only the 2 first inputs of the GXBB AO PWMs are valid > - * The last 2 are grounded > - */ > -static const char * const pwm_gxbb_ao_parent_names[] = { > - "xtal", "clk81", NULL, NULL, > -}; > - > -static const struct meson_pwm_data pwm_gxbb_ao_data = { > - .parent_names = pwm_gxbb_ao_parent_names, > -}; > - > -static const char * const pwm_axg_ee_parent_names[] = { > - "xtal", "fclk_div5", "fclk_div4", "fclk_div3" > -}; > - > -static const struct meson_pwm_data pwm_axg_ee_data = { > - .parent_names = pwm_axg_ee_parent_names, > -}; > - > -static const char * const pwm_axg_ao_parent_names[] = { > - "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5" > -}; > - > -static const struct meson_pwm_data pwm_axg_ao_data = { > - .parent_names = pwm_axg_ao_parent_names, > -}; > - > -static const char * const pwm_g12a_ao_ab_parent_names[] = { > - "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5" > -}; > - > -static const struct meson_pwm_data pwm_g12a_ao_ab_data = { > - .parent_names = pwm_g12a_ao_ab_parent_names, > -}; > - > -static const char * const pwm_g12a_ao_cd_parent_names[] = { > - "xtal", "g12a_ao_clk81", NULL, NULL, > -}; > - > -static const struct meson_pwm_data pwm_g12a_ao_cd_data = { > - .parent_names = pwm_g12a_ao_cd_parent_names, > -}; > - > -static const struct of_device_id meson_pwm_matches[] = { > - { > - .compatible = "amlogic,meson8b-pwm", > - .data = &pwm_meson8b_data > - }, > - { > - .compatible = "amlogic,meson-gxbb-pwm", > - .data = &pwm_meson8b_data > - }, > - { > - .compatible = "amlogic,meson-gxbb-ao-pwm", > - .data = &pwm_gxbb_ao_data > - }, > - { > - .compatible = "amlogic,meson-axg-ee-pwm", > - .data = &pwm_axg_ee_data > - }, > - { > - .compatible = "amlogic,meson-axg-ao-pwm", > - .data = &pwm_axg_ao_data > - }, > - { > - .compatible = "amlogic,meson-g12a-ee-pwm", > - .data = &pwm_meson8b_data > - }, > - { > - .compatible = "amlogic,meson-g12a-ao-pwm-ab", > - .data = &pwm_g12a_ao_ab_data > - }, > - { > - .compatible = "amlogic,meson-g12a-ao-pwm-cd", > - .data = &pwm_g12a_ao_cd_data > - }, > - {}, > -}; > -MODULE_DEVICE_TABLE(of, meson_pwm_matches); > - > static int meson_pwm_init_clocks_legacy(struct device *dev, > struct clk_parent_data *mux_parent_data) > { > @@ -528,12 +440,15 @@ static int meson_pwm_init_clocks_legacy(struct device *dev, > return 0; > } > > -static int meson_pwm_init_channels(struct device *dev) > +static int meson_pwm_init_channels_legacy(struct device *dev) > { > struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; > struct meson_pwm *meson = dev_get_drvdata(dev); > int i; > > + dev_info(dev, "using obsolete compatible, please consider updating dt\n"); I think dev_warn_once would be more appropriate > + > + > for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) { > mux_parent_data[i].index = -1; > mux_parent_data[i].name = meson->data->parent_names[i]; > @@ -542,6 +457,133 @@ static int meson_pwm_init_channels(struct device *dev) > return meson_pwm_init_clocks_legacy(dev, mux_parent_data); > } > > +static int meson_pwm_init_channels_meson8b_v2(struct device *dev) > +{ > + struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; > + int i; > + > + /* > + * NOTE: Instead of relying on the hard coded names in the driver > + * as the legacy version, this relies on DT to provide the list of > + * clocks. > + * For once, using input numbers actually makes more sense than names. > + * Also DT requires clock-names to be explicitly ordered, so there is > + * no point bothering with clock names in this case. > + */ > + for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) > + mux_parent_data[i].index = i; > + > + return meson_pwm_init_clocks_legacy(dev, mux_parent_data); > +} > + > +static const char * const pwm_meson8b_parent_names[] = { > + "xtal", NULL, "fclk_div4", "fclk_div3" > +}; > + > +static const struct meson_pwm_data pwm_meson8b_data = { > + .parent_names = pwm_meson8b_parent_names, > + .channels_init = meson_pwm_init_channels_legacy, > +}; > + > +/* > + * Only the 2 first inputs of the GXBB AO PWMs are valid > + * The last 2 are grounded > + */ > +static const char * const pwm_gxbb_ao_parent_names[] = { > + "xtal", "clk81", NULL, NULL, > +}; > + > +static const struct meson_pwm_data pwm_gxbb_ao_data = { > + .parent_names = pwm_gxbb_ao_parent_names, > + .channels_init = meson_pwm_init_channels_legacy, > +}; > + > +static const char * const pwm_axg_ee_parent_names[] = { > + "xtal", "fclk_div5", "fclk_div4", "fclk_div3" > +}; > + > +static const struct meson_pwm_data pwm_axg_ee_data = { > + .parent_names = pwm_axg_ee_parent_names, > + .channels_init = meson_pwm_init_channels_legacy, > +}; > + > +static const char * const pwm_axg_ao_parent_names[] = { > + "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5" > +}; > + > +static const struct meson_pwm_data pwm_axg_ao_data = { > + .parent_names = pwm_axg_ao_parent_names, > + .channels_init = meson_pwm_init_channels_legacy, > +}; > + > +static const char * const pwm_g12a_ao_ab_parent_names[] = { > + "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5" > +}; > + > +static const struct meson_pwm_data pwm_g12a_ao_ab_data = { > + .parent_names = pwm_g12a_ao_ab_parent_names, > + .channels_init = meson_pwm_init_channels_legacy, > +}; > + > +static const char * const pwm_g12a_ao_cd_parent_names[] = { > + "xtal", "g12a_ao_clk81", NULL, NULL, > +}; > + > +static const struct meson_pwm_data pwm_g12a_ao_cd_data = { > + .parent_names = pwm_g12a_ao_cd_parent_names, > + .channels_init = meson_pwm_init_channels_legacy, > +}; > + > +static const struct meson_pwm_data pwm_meson8_v2_data = { > + .channels_init = meson_pwm_init_channels_meson8b_v2, > +}; > + > +static const struct of_device_id meson_pwm_matches[] = { > + { > + .compatible = "amlogic,meson8-pwm-v2", > + .data = &pwm_meson8_v2_data > + }, > + /* > + * The following compatibles are obsolete. > + * Support for these may be removed once the related > + * platforms have been updated > + */ Not really, support will be needed until there's DT in the wild with the old bindings, which is likely forever. Drop the 2 last lines, only specify they are obsolete, and perhaps note support for legacy bindings will be kept as best effort but regressions may happen or something similar. > + { > + .compatible = "amlogic,meson8b-pwm", > + .data = &pwm_meson8b_data > + }, > + { > + .compatible = "amlogic,meson-gxbb-pwm", > + .data = &pwm_meson8b_data > + }, > + { > + .compatible = "amlogic,meson-gxbb-ao-pwm", > + .data = &pwm_gxbb_ao_data > + }, > + { > + .compatible = "amlogic,meson-axg-ee-pwm", > + .data = &pwm_axg_ee_data > + }, > + { > + .compatible = "amlogic,meson-axg-ao-pwm", > + .data = &pwm_axg_ao_data > + }, > + { > + .compatible = "amlogic,meson-g12a-ee-pwm", > + .data = &pwm_meson8b_data > + }, > + { > + .compatible = "amlogic,meson-g12a-ao-pwm-ab", > + .data = &pwm_g12a_ao_ab_data > + }, > + { > + .compatible = "amlogic,meson-g12a-ao-pwm-cd", > + .data = &pwm_g12a_ao_cd_data > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, meson_pwm_matches); > + > static int meson_pwm_probe(struct platform_device *pdev) > { > struct meson_pwm *meson; > @@ -573,7 +615,7 @@ static int meson_pwm_probe(struct platform_device *pdev) > return -ENODEV; > } > > - err = meson_pwm_init_channels(&pdev->dev); > + err = meson->data->channels_init(&pdev->dev); > if (err < 0) > return err; > Apart the dev_info change and the meson_pwm_init_clocks_legacy rename, it looks fine. Neil
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 5cbd65cae28a..d5d745a651d3 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -95,6 +95,7 @@ struct meson_pwm_channel { struct meson_pwm_data { const char * const *parent_names; + int (*channels_init)(struct device *dev); }; struct meson_pwm { @@ -333,95 +334,6 @@ static const struct pwm_ops meson_pwm_ops = { .get_state = meson_pwm_get_state, }; -static const char * const pwm_meson8b_parent_names[] = { - "xtal", NULL, "fclk_div4", "fclk_div3" -}; - -static const struct meson_pwm_data pwm_meson8b_data = { - .parent_names = pwm_meson8b_parent_names, -}; - -/* - * Only the 2 first inputs of the GXBB AO PWMs are valid - * The last 2 are grounded - */ -static const char * const pwm_gxbb_ao_parent_names[] = { - "xtal", "clk81", NULL, NULL, -}; - -static const struct meson_pwm_data pwm_gxbb_ao_data = { - .parent_names = pwm_gxbb_ao_parent_names, -}; - -static const char * const pwm_axg_ee_parent_names[] = { - "xtal", "fclk_div5", "fclk_div4", "fclk_div3" -}; - -static const struct meson_pwm_data pwm_axg_ee_data = { - .parent_names = pwm_axg_ee_parent_names, -}; - -static const char * const pwm_axg_ao_parent_names[] = { - "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5" -}; - -static const struct meson_pwm_data pwm_axg_ao_data = { - .parent_names = pwm_axg_ao_parent_names, -}; - -static const char * const pwm_g12a_ao_ab_parent_names[] = { - "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5" -}; - -static const struct meson_pwm_data pwm_g12a_ao_ab_data = { - .parent_names = pwm_g12a_ao_ab_parent_names, -}; - -static const char * const pwm_g12a_ao_cd_parent_names[] = { - "xtal", "g12a_ao_clk81", NULL, NULL, -}; - -static const struct meson_pwm_data pwm_g12a_ao_cd_data = { - .parent_names = pwm_g12a_ao_cd_parent_names, -}; - -static const struct of_device_id meson_pwm_matches[] = { - { - .compatible = "amlogic,meson8b-pwm", - .data = &pwm_meson8b_data - }, - { - .compatible = "amlogic,meson-gxbb-pwm", - .data = &pwm_meson8b_data - }, - { - .compatible = "amlogic,meson-gxbb-ao-pwm", - .data = &pwm_gxbb_ao_data - }, - { - .compatible = "amlogic,meson-axg-ee-pwm", - .data = &pwm_axg_ee_data - }, - { - .compatible = "amlogic,meson-axg-ao-pwm", - .data = &pwm_axg_ao_data - }, - { - .compatible = "amlogic,meson-g12a-ee-pwm", - .data = &pwm_meson8b_data - }, - { - .compatible = "amlogic,meson-g12a-ao-pwm-ab", - .data = &pwm_g12a_ao_ab_data - }, - { - .compatible = "amlogic,meson-g12a-ao-pwm-cd", - .data = &pwm_g12a_ao_cd_data - }, - {}, -}; -MODULE_DEVICE_TABLE(of, meson_pwm_matches); - static int meson_pwm_init_clocks_legacy(struct device *dev, struct clk_parent_data *mux_parent_data) { @@ -528,12 +440,15 @@ static int meson_pwm_init_clocks_legacy(struct device *dev, return 0; } -static int meson_pwm_init_channels(struct device *dev) +static int meson_pwm_init_channels_legacy(struct device *dev) { struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; struct meson_pwm *meson = dev_get_drvdata(dev); int i; + dev_info(dev, "using obsolete compatible, please consider updating dt\n"); + + for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) { mux_parent_data[i].index = -1; mux_parent_data[i].name = meson->data->parent_names[i]; @@ -542,6 +457,133 @@ static int meson_pwm_init_channels(struct device *dev) return meson_pwm_init_clocks_legacy(dev, mux_parent_data); } +static int meson_pwm_init_channels_meson8b_v2(struct device *dev) +{ + struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {}; + int i; + + /* + * NOTE: Instead of relying on the hard coded names in the driver + * as the legacy version, this relies on DT to provide the list of + * clocks. + * For once, using input numbers actually makes more sense than names. + * Also DT requires clock-names to be explicitly ordered, so there is + * no point bothering with clock names in this case. + */ + for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) + mux_parent_data[i].index = i; + + return meson_pwm_init_clocks_legacy(dev, mux_parent_data); +} + +static const char * const pwm_meson8b_parent_names[] = { + "xtal", NULL, "fclk_div4", "fclk_div3" +}; + +static const struct meson_pwm_data pwm_meson8b_data = { + .parent_names = pwm_meson8b_parent_names, + .channels_init = meson_pwm_init_channels_legacy, +}; + +/* + * Only the 2 first inputs of the GXBB AO PWMs are valid + * The last 2 are grounded + */ +static const char * const pwm_gxbb_ao_parent_names[] = { + "xtal", "clk81", NULL, NULL, +}; + +static const struct meson_pwm_data pwm_gxbb_ao_data = { + .parent_names = pwm_gxbb_ao_parent_names, + .channels_init = meson_pwm_init_channels_legacy, +}; + +static const char * const pwm_axg_ee_parent_names[] = { + "xtal", "fclk_div5", "fclk_div4", "fclk_div3" +}; + +static const struct meson_pwm_data pwm_axg_ee_data = { + .parent_names = pwm_axg_ee_parent_names, + .channels_init = meson_pwm_init_channels_legacy, +}; + +static const char * const pwm_axg_ao_parent_names[] = { + "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5" +}; + +static const struct meson_pwm_data pwm_axg_ao_data = { + .parent_names = pwm_axg_ao_parent_names, + .channels_init = meson_pwm_init_channels_legacy, +}; + +static const char * const pwm_g12a_ao_ab_parent_names[] = { + "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5" +}; + +static const struct meson_pwm_data pwm_g12a_ao_ab_data = { + .parent_names = pwm_g12a_ao_ab_parent_names, + .channels_init = meson_pwm_init_channels_legacy, +}; + +static const char * const pwm_g12a_ao_cd_parent_names[] = { + "xtal", "g12a_ao_clk81", NULL, NULL, +}; + +static const struct meson_pwm_data pwm_g12a_ao_cd_data = { + .parent_names = pwm_g12a_ao_cd_parent_names, + .channels_init = meson_pwm_init_channels_legacy, +}; + +static const struct meson_pwm_data pwm_meson8_v2_data = { + .channels_init = meson_pwm_init_channels_meson8b_v2, +}; + +static const struct of_device_id meson_pwm_matches[] = { + { + .compatible = "amlogic,meson8-pwm-v2", + .data = &pwm_meson8_v2_data + }, + /* + * The following compatibles are obsolete. + * Support for these may be removed once the related + * platforms have been updated + */ + { + .compatible = "amlogic,meson8b-pwm", + .data = &pwm_meson8b_data + }, + { + .compatible = "amlogic,meson-gxbb-pwm", + .data = &pwm_meson8b_data + }, + { + .compatible = "amlogic,meson-gxbb-ao-pwm", + .data = &pwm_gxbb_ao_data + }, + { + .compatible = "amlogic,meson-axg-ee-pwm", + .data = &pwm_axg_ee_data + }, + { + .compatible = "amlogic,meson-axg-ao-pwm", + .data = &pwm_axg_ao_data + }, + { + .compatible = "amlogic,meson-g12a-ee-pwm", + .data = &pwm_meson8b_data + }, + { + .compatible = "amlogic,meson-g12a-ao-pwm-ab", + .data = &pwm_g12a_ao_ab_data + }, + { + .compatible = "amlogic,meson-g12a-ao-pwm-cd", + .data = &pwm_g12a_ao_cd_data + }, + {}, +}; +MODULE_DEVICE_TABLE(of, meson_pwm_matches); + static int meson_pwm_probe(struct platform_device *pdev) { struct meson_pwm *meson; @@ -573,7 +615,7 @@ static int meson_pwm_probe(struct platform_device *pdev) return -ENODEV; } - err = meson_pwm_init_channels(&pdev->dev); + err = meson->data->channels_init(&pdev->dev); if (err < 0) return err;
Introduce a new compatible support in the Amlogic PWM driver. The PWM HW is actually the same for all SoCs supported so far. A specific compatible is needed only because the clock sources of the PWMs are hard-coded in the driver. It is better to have the clock source described in DT but this changes the bindings so a new compatible must be introduced. When all supported platform have migrated to the new compatible, support for the legacy ones may be removed from the driver. Adding a callback to setup the clock will also make it easier to add support for the new PWM HW found in a1, s4, c3 and t7 SoC families. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/pwm/pwm-meson.c | 224 ++++++++++++++++++++++++---------------- 1 file changed, 133 insertions(+), 91 deletions(-)