Received-SPF: pass (google.com: domain of linux-kernel+bounces-130144-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240401-ad4111-v1-0-34618a9cc502@analog.com> <20240401-ad4111-v1-3-34618a9cc502@analog.com>
 <CAMknhBHo3xkkZymVfvauL7nBPn9+c1ZUoPwyk4mJO4syRJEhiQ@mail.gmail.com> <1d777161-7d86-4d45-91bc-c7653504b890@gmail.com>
In-Reply-To: <1d777161-7d86-4d45-91bc-c7653504b890@gmail.com>
From: David Lechner <dlechner@baylibre.com>
Date: Wed, 3 Apr 2024 10:55:11 -0500
Message-ID: <CAMknhBFohF_0NUDeweO=dWdStREZrphpvTzke7F6Ngr-7rXriQ@mail.gmail.com>
Subject: Re: [PATCH 3/6] iio: adc: ad7173: refactor channel configuration parsing
To: "Ceclan, Dumitru" <mitrutzceclan@gmail.com>
Cc: dumitru.ceclan@analog.com, Lars-Peter Clausen <lars@metafoo.de>, 
	Michael Hennerich <Michael.Hennerich@analog.com>, Jonathan Cameron <jic23@kernel.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 3, 2024 at 5:01=E2=80=AFAM Ceclan, Dumitru <mitrutzceclan@gmail=
com> wrote:
>
> On 01/04/2024 22:39, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10=E2=80=AFAM Dumitru Ceclan via B4 Relay
> > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >>
> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>
> >> Move configurations regarding number of channels from
> >> *_fw_parse_device_config to *_fw_parse_channel_config.
> >>
> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >> ---
> >
> > Commit messages need to explain _why_ the change is being made [1]. It
> > is not obvious to me why this needs to be moved.
> >
> > [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.=
html#the-canonical-patch-format
>
>
> Jonathan Cameron:
>
> "
> > +     if (num_channels =3D=3D 0)
> > +             return dev_err_probe(dev, -ENODATA, "No channels specifie=
d\n");
> > +     indio_dev->num_channels =3D num_channels;
> > +     st->num_channels =3D num_channels;
>
> I'm not seeing benefit of duplication here really and logically it feels =
like
> a lot of this last chunk would sit better in ad7173_fw_parse_channel_conf=
ig()
>
> Perhaps that's a job for a future tidying up patch.
> "
> https://lore.kernel.org/all/20240303162148.3ad91aa2@jic23-huawei/
>

Thanks.

A Link: and Suggested-by: in the commit message with this info would
be a reasonable way to communicate this.

I looks like this is also adding an additional check "  if
(num_channels > st->info->num_channels)" in addition to moving
existing code. It would be helpful to have the reason for this in the
commit message as well.

With the suggested additions to the commit message...
Reviewed-by: David Lechner <dlechner@baylibre.com>