| 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
| ||
|
Message-ID: <80196942-4c33-7625-3945-86ce5b7b347f@gmail.com>
Date: Thu, 14 Apr 2022 14:06:56 +0300
From: Cosmin Tanislav <demonsingur@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
linux-iio <linux-iio@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
On 4/13/22 18:41, Andy Shevchenko wrote:
> On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@...il.com> wrote:
> >
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>
> ...
>
>> +#define AD4130_8_NAME "ad4130-8"
>
> What the meaning of -8 ? Is it number of channels? Or is it part of
> the official model (part number)? Can we see, btw, Datasheet: tag with
> a corresponding link in the commit message?
>
That's just the name specified in the datasheet. I honestly don't have
much of an idea about why it is like that. Also, I already put the
datasheet in the .yaml documentation. Do you really also want it
in each commit message too?
>
>> +#define AD4130_RESET_CLK_COUNT 64
>> +#define AD4130_RESET_BUF_SIZE (AD4130_RESET_CLK_COUNT / 8)
>
> To be more precise shouldn't the above need to have DIV_ROUND_UP() ?
>
Does it look like 64 / 8 needs any rounding?
> ...
>
>> +#define AD4130_FREQ_FACTOR 1000000000ull
>> +#define AD4130_DB3_FACTOR 1000
>
> Ditto.
AD4130_DB3_FACTOR is unit-less. In the datasheet, the relation between
sampling frequency and 3db frequency is represented as a 0.xyz value,
hence why the db3_div values and 1000 factor.
>
>> +enum ad4130_fifo_mode {
>> + AD4130_FIFO_MODE_DISABLED = 0b00,
>> + AD4130_FIFO_MODE_WATERMARK = 0b01,
>> +};
>> +
>> +enum ad4130_mode {
>> + AD4130_MODE_CONTINUOUS = 0b0000,
>> + AD4130_MODE_IDLE = 0b0100,
>> +};
>
> 0b?! Hmm... Not that this is bad, just not so usual :-)
>
I always use 0b to be consistent with the datasheet values which are
represented in binary. I think it makes it easier to not mess up
when initially translating the datasheet into code and later when
cross-checking with the datasheet.
>
>> +struct ad4130_filter_config {
>> + enum ad4130_filter_mode filter_mode;
>> + unsigned int odr_div;
>> + unsigned int fs_max;
>> + unsigned int db3_div;
>> + enum iio_available_type samp_freq_avail_type;
>> + int samp_freq_avail_len;
>> + int samp_freq_avail[3][2];
>> + enum iio_available_type db3_freq_avail_type;
>> + int db3_freq_avail_len;
>> + int db3_freq_avail[3][2];
>
> These 3:s can be defined?
>
I could define IIO_AVAIL_RANGE_LEN and IIO_AVAIL_SINGLE_LEN and then
define another IIO_AVAIL_LEN that is the max between the two.
But that's just over-complicating it, really.
> ...
>
>
>> +static int ad4130_get_reg_size(struct ad4130_state *st, unsigned int reg,
>> + unsigned int *size)
>> +{
>
>> + if (reg >= ARRAY_SIZE(ad4130_reg_size))
>> + return -EINVAL;
>
> When this condition is true?
When the user tries reading a register from direct_reg_access
that hasn't had its size defined.
>
>> + regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
>> + value ? mask : 0);
>
> One line?
>
> No error check?
>
I actually can't think of a scenario where this would fail. It doesn't
if the chip is not even connected.
>
>> + if (setup_info->enabled_channels)
>> + return -EINVAL;
>
> -EBUSY?
>
Eh, I don't think so. It would be pretty impossible for the code to hit
this if statement, taking into account the ad4130_find_slot() logic.
I could as well not have it at all.
>
>> +static void ad4130_freq_to_fs(enum ad4130_filter_mode filter_mode,
>> + int val, int val2, unsigned int *fs, bool db3)
>> +{
>> + const struct ad4130_filter_config *filter_config =
>> + &ad4130_filter_configs[filter_mode];
>> + unsigned long long dividend, divisor;
>> + int temp;
>> +
>> + dividend = filter_config->fs_max * filter_config->odr_div *
>> + (val * AD4130_FREQ_FACTOR + val2);
>> + divisor = AD4130_MAX_ODR * AD4130_FREQ_FACTOR;
>> +
>> + if (db3) {
>> + dividend *= AD4130_DB3_FACTOR;
>> + divisor *= filter_config->db3_div;
>> + }
>> +
>> + temp = AD4130_FS_MIN + filter_config->fs_max -
>> + DIV64_U64_ROUND_CLOSEST(dividend, divisor);
>> +
>> + if (temp < AD4130_FS_MIN)
>> + temp = AD4130_FS_MIN;
>> + else if (temp > filter_config->fs_max)
>> + temp = filter_config->fs_max;
>> +
>> + *fs = temp;
>
> Would be nice to put a comment explaining the math behind this code.
>
>> +}
>> +
>> +static void ad4130_fs_to_freq(enum ad4130_filter_mode filter_mode,
>> + unsigned int fs, int *val, int *val2, bool db3)
>> +{
>> + const struct ad4130_filter_config *filter_config =
>> + &ad4130_filter_configs[filter_mode];
>> + unsigned int dividend, divisor;
>> + u64 temp;
>> +
>> + dividend = (filter_config->fs_max - fs + AD4130_FS_MIN) *
>> + AD4130_MAX_ODR;
>> + divisor = filter_config->fs_max * filter_config->odr_div;
>> +
>> + if (db3) {
>> + dividend *= filter_config->db3_div;
>> + divisor *= AD4130_DB3_FACTOR;
>> + }
>> +
>> + temp = div_u64(dividend * AD4130_FREQ_FACTOR, divisor);
>> + *val = div_u64_rem(temp, AD4130_FREQ_FACTOR, val2);
>
>
> Ditto.
>
I'll see what I can come up with.
>
>> + out:
>
> out_unlock: ?
> Ditto for similar cases.
There's a single label in the function, and there's a mutex being
taken, and, logically, the mutex must be released on the exit path.
It's clear what the label is for to me.
>
>> + *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
>
> Hmm... It seems like specific way to have a sign_extended, or actually
> reduced) mask.
> Can you rewrite it with the (potential)UB-free approach?
>
> (Note, that if realbits == 32, this will have a lot of fun in
> accordance with C standard.)
>
Can you elaborate on this? The purpose of this statement is to shift the
results so that, when bipolar configuration is enabled, the raw value is
offset with 1 << (realbits - 1) towards negative.
For the 24bit chips, 0x800000 becomes 0x000000.
Maybe you misread it as left shift on a negative number? The number
is turned negative only after the shift...
> ...
>
>> + *vals = (int *)st->scale_tbls[setup_info->ref_sel];
>
> Can we get rid of casting here and in the similar cases?
>
I feel like scale_tbls is best defined as an array of two-element
arrays. Because its type is IIO_VAL_FRACTIONAL.
But obviously the IIO framework can't take this case into account by
itself, so we cast it so it receives what it wants.
>
>> +
>> + if (val > AD4130_FIFO_SIZE)
>> + return -EINVAL;
>> +
>> + /*
>> + * Always set watermark to a multiple of the number of enabled channels
>> + * to avoid making the FIFO unaligned.
>> + */
>> + eff = rounddown(val, st->num_enabled_channels);
>> +
>> + mutex_lock(&st->lock);
>> +
>> + ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
>> + AD4130_WATERMARK_MASK,
>> + FIELD_PREP(AD4130_WATERMARK_MASK,
>> + ad4130_watermark_reg_val(eff)));
>
> Temporary variable for mask?
>
You mean for value?
>
>> +static int ad4130_get_ref_voltage(struct ad4130_state *st,
>> + enum ad4130_ref_sel ref_sel,
>> + unsigned int *ref_uv)
>> +{
>> + struct device *dev = &st->spi->dev;
>> + int ret;
>> +
>> + switch (ref_sel) {
>> + case AD4130_REF_REFIN1:
>> + ret = regulator_get_voltage(st->regulators[2].consumer);
>> + break;
>> + case AD4130_REF_REFIN2:
>> + ret = regulator_get_voltage(st->regulators[3].consumer);
>> + break;
>> + case AD4130_REF_AVDD_AVSS:
>> + ret = regulator_get_voltage(st->regulators[0].consumer);
>> + break;
>> + case AD4130_REF_REFOUT_AVSS:
>
>> + if (!st->int_ref_en) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + ret = st->int_ref_uv;
>> + break;
>
> Can be one if-else instead.
>
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + if (ret <= 0)
>
> = 0 ?! Can you elaborate, please, this case taking into account below?
>
I guess I just did it because voltage = 0 doesn't make sense and would
make scale be 0.0.
>> + return dev_err_probe(dev, ret, "Cannot use reference %u\n",
>> + ref_sel);
>> +
>> + if (ref_uv)
>> + *ref_uv = ret;
>> +
>> + return 0;
>> +}
>
>
>> + fwnode_property_read_u32(child, "adi,excitation-pin-0",
>> + &chan_info->iout0);
>
> No default and/or error check?
>
> ...
>
>> + fwnode_property_read_u32(child, "adi,excitation-pin-1",
>> + &chan_info->iout1);
>
> Ditto.
Default is 0, just like the register defaults.
>
> ...
>
>> +static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
>> +{
>> + struct ad4130_state *st = iio_priv(indio_dev);
>> + struct device *dev = &st->spi->dev;
>> + struct fwnode_handle *child;
>> + int ret;
>> +
>> + indio_dev->channels = st->chans;
>> +
>> + device_for_each_child_node(dev, child) {
>> + ret = ad4130_parse_fw_channel(indio_dev, child);
>> + if (ret)
>> + break;
>> + }
>
>> + fwnode_handle_put(child);
>
> There is no need to put fwnode if child is NULL. Moreover, the above
> pattern might be percepted wrongly, i.e. one may think that
> fwnode_handle_put() is a must after a loop.
>
fwnode_handle_put already checks if the child is NULL. Why do the same
check twice?
>
> Can you explain why regmap locking is needed?
>
Am I supposed to set .disable_locking = true since SPI has its own
locking?
Powered by blists - more mailing lists