| 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: <aDTMSwhodZQLzZ4q@smile.fi.intel.com>
Date: Mon, 26 May 2025 23:17:15 +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,
arthur.becker@...tec.com, perdaniel.olsson@...s.com,
mgonellabolduc@...onoff.com, muditsharma.info@...il.com,
clamor95@...il.com, emil.gedenryd@...s.com,
devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] iio: light: add support for veml6046x00 RGBIR
color sensor
On Mon, May 26, 2025 at 10:50:40AM +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.
...
+ array_size.h
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
+ dev_printk.h
+ err.h
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
You also want mod_devicetable.h.
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
+ blank line
+ asm/byteorder.h
...
> +#define VEML6046X00_REG_DATA(ch) (VEML6046X00_REG_R_L + (ch))
Dead code / leftover. Even if you want to keep it for documentation purposes
it's not fine as it uses undefined constant.
...
> +/**
> + * struct veml6046x00_data - Private data of driver.
> + * @regmap: Regmap definition of sensor.
> + * @trig: Industrial-IO trigger.
> + * @rf: Regmap field of configuration.
> + */
> +struct veml6046x00_data {
> + struct regmap *regmap;
> + struct iio_trigger *trig;
> + struct veml6046x00_rf rf;
Does pahole agree on the choice of the layout?
> +};
...
> +/**
> + * struct veml6046x00_gain_pd - Translation of gain and photodiode size (PD).
> + * @gain_sen: Gain used in the sensor as described in the datasheet of the
> + * sensor
> + * @pd: Photodiode size in the sensor
> + *
> + * This is the translation table from the gain used in the driver (and also used
> + * by the userspace interface in sysfs) to the gain and PD used in the sensor
> + * hardware.
> + *
> + * There are six gain values visible to the user (0.25 .. 2) which translate to
> + * two different gains in the sensor hardware (x0.5 .. x2) and two PD (1/2 and
> + * 2/2). Theoretical are there eight combinations, but gain values 0.5 and 1 are
> + * doubled and therefore the combination with the larger PD (2/2) is taken as
> + * more photodiode cells are supposed to deliver a more precise result.
> + */
> +struct veml6046x00_gain_pd {
> + int gain_sen;
> + int pd;
Why are they signed? I haven't found justification in the above description
(did I missed it?).
> +};
...
> +static int veml6046x00_get_gain_idx(struct veml6046x00_data *data)
> +{
> + int ret, i;
Why is 'i' signed? Same to other similar cases (more than 1).
> + unsigned int reg, reg_gain, reg_pd;
> +
> + ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, ®);
> + if (ret)
> + return ret;
> +
> + reg_gain = FIELD_GET(VEML6046X00_CONF1_GAIN, reg);
> + reg_pd = reg & VEML6046X00_CONF1_PD_D2;
> +
> + for (i = 0; i < ARRAY_SIZE(veml6046x00_gain_pd); i++) {
> + if ((veml6046x00_gain_pd[i].gain_sen == reg_gain) &&
> + (veml6046x00_gain_pd[i].pd == reg_pd)) {
> + return i;
> + }
> + }
> +
> + return -EINVAL;
> +}
...
> +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
Can usecs be negative? What would be the meaning?
> +{
> + struct veml6046x00_data *data = iio_priv(iio);
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret, i, cnt = 2;
> + u8 reg[2];
> +
> + for (i = 0; i < cnt; i++) {
> + /*
> + * Note from the vendor, but not explicitly in the datasheet: we
> + * should always read both registers together
Missing period at the end.
> + */
> + ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_INT,
> + ®, sizeof(reg));
> + if (ret) {
> + dev_err(dev,
> + "Failed to read interrupt register %d\n", ret);
> + return -EIO;
> + }
> +
> + if (reg[1] & VEML6046X00_INT_DRDY)
> + return 1;
> + if (i < (cnt - 1))
> + fsleep(usecs);
This for-loop for cnt=2 seems an overkill. Why not to have a helper and call it twice?
call_helper();
fsleep();
call_helper();
?
> + }
> +
> + return 0;
> +}
...
> + ret = veml6046x00_wait_data_available(iio, it_usec * 4);
> + if (ret < 0)
> + return ret;
> + else if (ret == 0)
Redundant 'else'
> + return -EAGAIN;
...
> + ret = regmap_field_write(data->rf.trig, 0);
> + if (ret) {
> + dev_err(dev, "Failed to set trigger %d\n", ret);
> + return ret;
> + }
> + return pm_runtime_resume_and_get(dev);
Shouldn't you unroll the above settings somehow? If it's okay, perhaps a
comment here?
...
> +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");
Can be made one line.
> + iio = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!iio)
> + return -ENOMEM;
> +
> + data = iio_priv(iio);
> + i2c_set_clientdata(i2c, iio);
Hmm... I do not see how it's being used. Can you elaborate?
> + 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