Re: [PATCH v5 4/5] pwm: meson: don't carry internal clock elements around

[Uwe Kleine-König]

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