Thierry, On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding <thierry.red...@gmail.com> wrote: >> pwm_get_period(): get the period of the PWM; if the PWM has not yet >> been configured by software this gets the default period (possibly >> specified by the device tree). > > No. I think we'll need a different construct for the period defined by > DT or board files. pwm_get_period() is the legacy API to retrieve the > "current" period, even if it was lying a little before the atomic API.
Ah, got it. I think I missed that you considered pwm_get_period() legacy and that you eventually wanted to get rid of it. OK, then what you say makes sense. >> That should work with one minor problem. If HW readout isn't >> supported then pwm_get_state() in probe will presumably return 0 for >> the duty cycle. That means it will change the voltage. That's in >> contrast to how I think things work today where the voltage isn't >> changed until the first set_voltage() call. At least the last time I >> tested things get_voltage() would simply report an incorrect value >> until the first set_voltage(). I think existing behavior (reporting >> the wrong value) is better than new behavior (change the value at >> probe). > > That's exactly the point. Reporting a wrong value isn't really a good > option. Changing the voltage on boot is the only way to make the logical > state match the hardware state on boot. Chances are that if you don't > have hardware readout support you probably don't care what state your > regulator will be in. > > Then again, if we don't support hardware readout, setting up the logical > state with data from DT (or board files) and defaulting the duty cycle > to 0, we end up with exactly what we had before, even with the atomic > API, right? Maybe that's okay, too. IMHO this is a change in behavior that will break existing users. Anyone using a PWM regulator will suddenly find their voltage changing at bootup. Certainly today all users of the PWM regulator don't seem to mind (apparently) the the voltage is reported incorrectly at bootup but I bet they'd mind if the voltage suddenly started changing for them at bootup. It seems better to preserve existing behavior and print a warning that the voltage will be reported incorrectly until HW Readout is supported. Of course, we're only talking about two real users in mainline here: Rockchip boards and the "stih407-family". If we just fix both of those to support HW Readout before landing the change then I'm fine with doing what you say. >> ...and if set_voltage() remains untouched then we can solve my probe >> problem by renaming pwm_get_state() to pwm_get_hw_state() and having >> it return an error if HW readout is not supported. Then we only call >> pwm_get_args() / pwm_apply_state() when we support HW readout. > > The problem is that we make the API clumsy to use. If we don't sync the > "initial" state (as defined by DT or board files) to hardware at any > point, then we need to add the pwm_args construct and always stick to > it. I think it weird to have to use the pwm_args.period instead of the > current period. > > So we're back to square one, really. That's exactly what Mark brought up > originally. I had missed the part where you wanted to deprecate pwm_get_period(). Thus my points here aren't really valid. In my mind the old API was perfectly fine (and actually quite clean / simple to use) except in the special case of avoiding the PWM regulator glitches. With that mindset I think my previous email make sense. However, this is your subsystem to maintain and if you think moving everyone to a new atomic API makes more sense then you're in the best position to make that decision. :) -Doug -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
