linux-kernel - Re: [PATCH v2 5/5] iio: adc: add support for ad4052

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <nz2o4fi5geowbki3flpou2ccs4hfjr356qmfx763u6lilrgp33@72bj5i7qqark>
Date: Mon, 2 Jun 2025 12:38:27 +0200
From: Jorge Marques <gastmaier@...il.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	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

Hi Uwe,

On Fri, May 16, 2025 at 12:11:56PM +0200, Uwe Kleine-König wrote:
> 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.

Ack. I will mantain pwm_state in ad4052_state.

> 
> > +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)?
> 

Yes, but I will just use the (new) managed pwm_state instead:

  *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
  return IIO_VAL_INT;

> > +
> > [...]
> > +
> > +	ret = pwm_enable(st->cnv_pwm);
> > +	if (ret)
> > +		goto out_pwm_error;
> 
> pwm_enable() is another disguised pwm_get_state().
> 

Ack.

> > +
> > +	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

Best regards,
Jorge