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

[Andi Shyti <andi.shyti@kernel.org>]

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.

...

> +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?

> +	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.

> +
> +	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(...)

Thanks,
Andi

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