Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Uwe Kleine-König]

Hello,

On Tue, Jun 17, 2025 at 05:34:56PM +0200, Jorge Marques wrote:
> On Tue, Jun 17, 2025 at 04:59:48PM +0200, Uwe Kleine-König wrote:
> > On Tue, Jun 10, 2025 at 09:34:37AM +0200, Jorge Marques wrote:
> > > +static int ad4052_get_samp_freq(struct iio_dev *indio_dev,
> > > +				struct iio_chan_spec const *chan,
> > > +				int *val,
> > > +				int *val2)
> > > +{
> > > +	struct ad4052_state *st = iio_priv(indio_dev);
> > > +
> > > +	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
> > > +	return IIO_VAL_INT;
> > 
> > st->pwm_st.period is the period that was requested before. If you want
> > the real period that is currently emitted, check pwm_get_state_hw().
> 
> I believe only ad4695.c uses this method and the reason for that is if
> the pwm is disabled we still want to obtain the requested value.
> 
> Reverting slightly to v2, the semantic to allow fetching from hw when
> enabled, and using the managed state when disabled, would be:
> 
> 	struct pwm_state pwm_st;
> 	int ret
> 
> 	ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> 	if (ret)
> 		goto out_release;
> 
> 	if (!pwm_st.enabled)
> 		pwm_st = st->pwm_st;
> 
> 	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> 
> Is this ok?

Looks fine to me. I didn't object the original suggested code, just
wanted to highlight the semantics.

I would expect that the compiler optimizes out the unnecessary
assignments done in pwm_st = st->pwm_st, as only pwm_st.period is used
later on.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature