[PATCH 1/3] iio: light: stk3310: relax chipid check warning

[PATCH 1/3] iio: light: stk3310: relax chipid check warning

[Kaustabh Chakraborty]

In order to allow newer devices which are compatible with existing
sensors, issuing a warning for an unknown chipid indicates that
something has gone wrong with the init process, which isn't ideal.
Swap it with a friendlier info message to get things right.

Suggested-by: Conor Dooley <conor@kernel.org>
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 drivers/iio/light/stk3310.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index e3470d6743ef..48a971de6a04 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -496,7 +496,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
 
 	ret = stk3310_check_chip_id(chipid);
 	if (ret < 0)
-		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
+		dev_info(&client->dev, "new unknown chip id: 0x%x\n", chipid);
 
 	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
 	ret = stk3310_set_state(data, state);
-- 
2.45.2

[PATCH 2/3] iio: light: stk3310: add support for stk3013

[Kaustabh Chakraborty]

Add support for Sensortek's STK3013 in the driver. The part bears the
product ID 0x31.

As seen in [1], Sensortek lists STK3013 as a proximity sensor. But it
has been experimentally observed that they do have ambient light sensing
capabilities. Furthermore, [2] implements a proximity and ambient light
sensor driver for STK3x1x devices, which is also indicative of the fact
that these parts are also ambient light sensors.

[1] https://www.sensortek.com.tw/index.php/en/products/optical-sensor/
[2] https://android.googlesource.com/kernel/msm.git/+/e6dfa4641d88201e8019be19ff557e5d2cf4572f

Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 drivers/iio/light/stk3310.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 48a971de6a04..ed20b6714546 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -35,6 +35,7 @@
 #define STK3310_STATE_EN_ALS			BIT(1)
 #define STK3310_STATE_STANDBY			0x00
 
+#define STK3013_CHIP_ID_VAL			0x31
 #define STK3310_CHIP_ID_VAL			0x13
 #define STK3311_CHIP_ID_VAL			0x1D
 #define STK3311A_CHIP_ID_VAL			0x15
