Received-SPF: pass (google.com: domain of linux-kernel+bounces-110820-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 22 Mar 2024 01:26:49 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: Piyush Malgujar <pmalgujar@marvell.com>
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, 
	sgarapati@marvell.com, cchavva@marvell.com, jannadurai@marvell.com
Subject: Re: [PATCH v4 2/4] i2c: thunderx: Add support for High speed mode
Message-ID: <5pv3ovaukhzhe2bh5ko7463xduqlnweiaxv2hbafc6fadynej7@aagcu7av45ov>
References: <20240223125725.1709624-1-pmalgujar@marvell.com>
 <20240223125725.1709624-3-pmalgujar@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240223125725.1709624-3-pmalgujar@marvell.com>

Hi Piyush,

On Fri, Feb 23, 2024 at 04:57:23AM -0800, Piyush Malgujar wrote:
> From: Suneel Garapati <sgarapati@marvell.com>
> 
> 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