Hi Uwe,

On 08.01.2019 00:10, Uwe Kleine-König wrote:

Hello Claudiu,

On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote:

quoted

On 05.01.2019 23:05, Uwe Kleine-König wrote:

quoted

On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote:

quoted

From: Claudiu Beznea <redacted>

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.

True it is more generic in the way that it gives the possibility to
configure all kind of waveforms. But not all controllers supports this.
The use case of this would be to have dead-times with any values, right?

Then you
wrote that the push-pull mode is hardware generated on Atmel with some
implementation details.

Yes, I wanted to say that Atmel PWM generates only a set of waveforms.

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 so, the user should have been taken care of the relations b/w these
values in order to configure a proper working mode.

It would be good to have Thierry's inputs on these.

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.

quoted

quoted

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.
 

quoted

quoted

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.)

quoted

quoted

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.

This is checked by pwm_supports_mode() called in pwm_apply_state(). If
controller doesn't support the mode, the apply will fail.

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.)

quoted

quoted

quoted

[...]

@@ -53,12 +75,14 @@ enum {
  * @period: PWM period (in nanoseconds)
  * @duty_cycle: PWM duty cycle (in nanoseconds)
  * @polarity: PWM polarity
+ * @modebit: PWM mode bit
  * @enabled: PWM enabled status
  */
 struct pwm_state {
 	unsigned int period;
 	unsigned int duty_cycle;
 	enum pwm_polarity polarity;
+	unsigned long modebit;

I fail to see the upside of storing the mode as 2^mode instead of a
plain enum pwm_mode. Given that struct pwm_state is visible for pwm
users a plain pwm_mode would at least be more intuitive.

To have all modes supported by a controller grouped in pwm_caps::modes_msk.

My question was not about struct pwm_caps::modes_msk but about
struct pwm_state::modebit. As struct pwm_state has visibility even
outside of the pwm API (i.e. it is used by consumers) it is beneficial
to keep that simple. Letting a consumer pass in the mode he wants is
easier to explain than setting a single bit. Also error checking with a
plain enum is easier because you just do:

	if (mode >= MODE_CNT)
		error()

which is easy to grasp. Compare that to

	if (!is_power_of_two(modebit) || modebit >= PWM_MODE_BIT(CNT))
		error()

(modulo syntactical correctness).

The reason I choose to have it as bit was the memcmp() at the beginning of
pwm_apply_state() and to avoid starting enum pwm_mode from 1 and to avoid
having bit 0 of pwm_caps::modes_msk unused (in the driver I'm using
PWM_MODE_BIT() macro to fill in the driver's supported modes).

Thank you,
Claudiu Beznea

Best regards
Uwe