| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 May 2022 14:04:50 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Shreeya Patel <shreeya.patel@...labora.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>, Zhigang.Shi@...eon.com,
krisman@...labora.com, linux-iio <linux-iio@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Collabora Kernel ML <kernel@...labora.com>,
alvaro.soliverez@...labora.com
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@...labora.com> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@...eon.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
Powered by blists - more mailing lists