Received-SPF: pass (google.com: domain of linux-kernel+bounces-112684-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sun, 24 Mar 2024 13:48:35 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Lothar Rubusch <l.rubusch@gmail.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, 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 v2 1/3] iio: accel: adxl345: Update adxl345
Message-ID: <20240324134835.56bd216c@jic23-huawei>
In-Reply-To: <CAFXKEHYUVoiW6MthJokV2=AYcZnqw36jETL=-m6kpDCP_UNHAg@mail.gmail.com>
References: <20240322003713.6918-1-l.rubusch@gmail.com>
	<20240322003713.6918-2-l.rubusch@gmail.com>
	<51e3683f-be53-4bb7-a994-ffd05744a745@linaro.org>
	<CAFXKEHYUVoiW6MthJokV2=AYcZnqw36jETL=-m6kpDCP_UNHAg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sat, 23 Mar 2024 13:16:56 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> (...)
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345.h      |  44 +++++++++++-
> > >  drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> > >  drivers/iio/accel/adxl345_i2c.c  |  30 ++++----
> > >  drivers/iio/accel/adxl345_spi.c  |  28 ++++----
> > >  4 files changed, 134 insertions(+), 85 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 284bd387c..01493c999 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -8,6 +8,39 @@
> > >  #ifndef _ADXL345_H_
> > >  #define _ADXL345_H_
> > >
> > > +#include <linux/iio/iio.h>
> > > +
> > > +/* ADXL345 register definitions */
> > > +#define ADXL345_REG_DEVID            0x00
> > > +#define ADXL345_REG_OFSX             0x1E
> > > +#define ADXL345_REG_OFSY             0x1F
> > > +#define ADXL345_REG_OFSZ             0x20
> > > +#define ADXL345_REG_OFS_AXIS(index)  (ADXL345_REG_OFSX + (index))
> > > +#define ADXL345_REG_BW_RATE          0x2C
> > > +#define ADXL345_REG_POWER_CTL                0x2D
> > > +#define ADXL345_REG_DATA_FORMAT              0x31
> > > +#define ADXL345_REG_DATAX0           0x32
> > > +#define ADXL345_REG_DATAY0           0x34
> > > +#define ADXL345_REG_DATAZ0           0x36
> > > +#define ADXL345_REG_DATA_AXIS(index) \
> > > +     (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > > +
> > > +#define ADXL345_BW_RATE                      GENMASK(3, 0)
> > > +#define ADXL345_BASE_RATE_NANO_HZ    97656250LL
> > > +
> > > +#define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > > +#define ADXL345_POWER_CTL_STANDBY    0x00
> > > +
> > > +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > > +#define ADXL345_DATA_FORMAT_SPI         BIT(6) /* spi-3wire */
> > > +#define ADXL345_DATA_FORMAT_2G               0
> > > +#define ADXL345_DATA_FORMAT_4G               1
> > > +#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_DEVID                        0xE5
> > > +  
> (...)
> 
> I think I see your point. My patch has more noise and lacks a logic
> structure in proceding.
> I will resubmit, but may I ask one question in particular. I moved the
> entire list of register
> defines from the adxl345_core.c to the common adxl345.h.
> For setting spi-3wire with my approach, only two of those defines are
> needed. I think it is
> nicer for readability to keep the defines together, though, in a
> commonly shared header.
> Nevertheless most of the defines are just used locally in the .._core.c
> Should I move them for refactory?

Move them as a block (which you did).  It's confusing to have only a subset of
defines in one place.

> I feel there is no reason to move them. On the other hand I see many
> drivers keep them in a common header. Hence, is there a best practice
> which justifies moving them to a header?