On Thu, Apr 14, 2022 at 2:06 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> On 4/13/22 18:41, Andy Shevchenko wrote:
> > On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
...
> >> +#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.
That's cool!
> Do you really also want it
> in each commit message too?
No, just in this one.
...
> >> +#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?
Currently no, but if someone puts 63 there or 65, what would be the outcome?
OTOH, you may add a static assert to guarantee that CLK_COUNT is multiple of 8.
...
> >> +#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.
But does the above mean MILLI or KILO? Similar for the FREQ factor.
...
> >> + int samp_freq_avail_len;
> >> + int samp_freq_avail[3][2];
> >> + 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.
I was talking only about 3:s (out array). IIRC I saw 3 hard coded in
the driver, but not sure if its meaning is the same. Might be still
good to define.
...
> >> + 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.
But how is it possible? Is the reg parameter taken directly from the user?
...
> >> + 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.
Why to check errors in many other cases then? Be consistent one way or
the other.
...
> >> + 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.
If it's a dead code, we do not want it.
...
> >> + 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.
Wasn't clear to me until I went to the end of each of them (who
guarantees that's the case for all of them?).
...
> >> + *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...
1 << 31 is UB in accordance with the C standard.
And the magic above seems to me the opposite to what sign_extend()
does. Maybe even providing a general function for sign_comact() or so
(you name it) would be also nice to have.
...
> >> + 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?
mask = AD4130_WATERMARK_MASK;
ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
mask, FIELD_PREP(mask,
ad4130_watermark_reg_val(eff)));
...
> >> + 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.
Again, what's the meaning of having it in the conjunction with
dev_err_probe() call?
> >> + return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> >> + ref_sel);
It's confusing. I believe you need two different messages if you want
to handle the 0 case.
...
> >> +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?
Exactly my point. Why do you check it twice?
...
> > Can you explain why regmap locking is needed?
> Am I supposed to set .disable_locking = true since SPI has its own
> locking?
You tell me. I have no idea of what the locking schema is being used
in your code.
--
With Best Regards,
Andy Shevchenko
