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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bf18cc63-77b2-7929-625a-1ea3d108916e@ieee.org>
Date:   Tue, 1 Dec 2020 14:21:58 -0600
From:   Alex Elder <elder@...e.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linus.walleij@...aro.org, bjorn.andersson@...aro.org
Cc:     robh+dt@...nel.org, agross@...nel.org,
        linux-arm-msm@...r.kernel.org, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver

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@...aro.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