Re: [PATCH v4 3/3] iio: light: Add support for ltrf216a sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 11, 2022 at 1:11 PM Shreeya Patel
<shreeya.patel@xxxxxxxxxxxxx> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx>
>
> 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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux