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
