[U-Boot] [PATCH v2 2/3] i2c: mxc_i2c: The i2c controller generates a stop signal before reading the register data

[Lukasz Majewski]

Hi Chuanhua,

> This patch enables the i2c controller to generate a stop signal before
> reading the slave device's internal register after setting the
> register address (need to determine if the signal is needed according
> to the message flag).
> 
> Signed-off-by: Biwen Li <biwen.li at nxp.com>
> Signed-off-by: Chuanhua Han <chuanhua.han at nxp.com>
> ---
> Changes in v2: 
> 	- Split the original patch into 3 patches
> 	- Add detailed description information for each patch
> 
>  drivers/i2c/mxc_i2c.c | 70

I must admit that I'm a bit puzzled - in the patch 1/3 you added the
U-Boot wide I2C_M_RD_NEED_STOP_BIT flag to indicate the need to generate
a stop bit.

Iin this patch you change the respective mxc_i2c.c
driver (which is NXP SoC specific). Shouldn't those changes go in to a
"generic" i2c code (or better to the pcf2127 driver) ?

What would happen if somebody use this device (pcf2127.c) on board
running e.g. TI processor? (like Beagle Bone Black). Would the pcf2127
work properly in that case?

> ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 62
> insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index 23119cce65..f3811eb0c5 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -961,6 +961,8 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) int reg_shift = i2c_bus->driver_data
> & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
>  	int read_mode;
> +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ? true :
> false;
> +	unsigned int temp;
>  
>  	/* Here address len is set to -1 to not send any address at
> first.
>  	 * Otherwise i2c_init_transfer will send the chip address
> with write @@ -975,6 +977,7 @@ static int mxc_i2c_xfer(struct udevice
> *bus, struct i2c_msg *msg, int nmsgs) read_mode = -1; /* So it's
> always different on the first message */ for (; nmsgs > 0; nmsgs--,
> msg++) { const int msg_is_read = !!(msg->flags & I2C_M_RD);
> +		bool next_is_read = nmsgs > 1 && (msg[1].flags &
> I2C_M_RD); 
>  		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n",
> msg->addr, msg->len, msg_is_read ? 'R' : 'W');
> @@ -982,13 +985,16 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) if (msg_is_read != read_mode) {
>  			/* Send repeated start if not 1st message */
>  			if (read_mode != -1) {
> -				debug("i2c_xfer: [RSTART]\n");
> -				ret = readb(base + (I2CR <<
> reg_shift));
> -				ret |= I2CR_RSTA;
> -				writeb(ret, base + (I2CR <<
> reg_shift));
> +				if (!(msg[1].flags &
> I2C_M_RD_NEED_STOP_BIT)) {
> +					debug("i2c_xfer:
> [RSTART]\n");
> +					ret = readb(base + (I2CR <<
> reg_shift));
> +					ret |= I2CR_RSTA;
> +					writeb(ret, base + (I2CR <<
> reg_shift));
> +				}
>  			}
>  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> msg->addr, msg_is_read ? 'R' : 'W');
> +
>  			ret = tx_byte(i2c_bus, (msg->addr << 1) |
> msg_is_read); if (ret < 0) {
>  				debug("i2c_xfer: [STOP]\n");
> @@ -998,16 +1004,64 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
>  		}
>  
> -		if (msg->flags & I2C_M_RD)
> +		if (msg->flags & I2C_M_RD) {
>  			ret = i2c_read_data(i2c_bus, msg->addr,
> msg->buf, msg->len, nmsgs == 1 ||
>  						      (msg->flags &
> I2C_M_STOP));
> -		else
> -			ret = i2c_write_data(i2c_bus, msg->addr,
> msg->buf,
> -					     msg->len);
> +			if (ret < 0)
> +				break;
> +			continue;
> +		}
>  
> +		 /* Write message */
> +		ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> +				     msg->len);
>  		if (ret < 0)
>  			break;
> +
> +		if (!next_is_read)
> +			continue;
> +
> +		/* Read message following write message */
> +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> +			/* Generate a stop bit */
> +			i2c_imx_stop(i2c_bus);
> +			/* Reset i2c slave */
> +			i2c_force_reset_slave();
> +
> +			/* Enable I2C controller */
> +			if (quirk)
> +				ret = readb(base + (I2CR <<
> reg_shift))
> +					& I2CR_IDIS;
> +			else
> +				ret = !(readb(base + (I2CR <<
> reg_shift))
> +						& I2CR_IEN);
> +			if (ret) {
> +				writeb(I2CR_IEN, base + (I2CR <<
> reg_shift));
> +				/* Wait for controller to be stable
> */
> +				udelay(50);
> +			}
> +
> +			/* Clear interrupt bit */
> +			writeb(I2SR_IIF_CLEAR, base + (I2SR <<
> reg_shift));
> +			ret = wait_for_sr_state(i2c_bus,
> ST_BUS_IDLE);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Start I2C transaction */
> +			temp = readb(base + (I2CR << reg_shift));
> +			temp |= I2CR_MSTA;
> +			writeb(temp, base + (I2CR << reg_shift));
> +
> +			ret = wait_for_sr_state(i2c_bus,
> ST_BUS_BUSY);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Enter transfer mode */
> +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> +			writeb(temp, base + (I2CR << reg_shift));
> +			udelay(50);
> +		}
>  	}
>  
>  	if (ret)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190524/3653fdf0/attachment.sig>