| 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: <20250418153347.6d8e8057@jic23-huawei>
Date: Fri, 18 Apr 2025 15:33:47 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Antoniu Miclaus <antoniu.miclaus@...log.com>, robh@...nel.org,
conor+dt@...nel.org, linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/13] iio: adc: ad4080: add driver support
AD4080_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");
> > + } 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_RESERVED_CONFIG_A_MSK);
> > + else
> > + return regmap_write(st->regmap,
> > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > + AD4080_RESERVED_CONFIG_A_MSK |
> > + AD4080_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_RESERVED_CONFIG_A_MSK);
> > + if (ret)
> > + return ret;
> > + } else {
> > + ret = regmap_write(st->regmap,
> > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > + AD4080_RESERVED_CONFIG_A_MSK |
> > + AD4080_SPI_LVDS_LANES_MSK);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return -ETIMEDOUT;
>
> So, first the things that I don't really get (also the hdl docs need to be
> improved FWIW):
>
> * It seems that we can have data alignment or data capture synchronization
> through an internal AMD FPGA technique called bit-slip or an external signal,
> right? In here, it seems that we only use bit-slip and never disable it. Is that
> really the goal?
>
> * This whole process seems to be some kind of calibration at the interface
> level, right?
>
> * What's the point of the conv clock? Is it only used to get the sample rate? If
> so, the hdl docs are misleading as it seems that it can be used instead of bit-
> slip for data capture alignment?
>
> Now, speaking about the backend API's, it looks like that
> iio_backend_self_sync_enable() and iio_backend_data_alignment_enable() could be
> merged into one API. From what I can tell, iio_backend_data_alignment_enable()
> just enables the bit-slip process but that likely does nothing unless the
> SELF_SYNC bit is also set to bit-slip, right? In fact, we could think about a
> more generic (less flexible though) API like:
>
> * iio_backend_intf_data_align(back, timeout_us), or
> * iio_backend_intf_calib(back, timeout_us), or
> * iio_backend_intf_data_capture_sync(back, timeout_us)
>
> On the backend side, you could then enable bit-slip and do the status read (with
> timeout) and return success or an error code.
>
> The above seems to be pretty much what you're doing but in just one API that
> makes sense to me.
>
> Of course that the following questions still come to mind:
>
> * Do we need to disable bit-slip after we're done (still fits into the one API
> model)?
> * Do we need a meaningful API to change between the syncing/alignment methods?
> External signal vs bit-slip?
>
> The above is key to better think of an API. Right now it feels that you're just
> adding an API for every bit you want to control in this process...
>
> If we end up needing more flexibility for this, we can also consider the
> existing iio_backend_data_sample_trigger() API. I know is abusing a bit and a
> stretch but in the end of the day the whole thing is related with aligning,
> syncing, calibrating the interface for properly sampling data. Even bit-slip
> (while not a traditional external trigger) looks like some kind of self-
> adjusting, data-driven trigger mechanism to establish the correct starting point
> for capturing data. So having two new enums like:
>
> IIO_BACKEND_SAMPLE_TRIGGER_EXT_SIGNAL,
> IIO_BACKEND_SAMPLE_TRIGGER_BIT_SLIP // or maybe a more generic name like
> s/BIT_SLIP/INTERNAL
>
> I do not think the above is that horrible :P... But I would really like to get
> more opinions about this.
Seems reasonable to me to combine the two as long as we have good documentation.
Agreed it is kind of deriving a trigger from the signal so turning it on with
a trigger type selection doesn't seem unreasonable.
Name tricky though.
Jonathan
>
> > + }
> > +}
>
> ...
>
> >
> > +static const struct ad4080_chip_info ad4080_chip_info = {
> > + .name = "AD4080",
> > + .product_id = AD4080_CHIP_ID,
> > + .scale_table = ad4080_scale_table,
> > + .num_scales = ARRAY_SIZE(ad4080_scale_table),
> > + .num_channels = 1,
> > + .channels = ad4080_channels,
> > +};
> > +
> > +static int ad4080_setup(struct iio_dev *indio_dev)
> > +{
> > + struct ad4080_state *st = iio_priv(indio_dev);
> > + unsigned int id;
> > + int ret;
> > +
> > + ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> > + AD4080_SW_RESET);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> > + AD4080_SDO_ENABLE_MSK);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> > + if (ret)
> > + return ret;
> > +
> > + if (id != AD4080_CHIP_ID)
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "Unrecognized CHIP_ID 0x%X\n", id);
> > +
> > + ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> > + AD4080_GPO_1_EN);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> > + FIELD_PREP(AD4080_GPIO_1_SEL, 3));
> > + if (ret)
> > + return ret;
> > +
> > + ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> > + if (ret)
> > + return ret;
> > +
> > + ret = iio_backend_self_sync_enable(st->back);
> > + if (ret)
> > + return ret;
> > +
>
> AFAIU, the above is enabling bit-slip?
>
> > + if (st->lvds_cnv_en) {
> > + if (st->num_lanes) {
> > + ret = regmap_update_bits(st->regmap,
> > +
> > AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> > + AD4080_LVDS_CNV_CLK_CNT_MSK,
> > +
> > FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = regmap_set_bits(st->regmap,
> > + AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> > + AD4080_LVDS_CNV_EN_MSK);
> > + if (ret)
> > + return ret;
> > +
> > + return ad4080_lvds_sync_write(st);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void ad4080_properties_parse(struct ad4080_state *st)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
> > + "adi,lvds-cnv-enable");
> > +
>
> nit: I would probably drop the enable part. The property is about stating that
> the signal is LVDS instead of CMOS. And IIUC, this should also depend on the
> existence of `st->clk`
>
> > + st->num_lanes = 1;
> > + ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
> > + if (!ret)
> > + st->num_lanes = val;
> > +}
> > +
> > +static int ad4080_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct device *dev = &spi->dev;
> > + struct ad4080_state *st;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
> > +
> > + ret = devm_regulator_bulk_get_enable(dev,
> > +
> > ARRAY_SIZE(ad4080_power_supplies),
> > + ad4080_power_supplies);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to get and enable supplies\n");
> > +
> > + st->regmap = devm_regmap_init_spi(spi, &ad4080_regmap_config);
> > + if (IS_ERR(st->regmap))
> > + return PTR_ERR(st->regmap);
> > +
> > + st->info = spi_get_device_match_data(spi);
> > + if (!st->info)
> > + return -ENODEV;
> > +
> > + ret = devm_mutex_init(dev, &st->lock);
> > + if (ret)
> > + return ret;
> > +
> > + st->info = spi_get_device_match_data(spi);
> > + if (!st->info)
> > + return -ENODEV;
> > +
>
> The above is duplicated...
>
> > + indio_dev->name = st->info->name;
> > + indio_dev->channels = st->info->channels;
> > + indio_dev->num_channels = st->info->num_channels;
> > + indio_dev->info = &ad4080_iio_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ad4080_properties_parse(st);
> > +
> > + st->clk = devm_clk_get_enabled(&spi->dev, "cnv");
> > + if (IS_ERR(st->clk))
> > + return PTR_ERR(st->clk);
> > +
>
> From the datasheet it looks like this clock is optional? Moreover in the IP docs
> we have the following:
>
>
> "SELF_SYNC: Controls if the data capture synchronization is done through CNV
> signal or bit-slip."
>
> So I wonder, is the cnv clock meaning that we want to have capture
> sync/alignment through that external signal instead of bit-slip?
>
> - Nuno Sá
Powered by blists - more mailing lists