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

From: Andi Shyti
Date: Thu Mar 21 2024 - 18:54:00 EST


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