| 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: <CAHp75Vcz1kv_xhaRZMdK21KrkAbeBLE3evJNsjA2yMa=V0Yb0g@mail.gmail.com>
Date: Tue, 28 Jul 2020 21:00:31 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Christian Eggers <ceggers@...i.de>
Cc: Rob Herring <robh+dt@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
linux-iio <linux-iio@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: light: as73211: New driver
On Tue, Jul 28, 2020 at 9:32 AM Christian Eggers <ceggers@...i.de> wrote:
>
> Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor.
>
> This driver has no built-in trigger. In order for making triggered
> measurements, an external (software) trigger driver like
> iio-trig-hrtimer or iio-trig-sysfs is required.
>
> The sensor supports single and continuous measurement modes. The latter
> is not used by design as this would require tight timing synchronization
> between hardware and driver without much benefit.
...
> +/*
> + * as73211.c - Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor
No need to have a filename in the very file.
> + *
> + * Author: Christian Eggers <ceggers@...i.de>
> + *
> + * Copyright (c) 2020 ARRI Lighting
> + *
> + * Color light sensor with 16-bit channels for red, green, blue, clear);
> + * 7-bit I2C slave address 0x74 .. 0x77.
> + *
> + * Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df
Can you also put it in the same form in the tag block of the commit message?
> + *
> + */
...
> +#define AS73211_OSR_DOS_MASK (0x7 << AS73211_OSR_DOS_SHIFT)
GENMASK()
...
> +#define AS73211_AGEN_DEVID_MASK (0xf << AS73211_AGEN_DEVID_SHIFT)
Ditto for all _MASK definitions.
...
> +
> +
One blank line is enough.
> +/**
> + * struct as73211_data - Instance data for one AS73211
> + * @client: I2C client.
> + * @osr: Cached Operational State Register.
> + * @creg1: Cached Configuration Register 1.
> + * @creg2: Cached Configuration Register 2.
> + * @creg3: Cached Configuration Register 3.
> + * @buffer: Buffer for triggered measurements.
> + * @mutex: Device mutex. N.B: We do not use iio_dev::mlock because we need to
> + * change device properties while in buffered mode.
I'm wondering if we have an established way to refer to fields in
other structures in kernel doc.
> + * @completion: Completion to wait for interrupt.
> + */
...
> +static unsigned int as73211_integration_time_cycles(struct as73211_data *data)
> +{
> + return 1 << (data->creg1 & AS73211_CREG1_TIME_MASK);
BIT() ?
> +}
...
> +/* return current integration time (in us) */
Useless comment
> +static unsigned int as73211_integration_time_us(struct as73211_data *data)
> +{
> + /* Integration time has 15 steps, the step size depends on the clock. */
> + unsigned int mul = 1 << (3 - (data->creg3 & 0b11)); /* 8 = 1.024 MHz, 4 = 2.048 MHz ...*/
BIT() ?
0b11?! 0x3 would work, but shouldn't be defined via GENMASK() or alike?
> + unsigned int time_cycles = as73211_integration_time_cycles(data);
> + unsigned int time_us = time_cycles * 125 * mul;
> +
> + return time_us;
return as73211_...(...) * 125 * mul;
> +}
...
> +/* return gain (1, 2, 4, 8, ...) */
Ditto. (Applies for all such)
> +static unsigned int as73211_gain(struct as73211_data *data)
> +{
> + return 1 << (0b1011 - (data->creg1 >> AS73211_CREG1_GAIN_SHIFT));
> +}
Similar comments. Also applies to the rest.
> +/* must be called with as73211_data::mutex held. */
> +static int as73211_req_data(struct as73211_data *data)
> +{
> + unsigned int time_us = as73211_integration_time_us(data), ret;
Please, split to separate lines.
Esp. wrong type for ret.
> + union i2c_smbus_data smbus_data;
> + s32 osr_status;
> +
> + if (data->client->irq)
> + init_completion(&data->completion);
I believe it should be reinit_completion().
> + /* During measurement, there should be no traffic on the i2c bus */
> + i2c_lock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
Hmm.. Really?
> + data->osr &= ~AS73211_OSR_DOS_MASK;
> + data->osr |= AS73211_OSR_DOS_MEASURE | AS73211_OSR_SS;
> +
> + smbus_data.byte = data->osr;
> + ret = __i2c_smbus_xfer(data->client->adapter, data->client->addr,
> + data->client->flags, I2C_SMBUS_WRITE,
> + AS73211_REG_OSR, I2C_SMBUS_BYTE_DATA, &smbus_data);
> + if (ret) {
> + i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
> + return ret;
> + }
> +
> + /* Reset AS73211_OSR_SS (is self clearing) in order to avoid unintentional
> + * triggering of further measurements later.
> + */
Comment style.
> + data->osr &= ~AS73211_OSR_SS;
> +
> + /* Add some extra margin for the timeout. sensor timing is not as precise
> + * as our one ...
> + */
> + time_us += time_us / 8;
> + if (data->client->irq) {
> + dev_dbg(&data->client->dev, "%s(): Waiting for completion...\n", __func__);
> + ret = wait_for_completion_timeout(&data->completion,
> + 2 + usecs_to_jiffies(time_us));
> + if (!ret) {
> + dev_err(&data->client->dev,
struct device = &data->client->dev;
will save you some LOCs here and there.
> + "timeout waiting for READY IRQ\n");
> + i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
> + return -ETIMEDOUT;
> + }
> + } else {
> + /* Wait integration time */
> + dev_dbg(&data->client->dev, "%s(): Sleeping %d us\n", __func__, time_us);
Please, drop __func__ from dev_dbg() calls. It duplicates Dynamic
Debug functionality. And TBH these are quite useless if you know how
to use ftrace, perf, etc.
> + usleep_range(time_us, time_us + 100000);
> + }
> +
> + i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
> +
> + osr_status = i2c_smbus_read_word_data(data->client, AS73211_OUT_OSR_STATUS);
> + if (osr_status < 0) {
> + ret = osr_status;
> + return ret;
> + }
> +
> + dev_dbg(&data->client->dev, "%s: osr_status = 0x%04x\n", __func__, osr_status);
> + if (osr_status != (AS73211_OSR_DOS_MEASURE | AS73211_OSR_STATUS_NDATA)) {
> + if (osr_status & AS73211_OSR_SS) {
> + dev_warn(&data->client->dev, "%s() Measurement has not stopped\n",
> + __func__);
> + return -ETIME;
> + }
> + if (osr_status & AS73211_OSR_STATUS_NOTREADY) {
> + dev_warn(&data->client->dev, "%s() Data is not ready\n", __func__);
> + return -ENODATA;
> + }
> + if (!(osr_status & AS73211_OSR_STATUS_NDATA)) {
> + dev_warn(&data->client->dev, "%s() New new data available\n", __func__);
> + return -ENODATA;
> + }
> + if (osr_status & AS73211_OSR_STATUS_LDATA) {
> + dev_warn(&data->client->dev, "%s() Result buffer overrun\n", __func__);
> + return -ENOBUFS;
> + }
> + if (osr_status & AS73211_OSR_STATUS_ADCOF) {
> + dev_warn(&data->client->dev, "%s() ADC overflow\n", __func__);
> + return -EOVERFLOW;
> + }
> + if (osr_status & AS73211_OSR_STATUS_MRESOF) {
> + dev_warn(&data->client->dev, "%s() Measurement result overflow\n",
> + __func__);
> + return -EOVERFLOW;
> + }
> + if (osr_status & AS73211_OSR_STATUS_OUTCONVOF) {
> + dev_warn(&data->client->dev, "%s() Timer overflow\n", __func__);
> + return -EOVERFLOW;
> + }
> + dev_warn(&data->client->dev, "%s() Unexpected status value\n", __func__);
> + return -EIO;
> + }
> +
> + return 0;
> +}
...
> + *val = (1 << (data->creg3 & 0b11)) * 1024 * 1000;
BIT()? GENMASK() ? 1000 I believe defined already.
...
> + }
> + } // switch (mask)
Strange indentation and useless comment.
I guess better to have something like
switch () {
...
case X: {
...
}}
...
> + if (*indio_dev->active_scan_mask == 0xf) {
Magic!
...
> + /* saturate all channels (useful for overflows) */
> + iio_buffer[1] = 0xffff;
> + iio_buffer[2] = 0xffff;
> + iio_buffer[3] = 0xffff;
Magic!
...
> + else if (*indio_dev->active_scan_mask == 0xe) {
Ditto.
...
> +static ssize_t as73211_show_samp_freq_available(
> + struct device *dev __always_unused,
> + struct device_attribute *attr __always_unused,
> + char *buf)
> +{
> + size_t len = 0;
> + int i;
> +
> + for (i = 0; i < 4; i++) {
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
> + (1 << (i + 10)) * 1000);
> + }
> +
> + /* replace trailing space by newline */
> + buf[len - 1] = '\n';
> +
> + return len;
> +}
Repetition of IIO core functionality?
Ditto question for the other similar functions.
...
> + struct i2c_device_id const *id __always_unused)
And why it's not a ->probe_new()?
...
> + if (indio_dev == NULL)
if (!indio_dev)
> + return -ENOMEM;
...
> + if ((ret >> AS73211_AGEN_DEVID_SHIFT) == 0b0010)
> + dev_info(&client->dev, "AS73211 found\n");
> + else
> + return -ENODEV;
Other way around (usual pattern)
err = ...
if (err) {
...
}
But that message is simple spam and noise in the log.
...
> +#ifdef CONFIG_PM_SLEEP
__maybe_unused
> +static int as73211_suspend(struct device *dev)
> +{
> +}
> +
> +static int as73211_resume(struct device *dev)
> +{
> +}
> +#endif
...
> +#ifdef CONFIG_OF
Dtop it.
> +static const struct of_device_id as73211_of_match[] = {
> + {
> + .compatible = "ams,as73211",
> + },
Can be one line.
> + {},
No comma here
> +};
> +MODULE_DEVICE_TABLE(of, as73211_of_match);
> +#else
> +#define as73211_of_match NULL
> +#endif
...
> +static struct i2c_driver as73211_driver = {
> + .driver = {
> + .name = AS73211_DRV_NAME,
> + .of_match_table = of_match_ptr(as73211_of_match),
Drop of_match_ptr().
> + .pm = &as73211_pm_ops,
> + },
> + .probe = as73211_probe,
> + .remove = as73211_remove,
> + .id_table = as73211_id,
> +};
> +module_i2c_driver(as73211_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists