| 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
| ||
|
Message-ID: <mwpaontcdzmzwjay66cqknkxv4xzv364d4vm433pazajhbu2si@epzgjj77clrj>
Date: Sat, 4 Jan 2025 01:34:25 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Troy Mitchell <troymitchell988@...il.com>
Cc: Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
linux-riscv@...ts.infradead.org, linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
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/
> + 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 :-)).
> 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_
...
> +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.
> + dev_alert(i2c->dev, "msg completion timeout\n");
dev_alert is a bit too much, please use dev_err instead.
> + 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.
> + if (ret != -ETIMEDOUT)
> + ret = -EINVAL;
why do we need to change to -EINVAL? Doesn't seem like a good
practice
> + }
> +
> + return ret;
> +}
...
> + spacemit_i2c_disable(i2c);
> +
> + if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
is unlikely necessary? What if ret has a different value?
Andi
Powered by blists - more mailing lists