| Message ID | 20240705091222.86916-1-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] dt-bindings: iio: light: ltrf216a: Document LTR-308 support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 10 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml index 7de1b0e721ca2..877e955d4ebd1 100644 --- a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml @@ -14,7 +14,9 @@ description: properties: compatible: - const: liteon,ltrf216a + enum: + - liteon,ltr308 + - liteon,ltrf216a reg: maxItems: 1
Document LiteOn LTR-308 support in LTR-F216A bindings. 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. 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 --- .../devicetree/bindings/iio/light/liteon,ltrf216a.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)