| Message ID | 1654079415-26217-3-git-send-email-quic_srivasam@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add pinctrl support adsp based platforms | expand |
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: > 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 = { Remove static and export this struct in drivers/pinctrl/qcom/pinctrl-lpass-lpi.h > + .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; Yours, Linus Walleij
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@quicinc.com> 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@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 = { > 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
On Fri, Jun 3, 2022 at 1:03 PM Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> wrote: > >> @@ -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; Looks good to me! Yours, Linus Walleij
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);