On 09.01.2019 00:08, Uwe Kleine-König wrote:

On Tue, Jan 08, 2019 at 09:21:34AM +0000, Claudiu.Beznea@microchip.com wrote:

quoted

Hi Uwe,

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

quoted

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?

Well, I didn't target that. The model I suggested is just a generic set
of parameters that are able to describe the wave forms for all three
modes you suggested. That it allows to express dead-times is just a nice
by-product.

quoted

quoted

quoted

quoted

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

Does that mean you are convinced by my argument?

I know what you are talking about. I balanced in between these two paths
while I wrote the code. The approach I chose looked to me to be more easy
to read. Now, since there is another person (you) thinking the other
approach is best, I'm thinking to change it in the next version.

Thank you,
Claudiu Beznea

Best regards
Uwe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel