| 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:
<CY4PR03MB3399A9584A631E5C30CF72EC9B832@CY4PR03MB3399.namprd03.prod.outlook.com>
Date: Wed, 30 Apr 2025 12:31:51 +0000
From: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
To: David Lechner <dlechner@...libre.com>
CC: "jic23@...nel.org" <jic23@...nel.org>,
"robh@...nel.org"
<robh@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 11/11] iio: adc: ad4080: add driver support
...
> > + unsigned int num_channels;
> > +};
>
> I guess this is preparing the driver to support more than one chip?
>
Yes. It is stated also in the cover letter.
> > +
> > +struct ad4080_state {
> > + struct spi_device *spi;
>
> It looks like this is only ever used to get &spi->dev. We could drop this and
> get dev from regmap instead.
How can I get the dev from regmap?
> > + struct regmap *regmap;
> > + struct clk *clk;
> > + struct iio_backend *back;
> > + const struct ad4080_chip_info *info;
> > + /*
> > + * Synchronize access to members the of driver state, and ensure
> > + * atomicity of consecutive regmap operations.
> > + */
> > + struct mutex lock;
> > + unsigned int num_lanes;
> > + unsigned int dec_rate;
> > + enum ad4080_filter_type filter_type;
> > + bool lvds_cnv_en;
> > +};
> > +
> > +static const struct regmap_config ad4080_regmap_config = {
> > + .reg_bits = 16,
> > + .val_bits = 8,
> > + .read_flag_mask = BIT(7),
> > + .max_register = 0x29,
> > +};
> > +
> > +static int ad4080_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> > + unsigned int writeval, unsigned int *readval)
> > +{
> > + struct ad4080_state *st = iio_priv(indio_dev);
> > +
>
> Missing guard(mutex)(&st->lock); ?
Aren't regmap operations thread safe? (own internal locking).
> > + if (readval)
> > + return regmap_read(st->regmap, reg, readval);
> > +
> > + return regmap_write(st->regmap, reg, writeval);
> > +}
> > +
> > +static int ad4080_get_scale(struct ad4080_state *st, int *val, int *val2)
> > +{
> > + unsigned int tmp;
> > +
> > + tmp = (st->info->scale_table[0][0] * 1000000ULL) >>
> > + st->info->channels[0].scan_type.realbits;
> > + *val = tmp / 1000000;
> > + *val2 = tmp % 1000000;
> > +
> > + return IIO_VAL_INT_PLUS_NANO;
>
> Seems like this could be simplifed by using IIO_VAL_FRACTIONAL_LOG2
> instead.
>
> > +}
> > +
> > +static unsigned int ad4080_get_dec_rate(struct iio_dev *dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct ad4080_state *st = iio_priv(dev);
> > + int ret;
> > + unsigned int data;
> > +
>
> Missing guard(mutex)(&st->lock); ?
>
> > + ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG,
> &data);
> > + if (ret)
> > + return ret;
> > +
> > + return (1 <<
> (FIELD_GET(AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK, data) + 1));
>
> nit: doen't need outermost ().
>
> > +}
> > +
> > +static int ad4080_set_dec_rate(struct iio_dev *dev,
> > + const struct iio_chan_spec *chan,
> > + unsigned int mode)
> > +{
> > + struct ad4080_state *st = iio_priv(dev);
> > + int ret;
> > +
>
> Don't we need to check for < 2 as well?
>
> > + if (st->filter_type >= SINC_5 && mode >= 512)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&st->lock);
> > + ret = regmap_update_bits(st->regmap, AD4080_REG_FILTER_CONFIG,
> > +
> AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK,
> > +
> FIELD_PREP(AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK,
> > + (ilog2(mode) - 1)));
>
> Otherwise ilog2(mode) - 1 could be < 0.
>
> > + if (ret)
> > + return ret;
> > +
> > + st->dec_rate = mode;
>
> This saves the value the user entered, not what the hardware is actually doing.
> It should be saving the power of 2 value instead.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int ad4080_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long m)
> > +{
> > + struct ad4080_state *st = iio_priv(indio_dev);
> > + int dec_rate;
> > +
> > + switch (m) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return ad4080_get_scale(st, val, val2);
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + if (st->filter_type == SINC_5_COMP)
> > + dec_rate = st->dec_rate * 2;
> > + else
> > + dec_rate = st->dec_rate;
>
> As a concequence of the above, this will return incorrect information if the
> user didn't enter an exact value.
Isn't the user constrained by the ad4080_read_avail for entering the dec_rate values?
The user both writes and reads the actual decimation rate value. The conversions are done inside the functions.
>
> > + if (st->filter_type)
> > + *val = DIV_ROUND_CLOSEST(clk_get_rate(st->clk),
> dec_rate);
> > + else
> > + *val = clk_get_rate(st->clk);
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + *val = ad4080_get_dec_rate(indio_dev, chan);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad4080_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return -EINVAL;
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + return -EINVAL;
>
> Can leave these 2 out and just let them fall through to the default.
>
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + return ad4080_set_dec_rate(indio_dev, chan, val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> > +{
> > + unsigned int timeout = 100;
> > + bool sync_en;
> > + int ret;
>
> nit: some comments in this function would be helpful to readers not familiar
> with the part.
>
> > +
> > + guard(mutex)(&st->lock);
> > + if (st->num_lanes == 1)
> > + ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_INTF_CHK_EN_MSK);
> > + else
> > + ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_INTF_CHK_EN_MSK |
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES_MSK);
> > + if (ret)
> > + return ret;
> > +
> > + ret = iio_backend_data_alignment_enable(st->back);
> > + if (ret)
> > + return ret;
> > +
> > + do {
> > + ret = iio_backend_sync_status_get(st->back, &sync_en);
> > + if (ret)
> > + return ret;
> > +
> > + if (!sync_en)
> > + dev_dbg(&st->spi->dev, "Not Locked: Running Bit
> Slip\n");
> > +
> > + fsleep(500);
> > + } while (--timeout && !sync_en);
> > +
> > + if (timeout) {
> > + dev_info(&st->spi->dev, "Success: Pattern correct and
> Locked!\n");
> > + if (st->num_lanes == 1)
> > + return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK);
> > + else
> > + return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES_MSK);
> > + } else {
> > + dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> > + if (st->num_lanes == 1) {
> > + ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK);
> > + if (ret)
> > + return ret;
> > + } else {
> > + ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_RESERVED_CONFIG_A_MSK |
> > +
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES_MSK);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return -ETIMEDOUT;
> > + }
> > +}
> > +
> > +static ssize_t ad4080_get_filter_type(struct iio_dev *dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct ad4080_state *st = iio_priv(dev);
> > + unsigned int data;
> > + int ret;
> > +
>
> Missing guard(mutex)(&st->lock); ?
>
> > + ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG,
> &data);
> > + if (ret)
> > + return ret;
> > +
> > + return FIELD_GET(AD4080_FILTER_CONFIG_FILTER_SEL_MSK, data);
> > +}
> > +
>
> ...
>
> > +static struct iio_chan_spec_ext_info ad4080_ext_info[] = {
> > + IIO_ENUM("filter_type", IIO_SHARED_BY_ALL,
> &ad4080_filter_type_enum),
> > + IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
> > + &ad4080_filter_type_enum),
> > + { }
> > +};
> > +
> > +static const struct iio_chan_spec ad4080_channels[] = {
>
> Array with one element doesn't make sense. It can just be a single struct.
Isn't indio_dev->channels expecting an array?
> > + {
> > + .type = IIO_VOLTAGE,
> > + .indexed = 1,
> > + .channel = 0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_shared_by_all =
> BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> > + .info_mask_shared_by_all_available =
> > +
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> > + .ext_info = ad4080_ext_info,
> > + .scan_index = 0,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 20,
> > + .storagebits = 32,
> > + .shift = 0,
> > + },
> > + }
> > +};
> > +
Powered by blists - more mailing lists