Re: [PATCH v2 2/2] pinctrl: qcom: sc7280: Add lpi pinctrl variant data for adsp based targets

Srinivasa Rao Mandadapu <quic_srivasam@xxxxxxxxxxx> · Fri, 3 Jun 2022 16:33:12 +0530

On 6/3/2022 3:58 PM, Linus Walleij wrote:
Thanks for Your time and valuable inputs Linus!!!
On Wed, Jun 1, 2022 at 12:30 PM Srinivasa Rao Mandadapu
<quic_srivasam@xxxxxxxxxxx> wrote:

So one way to just use a propert and avoid more compatible strings:

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@xxxxxxxxxxx>
Co-developed-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
Signed-off-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
---
  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 = {
Remove static and export this struct in drivers/pinctrl/qcom/pinctrl-lpass-lpi.h
We can remove entire data structure if my below approach is okay.

+       .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,
+       },
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;

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






