Re: [PATCH v8 2/3] i2c: nt72676: Add Novatek NT72676 I2C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver unconditionally logs a hard error via `dev_err` on I2C NACK, which causes kernel log spam during normal bus probing.
- [Low] Accessing uninitialized i2c->adapter.name in nvt_i2c_init() error path.
--

commit ad108b18a20082d1ab2afdd3a9f9c59f5896754b
Author: Ben Huang <Ben_Huang@novatek.com.tw>

i2c: nt72676: Add Novatek NT72676 I2C controller driver

This commit introduces the driver for the Novatek NT72676 I2C controller,
supporting fundamental read/write functions and both Standard-mode and 
Fast-mode operations.

> diff --git a/drivers/i2c/busses/i2c-nt72676.c b/drivers/i2c/busses/i2c-nt72676.c
> new file mode 100644
> index 0000000000000..ab7be28615f0f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nt72676.c
[ ... ]
> +static int nvt_i2c_init(struct nvt_i2c_bus *i2c)
> +{
> +	if (i2c->comp_data->stbc_i2c) {
> +		int ret = nvt_i2c_stbc_auth(i2c);
> +
> +		if (ret) {
> +			dev_err(i2c->dev, "[%s] STBC authentication failed, ret = %d\n",
> +				i2c->adapter.name, ret);

[Severity: Low]
Does this log an empty string for the adapter name? 

Looking at nvt_i2c_probe(), nvt_i2c_init() is called before the adapter name 
is initialized, meaning this early error path accesses an empty string:

nvt_i2c_probe() {
    ...
    ret = nvt_i2c_init(i2c);
    ...
    strscpy(i2c->adapter.name, dev_name(&pdev->dev), sizeof(i2c->adapter.name));
    ...
}

[ ... ]
> +static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +	if (!ret)
> +		i2c->error_code = -ETIMEDOUT;
> +	nvt_i2c_reset(i2c);
> +	ret = i2c->error_code;
> +	i2c->current_msg = NULL;
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	if (ret)
> +		dev_err(i2c->dev, "[%s]: write failed (err:%d); SA[0x%X]\n",
> +			i2c->adapter.name, i2c->error_code, msg->addr);

[Severity: Medium]
Will this spam the kernel ring buffer during normal bus scanning?

Tools like i2cdetect deliberately scan the bus by sending messages to all 
possible addresses. This generates expected NACKs (-ENXIO) for unpopulated 
addresses. 

If we unconditionally log a hard error via dev_err() on NACKs, the kernel 
log can easily get flooded. Could we omit this log or use dev_dbg() instead?

[ ... ]
> +static int nvt_i2c_read(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +	if (!ret)
> +		i2c->error_code = -ETIMEDOUT;
> +	nvt_i2c_reset(i2c);
> +	ret = i2c->error_code;
> +	i2c->current_msg = NULL;
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	if (ret)
> +		dev_err(i2c->dev, "[%s]: read failed (err:%d); SA[0x%X]\n",
> +			i2c->adapter.name, i2c->error_code, msg->addr);

[Severity: Medium]
Similarly, does this also cause unnecessary log spam when a NACK occurs 
during an expected I2C read failure?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902091318.208074-1-nina_kuo@novatek.com.tw?part=2