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