| 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
| ||
|
Message-ID: <0ed0e573-08c9-4216-ad4c-66cc6563e036@linaro.org>
Date: Wed, 29 Nov 2023 17:25:44 +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 3/4] pwm: meson: prepare addition of new compatible
types
Hi,
On 29/11/2023 14:39, Jerome Brunet wrote:
> Clean the amlogic pwm driver to prepare the addition of new pwm compatibles
> * Generalize 4 inputs clock per channel.
> AO pwm may just get 2 extra NULL entries which actually better
> describes the reality of the HW.
> * Use driver data to carry the device data and remove pwm_chip from it
> * Stop carrying the internal clock elements with the device data.
> These are not needed past init.
>
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---
> drivers/pwm/pwm-meson.c | 150 +++++++++++++++++++++++-----------------
> 1 file changed, 87 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 5bea53243ed2..5cbd65cae28a 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -60,7 +60,7 @@
> #define MISC_A_EN BIT(0)
>
> #define MESON_NUM_PWMS 2
> -#define MESON_MAX_MUX_PARENTS 4
> +#define MESON_NUM_MUX_PARENTS 4
>
> static struct meson_pwm_channel_data {
> u8 reg_offset;
> @@ -90,19 +90,14 @@ struct meson_pwm_channel {
> unsigned int hi;
> unsigned int lo;
>
> - struct clk_mux mux;
> - struct clk_divider div;
> - struct clk_gate gate;
> struct clk *clk;
> };
>
> struct meson_pwm_data {
> const char * const *parent_names;
> - unsigned int num_parents;
> };
>
> struct meson_pwm {
> - struct pwm_chip chip;
It seems you're mixing multiple things in the same patch,
you should split it to implement the pwm_chip migration,
and parent_names/clk init in 2 patches.
> const struct meson_pwm_data *data;
> struct meson_pwm_channel channels[MESON_NUM_PWMS];
> void __iomem *base;
> @@ -115,7 +110,7 @@ struct meson_pwm {
>
> static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
> {
> - return container_of(chip, struct meson_pwm, chip);
> + return dev_get_drvdata(chip->dev);
> }
>
> static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -147,6 +142,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
> + struct device *dev = pwm->chip->dev;
> unsigned int cnt, duty_cnt;
> unsigned long fin_freq;
> u64 duty, period, freq;
> @@ -169,19 +165,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>
> fin_freq = clk_round_rate(channel->clk, freq);
> if (fin_freq == 0) {
> - dev_err(meson->chip.dev, "invalid source clock frequency\n");
> + dev_err(dev, "invalid source clock frequency\n");
> return -EINVAL;
> }
>
> - dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
> + dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq);
>
> cnt = div_u64(fin_freq * period, NSEC_PER_SEC);
> if (cnt > 0xffff) {
> - dev_err(meson->chip.dev, "unable to get period cnt\n");
> + dev_err(dev, "unable to get period cnt\n");
> return -EINVAL;
> }
>
> - dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt);
> + dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt);
>
> if (duty == period) {
> channel->hi = cnt;
> @@ -192,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> } else {
> duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC);
>
> - dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
> + dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
>
> channel->hi = duty_cnt;
> channel->lo = cnt - duty_cnt;
> @@ -215,7 +211,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>
> err = clk_set_rate(channel->clk, channel->rate);
> if (err)
> - dev_err(meson->chip.dev, "setting clock rate failed\n");
> + dev_err(pwm->chip->dev, "setting clock rate failed\n");
>
> spin_lock_irqsave(&meson->lock, flags);
>
> @@ -343,7 +339,6 @@ static const char * const pwm_meson8b_parent_names[] = {
>
> static const struct meson_pwm_data pwm_meson8b_data = {
> .parent_names = pwm_meson8b_parent_names,
> - .num_parents = ARRAY_SIZE(pwm_meson8b_parent_names),
> };
>
> /*
> @@ -351,12 +346,11 @@ static const struct meson_pwm_data pwm_meson8b_data = {
> * The last 2 are grounded
> */
> static const char * const pwm_gxbb_ao_parent_names[] = {
> - "xtal", "clk81"
> + "xtal", "clk81", NULL, NULL,
> };
>
> static const struct meson_pwm_data pwm_gxbb_ao_data = {
> .parent_names = pwm_gxbb_ao_parent_names,
> - .num_parents = ARRAY_SIZE(pwm_gxbb_ao_parent_names),
> };
>
> static const char * const pwm_axg_ee_parent_names[] = {
> @@ -365,7 +359,6 @@ static const char * const pwm_axg_ee_parent_names[] = {
>
> static const struct meson_pwm_data pwm_axg_ee_data = {
> .parent_names = pwm_axg_ee_parent_names,
> - .num_parents = ARRAY_SIZE(pwm_axg_ee_parent_names),
> };
>
> static const char * const pwm_axg_ao_parent_names[] = {
> @@ -374,7 +367,6 @@ static const char * const pwm_axg_ao_parent_names[] = {
>
> static const struct meson_pwm_data pwm_axg_ao_data = {
> .parent_names = pwm_axg_ao_parent_names,
> - .num_parents = ARRAY_SIZE(pwm_axg_ao_parent_names),
> };
>
> static const char * const pwm_g12a_ao_ab_parent_names[] = {
> @@ -383,16 +375,14 @@ static const char * const pwm_g12a_ao_ab_parent_names[] = {
>
> static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
> .parent_names = pwm_g12a_ao_ab_parent_names,
> - .num_parents = ARRAY_SIZE(pwm_g12a_ao_ab_parent_names),
> };
>
> static const char * const pwm_g12a_ao_cd_parent_names[] = {
> - "xtal", "g12a_ao_clk81",
> + "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,
> - .num_parents = ARRAY_SIZE(pwm_g12a_ao_cd_parent_names),
> };
>
> static const struct of_device_id meson_pwm_matches[] = {
> @@ -432,23 +422,25 @@ static const struct of_device_id meson_pwm_matches[] = {
> };
> MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>
> -static int meson_pwm_init_channels(struct meson_pwm *meson)
> +static int meson_pwm_init_clocks_legacy(struct device *dev,
> + struct clk_parent_data *mux_parent_data)
It's not yet legacy, the rename should be in the next patch
> {
> - struct clk_parent_data mux_parent_data[MESON_MAX_MUX_PARENTS] = {};
> - struct device *dev = meson->chip.dev;
> + struct meson_pwm *meson = dev_get_drvdata(dev);
> unsigned int i;
> char name[255];
> int err;
>
> - for (i = 0; i < meson->data->num_parents; i++) {
> - mux_parent_data[i].index = -1;
> - mux_parent_data[i].name = meson->data->parent_names[i];
> - }
> -
> - for (i = 0; i < meson->chip.npwm; i++) {
> + for (i = 0; i < MESON_NUM_PWMS; i++) {
> struct meson_pwm_channel *channel = &meson->channels[i];
> struct clk_parent_data div_parent = {}, gate_parent = {};
> struct clk_init_data init = {};
> + struct clk_divider *div;
> + struct clk_gate *gate;
> + struct clk_mux *mux;
> +
> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return -ENOMEM;
>
> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
>
> @@ -456,69 +448,76 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
> init.ops = &clk_mux_ops;
> init.flags = 0;
> init.parent_data = mux_parent_data;
> - init.num_parents = meson->data->num_parents;
> -
> - channel->mux.reg = meson->base + REG_MISC_AB;
> - channel->mux.shift =
> - meson_pwm_per_channel_data[i].clk_sel_shift;
> - channel->mux.mask = MISC_CLK_SEL_MASK;
> - channel->mux.flags = 0;
> - channel->mux.lock = &meson->lock;
> - channel->mux.table = NULL;
> - channel->mux.hw.init = &init;
> -
> - err = devm_clk_hw_register(dev, &channel->mux.hw);
> + init.num_parents = MESON_NUM_MUX_PARENTS;
> +
> + mux->reg = meson->base + REG_MISC_AB;
> + mux->shift = meson_pwm_per_channel_data[i].clk_sel_shift;
> + mux->mask = MISC_CLK_SEL_MASK;
> + mux->flags = 0;
> + mux->lock = &meson->lock;
> + mux->table = NULL;
> + mux->hw.init = &init;
> +
> + err = devm_clk_hw_register(dev, &mux->hw);
> if (err) {
> dev_err(dev, "failed to register %s: %d\n", name, err);
> return err;
> }
>
> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> + if (!div)
> + return -ENOMEM;
> +
> snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i);
>
> init.name = name;
> init.ops = &clk_divider_ops;
> init.flags = CLK_SET_RATE_PARENT;
> div_parent.index = -1;
> - div_parent.hw = &channel->mux.hw;
> + div_parent.hw = &mux->hw;
> init.parent_data = &div_parent;
> init.num_parents = 1;
>
> - channel->div.reg = meson->base + REG_MISC_AB;
> - channel->div.shift = meson_pwm_per_channel_data[i].clk_div_shift;
> - channel->div.width = MISC_CLK_DIV_WIDTH;
> - channel->div.hw.init = &init;
> - channel->div.flags = 0;
> - channel->div.lock = &meson->lock;
> + div->reg = meson->base + REG_MISC_AB;
> + div->shift = meson_pwm_per_channel_data[i].clk_div_shift;
> + div->width = MISC_CLK_DIV_WIDTH;
> + div->hw.init = &init;
> + div->flags = 0;
> + div->lock = &meson->lock;
>
> - err = devm_clk_hw_register(dev, &channel->div.hw);
> + err = devm_clk_hw_register(dev, &div->hw);
> if (err) {
> dev_err(dev, "failed to register %s: %d\n", name, err);
> return err;
> }
>
> + gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> + if (!gate)
> + return -ENOMEM;
> +
> snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i);
>
> init.name = name;
> init.ops = &clk_gate_ops;
> init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
> gate_parent.index = -1;
> - gate_parent.hw = &channel->div.hw;
> + gate_parent.hw = &div->hw;
> init.parent_data = &gate_parent;
> init.num_parents = 1;
>
> - channel->gate.reg = meson->base + REG_MISC_AB;
> - channel->gate.bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
> - channel->gate.hw.init = &init;
> - channel->gate.flags = 0;
> - channel->gate.lock = &meson->lock;
> + gate->reg = meson->base + REG_MISC_AB;
> + gate->bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
> + gate->hw.init = &init;
> + gate->flags = 0;
> + gate->lock = &meson->lock;
>
> - err = devm_clk_hw_register(dev, &channel->gate.hw);
> + err = devm_clk_hw_register(dev, &gate->hw);
> if (err) {
> dev_err(dev, "failed to register %s: %d\n", name, err);
> return err;
> }
>
> - channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL);
> + channel->clk = devm_clk_hw_get_clk(dev, &gate->hw, NULL);
> if (IS_ERR(channel->clk)) {
> err = PTR_ERR(channel->clk);
> dev_err(dev, "failed to register %s: %d\n", name, err);
> @@ -529,31 +528,56 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
> return 0;
> }
>
> +static int meson_pwm_init_channels(struct device *dev)
> +{
> + struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
> + struct meson_pwm *meson = dev_get_drvdata(dev);
> + int i;
> +
> + 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];
> + }
> +
> + return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
> +}
> +
> static int meson_pwm_probe(struct platform_device *pdev)
> {
> struct meson_pwm *meson;
> + struct pwm_chip *chip;
> int err;
>
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL);
> if (!meson)
> return -ENOMEM;
>
> + platform_set_drvdata(pdev, meson);
> +
> meson->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(meson->base))
> return PTR_ERR(meson->base);
>
> spin_lock_init(&meson->lock);
> - meson->chip.dev = &pdev->dev;
> - meson->chip.ops = &meson_pwm_ops;
> - meson->chip.npwm = MESON_NUM_PWMS;
> + chip->dev = &pdev->dev;
> + chip->ops = &meson_pwm_ops;
> + chip->npwm = MESON_NUM_PWMS;
>
> meson->data = of_device_get_match_data(&pdev->dev);
> + if (!meson->data) {
> + dev_err(&pdev->dev, "failed to match device\n");
> + return -ENODEV;
> + }
>
> - err = meson_pwm_init_channels(meson);
> + err = meson_pwm_init_channels(&pdev->dev);
> if (err < 0)
> return err;
>
> - err = devm_pwmchip_add(&pdev->dev, &meson->chip);
> + err = devm_pwmchip_add(&pdev->dev, chip);
> if (err < 0) {
> dev_err(&pdev->dev, "failed to register PWM chip: %d\n", err);
> return err;
Thanks,
Neil
Powered by blists - more mailing lists