[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 2 Feb 2023 15:37:31 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Okan Sahin <okan.sahin@...log.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Lee Jones <lee@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Caleb Connolly <caleb.connolly@...aro.org>,
Marcus Folkesson <marcus.folkesson@...il.com>,
Ramona Bolboaca <ramona.bolboaca@...log.com>,
ChiYuan Huang <cy_huang@...htek.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
William Breathitt Gray <william.gray@...aro.org>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org
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
Powered by blists - more mailing lists