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

[sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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