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

[Kaustabh Chakraborty <kauschluss@disroot.org>]

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.