@@ -84,6 +85,7 @@ static const struct reg_field stk3310_reg_field_flag_nf =
 				REG_FIELD(STK3310_REG_FLAG, 0, 0);
 
 static const u8 stk3310_chip_ids[] = {
+	STK3013_CHIP_ID_VAL,
 	STK3310_CHIP_ID_VAL,
 	STK3311A_CHIP_ID_VAL,
 	STK3311S34_CHIP_ID_VAL,
@@ -700,6 +702,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend,
 				stk3310_resume);
 
 static const struct i2c_device_id stk3310_i2c_id[] = {
+	{ "STK3013" },
 	{ "STK3310" },
 	{ "STK3311" },
 	{ "STK3335" },
@@ -708,6 +711,7 @@ static const struct i2c_device_id stk3310_i2c_id[] = {
 MODULE_DEVICE_TABLE(i2c, stk3310_i2c_id);
 
 static const struct acpi_device_id stk3310_acpi_id[] = {
+	{"STK3013", 0},
 	{"STK3310", 0},
 	{"STK3311", 0},
 	{}
@@ -716,6 +720,7 @@ static const struct acpi_device_id stk3310_acpi_id[] = {
 MODULE_DEVICE_TABLE(acpi, stk3310_acpi_id);
 
 static const struct of_device_id stk3310_of_match[] = {
+	{ .compatible = "sensortek,stk3013", },
 	{ .compatible = "sensortek,stk3310", },
 	{ .compatible = "sensortek,stk3311", },
 	{ .compatible = "sensortek,stk3335", },
-- 
2.45.2

[PATCH 3/3] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Kaustabh Chakraborty]

Add the compatible string of stk3013 to the existing list.

Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
index f6e22dc9814a..6003da66a7e6 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -19,6 +19,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - sensortek,stk3013
       - sensortek,stk3310
       - sensortek,stk3311
       - sensortek,stk3335
-- 
2.45.2

Re: [PATCH 3/3] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Jonathan Cameron]

On Fri, 12 Jul 2024 20:54:02 +0530
Kaustabh Chakraborty <kauschluss@disroot.org> wrote:

> Add the compatible string of stk3013 to the existing list.

Should include how this differs from existing devices such that it doesn't
make sense to use a fallback compatible.

> 
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
>  Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> index f6e22dc9814a..6003da66a7e6 100644
> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> @@ -19,6 +19,7 @@ allOf:
>  properties:
>    compatible:
>      enum:
> +      - sensortek,stk3013
>        - sensortek,stk3310
>        - sensortek,stk3311
>        - sensortek,stk3335

Re: [PATCH 3/3] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Kaustabh Chakraborty]

On 2024-07-13 12:06, Jonathan Cameron wrote:
> On Fri, 12 Jul 2024 20:54:02 +0530
> Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> 
>> Add the compatible string of stk3013 to the existing list.
> 
> Should include how this differs from existing devices such that it doesn't
> make sense to use a fallback compatible.

STK3013 is a proximity sensor by Sensortek, bearing chipid of 0x31. Despite
being marketed as a proximity sensor, it also appears to have ambient
light sensing capabilities.

Add the compatible string of stk3013 to the existing list, as a part not
compatible with other devices.

I hope this is good enough. I couldn't find anything more convincing.

> 
>> 
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> ---
>>  Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
>> index f6e22dc9814a..6003da66a7e6 100644
>> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
>> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
>> @@ -19,6 +19,7 @@ allOf:
>>  properties:
>>    compatible:
>>      enum:
>> +      - sensortek,stk3013
>>        - sensortek,stk3310
>>        - sensortek,stk3311
>>        - sensortek,stk3335

Re: [PATCH 3/3] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Jonathan Cameron]

On Mon, 15 Jul 2024 20:02:57 +0000
Kaustabh Chakraborty <kauschluss@disroot.org> wrote:

> On 2024-07-13 12:06, Jonathan Cameron wrote:
> > On Fri, 12 Jul 2024 20:54:02 +0530
> > Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> >   
> >> Add the compatible string of stk3013 to the existing list.  
> > 
> > Should include how this differs from existing devices such that it doesn't
> > make sense to use a fallback compatible.  
> 
> STK3013 is a proximity sensor by Sensortek, bearing chipid of 0x31. Despite
> being marketed as a proximity sensor, it also appears to have ambient
> light sensing capabilities.
> 
> Add the compatible string of stk3013 to the existing list, as a part not
> compatible with other devices.

That would be fine, but I'm not seeing any driver code changes, so when
you say not compatible, in what way? If it's register changes in features
we don't support yet or something like that, just add some examples.

A different whoami register value isn't sufficient as after the fix
you have as patch 1 that will only result in a message print.

Obviously doesn't help much for this addition as you are adding the
bypass of the whoami and the new ID in the same series, but we want
to set a precedence for future devices to use fallback compatibles
now that path works.

Jonathan

> 
> I hope this is good enough. I couldn't find anything more convincing.
> 
> >   
> >> 
> >> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> >> ---
> >>  Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> >> index f6e22dc9814a..6003da66a7e6 100644
> >> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> >> @@ -19,6 +19,7 @@ allOf:
> >>  properties:
> >>    compatible:
> >>      enum:
> >> +      - sensortek,stk3013
> >>        - sensortek,stk3310
> >>        - sensortek,stk3311
> >>        - sensortek,stk3335  

Re: [PATCH 3/3] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Kaustabh Chakraborty]

