| Message ID | 20250212064657.5683-3-clamor95@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: add al3000a als support | expand |
On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote: > AL3000a is a simple I2C-based ambient light sensor, which is > closely related to AL3010 and AL3320a, but has significantly > different hardware configuration. (Note, the part of the below comments are applicable to your other series) ... > +/* > + * AL3000a - Dyna Image Ambient Light Sensor > + */ Can be on a single line. ... > +#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. > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> ... > +/* > + * This are pre-calculated lux values based on possible output > + * of sensor (range 0x00 - 0x3F) > + */ types.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). > +}; ... > +struct al3000a_data { > + struct i2c_client *client; struct regmap *map; will suffice, I believe, see below. > + struct regulator *vdd_supply; > +}; ... > +static const struct iio_chan_spec al3000a_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + } Leave trailing comma > +}; ... > +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr) > +{ > + struct device *dev = &data->client->dev; > + u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE; > + int ret; > + > + if (pwr) { > + ret = regulator_enable(data->vdd_supply); > + if (ret < 0) { > + dev_err(dev, "failed to enable vdd power supply\n"); > + return ret; With struct regmap *map in mind, the struct device *dev can be derived using the respective API. > + } > + } > + > + ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val); Why not using regmap I²C APIs? > + if (ret < 0) { > + dev_err(dev, "failed to write system register\n"); > + return ret; > + } > + > + if (!pwr) { > + ret = regulator_disable(data->vdd_supply); > + if (ret < 0) { > + dev_err(dev, "failed to disable vdd power supply\n"); > + return ret; > + } > + } > + > + return 0; > +} ... > +static int al3000a_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct al3000a_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = i2c_smbus_read_byte_data(data->client, > + AL3000A_REG_DATA); It may be a single line. There is a lot of room. > + if (ret < 0) > + return ret; > + > + *val = lux_table[ret & 0x3F]; I believe you want to define the size of that table and use it here. Also this needs a comment to explain the meaning of the ret >= 64 and when it may happen. > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + *val = 1; > + > + return IIO_VAL_INT; > + default: > + break; > + } > + > + return -EINVAL; Return directly from the default case. > +} ... > +static int al3000a_probe(struct i2c_client *client) > +{ > + struct al3000a_data *data; > + struct iio_dev *indio_dev; > + int ret; struct device *dev = &client->dev; will make the below lines shorter and easier to read. > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + data->client = client; > + > + data->vdd_supply = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(data->vdd_supply)) > + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply), > + "failed to get vdd regulator\n"); err.h > + indio_dev->info = &al3000a_info; > + indio_dev->name = AL3000A_DRV_NAME; > + indio_dev->channels = al3000a_channels; > + indio_dev->num_channels = ARRAY_SIZE(al3000a_channels); array_size.h > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = al3000a_init(data); > + if (ret < 0) > + return dev_err_probe(&client->dev, ret, > + "failed to init ALS\n"); Single line. > + ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off, > + data); Ditto. device.h > + if (ret < 0) > + return dev_err_probe(&client->dev, ret, > + "failed to add action\n"); > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} ... > +static const struct of_device_id al3000a_of_match[] = { mod_devicetable.h > + { .compatible = "dynaimage,al3000a" }, > + { /* sentinel */ } > +}; ... > +static struct i2c_driver al3000a_driver = { > + .driver = { > + .name = AL3000A_DRV_NAME, > + .of_match_table = al3000a_of_match, > + .pm = pm_sleep_ptr(&al3000a_pm_ops), pm.h > + }, > + .probe = al3000a_probe, > +};
ср, 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 is a simple I2C-based ambient light sensor, which is > > closely related to AL3010 and AL3320a, but has significantly > > different hardware configuration. > > (Note, the part of the below comments are applicable to your other series) > > ... > > > +/* > > + * 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. > ... > > > +#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 > > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > ... > > > +/* > > + * This are pre-calculated lux values based on possible output > > + * of sensor (range 0x00 - 0x3F) > > + */ > > types.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). > > +}; > > ... > > > +struct al3000a_data { > > + struct i2c_client *client; > > struct regmap *map; > > will suffice, I believe, see below. > > > > + struct regulator *vdd_supply; > > +}; > > ... > > > +static const struct iio_chan_spec al3000a_channels[] = { > > + { > > + .type = IIO_LIGHT, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE), > > > + } > > Leave trailing comma > > > +}; > > ... > > > +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr) > > +{ > > + struct device *dev = &data->client->dev; > > + u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE; > > + int ret; > > + > > + if (pwr) { > > + ret = regulator_enable(data->vdd_supply); > > + if (ret < 0) { > > + dev_err(dev, "failed to enable vdd power supply\n"); > > + return ret; > > With struct regmap *map in mind, the struct device *dev can be derived using > the respective API. > > > + } > > + } > > + > > + 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 I am not a full time linux contributor and may not be familiar with the recent rules. > > + if (ret < 0) { > > + dev_err(dev, "failed to write system register\n"); > > + return ret; > > + } > > + > > + if (!pwr) { > > + ret = regulator_disable(data->vdd_supply); > > + if (ret < 0) { > > + dev_err(dev, "failed to disable vdd power supply\n"); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > ... > > > +static int al3000a_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, int *val, > > + int *val2, long mask) > > +{ > > + struct al3000a_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > > + ret = i2c_smbus_read_byte_data(data->client, > > + AL3000A_REG_DATA); > > It may be a single line. There is a lot of room. > > > + if (ret < 0) > > + return ret; > > + > > + *val = lux_table[ret & 0x3F]; > > I believe you want to define the size of that table and use it here. > Also this needs a comment to explain the meaning of the ret >= 64 and > when it may happen. > > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1; > > + > > + return IIO_VAL_INT; > > > + default: > > + break; > > + } > > + > > + return -EINVAL; > > Return directly from the default case. > > > +} > > ... > > > +static int al3000a_probe(struct i2c_client *client) > > +{ > > + struct al3000a_data *data; > > + struct iio_dev *indio_dev; > > + int ret; > > struct device *dev = &client->dev; > > will make the below lines shorter and easier to read. > > > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + data = iio_priv(indio_dev); > > + i2c_set_clientdata(client, indio_dev); > > + data->client = client; > > + > > + data->vdd_supply = devm_regulator_get(&client->dev, "vdd"); > > + if (IS_ERR(data->vdd_supply)) > > + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply), > > + "failed to get vdd regulator\n"); > > err.h > > > + indio_dev->info = &al3000a_info; > > + indio_dev->name = AL3000A_DRV_NAME; > > + indio_dev->channels = al3000a_channels; > > + indio_dev->num_channels = ARRAY_SIZE(al3000a_channels); > > array_size.h > > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + > > + ret = al3000a_init(data); > > + if (ret < 0) > > > + return dev_err_probe(&client->dev, ret, > > + "failed to init ALS\n"); > > Single line. > > > + ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off, > > + data); > > Ditto. > > device.h > > > + if (ret < 0) > > + return dev_err_probe(&client->dev, ret, > > + "failed to add action\n"); > > + > > + return devm_iio_device_register(&client->dev, indio_dev); > > +} > > ... > > > +static const struct of_device_id al3000a_of_match[] = { > > mod_devicetable.h > > > + { .compatible = "dynaimage,al3000a" }, > > + { /* sentinel */ } > > +}; > > ... > > > +static struct i2c_driver al3000a_driver = { > > + .driver = { > > + .name = AL3000A_DRV_NAME, > > + .of_match_table = al3000a_of_match, > > > + .pm = pm_sleep_ptr(&al3000a_pm_ops), > > pm.h > > > + }, > > + .probe = al3000a_probe, > > +}; > > -- > With Best Regards, > Andy Shevchenko > > Everything else is valid, thank you. I will add fixes and try to switch to regmap.
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).
ср, 12 лют. 2025 р. о 18:10 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. > 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 > >
ср, 12 лют. 2025 р. о 18:10 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 > > 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.
On Wed, Feb 12, 2025 at 06:36:37PM +0200, Svyatoslav Ryhel wrote: > ср, 12 лют. 2025 р. о 18:10 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: ... > > > > > +#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. I understand you want to push your way, but here is the thing. This is a community of people and review is not something that comes for free. People, who are reviewing a code, want to make sure the code follows not only documented style, etc., but also common sense and the future maintenance. When a contributor comes and drops something into Linux Kernel project it adds a lot of work on maintainers' shoulders and other contributors who may be in progress of solving the other tasks. I specifically sent you a link where the tool and the principle is _explained_. So, it's not about the tool, it's about the whole project to become better and easier to maintain. You are a new guy in the development as you stated, so, please try to see how this all works. Of course, the last word here is Jonathan's as he is the maintainer of IIO, but I truly believe he will suggest you to follow my advice and not otherwise. > > > > > +#include <linux/iio/iio.h> > > > > > +#include <linux/iio/sysfs.h> ... I assume the non-commented parts you are satisfied with and they will be addressed as suggested. Thank you!
On Wed, Feb 12, 2025 at 07:28:01PM +0200, Svyatoslav Ryhel wrote: > ср, 12 лют. 2025 р. о 18:10 Andy Shevchenko > <andriy.shevchenko@linux.intel.com> пише: ... > 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. Sure, it was just a hint. I will check the other series as well when I have time and motivation.
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index e34e551eef3e..142f7f7ef0ec 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -43,6 +43,16 @@ config ADUX1020 To compile this driver as a module, choose M here: the module will be called adux1020. +config AL3000A + tristate "AL3000a ambient light sensor" + depends on I2C + help + Say Y here if you want to build a driver for the Dyna Image AL3000a + ambient light sensor. + + To compile this driver as a module, choose M here: the + module will be called al3000a. + config AL3010 tristate "AL3010 ambient light sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 11a4041b918a..17030a4cc340 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_ACPI_ALS) += acpi-als.o obj-$(CONFIG_ADJD_S311) += adjd_s311.o obj-$(CONFIG_ADUX1020) += adux1020.o +obj-$(CONFIG_AL3000A) += al3000a.o obj-$(CONFIG_AL3010) += al3010.o obj-$(CONFIG_AL3320A) += al3320a.o obj-$(CONFIG_APDS9300) += apds9300.o diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c new file mode 100644 index 000000000000..9e1f2ac6a933 --- /dev/null +++ b/drivers/iio/light/al3000a.c @@ -0,0 +1,214 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * AL3000a - Dyna Image Ambient Light Sensor + */ + +#include <linux/bitfield.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regulator/consumer.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define AL3000A_DRV_NAME "al3000a" +#define AL3000A_REG_SYSTEM 0x00 +#define AL3000A_REG_DATA 0x05 + +#define AL3000A_CONFIG_ENABLE 0x00 +#define AL3000A_CONFIG_DISABLE 0x0b +#define AL3000A_CONFIG_RESET 0x0f + +/* + * This are pre-calculated lux values based on possible output + * of sensor (range 0x00 - 0x3F) + */ +static const u32 lux_table[64] = { + 1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16, + 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 +}; + +struct al3000a_data { + struct i2c_client *client; + struct regulator *vdd_supply; +}; + +static const struct iio_chan_spec al3000a_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + } +}; + +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr) +{ + struct device *dev = &data->client->dev; + u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE; + int ret; + + if (pwr) { + ret = regulator_enable(data->vdd_supply); + if (ret < 0) { + dev_err(dev, "failed to enable vdd power supply\n"); + return ret; + } + } + + ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val); + if (ret < 0) { + dev_err(dev, "failed to write system register\n"); + return ret; + } + + if (!pwr) { + ret = regulator_disable(data->vdd_supply); + if (ret < 0) { + dev_err(dev, "failed to disable vdd power supply\n"); + return ret; + } + } + + return 0; +} + +static void al3000a_set_pwr_off(void *_data) +{ + struct al3000a_data *data = _data; + + al3000a_set_pwr(data, false); +} + +static int al3000a_init(struct al3000a_data *data) +{ + int ret; + + ret = al3000a_set_pwr(data, true); + if (ret < 0) + return ret; + + ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, + AL3000A_CONFIG_RESET); + if (ret < 0) + return ret; + + ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, + AL3000A_CONFIG_ENABLE); + if (ret < 0) + return ret; + + return 0; +} + +static int al3000a_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + struct al3000a_data *data = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = i2c_smbus_read_byte_data(data->client, + AL3000A_REG_DATA); + if (ret < 0) + return ret; + + *val = lux_table[ret & 0x3F]; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = 1; + + return IIO_VAL_INT; + default: + break; + } + + return -EINVAL; +} + +static const struct iio_info al3000a_info = { + .read_raw = al3000a_read_raw, +}; + +static int al3000a_probe(struct i2c_client *client) +{ + struct al3000a_data *data; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + i2c_set_clientdata(client, indio_dev); + data->client = client; + + data->vdd_supply = devm_regulator_get(&client->dev, "vdd"); + if (IS_ERR(data->vdd_supply)) + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply), + "failed to get vdd regulator\n"); + + indio_dev->info = &al3000a_info; + indio_dev->name = AL3000A_DRV_NAME; + indio_dev->channels = al3000a_channels; + indio_dev->num_channels = ARRAY_SIZE(al3000a_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = al3000a_init(data); + if (ret < 0) + return dev_err_probe(&client->dev, ret, + "failed to init ALS\n"); + + ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off, + data); + if (ret < 0) + return dev_err_probe(&client->dev, ret, + "failed to add action\n"); + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static int al3000a_suspend(struct device *dev) +{ + struct al3000a_data *data = iio_priv(dev_get_drvdata(dev)); + + return al3000a_set_pwr(data, false); +} + +static int al3000a_resume(struct device *dev) +{ + struct al3000a_data *data = iio_priv(dev_get_drvdata(dev)); + + return al3000a_set_pwr(data, true); +} + +static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume); + +static const struct of_device_id al3000a_of_match[] = { + { .compatible = "dynaimage,al3000a" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, al3000a_of_match); + +static struct i2c_driver al3000a_driver = { + .driver = { + .name = AL3000A_DRV_NAME, + .of_match_table = al3000a_of_match, + .pm = pm_sleep_ptr(&al3000a_pm_ops), + }, + .probe = al3000a_probe, +}; +module_i2c_driver(al3000a_driver); + +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>"); +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver"); +MODULE_LICENSE("GPL");
AL3000a is a simple I2C-based ambient light sensor, which is closely related to AL3010 and AL3320a, but has significantly different hardware configuration. Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> --- drivers/iio/light/Kconfig | 10 ++ drivers/iio/light/Makefile | 1 + drivers/iio/light/al3000a.c | 214 ++++++++++++++++++++++++++++++++++++ 3 files changed, 225 insertions(+) create mode 100644 drivers/iio/light/al3000a.c