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
