| 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: <CAHp75VfwCLPAD84Rqt7hVC123U7=-GiyCemuQwnMCeR18fmRew@mail.gmail.com>
Date: Fri, 12 Sep 2025 09:00:05 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, robh@...nel.org, conor+dt@...nel.org, krzk+dt@...nel.org,
linux-iio@...r.kernel.org, s32@....com, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, chester62515@...il.com, mbrugger@...e.com,
ghennadi.procopciuc@....nxp.com
Subject: Re: [PATCH v2 2/2] iio: adc: Add the NXP SAR ADC support for the
s32g2/3 platforms
On Wed, Sep 10, 2025 at 6:58 PM Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
>
> The NXP S32G2 and S32G3 platforms integrate a successive approximation
> register (SAR) ADC. Two instances are available, each providing 8
> multiplexed input channels with 12-bit resolution. The conversion rate
> is up to 1 Msps depending on the configuration and sampling window.
>
> The SAR ADC supports raw, buffer, and trigger modes. It can operate
> in both single-shot and continuous conversion modes, with optional
> hardware triggering through the cross-trigger unit (CTU) or external
> events. An internal prescaler allows adjusting the sampling clock,
> while per-channel programmable sampling times provide fine-grained
> trade-offs between accuracy and latency. Automatic calibration is
> performed at probe time to minimize offset and gain errors.
>
> The driver is derived from the BSP implementation and has been partly
> rewritten to comply with upstream requirements. For this reason, all
> contributors are listed as co-developers, while the author refers to
> the initial BSP driver file creator.
>
> All modes have been validated on the S32G274-RDB2 platform using an
> externally generated square wave captured by the ADC. Tests covered
> buffered streaming via IIO, trigger synchronization, and accuracy
> verification against a precision laboratory signal source.
...
> +#include <linux/bitops.h>
> +#include <linux/circ_buf.h>
Is it used?
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
+ err.h
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
+ minmax.h
> +#include <linux/module.h>
> +#include <linux/of.h>
Wrong header, should be property.h.
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
+ time.h
> +#include <linux/types.h>
> +#include <linux/units.h>
...
> +#define REG_ADC_CTR_INPSAMP_MIN 8
> +#define REG_ADC_CTR_INPSAMP_MAX 0xFF
First of all, the value is written in a different style. Be
consistent. Second, is this limited by a bit field resolution? If so,
form (BIT($x) - 1) is better as it shows the limit in $x ($x should be
replaced with the actual value).
...
> +struct nxp_sar_adc {
> + void __iomem *regs;
> + phys_addr_t regs_phys;
> + struct clk *clk;
> +
> + u16 value;
> + u32 vref_mV;
> + u8 current_channel;
> + u8 channels_used;
Run `pahole` and act accordingly.
> + struct completion completion;
> +
> + u16 buffer[NXP_SAR_ADC_IIO_BUFF_SZ];
> + u16 buffered_chan[NXP_SAR_ADC_NR_CHANNELS];
> +
> + struct circ_buf dma_buf;
> + struct dma_chan *dma_chan;
> + struct dma_slave_config dma_config;
> + dma_addr_t rx_dma_buf;
> + dma_cookie_t cookie;
> +
> + /* Protect circular buffers access. */
> + spinlock_t lock;
> +
> + /*
> + * Save and restore context
> + */
> + u32 inpsamp;
> + u32 pwdn;
> +};
...
> +static bool __nxp_sar_adc_enable(struct nxp_sar_adc *info, bool enable)
> +{
> + u32 mcr;
> + bool pwdn;
> +
> + mcr = readl(REG_ADC_MCR(info->regs));
> + /*
> + * Return the current state
> + */
This is perfectly a single line comment. Same applies to a few
comments in the code.
> + pwdn = mcr & REG_ADC_MCR_PWDN;
> +
> + if (enable)
> + mcr &= ~REG_ADC_MCR_PWDN;
> + else
> + mcr |= REG_ADC_MCR_PWDN;
> +
> + writel(mcr, REG_ADC_MCR(info->regs));
> +
> + /*
> + * Ensure there are at least three cycles between the
> + * configuration of NCMR and the setting of NSTART
> + */
Missing period at the end. Same applies to a few comments in the code.
> + if (enable)
> + ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3U));
Do you need 'U'? AFAIR frequency can't be negative. Same applies to a
few cases in the code.
> + return pwdn;
> +}
> + ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
...
> + ret = wait_for_completion_interruptible_timeout
> + (&info->completion,
> + msecs_to_jiffies(NXP_SAR_ADC_CONV_TIMEOUT_MS));
This is bad style, please reindent in a better way.
...
> +static int sar_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct nxp_sar_adc *info = iio_priv(indio_dev);
> + u32 inpsamp;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + /*
> + * Configures the sample period duration in terms of
> + * the SAR controller clock. The minimum acceptable
> + * value is 8. Configuring to a value lower than 8
Configuring it to
> + * sets the sample period to 8 cycles. We read the
> + * clock value and divide by the sampling timing which
> + * gives us the number of cycles expected. The value
> + * is 8 bits wide, consequently the max value is 0xFF
> + */
> + inpsamp = clk_get_rate(info->clk) / val - NXP_SAR_ADC_CONV_TIME;
> + nxp_sar_adc_conversion_timing_set(info, inpsamp);
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
...
> + /* If everything transferred, avoid an off by one error. */
was/is transfered.
...
> + /* dma_buf->tail != dma_buf->head condition will become false
> + * because dma_buf->tail will be incremented with 1.
> + */
Wrong multi-line comment style. Driver was written by a few people?
...
> +/*
> + * The documentation describes the reset values for the
> + * registers. However some registers do not have these values after a
> + * reset. It is a not desirable situation. In some other SoC family
It is not a
> + * documentation NXP recommend to not assume the default values are
recommends
assuming
> + * set and to initialize the registers conforming to the documentation
> + * reset information to prevent this situation. Assume the same rule
> + * applies here as there is a discrepancy between what is read from
> + * the registers at reset time and the documentation.
> + */
...
> +static int nxp_sar_adc_probe(struct platform_device *pdev)
> +{
> + const struct nxp_sar_adc_data *data;
> + struct nxp_sar_adc *info;
> + struct iio_dev *indio_dev;
> + struct resource *mem;
> + struct device *dev = &pdev->dev;
> + int irq;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(struct nxp_sar_adc));
> + if (!indio_dev)
> + return -ENOMEM;
> + info = iio_priv(indio_dev);
> +
> + data = device_get_match_data(dev);
> +
> + info->vref_mV = data->vref_mV;
Instead of split, move data assignment to be before indio_dev allocation.
> + info->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> + if (IS_ERR(info->regs))
> + return dev_err_probe(dev, PTR_ERR(info->regs),
> + "failed to get and remap resource");
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> + dev_name(dev), indio_dev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
> +
> + info->regs_phys = mem->start;
> + spin_lock_init(&info->lock);
> +
> + info->clk = devm_clk_get_enabled(dev, "adc");
> + if (IS_ERR(info->clk))
> + return dev_err_probe(dev, PTR_ERR(info->clk),
> + "failed to get the clock\n");
> + platform_set_drvdata(pdev, indio_dev);
> + init_completion(&info->completion);
> +
> + indio_dev->name = dev_name(dev);
> + indio_dev->info = &nxp_sar_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> + indio_dev->channels = nxp_sar_adc_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(nxp_sar_adc_iio_channels);
> +
> + nxp_sar_adc_set_default_values(info);
> +
> + ret = nxp_sar_adc_calibration(info);
> + if (ret) {
> + dev_err(dev, "Calibration failed: %d\n", ret);
> + return ret;
return dev_err_probe(...);
> + }
> +
> + ret = nxp_sar_adc_dma_probe(dev, info);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize the dma\n");
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &nxp_sar_adc_trigger_handler,
> + &iio_triggered_buffer_setup_ops);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Couldn't initialise the buffer\n");
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Couldn't register the device\n");
> +
> + return 0;
> +}
> +
> +static void nxp_sar_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct nxp_sar_adc *info = iio_priv(indio_dev);
> +
> + nxp_sar_adc_stop_conversion(info);
> + nxp_sar_adc_channels_disable(info, REG_ADC_CH_MASK);
> + nxp_sar_adc_dma_channels_disable(info, REG_ADC_CH_MASK);
> + nxp_sar_adc_dma_cfg(info, false);
> + nxp_sar_adc_disable(info);
> + dmaengine_terminate_sync(info->dma_chan);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists