On 10/20/21 9:35 AM, Maslov Dmitry wrote:
Previously ltr501 driver supported a number of light and,
proximity sensors including ltr501, ltr559 and ltr301.
This adds support for another light sensor ltr303
used in Seeed Studio reTerminal, a carrier board
for Raspberry Pi 4 CM.
Hi,
Thanks for the patch. This looks good. But there are a couple of
different things in this change that go beyond what is described in the
commit message. It would be best to split these out into separate
patches and provide a description of what they are doing.
Signed-off-by: Maslov Dmitry <maslovdmitry@xxxxxxxx>
---
drivers/iio/light/ltr501.c | 46 +++++++++++++++++++++++++++++++++++---
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 7e51aaac0bf..733f9224bd1 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * ltr501.c - Support for Lite-On LTR501 ambient light and proximity sensor
+ * ltr.c - Support for Lite-On LTR501, LTR509, LTR301, LTR303 ambient light
The filename did not change. Me personally I'm a big fan of not
referencing the filename in the file, because it is easily outdated when
renamed. So maybe just remove that part of the comment.
+ * and proximity sensors (only LTR5xx)
*
* Copyright 2014 Peter Meerwald <pmeerw@xxxxxxxxxx>
*
[...]
@@ -1231,6 +1241,18 @@ static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
.channels = ltr301_channels,
.no_channels = ARRAY_SIZE(ltr301_channels),
},
+ [ltr303] = {
+ .partid = 0x0A,
+ .als_gain = ltr559_als_gain_tbl,
+ .als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl),
+ .als_mode_active = BIT(0),
+ .als_gain_mask = BIT(2) | BIT(3) | BIT(4),
+ .als_gain_shift = 2,
+ .info = <r301_info,
+ .info_no_irq = <r301_info_no_irq,
+ .channels = ltr301_channels,
+ .no_channels = ARRAY_SIZE(ltr301_channels),
+ },
};
static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val)
@@ -1337,7 +1359,7 @@ static int ltr501_init(struct ltr501_data *data)
if (ret < 0)
return ret;
- data->als_contr = status | data->chip_info->als_mode_active;
+ data->als_contr = status | data->chip_info->als_mode_active | LTR501_ALS_DEF_GAIN;
This is not mentioned in the commit description. Why is this necessary
now, but wasn't before?
ret = regmap_read(data->regmap, LTR501_PS_CONTR, &status);
if (ret < 0)
@@ -1504,7 +1526,23 @@ static int ltr501_probe(struct i2c_client *client,
if (ret < 0)
return ret;
- if (id) {
+ if (client->dev.of_node) {
+ int i = 0;
+
+ chip_idx = (int)of_device_get_match_data(&client->dev);
+ for (i = 0; i < ltr_max; i++) {
+ if (ltr501_id[i].name == NULL) {
+ break;
+ }
+ if (ltr501_id[i].driver_data == chip_idx) {
+ name = ltr501_id[i].name;
+ break;
+ }
+ }
+ if (i >= ltr_max) {
+ name = LTR501_DRV_NAME;
+ }
Same here, this doesn't seem to be 303 specific and there is no mention
of why this is needed in the patch description. There are no other
drivers which seem to do something similar, why is it need for this driver?
+ } else if (id) {
name = id->name;
chip_idx = id->driver_data;
} else if (ACPI_HANDLE(&client->dev)) {
