Hi, Here's a quick review based on the experience of me writing my own driver. On Mon, May 20, 2024 at 09:42:20PM +0300, Aleksandr Shubin wrote: > + act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val); > + ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val); > + > + /* > + * The duration of the active phase should not be longer > + * than the duration of the period > + */ > + if (act_cycle > ent_cycle) > + act_cycle = ent_cycle; > + > + /* > + * We have act_cycle <= ent_cycle <= 0xffff, prescale_k <= 0x100, > + * div_m <= 8. So the multiplication fits into an u64 without > + * overflow. > + */ > + tmp = ((u64)(act_cycle) * prescale_k << div_m) * NSEC_PER_SEC; > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate); > + tmp = ((u64)(ent_cycle) * prescale_k << div_m) * NSEC_PER_SEC; > + state->period = DIV_ROUND_UP_ULL(tmp, clk_rate); Doesn't ent_cycle require a + 1 here? Shouldn't act_cycle be > ent_cycle on 0% duty cycles? > + /* if the neighbor channel is enable, check period only */ > + use_bus_clk = FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, clk_cfg) != 0; > + val = mul_u64_u64_div_u64(state->period, > + (use_bus_clk ? bus_rate : hosc_rate), > + NSEC_PER_SEC); It would be nice if it reclocked both channels. > + /* calculate prescale_k, PWM entire cycle */ > + ent_cycle = val >> div_m; > + prescale_k = DIV_ROUND_DOWN_ULL(ent_cycle, 65537); > + if (prescale_k > SUN20I_PWM_CTL_PRESCAL_K_MAX) > + prescale_k = SUN20I_PWM_CTL_PRESCAL_K_MAX; > + > + do_div(ent_cycle, prescale_k + 1); > + > + /* for N cycles, PPRx.PWM_ENTIRE_CYCLE = (N-1) */ > + reg_period = FIELD_PREP(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, ent_cycle - 1); > + > + /* set duty cycle */ > + val = mul_u64_u64_div_u64(state->duty_cycle, > + (use_bus_clk ? bus_rate : hosc_rate), > + NSEC_PER_SEC); > + act_cycle = val >> div_m; > + do_div(act_cycle, prescale_k + 1); I'm not sure about this code. I don't quite get where the 65537 comes from or what's really happening here. To my understanding you either want to limit PWM_ENTIRE_CYCLE to 0xFFFE so and scale PWM_ACTIVE_CYCLE from 0 to 65535 so it can be 0x0 at 100% duty cycles and 0xFFFF at 0% duty cycles, OR you want to scale it from 0 to 65536 and check if the value is 65536, and if it is wrap it around to 0 and flip the polarity. Thanks, John.
