Re: [PATCH v1 2/3] iio: light: Add support for AL3000a illuminance sensor

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote:
> ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
> <andriy.shevchenko@linux.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