| Message ID | 1638891339-21806-1-git-send-email-quic_srivasam@quicinc.com |
|---|---|
| Headers | show |
| Series | Add pin control support for lpass sc7280 | expand |
On Wed, Dec 8, 2021 at 2:39 AM Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> wrote: > > Extract the chip specific SM8250 data from the LPASS LPI pinctrl driver > to allow reusing the common code in the addition of subsequent > platforms. ... > @@ -661,8 +454,10 @@ static int lpi_pinctrl_probe(struct platform_device *pdev) > > return ret; > } > +EXPORT_SYMBOL(lpi_pinctrl_probe); > + Stray change. ... > +#ifndef __PINCTRL_LPASS_LPI_H__ > +#define __PINCTRL_LPASS_LPI_H__ Missed headers. At least bits.h. ... > +#define NO_SLEW -1 Naming sucks for the header. LPI_NO_SLEW ? ... > +struct lpi_pingroup { > + const char *name; > + const unsigned int *pins; > + unsigned int npins; > + unsigned int pin; > + /* Bit offset in slew register for SoundWire pins only */ > + int slew_offset; > + unsigned int *funcs; > + unsigned int nfuncs; > +}; Are you going to convert this to use struct group_desc? ... > + LPI_MUX__, Strange naming. Besides, if it is the terminator, drop the comma.
On 07/12/2021 15:35, Srinivasa Rao Mandadapu wrote: > Add device tree binding Documentation details for Qualcomm SC7280 > LPASS LPI pinctrl driver. > > 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> > --- I remember in my previous review that I requested you to use git mv for renaming this If you do that you will endup diff stat something like this: ------------------------->cut<----------------------------- diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml similarity index 97% rename from Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml rename to Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml index e47ebf934daf..76f205a47640 100644 --- a/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml @@ -1,7 +1,7 @@ # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 --- -$id: http://devicetree.org/schemas/pinctrl/qcom,lpass-lpi-pinctrl.yaml# +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# title: Qualcomm Technologies, Inc. Low Power Audio SubSystem (LPASS) ------------------------->cut<----------------------------- --srini > .../pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml | 115 +++++++++++++++++++++ > 1 file changed, 115 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml > new file mode 100644 > index 0000000..d32ee32 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml > @@ -0,0 +1,115 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Technologies, Inc. Low Power Audio SubSystem (LPASS) > + Low Power Island (LPI) TLMM block > + > +maintainers: > + - Srinivasa Rao Mandadapu <srivasam@codeaurora.org> > + - Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + > +description: | > + This binding describes the Top Level Mode Multiplexer block found in the > + LPASS LPI IP on most Qualcomm SoCs > + > +properties: > + compatible: > + const: qcom,sc7280-lpass-lpi-pinctrl > + > + reg: > + minItems: 2 > + maxItems: 2 > + > + gpio-controller: true > + > + '#gpio-cells': > + description: Specifying the pin number and flags, as defined in > + include/dt-bindings/gpio/gpio.h > + const: 2 > + > + gpio-ranges: > + maxItems: 1 > + > +#PIN CONFIGURATION NODES > +patternProperties: > + '-pins$': > + type: object > + description: > + Pinctrl node's client devices use subnodes for desired pin configuration. > + Client device subnodes use below standard properties. > + $ref: "/schemas/pinctrl/pincfg-node.yaml" > + > + properties: > + pins: > + description: > + List of gpio pins affected by the properties specified in this > + subnode. > + items: > + oneOf: > + - pattern: "^gpio([0-9]|[1-9][0-9])$" > + minItems: 1 > + maxItems: 15 > + > + function: > + enum: [ gpio, swr_tx_clk, qua_mi2s_sclk, swr_tx_data, qua_mi2s_ws, > + qua_mi2s_data, swr_rx_clk, swr_rx_data, dmic1_clk, i2s1_clk, > + dmic1_data, i2s1_ws, dmic2_clk, dmic2_data, i2s1_data, > + i2s2_clk, wsa_swr_clk, i2s2_ws, wsa_swr_data, dmic3_clk, > + dmic3_data, i2s2_data ] > + description: > + Specify the alternative function to be configured for the specified > + pins. > + > + drive-strength: > + enum: [2, 4, 6, 8, 10, 12, 14, 16] > + default: 2 > + description: > + Selects the drive strength for the specified pins, in mA. > + > + slew-rate: > + enum: [0, 1, 2, 3] > + default: 0 > + description: | > + 0: No adjustments > + 1: Higher Slew rate (faster edges) > + 2: Lower Slew rate (slower edges) > + 3: Reserved (No adjustments) > + > + bias-pull-down: true > + > + bias-pull-up: true > + > + bias-disable: true > + > + output-high: true > + > + output-low: true > + > + required: > + - pins > + - function > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - gpio-controller > + - '#gpio-cells' > + - gpio-ranges > + > +additionalProperties: false > + > +examples: > + - | > + lpass_tlmm: pinctrl@33c0000 { > + compatible = "qcom,sc7280-lpass-lpi-pinctrl"; > + reg = <0x33c0000 0x20000>, > + <0x3550000 0x10000>; > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ranges = <&lpass_tlmm 0 0 15>; > + }; >
On 12/8/2021 2:54 PM, Srinivas Kandagatla wrote: Thanks froYour time Srini!!! > > On 07/12/2021 15:35, Srinivasa Rao Mandadapu wrote: >> Add device tree binding Documentation details for Qualcomm SC7280 >> LPASS LPI pinctrl driver. >> >> 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> >> --- > > > I remember in my previous review that I requested you to use git mv > for renaming this Yes. Created patch with "git mv" and commit. Not sure why diff is not as expected. > > If you do that you will endup diff stat something like this: > > ------------------------->cut<----------------------------- > diff --git > a/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml > b/Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml > > similarity index 97% > rename from > Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml > rename to > Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml > index e47ebf934daf..76f205a47640 100644 > --- > a/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml > +++ > b/Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > %YAML 1.2 > --- > -$id: http://devicetree.org/schemas/pinctrl/qcom,lpass-lpi-pinctrl.yaml# > +$id: > http://devicetree.org/schemas/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > title: Qualcomm Technologies, Inc. Low Power Audio SubSystem (LPASS) > ------------------------->cut<----------------------------- > > --srini > >> .../pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml | 115 >> +++++++++++++++++++++ >> 1 file changed, 115 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml >> >> diff --git >> a/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml >> b/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml >> >> new file mode 100644 >> index 0000000..d32ee32 >> --- /dev/null >> +++ >> b/Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml >> @@ -0,0 +1,115 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: >> http://devicetree.org/schemas/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm Technologies, Inc. Low Power Audio SubSystem (LPASS) >> + Low Power Island (LPI) TLMM block >> + >> +maintainers: >> + - Srinivasa Rao Mandadapu <srivasam@codeaurora.org> >> + - Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> + >> +description: | >> + This binding describes the Top Level Mode Multiplexer block found >> in the >> + LPASS LPI IP on most Qualcomm SoCs >> + >> +properties: >> + compatible: >> + const: qcom,sc7280-lpass-lpi-pinctrl >> + >> + reg: >> + minItems: 2 >> + maxItems: 2 >> + >> + gpio-controller: true >> + >> + '#gpio-cells': >> + description: Specifying the pin number and flags, as defined in >> + include/dt-bindings/gpio/gpio.h >> + const: 2 >> + >> + gpio-ranges: >> + maxItems: 1 >> + >> +#PIN CONFIGURATION NODES >> +patternProperties: >> + '-pins$': >> + type: object >> + description: >> + Pinctrl node's client devices use subnodes for desired pin >> configuration. >> + Client device subnodes use below standard properties. >> + $ref: "/schemas/pinctrl/pincfg-node.yaml" >> + >> + properties: >> + pins: >> + description: >> + List of gpio pins affected by the properties specified in >> this >> + subnode. >> + items: >> + oneOf: >> + - pattern: "^gpio([0-9]|[1-9][0-9])$" >> + minItems: 1 >> + maxItems: 15 >> + >> + function: >> + enum: [ gpio, swr_tx_clk, qua_mi2s_sclk, swr_tx_data, >> qua_mi2s_ws, >> + qua_mi2s_data, swr_rx_clk, swr_rx_data, dmic1_clk, >> i2s1_clk, >> + dmic1_data, i2s1_ws, dmic2_clk, dmic2_data, i2s1_data, >> + i2s2_clk, wsa_swr_clk, i2s2_ws, wsa_swr_data, >> dmic3_clk, >> + dmic3_data, i2s2_data ] >> + description: >> + Specify the alternative function to be configured for the >> specified >> + pins. >> + >> + drive-strength: >> + enum: [2, 4, 6, 8, 10, 12, 14, 16] >> + default: 2 >> + description: >> + Selects the drive strength for the specified pins, in mA. >> + >> + slew-rate: >> + enum: [0, 1, 2, 3] >> + default: 0 >> + description: | >> + 0: No adjustments >> + 1: Higher Slew rate (faster edges) >> + 2: Lower Slew rate (slower edges) >> + 3: Reserved (No adjustments) >> + >> + bias-pull-down: true >> + >> + bias-pull-up: true >> + >> + bias-disable: true >> + >> + output-high: true >> + >> + output-low: true >> + >> + required: >> + - pins >> + - function >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - gpio-controller >> + - '#gpio-cells' >> + - gpio-ranges >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + lpass_tlmm: pinctrl@33c0000 { >> + compatible = "qcom,sc7280-lpass-lpi-pinctrl"; >> + reg = <0x33c0000 0x20000>, >> + <0x3550000 0x10000>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + gpio-ranges = <&lpass_tlmm 0 0 15>; >> + }; >>
On Wed, Dec 08, 2021 at 03:41:25PM +0530, Srinivasa Rao Mandadapu wrote: > > On 12/8/2021 2:54 PM, Srinivas Kandagatla wrote: > Thanks froYour time Srini!!! > > > > On 07/12/2021 15:35, Srinivasa Rao Mandadapu wrote: > > > Add device tree binding Documentation details for Qualcomm SC7280 > > > LPASS LPI pinctrl driver. > > > > > > 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> > > > --- > > > > > > I remember in my previous review that I requested you to use git mv for > > renaming this > Yes. Created patch with "git mv" and commit. Not sure why diff is not as > expected. The 'git mv' is not what matters. You need the -M option for git-format-patch/send-email. There's a config option you can enable that by default. Rob
On Tue, 07 Dec 2021 21:05:36 +0530, Srinivasa Rao Mandadapu wrote: > Add device tree binding Documentation details for Qualcomm SC7280 > LPASS LPI pinctrl driver. > > 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> > --- > .../pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml | 115 +++++++++++++++++++++ > 1 file changed, 115 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On 12/8/2021 11:58 AM, Andy Shevchenko wrote: Thanks for your time Andy!!! > On Wed, Dec 8, 2021 at 2:39 AM Srinivasa Rao Mandadapu > <quic_srivasam@quicinc.com> wrote: >> Extract the chip specific SM8250 data from the LPASS LPI pinctrl driver >> to allow reusing the common code in the addition of subsequent >> platforms. > ... > >> @@ -661,8 +454,10 @@ static int lpi_pinctrl_probe(struct platform_device *pdev) >> >> return ret; >> } >> +EXPORT_SYMBOL(lpi_pinctrl_probe); >> + > Stray change. > > ... okay. will remove it. > >> +#ifndef __PINCTRL_LPASS_LPI_H__ >> +#define __PINCTRL_LPASS_LPI_H__ > Missed headers. > At least bits.h. > > ... Okay. will add. >> +#define NO_SLEW -1 > Naming sucks for the header. > > LPI_NO_SLEW ? Actually it's already mainline code. Just these patches are rearrangement of old code. still do you suggest to change? > > ... > >> +struct lpi_pingroup { >> + const char *name; >> + const unsigned int *pins; >> + unsigned int npins; >> + unsigned int pin; >> + /* Bit offset in slew register for SoundWire pins only */ >> + int slew_offset; >> + unsigned int *funcs; >> + unsigned int nfuncs; >> +}; > Are you going to convert this to use struct group_desc? > > ... > >> + LPI_MUX__, > Strange naming. Besides, if it is the terminator, drop the comma. okay will remove comma. but name is from existing code. >
On Tue, Dec 14, 2021 at 7:15 PM Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> wrote: > On 12/8/2021 11:58 AM, Andy Shevchenko wrote: ... > >> +#define NO_SLEW -1 > > Naming sucks for the header. > > > > LPI_NO_SLEW ? > > Actually it's already mainline code. Just these patches are > rearrangement of old code. > > still do you suggest to change? I would, but this means it should be in a separate change. ... > >> +struct lpi_pingroup { > >> + const char *name; > >> + const unsigned int *pins; > >> + unsigned int npins; > >> + unsigned int pin; > >> + /* Bit offset in slew register for SoundWire pins only */ > >> + int slew_offset; > >> + unsigned int *funcs; > >> + unsigned int nfuncs; > >> +}; > > Are you going to convert this to use struct group_desc? Any comments on this? It sounds like further improvements.
On 12/14/2021 10:46 PM, Andy Shevchenko wrote: > On Tue, Dec 14, 2021 at 7:15 PM Srinivasa Rao Mandadapu > <quic_srivasam@quicinc.com> wrote: >> On 12/8/2021 11:58 AM, Andy Shevchenko wrote: > ... > >>>> +#define NO_SLEW -1 >>> Naming sucks for the header. >>> >>> LPI_NO_SLEW ? >> Actually it's already mainline code. Just these patches are >> rearrangement of old code. >> >> still do you suggest to change? > I would, but this means it should be in a separate change. > > ... Yes. Will do it separate patch later. > >>>> +struct lpi_pingroup { >>>> + const char *name; >>>> + const unsigned int *pins; >>>> + unsigned int npins; >>>> + unsigned int pin; >>>> + /* Bit offset in slew register for SoundWire pins only */ >>>> + int slew_offset; >>>> + unsigned int *funcs; >>>> + unsigned int nfuncs; >>>> +}; >>> Are you going to convert this to use struct group_desc? > Any comments on this? It sounds like further improvements. Actually this also needs as separate patch. these patches will do as separate series. >
On Tue, Dec 14, 2021 at 7:22 PM Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> wrote: > On 12/14/2021 10:46 PM, Andy Shevchenko wrote: > > On Tue, Dec 14, 2021 at 7:15 PM Srinivasa Rao Mandadapu > > <quic_srivasam@quicinc.com> wrote: > >> On 12/8/2021 11:58 AM, Andy Shevchenko wrote: ... > >>>> +struct lpi_pingroup { > >>>> + const char *name; > >>>> + const unsigned int *pins; > >>>> + unsigned int npins; > >>>> + unsigned int pin; > >>>> + /* Bit offset in slew register for SoundWire pins only */ > >>>> + int slew_offset; > >>>> + unsigned int *funcs; > >>>> + unsigned int nfuncs; > >>>> +}; > >>> Are you going to convert this to use struct group_desc? > > Any comments on this? It sounds like further improvements. > Actually this also needs as separate patch. these patches will do as > separate series. Of course, that's why I put the second sentence after the question in my reply.
Quoting Srinivasa Rao Mandadapu (2021-12-07 07:35:34) > This patch series is to split lpass variant common pin control > functions and SoC specific functions and to add lpass sc7280 pincontrol support. > It also Adds dt-bindings for lpass sc7280 lpass lpi pincontrol. What ensures that the LPI pins are being muxed out on the pads of the SoC? There's the eGPIO support in the tlmm driver, which seems to let us override the LPI pins and mux them away from this pinctrl device to the tlmm pinctrl device. Should this driver be requesting gpios from tlmm and making sure they're not muxed away to tlmm so we don't have conflicts?
This patch series is to split lpass variant common pin control functions and SoC specific functions and to add lpass sc7280 pincontrol support. It also Adds dt-bindings for lpass sc7280 lpass lpi pincontrol. Changes Since V4: -- Update commit message and description of the chip specific extraction patch. -- Sort macros in kconfig and makefile. -- Update optional clock voting to conditional clock voting. -- Fix typo errors. -- Move to quicinc domain email id's. Changes Since V3: -- Update separate Kconfig fields for sm8250 and sc7280. -- Update module license and description. -- Move static variables to corresponding .c files from header file. Changes Since V2: -- Add new dt-bindings for sc7280 lpi driver. -- Make clock voting change as separate patch. -- Split existing pincontrol driver and make common functions as part of separate file. -- Rename lpass pincontrol lpi dt-bindings to sm8250 specific dt-bindings Changes Since V1: -- Make lpi pinctrl variant data structure as constant -- Add appropriate commit message -- Change signedoff by sequence. Srinivasa Rao Mandadapu (5): dt-bindings: pinctrl: qcom: Update lpass lpi file name to SoC specific dt-bindings: pinctrl: qcom: Add sc7280 lpass lpi pinctrl bindings pinctrl: qcom: Extract chip specific LPASS LPI code pinctrl: qcom: Add SC7280 lpass pin configuration pinctrl: qcom: Update clock voting as optional .../bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml | 130 ----------- .../pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml | 115 ++++++++++ .../pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml | 130 +++++++++++ drivers/pinctrl/qcom/Kconfig | 16 ++ drivers/pinctrl/qcom/Makefile | 2 + drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 249 ++------------------- drivers/pinctrl/qcom/pinctrl-lpass-lpi.h | 85 +++++++ drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 170 ++++++++++++++ drivers/pinctrl/qcom/pinctrl-sm8250-lpass-lpi.c | 165 ++++++++++++++ 9 files changed, 697 insertions(+), 365 deletions(-) delete mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sc7280-lpass-lpi-pinctrl.yaml create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8250-lpass-lpi-pinctrl.yaml create mode 100644 drivers/pinctrl/qcom/pinctrl-lpass-lpi.h create mode 100644 drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c create mode 100644 drivers/pinctrl/qcom/pinctrl-sm8250-lpass-lpi.c