Re: [PATCH 3/6] dt-bindings: pinctrl: Add fsl,ls1012a-pinctrl yaml file

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 28/08/2024 02:43, David Leonard wrote:
> On Tue, 27 Aug 2024, Krzysztof Kozlowski wrote:
> 
>> On 27/08/2024 18:51, David Leonard wrote:
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: fsl,ls1012a-pinctrl
>>>>> +
>>>>> +  reg:
>>>>> +    description: Specifies the base address of the PMUXCR0 register.
>>>>> +    maxItems: 2
>>>>
>>>> Instead list and describe the items.
>>>
>>> Changed to
>>>
>>>     reg:
>>>       items:
>>>         - description: Physical base address of the PMUXCR0 register.
>>>         - description: Size of the PMUXCR0 register (4).
>>>
>>> Is this what you meant?
>>
>> Almost, second reg is not a size. You claim there are two IO address
>> spaces. Each address space contains base address and size. Look at other
>> bindings how they do it.
> 
> Previously, dt_binding_check was giving me a warning that 'maxItems' needed
> to be supplied. Which confused me because I thought of reg as an opaque subspace

No.

> descriptor, but I was just trying to placate the checks. After tool upgrades,
> that warning doesn't appear any more.
> 
> So the neatest documentation would be:
> 
>    reg:
>      description: Specifies the address of the PMUXCR0 register.

No. Please go through example-schema. Or any other binding... If you
find any binding with above syntax, let me know.

Instead: maxItems: 1.

> 
>>>>> +  big-endian:
>>>>> +    description: If present, the PMUXCR0 register is implemented in big-endian.
>>>>
>>>> Why is this here? Either it is or it is not?
>>>
>>> You're right. Changed to
>>>
>>>     big-endian: true
>>>
>>> (This also lead to some code simplification)
>>
>> OK, but I still wonder why is it here. Without it the hardware will work
>> in little-endian?
> 
> Well, it's here firstly because I was trying to follow a perceived convention in
> dts/freescale/fsl,ls1012a.dtsi. That DT uses big-endian in child

I am confused. Are you adding new device or converting existing bindings?


> nodes of /soc that match up with memory map tables from the datasheet.
> (Not only do different and adjacent parts of the register map have
> opposing endianess, some register layouts also seem to be reversed
> bitwise, others bytewise.)
> 
> The second reason is practical/dodgy. The pinctrl driver should logically
> be a child of the scfg node, but the syscon driver doesn't populate its
> child nodes. To get the pinctrl driver to work meant making it a sibling

So fix driver...

> node with an unsatisfactory overlap with the scfg's address region
> 0x1570000+0x10000. (No driver binds to "fsl,ls1012a-scfg".)
> 

Several big-endian properties were added unnecessarily and are now being
removed. You cannot provide me answer whether this hardware is
little/big endian, so it also feels unnecessary.

Best regards,
Krzysztof