Message-ID: <CAPVz0n0YFXUugt1E5siZSYbCBcp6LdNv136eTWGQTbLAXE4pxQ@mail.gmail.com>
Date: Wed, 12 Feb 2025 19:28:01 +0200
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Thierry Reding <thierry.reding@...il.com>, Jonathan Hunter <jonathanh@...dia.com>, 
	Javier Carrasco <javier.carrasco.cruz@...il.com>, Matti Vaittinen <mazziesaccount@...il.com>, 
	Emil Gedenryd <emil.gedenryd@...s.com>, Arthur Becker <arthur.becker@...tec.com>, 
	Mudit Sharma <muditsharma.info@...il.com>, Per-Daniel Olsson <perdaniel.olsson@...s.com>, 
	Subhajit Ghosh <subhajit.ghosh@...aklogic.com>, Ivan Orlov <ivan.orlov0322@...il.com>, 
	David Heidelberg <david@...t.cz>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1 2/3] iio: light: Add support for AL3000a illuminance sensor

ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote:
> > ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
> > <andriy.shevchenko@...ux.intel.com> пише:
> > > On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:
>
> ...
>
> > > > +/*
> > > > + * AL3000a - Dyna Image Ambient Light Sensor
> > > > + */
> > >
> > > Can be on a single line.
> >
> > Patch checking script did not catch this as warning or style issue. If
> > such commenting is discouraged than please add this to patch checking
> > script. Comments are stripped on compilation anyway, they do not
> > affect size.
>
> First of all, it uses verb 'can' for a reason (it's not anyhow big deal).
> Second, not all stuff should be documented or scripted, we called it
> a "common sense". The common sense rule in the code is: "Do not introduce
> lines that are not needed or do not add a value". I see these 3 LoCs can
> be replaced without any downsides to 1 LoC and make it even more readable,
> less consumable of the resources, and more informative as opening the
> first page in the editor will give me more code than mostly unrelated
> comments.
>
> ...
>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of.h>
> > >
> > > No of*.h in the new code, please.
> > >
> > > > +#include <linux/regulator/consumer.h>
> > >
> > > Too small headers to be included. You use much more.
> >
> > Is there a check which determines the amount of headers I must include
> > and which headers are mandatory to be included and which are forbidden
> > to inclusion. Maybe at least a list? Thanks
>
> No check, there is IWYU principle.
>
> https://include-what-you-use.org/
>
> The tool is not (yet?) suitable for the Linux kernel project and Jonathan,
> who is the maintainer of IIO code, had even tried it for real some time ago.
>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
>
> ...
>
> > > > +static const u32 lux_table[64] = {
> > >
> > > I think you don't need 64 to be there, but okay, I understand the intention.
> > >
> > > > +     1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,
> > >
> > > For the better readability and maintenance put pow-of-2 amount of values per
> > > line, like 8, and add the respective comment:
> > >
> > >         1, 1, 1, 2, 2, 2, 3, 4,                                 /*  0 -  7 */
> > >         4, 5, 6, 7, 9, 11, 13, 16,                              /*  8 - 15 */
> > >
> > > > +     19, 22, 27, 32, 39, 46, 56, 67, 80, 96, 116, 139,
> > > > +     167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> > > > +     1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728, 4475,
> > > > +     5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> > > > +     23180, 27828, 33408, 40107, 48148, 57803, 69393,
> > > > +     83306, 100000
> > >
> > > Leave trailing comma, it's not a terminated list generally speaking
> > > (in the future it might grow).
> >
> > No, this list will not grow since the bit field seems to be 0x3f
> > (datasheet is not available, code is adaptation of downstream driver).
>
> You never know. Sometimes driver is getting reused to support other compatible
> HW. Telling you from the experience.
>
> > > > +};
>
> ...
>
> > > > +     ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);
> > >
> > > Why not using regmap I涎 APIs?
> >
> > This adaptation was written quite a long time ago, patch check did not
> > complained about using of i2c smbus. Is use of regmap mandatory now?
> > Is it somewhere specified? Thanks
>
> It adds a value to the code (in particular debugfs for free and
> many nice helper APIs). It's recommended and many maintainers would like
> to have it. It's rare that some of the generic framework or library committed
> into the kernel just left to become rotten there.
>
> > I am not a full time linux contributor and may not be familiar with
> > the recent rules.
>
> Many are not the rules so far, but recommendations and/or preferences by
> certain maintainer(s).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

I will apply all your suggestions. Thank you.

Regards other patch series, may you please contain all advice inside
patch series since it is quite hard to track between them. For future
patches, I will try to avoid listed issues. Thank you.

Powered by blists - more mailing lists