On 2024-07-16 16:43, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 20:02:57 +0000
> Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> 
>> On 2024-07-13 12:06, Jonathan Cameron wrote:
>> > On Fri, 12 Jul 2024 20:54:02 +0530
>> > Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
>> >   
>> >> Add the compatible string of stk3013 to the existing list.  
>> > 
>> > Should include how this differs from existing devices such that it doesn't
>> > make sense to use a fallback compatible.  
>> 
>> STK3013 is a proximity sensor by Sensortek, bearing chipid of 0x31. Despite
>> being marketed as a proximity sensor, it also appears to have ambient
>> light sensing capabilities.
>> 
>> Add the compatible string of stk3013 to the existing list, as a part not
>> compatible with other devices.
> 
> That would be fine, but I'm not seeing any driver code changes, so when
> you say not compatible, in what way? If it's register changes in features
> we don't support yet or something like that, just add some examples.
> 
> A different whoami register value isn't sufficient as after the fix
> you have as patch 1 that will only result in a message print.

I understand that a whoami is not enough to justify not having a fallback
compatible. That's why I mentioned it's the most "convincing" argument I
could come up with, which is admittedly, isn't enough.

And there really isn't anything feature-wise which sets STK3013 apart from
other devices. All register addresses and functions are fully compatible
with the current driver.

> 
> Obviously doesn't help much for this addition as you are adding the
> bypass of the whoami and the new ID in the same series, but we want
> to set a precedence for future devices to use fallback compatibles
> now that path works.

I'll add stk3310 as a fallback compatible and change the commit message
appropriately. Conor did mention it in the last revision, but I totally
missed that. Apologies.

Ending the description with something along the lines of:

The part is fully compatible with the existing implementation of the
device driver. Add the compatible string of stk3013 to the existing list,
with a fallback of stk3310.

...would be alright?

> 
> Jonathan
> 
>> 
>> I hope this is good enough. I couldn't find anything more convincing.
>> 
>> >   
>> >> 
>> >> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> >> ---
>> >>  Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >> 
>> >> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
>> >> index f6e22dc9814a..6003da66a7e6 100644
>> >> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
>> >> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
>> >> @@ -19,6 +19,7 @@ allOf:
>> >>  properties:
>> >>    compatible:
>> >>      enum:
>> >> +      - sensortek,stk3013
>> >>        - sensortek,stk3310
>> >>        - sensortek,stk3311
>> >>        - sensortek,stk3335

Re: [PATCH 3/3] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Jonathan Cameron]

On Wed, 17 Jul 2024 15:58:50 +0000
Kaustabh Chakraborty <kauschluss@disroot.org> wrote:

> On 2024-07-16 16:43, Jonathan Cameron wrote:
> > On Mon, 15 Jul 2024 20:02:57 +0000
> > Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> >   
> >> On 2024-07-13 12:06, Jonathan Cameron wrote:  
> >> > On Fri, 12 Jul 2024 20:54:02 +0530
> >> > Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> >> >     
> >> >> Add the compatible string of stk3013 to the existing list.    
> >> > 
> >> > Should include how this differs from existing devices such that it doesn't
> >> > make sense to use a fallback compatible.    
> >> 
> >> STK3013 is a proximity sensor by Sensortek, bearing chipid of 0x31. Despite
> >> being marketed as a proximity sensor, it also appears to have ambient
> >> light sensing capabilities.
> >> 
> >> Add the compatible string of stk3013 to the existing list, as a part not
> >> compatible with other devices.  
> > 
> > That would be fine, but I'm not seeing any driver code changes, so when
> > you say not compatible, in what way? If it's register changes in features
> > we don't support yet or something like that, just add some examples.
> > 
> > A different whoami register value isn't sufficient as after the fix
> > you have as patch 1 that will only result in a message print.  
> 
> I understand that a whoami is not enough to justify not having a fallback
> compatible. That's why I mentioned it's the most "convincing" argument I
> could come up with, which is admittedly, isn't enough.
> 
> And there really isn't anything feature-wise which sets STK3013 apart from
> other devices. All register addresses and functions are fully compatible
> with the current driver.
> 
> > 
> > Obviously doesn't help much for this addition as you are adding the
> > bypass of the whoami and the new ID in the same series, but we want
> > to set a precedence for future devices to use fallback compatibles
> > now that path works.  
> 
> I'll add stk3310 as a fallback compatible and change the commit message
> appropriately. Conor did mention it in the last revision, but I totally
> missed that. Apologies.
> 
> Ending the description with something along the lines of:
> 
> The part is fully compatible with the existing implementation of the
> device driver. Add the compatible string of stk3013 to the existing list,
> with a fallback of stk3310.
> 
> ...would be alright?

