How to write "modern" pinctrl DT node

How to write "modern" pinctrl DT node

[Marc Gonzalez]

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.

Re: How to write "modern" pinctrl DT node

[Jonathan Neuschäfer]

[-- 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 --]

Re: How to write "modern" pinctrl DT node

[Bjorn Andersson]

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

Re: How to write "modern" pinctrl DT node

[Marc Gonzalez]

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.

Re: How to write "modern" pinctrl DT node

[Bjorn Andersson]

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

[PATCH v1] pinctrl: msm8998: Squash TSIF pins together

[Marc Gonzalez]

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

Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together

[Jeffrey Hugo]

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

Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together

[Marc Gonzalez]

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.

Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together

[Jonathan Neuschäfer]

[-- 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 --]