Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues

[Conor Dooley <conor@kernel.org>]

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

On Wed, Nov 15, 2023 at 12:50:41PM +0600, Binbin Zhou wrote:
> On Wed, Nov 1, 2023 at 1:59 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Nov 01, 2023 at 07:38:39AM +0600, Binbin Zhou wrote:
> > > On Tue, Oct 31, 2023 at 10:58 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Tue, Oct 31, 2023 at 07:05:49PM +0800, Binbin Zhou wrote:
> > > > > Add the missing 'thermal-sensor-cells' property which is required for
> > > > > every thermal sensor as it's used when using phandles.
> > > > > And add the thermal-sensor.yaml reference.
> > > > >
> > > > > Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> > > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > > > ---
> > > > >  .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > index 7538469997f9..b634f57cd011 100644
> > > > > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > @@ -10,6 +10,9 @@ maintainers:
> > > > >    - zhanghongchen <zhanghongchen@loongson.cn>
> > > > >    - Yinbo Zhu <zhuyinbo@loongson.cn>
> > > > >
> > > > > +allOf:
> > > > > +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > > > > +
> > > > >  properties:
> > > > >    compatible:
> > > > >      oneOf:
> > > > > @@ -26,12 +29,16 @@ properties:
> > > > >    interrupts:
> > > > >      maxItems: 1
> > > > >
> > > > > +  '#thermal-sensor-cells':
> > > > > +    const: 1
> > > > > +
> > > > >  required:
> > > > >    - compatible
> > > > >    - reg
> > > > >    - interrupts
> > > > > +  - '#thermal-sensor-cells'
> > > >
> > > > Why does it need to be a required property now though?
> > > > Adding new required properties is technically an ABI break.
> > >
> > > Hi Conor:
> > >
> > > I don't think it makes sense to have a separate thermal sensor
> > > definition, it needs thermal-zones to describe specific behaviors,
> > > e.g. cpu-thermal, so we need '#thermal-sensor-cells' to specify the
> > > reference.
> > > And the Loongson-2K1000 has 4 sets of control registers, we need to
> > > specify the id when referencing it.
> >
> > Unfortunately, none of this is an answer to my question.
> 
> Hi Conor:
> 
> Sorry for my late reply.
> 
> Over the past few days, I've been communicating offline with Yinbo
> (the driver author) about the use of the '#thermal-sensor-cells'
> attribute. He retested the attribute and determined that it is
> 'required'.
> 
> We can see that the '#thermal-sensor-cells' attribute in the
> dt-binding was dropped between the V12 patchset[1] and the V13
> patchset[2]. Yinbo may have misunderstood Daniel's comment and removed
> the '#thermal-sensor-cells' attribute from the dt-binding. But the
> attribute was carelessly still left in the dts file, resulting in the
> issue not being found during functional validation.
> 
> Indeed, re-adding the '#thermal-sensor-cells' attribute as "required"
> is technically an ABI breakage, but the driver does not work properly
> under the current dt-binding rules.

I was going to say that you should add some comment about this in the
commit message, but at the end of the day - you're not much of a thermal
sensor without having thermal-sensor-cells, so I think your commit
message is actually fine.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

and I probably should have recognised that earlier.

Thanks,
Conor

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