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: <thazi6n7jwqp6xoz4p6ce7ohxts7ubhgs5h6chqsnnexbkiy3j@q6xzdrze6a6f>
Date: Wed, 19 Mar 2025 23:19:10 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Aryan Srivastava <aryan.srivastava@...iedtelesis.co.nz>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, 
	Markus Elfring <Markus.Elfring@....de>, linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Robert Richter <rric@...nel.org>
Subject: Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

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?

> +	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.

> +		/* 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.

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)