Re: [RFC PATCH 2/2] iio: light: Add support for LTR559 chip

[Jonathan Cameron <jic23@kernel.org>]

On 31/03/15 11:37, Daniel Baluta wrote:
> This device is register compatible with LTR501, with a minor difference for
> ALS control register as showed below:
> 
> ALS Control register for LTR501:
> 
>     7      6      5      4      3      2      1      0
> +------+------+------+------+------+------+------+------+
> |                           |      |      |             |
> |        Reserved           | Gain |  SW  |    ALS Mode |
> |                           |      | Reset|             |
> +------+------+------+------+------+------+------+------+
> 
> ALS Control register for LTR559:
> 
>     7      6      5      4      3      2      1      0
> +------+------+------+------+------+------+------+------+
> |                    |                    |      |      |
> |     Reserved       |        Gain        |  SW  | ALS  |
> |                    |                    | Reset| Mode |
> +------+------+------+------+------+------+------+------+
> 
> We handle this difference by introducing ltr501_chip_info.
> 
> Datasheet for LTR559 is at:
> http://optoelectronics.liteon.com/upload/download/DS86-2013-0003/S_110_LTR-559ALS-01_DS_V1.pdf
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Looks good to me.  Let us know when you've tested it ;)
Anyhow, one comment inline.  It 'think' you can end up with a device probing
succesfully with no known name...

