Received-SPF: pass (google.com: domain of linux-kernel+bounces-117948-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 25 Mar 2024 20:37:46 +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 v4 7/7] iio: accel: adxl345: Add spi-3wire option
Message-ID: <20240325203746.01e834e9@jic23-huawei>
In-Reply-To: <20240325153356.46112-8-l.rubusch@gmail.com>
References: <20240325153356.46112-1-l.rubusch@gmail.com>
	<20240325153356.46112-8-l.rubusch@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 25 Mar 2024 15:33:56 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a setup function implementation to the spi module to enable
> spi-3wire as option when specified in the device-tree.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h     |  1 +
>  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 620a2e0f0..55a72ca38 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -36,6 +36,7 @@
>  #define ADXL345_DATA_FORMAT_8G		2
>  #define ADXL345_DATA_FORMAT_16G		3
>  #define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
If you argue in favour of keeping the MSK as negation of this bit, then
reorder these two and define FORMAT_MSK in terms of SPI_3WIRE.

Note the name of FORMAT_MASK is also an issue now you actually write the 3wire bit.
So I'd get rid of that define in favour of positive defines of all the fields that
make it up used inline as it's only accessed in one place anyway.
That avoids the need for a mask of 'not' something.

>  
>  #define ADXL345_DEVID			0xE5
>  
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 1c0513bd3..f145d5c1d 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
>  	.read_flag_mask = BIT(7) | BIT(6),
>  };
>  
> +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> +{
> +	struct spi_device *spi = container_of(dev, struct spi_device, dev);
> +
> +	if (spi->mode & SPI_3WIRE)
> +		return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> +				    ADXL345_DATA_FORMAT_SPI_3WIRE); 
> +	return 0;
> +}
> +
>  static int adxl345_spi_probe(struct spi_device *spi)
>  {
>  	struct regmap *regmap;
> @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
>  	if (IS_ERR(regmap))
>  		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
>  
> -	return adxl345_core_probe(&spi->dev, regmap, NULL);
> +	return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
>  }
>  
>  static const struct adxl345_chip_info adxl345_spi_info = {