Re: [PATCH -next v3 1/2] i2c: add SCCB helpers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Wolfram,

On Tuesday, 10 July 2018 15:07:47 EEST Wolfram Sang wrote:
> >> +static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
> >> +{
> >> +	int ret;
> >> +	union i2c_smbus_data data;
> >> +
> >> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> >> +
> >> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> >> +				I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
> >> +	if (ret < 0)
> >> +		goto out;
> >> +
> >> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> >> +				I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
> >> +out:
> >> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> >> +
> >> +	return ret < 0 ? ret : data.byte;
> >> +}
> > 
> > I think I mentioned in a previous review of this patch that the function
> > is too big to be a static inline. It should instead be moved to a .c file.
> 
> Can be argued.

Especially if sccb_read_byte() is called in multiple places in a driver, not 
just once in a read helper, as you've advised for patch 2/2 in this series :-)

> I assume just removing the 'inline' won't do it for you?

Just removing the inline keyword will create many instances of the function, 
even when not used. I think it will also cause the compiler to emit warnings 
for unused functions. I don't think that's a good idea.

> I'd be fine with that, there are not many SCCB useres out there...
> 
> But if you insist on drivers/i2c/i2c-sccb.c, then it should be a
> seperate module, I'd think?

Given how small the functions are, I wouldn't request that, as it would 
introduce another Kconfig symbol, but I'm not opposed to such a new module 
either.

-- 
Regards,

Laurent Pinchart