Yes. Looks good.

Jonathan

> 
> > 
> > Jonathan
> >   
> >> 
> >> I hope this is good enough. I couldn't find anything more convincing.
> >>   
> >> >     
> >> >> 
> >> >> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> >> >> ---
> >> >>  Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >> 
> >> >> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> >> >> index f6e22dc9814a..6003da66a7e6 100644
> >> >> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> >> >> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> >> >> @@ -19,6 +19,7 @@ allOf:
> >> >>  properties:
> >> >>    compatible:
> >> >>      enum:
> >> >> +      - sensortek,stk3013
> >> >>        - sensortek,stk3310
> >> >>        - sensortek,stk3311
> >> >>        - sensortek,stk3335  

Re: [PATCH 1/3] iio: light: stk3310: relax chipid check warning

[Jonathan Cameron]

On Fri, 12 Jul 2024 20:54:00 +0530
Kaustabh Chakraborty <kauschluss@disroot.org> wrote:

> In order to allow newer devices which are compatible with existing
> sensors, issuing a warning for an unknown chipid indicates that
> something has gone wrong with the init process, which isn't ideal.
> Swap it with a friendlier info message to get things right.
> 
> Suggested-by: Conor Dooley <conor@kernel.org>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
Give the patch series a cover letter next time as it makes it easy for
people to comment on whole series together + gives the series a nice
name in patchwork etc.

Plus this isn't (I think) version 1.  So that should be in the patch naming.

Actual change looks fine to me.

Thanks,

Jonathan

> ---
>  drivers/iio/light/stk3310.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index e3470d6743ef..48a971de6a04 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -496,7 +496,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
>  
>  	ret = stk3310_check_chip_id(chipid);
>  	if (ret < 0)
> -		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
> +		dev_info(&client->dev, "new unknown chip id: 0x%x\n", chipid);
>  
>  	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
>  	ret = stk3310_set_state(data, state);

Re: [PATCH 1/3] iio: light: stk3310: relax chipid check warning

[Kaustabh Chakraborty]

On 2024-07-13 12:06, Jonathan Cameron wrote:
> On Fri, 12 Jul 2024 20:54:00 +0530
> Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> 
>> In order to allow newer devices which are compatible with existing
>> sensors, issuing a warning for an unknown chipid indicates that
>> something has gone wrong with the init process, which isn't ideal.
>> Swap it with a friendlier info message to get things right.
>> 
>> Suggested-by: Conor Dooley <conor@kernel.org>
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> Give the patch series a cover letter next time as it makes it easy for
> people to comment on whole series together + gives the series a nice
> name in patchwork etc.

Sure, I assume I can add one in the next version, right?

> 
> Plus this isn't (I think) version 1.  So that should be in the patch naming.

Sorry, forgot adding v2 to the messages. I'll properly send a v3.

> 
> Actual change looks fine to me.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/light/stk3310.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
>> index e3470d6743ef..48a971de6a04 100644
>> --- a/drivers/iio/light/stk3310.c
>> +++ b/drivers/iio/light/stk3310.c
>> @@ -496,7 +496,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
>>  
>>  	ret = stk3310_check_chip_id(chipid);
>>  	if (ret < 0)
>> -		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
>> +		dev_info(&client->dev, "new unknown chip id: 0x%x\n", chipid);
>>  
>>  	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
>>  	ret = stk3310_set_state(data, state);