Received-SPF: pass (google.com: domain of linux-kernel+bounces-142579-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 12 Apr 2024 13:27:04 +0200
From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To: Jerome Brunet <jbrunet@baylibre.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>, 
	Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Kevin Hilman <khilman@baylibre.com>, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org, linux-pwm@vger.kernel.org, 
	JunYi Zhao <junyi.zhao@amlogic.com>
Subject: Re: [PATCH v5 4/5] pwm: meson: don't carry internal clock elements
 around
Message-ID: <5j2n53asv5yw3apmwgcmopua55dqejqjmhdvfszss4r5xmc37f@uryn2fvwbkqy>
References: <20240221151154.26452-1-jbrunet@baylibre.com>
 <20240221151154.26452-5-jbrunet@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="zt6g3v346aznprz2"
Content-Disposition: inline
In-Reply-To: <20240221151154.26452-5-jbrunet@baylibre.com>


--zt6g3v346aznprz2
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Rework the clock registration to let devres deal with it
>=20
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/pwm/pwm-meson.c | 73 ++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 33 deletions(-)
>=20
> 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 {
>  	}
>  };
> =20
> +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;
> =20
> -	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)
> =20
>  	for (i =3D 0; i < chip->npwm; i++) {
>  		struct meson_pwm_channel *channel =3D &meson->channels[i];
> -		struct clk_parent_data div_parent =3D {}, gate_parent =3D {};
> +		struct clk_parent_data pdata =3D {};
> +		struct meson8b_pwm_clocks *clks;
>  		struct clk_init_data init =3D {};
> =20
> +		clks =3D devm_kzalloc(dev, sizeof(*clks), GFP_KERNEL);
> +		if (!clks)
> +			return -ENOMEM;
> +
>  		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
> =20
>  		init.name =3D name;
> @@ -430,16 +438,15 @@ static int meson_pwm_init_channels(struct pwm_chip =
*chip)
>  		init.parent_data =3D mux_parent_data;
>  		init.num_parents =3D MESON_NUM_MUX_PARENTS;
> =20
> -		channel->mux.reg =3D meson->base + REG_MISC_AB;
> -		channel->mux.shift =3D
> -				meson_pwm_per_channel_data[i].clk_sel_shift;
> -		channel->mux.mask =3D MISC_CLK_SEL_MASK;
> -		channel->mux.flags =3D 0;
> -		channel->mux.lock =3D &meson->lock;
> -		channel->mux.table =3D NULL;
> -		channel->mux.hw.init =3D &init;
> +		clks->mux.reg =3D meson->base + REG_MISC_AB;
> +		clks->mux.shift =3D meson_pwm_per_channel_data[i].clk_sel_shift;
> +		clks->mux.mask =3D MISC_CLK_SEL_MASK;
> +		clks->mux.flags =3D 0;
> +		clks->mux.lock =3D &meson->lock;
> +		clks->mux.table =3D NULL;
> +		clks->mux.hw.init =3D &init;
> =20
> -		err =3D devm_clk_hw_register(dev, &channel->mux.hw);
> +		err =3D 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 =3D name;
>  		init.ops =3D &clk_divider_ops;
>  		init.flags =3D CLK_SET_RATE_PARENT;
> -		div_parent.index =3D -1;
> -		div_parent.hw =3D &channel->mux.hw;
> -		init.parent_data =3D &div_parent;
> +		pdata.index =3D -1;
> +		pdata.hw =3D &clks->mux.hw;
> +		init.parent_data =3D &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 =3D 1;
> =20
> -		channel->div.reg =3D meson->base + REG_MISC_AB;
> -		channel->div.shift =3D meson_pwm_per_channel_data[i].clk_div_shift;
> -		channel->div.width =3D MISC_CLK_DIV_WIDTH;
> -		channel->div.hw.init =3D &init;
> -		channel->div.flags =3D 0;
> -		channel->div.lock =3D &meson->lock;
> +		clks->div.reg =3D meson->base + REG_MISC_AB;
> +		clks->div.shift =3D meson_pwm_per_channel_data[i].clk_div_shift;
> +		clks->div.width =3D MISC_CLK_DIV_WIDTH;
> +		clks->div.hw.init =3D &init;
> +		clks->div.flags =3D 0;
> +		clks->div.lock =3D &meson->lock;
> =20
> -		err =3D devm_clk_hw_register(dev, &channel->div.hw);
> +		err =3D 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 =3D name;
>  		init.ops =3D &clk_gate_ops;
>  		init.flags =3D CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
> -		gate_parent.index =3D -1;
> -		gate_parent.hw =3D &channel->div.hw;
> -		init.parent_data =3D &gate_parent;
> +		pdata.index =3D -1;
> +		pdata.hw =3D &clks->div.hw;
> +		init.parent_data =3D &pdata;
>  		init.num_parents =3D 1;
> =20
> [...]

Best regards
Uwe

--=20
Pengutronix e.K.                           | Uwe Kleine-K=F6nig            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

--zt6g3v346aznprz2
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmYZGogACgkQj4D7WH0S
/k7fEQf+NQGnK13u+LhUqW/7gz4svWQrnLUls2gZbxMsaAOp+aANWHeJHJOeA3xE
fvht30GOvICObIWWvMu1ZaRTSlj/qirKhSYckkZ/wuUpCC2J1Rov9Wsrg4N3Dhkz
KCqQc7B9LCrEqrzRbi52LO+t8OcplkY3C0m9qaw/ul4dKKeF87ip2dVxh4VCmO08
sYNxFarlAiGpU7Qgavk5gBYitR4JRVVcw//9uWpOx8AbeKZFwXlSNCLZnz67J73m
nzHpE9VYcvKMm8ZWE7TksbY88ZaLoehY3PyUx2Fxok00o27ubtMxXOt9m/v+yw3V
jLCR6LOd8c+LV6b4gGnHhwNKOXtm1w==
=gk25
-----END PGP SIGNATURE-----

--zt6g3v346aznprz2--