Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver

Alex Elder
2020-12-01T20:23:53Z

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