On Tue, Dec 5, 2023 at 3:46 PM Dumitru Ceclan <[email protected]> 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
On Tue, Dec 05, 2023 at 05:28:34PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 5, 2023 at 3:46 PM Dumitru Ceclan <[email protected]> wrote:
...
> > + case IIO_CHAN_INFO_SCALE:
> > + if (chan->type == IIO_TEMP) {
> > + *val = 250000000;
>
> MEGA?
Or MICRO?
--
With Best Regards,
Andy Shevchenko
On 12/5/23 17:28, Andy Shevchenko wrote:
>> + 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_property_match_string() searches a given string in a device-tree
string array and returns the index. I do not think that this function
fits here as the DT attribute is a single string.
On Tue, Dec 05, 2023 at 06:12:18PM +0200, Ceclan Dumitru wrote:
> On 12/5/23 17:28, Andy Shevchenko wrote:
> >> + 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_property_match_string() searches a given string in a device-tree
> string array and returns the index. I do not think that this function
> fits here as the DT attribute is a single string.
I'm not talking about that. I mentioned different API call.
/**
* fwnode_property_match_property_string - find a property string value in an array and return index
* @fwnode: Firmware node to get the property of
* @propname: Name of the property holding the string value
* @array: String array to search in
* @n: Size of the @array
*
* Find a property string value in a given @array and if it is found return
* the index back.
*
* Return: index, starting from %0, if the string value was found in the @array (success),
* %-ENOENT when the string value was not found in the @array,
* %-EINVAL if given arguments are not valid,
* %-ENODATA if the property does not have a value,
* %-EPROTO or %-EILSEQ if the property is not a string,
* %-ENXIO if no suitable firmware interface is present.
*/
--
With Best Regards,
Andy Shevchenko
On Tue, 5 Dec 2023 19:18:12 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 05, 2023 at 06:12:18PM +0200, Ceclan Dumitru wrote:
> > On 12/5/23 17:28, Andy Shevchenko wrote:
> > >> + 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_property_match_string() searches a given string in a device-tree
> > string array and returns the index. I do not think that this function
> > fits here as the DT attribute is a single string.
>
> I'm not talking about that. I mentioned different API call.
>
> /**
> * fwnode_property_match_property_string - find a property string value in an array and return index
> * @fwnode: Firmware node to get the property of
> * @propname: Name of the property holding the string value
> * @array: String array to search in
> * @n: Size of the @array
> *
> * Find a property string value in a given @array and if it is found return
> * the index back.
> *
> * Return: index, starting from %0, if the string value was found in the @array (success),
> * %-ENOENT when the string value was not found in the @array,
> * %-EINVAL if given arguments are not valid,
> * %-ENODATA if the property does not have a value,
> * %-EPROTO or %-EILSEQ if the property is not a string,
> * %-ENXIO if no suitable firmware interface is present.
> */
>
Which is in the togreg branch of iio.git (was a patch from Andy that I've queued up)
Jonathan