Re: How to write "modern" pinctrl DT node

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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