Message-ID: <6zn53fgyiwtm5ad5piyt32uxcwenwgkhwhantizsjytwbf42ts@4pg6hkna3yah>
Date: Fri, 16 May 2025 12:11:56 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Jorge Marques <jorge.marques@...log.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, 
	David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 5/5] iio: adc: add support for ad4052

Hello,

On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> +{
> +	struct pwm_state pwm_st;
> +
> +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> +		return -EINVAL;
> +
> +	pwm_get_state(st->cnv_pwm, &pwm_st);
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);

Is it clear that pwm_st.duty_cycle isn't greater than
DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);

I'm not a big fan of pwm_get_state() because the semantic is a bit
strange. My preferred alternative would be to either use pwm_init_state
and initialize all fields, or maintain a struct pwm_state in struct
ad4052_state.

> +}
> +
> +static int ad4052_soft_reset(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> +	for (int i = 0; i < 3; i++)
> +		st->buf_reset_pattern[6 * (i + 1) - 1] = 0xFE;
> +
> +	ret = spi_write(st->spi, st->buf_reset_pattern,
> +			sizeof(st->buf_reset_pattern));
> +	if (ret)
> +		return ret;
> +
> +	/* Wait AD4052 reset delay */
> +	fsleep(5000);
> +
> +	return 0;
> +}
> +
> +static int ad4052_setup(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	u8 val = FIELD_PREP(AD4052_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = 0;
> +	if (scan_type->sign == 's')
> +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> +
> +	st->data_format = val;
> +
> +	if (st->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> +					      AD4052_TIMER_CONFIG_300KSPS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> +}
> +
> +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	iio_push_event(indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> +					    IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
> +{
> +	struct ad4052_state *st = private;
> +
> +	complete(&st->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad4052_request_irq(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	int ret = 0;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	ret = devm_request_threaded_irq(dev, ret, NULL,
> +					ad4052_irq_handler_thresh,
> +					IRQF_ONESHOT, indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	st->gp1_irq = ret;
> +	return devm_request_threaded_irq(dev, ret, NULL,
> +					 ad4052_irq_handler_drdy,
> +					 IRQF_ONESHOT, indio_dev->name,
> +					 st);
> +}
> +
> +static const int ad4052_oversampling_avail[] = {
> +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> +};
> +
> +static int ad4052_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, const int **vals,
> +			     int *type, int *len, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4052_oversampling_avail;
> +		*len = ARRAY_SIZE(ad4052_oversampling_avail);
> +		*type = IIO_VAL_INT;
> +
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> +{
> +	struct spi_device *spi = st->spi;
> +	int ret;
> +	struct spi_transfer t_cnv = {
> +		.len = 0
> +	};
> +
> +	reinit_completion(&st->completion);
> +
> +	if (st->cnv_gp) {
> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> +	} else {
> +		ret = spi_sync_transfer(spi, &t_cnv, 1);
> +		if (ret)
> +			return ret;
> +	}
> +	/*
> +	 * Single sample read should be used only for oversampling and
> +	 * sampling frequency pairs that take less than 1 sec.
> +	 */
> +	ret = wait_for_completion_timeout(&st->completion,
> +					  msecs_to_jiffies(1000));
> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (st->xfer.len == 2) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 15);
> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 23);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ad4052_read_chan_raw(struct iio_dev *indio_dev, int *val)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = __ad4052_read_chan_raw(st, val);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_exit_command(st);
> +
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +	return ret;
> +}
> +
> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);

Is this the expected semantic? I.e. if the PWM isn't running report
sample freq assuming the last set period (or if the pwm wasn't set
before the configured period length set by the bootloader, or the value
specified in the device tree)?

> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> [...]
> +static int ad4052_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct spi_offload_trigger_config config = {
> +		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	};
> +	int ret;
> +
> +	if (st->wait_event)
> +		return -EBUSY;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
> +	if (ret)
> +		goto out_error;
> +
> +	/* SPI Offload handles the data ready irq */
> +	disable_irq(st->gp1_irq);
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
> +	if (ret)
> +		goto out_offload_error;
> +
> +	ret = pwm_enable(st->cnv_pwm);
> +	if (ret)
> +		goto out_pwm_error;

pwm_enable() is another disguised pwm_get_state().

> +
> +	return 0;
> +
> +out_pwm_error:
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +out_offload_error:
> +	enable_irq(st->gp1_irq);
> +	spi_unoptimize_message(&st->offload_msg);
> +	ad4052_exit_command(st);
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	return ret;
> +}

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists