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
