ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> пише: > > On Wed, Feb 12, 2025 at 05:20:04PM +0200, Svyatoslav Ryhel wrote: > > ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko > > <andriy.shevchenko@xxxxxxxxxxxxxxx> пише: > > > 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. > So it is not adopted by the Linux kernel. Lets return to this once it will be adopted. > > > > +#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 > >
