Re: [PATCH 1/1] commands: add pwm manipulation command

[Jookia <contact@jookia.org>]

Hi, not really a Barebox developer but I figured I might chime in.

On Sun, May 28, 2023 at 09:24:09AM +1000, Marc Reilly wrote:
> This introduces a command to set parameters for a pwm device.
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---

...

> +	if ((n < 0) && !devname) {
> +		printf(" need to specify a device\n");
> +		return COMMAND_ERROR_USAGE;
> +	}
> +	if ((freq == 0) || (period == 0)) {
> +		printf(" period or freqency needs to be non-zero\n");
> +		return COMMAND_ERROR_USAGE;
> +	}
> +
> +	if (!devname) {
> +		sprintf(namebuf, "pwm%d", n);
> +	} else {
> +		strcpy(namebuf, devname);
> +	}

Is devname capped to namebuf length? It might be better refer to devname
instead of namebuf and point devname to namebuf when you use namebuf, otherwise
point it to the the optarg.

Is it even worth supporting refering by number instead of just having scripts
type pwmX?

> +	pwm = pwm_request(namebuf);
> +	if (!pwm) {
> +		printf("pwm device %s not found\n", namebuf);

No space at the start?

> +		return -ENODEV;
> +	}
> +
> +	pwm_get_state(pwm, &state);
> +
> +	if ((state.period_ns == 0)
> +			&& (freq < 0) && (duty < 0) && (period < 0)) {
> +		printf(" need to know some timing info; freq or dury/period\n");

No pwm_free?

> +		return COMMAND_ERROR_USAGE;
> +	}
> +
> +	if (polarity >= 0)
> +		state.polarity = polarity;
> +
> +	/* period */
> +	if (freq > 0) {
> +		state.p_enable = true;
> +		state.period_ns = HZ_TO_NANOSECONDS(freq);
> +		if (width < 0) {
> +			width = 50;
> +		}
> +	} else if (period > 0) {
> +		state.p_enable = true;
> +		state.period_ns = period;
> +	}
> +
> +	/* duty */
> +	if (width >= 0) {
> +		if (width > 100)
> +			width = 100;
> +
> +		pwm_set_relative_duty_cycle(&state, width, 100);
> +	} else if (duty >= 0) {
> +		if (duty > state.period_ns)
> +			printf(" warning: duty_ns is greater than period\n");
> +
> +		state.duty_ns = duty;
> +	}

It might be worth moving the width and duty checks to up with the opt parsing
and make a width > 100 and error.

Jookia.