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

[Jianlong Huang]

On Mon, 21 Nov 2022 09:39:46 +0100, Krzysztof Kozlowski wrote:
> On 21/11/2022 09:38, Krzysztof Kozlowski wrote:
>> On 18/11/2022 02:11, Hal Feng wrote:
>>> From: Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
>>>
>>> Add pinctrl definitions for StarFive JH7110 SoC.
>>>
>>> Co-developed-by: Emil Renner Berthing <kernel@xxxxxxxx>
>>> Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx>
>>> Signed-off-by: Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
>>> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>>> ---
>>>  .../pinctrl/pinctrl-starfive-jh7110.h         | 427 ++++++++++++++++++
>>>  1 file changed, 427 insertions(+)
>>>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-starfive-jh7110.h
>>>
>>> diff --git a/include/dt-bindings/pinctrl/pinctrl-starfive-jh7110.h b/include/dt-bindings/pinctrl/pinctrl-starfive-jh7110.h
>>> new file mode 100644
>>> index 000000000000..fb02345caa27
>>> --- /dev/null
>>> +++ b/include/dt-bindings/pinctrl/pinctrl-starfive-jh7110.h
>>> @@ -0,0 +1,427 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
>>> +/*
>>> + * Copyright (C) 2022 Emil Renner Berthing <kernel@xxxxxxxx>
>>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>>> + */
>>> +
>>> +#ifndef __DT_BINDINGS_PINCTRL_STARFIVE_JH7110_H__
>>> +#define __DT_BINDINGS_PINCTRL_STARFIVE_JH7110_H__
>>> +
>>> +/*
>>> + * mux bits:
>>> + *  | 31 - 24 | 23 - 16 | 15 - 10 |  9 - 8   |  7 - 0  |
>>> + *  |  din    |  dout   |  doen   | function | gpio nr |
>>> + *
>>> + * dout:     output signal
>>> + * doen:     output enable signal
>>> + * din:      optional input signal, 0xff = none
>>> + * function:
>>> + * gpio nr:  gpio number, 0 - 63
>>> + */
>>> +#define GPIOMUX(n, dout, doen, din) ( \
>>> +		(((din)  & 0xff) << 24) | \
>>> +		(((dout) & 0xff) << 16) | \
>>> +		(((doen) & 0x3f) << 10) | \
>>> +		((n) & 0x3f))
>>> +
>> 
>> 
>> (...)
>> 
>>> +/* sys_iomux doen */
>>> +#define GPOEN_ENABLE				 0
>>> +#define GPOEN_DISABLE				 1
>>> +#define GPOEN_SYS_HDMI_CEC_SDA			 2
>>> +#define GPOEN_SYS_HDMI_DDC_SCL			 3
>>> +#define GPOEN_SYS_HDMI_DDC_SDA			 4
>>> +#define GPOEN_SYS_I2C0_CLK			 5
>>> +#define GPOEN_SYS_I2C0_DATA			 6
>>> +#define GPOEN_SYS_HIFI4_JTAG_TDO		 7
>>> +#define GPOEN_SYS_JTAG_TDO			 8
>>> +#define GPOEN_SYS_PWM0_CHANNEL0			 9
>>> +#define GPOEN_SYS_PWM0_CHANNEL1			10
>>> +#define GPOEN_SYS_PWM0_CHANNEL2			11
>>> +#define GPOEN_SYS_PWM0_CHANNEL3			12
>>> +#define GPOEN_SYS_SPI0_NSSPCTL			13
>>> +#define GPOEN_SYS_SPI0_NSSP			14
>>> +#define GPOEN_SYS_TDM_SYNC			15
>>> +#define GPOEN_SYS_TDM_TXD			16
>>> +#define GPOEN_SYS_I2C1_CLK			17
>>> +#define GPOEN_SYS_I2C1_DATA			18
>>> +#define GPOEN_SYS_SDIO1_CMD			19
>>> +#define GPOEN_SYS_SDIO1_DATA0			20
>>> +#define GPOEN_SYS_SDIO1_DATA1			21
>>> +#define GPOEN_SYS_SDIO1_DATA2			22
>>> +#define GPOEN_SYS_SDIO1_DATA3			23
>>> +#define GPOEN_SYS_SDIO1_DATA4			24
>>> +#define GPOEN_SYS_SDIO1_DATA5			25
>>> +#define GPOEN_SYS_SDIO1_DATA6			26
>>> +#define GPOEN_SYS_SDIO1_DATA7			27
>>> +#define GPOEN_SYS_SPI1_NSSPCTL			28
>>> +#define GPOEN_SYS_SPI1_NSSP			29
>>> +#define GPOEN_SYS_I2C2_CLK			30
>>> +#define GPOEN_SYS_I2C2_DATA			31
>>> +#define GPOEN_SYS_SPI2_NSSPCTL			32
>>> +#define GPOEN_SYS_SPI2_NSSP			33
>>> +#define GPOEN_SYS_I2C3_CLK			34
>>> +#define GPOEN_SYS_I2C3_DATA			35
>>> +#define GPOEN_SYS_SPI3_NSSPCTL			36
>>> +#define GPOEN_SYS_SPI3_NSSP			37
>>> +#define GPOEN_SYS_I2C4_CLK			38
>>> +#define GPOEN_SYS_I2C4_DATA			39
>>> +#define GPOEN_SYS_SPI4_NSSPCTL			40
>>> +#define GPOEN_SYS_SPI4_NSSP			41
>>> +#define GPOEN_SYS_I2C5_CLK			42
>>> +#define GPOEN_SYS_I2C5_DATA			43
>>> +#define GPOEN_SYS_SPI5_NSSPCTL			44
>>> +#define GPOEN_SYS_SPI5_NSSP			45
>>> +#define GPOEN_SYS_I2C6_CLK			46
>>> +#define GPOEN_SYS_I2C6_DATA			47
>>> +#define GPOEN_SYS_SPI6_NSSPCTL			48
>>> +#define GPOEN_SYS_SPI6_NSSP			49
>>> +
>>> +/* 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,
and will change the name as "starfive,jh7110-pinctrl.h" to match bindings or compatible.

Best regards,
Jianlong Huang