drivers: pwm: pwm_led_esp32 outputs glitchy waveforms

[braiden]

Describe the bug

There's a phase shift on the low-speed channels of esp32c3 whenever PWM output period or duty is changed. Even when setting the pulse width to 0 (disabled) there's a phase shift / extra pulse before the output stops:

To Reproduce

Use the shell to change duty:

uart:~$ pwm usec ledc@60019000 0 20000 1000
uart:~$ pwm usec ledc@60019000 0 20000 0

prj.conf

CONFIG_LOG=y
CONFIG_LOG_DEFAULT_LEVEL=3
CONFIG_LOG_OVERRIDE_LEVEL=0
CONFIG_PWM=y
CONFIG_SHELL=y
CONFIG_PWM_SHELL=y
CONFIG_GPIO_SHELL=y

boards/esp32c3_devkitc.overlay

&pinctrl {
  ledc1_default: ledc1_default {
    group0 {
      pinmux = <LEDC_CH0_GPIO3>;
    };
    group1 {
      pinmux = <LEDC_CH1_GPIO4>;
    };
  };
};

&ledc0 {
  pinctrl-0 = <&ledc1_default>;
  pinctrl-names = "default";
  status = "okay";
  #address-cells = <1>;
  #size-cells = <0>;
  channel0@0 {
    reg = <0x0>;
    timer = <0>;
  };
  channel1@1 {
    reg = <0x1>;
    timer = <1>;
  };
};

/ {
  chosen {
    zephyr,console = &usb_serial;
    zephyr,shell-uart = &usb_serial;
  };
};

Expected behavior

Phase should not shift when updating the duty cycle.

Impact

In my use-case, the extra pulse when setting output to 0, causes the servo to jerk.

Logs and console output

Environment (please complete the following information):

• OS: Linux
• Toolchain: Zephyr SDK
• Commit: v4.0.0

Additional context

Issue appears to be that the driver re-initializes the hardware each time pwm_led_esp32_set_cycles is called, e.g. it resets the counter here

I moved ledc_hal_init, ledc_hal_timer_rst, and pwm_led_esp32_configure_pinctrl to init and it seems to fix the issue. (Note: pinctl needs to move too or there's a short negative pulse if output is high when duty is changed).

// SPDX-License-Identifier: Apache-2.0

int pwm_led_esp32_init(const struct device *dev)
{
	int ret;
	const struct pwm_ledc_esp32_config *config = dev->config;
	struct pwm_ledc_esp32_data *data = (struct pwm_ledc_esp32_data *const)(dev)->data;

	if (!device_is_ready(config->clock_dev)) {
		LOG_ERR("clock control device not ready");
		return -ENODEV;
	}

	/* Enable peripheral */
	clock_control_on(config->clock_dev, config->clock_subsys);

	for (int i = 0; i < config->channel_len; i++) {
		struct pwm_ledc_esp32_channel_config *channel = &config->channel_config[i];
		ledc_hal_init(&data->hal, channel->speed_mode);
		ledc_hal_timer_rst(&data->hal, channel->timer_num);
	}

	ret = pwm_led_esp32_configure_pinctrl(dev);
	if (ret < 0) {
		return ret;
	}

	return 0;
}

I can send a PR if this fix looks reasonable, but I'm just a little skeptical of performing ledc_hal_timer_rst before the timer/counter clock has been setup even though it appears to work fine. Unfortunately, I cannot completely initialize the peripheral in init because its not known what clock source the driver will select until an output period is specified by application code.