Re: [PATCH v10 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

[zychen]

Hi Andi,
Thanks for your review. 

Andi Shyti 於 2026/9/28 下午 08:24 寫道:
> Hi Zi-Yu,
> 
> ...
> 
>> +/* Constants */
>> +#define MA35_CLKDIV_MSK		GENMASK(9, 0)
>> +#define I2C_PM_TIMEOUT_MS	5000
>> +#define STOP_TIMEOUT_MS		50
> 
> these two defines are the only ones without the MA35 prefix.

Will add the MA35 prefix to these two definitions.
> 
> ...
> 
>> +static irqreturn_t ma35d1_i2c_irq_target_trx(struct ma35d1_i2c *i2c,
>> +					     unsigned long i2c_status)
>> +{
>> +	unsigned char byte = 0;
>> +
>> +	switch (i2c_status) {
>> +	case MA35_S_RECE_ARB_LOST:
>> +		/*
>> +		 * Arbitration lost during address transmission phase.
>> +		 * The hardware switches to Target Transmitter mode when
>> +		 * our own SLA+W is detected on the bus.
>> +		 */
>> +		i2c->err = -EAGAIN;
>> +		ma35d1_i2c_controller_complete(i2c);
>> +		i2c_slave_event(i2c->target, I2C_SLAVE_WRITE_REQUESTED, &byte);
> 
> All the return values of these i2c_slave_event()'s are ignored.
> 
>> +		break;
>> +
>> +	case MA35_S_RECE_ADDR_ACK:
>> +		/* Own SLA+W has been receive; ACK has been return */
>> +		i2c_slave_event(i2c->target, I2C_SLAVE_WRITE_REQUESTED, &byte);
>> +		break;
>> +
>> +	case MA35_S_TRAN_DATA_NACK:
>> +	case MA35_S_RECE_DATA_NACK:
>> +		/*
>> +		 * Data byte or last data in I2CDAT has been transmitted and NACK received,
>> +		 * or previously addressed with own SLA address and NACK returned.
>> +		 */
>> +		break;
>> +
> 
> ...
> 
>> +	default:
>> +		dev_err(i2c->dev, "Status 0x%02lx is NOT processed\n",
>> +			i2c_status);
>> +		ma35d1_i2c_restore_idle(i2c);
>> +		return IRQ_NONE;
>> +	}
>> +	ma35d1_i2c_write_ctl(i2c, MA35_CTL_SI_AA);
> 
> As far as I understood, this this is an unconditional ACK enabled
> for the next bytes received, right? In that case are we ignoring
> failed communications as above where we are supposed to send
> NACKs?
> 
Regarding the two comments above:
I will add handling for the return value of `I2C_SLAVE_WRITE_REQUESTED`. When it returns an error, subsequent bytes will be NACKed until the transfer ends.

For `I2C_SLAVE_WRITE_RECEIVED`, the MA35D1 hardware has already generated the ACK when this event is reported, so it is not possible to NACK the received byte at this point. Therefore, its return value can only be temporarily ignored.

>> +	return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +	i2c->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> +	if (IS_ERR(i2c->regs))
>> +		return PTR_ERR(i2c->regs);
>> +
>> +	i2c->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> +	if (IS_ERR(i2c->rst))
>> +		return dev_err_probe(dev, PTR_ERR(i2c->rst),
>> +				     "failed to get reset control\n");
>> +
>> +	ret = reset_control_deassert(i2c->rst);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to deassert reset line\n");
>> +
>> +	/* Setup info block for the I2C core */
>> +	strscpy(i2c->adap.name, "ma35d1-i2c", sizeof(i2c->adap.name));
>> +	i2c->adap.owner = THIS_MODULE;
>> +	i2c->adap.algo = &ma35d1_i2c_algorithm;
>> +	i2c->adap.quirks = &ma35d1_i2c_quirks;
>> +	i2c->adap.retries = 2;
>> +	i2c->adap.algo_data = i2c;
>> +	i2c->adap.dev.parent = &pdev->dev;
>> +	i2c->adap.dev.of_node = pdev->dev.of_node;
>> +	i2c_set_adapdata(&i2c->adap, i2c);
>> +
>> +	if (!device_property_read_u32(dev, "clock-frequency", &val)) {
>> +		if (val != 0 && val <= MEGA)
>> +			busfreq = val;
>> +	}
>> +	/* Calculate divider based on the current peripheral clock rate */
>> +	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(i2c->clk), busfreq * 4) - 1;
>> +	if (clkdiv < 0 || clkdiv > 0x3ff)
>> +		return dev_err_probe(dev, -EINVAL, "invalid clkdiv value: %d\n",
>> +				     clkdiv);
>> +
>> +	i2c->irq = platform_get_irq(pdev, 0);
>> +	if (i2c->irq < 0)
>> +		return dev_err_probe(dev, i2c->irq, "failed to get irq\n");
>> +
>> +	platform_set_drvdata(pdev, i2c);
>> +
>> +	pm_runtime_set_autosuspend_delay(dev, I2C_PM_TIMEOUT_MS);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = devm_add_action_or_reset(dev, ma35d1_i2c_pm_cleanup, dev);
>> +	if (ret)
>> +		return ret;
> 
> you are printing an error message everywhere, except of here.

Right. I’ll add an error message here as well.

> 
>> +
>> +	writel(MA35_CTL_I2CEN | MA35_CTL_INTEN, i2c->regs + MA35_CTL0);
>> +	writel(FIELD_PREP(MA35_CLKDIV_MSK, clkdiv), i2c->regs + MA35_CLKDIV);
>> +
>> +	ret = devm_request_irq(dev, i2c->irq, ma35d1_i2c_irq, 0, dev_name(dev),
>> +			       i2c);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "cannot claim IRQ %d\n", i2c->irq);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_i2c_add_adapter(dev, &i2c->adap);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "failed to add bus to i2c core\n");
>> +		return ret;
> 
> return dev_err_probe(...)
will do.

> 
> Thanks,
> Andi
> 
>> +	}
>> +
>> +	dev_info(&i2c->adap.dev, "%pa MA35D1 I2C adapter registered\n",
>> +		 &res->start);
>> +	return 0;
>> +}