Re: [PATCH v3] i2c: add support for microchip fpga i2c controllers

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8025 bytes --]

Hi Conor,

driver looks mostly good, but some comments:

> +/*
> + * mchp_corei2c_dev - I2C device context
> + * @base: pointer to register struct
> + * @msg: pointer to current message
> + * @msg_len: number of bytes transferred in msg
> + * @msg_err: error code for completed message
> + * @msg_complete: xfer completion object
> + * @dev: device reference
> + * @adapter: core i2c abstraction
> + * @i2c_clk: clock reference for i2c input clock
> + * @bus_clk_rate: current i2c bus clock rate
> + * @buf: ptr to msg buffer for easier use.
> + * @isr_status: cached copy of local ISR status.
> + * @lock: spinlock for IRQ synchronization.
> + */

This seems outdated, e.g. msg is not in the struct but addr is missing.
Also, proper kdoc header is missing?

> +struct mchp_corei2c_dev {
> +	void __iomem *base;
> +	size_t msg_len;

Maybe u16 to match the original type from struct i2c_msg?

> +	int msg_err;
> +	struct completion msg_complete;
> +	struct device *dev;
> +	struct i2c_adapter adapter;
> +	struct clk *i2c_clk;
> +	spinlock_t lock; /* IRQ synchronization */
> +	u32 bus_clk_rate;
> +	u32 msg_read;

This is initialized but never used?

> +	u32 isr_status;
> +	u8 *buf;
> +	u8 addr;
> +};
> +

...

> +static int mchp_corei2c_init(struct mchp_corei2c_dev *idev)
> +{
> +	u32 clk_rate = clk_get_rate(idev->i2c_clk);
> +	u32 divisor = clk_rate / idev->bus_clk_rate;

I don't see a protection against division by zero?

> +	int ret;
> +
> +	ret = mchp_corei2c_set_divisor(divisor, idev);
> +	if (ret)
> +		return ret;
> +
> +	mchp_corei2c_reset(idev);
> +
> +	return 0;
> +}
> +
> +static void mchp_corei2c_transfer(struct mchp_corei2c_dev *idev, u32 data)
> +{
> +	if (idev->msg_len > 0)
> +		writeb(data, idev->base + CORE_I2C_DATA);
> +}

Minor: this is very finegrained and only called once. I'd put it into
the calling function.

...

> +static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev)
> +{
> +	u32 status = idev->isr_status;
> +	u8 ctrl;
> +
> +	if (!idev->buf)
> +		return IRQ_NONE;
> +
> +	switch (status) {
> +	case STATUS_M_START_SENT:
> +	case STATUS_M_REPEATED_START_SENT:
> +		ctrl = readb(idev->base + CORE_I2C_CTRL);
> +		ctrl &= ~CTRL_STA;
> +		writeb(idev->addr, idev->base + CORE_I2C_DATA);
> +		writeb(ctrl, idev->base + CORE_I2C_CTRL);
> +		if (idev->msg_len <= 0)

msg_len is unsigned, can't be < 0?

> +			goto finished;
> +		break;
> +	case STATUS_M_ARB_LOST:
> +		idev->msg_err = -EAGAIN;
> +		goto finished;
> +	case STATUS_M_SLAW_ACK:
> +	case STATUS_M_TX_DATA_ACK:
> +		if (idev->msg_len > 0)
> +			mchp_corei2c_fill_tx(idev);
> +		else
> +			goto last_byte;

IMO this is a bit too much of goto here. Can't we have a flag for
last_byte and handle it at the end of the handler?

> +		break;
> +	case STATUS_M_TX_DATA_NACK:
> +	case STATUS_M_SLAR_NACK:
> +	case STATUS_M_SLAW_NACK:
> +		idev->msg_err = -ENXIO;
> +		goto last_byte;
> +	case STATUS_M_SLAR_ACK:
> +		ctrl = readb(idev->base + CORE_I2C_CTRL);
> +		if (idev->msg_len == 1u) {
> +			ctrl &= ~CTRL_AA;
> +			writeb(ctrl, idev->base + CORE_I2C_CTRL);
> +		} else {
> +			ctrl |= CTRL_AA;
> +			writeb(ctrl, idev->base + CORE_I2C_CTRL);
> +		}
> +		if (idev->msg_len < 1u)
> +			goto last_byte;
> +		break;
> +	case STATUS_M_RX_DATA_ACKED:
> +		mchp_corei2c_empty_rx(idev);
> +		break;
> +	case STATUS_M_RX_DATA_NACKED:
> +		mchp_corei2c_empty_rx(idev);
> +		if (idev->msg_len == 0)
> +			goto last_byte;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +
> +last_byte:
> +	/* On the last byte to be transmitted, send STOP */
> +	mchp_corei2c_stop(idev);
> +finished:
> +	complete(&idev->msg_complete);
> +	return IRQ_HANDLED;
> +}

...

> +
> +static int mchp_corei2c_xfer_msg(struct mchp_corei2c_dev *idev,
> +				 struct i2c_msg *msg)
> +{
> +	u8 ctrl;
> +	unsigned long time_left;
> +
> +	if (msg->len == 0)
> +		return -EINVAL;

If your hardware cannot do zero length messages, you need to set up an
i2c_adapter_quirk struct with I2C_AQ_NO_ZERO_LEN.

> +
> +	idev->addr = i2c_8bit_addr_from_msg(msg);
> +	idev->msg_len = msg->len;
> +	idev->buf = msg->buf;
> +	idev->msg_err = 0;
> +	idev->msg_read = (msg->flags & I2C_M_RD);
> +
> +	reinit_completion(&idev->msg_complete);
> +
> +	mchp_corei2c_core_enable(idev);
> +
> +	ctrl = readb(idev->base + CORE_I2C_CTRL);
> +	ctrl |= CTRL_STA;
> +	writeb(ctrl, idev->base + CORE_I2C_CTRL);
> +
> +	time_left = wait_for_completion_timeout(&idev->msg_complete,
> +						MICROCHIP_I2C_TIMEOUT);

You should use adapter.timeout here, so it can be changed from
userspace.

> +	if (!time_left)
> +		return -ETIMEDOUT;
> +
> +	return idev->msg_err;
> +}
> +
> +static int mchp_corei2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			     int num)
> +{
> +	struct mchp_corei2c_dev *idev = i2c_get_adapdata(adap);
> +	int i, ret;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = mchp_corei2c_xfer_msg(idev, msgs++);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return num;
> +}
> +
> +static u32 mchp_corei2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}

