linux-kernel - Re: [PATCH v4 2/2] iio: light: as73211: New driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 31 Jul 2020 12:52:22 +0200
From:   Christian Eggers <ceggers@...i.de>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Rob Herring <robh+dt@...nel.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Jonathan Cameron <jic23@...nel.org>,
        "Hartmut Knaack" <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Peter Meerwald-Stadler" <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] iio: light: as73211: New driver

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