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

[Bjorn Andersson <quic_bjorande@quicinc.com>]

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@quicinc.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
>