| 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
| ||
|
Date: Wed, 2 Dec 2020 10:05:25 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Alex Elder <elder@...e.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
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@...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.
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
Powered by blists - more mailing lists