| Message ID | 20240625165122.231182-2-kauschluss@disroot.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 >
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 > >
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.
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.
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.
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.
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.
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
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(+)