On Fri, Jul 31, 2020 at 10:03 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.
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.
--
With Best Regards,
Andy Shevchenko
