Hello Jerome,
On Wed, Feb 21, 2024 at 04:11:50PM +0100, Jerome Brunet wrote:
> Pointers to the internal clock elements of the PWM are useless
> after probe. There is no need to carry this around in the device
> data.
>
> Rework the clock registration to let devres deal with it
>
> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> ---
> drivers/pwm/pwm-meson.c | 73 ++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index a02fdbc61256..fe61335d87d0 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -85,14 +85,17 @@ static struct meson_pwm_channel_data {
> }
> };
>
> +struct meson8b_pwm_clocks {
> + struct clk_divider div;
> + struct clk_gate gate;
> + struct clk_mux mux;
> +};
> +
> struct meson_pwm_channel {
> unsigned long rate;
> unsigned int hi;
> unsigned int lo;
>
> - struct clk_mux mux;
> - struct clk_divider div;
> - struct clk_gate gate;
> struct clk *clk;
> };
I don't think there is a valuable benefit here. Yes, the three
structures are not used explicitly in the driver afterwards, but the
memory has to stay around to call clk_hw_unregister() when the device is
unregistered. So to hide these from struct meson_pwm_channel, we're not
saving any memory, but add the overhead of one additional devm_kzalloc
per PWM channel. For me the cost-benefit calculation is bad here.
> @@ -419,9 +422,14 @@ static int meson_pwm_init_channels(struct pwm_chip *chip)
>
> for (i = 0; i < chip->npwm; i++) {
> struct meson_pwm_channel *channel = &meson->channels[i];
> - struct clk_parent_data div_parent = {}, gate_parent = {};
> + struct clk_parent_data pdata = {};
> + struct meson8b_pwm_clocks *clks;
> struct clk_init_data init = {};
>
> + clks = devm_kzalloc(dev, sizeof(*clks), GFP_KERNEL);
> + if (!clks)
> + return -ENOMEM;
> +
> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
>
> init.name = name;
> @@ -430,16 +438,15 @@ static int meson_pwm_init_channels(struct pwm_chip *chip)
> init.parent_data = mux_parent_data;
> init.num_parents = MESON_NUM_MUX_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;
> + clks->mux.reg = meson->base + REG_MISC_AB;
> + clks->mux.shift = meson_pwm_per_channel_data[i].clk_sel_shift;
> + clks->mux.mask = MISC_CLK_SEL_MASK;
> + clks->mux.flags = 0;
> + clks->mux.lock = &meson->lock;
> + clks->mux.table = NULL;
> + clks->mux.hw.init = &init;
>
> - err = devm_clk_hw_register(dev, &channel->mux.hw);
> + err = devm_clk_hw_register(dev, &clks->mux.hw);
> if (err)
> return dev_err_probe(dev, err,
> "failed to register %s\n", name);
> @@ -449,19 +456,19 @@ static int meson_pwm_init_channels(struct pwm_chip *chip)
> init.name = name;
> init.ops = &clk_divider_ops;
> init.flags = CLK_SET_RATE_PARENT;
> - div_parent.index = -1;
> - div_parent.hw = &channel->mux.hw;
> - init.parent_data = &div_parent;
> + pdata.index = -1;
> + pdata.hw = &clks->mux.hw;
> + init.parent_data = &pdata;
Is it safe to use a single pdata instead of separate div_parent +
gate_parent below as before? That should be a separate change then (or
at least motivated in the commit log.)
> 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;
> + clks->div.reg = meson->base + REG_MISC_AB;
> + clks->div.shift = meson_pwm_per_channel_data[i].clk_div_shift;
> + clks->div.width = MISC_CLK_DIV_WIDTH;
> + clks->div.hw.init = &init;
> + clks->div.flags = 0;
> + clks->div.lock = &meson->lock;
>
> - err = devm_clk_hw_register(dev, &channel->div.hw);
> + err = devm_clk_hw_register(dev, &clks->div.hw);
> if (err)
> return dev_err_probe(dev, err,
> "failed to register %s\n", name);
> @@ -471,22 +478,22 @@ static int meson_pwm_init_channels(struct pwm_chip *chip)
> 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;
> - init.parent_data = &gate_parent;
> + pdata.index = -1;
> + pdata.hw = &clks->div.hw;
> + init.parent_data = &pdata;
> init.num_parents = 1;
>
> [...]
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
