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

[PATCH 1/2] 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 e3470d6743ef..003e7c1473d7 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 2/2] 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 2/2] dt-bindings: iio: light: stk33xx: add compatible for stk3013

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:
> 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

The driver change suggests that this device is compatible with the
existing sensors.
Jonathan, could we relax the warning during init
	ret = stk3310_check_chip_id(chipid);
	if (ret < 0)
		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
and allow fallback compatibles here please?

>        - sensortek,stk3310
>        - sensortek,stk3311
>        - sensortek,stk3335
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jonathan Cameron]

On Wed, 26 Jun 2024 17:06:48 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:
> > 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  
> 
> The driver change suggests that this device is compatible with the
> existing sensors.
> Jonathan, could we relax the warning during init
> 	ret = stk3310_check_chip_id(chipid);
> 	if (ret < 0)
> 		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
> and allow fallback compatibles here please?

Yes. Please do. This dates back to when my understanding on what
counted as fallback compatible and we are fixing that in drivers
as we touch them to add new parts.


> 
> >        - sensortek,stk3310
> >        - sensortek,stk3311
> >        - sensortek,stk3335
> > -- 
> > 2.45.2
> >   

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

[Kaustabh Chakraborty]

On 2024-06-26 16:06, Conor Dooley wrote:
> On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:
>> 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
> 
> The driver change suggests that this device is compatible with the
> existing sensors.
> Jonathan, could we relax the warning during init

What does 'relax' mean here? Earlier there used to be a probing error,
and now it's just a warning. Is that not relaxed enough?

> 	ret = stk3310_check_chip_id(chipid);
> 	if (ret < 0)
> 		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
> and allow fallback compatibles here please?

So, you mean something like this in devicetree?

  compatible = "sensortek,stk3013", "sensortek,stk3310";

I mean that's fine, but we also need to change devicetree sources for
other devices. If that's what we're doing, please let me know how do
I frame the commits.

> 
>>        - sensortek,stk3310
>>        - sensortek,stk3311
>>        - sensortek,stk3335
>> -- 
>> 2.45.2
>>

Thank you.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On Wed, Jul 03, 2024 at 06:31:13PM +0000, Kaustabh Chakraborty wrote:
> On 2024-06-26 16:06, Conor Dooley wrote:
> > On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:
> >> 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
> > 
> > The driver change suggests that this device is compatible with the
> > existing sensors.
> > Jonathan, could we relax the warning during init
> 
> What does 'relax' mean here? Earlier there used to be a probing error,
> and now it's just a warning. Is that not relaxed enough?

If it is something intentionally, I don't think a warning is suitable.
It makes the user thing something is wrong.

> 
> > 	ret = stk3310_check_chip_id(chipid);
> > 	if (ret < 0)
> > 		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
> > and allow fallback compatibles here please?
> 
> So, you mean something like this in devicetree?
> 
>   compatible = "sensortek,stk3013", "sensortek,stk3310";
> 
> I mean that's fine, but we also need to change devicetree sources for
> other devices. If that's what we're doing, please let me know how do
> I frame the commits.

Why would you need to change the dts for other devices to add a fallback
for this new compatible that is being added?

> >>        - sensortek,stk3310
> >>        - sensortek,stk3311
> >>        - sensortek,stk3335
> >> -- 
> >> 2.45.2
> >>
> 
> Thank you.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Kaustabh Chakraborty]

On 2024-07-03 19:30, Conor Dooley wrote:
> On Wed, Jul 03, 2024 at 06:31:13PM +0000, Kaustabh Chakraborty wrote:
>> On 2024-06-26 16:06, Conor Dooley wrote:
>> > On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:
>> >> 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
>> > 
>> > The driver change suggests that this device is compatible with the
>> > existing sensors.
>> > Jonathan, could we relax the warning during init
>> 
>> What does 'relax' mean here? Earlier there used to be a probing error,
>> and now it's just a warning. Is that not relaxed enough?
> 
> If it is something intentionally, I don't think a warning is suitable.
> It makes the user thing something is wrong.

So, something like:

  dev_info(&client->dev, "chip id: 0x%x\n", chipid);

is suitable in this context?

And doesn't it make stk3310_check_chip_id() obsolete? In all cases chipid
should be printed as it's not an error/warning message.

> 
>> 
>> > 	ret = stk3310_check_chip_id(chipid);
>> > 	if (ret < 0)
>> > 		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
>> > and allow fallback compatibles here please?
>> 
>> So, you mean something like this in devicetree?
>> 
>>   compatible = "sensortek,stk3013", "sensortek,stk3310";
>> 
>> I mean that's fine, but we also need to change devicetree sources for
>> other devices. If that's what we're doing, please let me know how do
>> I frame the commits.
> 
> Why would you need to change the dts for other devices to add a fallback
> for this new compatible that is being added?

Okay gotcha, so it's just for stk3013.

> 
>> >>        - sensortek,stk3310
>> >>        - sensortek,stk3311
>> >>        - sensortek,stk3335
>> >> -- 
>> >> 2.45.2
>> >>
>> 
>> Thank you.

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

[Jonathan Cameron]

On Thu, 04 Jul 2024 07:16:10 +0000
Kaustabh Chakraborty <kauschluss@disroot.org> wrote:

