Messages in this thread | | | Subject | Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver | From | Srinivas Kandagatla <> | Date | Wed, 2 Dec 2020 10:05:25 +0000 |
| |
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@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.
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
| |