Messages in this thread | | | From | David Lechner <> | Date | Wed, 3 Apr 2024 10:55:11 -0500 | Subject | Re: [PATCH 3/6] iio: adc: ad7173: refactor channel configuration parsing |
| |
On Wed, Apr 3, 2024 at 5:01 AM Ceclan, Dumitru <mitrutzceclan@gmailcom> wrote: > > On 01/04/2024 22:39, David Lechner wrote: > > On Mon, Apr 1, 2024 at 10:10 AM 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 == 0) > > + return dev_err_probe(dev, -ENODATA, "No channels specified\n"); > > + indio_dev->num_channels = num_channels; > > + st->num_channels = 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_config() > > 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>
| |