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@xxxxxxx> 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...
