Messages in this thread | | | Date | Thu, 2 Feb 2023 15:37:31 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 3/5] drivers: iio: adc: Add ADI MAX77541 ADC Support |
| |
On Wed, Feb 01, 2023 at 01:35:16PM +0300, Okan Sahin wrote: > The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC > with four multiplexers for supporting the telemetry feature.
...
> +struct max77541_adc_iio { > + struct regmap *regmap; > +};
Is it really needed? See below.
...
> +enum max77541_adc_channel { > + MAX77541_ADC_VSYS_V = 0,
0 is the default in C enums, why do you need this explicitly? If these are HW related, you need to assign them all to be crystal clear.
> + MAX77541_ADC_VOUT1_V, > + MAX77541_ADC_VOUT2_V, > + MAX77541_ADC_TEMP, > +};
...
> + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
> +
Redundant blank line.
> + switch (reg_val) { > + case LOW_RANGE: > + *val = 6; > + *val2 = 250000; > + break; > + case MID_RANGE: > + *val = 12; > + *val2 = 500000; > + break; > + case HIGH_RANGE: > + *val = 25; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + }
...
struct regmap **regmap;
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
sizeof(*regmap)
> + if (!indio_dev) > + return -ENOMEM;
> + info = iio_priv(indio_dev);
regmap = iio_priv(...);
> + info->regmap = dev_get_regmap(dev->parent, NULL);
*regmap = ...;
It won't probably gain anything in the code generation or memory footprint, so up to you. But in general depending on API it might be a benefit.
...
> +static const struct platform_device_id max77541_adc_platform_id[] = { > + { "max77541-adc", },
Inner comma is not needed.
> + { /* sentinel */ } > +};
-- With Best Regards, Andy Shevchenko
| |