Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 30 Jul 2020 14:31:31 +0300 | Subject | Re: [PATCH v3 2/2] iio: light: as73211: New driver |
| |
On Thu, Jul 30, 2020 at 1:52 PM Christian Eggers <ceggers@arri.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.
...
> +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/triggered_buffer.h>
Ordered?
...
> +/** > + * 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: Keeps cached registers in synch with the device. > + * @completion: Completion to wait for interrupt. > + */ > +struct as73211_data { > + struct i2c_client *client; > + u8 osr; > + u8 creg1; > + u8 creg2; > + u8 creg3; > + struct mutex mutex; > + struct completion completion; > +};
Actually Jonathan is correct -- the above has kernel doc issues.
...
> +#define AS73211_SCAN_MASK_COLOR \ > + (BIT(AS73211_SCAN_INDEX_X) | BIT(AS73211_SCAN_INDEX_Y) | BIT(AS73211_SCAN_INDEX_Z))
> +#define AS73211_SCAN_MASK_ALL (BIT(AS73211_SCAN_INDEX_TEMP) | AS73211_SCAN_MASK_COLOR)
Perhaps split for easy reading.
...
> +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 = BIT(3 - (data->creg3 & GENMASK(1, 0)));
Shouldn't be rather
#define FOO GENMASK(1, 0) mul = BIT(FOO - (creg & FOO));
with a descriptive name of FOO?
Or if both 3:s have different meaning, two definitions?
> + return as73211_integration_time_cycles(data) * 125 * mul; > +}
...
> +static unsigned int as73211_gain(struct as73211_data *data) > +{
> + return BIT(0xb - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
Magic!
> +}
...
> + struct device *dev = &data->client->dev; > + unsigned int time_us = as73211_integration_time_us(data); > + int ret; > + union i2c_smbus_data smbus_data; > + u16 osr_status;
Can you keep this in reversed tree order?
unsigned int time_us = as73211_integration_time_us(data); struct device *dev = &data->client->dev; union i2c_smbus_data smbus_data; u16 osr_status; int ret;
Same comment to the rest where applicable.
...
> + /* Add some extra margin for the timeout. sensor timing is not as precise > + * as our one ... > + */
Comment style. Same to the rest of the code where applicable.
...
> + if (data->client->irq) { > + dev_dbg(dev, "Waiting for completion...\n"); > + ret = wait_for_completion_timeout(&data->completion,
> + 2 + usecs_to_jiffies(time_us));
Magic!
> + if (!ret) { > + dev_err(dev, "timeout waiting for READY IRQ\n"); > + i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT); > + return -ETIMEDOUT; > + } > + } else { > + /* Wait integration time */
> + dev_dbg(dev, "Sleeping %d us\n", time_us);
Looks like a noise (good for development, bad for upstream)
> + usleep_range(time_us, time_us + 100000);
If time_us appears to be, let's say, 100. The above margin is way too high.
> + }
...
> + osr_status = (u16)ret;
Why casting?
...
> + dev_dbg(dev, "osr_status = 0x%04x\n", osr_status);
Looks like a noise (good for development, bad for upstream)
...
> + if (osr_status & AS73211_OSR_SS) { > + dev_warn(dev, "%s() Measurement has not stopped\n", __func__); > + return -ETIME; > + } > + if (osr_status & AS73211_OSR_STATUS_NOTREADY) { > + dev_warn(dev, "%s() Data is not ready\n", __func__); > + return -ENODATA; > + } > + if (!(osr_status & AS73211_OSR_STATUS_NDATA)) { > + dev_warn(dev, "%s() New new data available\n", __func__); > + return -ENODATA; > + } > + if (osr_status & AS73211_OSR_STATUS_LDATA) { > + dev_warn(dev, "%s() Result buffer overrun\n", __func__); > + return -ENOBUFS; > + } > + if (osr_status & AS73211_OSR_STATUS_ADCOF) { > + dev_warn(dev, "%s() ADC overflow\n", __func__); > + return -EOVERFLOW; > + } > + if (osr_status & AS73211_OSR_STATUS_MRESOF) { > + dev_warn(dev, "%s() Measurement result overflow\n", __func__); > + return -EOVERFLOW; > + } > + if (osr_status & AS73211_OSR_STATUS_OUTCONVOF) { > + dev_warn(dev, "%s() Timer overflow\n", __func__); > + return -EOVERFLOW; > + } > + dev_warn(dev, "%s() Unexpected status value\n", __func__); > + return -EIO; > + }
All above dev_warn() are wrong. Should be dev_err(). Otherwise all return -ERRNO are wrong.
...
> + int ret = iio_device_claim_direct_mode(indio_dev); > + > + if (ret) > + return ret;
Please, use better arrangement, i.e.
int ret;
ret = ... if (ret) ...
+ blank line.
> + ret = as73211_req_data(data); > + if (ret < 0) { > + iio_device_release_direct_mode(indio_dev); > + return ret; > + }
+ blank line
> + ret = i2c_smbus_read_word_data(data->client, chan->address); > + iio_device_release_direct_mode(indio_dev); > + if (ret < 0) > + return ret;
+ blank line
> + *val = ret; > + return IIO_VAL_INT; > + }
...
> + case IIO_CHAN_INFO_SCALE: > + {
On one line.
...
> + *val = BIT(data->creg3 & GENMASK(1, 0)) * 1024 * 1000;
1000 is magic and 1024 either.
1000 is frequency multiplier? If it's not defined do it like #define HZ_PER_KHZ #define KHZ_PER_MHZ or what is that?
...
> + *val = time_us / 1000000; > + *val2 = time_us % 1000000;
Ditto.
NSEC_PER_SEC or what?
...
> + int reg_bits, freq_kHz = val / 1000; /* 1024, 2048, ... */
HZ_PER_KHZ?
...
> + if (val < 0 || (freq_kHz * 1000) != val || val2)
Ditto.
...
> + return i2c_smbus_write_byte_data(data->client, AS73211_REG_CREG3, data->creg3);
Can it suddenly return positive number?
...
> + int time_us = val * 1000000 + val2, time_ms;
NSEC_PER_MSEC ? USEC_PER_SEC ?
...
> + time_ms = (time_us * mul) / 1000; /* 1 ms, 2 ms, ... (power of two) */
NSEC_PER_MSEC
...
> + if (time_ms < 0 || !is_power_of_2(time_ms) || time_ms > 16384)
Magic! Shouldn't be rather BIT(14) and defined somewhere?
...
> + /* Need to switch to config mode ... */
Not sure how this comment related to locking.
> + mutex_lock(&data->mutex);
...
> + dev_dbg(&indio_dev->dev, "Switching to configuration state...");
Noise.
...
> +static irqreturn_t as73211_ready_handler(int irq __always_unused, void *priv) > +{
> + struct iio_dev *indio_dev = priv;
Unneeded churn.
> + struct as73211_data *data = iio_priv(indio_dev);
iio_priv(priv)
> + complete(&data->completion); > + > + return IRQ_HANDLED; > +}
...
> + struct iio_dev *indio_dev = pf->indio_dev;
Ditto. Do you use indio_dev below (except one case)? I haven't noticed.
> + struct as73211_data *data = iio_priv(indio_dev);
...
> + struct i2c_msg msgs[] = { > + { > + .addr = data->client->addr, > + .flags = 0, > + .len = 1, > + .buf = &addr
+ comma
> + }, > + { > + .addr = data->client->addr, > + .flags = I2C_M_RD, > + .len = sizeof(scan.chan), > + .buf = (u8 *)&scan.chan
+ comma
> + }, > + };
...
> + } > + /* Optimization for reading only color channels */ > + else {
Should be one line. Had you run checkpatch?
...
> +static ssize_t as73211_show_samp_freq_available(struct device *dev __always_unused, > + struct device_attribute *attr __always_unused, > + char *buf) > +{ > +} > + > +static ssize_t as73211_show_int_time_available(struct device *dev, > + struct device_attribute *attr __always_unused, > + char *buf) > +{ > +} > + > +static ssize_t as73211_show_hardwaregain_available(struct device *dev __always_unused, > + struct device_attribute *attr __always_unused, > + char *buf) > +{ > +}
I guess above is an overkill. See example here
commit 6085102c494b8afc62bc093f8f32d6248f6d33e5 Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Mon Mar 23 12:41:26 2020 +0200
iio: pressure: bmp280: Convert to use ->read_avail()
...
> + ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR); > + if (ret < 0) > + return ret;
Yeah, be consistent. Either all such call should be checked against negative error code, or drop ' < 0' everywhere it applies.
...
> + /* At the time of writing this driver, only DEVID 2 and MUT 1 is known. */ > + if ((ret & AS73211_AGEN_DEVID_MASK) != AS73211_AGEN_DEVID(2) > + || (ret & AS73211_AGEN_MUT_MASK) != AS73211_AGEN_MUT(1))
Looks like broken indentation, and leave || on previous line.
> + return -ENODEV;
...
> + if (client->irq) { > + ret = request_threaded_irq(client->irq, > + NULL, > + as73211_ready_handler,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
Should not be IRQ trigger type here. (Do you have broken firmware table?)
> + client->name, indio_dev); > + if (ret) > + goto buffer_cleanup; > + }
-- With Best Regards, Andy Shevchenko
| |