Re: [PATCH v4 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver

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

On Tue 01 Dec 04:01 CST 2020, Srinivas Kandagatla wrote:

> Many thanks for review Bjorn,
> 
> 
> On 01/12/2020 00:47, Bjorn Andersson wrote:
> > On Mon 16 Nov 08:34 CST 2020, Srinivas Kandagatla wrote:
> > 
> > > Add initial pinctrl driver to support pin configuration for
> > > LPASS (Low Power Audio SubSystem) LPI (Low Power Island) pinctrl
> > > on SM8250.
> > > 
> > > This IP is an additional pin control block for Audio Pins on top the
> > > existing SoC Top level pin-controller.
> > > Hardware setup looks like:
> > > 
> > > TLMM GPIO[146 - 159] --> LPASS LPI GPIO [0 - 13]
> > > 
> > 
> > Iiuc the LPI TLMM block is just "another pinmux/pinconf block" found in
> > these SoCs, with the additional magic that the 14 pads are muxed with
> > some of the TLMM pins - to allow the system integrator to choose how
> > many pins the LPI should have access to.
> > 
> > I also believe this is what the "egpio" bit in the TLMM registers are
> > used for (i.e. egpio = route to LPI, egpio = 1 route to TLMM), so we
> > should need to add support for toggling this bit in the TLMM as well
> > (which I think we should do as a pinconf in the pinctrl-msm).
> 
> Yes, we should add egpio function to these pins in main TLMM pinctrl!
> 

I was thinking about abusing the pinconf system, but reading you
sentence makes me feel that expressing it as a "function" and adding a
special case handling in msm_pinmux_set_mux() would actually make things
much cleaner to the outside.

i.e. we would then end up with something in DT like:

	pin-is-normal-tlmm-pin {
		pins = "gpio146";
		function = "gpio";
	};

and

	pin-routed-to-lpi-pin {
		pins = "gpio146";
		function = "egpio";
	};

Only "drawback" I can see is that we're inverting the chip's meaning of
"egpio" (i.e. active means route-to-tlmm in the hardware).

> > 
> > > This pin controller has some similarities compared to Top level
> > > msm SoC Pin controller like 'each pin belongs to a single group'
> > > and so on. However this one is intended to control only audio
> > > pins in particular, which can not be configured/touched by the
> > > Top level SoC pin controller except setting them as gpios.
[..]
> > > diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
[..]
> > > +	LPI_MUX_qua_mi2s_sclk,
> > > +	LPI_MUX_swr_tx_data1,
> > 
> > As there's no single pin that can be both data1 and data2 I think you
> > should have a single group for swr_tx_data and use this function for
> > both swr_tx_data pins. Or perhaps even just have one for swr or swr_tx.
> > 
> > (This is nice when you're writing DT later on)
> 
> I did think about this, but we have a rx_data2 pin in different function
> compared to other rx data pins.
> 
> The reason to keep it as it is :
> 1> as this will bring in an additional complexity to the code

For each pin lpi_gpio_set_mux() will be invoked and you'd be searching
for the index (i) among that pins .funcs. So it doesn't matter that
looking up a particular function results in different register values
for different pins, it's already dealt with.

> 2> we have these represented exactly as what hw data sheet mentions it!
> 

That is true, but the result is that you have to write 2 states in the
DT to get your 2 pins to switch to the particular function. By grouping
them you could do:

	data-pins {
		pins = "gpio1", "gpio2";
		function = "swr_tx_data";
	};


We do this quite extensively for the TLMM (pinctrl-msm) because it
results in cleaner DT.

> > 
> > > +	LPI_MUX_qua_mi2s_ws,
[..]
> > > +static struct lpi_pinctrl_variant_data sm8250_lpi_data = {
> > > +	.tlmm_reg_offset = 0x1000,
> > 
> > Do we have any platform in sight where this is not 0x1000? Could we just
> > make a define out of it?
> Am not 100% sure ATM, But I wanted to keep this flexible as these offsets in
> downstream were part of device tree for some reason, so having offset here
> for particular compatible made more sense for me!
> 

Downtream does indeed favor "flexible" code. I tend to prefer a #define
until we actually need the flexibility...

Regards,
Bjorn