Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Aug 31, 2023 at 06:04:30AM +0000, Ryan Chen wrote:
> > On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

Stop overquoting! Remove the context you are not answering to.

...

> > > +				if (--i % 4 != 3)
> > > +					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
> > > +				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
> > > +				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> > 
> > Wrong memory accessors. You should use something from asm/byteorder.h
> > which includes linux/byteorder/generic.h.
> > 
> 
> Are you preferring add comment to explain more by following?
> 				/*
> 				 * The controller's buffer register supports dword writes only.
> 				 * Therefore, write dwords to the buffer register in a 4-byte aligned,
> 				 * and write the remaining unaligned data at the end.
> 				 */

This does not explain endianess bug (or feature) it has.
You are using CPU side byteorder for the aligned data.
This is not okay, on top of the code looking ugly and
prone to errors. Note, that somebody may refer to your
code, once accepted, in educational purposes, but since
the code is not good written, it makes a false positive
impression that this is the right thing to do in the similar
case elsewhere.

Please, fix this.

> 				for (i = 0; i < xfer_len; i++) {
> 					wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
> 					/* accumulating 4 bytes of data, write as a Dword to the buffer register */
> 					if (i % 4 == 3)
> 						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
> 				}
> 				/* less than 4 bytes of remaining data, write the remaining part as a Dword */
> 				if (--i % 4 != 3)
> 					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
> 				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
> 				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> 
> Or more columns (use get_unaligned_le32(wbuf); ) by following.
> 
> 	for (i = 0; i < xfer_len; i++) {
> 		wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
> 		if (i % 4 == 3) {
> 			wbuf_dword = get_unaligned_le32(wbuf);
> 			writel(wbuf_dword, i2c_bus->buf_base + i - 3);
> 		}
> 	}
> 
> 	if (--i % 4 != 3) {
> 		wbuf_dword = get_unaligned_le32(wbuf);
> 		writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4));
> 	}

-- 
With Best Regards,
Andy Shevchenko