Re: [PATCH v7 2/2] iio: adc: ad7173: add AD7173 driver

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Dec 5, 2023 at 3:46 PM Dumitru Ceclan <mitrutzceclan@xxxxxxxxx> wrote:
>
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.

...

> +config AD7173
> +       tristate "Analog Devices AD7173 driver"
> +       depends on SPI_MASTER
> +       select AD_SIGMA_DELTA
> +       select GPIO_REGMAP if GPIOLIB
> +       select REGMAP_SPI if GPIOLIB
> +       help
> +         Say yes here to build support for Analog Devices AD7173 and similar ADC
> +         Currently supported models:
> +          - AD7172-2,
> +          - AD7173-8,
> +          - AD7175-2,
> +          - AD7176-2

Drop commas (sorry if I was not clear enough).


> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>

> +#include <linux/gpio/driver.h>

This...

> +#include <linux/idr.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>

> +#include <linux/gpio/regmap.h>

...and this are too far from each other. I believe you should count on G order.

> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +#define AD7173_GPO12_DATA(x)   BIT(x)

x + 0 ?

> +#define AD7173_GPO23_DATA(x)   BIT(x + 4)
> +#define AD7173_GPO_DATA(x)     (x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))

...

> +               dev_warn(&st->sd.spi->dev,

Here...

> +                        "Unexpected device id: 0x%04X, expected: 0x%04X\n",
> +                        id, st->info->id);
> +
> +       st->adc_mode |= AD7173_ADC_MODE_SING_CYC;
> +       st->interface_mode = 0x0;
> +
> +       st->config_usage_counter = 0;
> +       st->config_cnts = devm_kcalloc(indio_dev->dev.parent,

...and here are different pointers in use, can you unify to use the
physical device pointer (as per above) here as well?

> +                                      st->info->num_configs, sizeof(u64),
> +                                      GFP_KERNEL);
> +       if (!st->config_cnts)
> +               return -ENOMEM;

...

> +       case IIO_CHAN_INFO_SCALE:
> +               if (chan->type == IIO_TEMP) {
> +                       *val = 250000000;

MEGA?

> +                       *val2 = 800273203; /* (2^24 * 477) / 10 */

Why not write it as is:

                       *val2 = (BIT(24) * 477) / 10;

?

It will be more robust as we don't expect compiler to give different
results here

> +                       return IIO_VAL_FRACTIONAL;
> +               } else {
> +                       *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> +                       *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +                       return IIO_VAL_FRACTIONAL_LOG2;
> +               }

...

> +       /* If a regulator is not available, it will be set to a dummy regulator.
> +        * Each channel reference is checked with regulator_get_voltage() before
> +        *  setting attributes so if any channel uses a dummy supply the driver
> +        *  probe will fail.
> +        */

/*
 * Multi-line comments should follow this style. Find the
 * difference.
 */

...

> +               ref_label = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF];
> +
> +               fwnode_property_read_string(child, "adi,reference-select",
> +                                           &ref_label);
> +               ref_sel = match_string(ad7173_ref_sel_str,
> +                                      ARRAY_SIZE(ad7173_ref_sel_str), ref_label);
> +               if (ref_sel < 0) {

Can we use fwnode_property_match_property_string()?

> +                       fwnode_handle_put(child);
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "Invalid channel reference name %s\n",
> +                                            ref_label);
> +               }

-- 
With Best Regards,
Andy Shevchenko