Re: [PATCH 07/10] iio: frequency: ad9910: support drctl control

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]

On Thu, Oct 15, 2026 at 01:49:47PM -0300, Rodrigo Alencar via B4 Relay wrote:
> +static int ad9910_drctl_sync(struct ad9910_state *st)
> +{
> +	struct pwm_state state;
> +	bool ndh, ndl;
> +	int ret;
> +
> +	if (!st->drctl_pwm)
> +		return 0;
> +
> +	ndh = FIELD_GET(AD9910_CFR2_DRG_NO_DWELL_HIGH_MSK,
> +			st->reg[AD9910_REG_CFR2].val32);
> +	ndl = FIELD_GET(AD9910_CFR2_DRG_NO_DWELL_LOW_MSK,
> +			st->reg[AD9910_REG_CFR2].val32);
> +
> +	pwm_init_state(st->drctl_pwm, &state);
> +
> +	if (!ndh && ndl) {
> +		state.duty_cycle = st->ramp_down_time_ns / 2;
> +		state.period = st->ramp_down_time_ns;
> +		state.polarity = PWM_POLARITY_INVERSED;
> +	} else if (ndh && !ndl) {
> +		state.duty_cycle = st->ramp_up_time_ns / 2;
> +		state.period = st->ramp_up_time_ns;
> +		state.polarity = PWM_POLARITY_NORMAL;
> +	} else if (!ndh && !ndl) {
> +		state.duty_cycle = st->ramp_up_time_ns;
> +		state.period = st->ramp_up_time_ns + st->ramp_down_time_ns;
> +		state.polarity = PWM_POLARITY_NORMAL;
> +	} else {
> +		state.duty_cycle = 0;
> +		state.period = st->ramp_up_time_ns + st->ramp_down_time_ns;
> +		state.polarity = PWM_POLARITY_NORMAL;
> +	}
> +
> +	if (!state.period) {
> +		pwm_disable(st->drctl_pwm);
> +		return 0;
> +	}
> +
> +	state.enabled = true;
> +	ret = pwm_apply_might_sleep(st->drctl_pwm, &state);
> +	if (ret)
> +		return ret;
> +
> +	ret = pwm_get_state_hw(st->drctl_pwm, &state);
> +	if (ret)
> +		return ret;

Does enabling the PWM have any side effects? Are you doing that only to
be able to call pwm_get_state_hw() and thus find out what the resulting
output looks like? In that case, pwm_round_waveform_might_sleep() might
be your friend.

> +
> +	if (!ndh && ndl) {
> +		st->ramp_down_time_ns = state.period;
> +	} else if (ndh && !ndl) {
> +		st->ramp_up_time_ns = state.period;
> +	} else if (!ndh && !ndl) {
> +		st->ramp_up_time_ns = state.duty_cycle;
> +		st->ramp_down_time_ns = state.period - st->ramp_up_time_ns;
> +	}

So the configuration depends on the previous state of the
AD9910_REG_CFR2 register? That's a recipe for surprises?!

> +
> +	return 0;
> +}
> +

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]