Re: [PATCH 1/3] light sensor: Add SMBUS support to the tsl2563 driver.

Re: [PATCH 1/3] light sensor: Add SMBUS support to the tsl2563 driver.

[Jonathan Cameron]

On 06/21/11 23:54, Bryan Freed wrote:
> This is so we can support it on x86 SMBUS adapters.
Hi Brian,

Please cc linux-iio@vger.kernel.org for iio patches.  Also this driver has fairly
clear authorship at the top, so more for your cc list. (added)
> 
> Signed-off-by: Bryan Freed <bfreed@chromium.org>
> ---
>  drivers/staging/iio/light/tsl2563.c |   29 ++++++++++++++++++++++++++++-
>  1 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
> index 9cffa2e..04aa155 100644
> --- a/drivers/staging/iio/light/tsl2563.c
> +++ b/drivers/staging/iio/light/tsl2563.c
> @@ -137,6 +137,8 @@ struct tsl2563_chip {
>  	u32			data1;
>  };
>  
> +static int use_smbus;
> +
>  static int tsl2563_write(struct i2c_client *client, u8 reg, u8 value)
>  {
>  	int ret;
> @@ -145,15 +147,35 @@ static int tsl2563_write(struct i2c_client *client, u8 reg, u8 value)
>  	buf[0] = TSL2563_CMD | reg;
>  	buf[1] = value;
>  
> +	if (use_smbus) {
> +		ret = i2c_smbus_write_byte_data(client, buf[0], value);
> +		return ret;
> +	}
> +
Here I'd prefer to see this in an else block to make the program flow clear. Same with the others.
Actually, is there any reason we can't use the smbus_write_byte_data for all cases?  I 'think'
it's emulated via i2c if that is available and smbus is not? (cc'd Jean to confirm this - though
a quick code browse of i2c-core.c looks promising.)

If smbus_xfer is not supplied by the adapter, i2c_smbus_xfer_emulated is called. 

