On Thu, Nov 23, 2023 at 05:23:22PM +0200, mitrutzceclan wrote:
> From: Dumitru Ceclan <mitrutzceclan@xxxxxxxxx>
Thank you for the update!
My comments below.
> 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.
>
>
One blank line is enough here.
> Reviewed-by: Michael Walle <michael@xxxxxxxx> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@xxxxxxxxx>
...
> V5->V6
> - No changes
Don't issue patches too often (minimum gap between versions is 24h).
...
> + 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
I would use
- FOO
- BAR
style that will reduce amount of potential churn if you need to add an entry at
the end of this list.
> + To compile this driver as a module, choose M here: the module will be
> + called ad7173.
...
> +#include <linux/stddef.h>
You probably meant types.h here (it will include stddef, at least most of
the code relies on that), which is currently absent.
...
> +struct ad7173_device_info {
> + char *name;
> + unsigned int id;
> + unsigned int num_inputs;
> + unsigned int num_configs;
> + unsigned int num_channels;
> + unsigned char num_gpios;
I would use u8 as you have done for cfg_slot, for example. As it holds a number
and not a real character.
> + bool has_temp;
> + unsigned int clock;
> +
> + const unsigned int *sinc5_data_rates;
> + unsigned int num_sinc5_data_rates;
> +};
...
> +struct ad7173_channel_config {
> + u8 cfg_slot;
> + bool live;
Perhaps a blank line?
> + /* Following fields are used to compare equality. */
> + struct_group(config_props,
> + bool bipolar;
> + bool input_buf;
> + u8 odr;
> + u8 ref_sel;
> + );
> +};
...
> +struct ad7173_state {
> + const struct ad7173_device_info *info;
> + struct ad_sigma_delta sd;
It might be better to embed that struct first. In any case you always can
consult with `pahole` tool for data structure layouts.
> + struct ad7173_channel *channels;
> + struct regulator_bulk_data regulators[3];
> + unsigned int adc_mode;
> + unsigned int interface_mode;
> + unsigned int num_channels;
> + struct ida cfg_slots_status;
> + unsigned long long config_usage_counter;
> + unsigned long long *config_cnts;
> +#if IS_ENABLED(CONFIG_GPIOLIB)
> + struct regmap *reg_gpiocon_regmap;
> + struct gpio_regmap *gpio_regmap;
> +#endif
> +};
...
> +static const char *const ad7173_ref_sel_str[] = {
> + [AD7173_SETUP_REF_SEL_EXT_REF] = "refin",
> + [AD7173_SETUP_REF_SEL_EXT_REF2] = "refin2",
> + [AD7173_SETUP_REF_SEL_INT_REF] = "refout-avss",
> + [AD7173_SETUP_REF_SEL_AVDD1_AVSS] = "avdd"
Leave trailing comma here as well.
> +};
...
> + struct device *dev = &st->sd.spi->dev;
For example, here st->sd become a no-op at compile time (see above about
placing sd to be the first member). The code generation can be checked
(for the size) by bloat-o-meter.
...
> +static int ad7173_free_config_slot_lru(struct ad7173_state *st)
> +static int ad7173_load_config(struct ad7173_state *st,
> + struct ad7173_channel_config *cfg)
Have you checked, btw, list_lru.h? Maybe all this can be simply changed by
using existing library?
...
> + return vref / (MICRO/MILLI);
Wouldn't MILLI in the denominator just suffice?
...
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + reg = st->channels[chan->address].cfg.odr;
> +
> + *val = st->info->sinc5_data_rates[reg] / MILLI;
> + *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;
Make this 'default' case.
...
> +static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> + int i, ret;
> +
> + for (i = 0; i < indio_dev->num_channels; i++) {
> + if (test_bit(i, scan_mask))
> + ret = ad7173_set_channel(&st->sd, i);
> + else
> + ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);
> +
Unneeded blank line.
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +static int ad7173_debug(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
Hmm... The function suggests it debugs something or helps with debugging
something. Without actual description is hard to understand the purpose.
Can you add a top comment on this function with explanations?
...
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> + st->regulators);
One line?
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to get regulators\n");
...
> + return dev_err_probe(dev, -EINVAL,
> + "Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
Seems broken indentation.
...
> + ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> + if (!ret) {
> + for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> + if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> + ref_sel = i;
> + break;
> + }
> + if (i == ARRAY_SIZE(ad7173_ref_sel_str))
> + return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);
Too long line.
> + } else if (ret != -EINVAL) {
> + return dev_err_probe(dev, ret, "Invalid channel reference value");
> + }
Use standard pattern and it will be easier to see that 'else' is redundant.
if (ret == -EINVAL) // However I don't like this handling of
// properties, but up to you and maintainer
ret = 0;
if (ret)
return dev_err_probe(...);
BUT. Isn't it a home grown variant of fwnode_property_match_property_string()?
...
> + ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);
Why casting?
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Cannot use reference %u", ref_sel);
...
> + return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8");
Missing \n. Check all your messages that they are terminated with \n.
...
> + struct ad7173_state *st;
> + struct iio_dev *indio_dev;
> + struct device *dev = &spi->dev;
> + int ret;
Reversed xmas tree order?
struct device *dev = &spi->dev;
struct iio_dev *indio_dev;
struct ad7173_state *st;
int ret;
...
> +static const struct of_device_id ad7173_of_match[] = {
> + { .compatible = "adi,ad7172-2",
> + .data = &ad7173_device_info[ID_AD7172_2], },
> + { .compatible = "adi,ad7173-8",
> + .data = &ad7173_device_info[ID_AD7173_8], },
> + { .compatible = "adi,ad7175-2",
> + .data = &ad7173_device_info[ID_AD7175_2], },
> + { .compatible = "adi,ad7176-2",
> + .data = &ad7173_device_info[ID_AD7176_2], },
Last inner commas are not needed.
> + { }
> +};
...
> +static const struct spi_device_id ad7173_id_table[] = {
> + { "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2], },
> + { "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8], },
> + { "ad7175-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_2], },
> + { "ad7176-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7176_2], },
Ditto.
> + { }
> +};
--
With Best Regards,
Andy Shevchenko
