On 2024-07-07 16:49, Jonathan Cameron wrote: > On Thu, 04 Jul 2024 07:16:10 +0000 > Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx> 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@xxxxxxxxxxx> >> >> >> --- >> >> >> 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.