The only possible issue I can think of is that a device supports full i2c + a partial smbus
support. (rather odd!)
>  	ret = i2c_master_send(client, buf, sizeof(buf));
>  	return (ret == sizeof(buf)) ? 0 : ret;
>  }
>  
> -static int tsl2563_read(struct i2c_client *client, u8 reg, void *buf, int len)
> +static int tsl2563_read(struct i2c_client *client, u8 reg, u8 *buf, int len)
>  {
>  	int ret;
>  	u8 cmd = TSL2563_CMD | reg;
>  
> +	if (use_smbus) {
> +		if (len == 1) {
> +			ret = i2c_smbus_read_byte_data(client, cmd);
> +			buf[0] = ret & 0xff;
> +		} else if (len == 2) {
> +			ret = i2c_smbus_read_word_data(client, cmd);
> +			buf[0] = ret & 0xff;
> +			buf[1] = (ret >> 8) & 0xff;
> +		} else
> +			ret = -1;
If we hit this something has gone hideously wrong.  Hence please
audit the driver to be sure this doesn't happen and don't bother
with this extra option.
> +		if (ret < 0)
> +			return 0; /* failure */
Please return the error, not 0 in event of failure.
> +		return len; /* success */
> +	}
> +
>  	ret = i2c_master_send(client, &cmd, sizeof(cmd));
>  	if (ret != sizeof(cmd))
>  		return ret;
> @@ -712,6 +734,11 @@ static int __devinit tsl2563_probe(struct i2c_client *client,
>  	int err = 0;
>  	int ret;
>  	u8 id;
> +	u32 smbus_func = I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA;
Can this be const?  Oddly, the answer looks to be no.  Given its an inline
in i2c.h, can't see why this one isn't const.  Jean, am I missing something
or wouldn't:
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a6c652e..be5515d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -458,7 +458,7 @@ static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
 }
 
 /* Return 1 if adapter supports everything we need, 0 if not. */
-static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
+static inline int i2c_check_functionality(struct i2c_adapter *adap, const u32 func)
 {
        return (func & i2c_get_functionality(adap)) == func;
 }

Be a sensible change?  For that matter, this code should probably just have that
value inline in the function call as it isn't used anywhere else.

> +	/* We support both I2C and SMBUS adapter interfaces. */
> +	if (i2c_check_functionality(client->adapter, smbus_func))
> +		use_smbus = 1;
>  
>  	indio_dev = iio_allocate_device(sizeof(*chip));
>  	if (!indio_dev)

Re: [PATCH 1/3] light sensor: Add SMBUS support to the tsl2563 driver.

[Jean Delvare]

On Wed, 22 Jun 2011 10:05:44 +0100, Jonathan Cameron wrote:
> On 06/21/11 23:54, Bryan Freed wrote:
> > This is so we can support it on x86 SMBUS adapters.
> Hi Brian,
> 
> Please cc linux-iio@vger.kernel.org for iio patches.  Also this driver has fairly
> clear authorship at the top, so more for your cc list. (added)
> > 
> > Signed-off-by: Bryan Freed <bfreed@chromium.org>
> > ---
> >  drivers/staging/iio/light/tsl2563.c |   29 ++++++++++++++++++++++++++++-
> >  1 files changed, 28 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
> > index 9cffa2e..04aa155 100644
> > --- a/drivers/staging/iio/light/tsl2563.c
> > +++ b/drivers/staging/iio/light/tsl2563.c
> > @@ -137,6 +137,8 @@ struct tsl2563_chip {
> >  	u32			data1;
> >  };
> >  
> > +static int use_smbus;

Global variables are bad.

> > +
> >  static int tsl2563_write(struct i2c_client *client, u8 reg, u8 value)
> >  {
> >  	int ret;
> > @@ -145,15 +147,35 @@ static int tsl2563_write(struct i2c_client *client, u8 reg, u8 value)
> >  	buf[0] = TSL2563_CMD | reg;
> >  	buf[1] = value;
> >  
> > +	if (use_smbus) {
> > +		ret = i2c_smbus_write_byte_data(client, buf[0], value);
> > +		return ret;
> > +	}
> > +
> Here I'd prefer to see this in an else block to make the program flow clear. Same with the others.
> Actually, is there any reason we can't use the smbus_write_byte_data for all cases?  I 'think'
> it's emulated via i2c if that is available and smbus is not? (cc'd Jean to confirm this - though
> a quick code browse of i2c-core.c looks promising.)
> 
> If smbus_xfer is not supplied by the adapter, i2c_smbus_xfer_emulated is called. 

You are totally right. Just use smbus all the time and be done with it.

> The only possible issue I can think of is that a device supports full i2c + a partial smbus
> support. (rather odd!)

That would be up to the underlying I2C bus driver to handle, not the
I2C device drivers individually.

> >  	ret = i2c_master_send(client, buf, sizeof(buf));
> >  	return (ret == sizeof(buf)) ? 0 : ret;
> >  }
> >  
> > -static int tsl2563_read(struct i2c_client *client, u8 reg, void *buf, int len)
> > +static int tsl2563_read(struct i2c_client *client, u8 reg, u8 *buf, int len)
> >  {
> >  	int ret;
> >  	u8 cmd = TSL2563_CMD | reg;
> >  
> > +	if (use_smbus) {
> > +		if (len == 1) {
> > +			ret = i2c_smbus_read_byte_data(client, cmd);
> > +			buf[0] = ret & 0xff;
> > +		} else if (len == 2) {
> > +			ret = i2c_smbus_read_word_data(client, cmd);
> > +			buf[0] = ret & 0xff;
> > +			buf[1] = (ret >> 8) & 0xff;

swab16 is your friend (and the bogus SMBus byte order convention be
damned.)

> > +		} else
> > +			ret = -1;
> If we hit this something has gone hideously wrong.  Hence please
> audit the driver to be sure this doesn't happen and don't bother
> with this extra option.

+1 In fact the whole tsl2563_read interface should be revisited.
Passing the length as a parameter makes no sense when using the SMBus
API. Just have tsl2563_read8() which reads a byte and tsl2563_read16()
which reads a word, this will be much more efficient, and you get
proper type checking for free.

> > +		if (ret < 0)
> > +			return 0; /* failure */
> Please return the error, not 0 in event of failure.
> > +		return len; /* success */
> > +	}
> > +
> >  	ret = i2c_master_send(client, &cmd, sizeof(cmd));
> >  	if (ret != sizeof(cmd))
> >  		return ret;
> > @@ -712,6 +734,11 @@ static int __devinit tsl2563_probe(struct i2c_client *client,
> >  	int err = 0;
> >  	int ret;
> >  	u8 id;
> > +	u32 smbus_func = I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA;
> Can this be const?  Oddly, the answer looks to be no.  Given its an inline

I see no reason why not.

> in i2c.h, can't see why this one isn't const.  Jean, am I missing something
> or wouldn't:
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a6c652e..be5515d 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -458,7 +458,7 @@ static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
>  }
>  
>  /* Return 1 if adapter supports everything we need, 0 if not. */
> -static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
> +static inline int i2c_check_functionality(struct i2c_adapter *adap, const u32 func)
>  {
>         return (func & i2c_get_functionality(adap)) == func;
>  }
> 
> Be a sensible change?

This is technically correct, as the function doesn't modify the
parameter, however this change has no effect on the caller. func is
passed by value, not address, so the caller doesn't care at all whether
i2c_check_functionality modifies it locally or not.

There are a lot of places where such by-value parameters could be made
const, however nobody bothers, because it makes the function
declarations harder to read for virtually no benefit (as opposed to
const pointers, which do have value for the caller.)

> For that matter, this code should probably just have that
> value inline in the function call as it isn't used anywhere else.

+1

> 
> > +	/* We support both I2C and SMBUS adapter interfaces. */
> > +	if (i2c_check_functionality(client->adapter, smbus_func))
> > +		use_smbus = 1;
> >  
> >  	indio_dev = iio_allocate_device(sizeof(*chip));
> >  	if (!indio_dev)
> 


-- 
Jean Delvare

Re: [PATCH 1/3] light sensor: Add SMBUS support to the tsl2563 driver.

[Jonathan Cameron]

On 06/22/11 15:10, Jean Delvare wrote:
> On Wed, 22 Jun 2011 10:05:44 +0100, Jonathan Cameron wrote:
>> On 06/21/11 23:54, Bryan Freed wrote:
>>> This is so we can support it on x86 SMBUS adapters.
>> Hi Brian,
>>
>> Please cc linux-iio@vger.kernel.org for iio patches.  Also this driver has fairly
>> clear authorship at the top, so more for your cc list. (added)
>>>
>>> Signed-off-by: Bryan Freed <bfreed@chromium.org>
>>> ---
>>>  drivers/staging/iio/light/tsl2563.c |   29 ++++++++++++++++++++++++++++-
>>>  1 files changed, 28 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
>>> index 9cffa2e..04aa155 100644
>>> --- a/drivers/staging/iio/light/tsl2563.c
>>> +++ b/drivers/staging/iio/light/tsl2563.c
>>> @@ -137,6 +137,8 @@ struct tsl2563_chip {
>>>  	u32			data1;
>>>  };
>>>  
>>> +static int use_smbus;
> 
> Global variables are bad.
+1
> 
>>> +
>>>  static int tsl2563_write(struct i2c_client *client, u8 reg, u8 value)
>>>  {
>>>  	int ret;
>>> @@ -145,15 +147,35 @@ static int tsl2563_write(struct i2c_client *client, u8 reg, u8 value)
>>>  	buf[0] = TSL2563_CMD | reg;
>>>  	buf[1] = value;
>>>  
>>> +	if (use_smbus) {
>>> +		ret = i2c_smbus_write_byte_data(client, buf[0], value);
>>> +		return ret;
>>> +	}
>>> +
>> Here I'd prefer to see this in an else block to make the program flow clear. Same with the others.
>> Actually, is there any reason we can't use the smbus_write_byte_data for all cases?  I 'think'
>> it's emulated via i2c if that is available and smbus is not? (cc'd Jean to confirm this - though
>> a quick code browse of i2c-core.c looks promising.)
>>
>> If smbus_xfer is not supplied by the adapter, i2c_smbus_xfer_emulated is called. 
> 
> You are totally right. Just use smbus all the time and be done with it.
> 
>> The only possible issue I can think of is that a device supports full i2c + a partial smbus
>> support. (rather odd!)
> 
> That would be up to the underlying I2C bus driver to handle, not the
> I2C device drivers individually.
Cool. Good to have that confirmed.
> 
>>>  	ret = i2c_master_send(client, buf, sizeof(buf));
>>>  	return (ret == sizeof(buf)) ? 0 : ret;
>>>  }
>>>  
>>> -static int tsl2563_read(struct i2c_client *client, u8 reg, void *buf, int len)
>>> +static int tsl2563_read(struct i2c_client *client, u8 reg, u8 *buf, int len)
>>>  {
>>>  	int ret;
>>>  	u8 cmd = TSL2563_CMD | reg;
>>>  
>>> +	if (use_smbus) {
>>> +		if (len == 1) {
>>> +			ret = i2c_smbus_read_byte_data(client, cmd);
>>> +			buf[0] = ret & 0xff;
>>> +		} else if (len == 2) {
>>> +			ret = i2c_smbus_read_word_data(client, cmd);
>>> +			buf[0] = ret & 0xff;
>>> +			buf[1] = (ret >> 8) & 0xff;
> 
> swab16 is your friend (and the bogus SMBus byte order convention be
> damned.)
I would imagine the be16_to_cpu etc is even better as we don't know the
cpu endianess. 
> 
>>> +		} else
>>> +			ret = -1;
>> If we hit this something has gone hideously wrong.  Hence please
>> audit the driver to be sure this doesn't happen and don't bother
>> with this extra option.
> 
> +1 In fact the whole tsl2563_read interface should be revisited.
> Passing the length as a parameter makes no sense when using the SMBus
> API. Just have tsl2563_read8() which reads a byte and tsl2563_read16()
> which reads a word, this will be much more efficient, and you get
> proper type checking for free.
Agreed.
> 
>>> +		if (ret < 0)
>>> +			return 0; /* failure */
>> Please return the error, not 0 in event of failure.
>>> +		return len; /* success */
>>> +	}
>>> +
>>>  	ret = i2c_master_send(client, &cmd, sizeof(cmd));
>>>  	if (ret != sizeof(cmd))
>>>  		return ret;
>>> @@ -712,6 +734,11 @@ static int __devinit tsl2563_probe(struct i2c_client *client,
>>>  	int err = 0;
>>>  	int ret;
>>>  	u8 id;
>>> +	u32 smbus_func = I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA;
>> Can this be const?  Oddly, the answer looks to be no.  Given its an inline
> 
> I see no reason why not.
oops. as below, I was clearly asleep this morning...
> 
>> in i2c.h, can't see why this one isn't const.  Jean, am I missing something
>> or wouldn't:
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index a6c652e..be5515d 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -458,7 +458,7 @@ static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
>>  }
>>  
>>  /* Return 1 if adapter supports everything we need, 0 if not. */
>> -static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
>> +static inline int i2c_check_functionality(struct i2c_adapter *adap, const u32 func)
>>  {
>>         return (func & i2c_get_functionality(adap)) == func;
>>  }
>>
>> Be a sensible change?
> 
> This is technically correct, as the function doesn't modify the
> parameter, however this change has no effect on the caller. func is
> passed by value, not address, so the caller doesn't care at all whether
> i2c_check_functionality modifies it locally or not.
Doh. I'm half asleep today and failed to notice it wasn't a pointer.
> 
> There are a lot of places where such by-value parameters could be made
> const, however nobody bothers, because it makes the function
> declarations harder to read for virtually no benefit (as opposed to
> const pointers, which do have value for the caller.)
> 
>> For that matter, this code should probably just have that
>> value inline in the function call as it isn't used anywhere else.
> 
> +1
> 
>>
>>> +	/* We support both I2C and SMBUS adapter interfaces. */
>>> +	if (i2c_check_functionality(client->adapter, smbus_func))
>>> +		use_smbus = 1;
>>>  
>>>  	indio_dev = iio_allocate_device(sizeof(*chip));
>>>  	if (!indio_dev)
>>
> 
> 

Re: [PATCH 1/3] light sensor: Add SMBUS support to the tsl2563 driver.

[Jean Delvare]

On Wed, 22 Jun 2011 16:53:48 +0100, Jonathan Cameron wrote:
> On 06/22/11 15:10, Jean Delvare wrote:
> > On Wed, 22 Jun 2011 10:05:44 +0100, Jonathan Cameron wrote:
> >> On 06/21/11 23:54, Bryan Freed wrote:
> >>> +	if (use_smbus) {
> >>> +		if (len == 1) {
> >>> +			ret = i2c_smbus_read_byte_data(client, cmd);
> >>> +			buf[0] = ret & 0xff;
> >>> +		} else if (len == 2) {
> >>> +			ret = i2c_smbus_read_word_data(client, cmd);
> >>> +			buf[0] = ret & 0xff;
> >>> +			buf[1] = (ret >> 8) & 0xff;
> > 
> > swab16 is your friend (and the bogus SMBus byte order convention be
> > damned.)
>
> I would imagine the be16_to_cpu etc is even better as we don't know the
> cpu endianess.

No, it has to do with SMBus endianess vs. I2C chip endianess, not host
endianess. SMBus specifies that the low byte of a word goes on the wire
first, but almost all I2C and SMBus devices I've see implement it the
other way around, high byte first. i2c-core's
i2c_smbus_read_word_data() stick to the specs, so for almost all I2C
devices, the driver has to swab16 the result.

(I didn't check the TSL2563 datasheet though... maybe this is one of
the rare chips that don't need it.)

-- 
Jean Delvare

Re: [PATCH 2/3] light sensor: Fix a panic in the tsl2563 driver.

[Jonathan Cameron]

> Add a wrapper for this driver around the IIO_CHAN() wrapper to make channel
> parameters more readable.  This fixes a panic caused by the info_masks being
> accidentally passed in as channel2 parameters which easily surpass the size
> of the iio_modifier_names_light array.
Good change (particularly as it's my bug in the first place - oops!).

Thanks,
> 
> Signed-off-by: Bryan Freed <bfreed@chromium.org>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

> ---
>  drivers/staging/iio/light/tsl2563.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
> index 04aa155..d9be19c 100644
> --- a/drivers/staging/iio/light/tsl2563.c
> +++ b/drivers/staging/iio/light/tsl2563.c
> @@ -570,15 +570,16 @@ error_ret:
>  	return ret;
>  }
>  
> +#define INFO_MASK (1 << IIO_CHAN_INFO_CALIBSCALE_SEPARATE)
> +#define EVENT_MASK (IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING) | \
> +		    IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING))
> +#define IIO_CHAN_2563(type, mod, proc, chan, imask, emask) \
> +	IIO_CHAN(type, mod, 1, proc, NULL, chan, 0, imask, 0, 0, {}, emask)
> +
>  static const struct iio_chan_spec tsl2563_channels[] = {
> -	IIO_CHAN(IIO_LIGHT, 0, 1, 1, NULL, 0, 0, 0, 0, 0, {}, 0),
> -	IIO_CHAN(IIO_INTENSITY, 1, 1, 0, "both", 0,
> -		 (1 << IIO_CHAN_INFO_CALIBSCALE_SEPARATE), 0, 0, 0, {},
> -		 IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING) |
> -		 IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING)),
> -	IIO_CHAN(IIO_INTENSITY, 1, 1, 0, "ir", 1,
> -		 (1 << IIO_CHAN_INFO_CALIBSCALE_SEPARATE), 0, 0, 0, {},
> -		 0)
> +	IIO_CHAN_2563(IIO_LIGHT,     0, 1, 0, 0, 0),
> +	IIO_CHAN_2563(IIO_INTENSITY, 1, 0, 0, INFO_MASK, EVENT_MASK),
> +	IIO_CHAN_2563(IIO_INTENSITY, 1, 0, 1, INFO_MASK, 0),
>  };
>  
>  static int tsl2563_read_thresh(struct iio_dev *indio_dev,

Re: [PATCH 3/3] light sensor: Fix a panic in the tsl2563 driver.

[Jonathan Cameron]

On 06/21/11 23:54, Bryan Freed wrote:
> Add the required read/write_raw functions to the tsl2563_info_no_irq data
> structure.  This structure is used insted of tsl2563_info when the I2C client
> has no IRQ.
> The absence of these functions causes a panic when reading or writing the
> created sysfs files.
Yikes.  Good spot.  I'd completely forgotten that no_irq version was there.

Greg, please pick these two up as fixes.  Sorry for the rubbish
conversion in the first place!
> 
> Signed-off-by: Bryan Freed <bfreed@chromium.org>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/light/tsl2563.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
> index d9be19c..bca99f1 100644
> --- a/drivers/staging/iio/light/tsl2563.c
> +++ b/drivers/staging/iio/light/tsl2563.c
> @@ -713,6 +713,8 @@ static struct i2c_driver tsl2563_i2c_driver;
>  
>  static const struct iio_info tsl2563_info_no_irq = {
>  	.driver_module = THIS_MODULE,
> +	.read_raw = &tsl2563_read_raw,
> +	.write_raw = &tsl2563_write_raw,
>  };
>  
>  static const struct iio_info tsl2563_info = {