Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 11 May 2022 14:04:50 +0200 | Subject | Re: [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(<rf216a_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
| |