On Tue, Jul 28, 2020 at 9:32 AM Christian Eggers <ceggers@xxxxxxx> 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@xxxxxxx> > + * > + * 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
