| 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: <20230425164522.sljcniui5ox5yx3l@intel.intel>
Date: Tue, 25 Apr 2023 18:45:22 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Zhigang Shi <Zhigang.Shi@...eon.com>,
Shreeya Patel <shreeya.patel@...labora.com>,
Paul Gazzillo <paul@...zz.com>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
Hi Matti,
[...]
> +static int bu27008_get_int_time(struct bu27008_data *data)
this is providing the time in 'us' if I understood correctly.
Can you add it at the end of the function to make it clear?
bu27008_get_int_time_us().
No need to resend it just for this.
> +{
> + int ret, sel;
> +
> + ret = bu27008_get_int_time_sel(data, &sel);
> + if (ret)
> + return ret;
> +
> + return iio_gts_find_int_time_by_sel(&data->gts,
> + sel & BU27008_MASK_MEAS_MODE);
> +}
[...]
> +static int bu27008_try_set_int_time(struct bu27008_data *data, int int_time_new)
> +{
> + int ret, old_time_sel, new_time_sel, old_gain, new_gain;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = bu27008_get_int_time_sel(data, &old_time_sel);
> + if (ret < 0)
> + goto unlock_out;
> +
> + if (!iio_gts_valid_time(&data->gts, int_time_new)) {
> + dev_dbg(data->dev, "Unsupported integration time %u\n",
> + int_time_new);
> +
> + ret = -EINVAL;
> + goto unlock_out;
> + }
> + new_time_sel = iio_gts_find_sel_by_int_time(&data->gts, int_time_new);
> + if (new_time_sel == old_time_sel) {
> + ret = 0;
ret is already '0' here.
> + goto unlock_out;
> + }
[...]
> +static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
> + struct iio_chan_spec const *chan, int *val, int *val2)
> +{
> + int ret, int_time;
> +
> + ret = bu27008_chan_cfg(data, chan);
> + if (ret)
> + return ret;
> +
> + ret = bu27008_meas_set(data, BU27008_MEAS_EN);
> + if (ret)
> + return ret;
> +
> + int_time = bu27008_get_int_time(data);
> + if (int_time < 0)
> + int_time = 400000;
> +
> + msleep((int_time + 500) / 1000);
What is this 500 doing? Is it making a real difference? it's
0.5ms.
What's the minimum int_time? Can we set a minimum, as well, just
for the sake of the msleep?
> + ret = bu27008_chan_read_data(data, chan->address, val);
> + if (!ret)
> + ret = IIO_VAL_INT;
[...]
> +static int bu27008_set_scale(struct bu27008_data *data,
> + struct iio_chan_spec const *chan,
> + int val, int val2)
> +{
> + int ret, gain_sel, time_sel, i;
> +
> + if (chan->scan_index == BU27008_IR)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = bu27008_get_int_time_sel(data, &time_sel);
> + if (ret < 0)
> + goto unlock_out;
> +
> +
extra line here.
> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
> + val, val2 * 1000, &gain_sel);
> + if (ret) {
> + /* Could not support new scale with existing int-time */
> + int new_time_sel;
> +
> + for (i = 0; i < data->gts.num_itime; i++) {
> + new_time_sel = data->gts.itime_table[i].sel;
> + ret = iio_gts_find_gain_sel_for_scale_using_time(
> + &data->gts, new_time_sel, val, val2 * 1000,
> + &gain_sel);
> + if (!ret)
> + break;
> + }
> + if (i == data->gts.num_itime) {
> + dev_err(data->dev, "Can't support scale %u %u\n", val,
> + val2);
> +
> + ret = -EINVAL;
> + goto unlock_out;
> + }
> +
> + ret = bu27008_set_int_time_sel(data, new_time_sel);
> + if (ret)
> + goto unlock_out;
> + }
just a nit: I see you got tight here and goto's are not made only
for error handling. You could:
if (!ret)
goto something;
/*
* everything that you have inside the 'if (ret)' with
* one level of indentation less
*/
something:
ret = bu27008_write_gain_sel(data, gain_sel);
/* ... */
free to ignore this comment, though, I just saw that there are a
few cases where you can save some indentation above.
> +
> + ret = bu27008_write_gain_sel(data, gain_sel);
> +
> +unlock_out:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
[...]
> +static int bu27008_chip_init(struct bu27008_data *data)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
> + BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
> +
> + /*
> + * The data-sheet does not tell how long performing the IC reset takes.
> + * However, the data-sheet says the minimum time it takes the IC to be
> + * able to take inputs after power is applied, is 100 uS. I'd assume
> + * > 1 mS is enough.
> + */
> + msleep(1);
please use usleep_range().
> +
> + return ret;
> +}
[...]
> +static irqreturn_t bu27008_trigger_handler(int irq, void *p)
Do we really need to be in atomic context here? Can this be
handled from a thread?
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *idev = pf->indio_dev;
> + struct bu27008_data *data = iio_priv(idev);
> + struct {
> + __le16 chan[BU27008_NUM_HW_CHANS];
> + s64 ts __aligned(8);
> + } raw;
> + int ret, dummy;
> +
> + memset(&raw, 0, sizeof(raw));
> +
> + /*
> + * After some measurements, it seems reading the
> + * BU27008_REG_MODE_CONTROL3 debounces the IRQ line
> + */
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy);
> + if (ret < 0)
> + goto err_read;
> +
> + ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, &raw.chan,
> + sizeof(raw.chan));
> + if (ret < 0)
> + goto err_read;
> +
> + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
> +err_read:
> + iio_trigger_notify_done(idev->trig);
> +
> + return IRQ_HANDLED;
> +}
[...]
> +static int bu27008_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct iio_trigger *itrig;
> + struct bu27008_data *data;
> + struct regmap *regmap;
> + unsigned int part_id, reg;
> + struct iio_dev *idev;
> + char *name;
> + int ret;
> +
> + if (!i2c->irq)
> + dev_warn(dev, "No IRQ configured\n");
[...]
> + if (i2c->irq) {
e.g.:
if (!i2c->irq) {
dev_warn(dev, "No IRQ configured\n");
goto no_irq;
}
/* ... */
or, if you don't like the goto used like this...
> + ret = devm_iio_triggered_buffer_setup(dev, idev,
> + &iio_pollfunc_store_time,
> + bu27008_trigger_handler,
> + &bu27008_buffer_ops);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "iio_triggered_buffer_setup_ext FAIL\n");
> +
> + itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d",
> + idev->name, iio_device_id(idev));
> + if (!itrig)
> + return -ENOMEM;
> +
> + data->trig = itrig;
> +
> + itrig->ops = &bu27008_trigger_ops;
> + iio_trigger_set_drvdata(itrig, data);
> +
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008",
> + dev_name(dev));
> +
> + ret = devm_request_threaded_irq(dev, i2c->irq,
> + iio_pollfunc_store_time,
> + &bu27008_irq_thread_handler,
> + IRQF_ONESHOT, name, idev->pollfunc);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Could not request IRQ\n");
> +
> +
> + ret = devm_iio_trigger_register(dev, itrig);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Trigger registration failed\n");
> + }
} else {
dev_warn(dev, "No IRQ configured\n");
}
and save the first 'if' of this function.
> +
> + ret = devm_iio_device_register(dev, idev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Unable to register iio device\n");
> +
> + return ret;
> +}
[...]
Andi
Powered by blists - more mailing lists