[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5pv3ovaukhzhe2bh5ko7463xduqlnweiaxv2hbafc6fadynej7@aagcu7av45ov>
Date: Fri, 22 Mar 2024 01:26:49 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Piyush Malgujar <pmalgujar@...vell.com>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
sgarapati@...vell.com, cchavva@...vell.com, jannadurai@...vell.com
Subject: Re: [PATCH v4 2/4] i2c: thunderx: Add support for High speed mode
Hi Piyush,
On Fri, Feb 23, 2024 at 04:57:23AM -0800, Piyush Malgujar wrote:
> From: Suneel Garapati <sgarapati@...vell.com>
>
> 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
Powered by blists - more mailing lists