[PATCH v2 0/2] Add pinctrl support adsp based platforms
This patch set is to add pinctrl support adsp enabled sc7280 platforms.
Changes Since V1: -- Update commit message.
Srinivasa Rao Mandadapu (2): dt-bindings: pinctrl: qcom: sc7280: Add compatible string for adsp based platforms pinctrl: qcom: sc7280: Add lpi pinctrl variant data for adsp based targets
.../bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml | 4 +++- drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-)
Add compatible string to support adsp enabled sc7280 platforms.
Signed-off-by: Srinivasa Rao Mandadapu quic_srivasam@quicinc.com Co-developed-by: Venkata Prasad Potturu quic_potturu@quicinc.com Signed-off-by: Venkata Prasad Potturu quic_potturu@quicinc.com Acked-by: Rob Herring robh@kernel.org --- .../devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml index d32ee32..53c2c59 100644 --- a/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml @@ -17,7 +17,9 @@ description: |
properties: compatible: - const: qcom,sc7280-lpass-lpi-pinctrl + enum: + - qcom,sc7280-lpass-lpi-pinctrl + - qcom,sc7280-lpass-adsp-lpi-pinctrl
reg: minItems: 2
Quoting Srinivasa Rao Mandadapu (2022-06-01 03:30:14)
Can you confirm that this is the same hardware (i.e. same reg property) but just a different compatible string used to convey that the device is using "adsp" mode or not? If so, this looks to be a common pattern for the audio hardware here, where we have two "views" of the hardware, one for adsp mode and one for not adsp mode. I guess the not adsp mode is called "adsp bypass"?
Is that right? Why are we conveying this information via the compatible string?
On 6/2/2022 6:43 AM, Stephen Boyd wrote:
Yes Your understanding is correct. The same hardware in scenario not using ADSP,
and in another enabling DSP.
Is that right? Why are we conveying this information via the compatible string?
Could you please suggest better way!. As pin control driver is the first one to probe, I am not getting better approach.
While up-streaming these drivers, concluded to use this approach.
On Fri, Jun 3, 2022 at 8:09 AM Srinivasa Rao Mandadapu quic_srivasam@quicinc.com wrote:
On 6/2/2022 6:43 AM, Stephen Boyd wrote:
The device tree conveys hardware description and some configuration.
If this is configuration thing, either you could perhaps determine it from the hardware (if set up in hardware or boot loader) and if that is not possible it should just be a boolean property of the device node:
{ compatible = "..."; qcom.adsp-mode; }
If you are probing two different drivers depending on the mode, then there is a problem of course, but it is a Linux problem not a device tree problem.
Yours, Linus Walleij
Add compatible string and lpi pinctrl variant data structure for adsp enabled sc7280 platforms. This variant data structure rnd compatible name required for distingushing ADSP based platforms and ADSP bypass platforms. In case of ADSP enabled platforms, where audio is routed through ADSP macro and decodec GDSC Switches are triggered as clocks by pinctrl driver and ADSP firmware controls them. So It's mandatory to enable them in ADSP based solutions. In case of ADSP bypass platforms clock voting is optional as these macro and dcodec GDSC switches are maintained as power domains and operated from lpass clock drivers.
Signed-off-by: Srinivasa Rao Mandadapu quic_srivasam@quicinc.com Co-developed-by: Venkata Prasad Potturu quic_potturu@quicinc.com Signed-off-by: Venkata Prasad Potturu quic_potturu@quicinc.com --- drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c index 2add9a4..c9e85d9 100644 --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c @@ -134,6 +134,16 @@ static const struct lpi_function sc7280_functions[] = { LPI_FUNCTION(wsa_swr_data), };
+static const struct lpi_pinctrl_variant_data sc7280_adsp_lpi_data = { + .pins = sc7280_lpi_pins, + .npins = ARRAY_SIZE(sc7280_lpi_pins), + .groups = sc7280_groups, + .ngroups = ARRAY_SIZE(sc7280_groups), + .functions = sc7280_functions, + .nfunctions = ARRAY_SIZE(sc7280_functions), + .is_clk_optional = false, +}; + static const struct lpi_pinctrl_variant_data sc7280_lpi_data = { .pins = sc7280_lpi_pins, .npins = ARRAY_SIZE(sc7280_lpi_pins), @@ -149,6 +159,10 @@ static const struct of_device_id lpi_pinctrl_of_match[] = { .compatible = "qcom,sc7280-lpass-lpi-pinctrl", .data = &sc7280_lpi_data, }, + { + .compatible = "qcom,sc7280-lpass-adsp-lpi-pinctrl", + .data = &sc7280_adsp_lpi_data, + }, { } }; MODULE_DEVICE_TABLE(of, lpi_pinctrl_of_match);
On Wed, Jun 1, 2022 at 12:30 PM Srinivasa Rao Mandadapu quic_srivasam@quicinc.com wrote:
So one way to just use a propert and avoid more compatible strings:
Remove static and export this struct in drivers/pinctrl/qcom/pinctrl-lpass-lpi.h
Drop this and instead add some code in the probe() in drivers/pinctrl/qcom/pinctrl-lpass-lpi.c lines:
if (of_device_is_compatible(np, "qcom,sc7280-lpass-lpi-pinctrl") && of_property_read_bool(np, "qcom,adsp-mode)) data = &sc7280_adsp_lpi_data;
Yours, Linus Walleij
On 6/3/2022 3:58 PM, Linus Walleij wrote: Thanks for Your time and valuable inputs Linus!!!
We can remove entire data structure if my below approach is okay.
Here, only diff between ADSP and ADSP bypass variant dats is "is_clk_optional" field.
So we can keep something like this. Kindly suggest, if it's not making sense.
if (of_device_is_compatible(np, "qcom,sc7280-lpass-lpi-pinctrl") && of_property_read_bool(np, "qcom,adsp-mode)) data->is_clk_optional = false;
Yours, Linus Walleij
On Fri, Jun 3, 2022 at 1:03 PM Srinivasa Rao Mandadapu quic_srivasam@quicinc.com wrote:
Looks good to me!
Yours, Linus Walleij
participants (3)
-
Linus Walleij
-
Srinivasa Rao Mandadapu
-
Stephen Boyd