All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: linus.walleij@linaro.org, robh+dt@kernel.org, agross@kernel.org,
	linux-arm-msm@vger.kernel.org, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver
Date: Tue, 1 Dec 2020 11:28:23 -0600	[thread overview]
Message-ID: <X8Z9N2Yu8xiyPRmj@builder.lan> (raw)
In-Reply-To: <ec14afaa-8660-03ac-fbf9-79ff37889de3@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

  reply	other threads:[~2020-12-01 17:29 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-16 14:34 [PATCH v4 0/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl support Srinivas Kandagatla
2020-11-16 14:34 ` [PATCH v4 1/2] dt-bindings: pinctrl: qcom: Add sm8250 lpass lpi pinctrl bindings Srinivas Kandagatla
2020-11-30 23:49   ` Rob Herring
2020-12-01  0:55   ` Bjorn Andersson
2020-12-01 10:03     ` Srinivas Kandagatla
2020-11-16 14:34 ` [PATCH v4 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver Srinivas Kandagatla
2020-12-01  0:47   ` Bjorn Andersson
2020-12-01 10:01     ` Srinivas Kandagatla
2020-12-01 17:28       ` Bjorn Andersson [this message]
2020-12-02 15:18         ` Srinivas Kandagatla

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=X8Z9N2Yu8xiyPRmj@builder.lan \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=srinivas.kandagatla@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.