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

[Jonathan Cameron <jic23@kernel.org>]

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.