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

[Krzysztof Kozlowski]

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).

Best regards,
Krzysztof