| Message ID | 20240705091222.86916-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: iio: light: ltrf216a: Document LTR-308 support | expand |
On 05/07/2024 11:11, Marek Vasut wrote: > Add LiteOn LTR-308 support into LTR-F216A kernel driver. > > The two devices seem to have almost identical register map, except that > the LTR-308 does not have three CLEAR_DATA registers, which are unused > by this driver. Furthermore, LTR-308 and LTR-F216A use different lux > calculation constants, 0.6 and 0.45 respectively. Both differences are > handled using chip info data. > > https://optoelectronics.liteon.com/upload/download/DS86-2016-0027/LTR-308ALS_Final_%20DS_V1%201.pdf > https://optoelectronics.liteon.com/upload/download/DS86-2019-0016/LTR-F216A_Final_DS_V1.4.PDF > > Signed-off-by: Marek Vasut <marex@denx.de> > --- ... > mutex_init(&data->lock); > > @@ -520,15 +537,27 @@ static int ltrf216a_runtime_resume(struct device *dev) > static DEFINE_RUNTIME_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_runtime_suspend, > ltrf216a_runtime_resume, NULL); > > +struct ltr_chip_info ltr308_chip_info = { static const > + .has_clear_data = false, > + .lux_multiplier = 60, > +}; > + > +struct ltr_chip_info ltrf216a_chip_info = { static const > + .has_clear_data = true, > + .lux_multiplier = 45, > +}; > + > static const struct i2c_device_id ltrf216a_id[] = { > - { "ltrf216a" }, > + { "ltr308", .driver_data = (kernel_ulong_t)<r308_chip_info }, > + { "ltrf216a", .driver_data = (kernel_ulong_t)<rf216a_chip_info }, > {} > }; > MODULE_DEVICE_TABLE(i2c, ltrf216a_id); > > static const struct of_device_id ltrf216a_of_match[] = { > - { .compatible = "liteon,ltrf216a" }, > - { .compatible = "ltr,ltrf216a" }, > + { .compatible = "liteon,ltr308", .data = <r308_chip_info }, > + { .compatible = "liteon,ltrf216a", .data = <rf216a_chip_info }, > + { .compatible = "ltr,ltrf216a", .data = <rf216a_chip_info }, Drop this one. You cannot have undocumented compatibles - and checkpatch tells you this - and we do not want to accept stuff just because someone made something somewhere (e.g. ACPI, out of tree junk etc). There was similar effort in the past and we made it clear. Best regards, Krzysztof
On 7/5/24 11:40 AM, Krzysztof Kozlowski wrote: > On 05/07/2024 11:11, Marek Vasut wrote: >> Add LiteOn LTR-308 support into LTR-F216A kernel driver. >> >> The two devices seem to have almost identical register map, except that >> the LTR-308 does not have three CLEAR_DATA registers, which are unused >> by this driver. Furthermore, LTR-308 and LTR-F216A use different lux >> calculation constants, 0.6 and 0.45 respectively. Both differences are >> handled using chip info data. >> >> https://optoelectronics.liteon.com/upload/download/DS86-2016-0027/LTR-308ALS_Final_%20DS_V1%201.pdf >> https://optoelectronics.liteon.com/upload/download/DS86-2019-0016/LTR-F216A_Final_DS_V1.4.PDF >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- > > ... > >> mutex_init(&data->lock); >> >> @@ -520,15 +537,27 @@ static int ltrf216a_runtime_resume(struct device *dev) >> static DEFINE_RUNTIME_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_runtime_suspend, >> ltrf216a_runtime_resume, NULL); >> >> +struct ltr_chip_info ltr308_chip_info = { > > static const > >> + .has_clear_data = false, >> + .lux_multiplier = 60, >> +}; >> + >> +struct ltr_chip_info ltrf216a_chip_info = { > > static const Both fixed, thanks. >> + .has_clear_data = true, >> + .lux_multiplier = 45, >> +}; >> + >> static const struct i2c_device_id ltrf216a_id[] = { >> - { "ltrf216a" }, >> + { "ltr308", .driver_data = (kernel_ulong_t)<r308_chip_info }, >> + { "ltrf216a", .driver_data = (kernel_ulong_t)<rf216a_chip_info }, >> {} >> }; >> MODULE_DEVICE_TABLE(i2c, ltrf216a_id); >> >> static const struct of_device_id ltrf216a_of_match[] = { >> - { .compatible = "liteon,ltrf216a" }, >> - { .compatible = "ltr,ltrf216a" }, >> + { .compatible = "liteon,ltr308", .data = <r308_chip_info }, >> + { .compatible = "liteon,ltrf216a", .data = <rf216a_chip_info }, >> + { .compatible = "ltr,ltrf216a", .data = <rf216a_chip_info }, > > Drop this one. You cannot have undocumented compatibles - and checkpatch > tells you this - and we do not want to accept stuff just because someone > made something somewhere (e.g. ACPI, out of tree junk etc). There was > similar effort in the past and we made it clear. The "ltr,ltrf216a" was already part of the driver, wouldn't removing it be an ABI break ? I will add a separate patch to remove it, so it can be reverted if someone complains. Thanks .
On Fri, 5 Jul 2024 11:11:45 +0200 Marek Vasut <marex@denx.de> wrote: > Add LiteOn LTR-308 support into LTR-F216A kernel driver. > > The two devices seem to have almost identical register map, except that > the LTR-308 does not have three CLEAR_DATA registers, which are unused > by this driver. Furthermore, LTR-308 and LTR-F216A use different lux > calculation constants, 0.6 and 0.45 respectively. Both differences are > handled using chip info data. > > https://optoelectronics.liteon.com/upload/download/DS86-2016-0027/LTR-308ALS_Final_%20DS_V1%201.pdf > https://optoelectronics.liteon.com/upload/download/DS86-2019-0016/LTR-F216A_Final_DS_V1.4.PDF > > Signed-off-by: Marek Vasut <marex@denx.de> One additional question inline... > --- > drivers/iio/light/ltrf216a.c | 49 ++++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c > index 68dc48420a886..375312db4ef58 100644 > --- a/drivers/iio/light/ltrf216a.c > +++ b/drivers/iio/light/ltrf216a.c > @@ -68,6 +68,13 @@ static const int ltrf216a_int_time_reg[][2] = { > { 25, 0x40 }, > @@ -382,15 +394,19 @@ static bool ltrf216a_writable_reg(struct device *dev, unsigned int reg) > > static bool ltrf216a_volatile_reg(struct device *dev, unsigned int reg) > { > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct ltrf216a_data *data = iio_priv(indio_dev); > + > switch (reg) { > case LTRF216A_MAIN_STATUS: > - case LTRF216A_ALS_CLEAR_DATA_0: > - case LTRF216A_ALS_CLEAR_DATA_1: > - case LTRF216A_ALS_CLEAR_DATA_2: > case LTRF216A_ALS_DATA_0: > case LTRF216A_ALS_DATA_1: > case LTRF216A_ALS_DATA_2: > return true; > + case LTRF216A_ALS_CLEAR_DATA_0: > + case LTRF216A_ALS_CLEAR_DATA_1: > + case LTRF216A_ALS_CLEAR_DATA_2: Is there any point in this covering registers we have already stated above are not readable? I guess we could argue that having this change is acting as a form of documentation. Maybe just adding a comment that they don't exist would be clearer? > + return data->info->has_clear_data; > default: > return false; > } ;
On 7/7/24 3:49 PM, Jonathan Cameron wrote: > On Fri, 5 Jul 2024 11:11:45 +0200 > Marek Vasut <marex@denx.de> wrote: > >> Add LiteOn LTR-308 support into LTR-F216A kernel driver. >> >> The two devices seem to have almost identical register map, except that >> the LTR-308 does not have three CLEAR_DATA registers, which are unused >> by this driver. Furthermore, LTR-308 and LTR-F216A use different lux >> calculation constants, 0.6 and 0.45 respectively. Both differences are >> handled using chip info data. >> >> https://optoelectronics.liteon.com/upload/download/DS86-2016-0027/LTR-308ALS_Final_%20DS_V1%201.pdf >> https://optoelectronics.liteon.com/upload/download/DS86-2019-0016/LTR-F216A_Final_DS_V1.4.PDF >> >> Signed-off-by: Marek Vasut <marex@denx.de> > One additional question inline... > >> --- >> drivers/iio/light/ltrf216a.c | 49 ++++++++++++++++++++++++++++-------- >> 1 file changed, 39 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c >> index 68dc48420a886..375312db4ef58 100644 >> --- a/drivers/iio/light/ltrf216a.c >> +++ b/drivers/iio/light/ltrf216a.c >> @@ -68,6 +68,13 @@ static const int ltrf216a_int_time_reg[][2] = { >> { 25, 0x40 }, >> @@ -382,15 +394,19 @@ static bool ltrf216a_writable_reg(struct device *dev, unsigned int reg) >> >> static bool ltrf216a_volatile_reg(struct device *dev, unsigned int reg) >> { >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> + struct ltrf216a_data *data = iio_priv(indio_dev); >> + >> switch (reg) { >> case LTRF216A_MAIN_STATUS: >> - case LTRF216A_ALS_CLEAR_DATA_0: >> - case LTRF216A_ALS_CLEAR_DATA_1: >> - case LTRF216A_ALS_CLEAR_DATA_2: >> case LTRF216A_ALS_DATA_0: >> case LTRF216A_ALS_DATA_1: >> case LTRF216A_ALS_DATA_2: >> return true; >> + case LTRF216A_ALS_CLEAR_DATA_0: >> + case LTRF216A_ALS_CLEAR_DATA_1: >> + case LTRF216A_ALS_CLEAR_DATA_2: > > Is there any point in this covering registers we have already stated above are > not readable? I guess we could argue that having this change is acting > as a form of documentation. Maybe just adding a comment that they > don't exist would be clearer? I'll add a comment, thanks.
diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c index 68dc48420a886..375312db4ef58 100644 --- a/drivers/iio/light/ltrf216a.c +++ b/drivers/iio/light/ltrf216a.c @@ -68,6 +68,13 @@ static const int ltrf216a_int_time_reg[][2] = { { 25, 0x40 }, }; +struct ltr_chip_info { + /* Chip contains CLEAR_DATA_0/1/2 registers at offset 0xa..0xc */ + bool has_clear_data; + /* Lux calculation multiplier for ALS data */ + int lux_multiplier; +}; + /* * Window Factor is needed when the device is under Window glass * with coated tinted ink. This is to compensate for the light loss @@ -79,6 +86,7 @@ static const int ltrf216a_int_time_reg[][2] = { struct ltrf216a_data { struct regmap *regmap; struct i2c_client *client; + const struct ltr_chip_info *info; u32 int_time; u16 int_time_fac; u8 als_gain_fac; @@ -246,7 +254,7 @@ static int ltrf216a_get_lux(struct ltrf216a_data *data) ltrf216a_set_power_state(data, false); - lux = greendata * 45 * LTRF216A_WIN_FAC; + lux = greendata * data->info->lux_multiplier * LTRF216A_WIN_FAC; return lux; } @@ -334,15 +342,15 @@ static const struct iio_info ltrf216a_info = { static bool ltrf216a_readable_reg(struct device *dev, unsigned int reg) { + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); + struct ltrf216a_data *data = iio_priv(indio_dev); + switch (reg) { case LTRF216A_MAIN_CTRL: case LTRF216A_ALS_MEAS_RES: case LTRF216A_ALS_GAIN: case LTRF216A_PART_ID: case LTRF216A_MAIN_STATUS: - case LTRF216A_ALS_CLEAR_DATA_0: - case LTRF216A_ALS_CLEAR_DATA_1: - case LTRF216A_ALS_CLEAR_DATA_2: case LTRF216A_ALS_DATA_0: case LTRF216A_ALS_DATA_1: case LTRF216A_ALS_DATA_2: @@ -355,6 +363,10 @@ static bool ltrf216a_readable_reg(struct device *dev, unsigned int reg) case LTRF216A_ALS_THRES_LOW_1: case LTRF216A_ALS_THRES_LOW_2: return true; + case LTRF216A_ALS_CLEAR_DATA_0: + case LTRF216A_ALS_CLEAR_DATA_1: + case LTRF216A_ALS_CLEAR_DATA_2: + return data->info->has_clear_data; default: return false; } @@ -382,15 +394,19 @@ static bool ltrf216a_writable_reg(struct device *dev, unsigned int reg) static bool ltrf216a_volatile_reg(struct device *dev, unsigned int reg) { + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); + struct ltrf216a_data *data = iio_priv(indio_dev); + switch (reg) { case LTRF216A_MAIN_STATUS: - case LTRF216A_ALS_CLEAR_DATA_0: - case LTRF216A_ALS_CLEAR_DATA_1: - case LTRF216A_ALS_CLEAR_DATA_2: case LTRF216A_ALS_DATA_0: case LTRF216A_ALS_DATA_1: case LTRF216A_ALS_DATA_2: return true; + case LTRF216A_ALS_CLEAR_DATA_0: + case LTRF216A_ALS_CLEAR_DATA_1: + case LTRF216A_ALS_CLEAR_DATA_2: + return data->info->has_clear_data; default: return false; } @@ -433,6 +449,7 @@ static int ltrf216a_probe(struct i2c_client *client) i2c_set_clientdata(client, indio_dev); data->client = client; + data->info = i2c_get_match_data(client); mutex_init(&data->lock); @@ -520,15 +537,27 @@ static int ltrf216a_runtime_resume(struct device *dev) static DEFINE_RUNTIME_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_runtime_suspend, ltrf216a_runtime_resume, NULL); +struct ltr_chip_info ltr308_chip_info = { + .has_clear_data = false, + .lux_multiplier = 60, +}; + +struct ltr_chip_info ltrf216a_chip_info = { + .has_clear_data = true, + .lux_multiplier = 45, +}; + static const struct i2c_device_id ltrf216a_id[] = { - { "ltrf216a" }, + { "ltr308", .driver_data = (kernel_ulong_t)<r308_chip_info }, + { "ltrf216a", .driver_data = (kernel_ulong_t)<rf216a_chip_info }, {} }; MODULE_DEVICE_TABLE(i2c, ltrf216a_id); static const struct of_device_id ltrf216a_of_match[] = { - { .compatible = "liteon,ltrf216a" }, - { .compatible = "ltr,ltrf216a" }, + { .compatible = "liteon,ltr308", .data = <r308_chip_info }, + { .compatible = "liteon,ltrf216a", .data = <rf216a_chip_info }, + { .compatible = "ltr,ltrf216a", .data = <rf216a_chip_info }, {} }; MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
Add LiteOn LTR-308 support into LTR-F216A kernel driver. The two devices seem to have almost identical register map, except that the LTR-308 does not have three CLEAR_DATA registers, which are unused by this driver. Furthermore, LTR-308 and LTR-F216A use different lux calculation constants, 0.6 and 0.45 respectively. Both differences are handled using chip info data. https://optoelectronics.liteon.com/upload/download/DS86-2016-0027/LTR-308ALS_Final_%20DS_V1%201.pdf https://optoelectronics.liteon.com/upload/download/DS86-2019-0016/LTR-F216A_Final_DS_V1.4.PDF Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Conor Dooley <conor+dt@kernel.org> Cc: Jonathan Cameron <jic23@kernel.org> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Marek Vasut <marex@denx.de> Cc: Rob Herring <robh@kernel.org> Cc: Shreeya Patel <shreeya.patel@collabora.com> Cc: devicetree@vger.kernel.org Cc: linux-iio@vger.kernel.org --- drivers/iio/light/ltrf216a.c | 49 ++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 10 deletions(-)