Re: [PATCH] arm64: dts: qcom: ipq6018: Fix gpio-ranges property

[Baruch Siach <baruch@tkos.co.il>]

Hi Florian,

On Wed, Dec 15 2021, Florian Fainelli wrote:
> On 12/6/21 11:27 PM, Baruch Siach wrote:
>> There must be three parameters in gpio-ranges property. Fixes this not
>> very helpful error message:
>> 
>>   OF: /soc/pinctrl@1000000: (null) = 3 found 3
>> 
>> Fixes: 1e8277854b49 ("arm64: dts: Add ipq6018 SoC and CP01 board support")
>> Cc: Sricharan R <sricharan@codeaurora.org>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>> 
>> The error message was improved in commit af3be70a321 ("of: Improve
>> of_phandle_iterator_next() error message"), but there is evidently some
>> more room for improvement. As I don't really understand the code, I
>> added the commit author and the DT list to Cc.
>
> In the commit message you find what the error message was and then
> became which tells you which property was used to determine the cell
> size, and what count was obtained instead.
>
> In your particular case here however, since the proper was with the
> 'gpio-ranges' property and there is no property that indicates the cell
> size AFAICT, it might been more helpful to revert back to the old
> message maybe?

Thanks for taking a look. A few comment below.

> So maybe something like which is hopefully the best of
> both worlds:
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 372a177f3b80..63bff37bf035 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1378,9 +1378,13 @@ int of_phandle_iterator_next(struct
> of_phandle_iterator *it)
>                  * property data length
>                  */
>                 if (it->cur + count > it->list_end) {
> -                       pr_err("%pOF: %s = %d found %d\n",
> -                              it->parent, it->cells_name,
> -                              count, it->cell_count);
> +                       if (it->cells_name)
> +                               pr_err("%pOF: %s = %d found %d\n",
> +                                       it->parent, it->cells_name,
> +                                       count, it->cell_count);

Thinking about it, shouldn't we have 'it->list_end - it->cur' instead of
it->cell_count here? This change produces:

  OF: /soc/pinctrl@1000000: (null) = 3 found 2

which looks more correct. This is not related to my original issue,
though it might have helped me back then.

> +                       else
> +                               pr_err("%pOF: arguments longer than
> property\n",
> +                                      it->parent);
>                         goto err;
>                 }
>         }

I think that the best aid would have been the property name. But as far
as I can see struct of_phandle_iterator does not preserve the list_name
parameter of of_phandle_iterator_init().

Second best would probably be the phandle name, that is
of_node_full_name(it->node). It is a little confusing in this case
because it->node is the same as it->parent.

What do you think?

baruch

>>  arch/arm64/boot/dts/qcom/ipq6018.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
>> index 6a22bb5f42f4..a717fc17523d 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
>> @@ -220,7 +220,7 @@ tlmm: pinctrl@1000000 {
>>  			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>>  			gpio-controller;
>>  			#gpio-cells = <2>;
>> -			gpio-ranges = <&tlmm 0 80>;
>> +			gpio-ranges = <&tlmm 0 0 80>;
>>  			interrupt-controller;
>>  			#interrupt-cells = <2>;
>>  
>> 


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -