* How to write "modern" pinctrl DT node @ 2019-06-11 16:12 Marc Gonzalez 2019-06-11 16:50 ` Jonathan Neuschäfer 2019-06-11 18:05 ` Bjorn Andersson 0 siblings, 2 replies; 9+ messages in thread From: Marc Gonzalez @ 2019-06-11 16:12 UTC (permalink / raw) To: Linus Walleij, Bjorn Andersson; +Cc: MSM, gpio, Jeffrey Hugo Hello, I'm working with a device (TSIF0) which apparently drives 4 pins: (Or maybe 5... it seems gpio40 might be associated with TSIF0 as well.) https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n2258 I'll copy the downstream DT nodes here for discussion: tsif0_signals_active: tsif0_signals_active { tsif1_clk { pins = "gpio89"; /* TSIF0 CLK */ function = "tsif1_clk"; }; tsif1_en { pins = "gpio90"; /* TSIF0 Enable */ function = "tsif1_en"; }; tsif1_data { pins = "gpio91"; /* TSIF0 DATA */ function = "tsif1_data"; }; signals_cfg { pins = "gpio89", "gpio90", "gpio91"; drive_strength = <2>; /* 2 mA */ bias-pull-down; /* pull down */ }; }; /* sync signal is only used if configured to mode-2 */ tsif0_sync_active: tsif0_sync_active { tsif1_sync { pins = "gpio9"; /* TSIF0 SYNC */ function = "tsif1_sync"; drive_strength = <2>; /* 2 mA */ bias-pull-down; /* pull down */ }; }; Can I rewrite the first node as: tsif0_default { pins = "gpio89", "gpio90", "gpio91"; /* clk, enable, data */ function = "is_this_just_a_label?"; /* Can I just leave it out? */ drive-strength = <2>; bias-pull-down; } Is this enough information to configure the 3 pins? Probably not... There must be some information hard-coded in drivers/pinctrl/qcom/pinctrl-msm8998.c Can I merge pin 9 in the above node, since it has the same "hardware properties" (drive_strength and bias_direction) ? Looking at relevant parts of drivers/pinctrl/qcom/pinctrl-msm8998.c PINCTRL_PIN(89, "GPIO_89"), PINCTRL_PIN(90, "GPIO_90"), PINCTRL_PIN(91, "GPIO_91"), DECLARE_MSM_GPIO_PINS(89); DECLARE_MSM_GPIO_PINS(90); DECLARE_MSM_GPIO_PINS(91); static const char * const tsif1_clk_groups[] = { "gpio89", }; static const char * const phase_flag10_groups[] = { "gpio89", }; static const char * const tsif1_en_groups[] = { "gpio90", }; static const char * const mdp_vsync0_groups[] = { "gpio90", }; static const char * const mdp_vsync1_groups[] = { "gpio90", }; static const char * const mdp_vsync2_groups[] = { "gpio90", }; static const char * const mdp_vsync3_groups[] = { "gpio90", }; static const char * const blsp1_spi_groups[] = { "gpio90", }; static const char * const tgu_ch0_groups[] = { "gpio90", }; static const char * const qdss_cti1_b_groups[] = { "gpio90", "gpio91", }; static const char * const tsif1_data_groups[] = { "gpio91", }; static const char * const sdc4_cmd_groups[] = { "gpio91", }; static const char * const tgu_ch1_groups[] = { "gpio91", }; static const char * const phase_flag1_groups[] = { "gpio91", }; PINGROUP(89, EAST, tsif1_clk, phase_flag10, NA, NA, NA, NA, NA, NA, NA), PINGROUP(90, EAST, tsif1_en, mdp_vsync0, mdp_vsync1, mdp_vsync2, mdp_vsync3, blsp1_spi, tgu_ch0, qdss_cti1_b, NA), PINGROUP(91, EAST, tsif1_data, sdc4_cmd, tgu_ch1, phase_flag1, qdss_cti1_b, NA, NA, NA, NA), (It seems to me there is some redundancy in this driver?) These last 3 lines seem to summarize how each pin is muxed? I.e. it's used as one function, exclusively? So a proper driver should be unloadable, to let other drivers claim the shared pins? Regards. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: How to write "modern" pinctrl DT node 2019-06-11 16:12 How to write "modern" pinctrl DT node Marc Gonzalez @ 2019-06-11 16:50 ` Jonathan Neuschäfer 2019-06-11 18:05 ` Bjorn Andersson 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Neuschäfer @ 2019-06-11 16:50 UTC (permalink / raw) To: Marc Gonzalez; +Cc: Linus Walleij, Bjorn Andersson, MSM, gpio, Jeffrey Hugo [-- Attachment #1: Type: text/plain, Size: 2688 bytes --] Hi, On Tue, Jun 11, 2019 at 06:12:30PM +0200, Marc Gonzalez wrote: > Hello, > > I'm working with a device (TSIF0) which apparently drives 4 pins: > (Or maybe 5... it seems gpio40 might be associated with TSIF0 as well.) > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n2258 > > I'll copy the downstream DT nodes here for discussion: > > tsif0_signals_active: tsif0_signals_active { > tsif1_clk { > pins = "gpio89"; /* TSIF0 CLK */ > function = "tsif1_clk"; > }; > tsif1_en { > pins = "gpio90"; /* TSIF0 Enable */ > function = "tsif1_en"; > }; > tsif1_data { > pins = "gpio91"; /* TSIF0 DATA */ > function = "tsif1_data"; > }; > signals_cfg { > pins = "gpio89", "gpio90", "gpio91"; > drive_strength = <2>; /* 2 mA */ > bias-pull-down; /* pull down */ > }; > }; > > /* sync signal is only used if configured to mode-2 */ > tsif0_sync_active: tsif0_sync_active { > tsif1_sync { > pins = "gpio9"; /* TSIF0 SYNC */ > function = "tsif1_sync"; > drive_strength = <2>; /* 2 mA */ > bias-pull-down; /* pull down */ > }; > }; Looking at the upstream binding[1], I think you can almost use the above snippet as-is, except for the drive_strength property which is spelled drive-strength. > Can I rewrite the first node as: > > tsif0_default { > pins = "gpio89", "gpio90", "gpio91"; /* clk, enable, data */ > function = "is_this_just_a_label?"; /* Can I just leave it out? */ > drive-strength = <2>; > bias-pull-down; > } The function property is not just an arbitrary label, it needs to be one of the values defined in the binding. (... which makes because the driver needs to know *which* hardware function to mux to a given pin.) > Is this enough information to configure the 3 pins? Probably not... No, I think you need the separate nodes in order to define the separate functions of the different pins. There isn't just one function called "tsif1" that you could apply to all pins. > Can I merge pin 9 in the above node, since it has the same > "hardware properties" (drive_strength and bias_direction) ? Merge what? &tsif0_signals_active/signals_cfg with &tsif0_sync_active/tsif1_sync? That depends on how &tsif0_signals_active and &tsif0_sync_active are used. Are they always used at the same time? Then I think so. Or are they different states of the TSIF interface that are configured at different times? Greetings, Jonathan Neuschäfer [1]: Documentation/devicetree/bindings/pinctrl/qcom%2Cmsm8998-pinctrl.txt [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: How to write "modern" pinctrl DT node 2019-06-11 16:12 How to write "modern" pinctrl DT node Marc Gonzalez 2019-06-11 16:50 ` Jonathan Neuschäfer @ 2019-06-11 18:05 ` Bjorn Andersson 2019-06-20 11:02 ` Marc Gonzalez 1 sibling, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2019-06-11 18:05 UTC (permalink / raw) To: Marc Gonzalez; +Cc: Linus Walleij, MSM, gpio, Jeffrey Hugo On Tue 11 Jun 09:12 PDT 2019, Marc Gonzalez wrote: > Hello, > > I'm working with a device (TSIF0) which apparently drives 4 pins: > (Or maybe 5... it seems gpio40 might be associated with TSIF0 as well.) > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n2258 > > I'll copy the downstream DT nodes here for discussion: > > tsif0_signals_active: tsif0_signals_active { This seems like the "default", "non-sleep" state. So I think a better name would be to make this: tsif0_default: tsif0-default (Note again, _ in label names, - in node names) > tsif1_clk { The namespace here is local to the tsif0-default state node, so there's no need to repeat the "tsif" prefix here. Just name it "clk". Also, what's up with tsif0 vs tsif1? > pins = "gpio89"; /* TSIF0 CLK */ This comment doesn't add any value, if you give the label a good name (i.e. name it clk and you know that this represents the clock pin) > function = "tsif1_clk"; Rather than having clk, en, data and error as separate functions we should have made them just "tsif1", as there's no overlap. But this is correct - see msm8998_functions[] (or the DT binding) for available functions. > }; > tsif1_en { > pins = "gpio90"; /* TSIF0 Enable */ > function = "tsif1_en"; > }; > tsif1_data { > pins = "gpio91"; /* TSIF0 DATA */ > function = "tsif1_data"; > }; > signals_cfg { This is written with the mindset that pinmux and pinconf properties should be kept separate, so here they specify the drive-strength and bias for the above 3 pins. I've come to prefer to have these properties added to the above nodes, rather than having this split. > pins = "gpio89", "gpio90", "gpio91"; > drive_strength = <2>; /* 2 mA */ As Jonathan pointed out, - not _. And in contrast to the previous downstream solution each property is of standard units and self explaining, so no need for comments. > bias-pull-down; /* pull down */ > }; > }; > > /* sync signal is only used if configured to mode-2 */ > tsif0_sync_active: tsif0_sync_active { This looks reasonable if this is a dynamic thing (if it's static it should be one node), and you can in your tsif0 node do: pinctrl-names = "default", "mode-2"; pinctrl-0 = <&tsif0_default>; pinctrl-1 = <&tsif0_default>, <&tsif0_sync_active>; The driver core will select "default" and your driver will have to use the pinctrl api to switch to the state "mode-2". > tsif1_sync { > pins = "gpio9"; /* TSIF0 SYNC */ > function = "tsif1_sync"; > drive_strength = <2>; /* 2 mA */ > bias-pull-down; /* pull down */ > }; > }; > > > Can I rewrite the first node as: > > tsif0_default { > pins = "gpio89", "gpio90", "gpio91"; /* clk, enable, data */ If you feel the need to add a comment then use subnodes to denote which is which instead. > function = "is_this_just_a_label?"; /* Can I just leave it out? */ For each pins you need to select a matching function name, per the mapping in msm8998_groups in the driver (or datasheet). As there are no collisions between the various tsif functions we should have just made them all "tsif1", but now you need to describe each pin and its associated function individually - as the example above shows.. > drive-strength = <2>; > bias-pull-down; > } Had we made the function name "tsif1" for all of them then you could definitely do this. > > Is this enough information to configure the 3 pins? Probably not... > There must be some information hard-coded in drivers/pinctrl/qcom/pinctrl-msm8998.c > > Can I merge pin 9 in the above node, since it has the same > "hardware properties" (drive_strength and bias_direction) ? > Had we made the tsif functions just "tsif1" then you could have done this. But based on the comment about mode-2 it seems like you still would like to keep this separate. > [..] > > PINGROUP(89, EAST, tsif1_clk, phase_flag10, NA, NA, NA, NA, NA, NA, NA), > PINGROUP(90, EAST, tsif1_en, mdp_vsync0, mdp_vsync1, mdp_vsync2, mdp_vsync3, blsp1_spi, tgu_ch0, qdss_cti1_b, NA), > PINGROUP(91, EAST, tsif1_data, sdc4_cmd, tgu_ch1, phase_flag1, qdss_cti1_b, NA, NA, NA, NA), > > (It seems to me there is some redundancy in this driver?) > > These last 3 lines seem to summarize how each pin is muxed? > I.e. it's used as one function, exclusively? > So a proper driver should be unloadable, to let other drivers > claim the shared pins? > The common example of this is for drivers to jump between a function and the implicit "gpio" function when going in and our of sleep. A state is described for the "default" (active) state with appropriate pinmux and pinconf and a separate ("sleep") is used to e.g. pull the pins low while in suspend. I can't think of an example where you would dynamically switch between the other functions; because note that for a given hardware design you have e.g. your tsif block soldered onto those pins. PS. I would suggest that you send a patch to the MSM8998 pinctrl driver (and binding) where you squash tsifN_* to tsifN. It would break backwards compatibility, but I think we can take that risk now before someone starts to use it... And after that you can go with your proposed squashed node. Regards, Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: How to write "modern" pinctrl DT node 2019-06-11 18:05 ` Bjorn Andersson @ 2019-06-20 11:02 ` Marc Gonzalez 2019-06-20 18:41 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Marc Gonzalez @ 2019-06-20 11:02 UTC (permalink / raw) To: Bjorn Andersson; +Cc: Linus Walleij, MSM, gpio, Jeffrey Hugo On 11/06/2019 20:05, Bjorn Andersson wrote: > Also, what's up with tsif0 vs tsif1? Some people count from 0; other people count from 1. The HW programming guide mentions TSIF_0 and TSIF_1. The pinctrl driver defines tsif1 and tsif2. I propose we use 0 and 1 consistently everywhere. > PS. I would suggest that you send a patch to the MSM8998 pinctrl driver > (and binding) where you squash tsifN_* to tsifN. It would break > backwards compatibility, but I think we can take that risk now before > someone starts to use it... And after that you can go with your proposed > squashed node. Here's what I have right now: $ git ls --stat 85c02fb4dfd1..HEAD 5ed38c44a92a (HEAD -> tsif-fixup) Fixup qcom,msm8998-pinctrl.txt example Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) 823402af81a6 Fixup qcom,msm8998-pinctrl.txt binding Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) afb686b8b3e7 Squash all 5 tsif1 pins together drivers/pinctrl/qcom/pinctrl-msm8998.c | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) 8e4d31c8d455 Squash all 5 tsif0 pins together drivers/pinctrl/qcom/pinctrl-msm8998.c | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) ee850fa510a6 sed -i 's/tsif2/tsif1/g' drivers/pinctrl/qcom/pinctrl-msm8998.c drivers/pinctrl/qcom/pinctrl-msm8998.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) c7ffe4075623 sed -i 's/tsif1/tsif0/g' drivers/pinctrl/qcom/pinctrl-msm8998.c drivers/pinctrl/qcom/pinctrl-msm8998.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) I'm wondering if the series needs to be split up (as it is) or squashed into a single patch (might be harder to review for mistakes). What do you think? Regards. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: How to write "modern" pinctrl DT node 2019-06-20 11:02 ` Marc Gonzalez @ 2019-06-20 18:41 ` Bjorn Andersson 2019-06-26 14:38 ` [PATCH v1] pinctrl: msm8998: Squash TSIF pins together Marc Gonzalez 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2019-06-20 18:41 UTC (permalink / raw) To: Marc Gonzalez; +Cc: Linus Walleij, MSM, gpio, Jeffrey Hugo On Thu 20 Jun 04:02 PDT 2019, Marc Gonzalez wrote: > On 11/06/2019 20:05, Bjorn Andersson wrote: > > > Also, what's up with tsif0 vs tsif1? > > Some people count from 0; other people count from 1. > > The HW programming guide mentions TSIF_0 and TSIF_1. > The pinctrl driver defines tsif1 and tsif2. > > I propose we use 0 and 1 consistently everywhere. > I do like when the tlmm functions matches the hardware block naming (which for some reason is not always the case). So I'm good with your suggestion. > > PS. I would suggest that you send a patch to the MSM8998 pinctrl driver > > (and binding) where you squash tsifN_* to tsifN. It would break > > backwards compatibility, but I think we can take that risk now before > > someone starts to use it... And after that you can go with your proposed > > squashed node. > > Here's what I have right now: > > $ git ls --stat 85c02fb4dfd1..HEAD > 5ed38c44a92a (HEAD -> tsif-fixup) Fixup qcom,msm8998-pinctrl.txt example > Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > 823402af81a6 Fixup qcom,msm8998-pinctrl.txt binding > Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > afb686b8b3e7 Squash all 5 tsif1 pins together > drivers/pinctrl/qcom/pinctrl-msm8998.c | 38 +++++++++----------------------------- > 1 file changed, 9 insertions(+), 29 deletions(-) > 8e4d31c8d455 Squash all 5 tsif0 pins together > drivers/pinctrl/qcom/pinctrl-msm8998.c | 38 +++++++++----------------------------- > 1 file changed, 9 insertions(+), 29 deletions(-) > ee850fa510a6 sed -i 's/tsif2/tsif1/g' drivers/pinctrl/qcom/pinctrl-msm8998.c > drivers/pinctrl/qcom/pinctrl-msm8998.c | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > c7ffe4075623 sed -i 's/tsif1/tsif0/g' drivers/pinctrl/qcom/pinctrl-msm8998.c > drivers/pinctrl/qcom/pinctrl-msm8998.c | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > I'm wondering if the series needs to be split up (as it is) or squashed > into a single patch (might be harder to review for mistakes). > > What do you think? > Go for one patch. Regards, Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1] pinctrl: msm8998: Squash TSIF pins together 2019-06-20 18:41 ` Bjorn Andersson @ 2019-06-26 14:38 ` Marc Gonzalez 2019-06-26 14:42 ` Jeffrey Hugo 2019-06-26 15:30 ` Jonathan Neuschäfer 0 siblings, 2 replies; 9+ messages in thread From: Marc Gonzalez @ 2019-06-26 14:38 UTC (permalink / raw) To: Bjorn Andersson, Linus Walleij; +Cc: MSM, gpio, Jeffrey Hugo Preamble: Rename tsif1 to tsif0, tsif2 to tsif1. Squash tsif0 pins into a single function. Same for tsif1. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- .../bindings/pinctrl/qcom,msm8998-pinctrl.txt | 19 ++--- drivers/pinctrl/qcom/pinctrl-msm8998.c | 76 +++++-------------- 2 files changed, 24 insertions(+), 71 deletions(-) diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt index 00174f08ba1d..47b0f30a39e9 100644 --- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt @@ -124,9 +124,8 @@ to specify in a pin configuration subnode: qlink_request, qua_mi2s, sd_card, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk, sdc4_cmd, sec_mi2s, sp_cmu, spkr_i2s, ssbi1, ssc_irq, ter_mi2s, tgu_ch0, tgu_ch1, - tsense_pwm1, tsense_pwm2, tsif1_clk, tsif1_data, tsif1_en, - tsif1_error, tsif1_sync, tsif2_clk, tsif2_data, tsif2_en, - tsif2_error, tsif2_sync, uim1_clk, uim1_data, uim1_present, + tsense_pwm1, tsense_pwm2, tsif0, tsif1, + uim1_clk, uim1_data, uim1_present, uim1_reset, uim2_clk, uim2_data, uim2_present, uim2_reset, uim_batt, usb_phy, vfr_1, vsense_clkout, vsense_data0, vsense_data1, vsense_mode, wlan1_adc0, wlan1_adc1, @@ -179,15 +178,9 @@ Example: #interrupt-cells = <2>; uart_console_active: uart_console_active { - mux { - pins = "gpio4", "gpio5"; - function = "blsp_uart8_a"; - }; - - config { - pins = "gpio4", "gpio5"; - drive-strength = <2>; - bias-disable; - }; + pins = "gpio4", "gpio5"; + function = "blsp_uart8_a"; + drive-strength = <2>; + bias-disable; }; }; diff --git a/drivers/pinctrl/qcom/pinctrl-msm8998.c b/drivers/pinctrl/qcom/pinctrl-msm8998.c index 00d7b94bc3f1..a05f41fe2706 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm8998.c +++ b/drivers/pinctrl/qcom/pinctrl-msm8998.c @@ -581,16 +581,8 @@ enum msm8998_functions { msm_mux_tgu_ch1, msm_mux_tsense_pwm1, msm_mux_tsense_pwm2, - msm_mux_tsif1_clk, - msm_mux_tsif1_data, - msm_mux_tsif1_en, - msm_mux_tsif1_error, - msm_mux_tsif1_sync, - msm_mux_tsif2_clk, - msm_mux_tsif2_data, - msm_mux_tsif2_en, - msm_mux_tsif2_error, - msm_mux_tsif2_sync, + msm_mux_tsif0, + msm_mux_tsif1, msm_mux_uim1_clk, msm_mux_uim1_data, msm_mux_uim1_present, @@ -692,9 +684,6 @@ static const char * const atest_usb13_groups[] = { static const char * const bimc_dte1_groups[] = { "gpio8", "gpio10", }; -static const char * const tsif1_sync_groups[] = { - "gpio9", -}; static const char * const wlan1_adc0_groups[] = { "gpio9", }; @@ -863,9 +852,6 @@ static const char * const lpass_slimbus_groups[] = { static const char * const sd_write_groups[] = { "gpio40", }; -static const char * const tsif1_error_groups[] = { - "gpio40", -}; static const char * const blsp_spi6_groups[] = { "gpio41", "gpio42", "gpio43", "gpio44", }; @@ -1048,11 +1034,8 @@ static const char * const blsp_uim2_b_groups[] = { static const char * const blsp_i2c5_groups[] = { "gpio87", "gpio88", }; -static const char * const tsif1_clk_groups[] = { - "gpio89", -}; -static const char * const tsif1_en_groups[] = { - "gpio90", +static const char * const tsif0_groups[] = { + "gpio9", "gpio40", "gpio89", "gpio90", "gpio91", }; static const char * const mdp_vsync0_groups[] = { "gpio90", @@ -1075,17 +1058,14 @@ static const char * const tgu_ch0_groups[] = { static const char * const qdss_cti1_b_groups[] = { "gpio90", "gpio91", }; -static const char * const tsif1_data_groups[] = { - "gpio91", -}; static const char * const sdc4_cmd_groups[] = { "gpio91", }; static const char * const tgu_ch1_groups[] = { "gpio91", }; -static const char * const tsif2_error_groups[] = { - "gpio92", +static const char * const tsif1_groups[] = { + "gpio92", "gpio93", "gpio94", "gpio95", "gpio96", }; static const char * const sdc43_groups[] = { "gpio92", @@ -1093,30 +1073,18 @@ static const char * const sdc43_groups[] = { static const char * const vfr_1_groups[] = { "gpio92", }; -static const char * const tsif2_clk_groups[] = { - "gpio93", -}; static const char * const sdc4_clk_groups[] = { "gpio93", }; -static const char * const tsif2_en_groups[] = { - "gpio94", -}; static const char * const sdc42_groups[] = { "gpio94", }; static const char * const sd_card_groups[] = { "gpio95", }; -static const char * const tsif2_data_groups[] = { - "gpio95", -}; static const char * const sdc41_groups[] = { "gpio95", }; -static const char * const tsif2_sync_groups[] = { - "gpio96", -}; static const char * const sdc40_groups[] = { "gpio96", }; @@ -1355,16 +1323,8 @@ static const struct msm_function msm8998_functions[] = { FUNCTION(tgu_ch1), FUNCTION(tsense_pwm1), FUNCTION(tsense_pwm2), - FUNCTION(tsif1_clk), - FUNCTION(tsif1_data), - FUNCTION(tsif1_en), - FUNCTION(tsif1_error), - FUNCTION(tsif1_sync), - FUNCTION(tsif2_clk), - FUNCTION(tsif2_data), - FUNCTION(tsif2_en), - FUNCTION(tsif2_error), - FUNCTION(tsif2_sync), + FUNCTION(tsif0), + FUNCTION(tsif1), FUNCTION(uim1_clk), FUNCTION(uim1_data), FUNCTION(uim1_present), @@ -1396,7 +1356,7 @@ static const struct msm_pingroup msm8998_groups[] = { PINGROUP(6, WEST, blsp_spi8, blsp_uart8_a, blsp_i2c8, _, _, _, _, _, _), PINGROUP(7, WEST, blsp_spi8, blsp_uart8_a, blsp_i2c8, ddr_bist, _, atest_tsens2, atest_usb1, _, _), PINGROUP(8, EAST, blsp_spi4, blsp_uart1_b, blsp_uim1_b, _, ddr_bist, _, wlan1_adc1, atest_usb13, bimc_dte1), - PINGROUP(9, EAST, blsp_spi4, blsp_uart1_b, blsp_uim1_b, tsif1_sync, ddr_bist, _, wlan1_adc0, atest_usb12, bimc_dte0), + PINGROUP(9, EAST, blsp_spi4, blsp_uart1_b, blsp_uim1_b, tsif0, ddr_bist, _, wlan1_adc0, atest_usb12, bimc_dte0), PINGROUP(10, EAST, mdp_vsync_a, blsp_spi4, blsp_uart1_b, blsp_i2c4, ddr_bist, atest_gpsadc1, wlan2_adc1, atest_usb11, bimc_dte1), PINGROUP(11, EAST, mdp_vsync_a, edp_lcd, blsp_spi4, blsp_uart1_b, blsp_i2c4, dbg_out, atest_gpsadc0, wlan2_adc0, atest_usb10), PINGROUP(12, EAST, mdp_vsync, m_voc, _, _, _, _, _, _, _), @@ -1427,7 +1387,7 @@ static const struct msm_pingroup msm8998_groups[] = { PINGROUP(37, NORTH, agera_pll, _, _, _, _, _, _, _, _), PINGROUP(38, WEST, usb_phy, _, _, _, _, _, _, _, _), PINGROUP(39, WEST, lpass_slimbus, _, _, _, _, _, _, _, _), - PINGROUP(40, EAST, sd_write, tsif1_error, _, _, _, _, _, _, _), + PINGROUP(40, EAST, sd_write, tsif0, _, _, _, _, _, _, _), PINGROUP(41, EAST, blsp_spi6, blsp_uart3_b, blsp_uim3_b, _, qdss, _, _, _, _), PINGROUP(42, EAST, blsp_spi6, blsp_uart3_b, blsp_uim3_b, _, qdss, _, _, _, _), PINGROUP(43, EAST, blsp_spi6, blsp_uart3_b, blsp_i2c6, _, qdss, _, _, _, _), @@ -1476,14 +1436,14 @@ static const struct msm_pingroup msm8998_groups[] = { PINGROUP(86, EAST, blsp_spi5, blsp_uart2_b, blsp_uim2_b, _, _, _, _, _, _), PINGROUP(87, EAST, blsp_spi5, blsp_uart2_b, blsp_i2c5, _, _, _, _, _, _), PINGROUP(88, EAST, blsp_spi5, blsp_uart2_b, blsp_i2c5, _, _, _, _, _, _), - PINGROUP(89, EAST, tsif1_clk, phase_flag, _, _, _, _, _, _, _), - PINGROUP(90, EAST, tsif1_en, mdp_vsync0, mdp_vsync1, mdp_vsync2, mdp_vsync3, blsp1_spi, tgu_ch0, qdss_cti1_b, _), - PINGROUP(91, EAST, tsif1_data, sdc4_cmd, tgu_ch1, phase_flag, qdss_cti1_b, _, _, _, _), - PINGROUP(92, EAST, tsif2_error, sdc43, vfr_1, phase_flag, _, _, _, _, _), - PINGROUP(93, EAST, tsif2_clk, sdc4_clk, _, qdss, _, _, _, _, _), - PINGROUP(94, EAST, tsif2_en, sdc42, _, _, _, _, _, _, _), - PINGROUP(95, EAST, tsif2_data, sdc41, _, _, _, _, _, _, _), - PINGROUP(96, EAST, tsif2_sync, sdc40, phase_flag, _, _, _, _, _, _), + PINGROUP(89, EAST, tsif0, phase_flag, _, _, _, _, _, _, _), + PINGROUP(90, EAST, tsif0, mdp_vsync0, mdp_vsync1, mdp_vsync2, mdp_vsync3, blsp1_spi, tgu_ch0, qdss_cti1_b, _), + PINGROUP(91, EAST, tsif0, sdc4_cmd, tgu_ch1, phase_flag, qdss_cti1_b, _, _, _, _), + PINGROUP(92, EAST, tsif1, sdc43, vfr_1, phase_flag, _, _, _, _, _), + PINGROUP(93, EAST, tsif1, sdc4_clk, _, qdss, _, _, _, _, _), + PINGROUP(94, EAST, tsif1, sdc42, _, _, _, _, _, _, _), + PINGROUP(95, EAST, tsif1, sdc41, _, _, _, _, _, _, _), + PINGROUP(96, EAST, tsif1, sdc40, phase_flag, _, _, _, _, _, _), PINGROUP(97, WEST, _, mdp_vsync_b, ldo_en, _, _, _, _, _, _), PINGROUP(98, WEST, _, mdp_vsync_b, ldo_update, _, _, _, _, _, _), PINGROUP(99, WEST, _, _, _, _, _, _, _, _, _), -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together 2019-06-26 14:38 ` [PATCH v1] pinctrl: msm8998: Squash TSIF pins together Marc Gonzalez @ 2019-06-26 14:42 ` Jeffrey Hugo 2019-06-26 14:46 ` Marc Gonzalez 2019-06-26 15:30 ` Jonathan Neuschäfer 1 sibling, 1 reply; 9+ messages in thread From: Jeffrey Hugo @ 2019-06-26 14:42 UTC (permalink / raw) To: Marc Gonzalez; +Cc: Bjorn Andersson, Linus Walleij, MSM, gpio On Wed, Jun 26, 2019 at 8:40 AM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > > Preamble: Rename tsif1 to tsif0, tsif2 to tsif1. > Squash tsif0 pins into a single function. Same for tsif1. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > .../bindings/pinctrl/qcom,msm8998-pinctrl.txt | 19 ++--- > drivers/pinctrl/qcom/pinctrl-msm8998.c | 76 +++++-------------- > 2 files changed, 24 insertions(+), 71 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt > index 00174f08ba1d..47b0f30a39e9 100644 > --- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt > @@ -124,9 +124,8 @@ to specify in a pin configuration subnode: > qlink_request, qua_mi2s, sd_card, sd_write, sdc40, sdc41, > sdc42, sdc43, sdc4_clk, sdc4_cmd, sec_mi2s, sp_cmu, > spkr_i2s, ssbi1, ssc_irq, ter_mi2s, tgu_ch0, tgu_ch1, > - tsense_pwm1, tsense_pwm2, tsif1_clk, tsif1_data, tsif1_en, > - tsif1_error, tsif1_sync, tsif2_clk, tsif2_data, tsif2_en, > - tsif2_error, tsif2_sync, uim1_clk, uim1_data, uim1_present, > + tsense_pwm1, tsense_pwm2, tsif0, tsif1, > + uim1_clk, uim1_data, uim1_present, > uim1_reset, uim2_clk, uim2_data, uim2_present, uim2_reset, > uim_batt, usb_phy, vfr_1, vsense_clkout, vsense_data0, > vsense_data1, vsense_mode, wlan1_adc0, wlan1_adc1, > @@ -179,15 +178,9 @@ Example: > #interrupt-cells = <2>; > > uart_console_active: uart_console_active { > - mux { > - pins = "gpio4", "gpio5"; > - function = "blsp_uart8_a"; > - }; > - > - config { > - pins = "gpio4", "gpio5"; > - drive-strength = <2>; > - bias-disable; > - }; > + pins = "gpio4", "gpio5"; > + function = "blsp_uart8_a"; > + drive-strength = <2>; > + bias-disable; > }; Whats with these "uart" changes? Modernizing the example? Doesn't seem related to the commit text... ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together 2019-06-26 14:42 ` Jeffrey Hugo @ 2019-06-26 14:46 ` Marc Gonzalez 0 siblings, 0 replies; 9+ messages in thread From: Marc Gonzalez @ 2019-06-26 14:46 UTC (permalink / raw) To: Jeffrey Hugo; +Cc: Bjorn Andersson, Linus Walleij, MSM, gpio On 26/06/2019 16:42, Jeffrey Hugo wrote: > On Wed, Jun 26, 2019 at 8:40 AM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > >> Preamble: Rename tsif1 to tsif0, tsif2 to tsif1. >> Squash tsif0 pins into a single function. Same for tsif1. >> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> >> --- >> .../bindings/pinctrl/qcom,msm8998-pinctrl.txt | 19 ++--- >> drivers/pinctrl/qcom/pinctrl-msm8998.c | 76 +++++-------------- >> 2 files changed, 24 insertions(+), 71 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt >> index 00174f08ba1d..47b0f30a39e9 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt >> @@ -124,9 +124,8 @@ to specify in a pin configuration subnode: >> qlink_request, qua_mi2s, sd_card, sd_write, sdc40, sdc41, >> sdc42, sdc43, sdc4_clk, sdc4_cmd, sec_mi2s, sp_cmu, >> spkr_i2s, ssbi1, ssc_irq, ter_mi2s, tgu_ch0, tgu_ch1, >> - tsense_pwm1, tsense_pwm2, tsif1_clk, tsif1_data, tsif1_en, >> - tsif1_error, tsif1_sync, tsif2_clk, tsif2_data, tsif2_en, >> - tsif2_error, tsif2_sync, uim1_clk, uim1_data, uim1_present, >> + tsense_pwm1, tsense_pwm2, tsif0, tsif1, >> + uim1_clk, uim1_data, uim1_present, >> uim1_reset, uim2_clk, uim2_data, uim2_present, uim2_reset, >> uim_batt, usb_phy, vfr_1, vsense_clkout, vsense_data0, >> vsense_data1, vsense_mode, wlan1_adc0, wlan1_adc1, >> @@ -179,15 +178,9 @@ Example: >> #interrupt-cells = <2>; >> >> uart_console_active: uart_console_active { >> - mux { >> - pins = "gpio4", "gpio5"; >> - function = "blsp_uart8_a"; >> - }; >> - >> - config { >> - pins = "gpio4", "gpio5"; >> - drive-strength = <2>; >> - bias-disable; >> - }; >> + pins = "gpio4", "gpio5"; >> + function = "blsp_uart8_a"; >> + drive-strength = <2>; >> + bias-disable; >> }; > > Whats with these "uart" changes? Modernizing the example? Doesn't > seem related to the commit text... Good point. I squashed one commit too many. I'll drop that hunk in v2. (Yes, I meant to document the syntax recommended by Bjorn.) Regards. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together 2019-06-26 14:38 ` [PATCH v1] pinctrl: msm8998: Squash TSIF pins together Marc Gonzalez 2019-06-26 14:42 ` Jeffrey Hugo @ 2019-06-26 15:30 ` Jonathan Neuschäfer 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Neuschäfer @ 2019-06-26 15:30 UTC (permalink / raw) To: Marc Gonzalez; +Cc: Bjorn Andersson, Linus Walleij, MSM, gpio, Jeffrey Hugo [-- Attachment #1: Type: text/plain, Size: 564 bytes --] Hi, On Wed, Jun 26, 2019 at 04:38:58PM +0200, Marc Gonzalez wrote: > Preamble: Rename tsif1 to tsif0, tsif2 to tsif1. > Squash tsif0 pins into a single function. Same for tsif1. "Preamble:"? What does this mean in context? BTW, the devicetree people (devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>), seem to be missing from the Cc list; they are at least printed when I run "scripts/get_maintainer.pl -f Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt" on v5.2-rc4. thanks and greetings, Jonathan Neuschäfer [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-26 15:30 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-11 16:12 How to write "modern" pinctrl DT node Marc Gonzalez 2019-06-11 16:50 ` Jonathan Neuschäfer 2019-06-11 18:05 ` Bjorn Andersson 2019-06-20 11:02 ` Marc Gonzalez 2019-06-20 18:41 ` Bjorn Andersson 2019-06-26 14:38 ` [PATCH v1] pinctrl: msm8998: Squash TSIF pins together Marc Gonzalez 2019-06-26 14:42 ` Jeffrey Hugo 2019-06-26 14:46 ` Marc Gonzalez 2019-06-26 15:30 ` Jonathan Neuschäfer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).