Re: [PATCH v4 2/4] i2c: thunderx: Add support for High speed mode

[Andi Shyti]

Hi Piyush,

On Fri, Feb 23, 2024 at 04:57:23AM -0800, Piyush Malgujar wrote:
> From: Suneel Garapati <sgarapati@xxxxxxxxxxx>
> 
> Support High speed mode clock setup for OcteonTX2 platforms.
> hs_mode bit in MODE register controls speed mode setup in controller

you could have called it Carl, Jim or John, but you decided to
call it hs_mode, why? Which bit? Bit 4? How setting it and
unsetting it affects the controller?

> and frequency is setup using set_clock function which sets up dividers

You mean octeon_set_clock()?

> for clock control register.

I asked you to explain better, but you just added a few words
here and there.

Please, explain what this patch really does and how does it. I do
not understand anocryms.

..

> @@ -668,7 +670,7 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  	 * Find divisors to produce target frequency, start with large delta
>  	 * to cover wider range of divisors, note thp = TCLK half period.
>  	 */
> -	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
> +	unsigned int ds = 10, thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;

either you add a comment here or you give it a more meaningful
name than "ds".

>  	unsigned int delta_hz = INITIAL_DELTA_HZ;
>  
>  	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));
> @@ -676,6 +678,8 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  	if (is_plat_otx2) {
>  		thp = TWSI_MASTER_CLK_REG_OTX2_VAL;
>  		mdiv_min = 0;
> +		if (!IS_LS_FREQ(i2c->twsi_freq))
> +			ds = 15;
>  	}

We could keep the assignments all in one place:

	if (is_plat_otx2)
		thp = ...
		mdiv_min = ...
		ds = ...
	else
		thp = ...
		mdiv_min = ...
		ds = ...

>  
>  	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {

..

>  #define SW_TWSI(x)	(x->roff.sw_twsi)
>  #define TWSI_INT(x)	(x->roff.twsi_int)
>  #define SW_TWSI_EXT(x)	(x->roff.sw_twsi_ext)
> +#define MODE(x)		(x->roff.mode)

A nice cleanup here is to add prefixes:

OCTEON_REG_SW_TWSI
OCTEON_REG_TWSI_INT
OCTEON_REG_SW_TWST_EXT
OCTEON_REG_MODE

MODE() is so generic. But this would be out of the scope of this
patch.

> +/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
> +#define TWSX_MODE_HS_MASK	(BIT(4) | BIT(0))

I think it's cleaner to have different defines for bit 4 and bit
0.

Thanks,
Andi