Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell <troymitchell988@gmail.com>]

On 2025/1/4 08:34, Andi Shyti wrote:
> Hi Troy,
> 
> sorry for having kept this patch unanswered for so long. I had a
> quick look and I have a few comments through the lines.>
> ...
> 
>> +config I2C_K1
>> +	tristate "Spacemit K1 I2C adapter"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on OF
>> +	help
>> +	  This option enables support for the I2C interface on the Spacemit K1
>> +	  platform.
>> +
>> +	  If you enable this configuration, the kernel will include support for
>> +	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
> 
> /ca/can/
I will
> 
>> +	  be used to manage I2C bus transactions, which are necessary for
>> +	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
>> +	  devices.
>> +
>> +	  This driver can also be compiled as a module. If you choose to build
>> +	  it as a module, the resulting kernel module will be named `i2c-k1`.
>> +	  Loading this module will enable the I2C functionality for the K1
>> +	  platform dynamically, without requiring a rebuild of the kernel.
> 
> This last paragraph contains more information than necessary,
> please check other similar cases and keep the same format.
> 
> (E.g.: "This driver can also be built as a module.  If so, the
> module will be called i2c-ali1563.". People know already what
> compiling as module means :-)).
thanks :)
> 
>>  config I2C_KEBA
>>  	tristate "KEBA I2C controller support"
>>  	depends on HAS_IOMEM
> 
> ...
> 
>> +/* spacemit i2c registers */
>> +#define ICR          0x0		/* Control Register */
>> +#define ISR          0x4		/* Status Register */
>> +#define ISAR         0x8		/* Slave Address Register */
>> +#define IDBR         0xc		/* Data Buffer Register */
>> +#define ILCR         0x10		/* Load Count Register */
>> +#define IWCR         0x14		/* Wait Count Register */
>> +#define IRST_CYC     0x18		/* Bus reset cycle counter */
>> +#define IBMR         0x1c		/* Bus monitor register */
>> +#define IWFIFO       0x20		/* Write FIFO Register */
>> +#define IWFIFO_WPTR  0x24		/* Write FIFO Write Pointer Register */
>> +#define IWFIFO_RPTR  0x28		/* Write FIFO Read Pointer Register */
>> +#define IRFIFO       0x2c		/* Read FIFO Register */
>> +#define IRFIFO_WPTR  0x30		/* Read FIFO Write Pointer Register */
>> +#define IRFIFO_RPTR  0x34		/* Read FIFO Read Pointer Register */
> 
> Please do use a prefix for all the defines here, e.g. SPACEMINT_
I will.
> 
> ...
> 
>> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>> +{
>> +	unsigned long time_left;
>> +
>> +	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
>> +		i2c->cur_msg = i2c->msgs + i2c->msg_idx;
>> +		i2c->msg_buf = i2c->cur_msg->buf;
>> +		i2c->err = 0;
>> +		i2c->status = 0;
>> +		i2c->unprocessed = i2c->cur_msg->len;
>> +
>> +		reinit_completion(&i2c->complete);
>> +
>> +		spacemit_i2c_start(i2c);
>> +
>> +		time_left = wait_for_completion_timeout(&i2c->complete,
>> +							i2c->adapt.timeout);
>> +		if (unlikely(time_left == 0)) {
> 
> no need for unlikely here.
I will drop all unlikely.
> 
>> +			dev_alert(i2c->dev, "msg completion timeout\n");
> 
> dev_alert is a bit too much, please use dev_err instead.
ok.
> 
>> +			spacemit_i2c_bus_reset(i2c);
>> +			spacemit_i2c_reset(i2c);
>> +			return -ETIMEDOUT;
>> +		}
> 
> ...
> 
>> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
>> +{
>> +	u32 val;
>> +	/*
>> +	 * send transaction complete signal:
>> +	 * error happens, detect master stop
>> +	 */
>> +	if (likely(i2c->err || (i2c->status & SR_MSD))) {
> 
> I don't see a need for likely here.
> 
>> +		/*
>> +		 * Here the transaction is already done, we don't need any
> 
> ...
> 
>> +	ret = spacemit_i2c_xfer_msg(i2c);
>> +	if (unlikely(ret < 0)) {
>> +		dev_dbg(i2c->dev, "i2c transfer error\n");
>> +		/* timeout error should not be overridden, and the transfer
>> +		 * error will be confirmed by err handle function latter,
>> +		 * the reset should be invalid argument error.
>> +		 */
> 
> Please fix the commenting style (refer to
> Documentation/process/coding-style.rst).
> 
> Besides, please, do not shorten words (err instead of error), we
> are not in urge to save comment space. Please reword this comment
> to be understood.
I will.
> 
>> +		if (ret != -ETIMEDOUT)
>> +			ret = -EINVAL;
> 
> why do we need to change to -EINVAL? Doesn't seem like a good
> practice
I will fix it.
> 
>> +	}
>> +
>> +	return ret;
>> +}
> 
> ...
> 
>> +	spacemit_i2c_disable(i2c);
>> +
>> +	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
> 
> is unlikely necessary? What if ret has a different value?
I will drop all unlikely/likely. It's not necessary.
> 
> Andi
Thank you Andi.

-- 
Troy Mitchell

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv