Re: [PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes

[Andi Shyti]

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