Re: [V4, 1/2] i2c: brcmstb: Add Broadcom settop SoC i2c controller driver

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

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

On Tue, May 19, 2015 at 12:23:44PM -0400, Kamal Dasu wrote:
> Adding support for i2c controller driver for Broadcom settop
> SoCs.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

We are very close.

> +/* Wait for device to be ready */
> +static int brcmstb_i2c_wait_if_busy(struct brcmstb_i2c_dev *dev)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(I2C_TIMEOUT);
> +
> +	while ((bsc_readl(dev, iic_enable) & BSC_IIC_EN_INTRP_MASK)) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(dev->device, "i2c device busy timeout\n");

Don't print out, it will spoil the logs. Timeouts can happen on I2C
busses, no need to inform the user.

...

> +	/* Wait for transaction to finish or timeout */
> +	rc = brcmstb_i2c_wait_for_completion(dev);
> +	if (rc) {
> +		dev_err(dev->device, "intr timeout for cmd %s\n",
> +			cmd_string[cmd]);
> +		goto cmd_out;
> +	}

ditto

> +
> +	if ((CMD_RD || CMD_WR) &&
> +	    bsc_readl(dev, iic_enable) & BSC_IIC_EN_NOACK_MASK) {
> +		rc = -EREMOTEIO;
> +		dev_dbg(dev->device, "controller received NOACK intr for %s\n",
> +			cmd_string[cmd]);

I'd leave this out, too. But you decide.

...

> +	rc = of_property_read_string(dev->device->of_node, "interrupt-names",
> +				     &int_name);

I haven't checked but is that really needed? of_irq_to_resource() seems
to parse the "interrupt-names" property.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]