On 06.02.2019 10:24, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Wed, Feb 06, 2019 at 12:01:26AM +0100, Thierry Reding wrote:
>> On Mon, Jan 07, 2019 at 11:10:40PM +0100, Uwe Kleine-König wrote:
>>> On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
>>>> On 05.01.2019 23:05, Uwe Kleine-König wrote:
>>>>> On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
>>>>>> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>>>>>>
>>>>>> Add basic PWM modes: normal and complementary. These modes should
>>>>>> differentiate the single output PWM channels from two outputs PWM
>>>>>> channels. These modes could be set as follow:
>>>>>> 1. PWM channels with one output per channel:
>>>>>> - normal mode
>>>>>> 2. PWM channels with two outputs per channel:
>>>>>> - normal mode
>>>>>> - complementary mode
>>>>>> Since users could use a PWM channel with two output as one output PWM
>>>>>> channel, the PWM normal mode is allowed to be set for PWM channels with
>>>>>> two outputs; in fact PWM normal mode should be supported by all PWMs.
>>>>>
>>>>> I still think that my suggestion that I sent in reply to your v5 using
>>>>> .alt_duty_cycle and .alt_offset is the better one as it is more generic.
>>>>
>>>> I like it better my way, I explained myself why.
>>>
>>> I couldn't really follow your argument though. You seemed to acknowledge
>>> that using .alt_duty_cycle and .alt_offset is more generic. Then you
>>> wrote that the push-pull mode is hardware generated on Atmel with some
>>> implementation details. IMHO these implementation details shouldn't be
>>> part of the PWM API and atmel's .apply should look as follows:
>>>
>>> if (state->alt_duty_cycle == 0) {
>>>
>>> ... configure for normal mode ...
>>>
>>> } else if (state->duty_cycle == state->alt_duty_cycle &&
>>> state->alt_offset == state->period / 2) {
>>>
>>> ... configure for push pull mode ...
>>>
>>> } else if (state->duty_cycle + state->alt_duty_cycle == state->period &&
>>> state->alt_offset == state->duty_cycle) {
>>>
>>> ... configure for complementary mode ...
>>>
>>> } else {
>>> return -EINVAL;
>>> }
>>>
>>> If it turns out to be a common pattern, we can add helper functions à la
>>> pwm_is_complementary_mode(state) and
>>> pwm_set_complementary_mode(state, period, duty_cycle). This allows to
>>> have a generic way to describe a wide range of wave forms in a uniform
>>> way in the API (which is good) and each driver implements the parts of
>>> this range that it can support.
>>
>> I think this is going to be the rule rather than the exception, so I'd
>> expect we'll see these helpers used in pretty much all drivers that
>> support more than just the normal mode.
>
> If you intended to contradict me here: You didn't. I have the same
> expectation.
>
>> But I really don't see the point in having consumers jump through hoops
>> to set one of the standard modes just to have the driver jump through
>> more hoops to determine which mode was meant.
>
> I think my approach is more natural and not more complicated at all. In
> all modes where this secondary output makes sense both outputs share the
> period length. In all modes both outputs have a falling and a raising
> edge each. Let's assume we support
>
> - normal mode (one output, secondary (if available) inactive)
> - complementary mode (secondary output is the inverse of primary
> output)
> - push-pull mode (primary output only does every second active phase,
> the secondy output does the ones that are skiped by the primary one)
> - complementary mode with deadtime (like above but there is a pause
> where both signals are inactive at the switch points, so the active
> phase of the secondary output is $deadtime_pre + $deadtime_post
> shorter than the primary output's inactive phase).
>
> To describe these modes we need with the approach suggested by Claudiu
> the following defines:
>
> enum mode {
> NORMAL,
> COMPLEMENTARY,
> PUSH_PULL
> PUSH_PULL_WITH_DEADTIME
I see push-pull mode as a particular mode of complementary mode with
dead-times equal to a half of a period.
I don't get the meaning of push-pull with dead-times, there is already
there a deadtime pre, post value equal with 1/2 of period.
> }
>
> struct pwm_state {
> unsigned int period;
> unsigned int duty_cycle;
> enum pwm_polarity polarity;
> bool enabled;
> enum mode mode;
> unsigned int deadtime_pre;
> unsigned int deadtime_post;
> }
>
> This has the following downsides:
>
> - The period in push-pull mode is somehow wrong because the signal
> combination repeats only every 2x $period ns. (I guess this is an
> implementation detail of the atmel hardware that leaks into the API
> here.)
As far as I'm concern of the PWM push-pull mode it is a specific PWM
working mode, not related to Atmel, which can be used to drive half-bridge
converters.
>
> - There is redundancy in the description:
>
> { .period = U, .duty_cycle = V, .polarity = W, .enabled = true, .mode = PUSH_PULL, .deadtime_pre = DC, .deadtime_post = DC }
>
> is the same as
>
> { .period = U, .duty_cycle = V, .polarity = W, .enabled = true, .mode = PUSH_PULL_WITH_DEADTIME, .deadtime_pre = 0, .deadtime_post = 0 }
>
> .
>
> This is all more sane with my suggestion, and pwm_state is smaller with
> my approach. .period has always the same meaning and for a device that
> supports secondary mode .alt_offset and .alt_period always have the same
> semantic. (Opposed to .deadtime_X that only matter sometimes.)
>
> Also I don't see hoops for the implementing pwm driver: Assume it only
> supports normal and complementary mode. The difference is:
>
> - With Claudiu's approach:
>
> switch (state->mode) {
> case NORMAL:
> ... do normal ...
> break;
> case COMPLEMENTARY:
> ... do complementary ...
> break;
> default:
> return -ESOMETHING;
> break;
> }
>
> - with my approach:
>
> if (pwm_is_normal_mode(state) {
> ... do normal ...
> } else if (pwm_is_complementary_mode(state) {
> .. do complementary ...
> } else {
> return -ESOMETHING;
> }
>
> So I don't see a hoop apart from needed some pwm_is_XX_mode helpers in
> the core. Moreover for a flexible hardware that supports the full range
> (e.g. the hifive one where a driver is currently under discussion if
> only one pwm cell is implemented as I suggested in my review) the
> implementation is simpler with my approach it just looks as follows:
>
> configure(period, duty_cycle, alt_offset, alt_period)
>
> instead of
>
> switch (state->mode) {
> case NORMAL:
> configure(period, duty_cycle, 0, 0);
> break;
> case COMPLEMENTARY:
> configure(period, duty_cycle, duty_cycle, period - duty_cycle);
> break;
> case PUSH_PULL:
> configure(2 * period, duty_cycle, period, duty_cycle);
> break;
> case PUSH_PULL_WITH_DEADTIME:
> configure(2 * period, duty_cycle, period + deadtime_pre,
> duty_cycle - deadtime_pre - deadtime_post);
> break;
> default:
> return -ESOMETHING;
> break;
> }
>
>> There are only so many modes and I have never seen hardware that
>> actually implements the kind of fine-grained control that would be
>> possible with your proposal.
>
> That there is hardware that actually implements all the flexibility that
> is available is second-order. (But as said above, the hifive
> implementation can do it. And I think the ST implementation I saw some
> time ago can do it, too; I didn't recheck though.) The key here is to
> have a natural description of the intended waveform. And describing it
> using a mode and additional parameters depending on the mode is more
> complex than two additional parameters that can cover all waveforms.
>
> That not all drivers can implement all waveforms that consumers might
> request is common to both approaches.
>
>> The goal of an API is to abstract, but .alt_duty_cycle and .alt_offset
>> would be an inversion of API abstraction.
>
> No, the goal of an API is to give a way that is easy and natural to let
> consumers request all the stuff they might want. And if there is a
> single set of parameters that describes a broad subset of waveforms with
> parameters that you can measure in the wave form that is better than to
> separate waveforms into categories (modes) and implement each of these
> with their own parameter set. And then your categorisation might not
> match the capabilities of some hardware. Consider a device that can
> implement PUSH_PULL_WITH_DEADTIME but only with .deadtime_pre =
> .deadtime_post.
>
> That the API has to abstract is actually bad because it limits users.
> If a consumer wants push-pull mode with dead time and the hardware
> supports that but the API has abstracted that away, that's bad.
> If a consumer doesn't care if the configured duty cycle is already
> active when pwm_apply_state() returns or is only scheduled in the
> hardware for after the current period ends, this forces a delay on the
> consumer because the abstraction is that the configured wave form is
> already on the wire on return.
>
> Abstraction is necessary to cover different hardware implementations and
> allow users to handle these in a uniform way. So from a consumer's POV
> an abstraction that doesn't limit the accessible capabilities of the
> hardware is optimal.
>
> Using .alt_offset and .alt_period is an abstraction that limits less and
> so gives more possibilities to the consumers.
>
>> That is, we'd be requiring the drivers to abstract the inputs of the
>> API, which is the wrong way around.
>
> That is a normal "problem" for drivers. The driver gets a request and it
> has to determine if it can implement that. And if this is done using a
> comparison of .mode to known "good" values or by using a helper function
> that compares .alt_offset to .period is an implementation detail that
> doesn't matter much.
>
>>>>> I don't repeat what I wrote there assuming you still remember or are
>>>>> willing to look it up at
>>>>> e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half
>>>>> of my mail).
>>>>
>>>> Yes, I remember it.
>>>
>>> I expected that, my words were more directed to Thierry than you.
>>>
>>>>> Also I think that if the capabilities function is the way forward adding
>>>>> support to detect availability of polarity inversion should be
>>>>> considered.
>>>>
>>>> Yep, why not. But it should be done in a different patch. It is not related
>>>> to this series.
>>>
>>> Yes, given that polarity already exists, this would be a good
>>> opportunity to introduce the capability function for that and only
>>> afterwards add the new use case with modes. (But having said this, read
>>> further as I think that this capability function is a bad idea.)
>>
>> I don't think we need to require this. The series is already big enough
>> as it is and has been in the works for long enough. There's no harm in
>> integrating polarity support into the capability function later on.
>
> I think "the series is long enough in the works" is not an argument to
> stop pointing out weaknesses. The harm that is done in not adding
> polarity support now is that it adds another thing to the todo list of
> things that are started a bit and need to be completed in the future.
>
> And the harm in adding underdone stuff to an API if there are known
> weaknesses is more work later.
>
>>>>> This would also be an opportunity to split the introduction
>>>>> of the capabilities function and the introduction of complementary mode.
>>>>> (But my personal preference would be to just let .apply fail when an
>>>>> unsupported configuration is requested.)
>>>>
>>>> .apply fails when something wrong is requested.
>>>
>>> If my controller doesn't support a second output is it "wrong" to
>>> request complementary mode? I'd say yes. So you have to catch that in
>>> .apply anyhow and there is little benefit to be able to ask the
>>> controller if it supports it beforehand.
>>>
>>> I don't have a provable statistic at hand, but my feeling is that quite
>>> some users of the i2c frame work get it wrong to first check the
>>> capabilities and only then try to use them. This is at least error prone
>>> and harder to use than the apply function returning an error code.
>>> And on the driver side the upside is to have all stuff related to which
>>> wave form can be generated and which cannot is a single place. (Just
>>> consider "inverted complementary mode". Theoretically this should work
>>> if your controller supports complementary mode and inverted mode. If you
>>> now have a driver for a controller that can do both, but not at the same
>>> time, the separation gets ugly. OK, this is a constructed example, but
>>> in my experience something like that happens earlier or later.)
>>
>> I think capabilities are useful in order to be able to implement
>> fallbacks in consumer drivers. Sure the same thing could be implemented
>> by trying to apply one state first and then downgrade and retry on
>> failure and so on, but sometimes it's more convenient to know what's
>> possible and determine what's the correct solution upfront.
>
> For me there is no big difference between:
>
> Oh, the driver cannot do inversed polarity, I have to come up
> with something else.
>
> and
>
> Oh, the driver can only implement periods that are powers of two
> of the input clk, I have to come up with something else.
>
> and
>
> Oh, I requested a duty cycle of 89 ns, but the hardware can only
> do 87 or 90 ns so I have to come up with something else.
>
> With capabilities you can only cover the first of these. With an
> approach similar to clk_round_rate you can easily cover all.
>
> Best regards
> Uwe
>
