Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver
From: Srinivas Kandagatla
Date: Wed Dec 02 2020 - 05:06:22 EST
Thanks Alex for the comments,
On 01/12/2020 20:21, Alex Elder wrote:
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@xxxxxxxxxx>
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.
I did realize that while I was doing replace bits ,but did not go
further in possibility of removing the redundant macros!
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.
Many thanks for the examples, Will convert to either using FIELD_*
macros or u32_encode/get apis and remove redundant SHIFT macros!
--srini
-Alex