| Message ID | 20201201142830.13152-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | pinctrl: qcom: Add sm8250 lpass lpi pinctrl support | expand |
On Tue 01 Dec 08:28 CST 2020, Srinivas Kandagatla wrote: > Add device tree binding Documentation details for Qualcomm SM8250 > LPASS(Low Power Audio Sub System) LPI(Low Power Island) pinctrl driver. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Reviewed-by: Rob Herring <robh@kernel.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > .../pinctrl/qcom,lpass-lpi-pinctrl.yaml | 132 ++++++++++++++++++ > 1 file changed, 132 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml > new file mode 100644 > index 000000000000..3543324d9194 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,lpass-lpi-pinctrl.yaml > @@ -0,0 +1,132 @@ > +# 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# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Technologies, Inc. Low Power Audio SubSystem (LPASS) > + Low Power Island (LPI) TLMM block > + > +maintainers: > + - 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,sm8250-lpass-lpi-pinctrl > + > + reg: > + minItems: 2 > + maxItems: 2 > + > + clocks: > + items: > + - description: LPASS Core voting clock > + - description: LPASS Audio voting clock > + > + clock-names: > + items: > + - const: core > + - const: audio > + > + 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: 14 > + > + function: > + enum: [ gpio, swr_tx_clk, qua_mi2s_sclk, swr_tx_data1, qua_mi2s_ws, > + swr_tx_data2, qua_mi2s_data0, swr_rx_clk, qua_mi2s_data1, > + swr_rx_data1, qua_mi2s_data2, swr_tx_data3, swr_rx_data2, > + dmic1_clk, i2s1_clk, dmic1_data, i2s1_ws, dmic2_clk, > + i2s1_data0, dmic2_data, i2s1_data1, i2s2_clk, wsa_swr_clk, > + i2s2_ws, wsa_swr_data, dmic3_clk, i2s2_data0, dmic3_data, > + i2s2_data1 ] > + 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 > + - clocks > + - clock-names > + - gpio-controller > + - '#gpio-cells' > + - gpio-ranges > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/sound/qcom,q6afe.h> > + lpi_tlmm: pinctrl@33c0000 { > + compatible = "qcom,sm8250-lpass-lpi-pinctrl"; > + reg = <0x33c0000 0x20000>, > + <0x3550000 0x10000>; > + clocks = <&q6afecc LPASS_HW_MACRO_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>, > + <&q6afecc LPASS_HW_DCODEC_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>; > + clock-names = "core", "audio"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ranges = <&lpi_tlmm 0 0 14>; > + }; > -- > 2.21.0 >
On 12/1/20 8:28 AM, 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] > > 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. > Apart from this, slew rate is also available in this block for > certain pins which are connected to SLIMbus or SoundWire Bus. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Bjorn called my attention to a comment he made on this patch. I'm not giving it a full review right now, but I have a general suggestion below. > --- > drivers/pinctrl/qcom/Kconfig | 8 + > drivers/pinctrl/qcom/Makefile | 1 + > drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 727 +++++++++++++++++++++++ > 3 files changed, 736 insertions(+) > create mode 100644 drivers/pinctrl/qcom/pinctrl-lpass-lpi.c > > diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig > index 5fe7b8aaf69d..d3e4e89c2810 100644 > --- a/drivers/pinctrl/qcom/Kconfig > +++ b/drivers/pinctrl/qcom/Kconfig > @@ -236,4 +236,12 @@ config PINCTRL_SM8250 > Qualcomm Technologies Inc TLMM block found on the Qualcomm > Technologies Inc SM8250 platform. > > +config PINCTRL_LPASS_LPI > + tristate "Qualcomm Technologies Inc LPASS LPI pin controller driver" > + depends on GPIOLIB > + help > + This is the pinctrl, pinmux, pinconf and gpiolib driver for the > + Qualcomm Technologies Inc LPASS (Low Power Audio SubSystem) LPI > + (Low Power Island) found on the Qualcomm Technologies Inc SoCs. > + > endif > diff --git a/drivers/pinctrl/qcom/Makefile b/drivers/pinctrl/qcom/Makefile > index 9e3d9c91a444..c8520155fb1b 100644 > --- a/drivers/pinctrl/qcom/Makefile > +++ b/drivers/pinctrl/qcom/Makefile > @@ -28,3 +28,4 @@ obj-$(CONFIG_PINCTRL_SDM660) += pinctrl-sdm660.o > obj-$(CONFIG_PINCTRL_SDM845) += pinctrl-sdm845.o > obj-$(CONFIG_PINCTRL_SM8150) += pinctrl-sm8150.o > obj-$(CONFIG_PINCTRL_SM8250) += pinctrl-sm8250.o > +obj-$(CONFIG_PINCTRL_LPASS_LPI) += pinctrl-lpass-lpi.o > diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c > new file mode 100644 > index 000000000000..96c63a33fc99 > --- /dev/null > +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c > @@ -0,0 +1,727 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved. > + * Copyright (c) 2020 Linaro Ltd. > + */ > + > +#include <linux/bitops.h> > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/gpio/driver.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of.h> > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinconf.h> > +#include <linux/pinctrl/pinmux.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include "../core.h" > +#include "../pinctrl-utils.h" > + > +#define LPI_GPIO_CFG_REG 0x00 > +#define LPI_GPIO_VALUE_REG 0x04 > +#define LPI_SLEW_RATE_CTL_REG 0xa000 > +#define LPI_SLEW_RATE_MAX 0x03 > +#define LPI_SLEW_BITS_SIZE 0x02 > +#define LPI_GPIO_PULL_SHIFT 0x0 > +#define LPI_GPIO_PULL_MASK GENMASK(1, 0) . . . If you have a mask like this, you do not need the shift. The mask alone encodes both the position and the width of the field you are describing. It is better to use just the one (mask) value and avoid even the possibility of the mask and shift being inconsistent. You halve the number of symbols you need to describe fields too. For the macros and functions in <linux/bitfield.h> the mask values must be constant at compile time, but you have that here. For the LPI_GPIO_PULL, you use it below this way: pull = (ctl_reg & LPI_GPIO_PULL_MASK) >> LPI_GPIO_PULL_SHIFT; Instead, use: pull = u32_get_bits(ctl_reg, LPI_GPIO_PULL_MASK); I see you're using u32_replace_bits(), and what I see looks good. But you can use u32_encode_bits() as well. For example, instead of: lpi_gpio_write(pctrl, group, LPI_GPIO_VALUE_REG, value << LPI_GPIO_DIR_SHIFT); Use: val = u32_encode_bits(value ? 1 : 0, LPI_GPIO_DIR_MASK); lpi_gpio_write(pctrl, group, LPI_GPIO_VALUE_REG, val); (This one-bit mask might not be a great example.) In addition to getting rid of extra symbols, using these functions typically results in simpler-looking code. -Alex