| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZVtYeWZmcDZ_SMPo@smile.fi.intel.com>
Date: Mon, 20 Nov 2023 15:00:41 +0200
From: Andy Shevchenko <andy@...nel.org>
To: mitrutzceclan <mitrutzceclan@...il.com>
Cc: linus.walleij@...aro.org, brgl@...ev.pl,
linux-gpio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
Jonathan Cameron <jic23@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Michael Walle <michael@...le.cc>,
Arnd Bergmann <arnd@...db.de>,
ChiaEn Wu <chiaen_wu@...htek.com>,
Niklas Schnelle <schnelle@...ux.ibm.com>,
Leonard Göhrs <l.goehrs@...gutronix.de>,
Mike Looijmans <mike.looijmans@...ic.nl>,
Haibo Chen <haibo.chen@....com>,
Hugo Villeneuve <hvilleneuve@...onoff.com>,
Ceclan Dumitru <dumitru.ceclan@...log.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] iio: adc: ad7173: add AD7173 driver
On Thu, Nov 16, 2023 at 03:46:55PM +0200, mitrutzceclan wrote:
> From: Dumitru Ceclan <mitrutzceclan@...il.com>
>
> 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.
...
> + help
> + Say yes here to build support for Analog Devices AD7173 and similar ADC
> + (currently supported: AD7172-2, AD7173-8, AD7175-2, AD7176-2).
This is hard to maintain, list it one model per a single line.
> + To compile this driver as a module, choose M here: the module will be
> + called ad7173.
...
+ array_size.h
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/bits.h>
This is guaranteed to be included by one from the above (don't remember
by heart which one or even both).
+ container_of.h
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
How is this being used (as not a proxy)?
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
...
> +#define AD7173_CH_ADDRESS(pos, neg) \
> + (FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) |\
Space before \ here and everywhere else in multi-line definitions.
> + FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
...
> +#define AD7173_VOLTAGE_INT_REF_MICROV 2500000
MICROV --> uV (yes, with small letter), it's a common use for Amperes, Volts,
etc.
...
> +struct ad7173_channel_config {
> + bool live;
> + u8 cfg_slot;
> + /* Following fields are used to compare equality. Bipolar must be first */
> + bool bipolar;
> + bool input_buf;
> + u8 odr;
> + u8 ref_sel;
If you group better by types, it might save a few bytes on the architectures /
compilers where bool != byte.
> +};
...
> + st->reg_gpiocon_regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
> + if (IS_ERR(st->reg_gpiocon_regmap)) {
> + return dev_err_probe(dev, PTR_ERR(st->reg_gpiocon_regmap),
> + "Unable to init regmap\n");
> + }
{} are not needed, can also be written as
st->reg_gpiocon_regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
ret = PTR_ERR_OR_ZERO(st->reg_gpiocon_regmap);
if (ret)
return dev_err_probe(dev, ret, "Unable to init regmap\n");
...
> + st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
> + if (IS_ERR(st->gpio_regmap)) {
> + return dev_err_probe(dev, PTR_ERR(st->gpio_regmap),
> + "Unable to init gpio-regmap\n");
> + }
Ditto.
...
> +static struct ad7173_channel_config *ad7173_find_live_config
> + (struct ad7173_state *st, struct ad7173_channel_config *cfg)
This is strange indentation.
Perhaps
static struct ad7173_channel_config *
ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
?
...
> + offset = offsetof(struct ad7173_channel_config, cfg_slot) +
> + sizeof(cfg->cfg_slot);
Isn't it a offsetofend() from stddef.h?
> + cmp_size = sizeof(*cfg) - offset;
sizeof_field() from the above mentioned header?
...
> + for (i = 0; i < st->num_channels; i++) {
> + cfg_aux = &st->channels[i].cfg;
> +
> + if (cfg_aux->live && !memcmp(&cfg->bipolar, &cfg_aux->bipolar,
> + cmp_size))
I would split this on logic operator, it will be easier to read.
> + return cfg_aux;
> + }
...
> + free_cfg_slot = find_first_zero_bit(st->cfg_slots_status,
> + st->info->num_configs);
> + if (free_cfg_slot == st->info->num_configs)
> + free_cfg_slot = ad7173_free_config_slot_lru(st);
> +
> + set_bit(free_cfg_slot, st->cfg_slots_status);
> + cfg->cfg_slot = free_cfg_slot;
Looks like reinvention of IDA.
...
> + struct ad7173_state *st = iio_priv(indio_dev);
> + unsigned int id;
> + u8 buf[AD7173_RESET_LENGTH];
> + int ret;
Reversed xmas tree order?
struct ad7173_state *st = iio_priv(indio_dev);
u8 buf[AD7173_RESET_LENGTH];
unsigned int id;
int ret;
...
> + return vref / (MICRO/MILLI);
What does the denominator mean and why you can't simply use MILL?
...
> + if (ch->cfg.bipolar)
> + /* (1<<31) is UB for a 32bit channel */
> + *val = (chan->scan_type.realbits == 32) ?
> + INT_MIN :
> + -(1 << (chan->scan_type.realbits - 1));
So, what's the issue to use BIT() which has no such issue with UB?
> + else
> + *val = 0;
...
> + *val = st->info->sinc5_data_rates[reg] / (MICRO/MILLI);
> + *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
Same Q about denominator.
...
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + freq = val * MILLI + val2 / MILLI;
> +
Unneeded blank line.
> + for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
> + if (freq >= st->info->sinc5_data_rates[i])
> + break;
> + }
> +
> + cfg = &st->channels[chan->address].cfg;
> + cfg->odr = i;
> +
> + if (!cfg->live)
> + break;
> +
> + ret = ad_sd_read_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, ®);
> + if (ret)
> + break;
> + reg &= ~AD7173_FILTER_ODR0_MASK;
> + reg |= FIELD_PREP(AD7173_FILTER_ODR0_MASK, i);
> + ret = ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, reg);
> + break;
...
> +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 = 0;
Use the 0 directly...
> +
> + 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);
> +
> + if (ret < 0)
> + return ret;
> + }
> +
> + return ret;
...here.
> +}
> + chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels,
> + GFP_KERNEL);
One line.
> + if (!chan_arr)
> + return -ENOMEM;
...
> + if (fwnode_property_read_u32(child, "adi,reference-select", &ref_sel))
> + ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
if is redundant.
ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
fwnode_property_read_u32(child, "adi,reference-select", &ref_sel);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists