[PATCH v1 0/1] LTR301 ALS support

[PATCH v1 0/1] LTR301 ALS support

[Kuppuswamy Sathyanarayanan]

Following patch adds support for Liteon 301 Ambient light sensor.

Please let me know your review comments.

v1:
Extended LTR501 driver to support both LTR301 and LTR501 device.

Kuppuswamy Sathyanarayanan (1):
  iio: ltr301: Add support for ltr301

 drivers/iio/light/Kconfig  |  2 +-
 drivers/iio/light/ltr501.c | 50 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH v1 1/1] iio: ltr301: Add support for ltr301

[Kuppuswamy Sathyanarayanan]

Added support for Liteon 301 Ambient light sensor. Since
LTR301 and LTR501 are register compatible(and even have same
part id), LTR501 driver has been extended to support both
devices. LTR501 is similar to LTR301 in ALS sensing, But the
only difference is, LTR501 also supports proximity sensing.

LTR501 - ALS + Proximity combo
LTR301 - ALS sensor.

Following are the data sheets for both devices:

http://optoelectronics.liteon.com/upload/download/DS86-2013-0004/S_110_LTR-303ALS-01_DS_V1.pdf

http://optoelectronics.liteon.com/upload/download/DS86-2012-0006/S_110_LTR-501ALS-01_PrelimDS_ver1[1].pdf

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/iio/light/Kconfig  |  2 +-
 drivers/iio/light/ltr501.c | 50 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index a224afd..4b2ec51 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -159,7 +159,7 @@ config LTR501
 	select IIO_TRIGGERED_BUFFER
 	help
 	 If you say yes here you get support for the Lite-On LTR-501ALS-01
-	 ambient light and proximity sensor.
+	 ambient light and proximity sensor or LTR301 Ambient light sensor.
 
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 62b7072..c113a23 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -45,10 +45,17 @@
 
 #define LTR501_PS_DATA_MASK 0x7ff
 
+enum ltr_chipset {
+	LTR301,
+	LTR501,
+	LTR_MAX_CHIPS /* this must be last */
+};
+
 struct ltr501_data {
 	struct i2c_client *client;
 	struct mutex lock_als, lock_ps;
 	u8 als_contr, ps_contr;
+	u8 chip_id;
 };
 
 static int ltr501_drdy(struct ltr501_data *data, u8 drdy_mask)
@@ -124,6 +131,13 @@ static const struct iio_chan_spec ltr501_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static const struct iio_chan_spec ltr301_channels[] = {
+	LTR501_INTENSITY_CHANNEL(0, LTR501_ALS_DATA0, IIO_MOD_LIGHT_BOTH, 0),
+	LTR501_INTENSITY_CHANNEL(1, LTR501_ALS_DATA1, IIO_MOD_LIGHT_IR,
+		BIT(IIO_CHAN_INFO_SCALE)),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
 static const int ltr501_ps_gain[4][2] = {
 	{1, 0}, {0, 250000}, {0, 125000}, {0, 62500}
 };
@@ -244,10 +258,19 @@ static struct attribute *ltr501_attributes[] = {
 	NULL
 };
 
+static struct attribute *ltr301_attributes[] = {
+	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
+	NULL
+};
+
 static const struct attribute_group ltr501_attribute_group = {
 	.attrs = ltr501_attributes,
 };
 
+static const struct attribute_group ltr301_attribute_group = {
+	.attrs = ltr301_attributes,
+};
+
 static const struct iio_info ltr501_info = {
 	.read_raw = ltr501_read_raw,
 	.write_raw = ltr501_write_raw,
@@ -255,6 +278,13 @@ static const struct iio_info ltr501_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static const struct iio_info ltr301_info = {
+	.read_raw = ltr501_read_raw,
+	.write_raw = ltr501_write_raw,
+	.attrs = &ltr301_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
 static int ltr501_write_contr(struct i2c_client *client, u8 als_val, u8 ps_val)
 {
 	int ret = i2c_smbus_write_byte_data(client, LTR501_ALS_CONTR, als_val);
@@ -347,6 +377,7 @@ static int ltr501_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	data->chip_id = id->driver_data;
 	mutex_init(&data->lock_als);
 	mutex_init(&data->lock_ps);
 
@@ -357,12 +388,24 @@ static int ltr501_probe(struct i2c_client *client,
 		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->modes = INDIO_DIRECT_MODE;
 
+	switch (data->chip_id) {
+	case LTR301:
+		indio_dev->info = &ltr301_info;
+		indio_dev->channels = ltr301_channels;
+		break;
+	case LTR501:
+		indio_dev->info = &ltr501_info;
+		indio_dev->channels = ltr501_channels;
+		break;
+	default:
+		pr_warn("ltr chip invalid\n");
+		return -ENODEV;
+	}
+
 	ret = ltr501_init(data);
 	if (ret < 0)
 		return ret;
@@ -422,7 +465,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 },
+	{ "ltr301", LTR301 },
+	{ "ltr501", LTR501 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ltr501_id);
-- 
1.9.1

Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301

[Daniel Baluta]

On Wed, Apr 1, 2015 at 7:06 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Added support for Liteon 301 Ambient light sensor. Since
> LTR301 and LTR501 are register compatible(and even have same
> part id), LTR501 driver has been extended to support both
> devices. LTR501 is similar to LTR301 in ALS sensing, But the
> only difference is, LTR501 also supports proximity sensing.
>
> LTR501 - ALS + Proximity combo
> LTR301 - ALS sensor.

As I'm adding support for LTR559 sensor, we should agree
on what's the best way to this.

I suggest we should add a ltr501_chip_info for each chip type as
done in [1]

>  static int ltr501_write_contr(struct i2c_client *client, u8 als_val, u8 ps_val)
>  {
>         int ret = i2c_smbus_write_byte_data(client, LTR501_ALS_CONTR, als_val);
> @@ -347,6 +377,7 @@ static int ltr501_probe(struct i2c_client *client,
>         data = iio_priv(indio_dev);
>         i2c_set_clientdata(client, indio_dev);
>         data->client = client;
> +       data->chip_id = id->driver_data;

id can be NULL here if enumerated via ACPI.

>         mutex_init(&data->lock_als);
>         mutex_init(&data->lock_ps);
>
> @@ -357,12 +388,24 @@ static int ltr501_probe(struct i2c_client *client,
>                 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;

Name should be taken via id->name or ACPI.

>         indio_dev->modes = INDIO_DIRECT_MODE;
>
> +       switch (data->chip_id) {
> +       case LTR301:
> +               indio_dev->info = &ltr301_info;
> +               indio_dev->channels = ltr301_channels;
> +               break;
> +       case LTR501:
> +               indio_dev->info = &ltr501_info;
> +               indio_dev->channels = ltr501_channels;
> +               break;
> +       default:
> +               pr_warn("ltr chip invalid\n");
We do have  dev struct here, so we should use dev_warn.

Daniel.

[1]  https://lkml.org/lkml/2015/3/31/198

Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301

[sathyanarayanan kuppuswamy]

Hi Daniel,

Thanks for the review comments.

On 04/01/2015 08:10 AM, Daniel Baluta wrote:
> On Wed, Apr 1, 2015 at 7:06 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Added support for Liteon 301 Ambient light sensor. Since
>> LTR301 and LTR501 are register compatible(and even have same
>> part id), LTR501 driver has been extended to support both
>> devices. LTR501 is similar to LTR301 in ALS sensing, But the
>> only difference is, LTR501 also supports proximity sensing.
>>
>> LTR501 - ALS + Proximity combo
>> LTR301 - ALS sensor.
> As I'm adding support for LTR559 sensor, we should agree
> on what's the best way to this.
>
> I suggest we should add a ltr501_chip_info for each chip type as
> done in [1]
Can you give me the link for your patch set ? If its just chip id , then 
there is no
need for a special structure for it. But if you have more data to be 
stored then it
would make sense.
>
>>   static int ltr501_write_contr(struct i2c_client *client, u8 als_val, u8 ps_val)
>>   {
>>          int ret = i2c_smbus_write_byte_data(client, LTR501_ALS_CONTR, als_val);
>> @@ -347,6 +377,7 @@ static int ltr501_probe(struct i2c_client *client,
>>          data = iio_priv(indio_dev);
>>          i2c_set_clientdata(client, indio_dev);
>>          data->client = client;
>> +       data->chip_id = id->driver_data;
> id can be NULL here if enumerated via ACPI.
I will fix it.
>
>>          mutex_init(&data->lock_als);
>>          mutex_init(&data->lock_ps);
>>
>> @@ -357,12 +388,24 @@ static int ltr501_probe(struct i2c_client *client,
>>                  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;
> Name should be taken via id->name or ACPI.
Agree. But it needs be fixed by a separate patch.
>
>>          indio_dev->modes = INDIO_DIRECT_MODE;
>>
>> +       switch (data->chip_id) {
>> +       case LTR301:
>> +               indio_dev->info = &ltr301_info;
>> +               indio_dev->channels = ltr301_channels;
>> +               break;
>> +       case LTR501:
>> +               indio_dev->info = &ltr501_info;
>> +               indio_dev->channels = ltr501_channels;
>> +               break;
>> +       default:
>> +               pr_warn("ltr chip invalid\n");
> We do have  dev struct here, so we should use dev_warn.
ok. I will fix it.
>
> Daniel.
>
> [1]  https://lkml.org/lkml/2015/3/31/198
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301

[Daniel Baluta]

On Wed, Apr 1, 2015 at 7:06 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Added support for Liteon 301 Ambient light sensor. Since
> LTR301 and LTR501 are register compatible(and even have same
> part id), LTR501 driver has been extended to support both
> devices. LTR501 is similar to LTR301 in ALS sensing, But the
> only difference is, LTR501 also supports proximity sensing.
>
> LTR501 - ALS + Proximity combo
> LTR301 - ALS sensor.
>
> Following are the data sheets for both devices:
>
> http://optoelectronics.liteon.com/upload/download/DS86-2013-0004/S_110_LTR-303ALS-01_DS_V1.pdf
>
> http://optoelectronics.liteon.com/upload/download/DS86-2012-0006/S_110_LTR-501ALS-01_PrelimDS_ver1[1].pdf

One more thing. The datasheet you posted is for LTR303 not LTR301.
Which chip are you adding support for?

If indeed the datasheet link is correct, then although LTR303 and
LTR501 are register compatbile at
a first glance I spotted at least one difference.

The ALS Control register is not the same.

(e.g ALS Gain uses 1 bit for LTR501 and 3 bits for LTR303).

thanks,
Daniel.

Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301

[sathyanarayanan kuppuswamy]

Hi Daniel,

On 04/01/2015 08:34 AM, Daniel Baluta wrote:
> On Wed, Apr 1, 2015 at 7:06 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Added support for Liteon 301 Ambient light sensor. Since
>> LTR301 and LTR501 are register compatible(and even have same
>> part id), LTR501 driver has been extended to support both
>> devices. LTR501 is similar to LTR301 in ALS sensing, But the
>> only difference is, LTR501 also supports proximity sensing.
>>
>> LTR501 - ALS + Proximity combo
>> LTR301 - ALS sensor.
>>
>> Following are the data sheets for both devices:
>>
>> http://optoelectronics.liteon.com/upload/download/DS86-2013-0004/S_110_LTR-303ALS-01_DS_V1.pdf
>>
>> http://optoelectronics.liteon.com/upload/download/DS86-2012-0006/S_110_LTR-501ALS-01_PrelimDS_ver1[1].pdf
> One more thing. The datasheet you posted is for LTR303 not LTR301.
> Which chip are you adding support for?
No. I posted link for the wrong data sheet. I will fix it once I find a 
proper link.

But LTR301 is register compatible with LTR501. I have verified it once 
again.
>
> If indeed the datasheet link is correct, then although LTR303 and
> LTR501 are register compatbile at
> a first glance I spotted at least one difference.
>
> The ALS Control register is not the same.
>
> (e.g ALS Gain uses 1 bit for LTR501 and 3 bits for LTR303).
>
> thanks,
> Daniel.
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301

[Daniel Baluta]

On Wed, Apr 1, 2015 at 8:42 PM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Hi Daniel,
>
> On 04/01/2015 08:34 AM, Daniel Baluta wrote:
>>
>> On Wed, Apr 1, 2015 at 7:06 AM, Kuppuswamy Sathyanarayanan
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>
>>> Added support for Liteon 301 Ambient light sensor. Since
>>> LTR301 and LTR501 are register compatible(and even have same
>>> part id), LTR501 driver has been extended to support both
>>> devices. LTR501 is similar to LTR301 in ALS sensing, But the
>>> only difference is, LTR501 also supports proximity sensing.
>>>
>>> LTR501 - ALS + Proximity combo
>>> LTR301 - ALS sensor.
>>>
>>> Following are the data sheets for both devices:
>>>
>>>
>>> http://optoelectronics.liteon.com/upload/download/DS86-2013-0004/S_110_LTR-303ALS-01_DS_V1.pdf
>>>
>>>
>>> http://optoelectronics.liteon.com/upload/download/DS86-2012-0006/S_110_LTR-501ALS-01_PrelimDS_ver1[1].pdf
>>
>> One more thing. The datasheet you posted is for LTR303 not LTR301.
>> Which chip are you adding support for?
>
> No. I posted link for the wrong data sheet. I will fix it once I find a
> proper link.

Oh, then it's all right.

Here is the link for my patchset:

https://lkml.org/lkml/2015/3/31/197

I guess you can take my first patch:
* iio: light: ltr501: Fix alignment to match open parenthesis

add to your patchset and rebase your changes on it.

Then when I'll receive the LTR501 device I will rebase my other patch
on your changes.

thanks,
Daniel.

Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301

[Daniel Baluta]

On Wed, Apr 1, 2015 at 8:54 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
> On Wed, Apr 1, 2015 at 8:42 PM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Hi Daniel,
>>
>> On 04/01/2015 08:34 AM, Daniel Baluta wrote:
>>>
>>> On Wed, Apr 1, 2015 at 7:06 AM, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>
>>>> Added support for Liteon 301 Ambient light sensor. Since
>>>> LTR301 and LTR501 are register compatible(and even have same
>>>> part id), LTR501 driver has been extended to support both
>>>> devices. LTR501 is similar to LTR301 in ALS sensing, But the
>>>> only difference is, LTR501 also supports proximity sensing.
>>>>
>>>> LTR501 - ALS + Proximity combo
>>>> LTR301 - ALS sensor.
>>>>
>>>> Following are the data sheets for both devices:
>>>>
>>>>
>>>> http://optoelectronics.liteon.com/upload/download/DS86-2013-0004/S_110_LTR-303ALS-01_DS_V1.pdf
>>>>
>>>>
>>>> http://optoelectronics.liteon.com/upload/download/DS86-2012-0006/S_110_LTR-501ALS-01_PrelimDS_ver1[1].pdf
>>>
>>> One more thing. The datasheet you posted is for LTR303 not LTR301.
>>> Which chip are you adding support for?
>>
>> No. I posted link for the wrong data sheet. I will fix it once I find a
>> proper link.
>
> Oh, then it's all right.
>
> Here is the link for my patchset:
>
> https://lkml.org/lkml/2015/3/31/197
>
> I guess you can take my first patch:
> * iio: light: ltr501: Fix alignment to match open parenthesis
>
> add to your patchset and rebase your changes on it.
>
> Then when I'll receive the LTR501 device I will rebase my other patch
> on your changes.

s/LTR501/LTR559