On Fri, 18 Aug 2023 11:30:03 +0100
Lee Jones <lee@xxxxxxxxxx> wrote:
> > -static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
> > +static int omnia_cmd_read_raw(struct i2c_adapter *adapter, u8 addr, u8 cmd,
> > + void *reply, size_t len)
> > {
> > struct i2c_msg msgs[2];
> > - u8 reply;
> > int ret;
> >
> > - msgs[0].addr = client->addr;
> > + msgs[0].addr = addr;
> > msgs[0].flags = 0;
> > msgs[0].len = 1;
> > msgs[0].buf = &cmd;
> > - msgs[1].addr = client->addr;
> > + msgs[1].addr = addr;
> > msgs[1].flags = I2C_M_RD;
> > - msgs[1].len = 1;
> > - msgs[1].buf = &reply;
> > + msgs[1].len = len;
> > + msgs[1].buf = reply;
> >
> > - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > if (likely(ret == ARRAY_SIZE(msgs)))
> > - return reply;
> > + return 0;
>
> Why not return ret and use that throughout?
>
> > else if (ret < 0)
> > return ret;
> > else
> > return -EIO;
> > }
>
As I explained to your similar question in your reply to patch 2/6
https://lore.kernel.org/linux-leds/20230821120136.130cc916@dellmb/T/#me5782c9896bc3d994cb36e8b5485d6368cd92da0
the reason I normalize to zero is because of the intended semantics of
these functions: return 0 on success, -errno on error.
If instead in omnia_cmd_read() and omnia_cmd_write() I simply return
the return value of the underlying functions, which are
i2c_transfer() for omnia_cmd_read()
i2c_master_send() for omnia_cmd_write()
the semantics would be inconsistent, because:
i2c_transfer() returns the number of successful I2C transfers,
i2c_manster_send() returns the number of written bytes over I2C.
Nonetheless, if you insist on this change, I will do it. Reluctantly
and with a silent protest, but I will not argue further, since I want
this functionality in upstream more than to argue over nitpicks :-)
Marek
