lkml.org 
[lkml]   [2022]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor
On Wed, May 11, 2022 at 1:11 PM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>
> Add initial support for ltrf216a ambient light sensor.

> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf

What is the protocol? https? git? ssh?

...

> +obj-$(CONFIG_LTRF216A) += ltrf216a.o

I believe alphabetically it goes after the below one.

> obj-$(CONFIG_LTR501) += ltr501.o

...

> +#include <linux/bitfield.h>

+ bits.h

...

> +static const int ltrf216a_int_time_available[5][2] = {

You may drop 5 here and below, correct?

> + {0, 400000},
> + {0, 200000},
> + {0, 100000},
> + {0, 50000},
> + {0, 25000},
> +};

...

> +/* open air. need to update based on TP transmission rate. */

I didn't get the small letters in conjunction with the period in the
middle of the phrase. Can you update grammar or spell the shortened
form of "air." fully?

> +#define WIN_FAC 1

Can it be namespaced?

...

> +static const struct iio_chan_spec ltrf216a_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_separate_available =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + }

+ Comma.

> +};

...

> +static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
> +{
> + int i, ret, index = -1;

Redundant assignment, and actually not needed variable 'index', see below.

> + u8 reg_val;
> +
> + for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
> + if (ltrf216a_int_time_available[i][1] == itime) {
> + index = i;
> + break;
> + }

if (...)
break;

> + }

> +

You can drop this blank line in order to show the tough connection
between for-loop and if-cond.

> + if (index < 0)
> + return -EINVAL;

if (i == ARRAY_SIZE(...))
return ...

And use i here, which actually can be typed as unsigned int above.

> + reg_val = ltrf216a_int_time_reg[index][1];
> + data->int_time_fac = ltrf216a_int_time_reg[index][0];
> +
> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + data->int_time = itime;
> +
> + return 0;
> +}

...

> +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
> +{
> + int ret = -1, tries = 25;

No need to assign ret. And please avoid assigning returned values to
some negative values that may be misinterpreted as error codes.

> + u8 buf[3];
> +
> + while (tries--) {
> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
> + if (ret < 0)
> + return ret;
> + if (ret & LTRF216A_ALS_DATA_STATUS)
> + break;
> + msleep(20);
> + }

NIH of a macro from iopoll.h. Use the appropriate one.

> + ret = i2c_smbus_read_i2c_block_data(data->client, addr, sizeof(buf), buf);
> + if (ret < 0)
> + return ret;
> +
> + return get_unaligned_le24(&buf[0]);
> +}

...

> + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> + if (greendata < 0 || cleardata < 0)

> + return -EINVAL;

I am expecting that each of them may contain the actual error code,
so, please decouple the condition and return the actual error codes.

...

> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = ltrf216a_get_lux(data);
> + if (ret < 0) {

> + mutex_unlock(&data->mutex);
> + return ret;

Wouldn't 'break' suffice?

> + }
> + *val = ret;
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_CHAN_INFO_INT_TIME:
> + ret = ltrf216a_get_int_time(data, val, val2);
> + break;
> + default:
> + ret = -EINVAL;

Missed break;.

> + }
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;

...

> + ret = ltrf216a_init(indio_dev);
> + if (ret < 0)
> + return dev_err_probe(&client->dev, ret,
> + "ltrf216a chip init failed\n");

One line? Esp. if you create a temporary variable to hold a device pointer.

...

> + ret = devm_add_action_or_reset(&client->dev, als_ltrf216a_disable,
> + indio_dev);

Ditto.

> + if (ret < 0)
> + return ret;

...

> +static struct i2c_driver ltrf216a_driver = {
> + .driver = {
> + .name = LTRF216A_DRV_NAME,
> + .pm = pm_sleep_ptr(&ltrf216a_pm_ops),
> + .of_match_table = ltrf216a_of_match,
> + },
> + .probe_new = ltrf216a_probe,
> + .id_table = ltrf216a_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(ltrf216a_driver);

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2022-05-11 14:07    [W:0.158 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site