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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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
>