Re: [PATCH] dt-bindings: leds: Describe optional 'reg' property used for Qualcomm LPG nodes

Re: [PATCH] dt-bindings: leds: Describe optional 'reg' property used for Qualcomm LPG nodes

[Bjorn Andersson]

On Thu 21 Jul 13:19 PDT 2022, Dmitry Baryshkov wrote:

> On Thu, 21 Jul 2022 at 22:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
> >
> > As Bjorn noted in [1], it is useful to describe the optional
> > 'reg' property for Qualcomm LPG nodes as it is used in
> > some Qualcomm dts files.
> 
> I don't think this is correct. LPG block maps to several regions, so
> using just one of them in reg doesn't look correct.
> 

I agree, but I also like the uniformity of having unit addresses for the
devices on the spmi buses.

> > This fixes the following 'make dtbs_check' error reported for
> > pm8350c & sc8280xp pwm nodes:
> >
> > arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dtb:
> >  pwm@e800: 'reg' does not match any of the regexes:
> >  '^led@[0-9a-f]$', 'pinctrl-[0-9]+'
> 
> I'd prefer to follow the existing schema and to drop the region from
> those files.
> 

I'm fine either way, but we have more of these nodes, so I would like to
hear from the DT maintainers on the direction to take. All nodes on the
spmi bus has an (at least one) address, so it would be accurate to state
this in the node.

It does however not seem like devicetree@, nor Krzysztof is Cc'ed on
this patch, so I've added them...

Regards,
Bjorn

> >
> > [1]. https://lore.kernel.org/linux-arm-msm/Ytg3tIaL5h5b9ewH@builder.lan/
> >
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: robh@kernel.org
> > Cc: pavel@ucw.cz
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> > index fe336fa16518..f394ab7a757b 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> > +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> > @@ -27,6 +27,9 @@ properties:
> >        - qcom,pmi8994-lpg
> >        - qcom,pmi8998-lpg
> >
> > +  reg:
> > +    maxItems: 1
> > +
> >    "#pwm-cells":
> >      const: 2
> >
> > --
> > 2.35.3
> >
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH] dt-bindings: leds: Describe optional 'reg' property used for Qualcomm LPG nodes

[Krzysztof Kozlowski]

On 08/08/2022 21:44, Bjorn Andersson wrote:
> On Thu 21 Jul 13:19 PDT 2022, Dmitry Baryshkov wrote:
> 
>> On Thu, 21 Jul 2022 at 22:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>>
>>> As Bjorn noted in [1], it is useful to describe the optional
>>> 'reg' property for Qualcomm LPG nodes as it is used in
>>> some Qualcomm dts files.
>>
>> I don't think this is correct. LPG block maps to several regions, so
>> using just one of them in reg doesn't look correct.
>>
> 
> I agree, but I also like the uniformity of having unit addresses for the
> devices on the spmi buses.

regulators also do not have reg, so I guess consistency is already gone.

I vote here to reflect the real hardware/device which means:
1. IIUC, the design of entire SPMI bindings and its implementation is
around parent device sitting on SPMI bus and children using its
regmap/io space.
2. The children are not really re-usable for different cases/devices
(e.g. standalone WLED or LPG, outside of PMIC).
3. This means entire design is tightly coupled and LPG (or wled,
regulators) bindings describe the piece of PMIC, thus I find appropriate
skipping "reg".
4. If we want to keep the "reg", then it should rather reflect reality,
so if Dmitry said - multiple items for separate IO address ranges.

> 
>>> This fixes the following 'make dtbs_check' error reported for
>>> pm8350c & sc8280xp pwm nodes:
>>>
>>> arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dtb:
>>>  pwm@e800: 'reg' does not match any of the regexes:
>>>  '^led@[0-9a-f]$', 'pinctrl-[0-9]+'
>>
>> I'd prefer to follow the existing schema and to drop the region from
>> those files.
>>
> 
> I'm fine either way, but we have more of these nodes, so I would like to
> hear from the DT maintainers on the direction to take. All nodes on the
> spmi bus has an (at least one) address, so it would be accurate to state
> this in the node.
> 
> It does however not seem like devicetree@, nor Krzysztof is Cc'ed on
> this patch, so I've added them...
>

Anyway this patch has to be resent to properly reach DT patchwork.

Bhupesh,

Please use scripts/get_maintainer.pl to Cc relevant folks and mailing
lists. While resending, add appropriate device prefix to subject, so:
dt-bindings: leds: qcom-lpg:


Best regards,
Krzysztof

Re: [PATCH] dt-bindings: leds: Describe optional 'reg' property used for Qualcomm LPG nodes

[Bhupesh Sharma]

On 8/9/22 12:21 PM, Krzysztof Kozlowski wrote:
> On 08/08/2022 21:44, Bjorn Andersson wrote:
>> On Thu 21 Jul 13:19 PDT 2022, Dmitry Baryshkov wrote:
>>
>>> On Thu, 21 Jul 2022 at 22:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>>>
>>>> As Bjorn noted in [1], it is useful to describe the optional
>>>> 'reg' property for Qualcomm LPG nodes as it is used in
>>>> some Qualcomm dts files.
>>>
>>> I don't think this is correct. LPG block maps to several regions, so
>>> using just one of them in reg doesn't look correct.
>>>
>>
>> I agree, but I also like the uniformity of having unit addresses for the
>> devices on the spmi buses.
> 
> regulators also do not have reg, so I guess consistency is already gone.
> 
> I vote here to reflect the real hardware/device which means:
> 1. IIUC, the design of entire SPMI bindings and its implementation is
> around parent device sitting on SPMI bus and children using its
> regmap/io space.
> 2. The children are not really re-usable for different cases/devices
> (e.g. standalone WLED or LPG, outside of PMIC).
> 3. This means entire design is tightly coupled and LPG (or wled,
> regulators) bindings describe the piece of PMIC, thus I find appropriate
> skipping "reg".
> 4. If we want to keep the "reg", then it should rather reflect reality,
> so if Dmitry said - multiple items for separate IO address ranges.

Ok, so I think the majority opinion is to skip 'reg' from the 
devicetree-binding. Lets stick to that.

>>
>>>> This fixes the following 'make dtbs_check' error reported for
>>>> pm8350c & sc8280xp pwm nodes:
>>>>
>>>> arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dtb:
>>>>   pwm@e800: 'reg' does not match any of the regexes:
>>>>   '^led@[0-9a-f]$', 'pinctrl-[0-9]+'
>>>
>>> I'd prefer to follow the existing schema and to drop the region from
>>> those files.
>>>
>>
>> I'm fine either way, but we have more of these nodes, so I would like to
>> hear from the DT maintainers on the direction to take. All nodes on the
>> spmi bus has an (at least one) address, so it would be accurate to state
>> this in the node.
>>
>> It does however not seem like devicetree@, nor Krzysztof is Cc'ed on
>> this patch, so I've added them...
>>
> 
> Anyway this patch has to be resent to properly reach DT patchwork.
> 
> Bhupesh,
> 
> Please use scripts/get_maintainer.pl to Cc relevant folks and mailing
> lists. While resending, add appropriate device prefix to subject, so:
> dt-bindings: leds: qcom-lpg:

Sure, will send v2 accordingly.

Thanks.