| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Nov 2023 17:30:47 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Jerome Brunet <jbrunet@...libre.com>,
Thierry Reding <thierry.reding@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: Kevin Hilman <khilman@...libre.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org,
linux-pwm@...r.kernel.org, JunYi Zhao <junyi.zhao@...ogic.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH v3 4/4] pwm: meson: add generic compatible for meson8 to
sm1
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@...libre.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
Powered by blists - more mailing lists