Received-SPF: pass (google.com: domain of linux-kernel+bounces-110774-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 21 Mar 2024 23:53:50 +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 1/4] i2c: thunderx: Clock divisor logic changes
Message-ID: <plzzqgrfs6m5a7mfizzuu2rf4yqt4fgecelvlc4ibthvocokbj@r6iwu46wryku>
References: <20240223125725.1709624-1-pmalgujar@marvell.com>
 <20240223125725.1709624-2-pmalgujar@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240223125725.1709624-2-pmalgujar@marvell.com>

Hi Piyush,

looks good, just a few little things.

..

> +#define INITIAL_DELTA_HZ	1000000
> +#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
> +#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x3

nit: we can have a better alignment here

#define INITIAL_DELTA_HZ		1000000
#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x03

..

>  void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  {
>  	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
> -	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
> +	unsigned int mdiv_min = 2;
> +	/*
> +	 * 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 delta_hz = INITIAL_DELTA_HZ;
> +
> +	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));

nit: please, don't leave space between variable declaration.
nit: please make important assignments not during the
     declaration, but on a different line.

..

> +/**
> + * octeon_i2c_is_otx2 - check for chip ID
> + * @pdev: PCI dev structure
> + *
> + * Returns TRUE if OcteonTX2, FALSE otherwise.

/TRUE/true/, /FALSE/false/

Can you please write it a bit better? At the end this becomes
documentation. Something like:

"Returns true if the device is an OcteonTX2, false otherwise."

> + */
> +static inline bool octeon_i2c_is_otx2(struct pci_dev *pdev)
> +{
> +	u32 chip_id = (pdev->subsystem_device >> 12) & 0xF;

You could use the bitops helpers here. I never remember which one
is the right one, if I remember correctly FIELD_GET() should be
the right one.

Andi