Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1669098 > unrolled thread

Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Wolfram Sang <wsa@the-dreams.de> - 2017-06-19 16:20 +0200
    Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Wolfram Sang <wsa@the-dreams.de> - 2017-06-19 16:30 +0200
      Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins <brendanhiggins@google.com> - 2017-06-20 03:00 +0200
        Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Wolfram Sang <wsa@the-dreams.de> - 2017-06-20 09:50 +0200
    Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins <brendanhiggins@google.com> - 2017-06-20 06:10 +0200

#1669098 — Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C

[Multipart message — attachments visible in raw view] — view raw

Hi Brendan,

here is my review. Only minor stuff, no real show-stopper.

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 144cbadc7c72..280f84a0d7d1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,16 @@ config I2C_POWERMAC
>  
>  comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>  
> +config I2C_ASPEED
> +	tristate "Aspeed I2C Controller"
> +	depends on ARCH_ASPEED

|| COMPILE_TEST?

> +	help
> +	  If you say yes to this option, support will be included for the
> +	  Aspeed I2C controller.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-aspeed.
> +
>  config I2C_AT91
>  	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
>  	depends on ARCH_AT91
> +struct aspeed_i2c_bus {
> +	struct i2c_adapter		adap;
> +	struct device			*dev;
> +	void __iomem			*base;
> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t			lock;
> +	struct completion		cmd_complete;
> +	int				irq;

'irq' not really needed, I'd think.

> +	unsigned long			parent_clk_frequency;
> +	u32				bus_frequency;
> +	/* Transaction state. */
> +	enum aspeed_i2c_master_state	master_state;
> +	struct i2c_msg			*msgs;
> +	size_t				buf_index;
> +	size_t				msgs_index;
> +	size_t				msgs_count;
> +	bool				send_stop;
> +	int				cmd_err;
> +	/* Protected only by i2c_lock_bus */
> +	int				master_xfer_result;
> +};

...

> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> +{
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +	u32 command;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	command = readl(bus->base + ASPEED_I2C_CMD_REG);
> +
> +	if (command & ASPEED_I2CD_SDA_LINE_STS) {
> +		/* Bus is idle: no recovery needed. */
> +		if (command & ASPEED_I2CD_SCL_LINE_STS)
> +			goto out;
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		reinit_completion(&bus->cmd_complete);
> +		writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			goto reset_out;
> +		else if (bus->cmd_err)
> +			goto reset_out;
> +		/* Recovery failed. */
> +		else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> +			   ASPEED_I2CD_SCL_LINE_STS))
> +			goto reset_out;
> +	/* Bus error. */
> +	} else {
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);

Same dbg message as in the condition? Move it out of the 'if'?

> +
> +		reinit_completion(&bus->cmd_complete);
> +		writel(ASPEED_I2CD_BUS_RECOVER_CMD,
> +		       bus->base + ASPEED_I2C_CMD_REG);

Out of interest: What does the RECOVER_CMD do?

> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			goto reset_out;
> +		else if (bus->cmd_err)
> +			goto reset_out;
> +		/* Recovery failed. */
> +		else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> +			   ASPEED_I2CD_SDA_LINE_STS))
> +			goto reset_out;
> +	}
> +
> +out:
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return ret;
> +
> +reset_out:
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return aspeed_i2c_reset(bus);
> +}

...

> +	case ASPEED_I2C_MASTER_INACTIVE:
> +		dev_err(bus->dev,
> +			"master received interrupt 0x%08x, but is inactive",
> +			irq_status);
> +		bus->cmd_err = -EIO;
> +		/* Do not STOP as we should be inactive. */
> +		goto out_complete;
> +	default:
> +		WARN(1, "unknown master state\n");
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		bus->cmd_err = -EIO;
> +		goto out_complete;
> +	}
> +error_and_stop:
> +	bus->cmd_err = -EIO;
> +	aspeed_i2c_do_stop(bus);
> +	goto out_no_complete;
> +out_complete:
> +	bus->msgs = NULL;
> +	if (bus->cmd_err)
> +		bus->master_xfer_result = bus->cmd_err;
> +	else
> +		bus->master_xfer_result = bus->msgs_index + 1;
> +	complete(&bus->cmd_complete);
> +out_no_complete:
> +	if (irq_status != status_ack)
> +		dev_err(bus->dev,
> +			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> +			irq_status, status_ack);
> +	spin_unlock(&bus->lock);
> +	return !!irq_status;
> +}

You return in the interrupt handler -EIO always in case of errors. Can
you check Documentation/i2c/fault-codes and see if you can follow those
guidelines? Especially ENXIO on NACK and EAGAIN...

> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> +	struct aspeed_i2c_bus *bus = dev_id;
> +
> +	if (aspeed_i2c_master_irq(bus))
> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;

Ternary operator? Your choice, though...

And one more question: Why do you use adapter->algo_data instead of
i2c_{get|set}_adapdata?

Kind regards,

   Wolfram

[toc] | [next] | [standalone]

#1669107

[Multipart message — attachments visible in raw view] — view raw

And any chance for a MAINTAINERS entry?

[toc] | [prev] | [next] | [standalone]

#1670129

On Mon, Jun 19, 2017 at 7:21 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
> And any chance for a MAINTAINERS entry?
>

We already have one:
http://elixir.free-electrons.com/linux/v4.11.5/source/MAINTAINERS#L1090

However, if you would like me to own this driver, I could add an entry
for myself.

[toc] | [prev] | [next] | [standalone]

#1670381

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jun 19, 2017 at 05:58:19PM -0700, Brendan Higgins wrote:
> On Mon, Jun 19, 2017 at 7:21 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> > And any chance for a MAINTAINERS entry?
> >
> 
> We already have one:
> http://elixir.free-electrons.com/linux/v4.11.5/source/MAINTAINERS#L1090
> 
> However, if you would like me to own this driver, I could add an entry
> for myself.

As far as get_maintainers.pl prints out someone responsibe, I am very
happy. The above line won't work for I2C (and maybe some other
subsystems), though,  because we are one directory level deeper
(drivers/i2c/busses/*aspeed*). That might want fixing.

[toc] | [prev] | [next] | [standalone]

#1670248

I thought all of the comments made sense and will be addressed in my
next revision,
except the following:

>> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
>> +{
>> +     unsigned long time_left, flags;
>> +     int ret = 0;
>> +     u32 command;
>> +
>> +     spin_lock_irqsave(&bus->lock, flags);
>> +     command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> +
>> +     if (command & ASPEED_I2CD_SDA_LINE_STS) {
>> +             /* Bus is idle: no recovery needed. */
>> +             if (command & ASPEED_I2CD_SCL_LINE_STS)
>> +                     goto out;
>> +             dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
>> +                     command);
>> +
>> +             reinit_completion(&bus->cmd_complete);
>> +             writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> +             spin_unlock_irqrestore(&bus->lock, flags);
>> +
>> +             time_left = wait_for_completion_timeout(
>> +                             &bus->cmd_complete, bus->adap.timeout);
>> +
>> +             spin_lock_irqsave(&bus->lock, flags);
>> +             if (time_left == 0)
>> +                     goto reset_out;
>> +             else if (bus->cmd_err)
>> +                     goto reset_out;
>> +             /* Recovery failed. */
>> +             else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
>> +                        ASPEED_I2CD_SCL_LINE_STS))
>> +                     goto reset_out;
>> +     /* Bus error. */
>> +     } else {
>> +             dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
>> +                     command);
>
> Same dbg message as in the condition? Move it out of the 'if'?

Message is the same; that's true; however, I only want to print this
if I am actually doing
the recovery. There is a case in the first condition where we actually
don't attempt recovery.
(See both SDA and SCL high). Nevertheless, now that I am looking at
this. I think it might
make sense to make the dbg statements different since I can explain
what type of recovery
I am attempting.

>
>> +
>> +             reinit_completion(&bus->cmd_complete);
>> +             writel(ASPEED_I2CD_BUS_RECOVER_CMD,
>> +                    bus->base + ASPEED_I2C_CMD_REG);
>
> Out of interest: What does the RECOVER_CMD do?

According to the documentation, it attempts to create 1 to 8 SCL
cycles to force any stuck
device to let go of the SDA line. It then puts the hardware in a sane
state once it detects
that the bus has been recovered. I will put a comment to this effect.

>
>> +             spin_unlock_irqrestore(&bus->lock, flags);
>> +
>> +             time_left = wait_for_completion_timeout(
>> +                             &bus->cmd_complete, bus->adap.timeout);
>> +
>> +             spin_lock_irqsave(&bus->lock, flags);
>> +             if (time_left == 0)
>> +                     goto reset_out;
>> +             else if (bus->cmd_err)
>> +                     goto reset_out;
>> +             /* Recovery failed. */
>> +             else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
>> +                        ASPEED_I2CD_SDA_LINE_STS))
>> +                     goto reset_out;
>> +     }
>> +
>> +out:
>> +     spin_unlock_irqrestore(&bus->lock, flags);
>> +
>> +     return ret;
>> +
>> +reset_out:
>> +     spin_unlock_irqrestore(&bus->lock, flags);
>> +
>> +     return aspeed_i2c_reset(bus);
>> +}

Cheers!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web