Received-SPF: pass (google.com: domain of linux-kernel+bounces-122963-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 28 Mar 2024 13:37:20 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Lothar Rubusch <l.rubusch@gmail.com>
Cc: lars@metafoo.de, Michael.Hennerich@analog.com, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, eraretuya@gmail.com
Subject: Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
Message-ID: <20240328133720.7dfd46b0@jic23-huawei>
In-Reply-To: <20240327220320.15509-2-l.rubusch@gmail.com>
References: <20240327220320.15509-1-l.rubusch@gmail.com>
	<20240327220320.15509-2-l.rubusch@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 27 Mar 2024 22:03:14 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Replace write() data_format by regmap_update_bits(), because bus specific
> pre-configuration may have happened before on the same register. For
> further updates to the data_format register then bus pre-configuration
> needs to be masked out.
> 
> Remove the data_range field from the struct adxl345_data, because it is
> not used anymore.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8bd30a23e..35df5e372 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -37,7 +37,15 @@
>  #define ADXL345_POWER_CTL_MEASURE	BIT(3)
>  #define ADXL345_POWER_CTL_STANDBY	0x00
>  
> +#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0) /* Set the g range */
> +#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2) /* Left-justified (MSB) mode */
>  #define ADXL345_DATA_FORMAT_FULL_RES	BIT(3) /* Up to 13-bits resolution */
> +#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7) /* Enable a self test */
> +#define ADXL345_DATA_FORMAT_MSK		(ADXL345_DATA_FORMAT_RANGE | \
> +					 ADXL345_DATA_FORMAT_JUSTIFY |  \
> +					 ADXL345_DATA_FORMAT_FULL_RES | \
> +					 ADXL345_DATA_FORMAT_SELF_TEST)
This needs renaming.  It's not a mask of everything in the register, or
even just of everything related to format. 

Actually I'd just not have this definition.  Use the build up value
from all the submasks at the call site.  Then we are just making it clear
only a subset of fields are being cleared.

Jonathan

> +
>  #define ADXL345_DATA_FORMAT_2G		0
>  #define ADXL345_DATA_FORMAT_4G		1
>  #define ADXL345_DATA_FORMAT_8G		2
> @@ -48,7 +56,6 @@
>  struct adxl345_data {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> -	u8 data_range;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -218,15 +225,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
>  
>  	data = iio_priv(indio_dev);
>  	data->regmap = regmap;
> -	/* Enable full-resolution mode */
> -	data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
>  	data->info = device_get_match_data(dev);
>  	if (!data->info)
>  		return -ENODEV;
>  
> -	ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> -			   data->data_range);
> -	if (ret < 0)
> +	/* Enable full-resolution mode */
> +	ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> +				 ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> +	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to set data range\n");
>  
>  	indio_dev->name = data->info->name;