Re: [PATCH v3 6/6] leds: turris-omnia: add support for enabling/disabling HW gamma correction

[Lee Jones <lee@kernel.org>]

On Mon, 21 Aug 2023, Marek Behún wrote:

> On Fri, 18 Aug 2023 11:30:03 +0100
> Lee Jones <lee@kernel.org> 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 :-)

API semantics is not nitpicking.  Besides, we have time to talk.  We are
too late in the cycle for this to get merge this side of the merge
window anyway.

Read and write APIs, even abstracted ones such as these generally return
the number of bytes successfully read and written respectively.  If you
are going to normalise, then please do so against this standard.

-- 
Lee Jones [李琼斯]