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

From: Krzysztof Kozlowski
Date: Mon Nov 21 2022 - 03:39:55 EST


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