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
