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@xxxxxxxxx>
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:
signature.asc
Description: Digital signature
