Re: [PATCH v7 2/2] iio: adc: ad7173: add AD7173 driver
From: Andy Shevchenko
Date: Tue Dec 05 2023 - 10:29:19 EST
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