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

[Srinivas Kandagatla]

On 01/12/2020 17:28, Bjorn Andersson wrote:
> 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";
> 	};

That is what I was thinking of.

> 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).
At somepoint we need to start defining what egpio really means w.r.t 
pinctrl setup!

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

These are now changed as requested!

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

Done!

--srini
> Regards,
> Bjorn