Re: [PATCH v2 1/5] dt-bindings: pinctrl: Add StarFive JH7110 pinctrl definitions

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/11/2022 15:46, Jianlong Huang wrote:
> On Tue, 29 Nov 2022 08:49:49 +0100, Krzysztof Kozlowski wrote:
>> On 29/11/2022 02:47, Jianlong Huang wrote:
>>> On Mon, 28 Nov 2022 09:32:45 +0100, Krzysztof Kozlowski wrote:
>>>> On 28/11/2022 01:48, Jianlong Huang wrote:
>>>>
>>>>>>>> +/* aon_iomux doen */
>>>>>>>> +#define GPOEN_AON_PTC0_OE_N_4			2
>>>>>>>> +#define GPOEN_AON_PTC0_OE_N_5			3
>>>>>>>> +#define GPOEN_AON_PTC0_OE_N_6			4
>>>>>>>> +#define GPOEN_AON_PTC0_OE_N_7			5
>>>>>>>> +
>>>>>>>
>>>>>>> It looks like you add register constants to the bindings. Why? The
>>>>>>> bindings are not the place to represent hardware programming model. Not
>>>>>>> mentioning that there is no benefit in this.
>>>>>>
>>>>>> Also: this entire file should be dropped, but if it stays, you have to
>>>>>> name it matching bindings or compatible (vendor,device.h).
>>>>>
>>>>> Thanks your comments.
>>>>> These macros are used to configure pinctrl in dts, so the file should stay,
>>>>
>>>> Why they should stay? What's the reason? If it is not a constant used by
>>>> driver, then register values should not be placed in the bindings, so
>>>> drop it.
>>>>
>>>
>>> Thanks.
>>>
>>> These macros in binding header(example, DOUT, DOEN etc) will be used in DTS,
>>> and driver will parse the DT for pinctrl configuration.
>>>
>>> Example in dts:
>>> uart0_pins: uart0-0 {
>>> 	tx-pins {
>>> 		pinmux = <GPIOMUX(5, GPOUT_SYS_UART0_TX, GPOEN_ENABLE, GPI_NONE)>;
>>
>> This is usage in DTS and is not an argument to store register
>> addresses/offsets as bindings. What is the usage (of define, not value)
>> in the driver?
>>
> 
> The existing implementation reuse the macros for DTS and driver.

Where in the driver? Grep gives zero results.

> Do you mean we need to separate the macros, one for DTS and one for driver usage?

No, if driver uses them it is fine. The problem is I cannot find it
anywhere.

> Or you have any better suggestion?
> 
> These macros are the value of register, not register addresses/offsets,
> except for with prefix of GPI.

Still, values are not usually part of bindings.

> 
> Drivers rarely reference macros directly, mostly parsing dts and writing them to registers.

So drivers do not use macros? Then there is no reason to store them in
bindings? What do you "bind" if there is no usage (and we do not talk
about DTS...)?

Best regards,
Krzysztof