On Sat, 2024-06-29 at 16:06 -0300, Marcelo Schmitt wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive approximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> ---
Hi Marcelo,
LGTM. Only one thing that needs to be addressed. With that,
Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4000.c | 708 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 722 insertions(+)
> create mode 100644 drivers/iio/adc/ad4000.c
>
...
>
> + st->gain_milli = 1000;
> + if (chip->has_hardware_gain &&
> + device_property_present(dev, "adi,gain-milli")) {
> + ret = device_property_read_u16(dev, "adi,gain-milli",
> + &st->gain_milli);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to read gain
> property\n");
> + }
The above is odd. Why not reading directly device_property_read_u16()? Skip the
call to device_property_present().
But most importantly, you're not doing any validation on gain_milli which is an
enum (by looking at the bindings). So in theory even 0 would be accepted which
would lead to a divide by 0 later on. I would do:
if (chip->has_hardware_gain) {
ret = device_property_read_u16(...)
if (!ret) {
/* validate here for a proper value /*
}
}
You can also check for ret < 0 and -EINVAL to detect an invalid devicetree
parameter instead of completely ignoring return codes (but for non mandatory
properties one typically does not care much - up to you)
- Nuno Sá
