[PATCH 1/2] dt-bindings: iio: light: ltrf216a: Document LTR-308 support

[PATCH 1/2] dt-bindings: iio: light: ltrf216a: Document LTR-308 support

[Marek Vasut]

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(-)

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
-- 
2.43.0

[PATCH 2/2] iio: light: ltrf216a: Add LTR-308 support

[Marek Vasut]

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(-)

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)&ltr308_chip_info },
+	{ "ltrf216a", .driver_data = (kernel_ulong_t)&ltrf216a_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 = &ltr308_chip_info },
+	{ .compatible = "liteon,ltrf216a", .data = &ltrf216a_chip_info },
+	{ .compatible = "ltr,ltrf216a", .data = &ltrf216a_chip_info },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
-- 
2.43.0

Re: [PATCH 2/2] iio: light: ltrf216a: Add LTR-308 support

[Krzysztof Kozlowski]

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)&ltr308_chip_info },
> +	{ "ltrf216a", .driver_data = (kernel_ulong_t)&ltrf216a_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 = &ltr308_chip_info },
> +	{ .compatible = "liteon,ltrf216a", .data = &ltrf216a_chip_info },
> +	{ .compatible = "ltr,ltrf216a", .data = &ltrf216a_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

Re: [PATCH 2/2] iio: light: ltrf216a: Add LTR-308 support

[Marek Vasut]

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)&ltr308_chip_info },
>> +	{ "ltrf216a", .driver_data = (kernel_ulong_t)&ltrf216a_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 = &ltr308_chip_info },
>> +	{ .compatible = "liteon,ltrf216a", .data = &ltrf216a_chip_info },
>> +	{ .compatible = "ltr,ltrf216a", .data = &ltrf216a_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 .

Re: [PATCH 2/2] iio: light: ltrf216a: Add LTR-308 support

[Jonathan Cameron]

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;
>  	}
;

Re: [PATCH 2/2] iio: light: ltrf216a: Add LTR-308 support

[Marek Vasut]

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.