J
> ---
>  drivers/iio/light/ltr501.c | 196 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 163 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 883855a..5939cda 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c
> @@ -45,12 +45,89 @@
>  
>  #define LTR501_PS_DATA_MASK 0x7ff
>  
> +#define LTR501_RESERVED_GAIN -1
> +
> +enum {
> +	ltr501 = 0,
> +	ltr559,
> +};
> +
> +struct ltr501_gain {
> +	int scale;
> +	int uscale;
> +};
> +
> +static struct ltr501_gain ltr501_als_gain_tbl[] = {
> +	{1, 0},
> +	{0, 5000},
> +};
> +
> +static struct ltr501_gain ltr559_als_gain_tbl[] = {
> +	{1, 0},
> +	{0, 500000},
> +	{0, 250000},
> +	{0, 125000},
> +	{LTR501_RESERVED_GAIN, LTR501_RESERVED_GAIN},
> +	{LTR501_RESERVED_GAIN, LTR501_RESERVED_GAIN},
> +	{0, 20000},
> +	{0, 10000},
> +};
> +
> +static struct ltr501_gain ltr501_ps_gain_tbl[] = {
> +	{1, 0},
> +	{0, 250000},
> +	{0, 125000},
> +	{0, 62500},
> +};
> +
> +static struct ltr501_gain ltr559_ps_gain_tbl[] = {
> +	{0, 62500}, /* x16 gain */
> +	{0, 31250}, /* x32 gain */
> +	{0, 15625}, /* bits X1 are for x64 gain */
> +	{0, 15624},
> +};
> +
> +struct ltr501_chip_info {
> +	u8 chip_id;
> +	struct ltr501_gain *als_gain;
> +	int als_gain_tbl_size;
> +	struct ltr501_gain *ps_gain;
> +	int ps_gain_tbl_size;
> +	u8 als_mode_active;
> +	u8 als_gain_mask;
> +	u8 als_gain_shift;
> +};
> +
>  struct ltr501_data {
>  	struct i2c_client *client;
>  	struct mutex lock_als, lock_ps;
> +	struct ltr501_chip_info *chip_info;
>  	u8 als_contr, ps_contr;
>  };
>  
> +static struct ltr501_chip_info ltr501_chip_info_tbl[] = {
> +	[ltr501] = {
> +		.chip_id = 0x08,
> +		.als_gain = ltr501_als_gain_tbl,
> +		.als_gain_tbl_size = ARRAY_SIZE(ltr501_als_gain_tbl),
> +		.ps_gain = ltr501_ps_gain_tbl,
> +		.ps_gain_tbl_size = ARRAY_SIZE(ltr501_ps_gain_tbl),
> +		.als_mode_active = BIT(0) | BIT(1),
> +		.als_gain_mask = BIT(3),
> +		.als_gain_shift = 3,
> +	},
> +	[ltr559] = {
> +		.chip_id = 0x09,
> +		.als_gain = ltr559_als_gain_tbl,
> +		.als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl),
> +		.ps_gain = ltr559_ps_gain_tbl,
> +		.ps_gain_tbl_size = ARRAY_SIZE(ltr559_ps_gain_tbl),
> +		.als_mode_active = BIT(1),
> +		.als_gain_mask = BIT(2) | BIT(3) | BIT(4),
> +		.als_gain_shift = 2,
> +	},
> +};
> +
>  static int ltr501_drdy(struct ltr501_data *data, u8 drdy_mask)
>  {
>  	int tries = 100;
> @@ -125,10 +202,6 @@ static const struct iio_chan_spec ltr501_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
> -static const int ltr501_ps_gain[4][2] = {
> -	{1, 0}, {0, 250000}, {0, 125000}, {0, 62500}
> -};
> -
>  static int ltr501_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
> @@ -166,20 +239,16 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_INTENSITY:
> -			if (data->als_contr & LTR501_CONTR_ALS_GAIN_MASK) {
> -				*val = 0;
> -				*val2 = 5000;
> -				return IIO_VAL_INT_PLUS_MICRO;
> -			} else {
> -				*val = 1;
> -				*val2 = 0;
> -				return IIO_VAL_INT;
> -			}
> +			i = (data->als_contr & data->chip_info->als_gain_mask) >>
> +				data->chip_info->als_gain_shift;
> +			*val = data->chip_info->als_gain[i].scale;
> +			*val2 = data->chip_info->als_gain[i].uscale;
> +			return IIO_VAL_INT_PLUS_MICRO;
>  		case IIO_PROXIMITY:
>  			i = (data->ps_contr & LTR501_CONTR_PS_GAIN_MASK) >>
>  				LTR501_CONTR_PS_GAIN_SHIFT;
> -			*val = ltr501_ps_gain[i][0];
> -			*val2 = ltr501_ps_gain[i][1];
> +			*val = data->chip_info->ps_gain[i].scale;
> +			*val2 = data->chip_info->ps_gain[i].uscale;
>  			return IIO_VAL_INT_PLUS_MICRO;
>  		default:
>  			return -EINVAL;
> @@ -188,12 +257,13 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static int ltr501_get_ps_gain_index(int val, int val2)
> +static int ltr501_get_gain_index(struct ltr501_gain *gain, int size,
> +				 int val, int val2)
>  {
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(ltr501_ps_gain); i++)
> -		if (val == ltr501_ps_gain[i][0] && val2 == ltr501_ps_gain[i][1])
> +	for (i = 0; i < size; i++)
> +		if (val == gain[i].scale && val2 == gain[i].uscale)
>  			return i;
>  
>  	return -1;
> @@ -204,6 +274,7 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
>  			    int val, int val2, long mask)
>  {
>  	struct ltr501_data *data = iio_priv(indio_dev);
> +	struct ltr501_chip_info *info = data->chip_info;
>  	int i;
>  
>  	if (iio_buffer_enabled(indio_dev))
> @@ -213,17 +284,20 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_INTENSITY:
> -			if (val == 0 && val2 == 5000)
> -				data->als_contr |= LTR501_CONTR_ALS_GAIN_MASK;
> -			else if (val == 1 && val2 == 0)
> -				data->als_contr &= ~LTR501_CONTR_ALS_GAIN_MASK;
> -			else
> +			i = ltr501_get_gain_index(info->als_gain,
> +						  info->als_gain_tbl_size,
> +						  val, val2);
> +			if (i < 0)
>  				return -EINVAL;
> +			data->als_contr &= ~info->als_gain_mask;
> +			data->als_contr |= i << info->als_gain_shift;
>  			return i2c_smbus_write_byte_data(data->client,
>  							 LTR501_ALS_CONTR,
>  							 data->als_contr);
>  		case IIO_PROXIMITY:
> -			i = ltr501_get_ps_gain_index(val, val2);
> +			i = ltr501_get_gain_index(info->ps_gain,
> +						  info->ps_gain_tbl_size,
> +						  val, val2);
>  			if (i < 0)
>  				return -EINVAL;
>  			data->ps_contr &= ~LTR501_CONTR_PS_GAIN_MASK;
> @@ -238,12 +312,58 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static IIO_CONST_ATTR(in_proximity_scale_available, "1 0.25 0.125 0.0625");
> -static IIO_CONST_ATTR(in_intensity_scale_available, "1 0.005");
> +static ssize_t ltr501_show_proximity_scale_avail(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct ltr501_data *data = iio_priv(dev_to_iio_dev(dev));
> +	struct ltr501_chip_info *info = data->chip_info;
> +	ssize_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < info->ps_gain_tbl_size; i++) {
> +		if (info->ps_gain[i].scale == LTR501_RESERVED_GAIN)
> +			continue;
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
> +				 info->ps_gain[i].scale,
> +				 info->ps_gain[i].uscale);
> +	}
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static ssize_t ltr501_show_intensity_scale_avail(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct ltr501_data *data = iio_priv(dev_to_iio_dev(dev));
> +	struct ltr501_chip_info *info = data->chip_info;
> +	ssize_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < info->als_gain_tbl_size; i++) {
> +		if (info->als_gain[i].scale == LTR501_RESERVED_GAIN)
> +			continue;
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
> +				 info->als_gain[i].scale,
> +				 info->als_gain[i].uscale);
> +	}
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(in_proximity_scale_available, S_IRUGO,
> +		       ltr501_show_proximity_scale_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(in_intensity_scale_available, S_IRUGO,
> +		       ltr501_show_intensity_scale_avail, NULL, 0);
>  
>  static struct attribute *ltr501_attributes[] = {
> -	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
> -	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_proximity_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_intensity_scale_available.dev_attr.attr,
>  	NULL
>  };
>  
> @@ -326,7 +446,7 @@ static int ltr501_init(struct ltr501_data *data)
>  	ret = i2c_smbus_read_byte_data(data->client, LTR501_ALS_CONTR);
>  	if (ret < 0)
>  		return ret;
> -	data->als_contr = ret | LTR501_CONTR_ACTIVE;
> +	data->als_contr = ret | data->chip_info->als_mode_active;
>  
>  	ret = i2c_smbus_read_byte_data(data->client, LTR501_PS_CONTR);
>  	if (ret < 0)
> @@ -342,6 +462,8 @@ static int ltr501_probe(struct i2c_client *client,
>  {
>  	struct ltr501_data *data;
>  	struct iio_dev *indio_dev;
> +	const char *name = NULL;
> +	int chip_id = 0;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> @@ -354,17 +476,24 @@ static int ltr501_probe(struct i2c_client *client,
>  	mutex_init(&data->lock_als);
>  	mutex_init(&data->lock_ps);
>  
> +	if (id) {
> +		name = id->name;
> +		chip_id = id->driver_data;
> +	}
> +
> +	data->chip_info = &ltr501_chip_info_tbl[chip_id];
> +
>  	ret = i2c_smbus_read_byte_data(data->client, LTR501_PART_ID);
>  	if (ret < 0)
>  		return ret;
> -	if ((ret >> 4) != 0x8)
> +	if ((ret >> 4) != data->chip_info->chip_id)
>  		return -ENODEV;
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &ltr501_info;
>  	indio_dev->channels = ltr501_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ltr501_channels);
> -	indio_dev->name = LTR501_DRV_NAME;
> +	indio_dev->name = name;
Can name still be NULL?  Needs a default value that makes sense.
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	ret = ltr501_init(data);
> @@ -390,7 +519,7 @@ error_unreg_buffer:
>  static int ltr501_powerdown(struct ltr501_data *data)
>  {
>  	return ltr501_write_contr(data->client,
> -				  data->als_contr & ~LTR501_CONTR_ACTIVE,
> +				  data->als_contr & ~data->chip_info->als_mode_active,
>  				  data->ps_contr & ~LTR501_CONTR_ACTIVE);
>  }
>  
> @@ -426,7 +555,8 @@ static int ltr501_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(ltr501_pm_ops, ltr501_suspend, ltr501_resume);
>  
>  static const struct i2c_device_id ltr501_id[] = {
> -	{ "ltr501", 0 },
> +	{ "ltr501", ltr501},
> +	{ "ltr559", ltr559},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ltr501_id);
>