Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     linus.walleij@linaro.org, robh+dt@kernel.org, agross@kernel.org,
        linux-arm-msm@vger.kernel.org, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20201201142830.13152-1-srinivas.kandagatla@linaro.org>
 <20201201142830.13152-3-srinivas.kandagatla@linaro.org>
 <X8ad9lw73ZKsqXKT@builder.lan>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <c7f8e95d-21aa-b723-2b6e-0420cb400025@linaro.org>
Date:   Wed, 2 Dec 2020 09:56:35 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <X8ad9lw73ZKsqXKT@builder.lan>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thanks Bjorn for the review,

On 01/12/2020 19:48, Bjorn Andersson wrote:
> On Tue 01 Dec 08:28 CST 2020, 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>
>> ---
>>   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)
>> +#define LPI_GPIO_FUNCTION_SHIFT		0x2
>> +#define LPI_GPIO_FUNCTION_MASK	GENMASK(5, 2)
>> +#define LPI_GPIO_OUT_STRENGTH_SHIFT	0x6
>> +#define LPI_GPIO_OUT_STRENGTH_MASK	GENMASK(8, 6)
>> +#define LPI_GPIO_OE_SHIFT		0x9
>> +#define LPI_GPIO_OE_MASK		BIT(9)
>> +#define LPI_GPIO_DIR_SHIFT		0x1
>> +#define LPI_GPIO_DIR_MASK		0x2
> 
> The naming of these two constants no longer relates the their register.
> And you no longer use the MASK, so please drop this.
> 
Yes, most of these defines are redundant can be removed, I will clean 
that up!

> That said the use of "shift" still doesn't give away the obvious fact
> that this is a single bit specifying if the output should be high or
> low. You would capture this by doing something like:
> 
> #define LPI_GPIO_VALUE_HIGH	BIT(1)
> 
Sure will make this explicit!

