On Sat, Jun 27, 2015 at 08:08:53PM -0400, David Kessler wrote:
> The i2c-pnx driver implements the i2c specification incorrectly. The
> specification allows for 'repeated start' transactions in which the i2c
> master generates two start conditions without generating a stop condition in
> between them. However, the i2c-pnx driver always generates a stop condition
> after every start condition. This patch correctly implements repeated start
> transactions.
Uh yes, this needs to be fixed. We'd need a few issues of this patch
fixed, first, though. From how I understand the patch, you only use
repeated start for write-then-read messages. You should do it for every
message in a transfer, that is for all messages passed to i2c_pnx_xfer().
This needs rework.
Also, there is not Signed-off line as described in
Documentation/SubmittingPatches, Chapter 11. I need it to apply the
patch.
Some other generic issues:
> @@ -467,6 +480,11 @@ static inline void bus_reset_if_active(struct i2c_pnx_algo_data *alg_data)
> alg_data->adapter.name);
> iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> I2C_REG_CTL(alg_data));
> +
> + dev_dbg(&alg_data->adapter.dev,
> + "%s: Resetting bus\n", __func__);
> + iowrite32(0xff | start_bit, I2C_REG_TX(alg_data));
> +
This changes seems unrelated? If so, it should go into a separate patch.
> +setup_repeated_start(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) {
> + struct i2c_pnx_algo_data *alg_data = adap->algo_data;
> +
> + if (1 < num && !(msgs[0].flags) && ((msgs[1].flags) & I2C_M_RD)) {
Please use 'var > constant'. This notation is too easy to get wrong.
> + alg_data->repeated_start = 1;
> + dev_dbg(&adap->dev,
> + "%s(): repeated start\n", __func__);
> + } else if (1 < num) {
> + alg_data->repeated_start = 0;
> + dev_dbg(&adap->dev,
> + "%s(): non-repeated start\n", __func__);
> + } else if (1 < msgs[0].len) {
> + alg_data->repeated_start = 0;
> + if (!msgs[0].flags) {
> + dev_dbg(&adap->dev,
> + "%s(): multi-byte write\n", __func__);
> + } else {
> + dev_dbg(&adap->dev,
> + "%s(): multi-byte read\n", __func__);
Too much debug output, I'd think. Once the mechanism works, it won't be
of much use to other developers.
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
