linux-kernel - Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d542d00d25e37a922c0fe9d25bba7bbc7220f580.camel@alliedtelesis.co.nz>
Date: Thu, 20 Mar 2025 01:36:12 +0000
From: Aryan Srivastava <Aryan.Srivastava@...iedtelesis.co.nz>
To: "andi.shyti@...nel.org" <andi.shyti@...nel.org>
CC: "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
	"Markus.Elfring@....de" <Markus.Elfring@....de>, "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "rric@...nel.org" <rric@...nel.org>
Subject: Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

Hi Andi,

On Wed, 2025-03-19 at 23:19 +0100, Andi Shyti wrote:
> Hi Aryan,
> 
> few nitpicks between the lines. Please send only this patch as I
> have applied already patch 1 and 2.
> 
> ...
> 
> > +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c,
> > struct i2c_msg *msgs)
> > +{
> > +       int ret;
> > +       u16 len;
> > +       u64 cmd;
> > +
> > +       octeon_i2c_hlc_enable(i2c);
> > +       octeon_i2c_block_enable(i2c);
> > +
> > +       /* Write (size - 1) into block control register */
> > +       len = msgs[1].len - 1;
> > +       octeon_i2c_writeq_flush((u64)len, i2c->twsi_base +
> > OCTEON_REG_BLOCK_CTL(i2c));
> > +
> > +       /* Prepare core command */
> > +       cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR |
> > SW_TWSI_OP_7_IA;
> > +       cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
> > +
> > +       /* Send core command */
> > +       ret = octeon_i2c_hlc_read_cmd(i2c, msgs[0], cmd);
> > +       if (ret)
> > +               return ret;
> 
> Do we need to disable the block mode?
> 
Do you mean, do we need to disable the block mode at all? i.e. have it
on all the time? Otherwise, it gets disabled at the bottom of this
func.
> > +       cmd = __raw_readq(i2c->twsi_base +
> > OCTEON_REG_SW_TWSI(i2c));
> > +       if ((cmd & SW_TWSI_R) == 0)
> > +               return octeon_i2c_check_status(i2c, false);
> > +
> > +       /* read data in FIFO */
> > +       octeon_i2c_writeq_flush(TWSX_BLOCK_STS_RESET_PTR,
> > +                               i2c->twsi_base +
> > OCTEON_REG_BLOCK_STS(i2c));
> > +       for (u16 i = 0; i <= len; i += 8) {
> 
> Please, do not declare the iterator inside the for loop.
> 
Done.
> > +               /* Byte-swap FIFO data and copy into msg buffer */
> > +               __be64 rd = cpu_to_be64(__raw_readq(i2c->twsi_base
> > + OCTEON_REG_BLOCK_FIFO(i2c)));
> > +
> > +               memcpy(&msgs[1].buf[i], &rd, min(8, msgs[1].len -
> > i));
> > +       }
> > +
> > +       octeon_i2c_block_disable(i2c);
> > +       return ret;
> > +}
> 
> ...
> 
> >  #define OCTEON_REG_SW_TWSI(x)          ((x)->roff.sw_twsi)
> >  #define OCTEON_REG_TWSI_INT(x)         ((x)->roff.twsi_int)
> >  #define OCTEON_REG_SW_TWSI_EXT(x)      ((x)->roff.sw_twsi_ext)
> >  #define OCTEON_REG_MODE(x)             ((x)->roff.mode)
> > +#define OCTEON_REG_BLOCK_CTL(x)        (x->roff.block_ctl)
> > +#define OCTEON_REG_BLOCK_STS(x)        (x->roff.block_sts)
> > +#define OCTEON_REG_BLOCK_FIFO(x)       (x->roff.block_fifo)
> 
> Please use the ((x)->...) form.
> 
Done.
> Andi
> 
> >  
> > -/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
> > +/* TWSX_MODE register */
> >  #define TWSX_MODE_REFCLK_SRC   BIT(4)
> > +#define TWSX_MODE_BLOCK_MODE   BIT(2)
> >  #define TWSX_MODE_HS_MODE      BIT(0)
> >  #define TWSX_MODE_HS_MASK      (TWSX_MODE_REFCLK_SRC |
> > TWSX_MODE_HS_MODE)