>> +#define LPI_GPIO_BIAS_DISABLE		0x0
>> +#define LPI_GPIO_PULL_DOWN		0x1
>> +#define LPI_GPIO_KEEPER			0x2
>> +#define LPI_GPIO_PULL_UP		0x3
>> +#define LPI_GPIO_DS_TO_VAL(v) ((v / 2 - 1) << LPI_GPIO_OUT_STRENGTH_SHIFT)
>> +#define NO_SLEW				-1
>> +
>> +#define LPI_FUNCTION(fname)			                \
>> +	[LPI_MUX_##fname] = {		                \
>> +		.name = #fname,				\
>> +		.groups = fname##_groups,               \
>> +		.ngroups = ARRAY_SIZE(fname##_groups),	\
>> +	}
>> +
>>
>> +
>> +static int lpi_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
>> +			    unsigned int group_num)
>> +{
>> +	struct lpi_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> +	const struct lpi_pingroup *g = &pctrl->data->groups[group_num];
>> +	u32 val;
>> +	int i, pin = g->pin;
>> +
>> +	for (i = 0; i < g->nfuncs; i++) {
>> +		if (g->funcs[i] == function)
>> +			break;
>> +	}
>> +
>> +	if (WARN_ON(i == g->nfuncs))
>> +		return -EINVAL;
>> +
>> +	val = lpi_gpio_read(pctrl, pin, LPI_GPIO_CFG_REG);
>> +	u32p_replace_bits(&val, i, LPI_GPIO_FUNCTION_MASK);
> 
> I really meant FIELD_SET(), but now realize that there's only
> FIELD_PREP() and FIELD_GET() in linux/bitfields.h. I think that should
> be corrected, but this seems to do the trick...
> 
I knew that FILED_SET does not exists, Howeever using FIELD_* macros 
here to modify bits would still require 2 lines, one to manually clear 
the bits and then use FILED_SET.

replace_bits() api does nicely!

>> +	lpi_gpio_write(pctrl, pin, LPI_GPIO_CFG_REG, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pinmux_ops lpi_gpio_pinmux_ops = {
>> +	.get_functions_count	= lpi_gpio_get_functions_count,
>> +	.get_function_name	= lpi_gpio_get_function_name,
>> +	.get_function_groups	= lpi_gpio_get_function_groups,
>> +	.set_mux		= lpi_gpio_set_mux,
>> +};
>> +
>> +static int lpi_config_get(struct pinctrl_dev *pctldev,
>> +			  unsigned int pin, unsigned long *config)
>> +{
>> +	unsigned int param = pinconf_to_config_param(*config);
>> +	struct lpi_pinctrl *state = dev_get_drvdata(pctldev->dev);
>> +	unsigned int arg = 0;
>> +	int is_out;
>> +	int pull;
>> +	u32 ctl_reg;
>> +
>> +	ctl_reg = lpi_gpio_read(state, pin, LPI_GPIO_CFG_REG);
>> +	is_out = ctl_reg & LPI_GPIO_OE_MASK;
>> +	pull = (ctl_reg & LPI_GPIO_PULL_MASK) >> LPI_GPIO_PULL_SHIFT;
>> +
>> +	switch (param) {
>> +	case PIN_CONFIG_BIAS_DISABLE:
>> +		if (pull == LPI_GPIO_BIAS_DISABLE)
>> +			arg = 1;
>> +		break;
>> +	case PIN_CONFIG_BIAS_PULL_DOWN:
>> +		if (pull == LPI_GPIO_PULL_DOWN)
>> +			arg = 1;
>> +		break;
>> +	case PIN_CONFIG_BIAS_BUS_HOLD:
>> +		if (pull == LPI_GPIO_KEEPER)
>> +			arg = 1;
>> +		break;
>> +	case PIN_CONFIG_BIAS_PULL_UP:
>> +		if (pull == LPI_GPIO_PULL_UP)
>> +			arg = 1;
>> +		break;
>> +	case PIN_CONFIG_INPUT_ENABLE:
>> +	case PIN_CONFIG_OUTPUT:
>> +		if (is_out)
>> +			arg = 1;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	*config = pinconf_to_config_packed(param, arg);
>> +	return 0;
>> +}
>> +
>> +static int lpi_config_set(struct pinctrl_dev *pctldev, unsigned int group,
>> +			  unsigned long *configs, unsigned int nconfs)
>> +{
>> +	struct lpi_pinctrl *pctrl = dev_get_drvdata(pctldev->dev);
>> +	unsigned int param, arg, pullup, strength;
>> +	bool value, output_enabled = false;
>> +	const struct lpi_pingroup *g;
>> +	unsigned long sval;
>> +	int i, slew_offset;
>> +	u32 val;
>> +
>> +	g = &pctrl->data->groups[group];
>> +	for (i = 0; i < nconfs; i++) {
>> +		param = pinconf_to_config_param(configs[i]);
>> +		arg = pinconf_to_config_argument(configs[i]);
>> +
>> +		switch (param) {
>> +		case PIN_CONFIG_BIAS_DISABLE:
>> +			pullup = LPI_GPIO_BIAS_DISABLE;
>> +			break;
>> +		case PIN_CONFIG_BIAS_PULL_DOWN:
>> +			pullup = LPI_GPIO_PULL_DOWN;
>> +			break;
>> +		case PIN_CONFIG_BIAS_BUS_HOLD:
>> +			pullup = LPI_GPIO_KEEPER;
>> +			break;
>> +		case PIN_CONFIG_BIAS_PULL_UP:
>> +			pullup = LPI_GPIO_PULL_UP;
>> +			break;
>> +		case PIN_CONFIG_INPUT_ENABLE:
>> +			output_enabled = false;
>> +			break;
>> +		case PIN_CONFIG_OUTPUT:
>> +			output_enabled = true;
>> +			value = arg;
>> +			break;
>> +		case PIN_CONFIG_DRIVE_STRENGTH:
>> +			strength = arg;
>> +			break;
>> +		case PIN_CONFIG_SLEW_RATE:
>> +			if (arg > LPI_SLEW_RATE_MAX) {
>> +				dev_err(pctldev->dev, "invalid slew rate %u for pin: %d\n",
>> +					arg, group);
>> +				return -EINVAL;
>> +			}
>> +
>> +			slew_offset = g->slew_offset;
>> +			if (slew_offset == NO_SLEW)
>> +				break;
>> +
>> +			mutex_lock(&pctrl->slew_access_lock);
>> +			sval = ioread32(pctrl->slew_base + LPI_SLEW_RATE_CTL_REG);
>> +
>> +			for (i = 0; i < LPI_SLEW_BITS_SIZE; i++) {
>> +				assign_bit(slew_offset, &sval, arg & 0x01);
>> +				slew_offset++;
>> +				arg = arg >> 1;
>> +			}
> 
> Isn't this loop just the same as
> 
> 	FIELD_SET(3 << slew_offset, arg & 3, sval)

This will not work FIELD_SET will not clear any bits wich are already 
set! assing_bit will work, but we could do better by adding slew_mask 
per pin rather than slew_offset which should allow us to use 
replace_bits straight away.

> 
> And as you already checked arg agains LPI_SLEW_RATE_MAX you should be
> able to drop the & 3 there in the middle.
> 
>> +
>> +			iowrite32(sval, pctrl->slew_base + LPI_SLEW_RATE_CTL_REG);
>> +
>> +			mutex_unlock(&pctrl->slew_access_lock);
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	val = lpi_gpio_read(pctrl, group, LPI_GPIO_CFG_REG);
>> +
>> +	u32p_replace_bits(&val, pullup, LPI_GPIO_PULL_MASK);
>> +	u32p_replace_bits(&val, LPI_GPIO_DS_TO_VAL(strength),
>> +			  LPI_GPIO_OUT_STRENGTH_MASK);
>> +	u32p_replace_bits(&val, output_enabled, LPI_GPIO_OE_MASK);
>> +
>> +	lpi_gpio_write(pctrl, group, LPI_GPIO_CFG_REG, val);
>> +
>> +	if (output_enabled)
>> +		lpi_gpio_write(pctrl, group, LPI_GPIO_VALUE_REG,
>> +			       value << LPI_GPIO_DIR_SHIFT);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pinconf_ops lpi_gpio_pinconf_ops = {
>> +	.is_generic			= true,
>> +	.pin_config_group_get		= lpi_config_get,
>> +	.pin_config_group_set		= lpi_config_set,
>> +};
>> +
>> +static int lpi_gpio_direction_input(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +	struct lpi_pinctrl *state = gpiochip_get_data(chip);
>> +	unsigned long config;
>> +
>> +	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
>> +
>> +	return lpi_config_set(state->ctrl, pin, &config, 1);
>> +}
>> +
>> +static int lpi_gpio_direction_output(struct gpio_chip *chip,
>> +				     unsigned int pin, int val)
>> +{
>> +	struct lpi_pinctrl *state = gpiochip_get_data(chip);
>> +	unsigned long config;
>> +
>> +	config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT, val);
>> +
>> +	return lpi_config_set(state->ctrl, pin, &config, 1);
>> +}
>> +
>> +static int lpi_gpio_get(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +	struct lpi_pinctrl *state = gpiochip_get_data(chip);
>> +	int value;
>> +
>> +	value = lpi_gpio_read(state, pin, LPI_GPIO_CFG_REG);
>> +	return value;
> 
> I must have missed this last round, gpio_get() should return the current
> value of the pin, i.e:
> 
Thanks I missed this one as well!
> 	return lpi_gpio_read(state, pin, LPI_GPIO_VALUE_REG) & BIT(0);
> 
>> +}
>> +
>> +static void lpi_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
>> +{
>> +	struct lpi_pinctrl *state = gpiochip_get_data(chip);
>> +	unsigned long config;
>> +
>> +	config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT, value);
>> +
>> +	lpi_config_set(state->ctrl, pin, &config, 1);
>> +}
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +#include <linux/seq_file.h>
>> +
>> +static unsigned int lpi_regval_to_drive(u32 val)
>> +{
>> +	return (val + 1) * 2;
>> +}
>> +
>> +static void lpi_gpio_dbg_show_one(struct seq_file *s,
>> +				  struct pinctrl_dev *pctldev,
>> +				  struct gpio_chip *chip,
>> +				  unsigned int offset,
>> +				  unsigned int gpio)
>> +{
>> +	struct lpi_pinctrl *state = gpiochip_get_data(chip);
>> +	struct pinctrl_pin_desc pindesc;
>> +	unsigned int func;
>> +	int is_out;
>> +	int drive;
>> +	int pull;
>> +	u32 ctl_reg;
>> +
>> +	static const char * const pulls[] = {
>> +		"no pull",
>> +		"pull down",
>> +		"keeper",
>> +		"pull up"
>> +	};
>> +
>> +	pctldev = pctldev ? : state->ctrl;
>> +	pindesc = pctldev->desc->pins[offset];
>> +	ctl_reg = lpi_gpio_read(state, offset, LPI_GPIO_CFG_REG);
>> +	is_out = ctl_reg & LPI_GPIO_OE_MASK;
>> +
>> +	func = (ctl_reg & LPI_GPIO_FUNCTION_MASK) >> LPI_GPIO_FUNCTION_SHIFT;
> 
> func = FIELD_GET(LPI_GPIO_FUNCTION_MASK, ctl_reg);
> 
>> +	drive = (ctl_reg & LPI_GPIO_OUT_STRENGTH_MASK) >>
>> +		 LPI_GPIO_OUT_STRENGTH_SHIFT;
> 
> drive = FIELD_GET(LPI_GPIO_OUT_STRENGTH_MASK, ctl_reg);
> 
>> +	pull = (ctl_reg & LPI_GPIO_PULL_MASK) >> LPI_GPIO_PULL_SHIFT;
> 
> pull = FIELD_GET(LPI_GPIO_PULL_MASK, ctl_reg);

Make sense, will do it!

> 
>> +
>> +	seq_printf(s, " %-8s: %-3s %d",
>> +		   pindesc.name, is_out ? "out" : "in", func);
> 
> This line doesn't need to be broken.


thanks,
srini
> 
> Regards,
> Bjorn
>