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
