Re: [PATCH 1/2] riscv: dts: cv1800b: add pinctrl node for cv1800b

[Samuel Holland <samuel.holland@sifive.com>]

Hi Conor,

On 2023-11-13 7:29 AM, Conor Dooley wrote:
> On Mon, Nov 13, 2023 at 09:03:11PM +0800, Jisheng Zhang wrote:
>> On Sun, Nov 12, 2023 at 08:51:20PM -0500, Samuel Holland wrote:
>>> On 2023-11-12 6:57 PM, Jisheng Zhang wrote:
>>>> Add the reset device tree node to cv1800b SoC reusing the
>>>           ^^^^^
>>>           I assume you mean pinctrl here?
>>
>> oops copy and paste the commit msg ;) thanks
>>>
>>>> pinctrl-single driver.
>>>>
>>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>>> ---
>>>>  arch/riscv/boot/dts/sophgo/cv-pinctrl.h | 19 +++++++++++++++++++
>>>>  arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 10 ++++++++++
>>>>  2 files changed, 29 insertions(+)
>>>>  create mode 100644 arch/riscv/boot/dts/sophgo/cv-pinctrl.h
>>>>
>>>> diff --git a/arch/riscv/boot/dts/sophgo/cv-pinctrl.h b/arch/riscv/boot/dts/sophgo/cv-pinctrl.h
>>>> new file mode 100644
>>>> index 000000000000..ed78b6fb3142
>>>> --- /dev/null
>>>> +++ b/arch/riscv/boot/dts/sophgo/cv-pinctrl.h
>>>
>>> A couple of questions: Should this go in include/dt-bindings? And is it worth
>>
>> When I cooked this series two weeks ago, I did put it in dt-binding, but
>> then I found commit fe49f2d776f799 ("arm64: dts: ti: Use local header for
>> pinctrl register values"), "These definitions were previously put in the
>> bindings header to avoid code duplication and to provide some context
>> meaning (name), but they do not fit the purpose of bindings." which is
>> suggested and acked by Krzysztof, so I just want to follow the style
>> here.
>>
>>
>>> including macros for the actual function mappings, like in the vendor source[1]?
>>
>> Do you want something as the following?
>>
>> #define UART0_TX	0
>> #define CAM_MCLK1	1
>> ...
>>
>> #define REG_UART0_TX	0x24
>> ...
>>
>> pinctrl-single,pins = <REG_UART0_TX UART0_TX>;
>>
>> Other pinctl-single users just uses the register value directly, I have
>> no preference. But I'd like to get suggestions from DT and pinctl-single
>> maintainers. Hi Rob, Krzysztof, Conor, Tony, what's your opinion?
> 
> Basically, if the definitions map directly to registers and are just
> used to make writing your devicetree easier then they do not belong
> in a binding. This differs from clock or reset indices, where we
> essentially make up a set of indices that may or may not correlate to
> offsets in the hardware as using the register values without any sort of
> abstraction is not defining an ABI.

Right. I should have remembered this policy :)

Regards,
Samuel