> On 2024-07-03 19:30, Conor Dooley wrote:
> > On Wed, Jul 03, 2024 at 06:31:13PM +0000, Kaustabh Chakraborty wrote:  
> >> On 2024-06-26 16:06, Conor Dooley wrote:  
> >> > On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:  
> >> >> 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  
> >> > 
> >> > The driver change suggests that this device is compatible with the
> >> > existing sensors.
> >> > Jonathan, could we relax the warning during init  
> >> 
> >> What does 'relax' mean here? Earlier there used to be a probing error,
> >> and now it's just a warning. Is that not relaxed enough?  
> > 
> > If it is something intentionally, I don't think a warning is suitable.
> > It makes the user thing something is wrong.  
> 
> So, something like:
> 
>   dev_info(&client->dev, "chip id: 0x%x\n", chipid);
> 
> is suitable in this context?

Key is to indicate in a 'friendly' fashion that we don't recognise the part
but we are treating it as what DT says.

dev_info(&client->dev, "New unknown chip id: 0x%x\n", chip_id);
only in the path where we don't have a match

> 
> And doesn't it make stk3310_check_chip_id() obsolete? In all cases chipid
> should be printed as it's not an error/warning message.

No. Printing it when we know what it is counts as annoying noise.
We want the print to indicate we don't know what it is.

There have been too many instances of manufacturers switching to
a part that is compatible with some non-mainline driver (because they
match on a whoami and handle it appropriately) that doesn't work
in Linux.  Hence we want to print a warning so that when we get such
a report we can ask for more info on what the device actually is.

If device manufacturers would actually update their DT when they changed
a sensor for an incompatible one we'd not need this.  Unfortunately
some of them don't :(

Jonathan



> 
> >   
> >>   
> >> > 	ret = stk3310_check_chip_id(chipid);
> >> > 	if (ret < 0)
> >> > 		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
> >> > and allow fallback compatibles here please?  
> >> 
> >> So, you mean something like this in devicetree?
> >> 
> >>   compatible = "sensortek,stk3013", "sensortek,stk3310";
> >> 
> >> I mean that's fine, but we also need to change devicetree sources for
> >> other devices. If that's what we're doing, please let me know how do
> >> I frame the commits.  
> > 
> > Why would you need to change the dts for other devices to add a fallback
> > for this new compatible that is being added?  
> 
> Okay gotcha, so it's just for stk3013.
> 
> >   
> >> >>        - sensortek,stk3310
> >> >>        - sensortek,stk3311
> >> >>        - sensortek,stk3335
> >> >> -- 
> >> >> 2.45.2
> >> >>  
> >> 
> >> Thank you.  

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

[Kaustabh Chakraborty]

On 2024-07-07 16:49, Jonathan Cameron wrote:
> On Thu, 04 Jul 2024 07:16:10 +0000
> Kaustabh Chakraborty <kauschluss@disroot.org> wrote:
> 
>> On 2024-07-03 19:30, Conor Dooley wrote:
>> > On Wed, Jul 03, 2024 at 06:31:13PM +0000, Kaustabh Chakraborty wrote:  
>> >> On 2024-06-26 16:06, Conor Dooley wrote:  
>> >> > On Tue, Jun 25, 2024 at 10:21:06PM +0530, Kaustabh Chakraborty wrote:  
>> >> >> 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  
>> >> > 
>> >> > The driver change suggests that this device is compatible with the
>> >> > existing sensors.
>> >> > Jonathan, could we relax the warning during init  
>> >> 
>> >> What does 'relax' mean here? Earlier there used to be a probing error,
>> >> and now it's just a warning. Is that not relaxed enough?  
>> > 
>> > If it is something intentionally, I don't think a warning is suitable.
>> > It makes the user thing something is wrong.  
>> 
>> So, something like:
>> 
>>   dev_info(&client->dev, "chip id: 0x%x\n", chipid);
>> 
>> is suitable in this context?
> 
> Key is to indicate in a 'friendly' fashion that we don't recognise the part
> but we are treating it as what DT says.
> 
> dev_info(&client->dev, "New unknown chip id: 0x%x\n", chip_id);
> only in the path where we don't have a match
> 
>> 
>> And doesn't it make stk3310_check_chip_id() obsolete? In all cases chipid
>> should be printed as it's not an error/warning message.
> 
> No. Printing it when we know what it is counts as annoying noise.
> We want the print to indicate we don't know what it is.
> 
> There have been too many instances of manufacturers switching to
> a part that is compatible with some non-mainline driver (because they
> match on a whoami and handle it appropriately) that doesn't work
> in Linux.  Hence we want to print a warning so that when we get such
> a report we can ask for more info on what the device actually is.
> 
> If device manufacturers would actually update their DT when they changed
> a sensor for an incompatible one we'd not need this.  Unfortunately
> some of them don't :(

I see. Sure, I'll modify it accordingly and send a v2.

> 
> Jonathan
> 
> 
> 
>> 
>> >   
>> >>   
>> >> > 	ret = stk3310_check_chip_id(chipid);
>> >> > 	if (ret < 0)
>> >> > 		dev_warn(&client->dev, "unknown chip id: 0x%x\n", chipid);
>> >> > and allow fallback compatibles here please?  
>> >> 
>> >> So, you mean something like this in devicetree?
>> >> 
>> >>   compatible = "sensortek,stk3013", "sensortek,stk3310";
>> >> 
>> >> I mean that's fine, but we also need to change devicetree sources for
>> >> other devices. If that's what we're doing, please let me know how do
>> >> I frame the commits.  
>> > 
>> > Why would you need to change the dts for other devices to add a fallback
>> > for this new compatible that is being added?  
>> 
>> Okay gotcha, so it's just for stk3013.
>> 
>> >   
>> >> >>        - sensortek,stk3310
>> >> >>        - sensortek,stk3311
>> >> >>        - sensortek,stk3335
>> >> >> -- 
>> >> >> 2.45.2
>> >> >>  
>> >> 
>> >> Thank you.