* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-06 14:42 ` Jerome Brunet
0 siblings, 0 replies; 57+ messages in thread
From: Jerome Brunet @ 2017-03-06 14:42 UTC (permalink / raw)
To: linus-amlogic
On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
> - PWM_A
> - PWM_B
> - PWM_C
> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
> m>
> ---
> ?drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
> +++++++++++++++++++++++++++++++
> ?1 file changed, 64 insertions(+)
>
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> index 4ab94a85e306..91dfc498780b 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
> = { PIN(GPIOZ_11, EE_OFF) };
> ?static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
> EE_OFF) };
> ?static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
> EE_OFF) };
> ?
> +static const unsigned int pwm_a_pins[] = {
> PIN(GPIOX_6, EE_OFF) };
> +
> +static const unsigned int pwm_b_pins[] = {
> PIN(GPIODV_29, EE_OFF) };
> +
> +static const unsigned int pwm_c_pins[] = {
> PIN(GPIOZ_15, EE_OFF) };
> +
> +static const unsigned int pwm_d_pins[] = {
> PIN(GPIODV_28, EE_OFF) };
> +
> ?static const unsigned int pwm_e_pins[] = {
> PIN(GPIOX_16, EE_OFF) };
> ?
> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
> EE_OFF) };
> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
> EE_OFF) };
> +
> ?static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
> EE_OFF) };
> ?static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
> EE_OFF) };
> ?static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
> EE_OFF) };
> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
> = { PIN(GPIOAO_3, 0) };
> ?
> ?static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
> };
> ?
> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
> 0) };
> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
> 0) };
> +
> ?static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
> 0) };
> ?
> ?static struct meson_pmx_group meson_gxl_periphs_groups[] = {
> @@ -350,7 +364,9 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(uart_rts_a, 5, 16),
> ? GROUP(uart_tx_c, 5, 13),
> ? GROUP(uart_rx_c, 5, 12),
> + GROUP(pwm_a, 5, 25),
> ? GROUP(pwm_e, 5, 15),
> + GROUP(pwm_f_x, 5, 14),
> ?
> ? /* Bank Z */
> ? GROUP(eth_mdio, 4, 22),
> @@ -367,6 +383,7 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(eth_txd1, 4, 12),
> ? GROUP(eth_txd2, 4, 11),
> ? GROUP(eth_txd3, 4, 10),
> + GROUP(pwm_c, 3, 20),
> ?
> ? /* Bank H */
> ? GROUP(hdmi_hpd, 6, 31),
> @@ -382,6 +399,8 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(i2c_sda_b, 1, 12),
> ? GROUP(i2c_sck_c, 1, 11),
> ? GROUP(i2c_sda_c, 1, 10),
> + GROUP(pwm_b, 2, 11),
> + GROUP(pwm_d, 2, 12),
> ?
> ? /* Bank BOOT */
> ? GROUP(emmc_nand_d07, 7, 31),
> @@ -404,6 +423,9 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(sdcard_d2, 6, 0),
> ? GROUP(sdcard_cmd, 6, 2),
> ? GROUP(sdcard_clk, 6, 3),
> +
> + /* Bank CLK */
> + GROUP(pwm_f_clk, 8, 30),
> ?};
> ?
> ?static struct meson_pmx_group meson_gxl_aobus_groups[] = {
> @@ -428,6 +450,8 @@ static struct meson_pmx_group
> meson_gxl_aobus_groups[] = {
> ? GROUP(uart_cts_ao_b, 0, 8),
> ? GROUP(uart_rts_ao_b, 0, 7),
> ? GROUP(remote_input_ao, 0, 0),
> + GROUP(pwm_ao_a_3, 0, 22),
> + GROUP(pwm_ao_a_8, 0, 17),
> ? GROUP(pwm_ao_b, 0, 3),
> ?};
> ?
> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
> ? "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
> ?};
> ?
> +static const char * const pwm_a_groups[] = {
> + "pwm_a",
> +};
> +
> +static const char * const pwm_b_groups[] = {
> + "pwm_b",
> +};
> +
> +static const char * const pwm_c_groups[] = {
> + "pwm_c",
> +};
> +
> +static const char * const pwm_d_groups[] = {
> + "pwm_d",
> +};
> +
> ?static const char * const pwm_e_groups[] = {
> ? "pwm_e",
> ?};
> ?
> +static const char * const pwm_f_clk_groups[] = {
> + "pwm_f_clk",
> +};
> +
> +static const char * const pwm_f_x_groups[] = {
> + "pwm_f_x",
> +};
> +
> ?static const char * const hdmi_hpd_groups[] = {
> ? "hdmi_hpd",
> ?};
> @@ -542,6 +590,14 @@ static const char * const
> remote_input_ao_groups[] = {
> ? "remote_input_ao",
> ?};
> ?
> +static const char * const pwm_ao_a_3_groups[] = {
> + "pwm_ao_a_3",
> +};
> +
> +static const char * const pwm_ao_a_8_groups[] = {
> + "pwm_ao_a_8",
> +};
> +
> ?static const char * const pwm_ao_b_groups[] = {
> ? "pwm_ao_b",
> ?};
> @@ -559,7 +615,13 @@ static struct meson_pmx_func
> meson_gxl_periphs_functions[] = {
> ? FUNCTION(i2c_b),
> ? FUNCTION(i2c_c),
> ? FUNCTION(eth),
> + FUNCTION(pwm_a),
> + FUNCTION(pwm_b),
> + FUNCTION(pwm_c),
> + FUNCTION(pwm_d),
> ? FUNCTION(pwm_e),
> + FUNCTION(pwm_f_clk),
> + FUNCTION(pwm_f_x),
I wonder if having function named "pwm_f_clk" really makes sense ?
Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
The actual pin used will be provided in the dt. Here, I suppose we
could have this:
+static const char * const pwm_f_groups[] = {
+ "pwm_f_x", "pwm_f_clk",
+};
Has far as I can see, on meson arch, the function does not carry much
information anyway, except for prints.
To be clear, I'm not questioning this change in particular. It looks
good, and follows what has been done in the past on meson. I know we
have been this a lot already, but I'm questioning whether we should
continue to do so ?
I asking because I also have a lot case like this coming up on audio
for gxl and gxbb, where the same function can use different pins.
Of course, I'd be happy to provide such change on top of martin patch.
> ? FUNCTION(hdmi_hpd),
> ? FUNCTION(hdmi_i2c),
> ?};
> @@ -569,6 +631,8 @@ static struct meson_pmx_func
> meson_gxl_aobus_functions[] = {
> ? FUNCTION(uart_ao),
> ? FUNCTION(uart_ao_b),
> ? FUNCTION(remote_input_ao),
> + FUNCTION(pwm_ao_a_3),
> + FUNCTION(pwm_ao_a_8),
Same here
> ? FUNCTION(pwm_ao_b),
> ?};
> ?
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-06 14:42 ` Jerome Brunet
0 siblings, 0 replies; 57+ messages in thread
From: Jerome Brunet @ 2017-03-06 14:42 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
> - PWM_A
> - PWM_B
> - PWM_C
> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
> m>
> ---
> ?drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
> +++++++++++++++++++++++++++++++
> ?1 file changed, 64 insertions(+)
>
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> index 4ab94a85e306..91dfc498780b 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
> = { PIN(GPIOZ_11, EE_OFF) };
> ?static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
> EE_OFF) };
> ?static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
> EE_OFF) };
> ?
> +static const unsigned int pwm_a_pins[] = {
> PIN(GPIOX_6, EE_OFF) };
> +
> +static const unsigned int pwm_b_pins[] = {
> PIN(GPIODV_29, EE_OFF) };
> +
> +static const unsigned int pwm_c_pins[] = {
> PIN(GPIOZ_15, EE_OFF) };
> +
> +static const unsigned int pwm_d_pins[] = {
> PIN(GPIODV_28, EE_OFF) };
> +
> ?static const unsigned int pwm_e_pins[] = {
> PIN(GPIOX_16, EE_OFF) };
> ?
> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
> EE_OFF) };
> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
> EE_OFF) };
> +
> ?static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
> EE_OFF) };
> ?static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
> EE_OFF) };
> ?static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
> EE_OFF) };
> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
> = { PIN(GPIOAO_3, 0) };
> ?
> ?static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
> };
> ?
> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
> 0) };
> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
> 0) };
> +
> ?static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
> 0) };
> ?
> ?static struct meson_pmx_group meson_gxl_periphs_groups[] = {
> @@ -350,7 +364,9 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(uart_rts_a, 5, 16),
> ? GROUP(uart_tx_c, 5, 13),
> ? GROUP(uart_rx_c, 5, 12),
> + GROUP(pwm_a, 5, 25),
> ? GROUP(pwm_e, 5, 15),
> + GROUP(pwm_f_x, 5, 14),
> ?
> ? /* Bank Z */
> ? GROUP(eth_mdio, 4, 22),
> @@ -367,6 +383,7 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(eth_txd1, 4, 12),
> ? GROUP(eth_txd2, 4, 11),
> ? GROUP(eth_txd3, 4, 10),
> + GROUP(pwm_c, 3, 20),
> ?
> ? /* Bank H */
> ? GROUP(hdmi_hpd, 6, 31),
> @@ -382,6 +399,8 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(i2c_sda_b, 1, 12),
> ? GROUP(i2c_sck_c, 1, 11),
> ? GROUP(i2c_sda_c, 1, 10),
> + GROUP(pwm_b, 2, 11),
> + GROUP(pwm_d, 2, 12),
> ?
> ? /* Bank BOOT */
> ? GROUP(emmc_nand_d07, 7, 31),
> @@ -404,6 +423,9 @@ static struct meson_pmx_group
> meson_gxl_periphs_groups[] = {
> ? GROUP(sdcard_d2, 6, 0),
> ? GROUP(sdcard_cmd, 6, 2),
> ? GROUP(sdcard_clk, 6, 3),
> +
> + /* Bank CLK */
> + GROUP(pwm_f_clk, 8, 30),
> ?};
> ?
> ?static struct meson_pmx_group meson_gxl_aobus_groups[] = {
> @@ -428,6 +450,8 @@ static struct meson_pmx_group
> meson_gxl_aobus_groups[] = {
> ? GROUP(uart_cts_ao_b, 0, 8),
> ? GROUP(uart_rts_ao_b, 0, 7),
> ? GROUP(remote_input_ao, 0, 0),
> + GROUP(pwm_ao_a_3, 0, 22),
> + GROUP(pwm_ao_a_8, 0, 17),
> ? GROUP(pwm_ao_b, 0, 3),
> ?};
> ?
> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
> ? "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
> ?};
> ?
> +static const char * const pwm_a_groups[] = {
> + "pwm_a",
> +};
> +
> +static const char * const pwm_b_groups[] = {
> + "pwm_b",
> +};
> +
> +static const char * const pwm_c_groups[] = {
> + "pwm_c",
> +};
> +
> +static const char * const pwm_d_groups[] = {
> + "pwm_d",
> +};
> +
> ?static const char * const pwm_e_groups[] = {
> ? "pwm_e",
> ?};
> ?
> +static const char * const pwm_f_clk_groups[] = {
> + "pwm_f_clk",
> +};
> +
> +static const char * const pwm_f_x_groups[] = {
> + "pwm_f_x",
> +};
> +
> ?static const char * const hdmi_hpd_groups[] = {
> ? "hdmi_hpd",
> ?};
> @@ -542,6 +590,14 @@ static const char * const
> remote_input_ao_groups[] = {
> ? "remote_input_ao",
> ?};
> ?
> +static const char * const pwm_ao_a_3_groups[] = {
> + "pwm_ao_a_3",
> +};
> +
> +static const char * const pwm_ao_a_8_groups[] = {
> + "pwm_ao_a_8",
> +};
> +
> ?static const char * const pwm_ao_b_groups[] = {
> ? "pwm_ao_b",
> ?};
> @@ -559,7 +615,13 @@ static struct meson_pmx_func
> meson_gxl_periphs_functions[] = {
> ? FUNCTION(i2c_b),
> ? FUNCTION(i2c_c),
> ? FUNCTION(eth),
> + FUNCTION(pwm_a),
> + FUNCTION(pwm_b),
> + FUNCTION(pwm_c),
> + FUNCTION(pwm_d),
> ? FUNCTION(pwm_e),
> + FUNCTION(pwm_f_clk),
> + FUNCTION(pwm_f_x),
I wonder if having function named "pwm_f_clk" really makes sense ?
Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
The actual pin used will be provided in the dt. Here, I suppose we
could have this:
+static const char * const pwm_f_groups[] = {
+ "pwm_f_x", "pwm_f_clk",
+};
Has far as I can see, on meson arch, the function does not carry much
information anyway, except for prints.
To be clear, I'm not questioning this change in particular. It looks
good, and follows what has been done in the past on meson. I know we
have been this a lot already, but I'm questioning whether we should
continue to do so ?
I asking because I also have a lot case like this coming up on audio
for gxl and gxbb, where the same function can use different pins.
Of course, I'd be happy to provide such change on top of martin patch.
> ? FUNCTION(hdmi_hpd),
> ? FUNCTION(hdmi_i2c),
> ?};
> @@ -569,6 +631,8 @@ static struct meson_pmx_func
> meson_gxl_aobus_functions[] = {
> ? FUNCTION(uart_ao),
> ? FUNCTION(uart_ao_b),
> ? FUNCTION(remote_input_ao),
> + FUNCTION(pwm_ao_a_3),
> + FUNCTION(pwm_ao_a_8),
Same here
> ? FUNCTION(pwm_ao_b),
> ?};
> ?
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-06 14:42 ` Jerome Brunet
(?)
@ 2017-03-09 19:47 ` Martin Blumenstingl
-1 siblings, 0 replies; 57+ messages in thread
From: Martin Blumenstingl @ 2017-03-09 19:47 UTC (permalink / raw)
To: Jerome Brunet, linus.walleij
Cc: linux-amlogic, linux-gpio, khilman, carlo, devicetree,
will.deacon, catalin.marinas, mark.rutland, robh+dt,
linux-arm-kernel
Hi Linus,
On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
>> - PWM_A
>> - PWM_B
>> - PWM_C
>> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
>> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
>> m>
>> ---
>> drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
>> +++++++++++++++++++++++++++++++
>> 1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> index 4ab94a85e306..91dfc498780b 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
>> = { PIN(GPIOZ_11, EE_OFF) };
>> static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
>> EE_OFF) };
>> static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
>> EE_OFF) };
>>
>> +static const unsigned int pwm_a_pins[] = {
>> PIN(GPIOX_6, EE_OFF) };
>> +
>> +static const unsigned int pwm_b_pins[] = {
>> PIN(GPIODV_29, EE_OFF) };
>> +
>> +static const unsigned int pwm_c_pins[] = {
>> PIN(GPIOZ_15, EE_OFF) };
>> +
>> +static const unsigned int pwm_d_pins[] = {
>> PIN(GPIODV_28, EE_OFF) };
>> +
>> static const unsigned int pwm_e_pins[] = {
>> PIN(GPIOX_16, EE_OFF) };
>>
>> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
>> EE_OFF) };
>> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
>> EE_OFF) };
>> +
>> static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
>> EE_OFF) };
>> static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
>> EE_OFF) };
>> static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
>> EE_OFF) };
>> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
>> = { PIN(GPIOAO_3, 0) };
>>
>> static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
>> };
>>
>> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
>> 0) };
>> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
>> 0) };
>> +
>> static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
>> 0) };
>>
>> static struct meson_pmx_group meson_gxl_periphs_groups[] = {
>> @@ -350,7 +364,9 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(uart_rts_a, 5, 16),
>> GROUP(uart_tx_c, 5, 13),
>> GROUP(uart_rx_c, 5, 12),
>> + GROUP(pwm_a, 5, 25),
>> GROUP(pwm_e, 5, 15),
>> + GROUP(pwm_f_x, 5, 14),
>>
>> /* Bank Z */
>> GROUP(eth_mdio, 4, 22),
>> @@ -367,6 +383,7 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(eth_txd1, 4, 12),
>> GROUP(eth_txd2, 4, 11),
>> GROUP(eth_txd3, 4, 10),
>> + GROUP(pwm_c, 3, 20),
>>
>> /* Bank H */
>> GROUP(hdmi_hpd, 6, 31),
>> @@ -382,6 +399,8 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(i2c_sda_b, 1, 12),
>> GROUP(i2c_sck_c, 1, 11),
>> GROUP(i2c_sda_c, 1, 10),
>> + GROUP(pwm_b, 2, 11),
>> + GROUP(pwm_d, 2, 12),
>>
>> /* Bank BOOT */
>> GROUP(emmc_nand_d07, 7, 31),
>> @@ -404,6 +423,9 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(sdcard_d2, 6, 0),
>> GROUP(sdcard_cmd, 6, 2),
>> GROUP(sdcard_clk, 6, 3),
>> +
>> + /* Bank CLK */
>> + GROUP(pwm_f_clk, 8, 30),
>> };
>>
>> static struct meson_pmx_group meson_gxl_aobus_groups[] = {
>> @@ -428,6 +450,8 @@ static struct meson_pmx_group
>> meson_gxl_aobus_groups[] = {
>> GROUP(uart_cts_ao_b, 0, 8),
>> GROUP(uart_rts_ao_b, 0, 7),
>> GROUP(remote_input_ao, 0, 0),
>> + GROUP(pwm_ao_a_3, 0, 22),
>> + GROUP(pwm_ao_a_8, 0, 17),
>> GROUP(pwm_ao_b, 0, 3),
>> };
>>
>> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
>> "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
>> };
>>
>> +static const char * const pwm_a_groups[] = {
>> + "pwm_a",
>> +};
>> +
>> +static const char * const pwm_b_groups[] = {
>> + "pwm_b",
>> +};
>> +
>> +static const char * const pwm_c_groups[] = {
>> + "pwm_c",
>> +};
>> +
>> +static const char * const pwm_d_groups[] = {
>> + "pwm_d",
>> +};
>> +
>> static const char * const pwm_e_groups[] = {
>> "pwm_e",
>> };
>>
>> +static const char * const pwm_f_clk_groups[] = {
>> + "pwm_f_clk",
>> +};
>> +
>> +static const char * const pwm_f_x_groups[] = {
>> + "pwm_f_x",
>> +};
>> +
>> static const char * const hdmi_hpd_groups[] = {
>> "hdmi_hpd",
>> };
>> @@ -542,6 +590,14 @@ static const char * const
>> remote_input_ao_groups[] = {
>> "remote_input_ao",
>> };
>>
>> +static const char * const pwm_ao_a_3_groups[] = {
>> + "pwm_ao_a_3",
>> +};
>> +
>> +static const char * const pwm_ao_a_8_groups[] = {
>> + "pwm_ao_a_8",
>> +};
>> +
>> static const char * const pwm_ao_b_groups[] = {
>> "pwm_ao_b",
>> };
>> @@ -559,7 +615,13 @@ static struct meson_pmx_func
>> meson_gxl_periphs_functions[] = {
>> FUNCTION(i2c_b),
>> FUNCTION(i2c_c),
>> FUNCTION(eth),
>> + FUNCTION(pwm_a),
>> + FUNCTION(pwm_b),
>> + FUNCTION(pwm_c),
>> + FUNCTION(pwm_d),
>> FUNCTION(pwm_e),
>> + FUNCTION(pwm_f_clk),
>> + FUNCTION(pwm_f_x),
>
> I wonder if having function named "pwm_f_clk" really makes sense ?
> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
> The actual pin used will be provided in the dt. Here, I suppose we
> could have this:
>
> +static const char * const pwm_f_groups[] = {
> + "pwm_f_x", "pwm_f_clk",
> +};
>
> Has far as I can see, on meson arch, the function does not carry much
> information anyway, except for prints.
>
> To be clear, I'm not questioning this change in particular. It looks
> good, and follows what has been done in the past on meson. I know we
> have been this a lot already, but I'm questioning whether we should
> continue to do so ?
>
> I asking because I also have a lot case like this coming up on audio
> for gxl and gxbb, where the same function can use different pins.
could you please look into Jerome's question?
personally I'm fine with either way, and changing my patch would be
quite trivial. but I'd like to know what's "the way to go" before
changing anything (and reverting that afterwards again).
Regards,
Martin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-09 19:47 ` Martin Blumenstingl
0 siblings, 0 replies; 57+ messages in thread
From: Martin Blumenstingl @ 2017-03-09 19:47 UTC (permalink / raw)
To: linus-amlogic
Hi Linus,
On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
>> - PWM_A
>> - PWM_B
>> - PWM_C
>> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
>> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
>> m>
>> ---
>> drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
>> +++++++++++++++++++++++++++++++
>> 1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> index 4ab94a85e306..91dfc498780b 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
>> = { PIN(GPIOZ_11, EE_OFF) };
>> static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
>> EE_OFF) };
>> static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
>> EE_OFF) };
>>
>> +static const unsigned int pwm_a_pins[] = {
>> PIN(GPIOX_6, EE_OFF) };
>> +
>> +static const unsigned int pwm_b_pins[] = {
>> PIN(GPIODV_29, EE_OFF) };
>> +
>> +static const unsigned int pwm_c_pins[] = {
>> PIN(GPIOZ_15, EE_OFF) };
>> +
>> +static const unsigned int pwm_d_pins[] = {
>> PIN(GPIODV_28, EE_OFF) };
>> +
>> static const unsigned int pwm_e_pins[] = {
>> PIN(GPIOX_16, EE_OFF) };
>>
>> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
>> EE_OFF) };
>> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
>> EE_OFF) };
>> +
>> static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
>> EE_OFF) };
>> static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
>> EE_OFF) };
>> static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
>> EE_OFF) };
>> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
>> = { PIN(GPIOAO_3, 0) };
>>
>> static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
>> };
>>
>> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
>> 0) };
>> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
>> 0) };
>> +
>> static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
>> 0) };
>>
>> static struct meson_pmx_group meson_gxl_periphs_groups[] = {
>> @@ -350,7 +364,9 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(uart_rts_a, 5, 16),
>> GROUP(uart_tx_c, 5, 13),
>> GROUP(uart_rx_c, 5, 12),
>> + GROUP(pwm_a, 5, 25),
>> GROUP(pwm_e, 5, 15),
>> + GROUP(pwm_f_x, 5, 14),
>>
>> /* Bank Z */
>> GROUP(eth_mdio, 4, 22),
>> @@ -367,6 +383,7 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(eth_txd1, 4, 12),
>> GROUP(eth_txd2, 4, 11),
>> GROUP(eth_txd3, 4, 10),
>> + GROUP(pwm_c, 3, 20),
>>
>> /* Bank H */
>> GROUP(hdmi_hpd, 6, 31),
>> @@ -382,6 +399,8 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(i2c_sda_b, 1, 12),
>> GROUP(i2c_sck_c, 1, 11),
>> GROUP(i2c_sda_c, 1, 10),
>> + GROUP(pwm_b, 2, 11),
>> + GROUP(pwm_d, 2, 12),
>>
>> /* Bank BOOT */
>> GROUP(emmc_nand_d07, 7, 31),
>> @@ -404,6 +423,9 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(sdcard_d2, 6, 0),
>> GROUP(sdcard_cmd, 6, 2),
>> GROUP(sdcard_clk, 6, 3),
>> +
>> + /* Bank CLK */
>> + GROUP(pwm_f_clk, 8, 30),
>> };
>>
>> static struct meson_pmx_group meson_gxl_aobus_groups[] = {
>> @@ -428,6 +450,8 @@ static struct meson_pmx_group
>> meson_gxl_aobus_groups[] = {
>> GROUP(uart_cts_ao_b, 0, 8),
>> GROUP(uart_rts_ao_b, 0, 7),
>> GROUP(remote_input_ao, 0, 0),
>> + GROUP(pwm_ao_a_3, 0, 22),
>> + GROUP(pwm_ao_a_8, 0, 17),
>> GROUP(pwm_ao_b, 0, 3),
>> };
>>
>> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
>> "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
>> };
>>
>> +static const char * const pwm_a_groups[] = {
>> + "pwm_a",
>> +};
>> +
>> +static const char * const pwm_b_groups[] = {
>> + "pwm_b",
>> +};
>> +
>> +static const char * const pwm_c_groups[] = {
>> + "pwm_c",
>> +};
>> +
>> +static const char * const pwm_d_groups[] = {
>> + "pwm_d",
>> +};
>> +
>> static const char * const pwm_e_groups[] = {
>> "pwm_e",
>> };
>>
>> +static const char * const pwm_f_clk_groups[] = {
>> + "pwm_f_clk",
>> +};
>> +
>> +static const char * const pwm_f_x_groups[] = {
>> + "pwm_f_x",
>> +};
>> +
>> static const char * const hdmi_hpd_groups[] = {
>> "hdmi_hpd",
>> };
>> @@ -542,6 +590,14 @@ static const char * const
>> remote_input_ao_groups[] = {
>> "remote_input_ao",
>> };
>>
>> +static const char * const pwm_ao_a_3_groups[] = {
>> + "pwm_ao_a_3",
>> +};
>> +
>> +static const char * const pwm_ao_a_8_groups[] = {
>> + "pwm_ao_a_8",
>> +};
>> +
>> static const char * const pwm_ao_b_groups[] = {
>> "pwm_ao_b",
>> };
>> @@ -559,7 +615,13 @@ static struct meson_pmx_func
>> meson_gxl_periphs_functions[] = {
>> FUNCTION(i2c_b),
>> FUNCTION(i2c_c),
>> FUNCTION(eth),
>> + FUNCTION(pwm_a),
>> + FUNCTION(pwm_b),
>> + FUNCTION(pwm_c),
>> + FUNCTION(pwm_d),
>> FUNCTION(pwm_e),
>> + FUNCTION(pwm_f_clk),
>> + FUNCTION(pwm_f_x),
>
> I wonder if having function named "pwm_f_clk" really makes sense ?
> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
> The actual pin used will be provided in the dt. Here, I suppose we
> could have this:
>
> +static const char * const pwm_f_groups[] = {
> + "pwm_f_x", "pwm_f_clk",
> +};
>
> Has far as I can see, on meson arch, the function does not carry much
> information anyway, except for prints.
>
> To be clear, I'm not questioning this change in particular. It looks
> good, and follows what has been done in the past on meson. I know we
> have been this a lot already, but I'm questioning whether we should
> continue to do so ?
>
> I asking because I also have a lot case like this coming up on audio
> for gxl and gxbb, where the same function can use different pins.
could you please look into Jerome's question?
personally I'm fine with either way, and changing my patch would be
quite trivial. but I'd like to know what's "the way to go" before
changing anything (and reverting that afterwards again).
Regards,
Martin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-09 19:47 ` Martin Blumenstingl
0 siblings, 0 replies; 57+ messages in thread
From: Martin Blumenstingl @ 2017-03-09 19:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Linus,
On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
>> - PWM_A
>> - PWM_B
>> - PWM_C
>> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
>> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
>> m>
>> ---
>> drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
>> +++++++++++++++++++++++++++++++
>> 1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> index 4ab94a85e306..91dfc498780b 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
>> = { PIN(GPIOZ_11, EE_OFF) };
>> static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
>> EE_OFF) };
>> static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
>> EE_OFF) };
>>
>> +static const unsigned int pwm_a_pins[] = {
>> PIN(GPIOX_6, EE_OFF) };
>> +
>> +static const unsigned int pwm_b_pins[] = {
>> PIN(GPIODV_29, EE_OFF) };
>> +
>> +static const unsigned int pwm_c_pins[] = {
>> PIN(GPIOZ_15, EE_OFF) };
>> +
>> +static const unsigned int pwm_d_pins[] = {
>> PIN(GPIODV_28, EE_OFF) };
>> +
>> static const unsigned int pwm_e_pins[] = {
>> PIN(GPIOX_16, EE_OFF) };
>>
>> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
>> EE_OFF) };
>> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
>> EE_OFF) };
>> +
>> static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
>> EE_OFF) };
>> static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
>> EE_OFF) };
>> static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
>> EE_OFF) };
>> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
>> = { PIN(GPIOAO_3, 0) };
>>
>> static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
>> };
>>
>> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
>> 0) };
>> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
>> 0) };
>> +
>> static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
>> 0) };
>>
>> static struct meson_pmx_group meson_gxl_periphs_groups[] = {
>> @@ -350,7 +364,9 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(uart_rts_a, 5, 16),
>> GROUP(uart_tx_c, 5, 13),
>> GROUP(uart_rx_c, 5, 12),
>> + GROUP(pwm_a, 5, 25),
>> GROUP(pwm_e, 5, 15),
>> + GROUP(pwm_f_x, 5, 14),
>>
>> /* Bank Z */
>> GROUP(eth_mdio, 4, 22),
>> @@ -367,6 +383,7 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(eth_txd1, 4, 12),
>> GROUP(eth_txd2, 4, 11),
>> GROUP(eth_txd3, 4, 10),
>> + GROUP(pwm_c, 3, 20),
>>
>> /* Bank H */
>> GROUP(hdmi_hpd, 6, 31),
>> @@ -382,6 +399,8 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(i2c_sda_b, 1, 12),
>> GROUP(i2c_sck_c, 1, 11),
>> GROUP(i2c_sda_c, 1, 10),
>> + GROUP(pwm_b, 2, 11),
>> + GROUP(pwm_d, 2, 12),
>>
>> /* Bank BOOT */
>> GROUP(emmc_nand_d07, 7, 31),
>> @@ -404,6 +423,9 @@ static struct meson_pmx_group
>> meson_gxl_periphs_groups[] = {
>> GROUP(sdcard_d2, 6, 0),
>> GROUP(sdcard_cmd, 6, 2),
>> GROUP(sdcard_clk, 6, 3),
>> +
>> + /* Bank CLK */
>> + GROUP(pwm_f_clk, 8, 30),
>> };
>>
>> static struct meson_pmx_group meson_gxl_aobus_groups[] = {
>> @@ -428,6 +450,8 @@ static struct meson_pmx_group
>> meson_gxl_aobus_groups[] = {
>> GROUP(uart_cts_ao_b, 0, 8),
>> GROUP(uart_rts_ao_b, 0, 7),
>> GROUP(remote_input_ao, 0, 0),
>> + GROUP(pwm_ao_a_3, 0, 22),
>> + GROUP(pwm_ao_a_8, 0, 17),
>> GROUP(pwm_ao_b, 0, 3),
>> };
>>
>> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
>> "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
>> };
>>
>> +static const char * const pwm_a_groups[] = {
>> + "pwm_a",
>> +};
>> +
>> +static const char * const pwm_b_groups[] = {
>> + "pwm_b",
>> +};
>> +
>> +static const char * const pwm_c_groups[] = {
>> + "pwm_c",
>> +};
>> +
>> +static const char * const pwm_d_groups[] = {
>> + "pwm_d",
>> +};
>> +
>> static const char * const pwm_e_groups[] = {
>> "pwm_e",
>> };
>>
>> +static const char * const pwm_f_clk_groups[] = {
>> + "pwm_f_clk",
>> +};
>> +
>> +static const char * const pwm_f_x_groups[] = {
>> + "pwm_f_x",
>> +};
>> +
>> static const char * const hdmi_hpd_groups[] = {
>> "hdmi_hpd",
>> };
>> @@ -542,6 +590,14 @@ static const char * const
>> remote_input_ao_groups[] = {
>> "remote_input_ao",
>> };
>>
>> +static const char * const pwm_ao_a_3_groups[] = {
>> + "pwm_ao_a_3",
>> +};
>> +
>> +static const char * const pwm_ao_a_8_groups[] = {
>> + "pwm_ao_a_8",
>> +};
>> +
>> static const char * const pwm_ao_b_groups[] = {
>> "pwm_ao_b",
>> };
>> @@ -559,7 +615,13 @@ static struct meson_pmx_func
>> meson_gxl_periphs_functions[] = {
>> FUNCTION(i2c_b),
>> FUNCTION(i2c_c),
>> FUNCTION(eth),
>> + FUNCTION(pwm_a),
>> + FUNCTION(pwm_b),
>> + FUNCTION(pwm_c),
>> + FUNCTION(pwm_d),
>> FUNCTION(pwm_e),
>> + FUNCTION(pwm_f_clk),
>> + FUNCTION(pwm_f_x),
>
> I wonder if having function named "pwm_f_clk" really makes sense ?
> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
> The actual pin used will be provided in the dt. Here, I suppose we
> could have this:
>
> +static const char * const pwm_f_groups[] = {
> + "pwm_f_x", "pwm_f_clk",
> +};
>
> Has far as I can see, on meson arch, the function does not carry much
> information anyway, except for prints.
>
> To be clear, I'm not questioning this change in particular. It looks
> good, and follows what has been done in the past on meson. I know we
> have been this a lot already, but I'm questioning whether we should
> continue to do so ?
>
> I asking because I also have a lot case like this coming up on audio
> for gxl and gxbb, where the same function can use different pins.
could you please look into Jerome's question?
personally I'm fine with either way, and changing my patch would be
quite trivial. but I'd like to know what's "the way to go" before
changing anything (and reverting that afterwards again).
Regards,
Martin
^ permalink raw reply [flat|nested] 57+ messages in thread[parent not found: <CAFBinCDty8v80tLxHJPR+SHv32LX834dPJUruRNqr6angx3OOQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-09 19:47 ` Martin Blumenstingl
(?)
@ 2017-03-11 0:16 ` Kevin Hilman
-1 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-11 0:16 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Jerome Brunet, linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-gpio-u79uwXL29TY76Z2rM5mHXA, carlo-KA+7E9HrN00dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA, will.deacon-5wv7dgnIgG8,
catalin.marinas-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> writes:
> Hi Linus,
>
> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
>>> - PWM_A
>>> - PWM_B
>>> - PWM_C
>>> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
>>> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23myhRSP0FMvGiw@public.gmane.org
>>> m>
>>> ---
>>> drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
>>> +++++++++++++++++++++++++++++++
>>> 1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> index 4ab94a85e306..91dfc498780b 100644
>>> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
>>> = { PIN(GPIOZ_11, EE_OFF) };
>>> static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
>>> EE_OFF) };
>>> static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
>>> EE_OFF) };
>>>
>>> +static const unsigned int pwm_a_pins[] = {
>>> PIN(GPIOX_6, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_b_pins[] = {
>>> PIN(GPIODV_29, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_c_pins[] = {
>>> PIN(GPIOZ_15, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_d_pins[] = {
>>> PIN(GPIODV_28, EE_OFF) };
>>> +
>>> static const unsigned int pwm_e_pins[] = {
>>> PIN(GPIOX_16, EE_OFF) };
>>>
>>> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
>>> EE_OFF) };
>>> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
>>> EE_OFF) };
>>> +
>>> static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
>>> EE_OFF) };
>>> static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
>>> EE_OFF) };
>>> static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
>>> EE_OFF) };
>>> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
>>> = { PIN(GPIOAO_3, 0) };
>>>
>>> static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
>>> };
>>>
>>> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
>>> 0) };
>>> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
>>> 0) };
>>> +
>>> static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
>>> 0) };
>>>
>>> static struct meson_pmx_group meson_gxl_periphs_groups[] = {
>>> @@ -350,7 +364,9 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(uart_rts_a, 5, 16),
>>> GROUP(uart_tx_c, 5, 13),
>>> GROUP(uart_rx_c, 5, 12),
>>> + GROUP(pwm_a, 5, 25),
>>> GROUP(pwm_e, 5, 15),
>>> + GROUP(pwm_f_x, 5, 14),
>>>
>>> /* Bank Z */
>>> GROUP(eth_mdio, 4, 22),
>>> @@ -367,6 +383,7 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(eth_txd1, 4, 12),
>>> GROUP(eth_txd2, 4, 11),
>>> GROUP(eth_txd3, 4, 10),
>>> + GROUP(pwm_c, 3, 20),
>>>
>>> /* Bank H */
>>> GROUP(hdmi_hpd, 6, 31),
>>> @@ -382,6 +399,8 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(i2c_sda_b, 1, 12),
>>> GROUP(i2c_sck_c, 1, 11),
>>> GROUP(i2c_sda_c, 1, 10),
>>> + GROUP(pwm_b, 2, 11),
>>> + GROUP(pwm_d, 2, 12),
>>>
>>> /* Bank BOOT */
>>> GROUP(emmc_nand_d07, 7, 31),
>>> @@ -404,6 +423,9 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(sdcard_d2, 6, 0),
>>> GROUP(sdcard_cmd, 6, 2),
>>> GROUP(sdcard_clk, 6, 3),
>>> +
>>> + /* Bank CLK */
>>> + GROUP(pwm_f_clk, 8, 30),
>>> };
>>>
>>> static struct meson_pmx_group meson_gxl_aobus_groups[] = {
>>> @@ -428,6 +450,8 @@ static struct meson_pmx_group
>>> meson_gxl_aobus_groups[] = {
>>> GROUP(uart_cts_ao_b, 0, 8),
>>> GROUP(uart_rts_ao_b, 0, 7),
>>> GROUP(remote_input_ao, 0, 0),
>>> + GROUP(pwm_ao_a_3, 0, 22),
>>> + GROUP(pwm_ao_a_8, 0, 17),
>>> GROUP(pwm_ao_b, 0, 3),
>>> };
>>>
>>> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
>>> "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
>>> };
>>>
>>> +static const char * const pwm_a_groups[] = {
>>> + "pwm_a",
>>> +};
>>> +
>>> +static const char * const pwm_b_groups[] = {
>>> + "pwm_b",
>>> +};
>>> +
>>> +static const char * const pwm_c_groups[] = {
>>> + "pwm_c",
>>> +};
>>> +
>>> +static const char * const pwm_d_groups[] = {
>>> + "pwm_d",
>>> +};
>>> +
>>> static const char * const pwm_e_groups[] = {
>>> "pwm_e",
>>> };
>>>
>>> +static const char * const pwm_f_clk_groups[] = {
>>> + "pwm_f_clk",
>>> +};
>>> +
>>> +static const char * const pwm_f_x_groups[] = {
>>> + "pwm_f_x",
>>> +};
>>> +
>>> static const char * const hdmi_hpd_groups[] = {
>>> "hdmi_hpd",
>>> };
>>> @@ -542,6 +590,14 @@ static const char * const
>>> remote_input_ao_groups[] = {
>>> "remote_input_ao",
>>> };
>>>
>>> +static const char * const pwm_ao_a_3_groups[] = {
>>> + "pwm_ao_a_3",
>>> +};
>>> +
>>> +static const char * const pwm_ao_a_8_groups[] = {
>>> + "pwm_ao_a_8",
>>> +};
>>> +
>>> static const char * const pwm_ao_b_groups[] = {
>>> "pwm_ao_b",
>>> };
>>> @@ -559,7 +615,13 @@ static struct meson_pmx_func
>>> meson_gxl_periphs_functions[] = {
>>> FUNCTION(i2c_b),
>>> FUNCTION(i2c_c),
>>> FUNCTION(eth),
>>> + FUNCTION(pwm_a),
>>> + FUNCTION(pwm_b),
>>> + FUNCTION(pwm_c),
>>> + FUNCTION(pwm_d),
>>> FUNCTION(pwm_e),
>>> + FUNCTION(pwm_f_clk),
>>> + FUNCTION(pwm_f_x),
>>
>> I wonder if having function named "pwm_f_clk" really makes sense ?
>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>> The actual pin used will be provided in the dt. Here, I suppose we
>> could have this:
>>
>> +static const char * const pwm_f_groups[] = {
>> + "pwm_f_x", "pwm_f_clk",
>> +};
>>
>> Has far as I can see, on meson arch, the function does not carry much
>> information anyway, except for prints.
>>
>> To be clear, I'm not questioning this change in particular. It looks
>> good, and follows what has been done in the past on meson. I know we
>> have been this a lot already, but I'm questioning whether we should
>> continue to do so ?
>>
>> I asking because I also have a lot case like this coming up on audio
>> for gxl and gxbb, where the same function can use different pins.
>
> could you please look into Jerome's question?
> personally I'm fine with either way, and changing my patch would be
> quite trivial. but I'd like to know what's "the way to go" before
> changing anything (and reverting that afterwards again).
I think Jerome is right.
Taking the PWM_F example, my (admittedely limited) understanding of
pinctrl/pinux is that there should be a single function (namely PWM_F)
but it could have multiple "groups" (namely GPIO_X and GPIOCLK). It's
up to the board specific DT to select the right group.
If that's the case, we've already let in some cases of doing this the
wrong way.
If I'm understanding things correctly, and after some clarification from
Linus, we should fix up the existing cases first.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-11 0:16 ` Kevin Hilman
0 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-11 0:16 UTC (permalink / raw)
To: linus-amlogic
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> Hi Linus,
>
> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
>>> - PWM_A
>>> - PWM_B
>>> - PWM_C
>>> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
>>> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
>>> m>
>>> ---
>>> drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
>>> +++++++++++++++++++++++++++++++
>>> 1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> index 4ab94a85e306..91dfc498780b 100644
>>> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
>>> = { PIN(GPIOZ_11, EE_OFF) };
>>> static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
>>> EE_OFF) };
>>> static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
>>> EE_OFF) };
>>>
>>> +static const unsigned int pwm_a_pins[] = {
>>> PIN(GPIOX_6, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_b_pins[] = {
>>> PIN(GPIODV_29, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_c_pins[] = {
>>> PIN(GPIOZ_15, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_d_pins[] = {
>>> PIN(GPIODV_28, EE_OFF) };
>>> +
>>> static const unsigned int pwm_e_pins[] = {
>>> PIN(GPIOX_16, EE_OFF) };
>>>
>>> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
>>> EE_OFF) };
>>> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
>>> EE_OFF) };
>>> +
>>> static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
>>> EE_OFF) };
>>> static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
>>> EE_OFF) };
>>> static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
>>> EE_OFF) };
>>> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
>>> = { PIN(GPIOAO_3, 0) };
>>>
>>> static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
>>> };
>>>
>>> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
>>> 0) };
>>> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
>>> 0) };
>>> +
>>> static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
>>> 0) };
>>>
>>> static struct meson_pmx_group meson_gxl_periphs_groups[] = {
>>> @@ -350,7 +364,9 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(uart_rts_a, 5, 16),
>>> GROUP(uart_tx_c, 5, 13),
>>> GROUP(uart_rx_c, 5, 12),
>>> + GROUP(pwm_a, 5, 25),
>>> GROUP(pwm_e, 5, 15),
>>> + GROUP(pwm_f_x, 5, 14),
>>>
>>> /* Bank Z */
>>> GROUP(eth_mdio, 4, 22),
>>> @@ -367,6 +383,7 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(eth_txd1, 4, 12),
>>> GROUP(eth_txd2, 4, 11),
>>> GROUP(eth_txd3, 4, 10),
>>> + GROUP(pwm_c, 3, 20),
>>>
>>> /* Bank H */
>>> GROUP(hdmi_hpd, 6, 31),
>>> @@ -382,6 +399,8 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(i2c_sda_b, 1, 12),
>>> GROUP(i2c_sck_c, 1, 11),
>>> GROUP(i2c_sda_c, 1, 10),
>>> + GROUP(pwm_b, 2, 11),
>>> + GROUP(pwm_d, 2, 12),
>>>
>>> /* Bank BOOT */
>>> GROUP(emmc_nand_d07, 7, 31),
>>> @@ -404,6 +423,9 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(sdcard_d2, 6, 0),
>>> GROUP(sdcard_cmd, 6, 2),
>>> GROUP(sdcard_clk, 6, 3),
>>> +
>>> + /* Bank CLK */
>>> + GROUP(pwm_f_clk, 8, 30),
>>> };
>>>
>>> static struct meson_pmx_group meson_gxl_aobus_groups[] = {
>>> @@ -428,6 +450,8 @@ static struct meson_pmx_group
>>> meson_gxl_aobus_groups[] = {
>>> GROUP(uart_cts_ao_b, 0, 8),
>>> GROUP(uart_rts_ao_b, 0, 7),
>>> GROUP(remote_input_ao, 0, 0),
>>> + GROUP(pwm_ao_a_3, 0, 22),
>>> + GROUP(pwm_ao_a_8, 0, 17),
>>> GROUP(pwm_ao_b, 0, 3),
>>> };
>>>
>>> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
>>> "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
>>> };
>>>
>>> +static const char * const pwm_a_groups[] = {
>>> + "pwm_a",
>>> +};
>>> +
>>> +static const char * const pwm_b_groups[] = {
>>> + "pwm_b",
>>> +};
>>> +
>>> +static const char * const pwm_c_groups[] = {
>>> + "pwm_c",
>>> +};
>>> +
>>> +static const char * const pwm_d_groups[] = {
>>> + "pwm_d",
>>> +};
>>> +
>>> static const char * const pwm_e_groups[] = {
>>> "pwm_e",
>>> };
>>>
>>> +static const char * const pwm_f_clk_groups[] = {
>>> + "pwm_f_clk",
>>> +};
>>> +
>>> +static const char * const pwm_f_x_groups[] = {
>>> + "pwm_f_x",
>>> +};
>>> +
>>> static const char * const hdmi_hpd_groups[] = {
>>> "hdmi_hpd",
>>> };
>>> @@ -542,6 +590,14 @@ static const char * const
>>> remote_input_ao_groups[] = {
>>> "remote_input_ao",
>>> };
>>>
>>> +static const char * const pwm_ao_a_3_groups[] = {
>>> + "pwm_ao_a_3",
>>> +};
>>> +
>>> +static const char * const pwm_ao_a_8_groups[] = {
>>> + "pwm_ao_a_8",
>>> +};
>>> +
>>> static const char * const pwm_ao_b_groups[] = {
>>> "pwm_ao_b",
>>> };
>>> @@ -559,7 +615,13 @@ static struct meson_pmx_func
>>> meson_gxl_periphs_functions[] = {
>>> FUNCTION(i2c_b),
>>> FUNCTION(i2c_c),
>>> FUNCTION(eth),
>>> + FUNCTION(pwm_a),
>>> + FUNCTION(pwm_b),
>>> + FUNCTION(pwm_c),
>>> + FUNCTION(pwm_d),
>>> FUNCTION(pwm_e),
>>> + FUNCTION(pwm_f_clk),
>>> + FUNCTION(pwm_f_x),
>>
>> I wonder if having function named "pwm_f_clk" really makes sense ?
>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>> The actual pin used will be provided in the dt. Here, I suppose we
>> could have this:
>>
>> +static const char * const pwm_f_groups[] = {
>> + "pwm_f_x", "pwm_f_clk",
>> +};
>>
>> Has far as I can see, on meson arch, the function does not carry much
>> information anyway, except for prints.
>>
>> To be clear, I'm not questioning this change in particular. It looks
>> good, and follows what has been done in the past on meson. I know we
>> have been this a lot already, but I'm questioning whether we should
>> continue to do so ?
>>
>> I asking because I also have a lot case like this coming up on audio
>> for gxl and gxbb, where the same function can use different pins.
>
> could you please look into Jerome's question?
> personally I'm fine with either way, and changing my patch would be
> quite trivial. but I'd like to know what's "the way to go" before
> changing anything (and reverting that afterwards again).
I think Jerome is right.
Taking the PWM_F example, my (admittedely limited) understanding of
pinctrl/pinux is that there should be a single function (namely PWM_F)
but it could have multiple "groups" (namely GPIO_X and GPIOCLK). It's
up to the board specific DT to select the right group.
If that's the case, we've already let in some cases of doing this the
wrong way.
If I'm understanding things correctly, and after some clarification from
Linus, we should fix up the existing cases first.
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-11 0:16 ` Kevin Hilman
0 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-11 0:16 UTC (permalink / raw)
To: linux-arm-kernel
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> Hi Linus,
>
> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>> This adds support for the missing PWM pins on Meson GXL SoCs, namely:
>>> - PWM_A
>>> - PWM_B
>>> - PWM_C
>>> - PWM_F (GPIOX_7 and GPIOCLK_1 can be selected as output)
>>> - PWM_AO_A (GPIOAO_3 and GPIOAO_8 can be selected as output)
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.co
>>> m>
>>> ---
>>> drivers/pinctrl/meson/pinctrl-meson-gxl.c | 64
>>> +++++++++++++++++++++++++++++++
>>> 1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> index 4ab94a85e306..91dfc498780b 100644
>>> --- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> +++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
>>> @@ -195,8 +195,19 @@ static const unsigned int eth_txd1_pins[]
>>> = { PIN(GPIOZ_11, EE_OFF) };
>>> static const unsigned int eth_txd2_pins[] = { PIN(GPIOZ_12,
>>> EE_OFF) };
>>> static const unsigned int eth_txd3_pins[] = { PIN(GPIOZ_13,
>>> EE_OFF) };
>>>
>>> +static const unsigned int pwm_a_pins[] = {
>>> PIN(GPIOX_6, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_b_pins[] = {
>>> PIN(GPIODV_29, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_c_pins[] = {
>>> PIN(GPIOZ_15, EE_OFF) };
>>> +
>>> +static const unsigned int pwm_d_pins[] = {
>>> PIN(GPIODV_28, EE_OFF) };
>>> +
>>> static const unsigned int pwm_e_pins[] = {
>>> PIN(GPIOX_16, EE_OFF) };
>>>
>>> +static const unsigned int pwm_f_clk_pins[] = { PIN(GPIOCLK_1,
>>> EE_OFF) };
>>> +static const unsigned int pwm_f_x_pins[] = { PIN(GPIOX_7,
>>> EE_OFF) };
>>> +
>>> static const unsigned int hdmi_hpd_pins[] = { PIN(GPIOH_0,
>>> EE_OFF) };
>>> static const unsigned int hdmi_sda_pins[] = { PIN(GPIOH_1,
>>> EE_OFF) };
>>> static const unsigned int hdmi_scl_pins[] = { PIN(GPIOH_2,
>>> EE_OFF) };
>>> @@ -225,6 +236,9 @@ static const unsigned int uart_rts_ao_b_pins[]
>>> = { PIN(GPIOAO_3, 0) };
>>>
>>> static const unsigned int remote_input_ao_pins[] = {PIN(GPIOAO_7, 0)
>>> };
>>>
>>> +static const unsigned int pwm_ao_a_3_pins[] = { PIN(GPIOAO_3,
>>> 0) };
>>> +static const unsigned int pwm_ao_a_8_pins[] = { PIN(GPIOAO_8,
>>> 0) };
>>> +
>>> static const unsigned int pwm_ao_b_pins[] = { PIN(GPIOAO_9,
>>> 0) };
>>>
>>> static struct meson_pmx_group meson_gxl_periphs_groups[] = {
>>> @@ -350,7 +364,9 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(uart_rts_a, 5, 16),
>>> GROUP(uart_tx_c, 5, 13),
>>> GROUP(uart_rx_c, 5, 12),
>>> + GROUP(pwm_a, 5, 25),
>>> GROUP(pwm_e, 5, 15),
>>> + GROUP(pwm_f_x, 5, 14),
>>>
>>> /* Bank Z */
>>> GROUP(eth_mdio, 4, 22),
>>> @@ -367,6 +383,7 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(eth_txd1, 4, 12),
>>> GROUP(eth_txd2, 4, 11),
>>> GROUP(eth_txd3, 4, 10),
>>> + GROUP(pwm_c, 3, 20),
>>>
>>> /* Bank H */
>>> GROUP(hdmi_hpd, 6, 31),
>>> @@ -382,6 +399,8 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(i2c_sda_b, 1, 12),
>>> GROUP(i2c_sck_c, 1, 11),
>>> GROUP(i2c_sda_c, 1, 10),
>>> + GROUP(pwm_b, 2, 11),
>>> + GROUP(pwm_d, 2, 12),
>>>
>>> /* Bank BOOT */
>>> GROUP(emmc_nand_d07, 7, 31),
>>> @@ -404,6 +423,9 @@ static struct meson_pmx_group
>>> meson_gxl_periphs_groups[] = {
>>> GROUP(sdcard_d2, 6, 0),
>>> GROUP(sdcard_cmd, 6, 2),
>>> GROUP(sdcard_clk, 6, 3),
>>> +
>>> + /* Bank CLK */
>>> + GROUP(pwm_f_clk, 8, 30),
>>> };
>>>
>>> static struct meson_pmx_group meson_gxl_aobus_groups[] = {
>>> @@ -428,6 +450,8 @@ static struct meson_pmx_group
>>> meson_gxl_aobus_groups[] = {
>>> GROUP(uart_cts_ao_b, 0, 8),
>>> GROUP(uart_rts_ao_b, 0, 7),
>>> GROUP(remote_input_ao, 0, 0),
>>> + GROUP(pwm_ao_a_3, 0, 22),
>>> + GROUP(pwm_ao_a_8, 0, 17),
>>> GROUP(pwm_ao_b, 0, 3),
>>> };
>>>
>>> @@ -513,10 +537,34 @@ static const char * const eth_groups[] = {
>>> "eth_txd0", "eth_txd1", "eth_txd2", "eth_txd3",
>>> };
>>>
>>> +static const char * const pwm_a_groups[] = {
>>> + "pwm_a",
>>> +};
>>> +
>>> +static const char * const pwm_b_groups[] = {
>>> + "pwm_b",
>>> +};
>>> +
>>> +static const char * const pwm_c_groups[] = {
>>> + "pwm_c",
>>> +};
>>> +
>>> +static const char * const pwm_d_groups[] = {
>>> + "pwm_d",
>>> +};
>>> +
>>> static const char * const pwm_e_groups[] = {
>>> "pwm_e",
>>> };
>>>
>>> +static const char * const pwm_f_clk_groups[] = {
>>> + "pwm_f_clk",
>>> +};
>>> +
>>> +static const char * const pwm_f_x_groups[] = {
>>> + "pwm_f_x",
>>> +};
>>> +
>>> static const char * const hdmi_hpd_groups[] = {
>>> "hdmi_hpd",
>>> };
>>> @@ -542,6 +590,14 @@ static const char * const
>>> remote_input_ao_groups[] = {
>>> "remote_input_ao",
>>> };
>>>
>>> +static const char * const pwm_ao_a_3_groups[] = {
>>> + "pwm_ao_a_3",
>>> +};
>>> +
>>> +static const char * const pwm_ao_a_8_groups[] = {
>>> + "pwm_ao_a_8",
>>> +};
>>> +
>>> static const char * const pwm_ao_b_groups[] = {
>>> "pwm_ao_b",
>>> };
>>> @@ -559,7 +615,13 @@ static struct meson_pmx_func
>>> meson_gxl_periphs_functions[] = {
>>> FUNCTION(i2c_b),
>>> FUNCTION(i2c_c),
>>> FUNCTION(eth),
>>> + FUNCTION(pwm_a),
>>> + FUNCTION(pwm_b),
>>> + FUNCTION(pwm_c),
>>> + FUNCTION(pwm_d),
>>> FUNCTION(pwm_e),
>>> + FUNCTION(pwm_f_clk),
>>> + FUNCTION(pwm_f_x),
>>
>> I wonder if having function named "pwm_f_clk" really makes sense ?
>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>> The actual pin used will be provided in the dt. Here, I suppose we
>> could have this:
>>
>> +static const char * const pwm_f_groups[] = {
>> + "pwm_f_x", "pwm_f_clk",
>> +};
>>
>> Has far as I can see, on meson arch, the function does not carry much
>> information anyway, except for prints.
>>
>> To be clear, I'm not questioning this change in particular. It looks
>> good, and follows what has been done in the past on meson. I know we
>> have been this a lot already, but I'm questioning whether we should
>> continue to do so ?
>>
>> I asking because I also have a lot case like this coming up on audio
>> for gxl and gxbb, where the same function can use different pins.
>
> could you please look into Jerome's question?
> personally I'm fine with either way, and changing my patch would be
> quite trivial. but I'd like to know what's "the way to go" before
> changing anything (and reverting that afterwards again).
I think Jerome is right.
Taking the PWM_F example, my (admittedely limited) understanding of
pinctrl/pinux is that there should be a single function (namely PWM_F)
but it could have multiple "groups" (namely GPIO_X and GPIOCLK). It's
up to the board specific DT to select the right group.
If that's the case, we've already let in some cases of doing this the
wrong way.
If I'm understanding things correctly, and after some clarification from
Linus, we should fix up the existing cases first.
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-09 19:47 ` Martin Blumenstingl
(?)
@ 2017-03-14 15:42 ` Linus Walleij
-1 siblings, 0 replies; 57+ messages in thread
From: Linus Walleij @ 2017-03-14 15:42 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Jerome Brunet, open list:ARM/Amlogic Meson...,
linux-gpio-u79uwXL29TY76Z2rM5mHXA, Kevin Hilman, Carlo Caione,
devicetree-u79uwXL29TY76Z2rM5mHXA, Will Deacon, Catalin Marinas,
Mark Rutland, Rob Herring,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>> + FUNCTION(pwm_f_clk),
>>> + FUNCTION(pwm_f_x),
>>
>> I wonder if having function named "pwm_f_clk" really makes sense ?
>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>> The actual pin used will be provided in the dt. Here, I suppose we
>> could have this:
>>
>> +static const char * const pwm_f_groups[] = {
>> + "pwm_f_x", "pwm_f_clk",
>> +};
>>
>> Has far as I can see, on meson arch, the function does not carry much
>> information anyway, except for prints.
>>
>> To be clear, I'm not questioning this change in particular. It looks
>> good, and follows what has been done in the past on meson. I know we
>> have been this a lot already, but I'm questioning whether we should
>> continue to do so ?
>>
>> I asking because I also have a lot case like this coming up on audio
>> for gxl and gxbb, where the same function can use different pins.
>
> could you please look into Jerome's question?
> personally I'm fine with either way, and changing my patch would be
> quite trivial. but I'd like to know what's "the way to go" before
> changing anything (and reverting that afterwards again).
I don't understand the question really.
I am not an expert on this system, if the people working with it
cannot tell a function from a group I don't know who can... certainly
not me.
What I can say is that pincontrol combines functions and groups to
states using a mapping. The functions should be something you poke
into a register, the groups are looser defined but may also be a
character of the hardware, but more usual a character of the
intended electronic usecase. Groups contain 1..n pins and can
be combined with some applicable functions.
Please re-read Documentation/pinctrl.txt very closely if anything is
unclear, I really put a lot of hours into getting that right. Especially
reexamine "Pinmux conventions".
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-14 15:42 ` Linus Walleij
0 siblings, 0 replies; 57+ messages in thread
From: Linus Walleij @ 2017-03-14 15:42 UTC (permalink / raw)
To: linus-amlogic
On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>> + FUNCTION(pwm_f_clk),
>>> + FUNCTION(pwm_f_x),
>>
>> I wonder if having function named "pwm_f_clk" really makes sense ?
>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>> The actual pin used will be provided in the dt. Here, I suppose we
>> could have this:
>>
>> +static const char * const pwm_f_groups[] = {
>> + "pwm_f_x", "pwm_f_clk",
>> +};
>>
>> Has far as I can see, on meson arch, the function does not carry much
>> information anyway, except for prints.
>>
>> To be clear, I'm not questioning this change in particular. It looks
>> good, and follows what has been done in the past on meson. I know we
>> have been this a lot already, but I'm questioning whether we should
>> continue to do so ?
>>
>> I asking because I also have a lot case like this coming up on audio
>> for gxl and gxbb, where the same function can use different pins.
>
> could you please look into Jerome's question?
> personally I'm fine with either way, and changing my patch would be
> quite trivial. but I'd like to know what's "the way to go" before
> changing anything (and reverting that afterwards again).
I don't understand the question really.
I am not an expert on this system, if the people working with it
cannot tell a function from a group I don't know who can... certainly
not me.
What I can say is that pincontrol combines functions and groups to
states using a mapping. The functions should be something you poke
into a register, the groups are looser defined but may also be a
character of the hardware, but more usual a character of the
intended electronic usecase. Groups contain 1..n pins and can
be combined with some applicable functions.
Please re-read Documentation/pinctrl.txt very closely if anything is
unclear, I really put a lot of hours into getting that right. Especially
reexamine "Pinmux conventions".
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-14 15:42 ` Linus Walleij
0 siblings, 0 replies; 57+ messages in thread
From: Linus Walleij @ 2017-03-14 15:42 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>> + FUNCTION(pwm_f_clk),
>>> + FUNCTION(pwm_f_x),
>>
>> I wonder if having function named "pwm_f_clk" really makes sense ?
>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>> The actual pin used will be provided in the dt. Here, I suppose we
>> could have this:
>>
>> +static const char * const pwm_f_groups[] = {
>> + "pwm_f_x", "pwm_f_clk",
>> +};
>>
>> Has far as I can see, on meson arch, the function does not carry much
>> information anyway, except for prints.
>>
>> To be clear, I'm not questioning this change in particular. It looks
>> good, and follows what has been done in the past on meson. I know we
>> have been this a lot already, but I'm questioning whether we should
>> continue to do so ?
>>
>> I asking because I also have a lot case like this coming up on audio
>> for gxl and gxbb, where the same function can use different pins.
>
> could you please look into Jerome's question?
> personally I'm fine with either way, and changing my patch would be
> quite trivial. but I'd like to know what's "the way to go" before
> changing anything (and reverting that afterwards again).
I don't understand the question really.
I am not an expert on this system, if the people working with it
cannot tell a function from a group I don't know who can... certainly
not me.
What I can say is that pincontrol combines functions and groups to
states using a mapping. The functions should be something you poke
into a register, the groups are looser defined but may also be a
character of the hardware, but more usual a character of the
intended electronic usecase. Groups contain 1..n pins and can
be combined with some applicable functions.
Please re-read Documentation/pinctrl.txt very closely if anything is
unclear, I really put a lot of hours into getting that right. Especially
reexamine "Pinmux conventions".
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-14 15:42 ` Linus Walleij
(?)
@ 2017-03-15 9:59 ` Neil Armstrong
-1 siblings, 0 replies; 57+ messages in thread
From: Neil Armstrong @ 2017-03-15 9:59 UTC (permalink / raw)
To: Linus Walleij, Martin Blumenstingl
Cc: Mark Rutland, devicetree, Kevin Hilman, Will Deacon, linux-gpio,
Rob Herring, Catalin Marinas, Carlo Caione,
open list:ARM/Amlogic Meson...,
linux-arm-kernel, Jerome Brunet
On 03/14/2017 04:42 PM, Linus Walleij wrote:
> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>
>>>> + FUNCTION(pwm_f_clk),
>>>> + FUNCTION(pwm_f_x),
>>>
>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>> The actual pin used will be provided in the dt. Here, I suppose we
>>> could have this:
>>>
>>> +static const char * const pwm_f_groups[] = {
>>> + "pwm_f_x", "pwm_f_clk",
>>> +};
>>>
>>> Has far as I can see, on meson arch, the function does not carry much
>>> information anyway, except for prints.
>>>
>>> To be clear, I'm not questioning this change in particular. It looks
>>> good, and follows what has been done in the past on meson. I know we
>>> have been this a lot already, but I'm questioning whether we should
>>> continue to do so ?
>>>
>>> I asking because I also have a lot case like this coming up on audio
>>> for gxl and gxbb, where the same function can use different pins.
>>
>> could you please look into Jerome's question?
>> personally I'm fine with either way, and changing my patch would be
>> quite trivial. but I'd like to know what's "the way to go" before
>> changing anything (and reverting that afterwards again).
>
> I don't understand the question really.
>
> I am not an expert on this system, if the people working with it
> cannot tell a function from a group I don't know who can... certainly
> not me.
>
> What I can say is that pincontrol combines functions and groups to
> states using a mapping. The functions should be something you poke
> into a register, the groups are looser defined but may also be a
> character of the hardware, but more usual a character of the
> intended electronic usecase. Groups contain 1..n pins and can
> be combined with some applicable functions.
>
> Please re-read Documentation/pinctrl.txt very closely if anything is
> unclear, I really put a lot of hours into getting that right. Especially
> reexamine "Pinmux conventions".
The point pushed by Jerome was purely cosmetic since the groups in the meson
pinctrl driver are purely cosmetic, since only the GPIO group is handled,
other groups are all handled the same.
This is because I pushed all the PWM pins in a separate group, but functionnaly
the internal signal (i.e. PWM F) is the same for multiple pins and should be
a single "PWM F" group instead of multiple ones.
My advice is to leave the PWM groups as is, and push new pins/functions/groups
grouped with the internal signal name if split on multiple pins.
This will be the case for audio, since the I2S pins can be configures on multiple
different pins.
Thanks,
Neil
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-15 9:59 ` Neil Armstrong
0 siblings, 0 replies; 57+ messages in thread
From: Neil Armstrong @ 2017-03-15 9:59 UTC (permalink / raw)
To: linus-amlogic
On 03/14/2017 04:42 PM, Linus Walleij wrote:
> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>
>>>> + FUNCTION(pwm_f_clk),
>>>> + FUNCTION(pwm_f_x),
>>>
>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>> The actual pin used will be provided in the dt. Here, I suppose we
>>> could have this:
>>>
>>> +static const char * const pwm_f_groups[] = {
>>> + "pwm_f_x", "pwm_f_clk",
>>> +};
>>>
>>> Has far as I can see, on meson arch, the function does not carry much
>>> information anyway, except for prints.
>>>
>>> To be clear, I'm not questioning this change in particular. It looks
>>> good, and follows what has been done in the past on meson. I know we
>>> have been this a lot already, but I'm questioning whether we should
>>> continue to do so ?
>>>
>>> I asking because I also have a lot case like this coming up on audio
>>> for gxl and gxbb, where the same function can use different pins.
>>
>> could you please look into Jerome's question?
>> personally I'm fine with either way, and changing my patch would be
>> quite trivial. but I'd like to know what's "the way to go" before
>> changing anything (and reverting that afterwards again).
>
> I don't understand the question really.
>
> I am not an expert on this system, if the people working with it
> cannot tell a function from a group I don't know who can... certainly
> not me.
>
> What I can say is that pincontrol combines functions and groups to
> states using a mapping. The functions should be something you poke
> into a register, the groups are looser defined but may also be a
> character of the hardware, but more usual a character of the
> intended electronic usecase. Groups contain 1..n pins and can
> be combined with some applicable functions.
>
> Please re-read Documentation/pinctrl.txt very closely if anything is
> unclear, I really put a lot of hours into getting that right. Especially
> reexamine "Pinmux conventions".
The point pushed by Jerome was purely cosmetic since the groups in the meson
pinctrl driver are purely cosmetic, since only the GPIO group is handled,
other groups are all handled the same.
This is because I pushed all the PWM pins in a separate group, but functionnaly
the internal signal (i.e. PWM F) is the same for multiple pins and should be
a single "PWM F" group instead of multiple ones.
My advice is to leave the PWM groups as is, and push new pins/functions/groups
grouped with the internal signal name if split on multiple pins.
This will be the case for audio, since the I2S pins can be configures on multiple
different pins.
Thanks,
Neil
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-15 9:59 ` Neil Armstrong
0 siblings, 0 replies; 57+ messages in thread
From: Neil Armstrong @ 2017-03-15 9:59 UTC (permalink / raw)
To: linux-arm-kernel
On 03/14/2017 04:42 PM, Linus Walleij wrote:
> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>
>>>> + FUNCTION(pwm_f_clk),
>>>> + FUNCTION(pwm_f_x),
>>>
>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>> The actual pin used will be provided in the dt. Here, I suppose we
>>> could have this:
>>>
>>> +static const char * const pwm_f_groups[] = {
>>> + "pwm_f_x", "pwm_f_clk",
>>> +};
>>>
>>> Has far as I can see, on meson arch, the function does not carry much
>>> information anyway, except for prints.
>>>
>>> To be clear, I'm not questioning this change in particular. It looks
>>> good, and follows what has been done in the past on meson. I know we
>>> have been this a lot already, but I'm questioning whether we should
>>> continue to do so ?
>>>
>>> I asking because I also have a lot case like this coming up on audio
>>> for gxl and gxbb, where the same function can use different pins.
>>
>> could you please look into Jerome's question?
>> personally I'm fine with either way, and changing my patch would be
>> quite trivial. but I'd like to know what's "the way to go" before
>> changing anything (and reverting that afterwards again).
>
> I don't understand the question really.
>
> I am not an expert on this system, if the people working with it
> cannot tell a function from a group I don't know who can... certainly
> not me.
>
> What I can say is that pincontrol combines functions and groups to
> states using a mapping. The functions should be something you poke
> into a register, the groups are looser defined but may also be a
> character of the hardware, but more usual a character of the
> intended electronic usecase. Groups contain 1..n pins and can
> be combined with some applicable functions.
>
> Please re-read Documentation/pinctrl.txt very closely if anything is
> unclear, I really put a lot of hours into getting that right. Especially
> reexamine "Pinmux conventions".
The point pushed by Jerome was purely cosmetic since the groups in the meson
pinctrl driver are purely cosmetic, since only the GPIO group is handled,
other groups are all handled the same.
This is because I pushed all the PWM pins in a separate group, but functionnaly
the internal signal (i.e. PWM F) is the same for multiple pins and should be
a single "PWM F" group instead of multiple ones.
My advice is to leave the PWM groups as is, and push new pins/functions/groups
grouped with the internal signal name if split on multiple pins.
This will be the case for audio, since the I2S pins can be configures on multiple
different pins.
Thanks,
Neil
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-15 9:59 ` Neil Armstrong
(?)
@ 2017-03-15 19:12 ` Kevin Hilman
-1 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-15 19:12 UTC (permalink / raw)
To: Neil Armstrong
Cc: Mark Rutland, devicetree, Martin Blumenstingl, Catalin Marinas,
Linus Walleij, Will Deacon, linux-gpio, Rob Herring,
Carlo Caione, open list:ARM/Amlogic Meson...,
linux-arm-kernel, Jerome Brunet
Neil Armstrong <narmstrong@baylibre.com> writes:
> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>
>>>>> + FUNCTION(pwm_f_clk),
>>>>> + FUNCTION(pwm_f_x),
>>>>
>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>> could have this:
>>>>
>>>> +static const char * const pwm_f_groups[] = {
>>>> + "pwm_f_x", "pwm_f_clk",
>>>> +};
>>>>
>>>> Has far as I can see, on meson arch, the function does not carry much
>>>> information anyway, except for prints.
>>>>
>>>> To be clear, I'm not questioning this change in particular. It looks
>>>> good, and follows what has been done in the past on meson. I know we
>>>> have been this a lot already, but I'm questioning whether we should
>>>> continue to do so ?
>>>>
>>>> I asking because I also have a lot case like this coming up on audio
>>>> for gxl and gxbb, where the same function can use different pins.
>>>
>>> could you please look into Jerome's question?
>>> personally I'm fine with either way, and changing my patch would be
>>> quite trivial. but I'd like to know what's "the way to go" before
>>> changing anything (and reverting that afterwards again).
>>
>> I don't understand the question really.
>>
>> I am not an expert on this system, if the people working with it
>> cannot tell a function from a group I don't know who can... certainly
>> not me.
>>
>> What I can say is that pincontrol combines functions and groups to
>> states using a mapping. The functions should be something you poke
>> into a register, the groups are looser defined but may also be a
>> character of the hardware, but more usual a character of the
>> intended electronic usecase. Groups contain 1..n pins and can
>> be combined with some applicable functions.
>>
>> Please re-read Documentation/pinctrl.txt very closely if anything is
>> unclear, I really put a lot of hours into getting that right. Especially
>> reexamine "Pinmux conventions".
>
> The point pushed by Jerome was purely cosmetic since the groups in the meson
> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
> other groups are all handled the same.
handled the same... as what?
> This is because I pushed all the PWM pins in a separate group, but functionnaly
> the internal signal (i.e. PWM F) is the same for multiple pins and should be
> a single "PWM F" group instead of multiple ones.
>
> My advice is to leave the PWM groups as is,
Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
> and push new pins/functions/groups
> grouped with the internal signal name if split on multiple pins.
Can somone do a quick patch for PWM_F for example, also showing how this
will look in the DT if someone wants to switch between the PWM_F on GPIOX
or GPIOCLK?
We shouldalso verify that the driver is detecting/removing conflicts
properly when something else is already using that pin (e.g. SDIO_IRQ
shares pin GPIOX_7 with PWM_F)
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-15 19:12 ` Kevin Hilman
0 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-15 19:12 UTC (permalink / raw)
To: linus-amlogic
Neil Armstrong <narmstrong@baylibre.com> writes:
> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>
>>>>> + FUNCTION(pwm_f_clk),
>>>>> + FUNCTION(pwm_f_x),
>>>>
>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>> could have this:
>>>>
>>>> +static const char * const pwm_f_groups[] = {
>>>> + "pwm_f_x", "pwm_f_clk",
>>>> +};
>>>>
>>>> Has far as I can see, on meson arch, the function does not carry much
>>>> information anyway, except for prints.
>>>>
>>>> To be clear, I'm not questioning this change in particular. It looks
>>>> good, and follows what has been done in the past on meson. I know we
>>>> have been this a lot already, but I'm questioning whether we should
>>>> continue to do so ?
>>>>
>>>> I asking because I also have a lot case like this coming up on audio
>>>> for gxl and gxbb, where the same function can use different pins.
>>>
>>> could you please look into Jerome's question?
>>> personally I'm fine with either way, and changing my patch would be
>>> quite trivial. but I'd like to know what's "the way to go" before
>>> changing anything (and reverting that afterwards again).
>>
>> I don't understand the question really.
>>
>> I am not an expert on this system, if the people working with it
>> cannot tell a function from a group I don't know who can... certainly
>> not me.
>>
>> What I can say is that pincontrol combines functions and groups to
>> states using a mapping. The functions should be something you poke
>> into a register, the groups are looser defined but may also be a
>> character of the hardware, but more usual a character of the
>> intended electronic usecase. Groups contain 1..n pins and can
>> be combined with some applicable functions.
>>
>> Please re-read Documentation/pinctrl.txt very closely if anything is
>> unclear, I really put a lot of hours into getting that right. Especially
>> reexamine "Pinmux conventions".
>
> The point pushed by Jerome was purely cosmetic since the groups in the meson
> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
> other groups are all handled the same.
handled the same... as what?
> This is because I pushed all the PWM pins in a separate group, but functionnaly
> the internal signal (i.e. PWM F) is the same for multiple pins and should be
> a single "PWM F" group instead of multiple ones.
>
> My advice is to leave the PWM groups as is,
Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
> and push new pins/functions/groups
> grouped with the internal signal name if split on multiple pins.
Can somone do a quick patch for PWM_F for example, also showing how this
will look in the DT if someone wants to switch between the PWM_F on GPIOX
or GPIOCLK?
We shouldalso verify that the driver is detecting/removing conflicts
properly when something else is already using that pin (e.g. SDIO_IRQ
shares pin GPIOX_7 with PWM_F)
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-15 19:12 ` Kevin Hilman
0 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-15 19:12 UTC (permalink / raw)
To: linux-arm-kernel
Neil Armstrong <narmstrong@baylibre.com> writes:
> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>
>>>>> + FUNCTION(pwm_f_clk),
>>>>> + FUNCTION(pwm_f_x),
>>>>
>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>> could have this:
>>>>
>>>> +static const char * const pwm_f_groups[] = {
>>>> + "pwm_f_x", "pwm_f_clk",
>>>> +};
>>>>
>>>> Has far as I can see, on meson arch, the function does not carry much
>>>> information anyway, except for prints.
>>>>
>>>> To be clear, I'm not questioning this change in particular. It looks
>>>> good, and follows what has been done in the past on meson. I know we
>>>> have been this a lot already, but I'm questioning whether we should
>>>> continue to do so ?
>>>>
>>>> I asking because I also have a lot case like this coming up on audio
>>>> for gxl and gxbb, where the same function can use different pins.
>>>
>>> could you please look into Jerome's question?
>>> personally I'm fine with either way, and changing my patch would be
>>> quite trivial. but I'd like to know what's "the way to go" before
>>> changing anything (and reverting that afterwards again).
>>
>> I don't understand the question really.
>>
>> I am not an expert on this system, if the people working with it
>> cannot tell a function from a group I don't know who can... certainly
>> not me.
>>
>> What I can say is that pincontrol combines functions and groups to
>> states using a mapping. The functions should be something you poke
>> into a register, the groups are looser defined but may also be a
>> character of the hardware, but more usual a character of the
>> intended electronic usecase. Groups contain 1..n pins and can
>> be combined with some applicable functions.
>>
>> Please re-read Documentation/pinctrl.txt very closely if anything is
>> unclear, I really put a lot of hours into getting that right. Especially
>> reexamine "Pinmux conventions".
>
> The point pushed by Jerome was purely cosmetic since the groups in the meson
> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
> other groups are all handled the same.
handled the same... as what?
> This is because I pushed all the PWM pins in a separate group, but functionnaly
> the internal signal (i.e. PWM F) is the same for multiple pins and should be
> a single "PWM F" group instead of multiple ones.
>
> My advice is to leave the PWM groups as is,
Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
> and push new pins/functions/groups
> grouped with the internal signal name if split on multiple pins.
Can somone do a quick patch for PWM_F for example, also showing how this
will look in the DT if someone wants to switch between the PWM_F on GPIOX
or GPIOCLK?
We shouldalso verify that the driver is detecting/removing conflicts
properly when something else is already using that pin (e.g. SDIO_IRQ
shares pin GPIOX_7 with PWM_F)
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-15 19:12 ` Kevin Hilman
(?)
@ 2017-03-15 20:11 ` Martin Blumenstingl
-1 siblings, 0 replies; 57+ messages in thread
From: Martin Blumenstingl @ 2017-03-15 20:11 UTC (permalink / raw)
To: Kevin Hilman
Cc: Mark Rutland, devicetree, Neil Armstrong, Catalin Marinas,
Linus Walleij, Will Deacon, linux-gpio, Rob Herring,
Carlo Caione, open list:ARM/Amlogic Meson...,
linux-arm-kernel, Jerome Brunet
On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
>
>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>
>>>>>> + FUNCTION(pwm_f_clk),
>>>>>> + FUNCTION(pwm_f_x),
>>>>>
>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>> could have this:
>>>>>
>>>>> +static const char * const pwm_f_groups[] = {
>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>> +};
>>>>>
>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>> information anyway, except for prints.
>>>>>
>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>> good, and follows what has been done in the past on meson. I know we
>>>>> have been this a lot already, but I'm questioning whether we should
>>>>> continue to do so ?
>>>>>
>>>>> I asking because I also have a lot case like this coming up on audio
>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>
>>>> could you please look into Jerome's question?
>>>> personally I'm fine with either way, and changing my patch would be
>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>> changing anything (and reverting that afterwards again).
>>>
>>> I don't understand the question really.
>>>
>>> I am not an expert on this system, if the people working with it
>>> cannot tell a function from a group I don't know who can... certainly
>>> not me.
>>>
>>> What I can say is that pincontrol combines functions and groups to
>>> states using a mapping. The functions should be something you poke
>>> into a register, the groups are looser defined but may also be a
>>> character of the hardware, but more usual a character of the
>>> intended electronic usecase. Groups contain 1..n pins and can
>>> be combined with some applicable functions.
>>>
>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>> unclear, I really put a lot of hours into getting that right. Especially
>>> reexamine "Pinmux conventions".
>>
>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>> other groups are all handled the same.
>
> handled the same... as what?
>
>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>> a single "PWM F" group instead of multiple ones.
>>
>> My advice is to leave the PWM groups as is,
>
> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>
>> and push new pins/functions/groups
>> grouped with the internal signal name if split on multiple pins.
>
> Can somone do a quick patch for PWM_F for example, also showing how this
> will look in the DT if someone wants to switch between the PWM_F on GPIOX
> or GPIOCLK?
it would look like this (node name, label and group stay the same,
function does not contain the _x/_clk suffix anymore):
pwm_f_clk_pins: pwm_f_clk {
mux {
groups = "pwm_f_clk";
function = "pwm_f";
};
};
pwm_f_x_pins: pwm_f_x {
mux {
groups = "pwm_f_x";
function = "pwm_f";
};
};
> We shouldalso verify that the driver is detecting/removing conflicts
> properly when something else is already using that pin (e.g. SDIO_IRQ
> shares pin GPIOX_7 with PWM_F)
if the same pin is assigned to two devices then the pinctrl subsystem
will throw an error (we don't have to take care of this, it's how I
discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
however, I have not tested yet what happens if the same function is
assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
being routed to *both* pins or just one pin?). if the same function
cannot be used by two pins simultaneously then we should probably use
function "pwm_f" instead of "pwm_f_x" (just an example) so we can
detect these "conflicts".
Regards,
Martin
[0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-15 20:11 ` Martin Blumenstingl
0 siblings, 0 replies; 57+ messages in thread
From: Martin Blumenstingl @ 2017-03-15 20:11 UTC (permalink / raw)
To: linus-amlogic
On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
>
>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>
>>>>>> + FUNCTION(pwm_f_clk),
>>>>>> + FUNCTION(pwm_f_x),
>>>>>
>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>> could have this:
>>>>>
>>>>> +static const char * const pwm_f_groups[] = {
>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>> +};
>>>>>
>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>> information anyway, except for prints.
>>>>>
>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>> good, and follows what has been done in the past on meson. I know we
>>>>> have been this a lot already, but I'm questioning whether we should
>>>>> continue to do so ?
>>>>>
>>>>> I asking because I also have a lot case like this coming up on audio
>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>
>>>> could you please look into Jerome's question?
>>>> personally I'm fine with either way, and changing my patch would be
>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>> changing anything (and reverting that afterwards again).
>>>
>>> I don't understand the question really.
>>>
>>> I am not an expert on this system, if the people working with it
>>> cannot tell a function from a group I don't know who can... certainly
>>> not me.
>>>
>>> What I can say is that pincontrol combines functions and groups to
>>> states using a mapping. The functions should be something you poke
>>> into a register, the groups are looser defined but may also be a
>>> character of the hardware, but more usual a character of the
>>> intended electronic usecase. Groups contain 1..n pins and can
>>> be combined with some applicable functions.
>>>
>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>> unclear, I really put a lot of hours into getting that right. Especially
>>> reexamine "Pinmux conventions".
>>
>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>> other groups are all handled the same.
>
> handled the same... as what?
>
>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>> a single "PWM F" group instead of multiple ones.
>>
>> My advice is to leave the PWM groups as is,
>
> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>
>> and push new pins/functions/groups
>> grouped with the internal signal name if split on multiple pins.
>
> Can somone do a quick patch for PWM_F for example, also showing how this
> will look in the DT if someone wants to switch between the PWM_F on GPIOX
> or GPIOCLK?
it would look like this (node name, label and group stay the same,
function does not contain the _x/_clk suffix anymore):
pwm_f_clk_pins: pwm_f_clk {
mux {
groups = "pwm_f_clk";
function = "pwm_f";
};
};
pwm_f_x_pins: pwm_f_x {
mux {
groups = "pwm_f_x";
function = "pwm_f";
};
};
> We shouldalso verify that the driver is detecting/removing conflicts
> properly when something else is already using that pin (e.g. SDIO_IRQ
> shares pin GPIOX_7 with PWM_F)
if the same pin is assigned to two devices then the pinctrl subsystem
will throw an error (we don't have to take care of this, it's how I
discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
however, I have not tested yet what happens if the same function is
assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
being routed to *both* pins or just one pin?). if the same function
cannot be used by two pins simultaneously then we should probably use
function "pwm_f" instead of "pwm_f_x" (just an example) so we can
detect these "conflicts".
Regards,
Martin
[0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-15 20:11 ` Martin Blumenstingl
0 siblings, 0 replies; 57+ messages in thread
From: Martin Blumenstingl @ 2017-03-15 20:11 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
>
>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>
>>>>>> + FUNCTION(pwm_f_clk),
>>>>>> + FUNCTION(pwm_f_x),
>>>>>
>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>> could have this:
>>>>>
>>>>> +static const char * const pwm_f_groups[] = {
>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>> +};
>>>>>
>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>> information anyway, except for prints.
>>>>>
>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>> good, and follows what has been done in the past on meson. I know we
>>>>> have been this a lot already, but I'm questioning whether we should
>>>>> continue to do so ?
>>>>>
>>>>> I asking because I also have a lot case like this coming up on audio
>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>
>>>> could you please look into Jerome's question?
>>>> personally I'm fine with either way, and changing my patch would be
>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>> changing anything (and reverting that afterwards again).
>>>
>>> I don't understand the question really.
>>>
>>> I am not an expert on this system, if the people working with it
>>> cannot tell a function from a group I don't know who can... certainly
>>> not me.
>>>
>>> What I can say is that pincontrol combines functions and groups to
>>> states using a mapping. The functions should be something you poke
>>> into a register, the groups are looser defined but may also be a
>>> character of the hardware, but more usual a character of the
>>> intended electronic usecase. Groups contain 1..n pins and can
>>> be combined with some applicable functions.
>>>
>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>> unclear, I really put a lot of hours into getting that right. Especially
>>> reexamine "Pinmux conventions".
>>
>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>> other groups are all handled the same.
>
> handled the same... as what?
>
>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>> a single "PWM F" group instead of multiple ones.
>>
>> My advice is to leave the PWM groups as is,
>
> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>
>> and push new pins/functions/groups
>> grouped with the internal signal name if split on multiple pins.
>
> Can somone do a quick patch for PWM_F for example, also showing how this
> will look in the DT if someone wants to switch between the PWM_F on GPIOX
> or GPIOCLK?
it would look like this (node name, label and group stay the same,
function does not contain the _x/_clk suffix anymore):
pwm_f_clk_pins: pwm_f_clk {
mux {
groups = "pwm_f_clk";
function = "pwm_f";
};
};
pwm_f_x_pins: pwm_f_x {
mux {
groups = "pwm_f_x";
function = "pwm_f";
};
};
> We shouldalso verify that the driver is detecting/removing conflicts
> properly when something else is already using that pin (e.g. SDIO_IRQ
> shares pin GPIOX_7 with PWM_F)
if the same pin is assigned to two devices then the pinctrl subsystem
will throw an error (we don't have to take care of this, it's how I
discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
however, I have not tested yet what happens if the same function is
assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
being routed to *both* pins or just one pin?). if the same function
cannot be used by two pins simultaneously then we should probably use
function "pwm_f" instead of "pwm_f_x" (just an example) so we can
detect these "conflicts".
Regards,
Martin
[0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
^ permalink raw reply [flat|nested] 57+ messages in thread[parent not found: <CAFBinCCCgbSvh8B81nXP6D11d5WZZ+71jDgvsjYDsWTb=V78Xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-15 20:11 ` Martin Blumenstingl
(?)
@ 2017-03-16 13:54 ` Neil Armstrong
-1 siblings, 0 replies; 57+ messages in thread
From: Neil Armstrong @ 2017-03-16 13:54 UTC (permalink / raw)
To: Martin Blumenstingl, Kevin Hilman
Cc: Linus Walleij, Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
Will Deacon, linux-gpio-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Catalin Marinas, Carlo Caione, open list:ARM/Amlogic Meson...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jerome Brunet
On 03/15/2017 09:11 PM, Martin Blumenstingl wrote:
> On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>> Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> writes:
>>
>>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>>> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>>
>>>>>>> + FUNCTION(pwm_f_clk),
>>>>>>> + FUNCTION(pwm_f_x),
>>>>>>
>>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>>> could have this:
>>>>>>
>>>>>> +static const char * const pwm_f_groups[] = {
>>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>>> +};
>>>>>>
>>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>>> information anyway, except for prints.
>>>>>>
>>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>>> good, and follows what has been done in the past on meson. I know we
>>>>>> have been this a lot already, but I'm questioning whether we should
>>>>>> continue to do so ?
>>>>>>
>>>>>> I asking because I also have a lot case like this coming up on audio
>>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>>
>>>>> could you please look into Jerome's question?
>>>>> personally I'm fine with either way, and changing my patch would be
>>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>>> changing anything (and reverting that afterwards again).
>>>>
>>>> I don't understand the question really.
>>>>
>>>> I am not an expert on this system, if the people working with it
>>>> cannot tell a function from a group I don't know who can... certainly
>>>> not me.
>>>>
>>>> What I can say is that pincontrol combines functions and groups to
>>>> states using a mapping. The functions should be something you poke
>>>> into a register, the groups are looser defined but may also be a
>>>> character of the hardware, but more usual a character of the
>>>> intended electronic usecase. Groups contain 1..n pins and can
>>>> be combined with some applicable functions.
>>>>
>>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>>> unclear, I really put a lot of hours into getting that right. Especially
>>>> reexamine "Pinmux conventions".
>>>
>>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>>> other groups are all handled the same.
>>
>> handled the same... as what?
>>
>>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>>> a single "PWM F" group instead of multiple ones.
>>>
>>> My advice is to leave the PWM groups as is,
>>
>> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>>
>>> and push new pins/functions/groups
>>> grouped with the internal signal name if split on multiple pins.
>>
>> Can somone do a quick patch for PWM_F for example, also showing how this
>> will look in the DT if someone wants to switch between the PWM_F on GPIOX
>> or GPIOCLK?
> it would look like this (node name, label and group stay the same,
> function does not contain the _x/_clk suffix anymore):
> pwm_f_clk_pins: pwm_f_clk {
> mux {
> groups = "pwm_f_clk";
> function = "pwm_f";
> };
> };
>
> pwm_f_x_pins: pwm_f_x {
> mux {
> groups = "pwm_f_x";
> function = "pwm_f";
> };
> };
>
>> We shouldalso verify that the driver is detecting/removing conflicts
>> properly when something else is already using that pin (e.g. SDIO_IRQ
>> shares pin GPIOX_7 with PWM_F)
> if the same pin is assigned to two devices then the pinctrl subsystem
> will throw an error (we don't have to take care of this, it's how I
> discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
> however, I have not tested yet what happens if the same function is
> assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
> pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
> being routed to *both* pins or just one pin?). if the same function
> cannot be used by two pins simultaneously then we should probably use
> function "pwm_f" instead of "pwm_f_x" (just an example) so we can
> detect these "conflicts".
>
>
> Regards,
> Martin
>
>
> [0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
>
Let's resume :
In the mainline meson pinctrl driver, we have :
- pins that represent the PAD name (GPIOX_3 or GPIODV_17)
- groups that represent each physical "bit" we set in the control registers, each group can group 1 or multiple physical pins
For example, the "emmc_nand_d07" sets the mode of multiple pins.
- functions are a purely logical set of groups
The only C function handling the "function" is :
static int meson_pmx_set_mux(func_num, group_num)
{
...
struct meson_pmx_func *func = &pc->data->funcs[func_num];
...
/* Function 0 (GPIO) doesn't need any additional setting */
if (func_num)
ret = regmap_update_bits(...);
...
}
So we could have only two big functions :
is_gpio
is_not_gpio
But now we group every logically similar "groups" into the same function, like :
static const char * const uart_a_groups[] = {
"uart_tx_a", "uart_rx_a", "uart_cts_a", "uart_rts_a",
};
When I pushed the PWM pins, I did not knew how to handle the fact that the same signal
is on multiple physical pins, so I added multiple functions for each physical pin mode.
What jerome pointed is that instead of :
static const char * const pwm_a_x_groups[] = {
"pwm_a_x",
};
static const char * const pwm_a_y_groups[] = {
"pwm_a_y",
};
we could have single :
static const char * const pwm_a_groups[] = {
"pwm_a_x", "pwm_a_y",
};
But we should also change the DT accordingly.
So we can see that from different perspective :
- from the "pin" perspective : they are different functions, so we leave like it's already mainline
- from the "signal" perspective : they are the same signal, so we should group
- both, so we set a function per groupable groups.
for example, on the GXL, we can have UART_AO_B TX&RX on multiple pins, so we can have either :
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
"uart_tx_ao_b_0", "uart_rx_ao_b_1",
};
or
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
};
static const char * const uart_ao_b_0_1_groups[] = {
"uart_tx_ao_b_0", "uart_rx_ao_b_1",
};
or
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
};
static const char * const uart_ao_b_0_1_groups[] = {
"uart_tx_ao_b_0", "uart_rx_ao_b_1", "uart_cts_ao_b", "uart_rts_ao_b",
};
The three of these are correct.
The point Jerome asked was for solution 1, and it what I pushed
The solution 2 is how functions are today.
The third solution is less easy but still valid.
What I want is :
- since it's purely logical, it's not very important and each solution is still valid from
all perspectives, so leave the PWM functions like this and push new PWMs in the same way
- The next functions should behave like solution 1, or solution 3 if better
Neil
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-16 13:54 ` Neil Armstrong
0 siblings, 0 replies; 57+ messages in thread
From: Neil Armstrong @ 2017-03-16 13:54 UTC (permalink / raw)
To: linus-amlogic
On 03/15/2017 09:11 PM, Martin Blumenstingl wrote:
> On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>
>>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>>
>>>>>>> + FUNCTION(pwm_f_clk),
>>>>>>> + FUNCTION(pwm_f_x),
>>>>>>
>>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>>> could have this:
>>>>>>
>>>>>> +static const char * const pwm_f_groups[] = {
>>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>>> +};
>>>>>>
>>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>>> information anyway, except for prints.
>>>>>>
>>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>>> good, and follows what has been done in the past on meson. I know we
>>>>>> have been this a lot already, but I'm questioning whether we should
>>>>>> continue to do so ?
>>>>>>
>>>>>> I asking because I also have a lot case like this coming up on audio
>>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>>
>>>>> could you please look into Jerome's question?
>>>>> personally I'm fine with either way, and changing my patch would be
>>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>>> changing anything (and reverting that afterwards again).
>>>>
>>>> I don't understand the question really.
>>>>
>>>> I am not an expert on this system, if the people working with it
>>>> cannot tell a function from a group I don't know who can... certainly
>>>> not me.
>>>>
>>>> What I can say is that pincontrol combines functions and groups to
>>>> states using a mapping. The functions should be something you poke
>>>> into a register, the groups are looser defined but may also be a
>>>> character of the hardware, but more usual a character of the
>>>> intended electronic usecase. Groups contain 1..n pins and can
>>>> be combined with some applicable functions.
>>>>
>>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>>> unclear, I really put a lot of hours into getting that right. Especially
>>>> reexamine "Pinmux conventions".
>>>
>>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>>> other groups are all handled the same.
>>
>> handled the same... as what?
>>
>>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>>> a single "PWM F" group instead of multiple ones.
>>>
>>> My advice is to leave the PWM groups as is,
>>
>> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>>
>>> and push new pins/functions/groups
>>> grouped with the internal signal name if split on multiple pins.
>>
>> Can somone do a quick patch for PWM_F for example, also showing how this
>> will look in the DT if someone wants to switch between the PWM_F on GPIOX
>> or GPIOCLK?
> it would look like this (node name, label and group stay the same,
> function does not contain the _x/_clk suffix anymore):
> pwm_f_clk_pins: pwm_f_clk {
> mux {
> groups = "pwm_f_clk";
> function = "pwm_f";
> };
> };
>
> pwm_f_x_pins: pwm_f_x {
> mux {
> groups = "pwm_f_x";
> function = "pwm_f";
> };
> };
>
>> We shouldalso verify that the driver is detecting/removing conflicts
>> properly when something else is already using that pin (e.g. SDIO_IRQ
>> shares pin GPIOX_7 with PWM_F)
> if the same pin is assigned to two devices then the pinctrl subsystem
> will throw an error (we don't have to take care of this, it's how I
> discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
> however, I have not tested yet what happens if the same function is
> assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
> pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
> being routed to *both* pins or just one pin?). if the same function
> cannot be used by two pins simultaneously then we should probably use
> function "pwm_f" instead of "pwm_f_x" (just an example) so we can
> detect these "conflicts".
>
>
> Regards,
> Martin
>
>
> [0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
>
Let's resume :
In the mainline meson pinctrl driver, we have :
- pins that represent the PAD name (GPIOX_3 or GPIODV_17)
- groups that represent each physical "bit" we set in the control registers, each group can group 1 or multiple physical pins
For example, the "emmc_nand_d07" sets the mode of multiple pins.
- functions are a purely logical set of groups
The only C function handling the "function" is :
static int meson_pmx_set_mux(func_num, group_num)
{
...
struct meson_pmx_func *func = &pc->data->funcs[func_num];
...
/* Function 0 (GPIO) doesn't need any additional setting */
if (func_num)
ret = regmap_update_bits(...);
...
}
So we could have only two big functions :
is_gpio
is_not_gpio
But now we group every logically similar "groups" into the same function, like :
static const char * const uart_a_groups[] = {
"uart_tx_a", "uart_rx_a", "uart_cts_a", "uart_rts_a",
};
When I pushed the PWM pins, I did not knew how to handle the fact that the same signal
is on multiple physical pins, so I added multiple functions for each physical pin mode.
What jerome pointed is that instead of :
static const char * const pwm_a_x_groups[] = {
"pwm_a_x",
};
static const char * const pwm_a_y_groups[] = {
"pwm_a_y",
};
we could have single :
static const char * const pwm_a_groups[] = {
"pwm_a_x", "pwm_a_y",
};
But we should also change the DT accordingly.
So we can see that from different perspective :
- from the "pin" perspective : they are different functions, so we leave like it's already mainline
- from the "signal" perspective : they are the same signal, so we should group
- both, so we set a function per groupable groups.
for example, on the GXL, we can have UART_AO_B TX&RX on multiple pins, so we can have either :
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
"uart_tx_ao_b_0", "uart_rx_ao_b_1",
};
or
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
};
static const char * const uart_ao_b_0_1_groups[] = {
"uart_tx_ao_b_0", "uart_rx_ao_b_1",
};
or
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
};
static const char * const uart_ao_b_0_1_groups[] = {
"uart_tx_ao_b_0", "uart_rx_ao_b_1", "uart_cts_ao_b", "uart_rts_ao_b",
};
The three of these are correct.
The point Jerome asked was for solution 1, and it what I pushed
The solution 2 is how functions are today.
The third solution is less easy but still valid.
What I want is :
- since it's purely logical, it's not very important and each solution is still valid from
all perspectives, so leave the PWM functions like this and push new PWMs in the same way
- The next functions should behave like solution 1, or solution 3 if better
Neil
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-16 13:54 ` Neil Armstrong
0 siblings, 0 replies; 57+ messages in thread
From: Neil Armstrong @ 2017-03-16 13:54 UTC (permalink / raw)
To: linux-arm-kernel
On 03/15/2017 09:11 PM, Martin Blumenstingl wrote:
> On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>
>>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>>
>>>>>>> + FUNCTION(pwm_f_clk),
>>>>>>> + FUNCTION(pwm_f_x),
>>>>>>
>>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>>> could have this:
>>>>>>
>>>>>> +static const char * const pwm_f_groups[] = {
>>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>>> +};
>>>>>>
>>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>>> information anyway, except for prints.
>>>>>>
>>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>>> good, and follows what has been done in the past on meson. I know we
>>>>>> have been this a lot already, but I'm questioning whether we should
>>>>>> continue to do so ?
>>>>>>
>>>>>> I asking because I also have a lot case like this coming up on audio
>>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>>
>>>>> could you please look into Jerome's question?
>>>>> personally I'm fine with either way, and changing my patch would be
>>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>>> changing anything (and reverting that afterwards again).
>>>>
>>>> I don't understand the question really.
>>>>
>>>> I am not an expert on this system, if the people working with it
>>>> cannot tell a function from a group I don't know who can... certainly
>>>> not me.
>>>>
>>>> What I can say is that pincontrol combines functions and groups to
>>>> states using a mapping. The functions should be something you poke
>>>> into a register, the groups are looser defined but may also be a
>>>> character of the hardware, but more usual a character of the
>>>> intended electronic usecase. Groups contain 1..n pins and can
>>>> be combined with some applicable functions.
>>>>
>>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>>> unclear, I really put a lot of hours into getting that right. Especially
>>>> reexamine "Pinmux conventions".
>>>
>>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>>> other groups are all handled the same.
>>
>> handled the same... as what?
>>
>>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>>> a single "PWM F" group instead of multiple ones.
>>>
>>> My advice is to leave the PWM groups as is,
>>
>> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>>
>>> and push new pins/functions/groups
>>> grouped with the internal signal name if split on multiple pins.
>>
>> Can somone do a quick patch for PWM_F for example, also showing how this
>> will look in the DT if someone wants to switch between the PWM_F on GPIOX
>> or GPIOCLK?
> it would look like this (node name, label and group stay the same,
> function does not contain the _x/_clk suffix anymore):
> pwm_f_clk_pins: pwm_f_clk {
> mux {
> groups = "pwm_f_clk";
> function = "pwm_f";
> };
> };
>
> pwm_f_x_pins: pwm_f_x {
> mux {
> groups = "pwm_f_x";
> function = "pwm_f";
> };
> };
>
>> We shouldalso verify that the driver is detecting/removing conflicts
>> properly when something else is already using that pin (e.g. SDIO_IRQ
>> shares pin GPIOX_7 with PWM_F)
> if the same pin is assigned to two devices then the pinctrl subsystem
> will throw an error (we don't have to take care of this, it's how I
> discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
> however, I have not tested yet what happens if the same function is
> assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
> pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
> being routed to *both* pins or just one pin?). if the same function
> cannot be used by two pins simultaneously then we should probably use
> function "pwm_f" instead of "pwm_f_x" (just an example) so we can
> detect these "conflicts".
>
>
> Regards,
> Martin
>
>
> [0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
>
Let's resume :
In the mainline meson pinctrl driver, we have :
- pins that represent the PAD name (GPIOX_3 or GPIODV_17)
- groups that represent each physical "bit" we set in the control registers, each group can group 1 or multiple physical pins
For example, the "emmc_nand_d07" sets the mode of multiple pins.
- functions are a purely logical set of groups
The only C function handling the "function" is :
static int meson_pmx_set_mux(func_num, group_num)
{
...
struct meson_pmx_func *func = &pc->data->funcs[func_num];
...
/* Function 0 (GPIO) doesn't need any additional setting */
if (func_num)
ret = regmap_update_bits(...);
...
}
So we could have only two big functions :
is_gpio
is_not_gpio
But now we group every logically similar "groups" into the same function, like :
static const char * const uart_a_groups[] = {
"uart_tx_a", "uart_rx_a", "uart_cts_a", "uart_rts_a",
};
When I pushed the PWM pins, I did not knew how to handle the fact that the same signal
is on multiple physical pins, so I added multiple functions for each physical pin mode.
What jerome pointed is that instead of :
static const char * const pwm_a_x_groups[] = {
"pwm_a_x",
};
static const char * const pwm_a_y_groups[] = {
"pwm_a_y",
};
we could have single :
static const char * const pwm_a_groups[] = {
"pwm_a_x", "pwm_a_y",
};
But we should also change the DT accordingly.
So we can see that from different perspective :
- from the "pin" perspective : they are different functions, so we leave like it's already mainline
- from the "signal" perspective : they are the same signal, so we should group
- both, so we set a function per groupable groups.
for example, on the GXL, we can have UART_AO_B TX&RX on multiple pins, so we can have either :
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
"uart_tx_ao_b_0", "uart_rx_ao_b_1",
};
or
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
};
static const char * const uart_ao_b_0_1_groups[] = {
"uart_tx_ao_b_0", "uart_rx_ao_b_1",
};
or
static const char * const uart_ao_b_groups[] = {
"uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
};
static const char * const uart_ao_b_0_1_groups[] = {
"uart_tx_ao_b_0", "uart_rx_ao_b_1", "uart_cts_ao_b", "uart_rts_ao_b",
};
The three of these are correct.
The point Jerome asked was for solution 1, and it what I pushed
The solution 2 is how functions are today.
The third solution is less easy but still valid.
What I want is :
- since it's purely logical, it's not very important and each solution is still valid from
all perspectives, so leave the PWM functions like this and push new PWMs in the same way
- The next functions should behave like solution 1, or solution 3 if better
Neil
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
2017-03-16 13:54 ` Neil Armstrong
(?)
@ 2017-03-16 18:52 ` Kevin Hilman
-1 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-16 18:52 UTC (permalink / raw)
To: Neil Armstrong
Cc: Martin Blumenstingl, Linus Walleij, Mark Rutland, devicetree,
Will Deacon, linux-gpio, Rob Herring, Catalin Marinas,
Carlo Caione, open list:ARM/Amlogic Meson...,
linux-arm-kernel, Jerome Brunet
Neil Armstrong <narmstrong@baylibre.com> writes:
> On 03/15/2017 09:11 PM, Martin Blumenstingl wrote:
>> On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>>
>>>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>>>
>>>>>>>> + FUNCTION(pwm_f_clk),
>>>>>>>> + FUNCTION(pwm_f_x),
>>>>>>>
>>>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>>>> could have this:
>>>>>>>
>>>>>>> +static const char * const pwm_f_groups[] = {
>>>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>>>> +};
>>>>>>>
>>>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>>>> information anyway, except for prints.
>>>>>>>
>>>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>>>> good, and follows what has been done in the past on meson. I know we
>>>>>>> have been this a lot already, but I'm questioning whether we should
>>>>>>> continue to do so ?
>>>>>>>
>>>>>>> I asking because I also have a lot case like this coming up on audio
>>>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>>>
>>>>>> could you please look into Jerome's question?
>>>>>> personally I'm fine with either way, and changing my patch would be
>>>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>>>> changing anything (and reverting that afterwards again).
>>>>>
>>>>> I don't understand the question really.
>>>>>
>>>>> I am not an expert on this system, if the people working with it
>>>>> cannot tell a function from a group I don't know who can... certainly
>>>>> not me.
>>>>>
>>>>> What I can say is that pincontrol combines functions and groups to
>>>>> states using a mapping. The functions should be something you poke
>>>>> into a register, the groups are looser defined but may also be a
>>>>> character of the hardware, but more usual a character of the
>>>>> intended electronic usecase. Groups contain 1..n pins and can
>>>>> be combined with some applicable functions.
>>>>>
>>>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>>>> unclear, I really put a lot of hours into getting that right. Especially
>>>>> reexamine "Pinmux conventions".
>>>>
>>>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>>>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>>>> other groups are all handled the same.
>>>
>>> handled the same... as what?
>>>
>>>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>>>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>>>> a single "PWM F" group instead of multiple ones.
>>>>
>>>> My advice is to leave the PWM groups as is,
>>>
>>> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>>>
>>>> and push new pins/functions/groups
>>>> grouped with the internal signal name if split on multiple pins.
>>>
>>> Can somone do a quick patch for PWM_F for example, also showing how this
>>> will look in the DT if someone wants to switch between the PWM_F on GPIOX
>>> or GPIOCLK?
>> it would look like this (node name, label and group stay the same,
>> function does not contain the _x/_clk suffix anymore):
>> pwm_f_clk_pins: pwm_f_clk {
>> mux {
>> groups = "pwm_f_clk";
>> function = "pwm_f";
>> };
>> };
>>
>> pwm_f_x_pins: pwm_f_x {
>> mux {
>> groups = "pwm_f_x";
>> function = "pwm_f";
>> };
>> };
>>
>>> We shouldalso verify that the driver is detecting/removing conflicts
>>> properly when something else is already using that pin (e.g. SDIO_IRQ
>>> shares pin GPIOX_7 with PWM_F)
>> if the same pin is assigned to two devices then the pinctrl subsystem
>> will throw an error (we don't have to take care of this, it's how I
>> discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
>> however, I have not tested yet what happens if the same function is
>> assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
>> pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
>> being routed to *both* pins or just one pin?). if the same function
>> cannot be used by two pins simultaneously then we should probably use
>> function "pwm_f" instead of "pwm_f_x" (just an example) so we can
>> detect these "conflicts".
>>
>>
>> Regards,
>> Martin
>>
>>
>> [0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
>>
>
> Let's resume :
> In the mainline meson pinctrl driver, we have :
> - pins that represent the PAD name (GPIOX_3 or GPIODV_17)
> - groups that represent each physical "bit" we set in the control registers, each group can group 1 or multiple physical pins
> For example, the "emmc_nand_d07" sets the mode of multiple pins.
> - functions are a purely logical set of groups
>
> The only C function handling the "function" is :
> static int meson_pmx_set_mux(func_num, group_num)
> {
> ...
> struct meson_pmx_func *func = &pc->data->funcs[func_num];
> ...
> /* Function 0 (GPIO) doesn't need any additional setting */
> if (func_num)
> ret = regmap_update_bits(...);
> ...
> }
>
> So we could have only two big functions :
> is_gpio
> is_not_gpio
>
> But now we group every logically similar "groups" into the same function, like :
> static const char * const uart_a_groups[] = {
> "uart_tx_a", "uart_rx_a", "uart_cts_a", "uart_rts_a",
> };
>
> When I pushed the PWM pins, I did not knew how to handle the fact that the same signal
> is on multiple physical pins, so I added multiple functions for each physical pin mode.
>
> What jerome pointed is that instead of :
> static const char * const pwm_a_x_groups[] = {
> "pwm_a_x",
> };
>
> static const char * const pwm_a_y_groups[] = {
> "pwm_a_y",
> };
>
> we could have single :
> static const char * const pwm_a_groups[] = {
> "pwm_a_x", "pwm_a_y",
> };
>
> But we should also change the DT accordingly.
>
> So we can see that from different perspective :
> - from the "pin" perspective : they are different functions, so we leave like it's already mainline
> - from the "signal" perspective : they are the same signal, so we should group
> - both, so we set a function per groupable groups.
>
> for example, on the GXL, we can have UART_AO_B TX&RX on multiple pins, so we can have either :
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> "uart_tx_ao_b_0", "uart_rx_ao_b_1",
> };
>
> or
>
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> static const char * const uart_ao_b_0_1_groups[] = {
> "uart_tx_ao_b_0", "uart_rx_ao_b_1",
> };
>
> or
>
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> static const char * const uart_ao_b_0_1_groups[] = {
> "uart_tx_ao_b_0", "uart_rx_ao_b_1", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> The three of these are correct.
> The point Jerome asked was for solution 1, and it what I pushed
> The solution 2 is how functions are today.
>
> The third solution is less easy but still valid.
>
> What I want is :
> - since it's purely logical, it's not very important and each solution is still valid from
> all perspectives, so leave the PWM functions like this and push new PWMs in the same way
> - The next functions should behave like solution 1, or solution 3 if better
Thanks for the clarification and examples.
I think solution 1 is fine.
I just want to be sure that flexibilty remains in DT, so it's the DT
files that determine exactly which groups (or pins) will be used.
e.g. it has to stay the case that DT can define a UART without CTS/RTS
signals.
I'm pretty sure that remains the case, since the "groups" property in DT
is selecting from the *available* groups defined in the driver, IIUC.
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-16 18:52 ` Kevin Hilman
0 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-16 18:52 UTC (permalink / raw)
To: linus-amlogic
Neil Armstrong <narmstrong@baylibre.com> writes:
> On 03/15/2017 09:11 PM, Martin Blumenstingl wrote:
>> On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>>
>>>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>>>
>>>>>>>> + FUNCTION(pwm_f_clk),
>>>>>>>> + FUNCTION(pwm_f_x),
>>>>>>>
>>>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>>>> could have this:
>>>>>>>
>>>>>>> +static const char * const pwm_f_groups[] = {
>>>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>>>> +};
>>>>>>>
>>>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>>>> information anyway, except for prints.
>>>>>>>
>>>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>>>> good, and follows what has been done in the past on meson. I know we
>>>>>>> have been this a lot already, but I'm questioning whether we should
>>>>>>> continue to do so ?
>>>>>>>
>>>>>>> I asking because I also have a lot case like this coming up on audio
>>>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>>>
>>>>>> could you please look into Jerome's question?
>>>>>> personally I'm fine with either way, and changing my patch would be
>>>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>>>> changing anything (and reverting that afterwards again).
>>>>>
>>>>> I don't understand the question really.
>>>>>
>>>>> I am not an expert on this system, if the people working with it
>>>>> cannot tell a function from a group I don't know who can... certainly
>>>>> not me.
>>>>>
>>>>> What I can say is that pincontrol combines functions and groups to
>>>>> states using a mapping. The functions should be something you poke
>>>>> into a register, the groups are looser defined but may also be a
>>>>> character of the hardware, but more usual a character of the
>>>>> intended electronic usecase. Groups contain 1..n pins and can
>>>>> be combined with some applicable functions.
>>>>>
>>>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>>>> unclear, I really put a lot of hours into getting that right. Especially
>>>>> reexamine "Pinmux conventions".
>>>>
>>>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>>>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>>>> other groups are all handled the same.
>>>
>>> handled the same... as what?
>>>
>>>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>>>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>>>> a single "PWM F" group instead of multiple ones.
>>>>
>>>> My advice is to leave the PWM groups as is,
>>>
>>> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>>>
>>>> and push new pins/functions/groups
>>>> grouped with the internal signal name if split on multiple pins.
>>>
>>> Can somone do a quick patch for PWM_F for example, also showing how this
>>> will look in the DT if someone wants to switch between the PWM_F on GPIOX
>>> or GPIOCLK?
>> it would look like this (node name, label and group stay the same,
>> function does not contain the _x/_clk suffix anymore):
>> pwm_f_clk_pins: pwm_f_clk {
>> mux {
>> groups = "pwm_f_clk";
>> function = "pwm_f";
>> };
>> };
>>
>> pwm_f_x_pins: pwm_f_x {
>> mux {
>> groups = "pwm_f_x";
>> function = "pwm_f";
>> };
>> };
>>
>>> We shouldalso verify that the driver is detecting/removing conflicts
>>> properly when something else is already using that pin (e.g. SDIO_IRQ
>>> shares pin GPIOX_7 with PWM_F)
>> if the same pin is assigned to two devices then the pinctrl subsystem
>> will throw an error (we don't have to take care of this, it's how I
>> discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
>> however, I have not tested yet what happens if the same function is
>> assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
>> pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
>> being routed to *both* pins or just one pin?). if the same function
>> cannot be used by two pins simultaneously then we should probably use
>> function "pwm_f" instead of "pwm_f_x" (just an example) so we can
>> detect these "conflicts".
>>
>>
>> Regards,
>> Martin
>>
>>
>> [0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
>>
>
> Let's resume :
> In the mainline meson pinctrl driver, we have :
> - pins that represent the PAD name (GPIOX_3 or GPIODV_17)
> - groups that represent each physical "bit" we set in the control registers, each group can group 1 or multiple physical pins
> For example, the "emmc_nand_d07" sets the mode of multiple pins.
> - functions are a purely logical set of groups
>
> The only C function handling the "function" is :
> static int meson_pmx_set_mux(func_num, group_num)
> {
> ...
> struct meson_pmx_func *func = &pc->data->funcs[func_num];
> ...
> /* Function 0 (GPIO) doesn't need any additional setting */
> if (func_num)
> ret = regmap_update_bits(...);
> ...
> }
>
> So we could have only two big functions :
> is_gpio
> is_not_gpio
>
> But now we group every logically similar "groups" into the same function, like :
> static const char * const uart_a_groups[] = {
> "uart_tx_a", "uart_rx_a", "uart_cts_a", "uart_rts_a",
> };
>
> When I pushed the PWM pins, I did not knew how to handle the fact that the same signal
> is on multiple physical pins, so I added multiple functions for each physical pin mode.
>
> What jerome pointed is that instead of :
> static const char * const pwm_a_x_groups[] = {
> "pwm_a_x",
> };
>
> static const char * const pwm_a_y_groups[] = {
> "pwm_a_y",
> };
>
> we could have single :
> static const char * const pwm_a_groups[] = {
> "pwm_a_x", "pwm_a_y",
> };
>
> But we should also change the DT accordingly.
>
> So we can see that from different perspective :
> - from the "pin" perspective : they are different functions, so we leave like it's already mainline
> - from the "signal" perspective : they are the same signal, so we should group
> - both, so we set a function per groupable groups.
>
> for example, on the GXL, we can have UART_AO_B TX&RX on multiple pins, so we can have either :
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> "uart_tx_ao_b_0", "uart_rx_ao_b_1",
> };
>
> or
>
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> static const char * const uart_ao_b_0_1_groups[] = {
> "uart_tx_ao_b_0", "uart_rx_ao_b_1",
> };
>
> or
>
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> static const char * const uart_ao_b_0_1_groups[] = {
> "uart_tx_ao_b_0", "uart_rx_ao_b_1", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> The three of these are correct.
> The point Jerome asked was for solution 1, and it what I pushed
> The solution 2 is how functions are today.
>
> The third solution is less easy but still valid.
>
> What I want is :
> - since it's purely logical, it's not very important and each solution is still valid from
> all perspectives, so leave the PWM functions like this and push new PWMs in the same way
> - The next functions should behave like solution 1, or solution 3 if better
Thanks for the clarification and examples.
I think solution 1 is fine.
I just want to be sure that flexibilty remains in DT, so it's the DT
files that determine exactly which groups (or pins) will be used.
e.g. it has to stay the case that DT can define a UART without CTS/RTS
signals.
I'm pretty sure that remains the case, since the "groups" property in DT
is selecting from the *available* groups defined in the driver, IIUC.
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH 1/2] pinctrl: meson: gxl: add the missing PWM pin definitions
@ 2017-03-16 18:52 ` Kevin Hilman
0 siblings, 0 replies; 57+ messages in thread
From: Kevin Hilman @ 2017-03-16 18:52 UTC (permalink / raw)
To: linux-arm-kernel
Neil Armstrong <narmstrong@baylibre.com> writes:
> On 03/15/2017 09:11 PM, Martin Blumenstingl wrote:
>> On Wed, Mar 15, 2017 at 8:12 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>>
>>>> On 03/14/2017 04:42 PM, Linus Walleij wrote:
>>>>> On Thu, Mar 9, 2017 at 8:47 PM, Martin Blumenstingl
>>>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>>> On Mon, Mar 6, 2017 at 3:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>>>>>> On Sat, 2017-03-04 at 22:23 +0100, Martin Blumenstingl wrote:
>>>>>
>>>>>>>> + FUNCTION(pwm_f_clk),
>>>>>>>> + FUNCTION(pwm_f_x),
>>>>>>>
>>>>>>> I wonder if having function named "pwm_f_clk" really makes sense ?
>>>>>>> Shouldn't it be just "pwm_f" ? This is real function, isn't it ?
>>>>>>> The actual pin used will be provided in the dt. Here, I suppose we
>>>>>>> could have this:
>>>>>>>
>>>>>>> +static const char * const pwm_f_groups[] = {
>>>>>>> + "pwm_f_x", "pwm_f_clk",
>>>>>>> +};
>>>>>>>
>>>>>>> Has far as I can see, on meson arch, the function does not carry much
>>>>>>> information anyway, except for prints.
>>>>>>>
>>>>>>> To be clear, I'm not questioning this change in particular. It looks
>>>>>>> good, and follows what has been done in the past on meson. I know we
>>>>>>> have been this a lot already, but I'm questioning whether we should
>>>>>>> continue to do so ?
>>>>>>>
>>>>>>> I asking because I also have a lot case like this coming up on audio
>>>>>>> for gxl and gxbb, where the same function can use different pins.
>>>>>>
>>>>>> could you please look into Jerome's question?
>>>>>> personally I'm fine with either way, and changing my patch would be
>>>>>> quite trivial. but I'd like to know what's "the way to go" before
>>>>>> changing anything (and reverting that afterwards again).
>>>>>
>>>>> I don't understand the question really.
>>>>>
>>>>> I am not an expert on this system, if the people working with it
>>>>> cannot tell a function from a group I don't know who can... certainly
>>>>> not me.
>>>>>
>>>>> What I can say is that pincontrol combines functions and groups to
>>>>> states using a mapping. The functions should be something you poke
>>>>> into a register, the groups are looser defined but may also be a
>>>>> character of the hardware, but more usual a character of the
>>>>> intended electronic usecase. Groups contain 1..n pins and can
>>>>> be combined with some applicable functions.
>>>>>
>>>>> Please re-read Documentation/pinctrl.txt very closely if anything is
>>>>> unclear, I really put a lot of hours into getting that right. Especially
>>>>> reexamine "Pinmux conventions".
>>>>
>>>> The point pushed by Jerome was purely cosmetic since the groups in the meson
>>>> pinctrl driver are purely cosmetic, since only the GPIO group is handled,
>>>> other groups are all handled the same.
>>>
>>> handled the same... as what?
>>>
>>>> This is because I pushed all the PWM pins in a separate group, but functionnaly
>>>> the internal signal (i.e. PWM F) is the same for multiple pins and should be
>>>> a single "PWM F" group instead of multiple ones.
>>>>
>>>> My advice is to leave the PWM groups as is,
>>>
>>> Do you mean as we have in mainline today? or as is proposed in $SUBJECT patch?
>>>
>>>> and push new pins/functions/groups
>>>> grouped with the internal signal name if split on multiple pins.
>>>
>>> Can somone do a quick patch for PWM_F for example, also showing how this
>>> will look in the DT if someone wants to switch between the PWM_F on GPIOX
>>> or GPIOCLK?
>> it would look like this (node name, label and group stay the same,
>> function does not contain the _x/_clk suffix anymore):
>> pwm_f_clk_pins: pwm_f_clk {
>> mux {
>> groups = "pwm_f_clk";
>> function = "pwm_f";
>> };
>> };
>>
>> pwm_f_x_pins: pwm_f_x {
>> mux {
>> groups = "pwm_f_x";
>> function = "pwm_f";
>> };
>> };
>>
>>> We shouldalso verify that the driver is detecting/removing conflicts
>>> properly when something else is already using that pin (e.g. SDIO_IRQ
>>> shares pin GPIOX_7 with PWM_F)
>> if the same pin is assigned to two devices then the pinctrl subsystem
>> will throw an error (we don't have to take care of this, it's how I
>> discovered as GPIOAO_1 was used by uart_rx_ao_a and uart_rx_ao_b).
>> however, I have not tested yet what happens if the same function is
>> assigned to multiple pins (let's say you pass both, pwm_f_clk_pins and
>> pwm_f_x_pins to the pwm_ef node - will this result in the PWM output
>> being routed to *both* pins or just one pin?). if the same function
>> cannot be used by two pins simultaneously then we should probably use
>> function "pwm_f" instead of "pwm_f_x" (just an example) so we can
>> detect these "conflicts".
>>
>>
>> Regards,
>> Martin
>>
>>
>> [0] https://github.com/torvalds/linux/commit/b27e36482c02a94194fec71fb29696f4c8e9241c
>>
>
> Let's resume :
> In the mainline meson pinctrl driver, we have :
> - pins that represent the PAD name (GPIOX_3 or GPIODV_17)
> - groups that represent each physical "bit" we set in the control registers, each group can group 1 or multiple physical pins
> For example, the "emmc_nand_d07" sets the mode of multiple pins.
> - functions are a purely logical set of groups
>
> The only C function handling the "function" is :
> static int meson_pmx_set_mux(func_num, group_num)
> {
> ...
> struct meson_pmx_func *func = &pc->data->funcs[func_num];
> ...
> /* Function 0 (GPIO) doesn't need any additional setting */
> if (func_num)
> ret = regmap_update_bits(...);
> ...
> }
>
> So we could have only two big functions :
> is_gpio
> is_not_gpio
>
> But now we group every logically similar "groups" into the same function, like :
> static const char * const uart_a_groups[] = {
> "uart_tx_a", "uart_rx_a", "uart_cts_a", "uart_rts_a",
> };
>
> When I pushed the PWM pins, I did not knew how to handle the fact that the same signal
> is on multiple physical pins, so I added multiple functions for each physical pin mode.
>
> What jerome pointed is that instead of :
> static const char * const pwm_a_x_groups[] = {
> "pwm_a_x",
> };
>
> static const char * const pwm_a_y_groups[] = {
> "pwm_a_y",
> };
>
> we could have single :
> static const char * const pwm_a_groups[] = {
> "pwm_a_x", "pwm_a_y",
> };
>
> But we should also change the DT accordingly.
>
> So we can see that from different perspective :
> - from the "pin" perspective : they are different functions, so we leave like it's already mainline
> - from the "signal" perspective : they are the same signal, so we should group
> - both, so we set a function per groupable groups.
>
> for example, on the GXL, we can have UART_AO_B TX&RX on multiple pins, so we can have either :
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> "uart_tx_ao_b_0", "uart_rx_ao_b_1",
> };
>
> or
>
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> static const char * const uart_ao_b_0_1_groups[] = {
> "uart_tx_ao_b_0", "uart_rx_ao_b_1",
> };
>
> or
>
> static const char * const uart_ao_b_groups[] = {
> "uart_tx_ao_b", "uart_rx_ao_b", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> static const char * const uart_ao_b_0_1_groups[] = {
> "uart_tx_ao_b_0", "uart_rx_ao_b_1", "uart_cts_ao_b", "uart_rts_ao_b",
> };
>
> The three of these are correct.
> The point Jerome asked was for solution 1, and it what I pushed
> The solution 2 is how functions are today.
>
> The third solution is less easy but still valid.
>
> What I want is :
> - since it's purely logical, it's not very important and each solution is still valid from
> all perspectives, so leave the PWM functions like this and push new PWMs in the same way
> - The next functions should behave like solution 1, or solution 3 if better
Thanks for the clarification and examples.
I think solution 1 is fine.
I just want to be sure that flexibilty remains in DT, so it's the DT
files that determine exactly which groups (or pins) will be used.
e.g. it has to stay the case that DT can define a UART without CTS/RTS
signals.
I'm pretty sure that remains the case, since the "groups" property in DT
is selecting from the *available* groups defined in the driver, IIUC.
Kevin
^ permalink raw reply [flat|nested] 57+ messages in thread