Received-SPF: pass (google.com: domain of linux-kernel+bounces-112824-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240323122030.21800-1-l.rubusch@gmail.com> <20240324133941.26814432@jic23-huawei>
In-Reply-To: <20240324133941.26814432@jic23-huawei>
From: Lothar Rubusch <l.rubusch@gmail.com>
Date: Sun, 24 Mar 2024 20:20:21 +0100
Message-ID: <CAFXKEHbvdQoqyirUC8ueihfTcCs7m5CViP27S1sNDA0VerUVYQ@mail.gmail.com>
Subject: Re: [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature
To: Jonathan Cameron <jic23@kernel.org>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 24, 2024 at 2:39=E2=80=AFPM Jonathan Cameron <jic23@kernel.org>=
 wrote:
>
> On Sat, 23 Mar 2024 12:20:24 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Pass a function setup() as pointer from SPI/I2C specific modules
> > to the core module. Implement setup() to pass the spi-3wire bus
> > option, if declared in the device-tree.
> >
> > In the core module, then update data_format register
> > configuration bits instead of overwriting it. The changes allow
> > to remove a data_range field, remove I2C and SPI redundant info
> > instances and replace them by a common info array instance.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> That patch break up seems reasonable (one minor request for a split
> in the relevant patch), but normal convention would be do do
> refactoring first, then functionality at the end. Also removal stuff
> and group, before adding things.
>
> So roughly speaking reorder as
>
> >   iio: accel: adxl345: Make data_format obsolete
> >   iio: accel: adxl345: Remove single info instances
> >   iio: accel: adxl345: Group bus configuration
> >   dt-bindings: iio: accel: adxl345: Add spi-3wire
> >   iio: accel: adxl345: Pass function pointer to core
> >   iio: accel: adxl345: Add the spi-3wire
>

Ok. If I split "Group bus configuration" into the grouping of the
indio_dev in the probe() and adding a comment to the core's probe(), I
will end up with something like this:

$ git log --oneline --reverse
 iio: accel: adxl345: Make data_range obsolete
 iio: accel: adxl345: Group bus configuration
 iio: accel: adxl345: Move defines to header <--- new
 dt-bindings: iio: accel: adxl345: Add spi-3wire
 iio: accel: adxl345: Pass function pointer to core
 iio: accel: adxl345: Add comment to probe  <--- new after split
 iio: accel: adxl345: Add spi-3wire option

I feel I have to add the comment after adding the passed function
pointer. Bascially I liked to add a comment mentioning especially the
new function pointer there. So, although being a comment, the commit
will be in this "high" position. Is this ok, or am I doing something
wrong? Should I split into setting the comment first, then inside
"Pass function pointer.." also update the comment?