On Tue, Nov 12, 2019 at 09:45:56AM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
>
> On 12/11/19 01:58, 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.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>
> I agree on the principle, but I have a question, see below.
>
> [...]
> > s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
> > - u8 command, u8 length, u8 *values)
> > + u8 command, u8 length, void *values)
> > {
> > u8 i = 0;
> > int status;
> > @@ -647,8 +648,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
> > status = i2c_smbus_read_word_data(client, command + i);
> > if (status < 0)
> > return status;
> > - values[i] = status & 0xff;
> > - values[i + 1] = status >> 8;
> > + put_unaligned_le16(status, values + i);
>
> The switch to put_unaligned_le16() looks unrelated, is it?
Yeah, I'll split it out.
>
> > i += 2;
> > }
> > }
> > @@ -657,7 +657,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
> > status = i2c_smbus_read_byte_data(client, command + i);
> > if (status < 0)
> > return status;
> > - values[i] = status;
> > + *(u8 *)(values + i) = status;
>
> My preference is to use an u8* helper variable in these cases:
Sure, I can do this.
>
> s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client
> *client,
> - u8 command, u8 length, u8 *values)
> + u8 command, u8 length, void *buf)
> {
> + u8 *bytes = buf;
> @@
> - values[i] = status;
> + bytes[i] = status;
>
> This clarifies we are accessing the raw bytes, avoids typecasts in the
> middle of code for readability and avoids void pointer math.
>
> PS: look, it's exactly what you do in the max1363.c file below! :)
>
> > 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
> > @@ -182,9 +182,9 @@ struct max1363_state {
> > struct regulator *vref;
> > u32 vref_uv;
> > int (*send)(const struct i2c_client *client,
> > - const char *buf, int count);
> > + const void *buf, int count);
> > int (*recv)(const struct i2c_client *client,
> > - char *buf, int count);
> > + void *buf, int count);
> > };
> >
> > #define MAX1363_MODE_SINGLE(_num, _mask) { \
> > @@ -310,27 +310,29 @@ static const struct max1363_mode
> > return NULL;
> > }
> >
> > -static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
> > +static int max1363_smbus_send(const struct i2c_client *client, const void *buf,
> > int count)
> > {
> > + const u8 *data = buf;
>
> Here it is! ^
>
Thanks for the review!
--
Dmitry
