From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <551C2D58.2040902@linux.intel.com> Date: Wed, 01 Apr 2015 10:39:36 -0700 From: sathyanarayanan kuppuswamy Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com MIME-Version: 1.0 To: Daniel Baluta CC: Jonathan Cameron , Peter Meerwald , "linux-iio@vger.kernel.org" , Srinivas Pandruvada Subject: Re: [PATCH v1 1/1] iio: ltr301: Add support for ltr301 References: In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed List-ID: 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 > 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 = <r501_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 = <r301_info; >> + indio_dev->channels = ltr301_channels; >> + break; >> + case LTR501: >> + indio_dev->info = <r501_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