| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aB0MycGnGRxJT7Pd@smile.fi.intel.com>
Date: Thu, 8 May 2025 22:58:01 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Andreas Klinger <ak@...klinger.de>
Cc: jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, lars@...afoo.de,
javier.carrasco.cruz@...il.com, mazziesaccount@...il.com,
muditsharma.info@...il.com, perdaniel.olsson@...s.com,
emil.gedenryd@...s.com, mgonellabolduc@...onoff.com,
arthur.becker@...tec.com, clamor95@...il.com,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/3] iio: light: add support for veml6046x00 RGBIR
color sensor
On Mon, May 05, 2025 at 10:23:12PM +0200, Andreas Klinger wrote:
> Add Vishay VEML6046X00 high accuracy RGBIR color sensor.
>
> This sensor provides three colour (red, green and blue) as well as one
> infrared (IR) channel through I2C.
>
> Support direct and buffered mode.
>
> An optional interrupt for signaling green colour threshold underflow or
> overflow is not supported so far.
...
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +#include <linux/pm_runtime.h>
+ blank line
and keep each group sorted.
There are missing inclusions as well.
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +/* Bit masks for specific functionality */
> +#define VEML6046X00_CONF0_ON_0 BIT(0)
> +#define VEML6046X00_CONF0_INT BIT(1)
> +#define VEML6046X00_CONF0_AF_TRIG BIT(2)
> +#define VEML6046X00_CONF0_AF BIT(3)
> +#define VEML6046X00_CONF0_IT GENMASK(6, 4)
> +#define VEML6046X00_CONF1_CAL BIT(0)
> +#define VEML6046X00_CONF1_PERS GENMASK(2, 1)
> +#define VEML6046X00_CONF1_GAIN GENMASK(4, 3)
> +#define VEML6046X00_CONF1_PD_D2 BIT(6)
> +#define VEML6046X00_CONF1_ON_1 BIT(7)
> +#define VEML6046X00_INT_TH_H BIT(1)
> +#define VEML6046X00_INT_TH_L BIT(2)
> +#define VEML6046X00_INT_DRDY BIT(3)
+ bits.h
...
> +#define VEML6046X00_INT_MASK (VEML6046X00_INT_TH_H | \
> + VEML6046X00_INT_TH_L | VEML6046X00_INT_DRDY)
Better formatting is
#define VEML6046X00_INT_MASK \
(VEML6046X00_INT_TH_H | VEML6046X00_INT_TH_L | VEML6046X00_INT_DRDY)
> +/* Autosuspend delay */
> +#define VEML6046X00_AUTOSUSPEND_MS 3000
(3 * MSEC_PER_SEC)
(will require time.h to be included)
...
> +struct veml6046x00_data {
> + struct device *dev;
> + struct regmap *regmap;
Isn't dev the same as in regmap?
Why to have both?
> + struct iio_trigger *trig;
> + struct veml6046x00_rf rf;
> +};
...
> +struct veml6046x00_scan_buf {
> + __le16 chans[4];
> + aligned_s64 timestamp;
> +};
This is only used in one function, move it there like other drivers do.
...
> + * Columns:
> + * x0.25 x0.33 x0.5 x0.66 x1 x2
> + * Rows:
> + * 3.125 6.25 12.5 25 50 100 200 400ms
Better to have these as comments
...
> +static const u32 veml6046x00_it_gains[][6][2] = {
Also indentation is wrong.
> +{{5, 376000}, {4, 72700}, {2, 688000}, {2, 36400}, {1, 344000}, {0, 672000}},
/* 3.125 */
{
{ 5, 376000 }, /* 0.25 */
{ 4, 72700 },
...
},
> +{{2, 688000}, {2, 36350}, {1, 344000}, {1, 18200}, {0, 672000}, {0, 336000}},
> +{{1, 344000}, {1, 18175}, {0, 672000}, {0, 509100}, {0, 336000}, {0, 168000}},
> +{{0, 672000}, {0, 509087}, {0, 336000}, {0, 254550}, {0, 168000}, {0, 84000}},
> +{{0, 336000}, {0, 254543}, {0, 168000}, {0, 127275}, {0, 84000}, {0, 42000}},
> +{{0, 168000}, {0, 127271}, {0, 84000}, {0, 63637}, {0, 42000}, {0, 21000}},
> +{{0, 84000}, {0, 63635}, {0, 42000}, {0, 31818}, {0, 21000}, {0, 10500}},
> +{{0, 42000}, {0, 31817}, {0, 21000}, {0, 15909}, {0, 10500}, {0, 5250}},
> +};
Yes, it will be much longer, but much easier to read and check.
...
> +static int veml6046x00_power_on(struct veml6046x00_data *data)
> +{
> + int ret;
> +
> + ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
> + VEML6046X00_CONF0_ON_0);
Wrong indentation. Same applies to more places in the code.
> + if (ret) {
> + dev_err(data->dev, "Failed to set bit for power on %d\n", ret);
> + return ret;
> + }
> +
> + return regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF1,
> + VEML6046X00_CONF1_ON_1);
> +}
...
> + /* register value is identical with index of array */
> + if ((reg >= 0) && (reg < ARRAY_SIZE(veml6046x00_it)))
> + return reg;
> +
> + return -EINVAL;
Invert the conditional to check for errors.
...
> +static int veml6046x00_get_it_usec(struct veml6046x00_data *data, int *it_usec)
> +{
> + int ret, reg;
> +
> + ret = regmap_field_read(data->rf.it, ®);
> + if (ret)
> + return ret;
> +
> + if ((reg >= 0) && (reg < ARRAY_SIZE(veml6046x00_it)))
> + *it_usec = veml6046x00_it[reg][1];
> + else
Redundant. (See above how to fix)
> + return -EINVAL;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
...
> +static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + int i;
Why signed?
> + for (i = 0; i < ARRAY_SIZE(veml6046x00_it); i++) {
> + if ((veml6046x00_it[i][0] == val) &&
> + (veml6046x00_it[i][1] == val2))
> + return regmap_field_write(data->rf.it, i);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int veml6046x00_get_val_gain_idx(struct veml6046x00_data *data, int val,
> + int val2)
> +{
> + int i;
Ditto. And so on...
> + int it_idx;
> +
> + it_idx = veml6046x00_get_it_index(data);
> + if (it_idx < 0)
> + return it_idx;
> +
> + for (i = 0; i < ARRAY_SIZE(veml6046x00_it_gains[it_idx]); i++) {
> + if ((veml6046x00_it_gains[it_idx][i][0] == val) &&
> + (veml6046x00_it_gains[it_idx][i][1] == val2)) {
> + return i;
> + }
> + }
> +
> + return -EINVAL;
> +}
...
> +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + int ret, reg;
> + int cnt = 2;
> + int i;
> +
> + for (i = 0; i < cnt; i++) {
> + ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, ®);
> + if (ret) {
> + dev_err(data->dev,
> + "Failed to read interrupt register %d\n", ret);
> + return -EIO;
> + }
> +
> + if (reg & VEML6046X00_INT_DRDY)
> + return 1;
> + if (i < cnt)
Always true, no?
> + fsleep(usecs);
> + }
> +
> + return 0;
> +}
...
> + /* integration time + 10 % to ensure completion */
> + fsleep(it_usec + (it_usec / 10));
Unneeded parentheses.
...
> + ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> + if (ret == 1) {
> + dev_dbg(data->dev, "data ready\n");
> + } else {
Redundant 'else'.
> + dev_warn(data->dev, "no data ready ret: %d\n", ret);
> + goto no_data;
> + }
...
> +static irqreturn_t veml6046x00_trig_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *iio = pf->indio_dev;
> + struct veml6046x00_data *data = iio_priv(iio);
> + int ret;
> + struct veml6046x00_scan_buf scan;
> +
> + memset(&scan, 0, sizeof(scan));
> +
> + ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R_L, &scan.chans,
> + sizeof(scan.chans));
Wrong indentation.
> + if (ret)
> + goto done;
> +
> + iio_push_to_buffers_with_timestamp(iio, &scan, iio_get_time_ns(iio));
> +
> +done:
> + iio_trigger_notify_done(iio->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> + int part_id, ret;
> + __le16 reg;
> +
> + ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, ®, sizeof(reg));
Remove _L from the definitions for the registers that you access with bulk IO.
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Failed to read ID\n");
> +
> + part_id = le16_to_cpu(reg);
> + if (part_id != 0x0001)
> + dev_info(data->dev, "Unknown ID %#02x\n", part_id);
> +
> + return 0;
> +}
...
> +static int veml6046x00_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct veml6046x00_data *data;
> + struct iio_dev *iio;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to set regmap\n");
One line.
> + iio = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!iio)
> + return -ENOMEM;
> +
> + data = iio_priv(iio);
> + i2c_set_clientdata(i2c, iio);
> + data->dev = dev;
> + data->regmap = regmap;
> +
> + ret = veml6046x00_regfield_init(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to init regfield\n");
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> +
> + /* bring device in a known state and switch device on */
> + ret = veml6046x00_setup_device(iio);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to add shut down action\n");
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> + pm_runtime_get_noresume(dev);
> + pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
> + pm_runtime_use_autosuspend(dev);
> +
> + ret = veml6046x00_validate_part_id(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to validate device ID\n");
> +
> + iio->name = "veml6046x00";
> + iio->channels = veml6046x00_channels;
> + iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
> + iio->modes = INDIO_DIRECT_MODE;
> +
> + iio->info = &veml6046x00_info_no_irq;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> + veml6046x00_trig_handler,
> + &veml6046x00_buffer_setup_ops);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to register triggered buffer");
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + ret = devm_iio_device_register(dev, iio);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register iio device");
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists