On Thu, Nov 23, 2023 at 05:23:22PM +0200, mitrutzceclan wrote:
> From: Dumitru Ceclan <[email protected]>
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 <[email protected]> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <[email protected]>
...
> 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
On Thu, Nov 23, 2023 at 06:47:26PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 23, 2023 at 05:23:22PM +0200, mitrutzceclan wrote:
...
> > +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?
Okay, it seems specific to MM, but maybe there something similar done which
can be split into generic LRU library? In any case it seems too much for this
nice series, so can you just add a comment on top of these functions to
mention that it may be switched to a generic LRU implementation if one exists?
--
With Best Regards,
Andy Shevchenko
>
> ...
>
> > + return vref / (MICRO/MILLI);
>
> Wouldn't MILLI in the denominator just suffice?
Just a quick comment here. Given this is converting from micro to milli units
I'd consider the maths here be acting as documentation of that which would be lost if
/MILLI only used. Need spaces around the / though
>
> ...
>
> > + 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;
> > + }
>
> > + 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
Personally I'd check for existence of property first and only try reading if it
exists. Avoid dance with resetting ret to 0.
> ret = 0;
> if (ret)
> return dev_err_probe(...);
>
>
> BUT. Isn't it a home grown variant of fwnode_property_match_property_string()?
true enough... I'd still add an existence check first given this one is optional.
Jonathan