[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191118083810.467iry6w4kt3s7kd@pengutronix.de>
Date: Mon, 18 Nov 2019 09:38:10 +0100
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Wolfram Sang <wsa@...-dreams.de>, linux-iio@...r.kernel.org,
Luca Ceresoli <luca@...aceresoli.net>,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Hartmut Knaack <knaack.h@....de>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>
Subject: Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for
reads and writes
[Fixing the address of the linux-iio list]
Hello Dmitry,
On Mon, Nov 18, 2019 at 12:04:46AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 18, 2019 at 08:43:49AM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 12, 2019 at 12:31:30PM -0800, Dmitry Torokhov wrote:
> > > There is no need to force users of i2c_master_send()/i2c_master_recv()
> > > and other i2c read/write bulk data API to cast everything into u8 pointers.
> > > While everything can be considered byte stream, the drivers are usually
> > > work with more structured data.
> > >
> > > Let's switch the APIs to accept [const] void pointers to cut amount of
> > > casting needed.
> > >
> > > Acked-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> >
> > Can you give an example where you save some casts? Given that i2c is a
> > byte oriented protocol (as opposed to for example spi) I think it's a
> > good idea to expose this in the API.
>
> I see this at least:
>
> dtor@...r-ws:~/kernel/work $ git grep "i2c_master.*(u8 \*)"
> drivers/crypto/atmel-i2c.c: ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
> drivers/iio/common/ms_sensors/ms_sensors_i2c.c: ret = i2c_master_recv(client, (u8 *)&buf, 3);
> drivers/iio/humidity/hdc100x.c: ret = i2c_master_recv(client, (u8 *)buf, 4);
I think this one has an endianness bug.
> drivers/iio/pressure/abp060mg.c: ret = i2c_master_send(client, (u8 *)&buf, state->mreq_len);
>From a quick look: mreq_len might be 1 in some cases and buf is declared
as __be16[2]. Hmm.
> drivers/iio/pressure/abp060mg.c: ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
> drivers/input/misc/ad714x-i2c.c: error = i2c_master_send(client, (u8 *)chip->xfer_buf,
> drivers/input/misc/ad714x-i2c.c: error = i2c_master_send(client, (u8 *)chip->xfer_buf,
> drivers/input/misc/ad714x-i2c.c: error = i2c_master_recv(client, (u8 *)chip->xfer_buf,
> drivers/input/touchscreen/sx8654.c: len = i2c_master_recv(ts->client, (u8 *)data, readlen);
> drivers/nfc/nfcmrvl/i2c.c: ret = i2c_master_recv(drv_data->i2c, (u8 *)&nci_hdr, NCI_CTRL_HDR_SIZE);
> drivers/nfc/nxp-nci/i2c.c: r = i2c_master_recv(client, (u8 *) &header, NXP_NCI_FW_HDR_LEN);
> drivers/nfc/nxp-nci/i2c.c: r = i2c_master_recv(client, (u8 *) &header, NCI_CTRL_HDR_SIZE);
> drivers/video/fbdev/ssd1307fb.c: ret = i2c_master_send(client, (u8 *)array, len);
>
> I am pretty sure there are more that my quick grep did not catch.
>
> And I agree that I2C itself is a byte-oriented protocol, but the data from the
> point of the driver (once transfer is done) is often more structured.
I think it is fine to require from a caller that they are aware that a
byte (or byte array) must be passed to i2c functions. Given the (maybe)
two problems I pointed out above making it a bit harder to pass non-byte
data to these functions doesn't sound like a bad idea to me.
Obviously your mileage varies, but I personally like having an explicit
type in the API. I guess we have to agree to not agree and let Wolfram
decide if he likes your change or not.
> > > diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> > > index 5c2cc61b666e7..48ed76a0e83d4 100644
> > > --- a/drivers/iio/adc/max1363.c
> > > +++ b/drivers/iio/adc/max1363.c
> >
> > This change isn't motivated in the commit log. Is this here by mistake?
>
> No, it is needed as signature of i2c_master_send/recv has changed.
Ah, I see, there is
st->send = i2c_master_send;
in the code. I think this is worth mentioning in the commit log that it
changes to this file don't look like a mistake as I wondered.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Powered by blists - more mailing lists