Re: [PATCH] arm64: dts: mediatek: mt6357: Drop regulator-fixed compatibles

[Alexandre Mergnat <amergnat@baylibre.com>]

On 06/05/2025 23:20, Nícolas F. R. A. Prado wrote:
> On Tue, May 06, 2025 at 11:30:08AM +0200, Alexandre Mergnat wrote:
>> Hello Nícolas and Angelo,
>>
>> On 06/05/2025 10:42, AngeloGioacchino Del Regno wrote:
>>> On Fri, 02 May 2025 11:32:10 -0400, Nícolas F. R. A. Prado wrote:
>>>> Some of the regulators in the MT6357 PMIC dtsi have compatible set to
>>>> regulator-fixed, even though they don't serve any purpose: all those
>>>> regulators are handled as a whole by the mt6357-regulator driver. In
>>>> fact this is the only dtsi in this family of chips where this is the
>>>> case: mt6359 and mt6358 don't have any such compatibles.
>>>>
>>>> A side-effect caused by this is that the DT kselftest, which is supposed
>>>> to identify nodes with compatibles that can be probed, but haven't,
>>>> shows these nodes as failures.
>>>>
>>>> [...]
>>>
>>> Applied to v6.15-next/dts64, thanks!
>>>
>>> [1/1] arm64: dts: mediatek: mt6357: Drop regulator-fixed compatibles
>>>         commit: d77e89b7b03fb945b4353f2dcc4a70b34baa7bcb
>>
>> I'm surprised that patch is applied after the Rob's bot reply.
>> Also, I've some concern:
>>
>> On 02/05/2025 17:32, Nícolas F. R. A. Prado wrote:
>>> Some of the regulators in the MT6357 PMIC dtsi have compatible set to
>>> regulator-fixed, even though they don't serve any purpose: all those
>>> regulators are handled as a whole by the mt6357-regulator driver. In
>>> fact this is the only dtsi in this family of chips where this is the
>>> case: mt6359 and mt6358 don't have any such compatibles.
>> This is the only dtsi in this family to do this, yes. But according to
>> all other vendor DTSI, which use regulator-fixed when a regulator can't
>> support a range of voltage, IMHO, it make sense to use it, isn't it ?
>> If other DTSI from the family of chips doesn't, why don't fix them to be
>> aligned with the other families?
> 
> Well, but this isn't just like any other regulator-fixed in a DTSI. In this case
> it is part of a multi-function device (MFD) and so it gets probed by a parent
> node. That's the source of the issue, because then no driver gets assigned to
> the node itself.
> 

Ok, thanks for the details. After Quick check, Maxim does't use "regulator-fixed"
in the MFD too.

>>
>>>
>>> A side-effect caused by this is that the DT kselftest, which is supposed
>>> to identify nodes with compatibles that can be probed, but haven't,
>>> shows these nodes as failures.
>>>
>> I lack of data about kselftest, but according to what is reported here, it
>> appear to me this is something which could be fixed in the test itself. It make
>> sense for a DTS, but not for a DTSI because it expose HW capability of a
>> device, not the board, so it isn't mandatory to probe all DTSI node.
>> Again, I'm not an expert, the test shouldn't show the DTSI node as failure,
>> but maybe more a warning.
> 
> The DT kselftest is a run-time test, so it wouldn't be able to distinguish
> between DTSI and DTS. But in any case, we do want to check that devices from
> DTSIs have probed, a lot of the devices come from them. When a particular board
> doesn't actually have a node from a DTSI present then the node should be
> disabled, and in that case the kselftest ignores the node.
> 
> It would be possible to ignore this particular compatible, "regulator-fixed", in
> the kselftest, if it is a compatible that can't be expected to be probed. Of
> course that would mean that all the other regulator nodes that aren't MFD
> children and do get probed by that driver would no longer be checked by the
> test.
> 

Yeah this isn't the wanted behavior.

>>
>>> Remove the useless compatibles to move the dtsi in line with the others
>>> in its family and fix the DT kselftest failures.
>> If you remove compatible from these regulators, I think mediatek,mt6357-regulator.yaml
>> documentation file should be modified to be consistent and avoid dt-check error.
> 
> Ah, yes, totally agreed, I seem to have missed running dtbs_check on this patch,
> sorry. Indeed now either the binding needs to be fixed or the patch reverted.
> 
> I believe the most reasonable option would be to update those regulators in the
> binding to reference the generic regulator binding, ie:
> 
> diff --git a/Documentation/devicetree/bindings/regulator/mediatek,mt6357-regulator.yaml b/Documentation/devicetree/bindings/regulator/mediatek,mt6357-regulator.yaml
> index 6327bb2f6ee0..9308008f420e 100644
> --- a/Documentation/devicetree/bindings/regulator/mediatek,mt6357-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/mediatek,mt6357-regulator.yaml
> @@ -33,7 +33,7 @@ patternProperties:
> 
>     "^ldo-v(camio18|aud28|aux18|io18|io28|rf12|rf18|cn18|cn28|fe28)$":
>       type: object
> -    $ref: fixed-regulator.yaml#
> +    $ref: regulator.yaml#
>       unevaluatedProperties: false
>       description:
>         Properties for single fixed LDO regulator.
> 
> as well as updating the examples in the YAML. The fixed-regulator.yaml binding
> doesn't seem to provide any additional checks compared to regulator.yaml,
> besides enforcing the regulator-fixed compatible, which in this case doesn't
> serve any purpose.
> 
> Thoughts?
> 

Yes, IMHO, this yaml change should be enough.

-- 
Thanks,
Alexandre