| Message ID | 20211020073518.3191-1-maslovdmitry@seeed.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: ltr501: Added ltr303 driver support | expand |
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@seeed.cc> > --- > 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@pmeerw.net> > * > [...] > @@ -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)) {
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 + * and proximity sensors (only LTR5xx) * * Copyright 2014 Peter Meerwald <pmeerw@pmeerw.net> * @@ -16,6 +17,8 @@ #include <linux/regmap.h> #include <linux/acpi.h> #include <linux/regulator/consumer.h> +#include <linux/of_device.h> +#include <linux/of.h> #include <linux/iio/iio.h> #include <linux/iio/events.h> @@ -65,11 +68,15 @@ #define LTR501_ALS_DEF_PERIOD 500000 #define LTR501_PS_DEF_PERIOD 100000 +#define LTR501_ALS_DEF_GAIN (BIT(4) | BIT(3) | BIT(2)) + #define LTR501_REGMAP_NAME "ltr501_regmap" #define LTR501_LUX_CONV(vis_coeff, vis_data, ir_coeff, ir_data) \ ((vis_coeff * vis_data) - (ir_coeff * ir_data)) +static const struct i2c_device_id ltr501_id[]; + static const int int_time_mapping[] = {100000, 50000, 200000, 400000}; static const struct reg_field reg_field_it = @@ -98,6 +105,9 @@ enum { ltr501 = 0, ltr559, ltr301, + ltr303, + + ltr_max }; struct ltr501_gain { @@ -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; 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; + } + } else if (id) { name = id->name; chip_idx = id->driver_data; } else if (ACPI_HANDLE(&client->dev)) { @@ -1597,6 +1635,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { {"LTER0501", ltr501}, {"LTER0559", ltr559}, {"LTER0301", ltr301}, + {"LTER0303", ltr303}, { }, }; MODULE_DEVICE_TABLE(acpi, ltr_acpi_match); @@ -1605,6 +1644,7 @@ static const struct i2c_device_id ltr501_id[] = { { "ltr501", ltr501}, { "ltr559", ltr559}, { "ltr301", ltr301}, + { "ltr303", ltr303}, { } }; MODULE_DEVICE_TABLE(i2c, ltr501_id);
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. Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> --- drivers/iio/light/ltr501.c | 46 +++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-)