[v2,10/17] iio: adc: axp20x_adc: Minor code cleanups

Message ID	20220607155324.118102-11-aidanmacdonald.0x0@gmail.com (mailing list archive)
State	Handled Elsewhere, archived
Headers	show Return-Path: <linux-pm-owner@kernel.org> From: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> To: linus.walleij@linaro.org, brgl@bgdev.pl, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, wens@csie.org, jic23@kernel.org, lee.jones@linaro.org, sre@kernel.org, broonie@kernel.org, gregkh@linuxfoundation.org, lgirdwood@gmail.com Cc: lars@metafoo.de, rafael@kernel.org, quic_gurus@quicinc.com, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, linux-pm@vger.kernel.org Subject: [PATCH v2 10/17] iio: adc: axp20x_adc: Minor code cleanups Date: Tue, 7 Jun 2022 16:53:17 +0100 Message-Id: <20220607155324.118102-11-aidanmacdonald.0x0@gmail.com> In-Reply-To: <20220607155324.118102-1-aidanmacdonald.0x0@gmail.com> References: <20220607155324.118102-1-aidanmacdonald.0x0@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Add support for AXP192 PMIC \| expand [v2,00/17] Add support for AXP192 PMIC [v2,01/17] regmap-irq: Use sub_irq_reg() to calculate unmask register address [v2,02/17] regmap-irq: Add get_irq_reg to support unusual register layouts [v2,03/17] dt-bindings: mfd: add bindings for AXP192 MFD device [v2,04/17] dt-bindings: iio: adc: axp209: Add AXP192 compatible [v2,05/17] dt-bindings: power: supply: axp20x: Add AXP192 compatible [v2,06/17] dt-bindings: gpio: Add AXP192 GPIO bindings [v2,07/17] dt-bindings: power: axp20x-battery: Add AXP192 compatible [v2,08/17] mfd: axp20x: Add support for AXP192 [v2,09/17] regulator: axp20x: Add support for AXP192 [v2,10/17] iio: adc: axp20x_adc: Minor code cleanups [v2,11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions [v2,12/17] iio: adc: axp20x_adc: Add support for AXP192 [v2,13/17] power: supply: axp20x_usb_power: Add support for AXP192 [v2,14/17] pinctrl: Add AXP192 pin control driver [v2,15/17] power: axp20x_battery: Add constant charge current table [v2,16/17] power: axp20x_battery: Support battery status without fuel gauge [v2,17/17] power: axp20x_battery: Add support for AXP192

Message ID

20220607155324.118102-11-aidanmacdonald.0x0@gmail.com (mailing list archive)

State

Handled Elsewhere, archived

Headers

From: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To: linus.walleij@linaro.org, brgl@bgdev.pl, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, wens@csie.org, jic23@kernel.org,
        lee.jones@linaro.org, sre@kernel.org, broonie@kernel.org,
        gregkh@linuxfoundation.org, lgirdwood@gmail.com
Cc: lars@metafoo.de, rafael@kernel.org, quic_gurus@quicinc.com,
        linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        linux-pm@vger.kernel.org
Subject: [PATCH v2 10/17] iio: adc: axp20x_adc: Minor code cleanups
Date: Tue,  7 Jun 2022 16:53:17 +0100
Message-Id: <20220607155324.118102-11-aidanmacdonald.0x0@gmail.com>
In-Reply-To: <20220607155324.118102-1-aidanmacdonald.0x0@gmail.com>
References: <20220607155324.118102-1-aidanmacdonald.0x0@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

Add support for AXP192 PMIC | expand

Commit Message

Aidan MacDonald June 7, 2022, 3:53 p.m. UTC

The code may be clearer if parameters are not re-purposed to hold
temporary results like register values, so introduce local variables
as necessary to avoid that. Also, use the common FIELD_PREP macro
instead of a hand-rolled version.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/iio/adc/axp20x_adc.c | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

Comments

Jonathan Cameron June 8, 2022, 1:22 p.m. UTC | #1

On Tue,  7 Jun 2022 16:53:17 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> The code may be clearer if parameters are not re-purposed to hold
> temporary results like register values, so introduce local variables
> as necessary to avoid that. Also, use the common FIELD_PREP macro
> instead of a hand-rolled version.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Hi Aidan,

Looks good.  One trivial further suggestion inline.

Also, am I fine picking up the IIO patches, or does the whole
set need to go in via mfd?

Thanks,

Jonathan

> ---
>  drivers/iio/adc/axp20x_adc.c | 61 +++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 53bf7d4899d2..9d5b1de24908 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -15,6 +15,7 @@
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/thermal.h>
> +#include <linux/bitfield.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/driver.h>
> @@ -22,20 +23,20 @@
>  #include <linux/mfd/axp20x.h>
>  
>  #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
> -
>  #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
> +
>  #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
>  
>  #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
>  #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
> -#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
> -#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>  
>  #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
> -#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
> -#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>  #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
> +
>  #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
> +
> +#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
> +#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>  #define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
>  #define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
>  #define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
> @@ -234,7 +235,7 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int size = 12;
> +	int ret, size;
>  
>  	/*
>  	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> @@ -246,10 +247,11 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	else
>  		size = 12;
>  
> -	*val = axp20x_read_variable_width(info->regmap, chan->address, size);
> -	if (*val < 0)
> -		return *val;
> +	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
> +	if (ret < 0)
> +		return ret;
>  
> +	*val = ret;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -257,11 +259,13 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int ret;
>  
> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (*val < 0)
> -		return *val;
> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> +	if (ret < 0)
> +		return ret;
>  
> +	*val = ret;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -269,11 +273,13 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int ret;
>  
> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (*val < 0)
> -		return *val;
> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> +	if (ret < 0)
> +		return ret;
>  
> +	*val = ret;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -443,27 +449,27 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>  				     int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	unsigned int regval;
>  	int ret;
>  
> -	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, val);
> +	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &regval);
>  	if (ret < 0)
>  		return ret;
>  
>  	switch (channel) {
>  	case AXP20X_GPIO0_V:
> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO0;
> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO0;

Maybe use FIELD_GET() here to be clear you are extracting that
field (even though we don't care about the shift).

Hopefully the compiler will be clever enough to remove the shift
anyway and using FIELD_GET() would act as slightly more 'documentation
in code'.



>  		break;
>  
>  	case AXP20X_GPIO1_V:
> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO1;
> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO1;
>  		break;
>  
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	*val = *val ? 700000 : 0;
> -
> +	*val = regval ? 700000 : 0;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -548,7 +554,7 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>  			    long mask)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	unsigned int reg, regval;
> +	unsigned int regmask, regval;
>  
>  	/*
>  	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
> @@ -560,25 +566,24 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>  	if (val != 0 && val != 700000)
>  		return -EINVAL;
>  
> -	val = val ? 1 : 0;
> +	regval = val ? 1 : 0;
>  
>  	switch (chan->channel) {
>  	case AXP20X_GPIO0_V:
> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO0;
> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(val);
> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO0;
> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO0, regval);
>  		break;
>  
>  	case AXP20X_GPIO1_V:
> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO1;
> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(val);
> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO1;
> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO1, regval);
>  		break;
>  
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, reg,
> -				  regval);
> +	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
>  }
>  
>  static const struct iio_info axp20x_adc_iio_info = {

Aidan MacDonald June 8, 2022, 10:58 p.m. UTC | #2

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Tue,  7 Jun 2022 16:53:17 +0100
> Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
>
>> The code may be clearer if parameters are not re-purposed to hold
>> temporary results like register values, so introduce local variables
>> as necessary to avoid that. Also, use the common FIELD_PREP macro
>> instead of a hand-rolled version.
>> 
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>
> Hi Aidan,
>
> Looks good.  One trivial further suggestion inline.
>
> Also, am I fine picking up the IIO patches, or does the whole
> set need to go in via mfd?
>
> Thanks,
>
> Jonathan
>

I think it has to go through mfd because of the GPIO2-3 ADC registers
which are added in the mfd patch. Cleanups are okay to pick up though.

>> ---
>>  drivers/iio/adc/axp20x_adc.c | 61 +++++++++++++++++++-----------------
>>  1 file changed, 33 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
>> index 53bf7d4899d2..9d5b1de24908 100644
>> --- a/drivers/iio/adc/axp20x_adc.c
>> +++ b/drivers/iio/adc/axp20x_adc.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/property.h>
>>  #include <linux/regmap.h>
>>  #include <linux/thermal.h>
>> +#include <linux/bitfield.h>
>>  
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/driver.h>
>> @@ -22,20 +23,20 @@
>>  #include <linux/mfd/axp20x.h>
>>  
>>  #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
>> -
>>  #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
>> +
>>  #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
>>  
>>  #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
>>  #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
>> -#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
>> -#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>>  
>>  #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
>> -#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
>> -#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>>  #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
>> +
>>  #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
>> +
>> +#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
>> +#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>>  #define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
>>  #define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
>>  #define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
>> @@ -234,7 +235,7 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>  			  struct iio_chan_spec const *chan, int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> -	int size = 12;
>> +	int ret, size;
>>  
>>  	/*
>>  	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
>> @@ -246,10 +247,11 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>  	else
>>  		size = 12;
>>  
>> -	*val = axp20x_read_variable_width(info->regmap, chan->address, size);
>> -	if (*val < 0)
>> -		return *val;
>> +	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> +	*val = ret;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -257,11 +259,13 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
>>  			  struct iio_chan_spec const *chan, int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	int ret;
>>  
>> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> -	if (*val < 0)
>> -		return *val;
>> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> +	*val = ret;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -269,11 +273,13 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
>>  			  struct iio_chan_spec const *chan, int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	int ret;
>>  
>> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> -	if (*val < 0)
>> -		return *val;
>> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> +	*val = ret;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -443,27 +449,27 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>>  				     int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	unsigned int regval;
>>  	int ret;
>>  
>> -	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, val);
>> +	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &regval);
>>  	if (ret < 0)
>>  		return ret;
>>  
>>  	switch (channel) {
>>  	case AXP20X_GPIO0_V:
>> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO0;
>> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO0;
>
> Maybe use FIELD_GET() here to be clear you are extracting that
> field (even though we don't care about the shift).
>
> Hopefully the compiler will be clever enough to remove the shift
> anyway and using FIELD_GET() would act as slightly more 'documentation
> in code'.
>

You're probably right; I erred on the side of premature optimization.
I'll go back to FIELD_GET in the v3 since I've got to resend anyway.

>>  		break;
>>  
>>  	case AXP20X_GPIO1_V:
>> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO1;
>> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO1;
>>  		break;
>>  
>>  	default:
>>  		return -EINVAL;
>>  	}
>>  
>> -	*val = *val ? 700000 : 0;
>> -
>> +	*val = regval ? 700000 : 0;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -548,7 +554,7 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>>  			    long mask)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> -	unsigned int reg, regval;
>> +	unsigned int regmask, regval;
>>  
>>  	/*
>>  	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
>> @@ -560,25 +566,24 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>>  	if (val != 0 && val != 700000)
>>  		return -EINVAL;
>>  
>> -	val = val ? 1 : 0;
>> +	regval = val ? 1 : 0;
>>  
>>  	switch (chan->channel) {
>>  	case AXP20X_GPIO0_V:
>> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO0;
>> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(val);
>> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO0;
>> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO0, regval);
>>  		break;
>>  
>>  	case AXP20X_GPIO1_V:
>> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO1;
>> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(val);
>> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO1;
>> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO1, regval);
>>  		break;
>>  
>>  	default:
>>  		return -EINVAL;
>>  	}
>>  
>> -	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, reg,
>> -				  regval);
>> +	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
>>  }
>>  
>>  static const struct iio_info axp20x_adc_iio_info = {

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index 53bf7d4899d2..9d5b1de24908 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -15,6 +15,7 @@ 
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/thermal.h>
+#include <linux/bitfield.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/driver.h>
@@ -22,20 +23,20 @@ 
 #include <linux/mfd/axp20x.h>
 
 #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
-
 #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
+
 #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
 
 #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
 #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
-#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
-#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
 
 #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
-#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
-#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
 #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
+
 #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
+
+#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
+#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
 #define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
 #define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
 #define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
@@ -234,7 +235,7 @@  static int axp20x_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
-	int size = 12;
+	int ret, size;
 
 	/*
 	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
@@ -246,10 +247,11 @@  static int axp20x_adc_raw(struct iio_dev *indio_dev,
 	else
 		size = 12;
 
-	*val = axp20x_read_variable_width(info->regmap, chan->address, size);
-	if (*val < 0)
-		return *val;
+	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
+	if (ret < 0)
+		return ret;
 
+	*val = ret;
 	return IIO_VAL_INT;
 }
 
@@ -257,11 +259,13 @@  static int axp22x_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	int ret;
 
-	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
-	if (*val < 0)
-		return *val;
+	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
+	if (ret < 0)
+		return ret;
 
+	*val = ret;
 	return IIO_VAL_INT;
 }
 
@@ -269,11 +273,13 @@  static int axp813_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	int ret;
 
-	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
-	if (*val < 0)
-		return *val;
+	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
+	if (ret < 0)
+		return ret;
 
+	*val = ret;
 	return IIO_VAL_INT;
 }
 
@@ -443,27 +449,27 @@  static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
 				     int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	unsigned int regval;
 	int ret;
 
-	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, val);
+	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &regval);
 	if (ret < 0)
 		return ret;
 
 	switch (channel) {
 	case AXP20X_GPIO0_V:
-		*val &= AXP20X_GPIO10_IN_RANGE_GPIO0;
+		regval &= AXP20X_GPIO10_IN_RANGE_GPIO0;
 		break;
 
 	case AXP20X_GPIO1_V:
-		*val &= AXP20X_GPIO10_IN_RANGE_GPIO1;
+		regval &= AXP20X_GPIO10_IN_RANGE_GPIO1;
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	*val = *val ? 700000 : 0;
-
+	*val = regval ? 700000 : 0;
 	return IIO_VAL_INT;
 }
 
@@ -548,7 +554,7 @@  static int axp20x_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
-	unsigned int reg, regval;
+	unsigned int regmask, regval;
 
 	/*
 	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
@@ -560,25 +566,24 @@  static int axp20x_write_raw(struct iio_dev *indio_dev,
 	if (val != 0 && val != 700000)
 		return -EINVAL;
 
-	val = val ? 1 : 0;
+	regval = val ? 1 : 0;
 
 	switch (chan->channel) {
 	case AXP20X_GPIO0_V:
-		reg = AXP20X_GPIO10_IN_RANGE_GPIO0;
-		regval = AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(val);
+		regmask = AXP20X_GPIO10_IN_RANGE_GPIO0;
+		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO0, regval);
 		break;
 
 	case AXP20X_GPIO1_V:
-		reg = AXP20X_GPIO10_IN_RANGE_GPIO1;
-		regval = AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(val);
+		regmask = AXP20X_GPIO10_IN_RANGE_GPIO1;
+		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO1, regval);
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, reg,
-				  regval);
+	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
 }
 
 static const struct iio_info axp20x_adc_iio_info = {

[v2,10/17] iio: adc: axp20x_adc: Minor code cleanups

Commit Message

Comments

Patch