| Message ID | 20200731070114.40471-1-ceggers@arri.de |
|---|---|
| Headers | show |
| Series | iio: light: Support AMS AS73211 digital XYZ sensor | expand |
On Fri, Jul 31, 2020 at 10:03 AM 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. Thanks for an update. My comments below. ... > +static const int as73211_samp_freq_avail[] = { 1024000, 2048000, 4096000, 8192000 }; This looks related to the below mentioned 1.024MHz. Perhaps add a definition above and comment here? #define AS73211_BASE_FREQ_1024KHZ 1024000 /* Available sample frequencies are power of two multiplier by 1.024MHz */ (Rephrase it better) > +static const int as73211_hardwaregain_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128, > + 256, 512, 1024, 2048 }; Indentation. Better is ... foo = { bar, baz, }; And in both cases leave comma at the last value. ... > +/** > + * 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. > + * @mutex: Keeps cached registers in synch with the device. sync > + * @completion: Completion to wait for interrupt. > + * @int_time_avail: Available integration times (depend on sampling frequency). > + */ ... > +/* integration time in units of 1024 clock cycles */ Unify this with below one. Or the other way around, i.o.w. join one of them into the other. > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data *data) > +{ > + /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */ > + return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1)); > +} ... > +static unsigned int as73211_integration_time_us(struct as73211_data *data, > + unsigned int integration_time_1024cyc) > +{ > + /* > + * f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) > + * t_cycl is configured in CREG1 in powers of 2 (x 1024 cycles) > + * t_int_us = 1 / (f_samp) * t_cycl * US_PER_SEC > + * = 1 / (2^CREG3_CCLK * 1,024,000) * 2^CREG1_CYCLES * 1,024 * US_PER_SEC > + * = 2^(-CREG3_CCLK) * 2^CREG1_CYCLES * 1,000 > + * in order to get rid of negative exponents, we extend the > + * "fraction" by 2^3 (3 == CREG3_CCLK,max) In the parentheses swap left and right parts for better reading. Perhaps shift left to have formulas separated from text visually. > + * = 2^(3-CREG3_CCLK) * 2^CREG1_CYCLES * 125 Okay, 125 = 1000/2^3. > + */ > + return BIT(3 - FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3)) * > + integration_time_1024cyc * 125; > +} ... > + data->int_time_avail[i * 2] = time_us / USEC_PER_SEC; I would do + 0, but it's up to you (complete style preference). > + data->int_time_avail[i * 2 + 1] = time_us % USEC_PER_SEC; ... > + unsigned int time_us = as73211_integration_time_us(data, > + as73211_integration_time_1024cyc(data)); One line? ... > + /* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */ > + *val = BIT(FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3)) * 1024000; As above mentioned, definition can be used. ... > + int reg_bits, freq_kHz = val / 1000 /* HZ_PER_KHZ */; /* 1024, 2048, ... */ > + > + /* val must be 1024 * 2^x */ > + if (val < 0 || (freq_kHz * 1000 /* HZ_PER_KHZ */) != val || > + !is_power_of_2(freq_kHz) || val2) > + return -EINVAL; Please, define HZ_PER_KHZ locally. It will really help when we move these definitions to a global level. ... > + /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */ > + reg_bits = 13 - ilog2(val); 13 is the second time in the code. Deserves a descriptive definition. ... > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (indio_dev == NULL) if (!indio_dev) > + return -ENOMEM; ... > + indio_dev->dev.parent = dev; Doesn't IIO core do this for you? ... > + /* At the time of writing this driver, only DEVID 2 and MUT 1 is known. */ are known ... > + /* enable device */ This is confusing and by the fact useless. ... > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) > + goto powerdown; > + > + return 0; > +powerdown: > + as73211_power(indio_dev, false); > + return ret; devm_*() is tricky. Here you broke ordering heavily. So, consider to add this under devm_add_action_or_reset(). ... > +static int as73211_remove(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > + > + as73211_power(indio_dev, false); > + > + return 0; > +} And as a result of the above this will be gone.
Hi Andy, thanks for further review. If nobody else sends comments, I'll publish the next version tonight. Maybe we could clarify the questions below in time. Best regards Christian > W=1 (not V=1) runs kernel doc validation script. without V=1, I get nothing. Neither excess nor missing members are reported on my system. On Friday, 31 July 2020, 09:34:02 CEST, Andy Shevchenko wrote: > On Fri, Jul 31, 2020 at 10:03 AM Christian Eggers <ceggers@arri.de> wrote: > > +static const int as73211_samp_freq_avail[] = { 1024000, 2048000, 4096000, > > 8192000 }; > This looks related to the below mentioned 1.024MHz. > > Perhaps add a definition above and comment here? > > #define AS73211_BASE_FREQ_1024KHZ 1024000 added similar define in v5. The array looks like the following now static const int as73211_samp_freq_avail[] = { AS73211_SAMPLE_FREQ_BASE, AS73211_SAMPLE_FREQ_BASE * 2, AS73211_SAMPLE_FREQ_BASE * 4, AS73211_SAMPLE_FREQ_BASE * 8 }; > > +/* integration time in units of 1024 clock cycles */ > > Unify this with below one. Or the other way around, i.o.w. join one of > them into the other. > > > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data > > *data) +{ > > + /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */ > > + return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1)); > > +} I'm not sure, whether this is possible. as73211_integration_time_1024cyc() returns the current setting from hardware. as73211_integration_time_us() calculates the resulting time. But as73211_integration_time_us() is also called in as73211_integration_time_calc_avail() inside the loop. > > + unsigned int time_us = as73211_integration_time_us(data, > > + as73211_integration_time_1024cyc(data)); > One line? checkpatch complains... ignore? > > + int reg_bits, freq_kHz = val / 1000 /* HZ_PER_KHZ */; /* > > 1024, 2048, ... */ + > > + /* val must be 1024 * 2^x */ > > + if (val < 0 || (freq_kHz * 1000 /* HZ_PER_KHZ */) != val > > || > > + !is_power_of_2(freq_kHz) || val2) > > + return -EINVAL; > > Please, define HZ_PER_KHZ locally. It will really help when we move > these definitions to a global level. ok > > ... > > > + /* gain can be calculated from CREG1 as 2^(13 - > > CREG1_GAIN) */ + reg_bits = 13 - ilog2(val); > > 13 is the second time in the code. Deserves a descriptive definition. I'm unsure how to solve this. Possible values for gain: CREG1[7:4] | gain ----------------------------- 0 | 2048x 1 | 1024x 2 | 512x ... | ... 13 | 1x #define AS73211_CREG1_GAIN_1_NON_SHIFTED 13 // this define is CREG1 related, but not shifted to the right position static unsigned int as73211_gain(struct as73211_data *data) { /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */ return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1)); } ---- or ---- #define AS73211_CREG1_GAIN_1 FIELD_PREP(AS73211_CREG1_GAIN_MASK, 13) static unsigned int as73211_gain(struct as73211_data *data) { /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */ return BIT(FIELD_GET(AS73211_CREG1_GAIN_MASK, AS73211_CREG1_GAIN_1) - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1)); } > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > + if (indio_dev == NULL) > > if (!indio_dev) > > > + return -ENOMEM; > > ... > > > + indio_dev->dev.parent = dev; > > Doesn't IIO core do this for you? devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc(). I already looked around, but I didn't find. And after debugging v5.4, devm_iio_device_alloc() definitely doesn't do it. > > + ret = devm_iio_device_register(dev, indio_dev); > > + if (ret < 0) > > + goto powerdown; > > + > > + return 0; > > > > +powerdown: > > + as73211_power(indio_dev, false); > > + return ret; > > devm_*() is tricky. Here you broke ordering heavily. So, consider to > add this under devm_add_action_or_reset(). Sorry, my mistake! I already felt that something may be wrong here...
On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <ceggers@arri.de> wrote: > > W=1 (not V=1) runs kernel doc validation script. > without V=1, I get nothing. Neither excess nor missing members > are reported on my system. It's strange. ... > > Perhaps add a definition above and comment here? > > > > #define AS73211_BASE_FREQ_1024KHZ 1024000 > added similar define in v5. The array looks like the following now > > static const int as73211_samp_freq_avail[] = { > AS73211_SAMPLE_FREQ_BASE, ' * 1' > AS73211_SAMPLE_FREQ_BASE * 2, > AS73211_SAMPLE_FREQ_BASE * 4, > AS73211_SAMPLE_FREQ_BASE * 8 > }; ... > > > +/* integration time in units of 1024 clock cycles */ > > > > Unify this with below one. Or the other way around, i.o.w. join one of > > them into the other. > > > > > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data > > > *data) +{ > > > + /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */ > > > + return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1)); > > > +} > I'm not sure, whether this is possible. as73211_integration_time_1024cyc() > returns the current setting from hardware. as73211_integration_time_us() > calculates the resulting time. But as73211_integration_time_us() is also > called in as73211_integration_time_calc_avail() inside the loop. What I meant is solely comments to be joined, not the code. ... > > > + unsigned int time_us = as73211_integration_time_us(data, > > > + as73211_integration_time_1024cyc(data)); > > One line? > checkpatch complains... ignore? Hmm... is it over 100? Or you are using some old tools to work with the kernel... ... > > > + /* gain can be calculated from CREG1 as 2^(13 - > > > CREG1_GAIN) */ + reg_bits = 13 - ilog2(val); > > > > 13 is the second time in the code. Deserves a descriptive definition. > I'm unsure how to solve this. Possible values for gain: > > CREG1[7:4] | gain > ----------------------------- > 0 | 2048x > 1 | 1024x > 2 | 512x > ... | ... > 13 | 1x > > #define AS73211_CREG1_GAIN_1_NON_SHIFTED 13 // this define is CREG1 related, but not shifted to the right position > > static unsigned int as73211_gain(struct as73211_data *data) > { > /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */ > return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1)); > } This way (w/o _NON_SHIFTED suffix) if both 13:s in the code are of the same meaning. ... > > > + indio_dev->dev.parent = dev; > > > > Doesn't IIO core do this for you? > devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc(). > I already looked around, but I didn't find. And after debugging > v5.4, devm_iio_device_alloc() definitely doesn't do it. Why are you talking about v5.4? We are in v5.8 cycle contributing to v5.9. Recently IIO gained some features among which I think the one that assigns parent devices.
On Fri, 31 Jul 2020 18:41:47 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <ceggers@arri.de> wrote: > > > > W=1 (not V=1) runs kernel doc validation script. > > without V=1, I get nothing. Neither excess nor missing members > > are reported on my system. > > It's strange. > > ... > > > > Perhaps add a definition above and comment here? > > > > > > #define AS73211_BASE_FREQ_1024KHZ 1024000 > > added similar define in v5. The array looks like the following now > > > > static const int as73211_samp_freq_avail[] = { > > > AS73211_SAMPLE_FREQ_BASE, > > ' * 1' > > > AS73211_SAMPLE_FREQ_BASE * 2, > > AS73211_SAMPLE_FREQ_BASE * 4, > > AS73211_SAMPLE_FREQ_BASE * 8 > > }; > > ... > > > > > +/* integration time in units of 1024 clock cycles */ > > > > > > Unify this with below one. Or the other way around, i.o.w. join one of > > > them into the other. > > > > > > > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data > > > > *data) +{ > > > > + /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */ > > > > + return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1)); > > > > +} > > I'm not sure, whether this is possible. as73211_integration_time_1024cyc() > > returns the current setting from hardware. as73211_integration_time_us() > > calculates the resulting time. But as73211_integration_time_us() is also > > called in as73211_integration_time_calc_avail() inside the loop. > > What I meant is solely comments to be joined, not the code. > > ... > unsigned int time_us = as73211_integration_time_us(data, as73211_integration_time_1024cyc(data)); > > > One line? > > > checkpatch complains... ignore? > > Hmm... is it over 100? Or you are using some old tools to work with > the kernel... It's over a 100... (about 103 by the handy scale at the top of my email client :) > > ... > > > > > + /* gain can be calculated from CREG1 as 2^(13 - > > > > CREG1_GAIN) */ + reg_bits = 13 - ilog2(val); > > > > > > 13 is the second time in the code. Deserves a descriptive definition. > > > I'm unsure how to solve this. Possible values for gain: > > > > CREG1[7:4] | gain > > ----------------------------- > > 0 | 2048x > > 1 | 1024x > > 2 | 512x > > ... | ... > > 13 | 1x > > > > #define AS73211_CREG1_GAIN_1_NON_SHIFTED 13 // this define is CREG1 related, but not shifted to the right position > > > > static unsigned int as73211_gain(struct as73211_data *data) > > { > > /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */ > > return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1)); > > } > > This way (w/o _NON_SHIFTED suffix) if both 13:s in the code are of the > same meaning. > > ... > > > > > + indio_dev->dev.parent = dev; > > > > > > Doesn't IIO core do this for you? > > devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc(). > > I already looked around, but I didn't find. And after debugging > > v5.4, devm_iio_device_alloc() definitely doesn't do it. > > Why are you talking about v5.4? We are in v5.8 cycle contributing to v5.9. This will be 5.10 now. 5.9 cycle for new stuff via IIO effectively closed last week. > > Recently IIO gained some features among which I think the one that > assigns parent devices. > yup. This should be in linux-next now for the coming merge window (which probably opens on Sunday). Note that we have lots of time to tidy up loose ends for this one as it will only sit in my tree for next few weeks if I do pick it up. Jonathan
On Fri, 31 Jul 2020 09:01:14 +0200 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. > > Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df > Signed-off-by: Christian Eggers <ceggers@arri.de> A question inline about flipping to config mode mid capture. > --- > MAINTAINERS | 7 + > drivers/iio/light/Kconfig | 15 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/as73211.c | 780 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 803 insertions(+) > create mode 100644 drivers/iio/light/as73211.c > ... > + > +static int as73211_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct as73211_data *data = iio_priv(indio_dev); > + int ret; > + > + mutex_lock(&data->mutex); > + > + /* Need to switch to config mode ... */ Is this safe whilst we are doing a capture? You may want to claim_direct_mode for write_raw to ensure we don't get such a race. > + if ((data->osr & AS73211_OSR_DOS_MASK) != AS73211_OSR_DOS_CONFIG) { > + data->osr &= ~AS73211_OSR_DOS_MASK; > + data->osr |= AS73211_OSR_DOS_CONFIG; > + > + ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_OSR, data->osr); > + if (ret < 0) > + goto error_release; > + } > + > + ret = _as73211_write_raw(indio_dev, chan, val, val2, mask); > + > +error_release: > + mutex_unlock(&data->mutex); > + return ret; > +}