linux-kernel - Re: [PATCH] iio: add support of the max5821

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 15 Jul 2014 16:56:17 +0800
From:	Antonio Borneo <borneo.antonio@...il.com>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	Philippe Reynes <tremyfr@...oo.fr>, linux-iio@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	armadeus-forum@...ts.sourceforge.net, devicetree@...r.kernel.org,
	julien.boibessot@...e.fr
Subject: Re: [PATCH] iio: add support of the max5821

On Tue, Jul 15, 2014 at 4:21 AM, Jonathan Cameron <jic23@...nel.org> wrote:
> On 14/07/14 18:32, Philippe Reynes wrote:

Hi Jonathan,

regarding your comment below

<snip>
>> +static int max5821_get_value(struct iio_dev *indio_dev,
>> +                            int *val, int channel)
>> +{
>> +       struct max5821_data *data = iio_priv(indio_dev);
>> +       struct i2c_client *client = data->client;
>> +       u8 outbuf[1];
>> +       u8 inbuf[2];
>> +       int ret;
>> +
>> +       switch (channel) {
>> +       case 0:
>> +               outbuf[0] = MAX5821_READ_DAC_A_COMMAND;
>> +               break;
>> +       case 1:
>> +               outbuf[0] = MAX5821_READ_DAC_B_COMMAND;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = i2c_master_send(client, outbuf, 1);
>> +       if (ret < 0)
>> +               return ret;
>> +       else if (ret != 1)
>> +               return -EIO;
>> +
>> +       ret = i2c_master_recv(client, inbuf, 2);
>> +       if (ret < 0)
>> +               return ret;
>> +       else if (ret != 2)
>> +               return -EIO;
>
> It somehow always feels like this error handling should be in the
> i2c core.  Just how often does it make sense to receive too little
> from and i2c transaction?  Anyhow, such is life ;)

You wrote:

> You could set this up to use i2c_transfer instead of separating it like
> this.

Accordingly to:
- Documentation/i2c/i2c-protocol
- Documentation/i2c/writing-clients
a sequence of i2c_master_send() and i2c_master_recv() is not fully
equivalent to a single i2c_transfer(); in latter case the transactions
would be combined and the stop bit in between would be removed.

I checked the datasheet of max5821 and it reports that
"Each transmit sequence is framed by a START (S) or REPEATED START
(Sr) condition and a STOP (P) condition."
So combined transaction should work with this device.

But we have few I2C controllers that cannot send combined transactions
and would return error.
E.g. in drivers/i2c/busses/i2c-powermac.c
i2c_powermac_master_xfer() returns -EOPNOTSUPP when num!=1.

What is the proper way to address this:
- use combine transactions, since supported by majority of (but not
all) controllers?
or
- keep individual transactions, if not strictly required by the
protocol of the I2C device?

Thanks,
Antonio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/