If you can't handle zero length messages, you need to mask
I2C_FUNC_SMBUS_QUICK out.

> +
> +static const struct i2c_algorithm mchp_corei2c_algo = {
> +	.master_xfer = mchp_corei2c_xfer,
> +	.functionality = mchp_corei2c_func,
> +};
> +
> +static int mchp_corei2c_probe(struct platform_device *pdev)
> +{
> +	struct mchp_corei2c_dev *idev = NULL;

Unneeded initialization.

> +	struct resource *res;
> +	int irq, ret;
> +	u32 val;
> +
> +	idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	idev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(idev->base))
> +		return PTR_ERR(idev->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq,
> +				     "missing interrupt resource\n");
> +
> +	idev->i2c_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(idev->i2c_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(idev->i2c_clk),
> +				     "missing clock\n");
> +
> +	idev->dev = &pdev->dev;
> +	init_completion(&idev->msg_complete);
> +	spin_lock_init(&idev->lock);
> +
> +	val = device_property_read_u32(idev->dev, "clock-frequency",
> +				       &idev->bus_clk_rate);

This functions returns an int. So, 'ret' please which is also more
readable.

> +	if (val) {

I think the missing check against 'division by zero' should go here.

> +		dev_info(&pdev->dev, "default to 100kHz\n");
> +		idev->bus_clk_rate = 100000;
> +	}
> +
> +	if (idev->bus_clk_rate > 400000)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "clock-frequency too high: %d\n",
> +				     idev->bus_clk_rate);
> +
> +	ret = devm_request_irq(&pdev->dev, irq, mchp_corei2c_isr, IRQF_SHARED,
> +			       pdev->name, idev);

Really SHARED?

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to claim irq %d\n", irq);
> +
> +	ret = clk_prepare_enable(idev->i2c_clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to enable clock\n");
> +
> +	ret = mchp_corei2c_init(idev);
> +	if (ret) {
> +		clk_disable_unprepare(idev->i2c_clk);
> +		return dev_err_probe(&pdev->dev, ret, "failed to program clock divider\n");
> +	}
> +
> +	i2c_set_adapdata(&idev->adapter, idev);
> +	snprintf(idev->adapter.name, sizeof(idev->adapter.name),
> +		 "Microchip I2C hw bus");

Most people add something like the base address here to distinguish
multiple instances.

> +	idev->adapter.owner = THIS_MODULE;
> +	idev->adapter.algo = &mchp_corei2c_algo;
> +	idev->adapter.dev.parent = &pdev->dev;
> +	idev->adapter.dev.of_node = pdev->dev.of_node;
> +
> +	platform_set_drvdata(pdev, idev);
> +
> +	ret = i2c_add_adapter(&idev->adapter);
> +	if (ret) {
> +		clk_disable_unprepare(idev->i2c_clk);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "Microchip I2C Probe Complete\n");
> +
> +	return 0;
> +}

Rest looks good, Thank you for the submission.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]