linux-kernel - Re: [PATCH 1/1] i2c: qcom-geni: add 32MHz I2C SE clock support for IPQ5424

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZvMk8T0dK+heMLer@hu-bjorande-lv.qualcomm.com>
Date: Tue, 24 Sep 2024 13:45:37 -0700
From: Bjorn Andersson <quic_bjorande@...cinc.com>
To: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
CC: <andi.shyti@...nel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_srichara@...cinc.com>, <quic_varada@...cinc.com>
Subject: Re: [PATCH 1/1] i2c: qcom-geni: add 32MHz I2C SE clock support for
 IPQ5424

On Tue, Sep 24, 2024 at 12:20:20PM +0530, Manikanta Mylavarapu wrote:

Subject gives a clear indication that this is specific to IPQ5424, which
it isn't. So, please drop that wording from the subject.

Perhaps:
"i2c: qcom-geni: Support systems with 32MHz SE clock"

> The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the
> existing map table is based on 19.2MHz, this patch incorporate the
> clock map table to derive the SCL clock from the 32MHz SE clock.

Then here you're doing the right thing of introducing the IPQ5424, so
this looks good to me.

> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 212336f724a6..bbd9ecf09f4b 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -71,6 +71,7 @@ enum geni_i2c_err_code {
>  
>  #define I2C_AUTO_SUSPEND_DELAY	250
>  #define KHZ(freq)		(1000 * freq)
> +#define MHZ(freq)		(1000000 * freq)
>  #define PACKING_BYTES_PW	4
>  
>  #define ABORT_TIMEOUT		HZ
> @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
>  	{KHZ(1000), 1, 3,  9, 18},
>  };
>  
> +/* source_clock = 32 MHz */
> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = {

I'd prefer that you s/32M/32mhz/, and that you rename geni_i2c_clk_map
to geni_i2c_clk_map_19p2mhz[].

> +	{KHZ(100), 7, 14, 18, 40},
> +	{KHZ(400), 4,  3, 11, 20},
> +	{KHZ(1000), 4, 3,  6, 15},
> +};
> +
>  static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>  {
>  	int i;
>  	const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map;
>  
> +	if (clk_get_rate(gi2c->se.clk) == MHZ(32))
> +		itr = geni_i2c_clk_map_32M;

Leave itr uninitialized above and add an else here with the assignment,
to make it clearer that it's one or the other case. (Compared to "It's
always 19.2MHz and then in some cases we override that with 32MHz")


PS. I wouldn't mind you dropping the addition of the MHZ macro and just
compare clk_get_rate() with 32000000 and 19200000. But that's a matter
of taste.

Regards,
Bjorn

> +
>  	for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) {
>  		if (itr->clk_freq_out == gi2c->clk_freq_out) {
>  			gi2c->clk_fld = itr;
> -- 
> 2.34.1
>