Re: [PATCH 2/2] i2c: atciic100: add Andes I2C driver support

[Andi Shyti]

Hi Ben,

I glanced throught the patch and looks OK, just few things with
little importance. Please, do not forget to run checkpatch.pl
before sendint the patch.

Once you send a v2, I will read it more carefully.

...

> +	if (status & ATCIIC100_STATUS_CMPL) {
> +		/* Write 1 to clear all status */
> +		writel(ATCIIC100_STATUS_W1C, i2c->base + ATCIIC100_STATUS_REG);
> +
> +		i2c->xfer_done = true;
> +		if (status & ATCIIC100_STATUS_ADDR_HIT)
> +			i2c->addr_hit = true;
> +
> +		/* For the last read, retrieve all remaining data in FIFO. */
> +		while (i2c->buf_len--)
> +			*i2c->buf++ = readl(i2c->base + ATCIIC100_DATA_REG);
> +

please, remove this blank space here

> +	}
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +}

...

> +static int atciic100_xfer_wait(struct atciic100 *i2c, struct i2c_msg *msg)
> +{
> +	u32 val;
> +
> +	/*
> +	 * Set the data count.
> +	 * If there are 256 bytes to be transmitted/received, write 0 to the
> +	 * data count field.
> +	 */
> +	val = readl(i2c->base + ATCIIC100_CTRL_REG) |
> +	      (i2c->buf_len & ATCIIC100_CTRL_DATA_CNT);

please checkt the alignment here.

> +	/* Set the transaction direction */
> +	if (msg->flags & I2C_M_RD)
> +		val |= ATCIIC100_CTRL_DIR;
> +	else
> +		val &= ~ATCIIC100_CTRL_DIR;

...

> +/* I2C may be needed to bring up other drivers */
> +static int __init atciic100_init_driver(void)
> +{
> +	return platform_driver_register(&atciic100_platform_driver);
> +}
> +subsys_initcall(atciic100_init_driver);
> +
> +static void __exit atciic100_exit_driver(void)
> +{
> +	platform_driver_unregister(&atciic100_platform_driver);
> +}
> +module_exit(atciic100_exit_driver);

can you please use module_platform_driver()?

Andi

> +
> +MODULE_AUTHOR("Ben Zong-You Xie <ben717@xxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Andes ATCIIC100 I2C bus adapter");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 2.34.1
>