Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1716175 > unrolled thread

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/2] extend PWM framework to support PWM dead-times Thierry Reding <thierry.reding@gmail.com> - 2017-08-21 10:30 +0200
    Re: [PATCH 0/2] extend PWM framework to support PWM dead-times m18063 <Claudiu.Beznea@microchip.com> - 2017-08-21 12:30 +0200
      Re: [PATCH 0/2] extend PWM framework to support PWM dead-times Thierry Reding <thierry.reding@gmail.com> - 2017-08-21 13:30 +0200
        Re: [PATCH 0/2] extend PWM framework to support PWM dead-times m18063 <Claudiu.Beznea@microchip.com> - 2017-08-22 14:20 +0200
          Re: [PATCH 0/2] extend PWM framework to support PWM dead-times Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2017-08-22 14:30 +0200
            Re: [PATCH 0/2] extend PWM framework to support PWM dead-times m18063 <Claudiu.Beznea@microchip.com> - 2017-08-22 15:00 +0200

#1716175 — Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[Multipart message — attachments visible in raw view] — view raw

On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote:
> Hi all,
> 
> Please give feedback on these patches which extends the PWM
> framework in order to support multiple PWM signal types.
> Since I didn't receive any inputs on RFC series I'm resending it as
> normal patch series.
> 
> The current patch series add the following PWM signal types:
> - PWM complementary signals
> - PWM push-pull signal
> 
> Definition of PWM complementary mode:
> For a PWM controllers with more than one outputs per PWM channel,
> e.g. with 2 outputs per PWM channels, the PWM complementary signals
> have opposite levels, same duration and same starting times,
> as in the following diagram:
> 
>         __    __    __    __
> PWMH __|  |__|  |__|  |__|  |__
>      __    __    __    __    __
> PWML   |__|  |__|  |__|  |__|
>        <--T-->
> Where T is the signal period.
> 
> Definition of PWM push-pull mode:
> For PWM controllers with more than one outputs per PWM channel,
> e.g. with 2 outputs per PWM channel, the PWM push-pull signals
> have same levels, same duration and are delayed until the begining
> of the next period, as in the following diagram:
> 
>         __          __
> PWMH __|  |________|  |________
>               __          __
> PWML ________|  |________|  |__
>        <--T-->
> 
> Where T is the signal period.
> 
> These output signals could be configured by setting PWM mode
> (a new input in sysfs has been added in order to support this
> operation).
> 
> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
> 
> The PWM push-pull mode could be usefull in applications like
> half bridge converters.

Sorry for taking an absurdly long time to get back to you on this.

One problem I see with this is that the PWM framework is built on the
assumption that we have a single output per PWM channel. This becomes
important when you start adding features such as this because now the
users have no way of determining whether or not the PWM they retrieve
will actually be able to do what they want.

For example, let's say you have a half bridge converter driver in the
kernel and it does a pwm_get() to obtain a PWM to use. There's nothing
preventing it from using a simple one-output PWM and there's no way to
check that we're actually doing something wrong.

I think any in-kernel API would have to be more fully-featured,
otherwise we're bound to run into problems. At the very least I think
we'd have to expose some sort of capabilities list.

A possibly simpler approach might be to restrict this to the driver that
you need this for. It looks like you will be mainly using this via sysfs
and in that case you might be able to just extend what information is
exposed in sysfs for the particular PWM you're dealing with.

Thierry

[toc] | [next] | [standalone]

#1716284

Hi Thierry,

Thank you for your response. I added few comments below.

Thank you,
Claudiu

On 21.08.2017 11:25, Thierry Reding wrote:
> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote:
>> Hi all,
>>
>> Please give feedback on these patches which extends the PWM
>> framework in order to support multiple PWM signal types.
>> Since I didn't receive any inputs on RFC series I'm resending it as
>> normal patch series.
>>
>> The current patch series add the following PWM signal types:
>> - PWM complementary signals
>> - PWM push-pull signal
>>
>> Definition of PWM complementary mode:
>> For a PWM controllers with more than one outputs per PWM channel,
>> e.g. with 2 outputs per PWM channels, the PWM complementary signals
>> have opposite levels, same duration and same starting times,
>> as in the following diagram:
>>
>>         __    __    __    __
>> PWMH __|  |__|  |__|  |__|  |__
>>      __    __    __    __    __
>> PWML   |__|  |__|  |__|  |__|
>>        <--T-->
>> Where T is the signal period.
>>
>> Definition of PWM push-pull mode:
>> For PWM controllers with more than one outputs per PWM channel,
>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals
>> have same levels, same duration and are delayed until the begining
>> of the next period, as in the following diagram:
>>
>>         __          __
>> PWMH __|  |________|  |________
>>               __          __
>> PWML ________|  |________|  |__
>>        <--T-->
>>
>> Where T is the signal period.
>>
>> These output signals could be configured by setting PWM mode
>> (a new input in sysfs has been added in order to support this
>> operation).
>>
>> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
>> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
>> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
>> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
>>
>> The PWM push-pull mode could be usefull in applications like
>> half bridge converters.
> 
> Sorry for taking an absurdly long time to get back to you on this.
> 
> One problem I see with this is that the PWM framework is built on the
> assumption that we have a single output per PWM channel. This becomes
> important when you start adding features such as this because now the
> users have no way of determining whether or not the PWM they retrieve
> will actually be able to do what they want.
I was thinking that the framework is there to offer support in configuring
the underlying hardware without taking into account how many outputs per
PWM channels are actually present. It is true I only worked with Atmel/Microchip
PWM controllers which for instance, SAMA5 SoC has a PWM controller
with 2 outputs per PWM channel. Taking into account that the driver is
already mainlined I though that the user is aware of what he is using
(either a one output per PWM channel controller, or 2 outputs or
more than 2) and about the underlying hardware support. I understand your
position on this. I'm just explaining myself on the approach I've chose
for this implementation.

> 
> For example, let's say you have a half bridge converter driver in the
> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing
> preventing it from using a simple one-output PWM and there's no way to
> check that we're actually doing something wrong.
I understand your position here. I've chose this approach taking into
account that the user of the PWM will be aware of the capabilities
of the underlying hardware because I worked on a controller which already
has a mainlined driver and which has more than one output per PWM channel
and I believe there are also other controllers with this kind of capability
and with already mainlined driver (e.g. reading the code of STM32 PWM driver
I saw a bool type variable in driver specific data structure (struct stm32_pwm):
"bool have_complementary_output" which let me think that their controller
also could support more than one output per PWM channel (I will also try
to find the controller specific datasheet). At a first look I saw that also
TI ECAP PWM controller supports this (it is true that I am not aware of how
it is initialized in kernel, I need to check the datasheet, if it is public).
 
> 
> I think any in-kernel API would have to be more fully-featured,
> otherwise we're bound to run into problems. At the very least I think
> we'd have to expose some sort of capabilities list.
About the exposing of these capabilities would you prefer to have the
supported PWM modes registered in driver probe as a new field mask
in "struct pwm_chip". e.g.:
struct pwm_chip {
        struct device *dev;
        struct list_head list;
        const struct pwm_ops *ops;
        int base;
        unsigned int npwm;
	unsigned int pwm_modes_mask;	/* bitfield of supported signal types */

        struct pwm_device *pwms;

        struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
                                        const struct of_phandle_args *args);
        unsigned int of_pwm_n_cells;
};

And having in driver_probe() something like this:
driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE;
pwmchip_add(&driver_data->chip);
Since the PWM push-pull mode imply more than one output per PWM channel. And
validate the supported PWM modes when trying to configure one.

Or registering, as a field in pwm_chip, how many outputs per channel are actually
supported by the PWM controller and then validate the supported PWM mode based
on this?
e.g.:
in "struct pwm_chip". e.g.:
struct pwm_chip {
        struct device *dev;
        struct list_head list;
        const struct pwm_ops *ops;
        int base;
        unsigned int npwm;
	unsigned int pwm_outputs;	/* Number of supported outputs per channel */

        struct pwm_device *pwms;

        struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
                                        const struct of_phandle_args *args);
        unsigned int of_pwm_n_cells;
};

In driver_probe():
//...
driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */
pwmchip_add(&driver_data->chip);

> 
> A possibly simpler approach might be to restrict this to the driver that
> you need this for. It looks like you will be mainly using this via sysfs
> and in that case you might be able to just extend what information is
> exposed in sysfs for the particular PWM you're dealing with.
By this you mean exporting the sysfs attributes only for my driver or possibly
other drivers interested in this new feature? I may have another in kernel driver
which may try to use this feature.

Thank you,
Claudiu
> 
> Thierry
> 

[toc] | [prev] | [next] | [standalone]

#1716345

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote:
> Hi Thierry,
> 
> Thank you for your response. I added few comments below.
> 
> Thank you,
> Claudiu
> 
> On 21.08.2017 11:25, Thierry Reding wrote:
> > On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote:
> >> Hi all,
> >>
> >> Please give feedback on these patches which extends the PWM
> >> framework in order to support multiple PWM signal types.
> >> Since I didn't receive any inputs on RFC series I'm resending it as
> >> normal patch series.
> >>
> >> The current patch series add the following PWM signal types:
> >> - PWM complementary signals
> >> - PWM push-pull signal
> >>
> >> Definition of PWM complementary mode:
> >> For a PWM controllers with more than one outputs per PWM channel,
> >> e.g. with 2 outputs per PWM channels, the PWM complementary signals
> >> have opposite levels, same duration and same starting times,
> >> as in the following diagram:
> >>
> >>         __    __    __    __
> >> PWMH __|  |__|  |__|  |__|  |__
> >>      __    __    __    __    __
> >> PWML   |__|  |__|  |__|  |__|
> >>        <--T-->
> >> Where T is the signal period.
> >>
> >> Definition of PWM push-pull mode:
> >> For PWM controllers with more than one outputs per PWM channel,
> >> e.g. with 2 outputs per PWM channel, the PWM push-pull signals
> >> have same levels, same duration and are delayed until the begining
> >> of the next period, as in the following diagram:
> >>
> >>         __          __
> >> PWMH __|  |________|  |________
> >>               __          __
> >> PWML ________|  |________|  |__
> >>        <--T-->
> >>
> >> Where T is the signal period.
> >>
> >> These output signals could be configured by setting PWM mode
> >> (a new input in sysfs has been added in order to support this
> >> operation).
> >>
> >> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
> >> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
> >> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
> >> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
> >> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode
> >> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
> >> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
> >> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
> >> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
> >>
> >> The PWM push-pull mode could be usefull in applications like
> >> half bridge converters.
> > 
> > Sorry for taking an absurdly long time to get back to you on this.
> > 
> > One problem I see with this is that the PWM framework is built on the
> > assumption that we have a single output per PWM channel. This becomes
> > important when you start adding features such as this because now the
> > users have no way of determining whether or not the PWM they retrieve
> > will actually be able to do what they want.
> I was thinking that the framework is there to offer support in configuring
> the underlying hardware without taking into account how many outputs per
> PWM channels are actually present. It is true I only worked with Atmel/Microchip
> PWM controllers which for instance, SAMA5 SoC has a PWM controller
> with 2 outputs per PWM channel. Taking into account that the driver is
> already mainlined I though that the user is aware of what he is using
> (either a one output per PWM channel controller, or 2 outputs or
> more than 2) and about the underlying hardware support. I understand your
> position on this. I'm just explaining myself on the approach I've chose
> for this implementation.

So currently we assume that there will be (at least) one output per PWM
channel. However there are currently no drivers that actually use any
output other than the "primary" one.

That means, even if there are currently PWM controllers that support
multiple outputs per PWM channel, we have no code making assumptions
about it.

Before we can add code that makes any assumptions about the number of
outputs per PWM channel, we have to add facitilities for such code to
detect capabilities, so that they can deal with PWMs that don't match
what they need.

> > For example, let's say you have a half bridge converter driver in the
> > kernel and it does a pwm_get() to obtain a PWM to use. There's nothing
> > preventing it from using a simple one-output PWM and there's no way to
> > check that we're actually doing something wrong.
> I understand your position here. I've chose this approach taking into
> account that the user of the PWM will be aware of the capabilities
> of the underlying hardware because I worked on a controller which already
> has a mainlined driver and which has more than one output per PWM channel
> and I believe there are also other controllers with this kind of capability
> and with already mainlined driver (e.g. reading the code of STM32 PWM driver
> I saw a bool type variable in driver specific data structure (struct stm32_pwm):
> "bool have_complementary_output" which let me think that their controller
> also could support more than one output per PWM channel (I will also try
> to find the controller specific datasheet). At a first look I saw that also
> TI ECAP PWM controller supports this (it is true that I am not aware of how
> it is initialized in kernel, I need to check the datasheet, if it is public).

Yes, it's quite possible that many of the controllers we currently
support have this feature or not. But that's not really relevant. The
only important bit is that none of the users know about it. We don't
have the means to configure anything beyond just one output. So the only
guarantee you will get with the current framework is that there will be
one output signal per PWM channel.

> > I think any in-kernel API would have to be more fully-featured,
> > otherwise we're bound to run into problems. At the very least I think
> > we'd have to expose some sort of capabilities list.
> About the exposing of these capabilities would you prefer to have the
> supported PWM modes registered in driver probe as a new field mask
> in "struct pwm_chip". e.g.:
> struct pwm_chip {
>         struct device *dev;
>         struct list_head list;
>         const struct pwm_ops *ops;
>         int base;
>         unsigned int npwm;
> 	unsigned int pwm_modes_mask;	/* bitfield of supported signal types */
> 
>         struct pwm_device *pwms;
> 
>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>                                         const struct of_phandle_args *args);
>         unsigned int of_pwm_n_cells;
> };
> 
> And having in driver_probe() something like this:
> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE;
> pwmchip_add(&driver_data->chip);
> Since the PWM push-pull mode imply more than one output per PWM channel. And
> validate the supported PWM modes when trying to configure one.
> 
> Or registering, as a field in pwm_chip, how many outputs per channel are actually
> supported by the PWM controller and then validate the supported PWM mode based
> on this?
> e.g.:
> in "struct pwm_chip". e.g.:
> struct pwm_chip {
>         struct device *dev;
>         struct list_head list;
>         const struct pwm_ops *ops;
>         int base;
>         unsigned int npwm;
> 	unsigned int pwm_outputs;	/* Number of supported outputs per channel */
> 
>         struct pwm_device *pwms;
> 
>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>                                         const struct of_phandle_args *args);
>         unsigned int of_pwm_n_cells;
> };
> 
> In driver_probe():
> //...
> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */
> pwmchip_add(&driver_data->chip);

I think I'd prefer something more flexible. I also think this can't be
per-chip, but really has to be at the granularity of single PWM
channels. How about something like this:

	struct pwm_caps {
		...
	};

	int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps);

From a high-level perspective that would give users an easy way to
obtain the capabilities of the PWM they're handed. Then we can start
filling in that structure.

	struct pwm_caps {
		unsigned long modes;
	};

	#define PWM_MODE_NORMAL (1 << 0)
	#define PWM_MODE_COMPLEMENTARY (1 << 1)
	#define PWM_MODE_PUSH_PULL (1 << 2)

	static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps,
						  unsigned long mode)
	{
		return (caps->modes & mode) != 0;
	}

and maybe something like this as a shortcut:

	static inline bool pwm_supports_mode(struct pwm_device *pwm,
					     unsigned long mode)
	{
		struct pwm_caps caps;
		int err;

		err = pwm_get_caps(pwm, &caps);
		if (err < 0)
			return false;

		return pwm_caps_supports_mode(&caps, mode);
	}

I think that gives us a lot of flexibility and easy extensibility for
other capabilities. The implementation of pwm_get_caps() should probably
call back into the driver.

But that would cause a lot of boilerplate for simple cases, and would be
a lot of work to convert all existing drivers up front. So I think we
can add helpers to solve the normal cases. Something along these lines
perhaps:

	struct pwm_chip {
		...

		int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps);

		const struct pwm_caps *caps;
	};

	static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
	{
		if (!pwm || !caps || !pwm->chip->caps)
			return -EINVAL;

		*caps = *pwm->chip->caps;

		return 0;
	}

And then perhaps even do something like this:

	const struct pwm_caps pwm_default_caps = {
		.modes = PWM_MODE_NORMAL,
	};

Now we can either have a patch that sets pwm_default_caps and
pwm_simple_get_caps() for all existing drivers, or even easier add this
to the pwmchip_add_with_polarity() function:

	int pwmchip_add_with_polarity(struct pwm_chip *chip,
				      enum pwm_polarity polarity)
	{
		...

		if (!chip->get_caps && !chip->caps) {
			chip->get_caps = pwm_simple_get_caps;
			chip->caps = pwm_default_caps;
		}

		...
	}

That way, every driver, unless upgraded with a custom ->get_caps() would
get the defaults, which encode the current assumptions of the framework.

I think what we'd also want to do is make sure that every driver always
at least implement NORMAL mode and perhaps even fail to register those
that don't. Not sure we want to go that far, but those are the current
assumptions, so devices must be supporting it already anyway.

> > A possibly simpler approach might be to restrict this to the driver that
> > you need this for. It looks like you will be mainly using this via sysfs
> > and in that case you might be able to just extend what information is
> > exposed in sysfs for the particular PWM you're dealing with.
> By this you mean exporting the sysfs attributes only for my driver or possibly
> other drivers interested in this new feature? I may have another in kernel driver
> which may try to use this feature.

If you've got another driver that will want to use this, I'm leaning
towards going with the fully featured approach. Ideally you would try to
implement both at once so we can get a better feeling about whether or
not the framework changes are adequate to cover at least the first two
cases, which is much more reassuring than just being able to cover a
single case.

One thing to consider is still that if you only use this from sysfs,
it'll be quite a lot of work to add in-kernel infrastructure when it's
never needed. There's still an advantage to do it anyway because it
allows us to abstract everything away properly from the start and avoid
fragmentation of the sysfs interface.

Thierry

[toc] | [prev] | [next] | [standalone]

#1717357

Hi Thierry,

I added few other comments below. Please let me know what you think.

Thank you,
Claudiu

On 21.08.2017 14:25, Thierry Reding wrote:
> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote:
>> Hi Thierry,
>>
>> Thank you for your response. I added few comments below.
>>
>> Thank you,
>> Claudiu
>>
>> On 21.08.2017 11:25, Thierry Reding wrote:
>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote:
>>>> Hi all,
>>>>
>>>> Please give feedback on these patches which extends the PWM
>>>> framework in order to support multiple PWM signal types.
>>>> Since I didn't receive any inputs on RFC series I'm resending it as
>>>> normal patch series.
>>>>
>>>> The current patch series add the following PWM signal types:
>>>> - PWM complementary signals
>>>> - PWM push-pull signal
>>>>
>>>> Definition of PWM complementary mode:
>>>> For a PWM controllers with more than one outputs per PWM channel,
>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals
>>>> have opposite levels, same duration and same starting times,
>>>> as in the following diagram:
>>>>
>>>>         __    __    __    __
>>>> PWMH __|  |__|  |__|  |__|  |__
>>>>      __    __    __    __    __
>>>> PWML   |__|  |__|  |__|  |__|
>>>>        <--T-->
>>>> Where T is the signal period.
>>>>
>>>> Definition of PWM push-pull mode:
>>>> For PWM controllers with more than one outputs per PWM channel,
>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals
>>>> have same levels, same duration and are delayed until the begining
>>>> of the next period, as in the following diagram:
>>>>
>>>>         __          __
>>>> PWMH __|  |________|  |________
>>>>               __          __
>>>> PWML ________|  |________|  |__
>>>>        <--T-->
>>>>
>>>> Where T is the signal period.
>>>>
>>>> These output signals could be configured by setting PWM mode
>>>> (a new input in sysfs has been added in order to support this
>>>> operation).
>>>>
>>>> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
>>>> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode
>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
>>>> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
>>>>
>>>> The PWM push-pull mode could be usefull in applications like
>>>> half bridge converters.
>>>
>>> Sorry for taking an absurdly long time to get back to you on this.
>>>
>>> One problem I see with this is that the PWM framework is built on the
>>> assumption that we have a single output per PWM channel. This becomes
>>> important when you start adding features such as this because now the
>>> users have no way of determining whether or not the PWM they retrieve
>>> will actually be able to do what they want.
>> I was thinking that the framework is there to offer support in configuring
>> the underlying hardware without taking into account how many outputs per
>> PWM channels are actually present. It is true I only worked with Atmel/Microchip
>> PWM controllers which for instance, SAMA5 SoC has a PWM controller
>> with 2 outputs per PWM channel. Taking into account that the driver is
>> already mainlined I though that the user is aware of what he is using
>> (either a one output per PWM channel controller, or 2 outputs or
>> more than 2) and about the underlying hardware support. I understand your
>> position on this. I'm just explaining myself on the approach I've chose
>> for this implementation.
> 
> So currently we assume that there will be (at least) one output per PWM
> channel. However there are currently no drivers that actually use any
> output other than the "primary" one.
> 
> That means, even if there are currently PWM controllers that support
> multiple outputs per PWM channel, we have no code making assumptions
> about it.
> 
> Before we can add code that makes any assumptions about the number of
> outputs per PWM channel, we have to add facitilities for such code to
> detect capabilities, so that they can deal with PWMs that don't match
> what they need.
> 
>>> For example, let's say you have a half bridge converter driver in the
>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing
>>> preventing it from using a simple one-output PWM and there's no way to
>>> check that we're actually doing something wrong.
>> I understand your position here. I've chose this approach taking into
>> account that the user of the PWM will be aware of the capabilities
>> of the underlying hardware because I worked on a controller which already
>> has a mainlined driver and which has more than one output per PWM channel
>> and I believe there are also other controllers with this kind of capability
>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver
>> I saw a bool type variable in driver specific data structure (struct stm32_pwm):
>> "bool have_complementary_output" which let me think that their controller
>> also could support more than one output per PWM channel (I will also try
>> to find the controller specific datasheet). At a first look I saw that also
>> TI ECAP PWM controller supports this (it is true that I am not aware of how
>> it is initialized in kernel, I need to check the datasheet, if it is public).
> 
> Yes, it's quite possible that many of the controllers we currently
> support have this feature or not. But that's not really relevant. The
> only important bit is that none of the users know about it. We don't
> have the means to configure anything beyond just one output. So the only
> guarantee you will get with the current framework is that there will be
> one output signal per PWM channel.
> 
>>> I think any in-kernel API would have to be more fully-featured,
>>> otherwise we're bound to run into problems. At the very least I think
>>> we'd have to expose some sort of capabilities list.
>> About the exposing of these capabilities would you prefer to have the
>> supported PWM modes registered in driver probe as a new field mask
>> in "struct pwm_chip". e.g.:
>> struct pwm_chip {
>>         struct device *dev;
>>         struct list_head list;
>>         const struct pwm_ops *ops;
>>         int base;
>>         unsigned int npwm;
>> 	unsigned int pwm_modes_mask;	/* bitfield of supported signal types */
>>
>>         struct pwm_device *pwms;
>>
>>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>                                         const struct of_phandle_args *args);
>>         unsigned int of_pwm_n_cells;
>> };
>>
>> And having in driver_probe() something like this:
>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE;
>> pwmchip_add(&driver_data->chip);
>> Since the PWM push-pull mode imply more than one output per PWM channel. And
>> validate the supported PWM modes when trying to configure one.
>>
>> Or registering, as a field in pwm_chip, how many outputs per channel are actually
>> supported by the PWM controller and then validate the supported PWM mode based
>> on this?
>> e.g.:
>> in "struct pwm_chip". e.g.:
>> struct pwm_chip {
>>         struct device *dev;
>>         struct list_head list;
>>         const struct pwm_ops *ops;
>>         int base;
>>         unsigned int npwm;
>> 	unsigned int pwm_outputs;	/* Number of supported outputs per channel */>>
>>         struct pwm_device *pwms;
>>
>>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>                                         const struct of_phandle_args *args);
>>         unsigned int of_pwm_n_cells;
>> };
>>
>> In driver_probe():
>> //...
>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */
>> pwmchip_add(&driver_data->chip);
> 
> I think I'd prefer something more flexible. I also think this can't be
> per-chip, but really has to be at the granularity of single PWM
> channels. How about something like this:
> 
> 	struct pwm_caps {
> 		...
> 	};
> 
> 	int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps);
> 
> From a high-level perspective that would give users an easy way to
> obtain the capabilities of the PWM they're handed.
I forgot to mention, I don't know if you had time to look over the other
patch series I send to extend the PWM framework:
"[PATCH v2 0/2] extends PWM framework to support PWM dead-times"
(v2 since the auto build robot prompted me some compilation errors).
In that series I proposed some extensions to be able to configure
dead-times for PWM channels. Those changes are also specific to PWM
controllers which has more than 1 output per PWM channel. Taking into
account that:
- this is also specific to PWM controllers with more than 1 output per
PWM channel and
- your proposed below to introduce capabilities specific to more than
one output per PWM channel and not to introduce something to specify that
the PWM chip or PWM device supports a number of X outputs per PWM channel and
- PWM dead-time is, in my opinion, some kind of PWM push-pull mode with
lower delays b/w the edges of complementary outputs: 
Do you think it would be ok (in order to also have a way to configure
PWM dead-times) to merge the changes of
"[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the
changes that I will do for push-pull mode (with your below proposals)
and have a single PWM mode for both PWM push-pull and PWM dead-times
(e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used
to configure the dead-times that will work only when the PWM device supports
push-pull mode and is configured in push-pull mode?
Something like this:

root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
-r--r--r--    1 root     root          4096 Feb  9 10:54 capture
-rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
-rw-r--r--    1 root     root          4096 Feb  9 10:54 deadtime_re	<<<< interface for configuring dead-time rising edge delay 
-rw-r--r--    1 root     root          4096 Feb  9 10:54 deadtime_fe	<<<< interface for configuring dead-time falling edge delay
-rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
-rw-r--r--    1 root     root          4096 Feb  9 10:54 mode		<<<< interface for PWM mode configuration
-rw-r--r--    1 root     root          4096 Feb  9 10:54 period
-rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
-rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent

When user switch to push-pull mode it could start configure
the dead-times (in nanoseconds) to obtain a signal of a form similar to:
                      ____0    D____P     ____      ____      ____
        PWM signal __|    |____|    |____|    |____|    |____|    |___
                        __        __        __        __        __
              PWMH ____|  |____re|  |______|  |______|  |______|  |___
                   __        __        __        __        __        __
              PWML   |______|  |____fe|  |______|  |______|  |______|

(see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover
letter),
and starting from a driver specific dead-time value (e.g. on Atmel/Microchip
PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM
outputs to actually switch to PWM push-pull mode (in this mode there is a delay
of a period introduce on a PWM ouptut) to obtain a signal similar to this one:
         __          __
 PWMH __|  |________|  |________
               __          __
 PWML ________|  |________|  |__
        <--T-->

Then we can start
> filling in that structure.
> 
> 	struct pwm_caps {
> 		unsigned long modes;
> 	};
> 
> 	#define PWM_MODE_NORMAL (1 << 0)
> 	#define PWM_MODE_COMPLEMENTARY (1 << 1)
> 	#define PWM_MODE_PUSH_PULL (1 << 2)
> 
> 	static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps,
> 						  unsigned long mode)
> 	{
> 		return (caps->modes & mode) != 0;
> 	}
> 
> and maybe something like this as a shortcut:
> 
> 	static inline bool pwm_supports_mode(struct pwm_device *pwm,
> 					     unsigned long mode)
> 	{
> 		struct pwm_caps caps;
> 		int err;
> 
> 		err = pwm_get_caps(pwm, &caps);
> 		if (err < 0)
> 			return false;
> 
> 		return pwm_caps_supports_mode(&caps, mode);
> 	}
> 
> I think that gives us a lot of flexibility and easy extensibility for
> other capabilities. The implementation of pwm_get_caps() should probably
> call back into the driver.> 
> But that would cause a lot of boilerplate for simple cases, and would be
> a lot of work to convert all existing drivers up front. So I think we
> can add helpers to solve the normal cases. Something along these lines
> perhaps:
> 
> 	struct pwm_chip {
> 		...
> 
> 		int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps);
> 
> 		const struct pwm_caps *caps;
> 	};
> 
> 	static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
> 	{
> 		if (!pwm || !caps || !pwm->chip->caps)
> 			return -EINVAL;
> 
> 		*caps = *pwm->chip->caps;
> 
> 		return 0;
> 	}
> 
> And then perhaps even do something like this:
> 
> 	const struct pwm_caps pwm_default_caps = {
> 		.modes = PWM_MODE_NORMAL,
> 	};
> > Now we can either have a patch that sets pwm_default_caps and
> pwm_simple_get_caps() for all existing drivers, or even easier add this
> to the pwmchip_add_with_polarity() function:
> 
> 	int pwmchip_add_with_polarity(struct pwm_chip *chip,
> 				      enum pwm_polarity polarity)
> 	{
> 		...
> 
> 		if (!chip->get_caps && !chip->caps) {
> 			chip->get_caps = pwm_simple_get_caps;
> 			chip->caps = pwm_default_caps;
> 		}
> 
> 		...
> 	}
> 
What about having a new member in "struct pwm_ops" something like this:
struct pwm_ops {
// ...
	int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps);
// ...
};
to get from the driver the per PWM capabilities.

And maybe to have, in core.c:
int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
{
	struct pwm_chip *chip = pwm->chip;
	int ret = 0;

	if (!pwm || !chip || !chip->ops || !caps)
		return -EINVAL;

	if (chip->ops->get_caps)
		ret = chip->ops->get_caps(chip, pwm, caps);
	else
		caps = &pwm_default_caps;

	return ret;
}

> That way, every driver, unless upgraded with a custom ->get_caps() would
> get the defaults, which encode the current assumptions of the framework.
> 
> I think what we'd also want to do is make sure that every driver always
> at least implement NORMAL mode and perhaps even fail to register those
> that don't. Not sure we want to go that far, but those are the current
> assumptions, so devices must be supporting it already anyway.
> 
>>> A possibly simpler approach might be to restrict this to the driver that
>>> you need this for. It looks like you will be mainly using this via sysfs
>>> and in that case you might be able to just extend what information is
>>> exposed in sysfs for the particular PWM you're dealing with.
>> By this you mean exporting the sysfs attributes only for my driver or possibly
>> other drivers interested in this new feature? I may have another in kernel driver
>> which may try to use this feature.
> 
> If you've got another driver that will want to use this, I'm leaning
> towards going with the fully featured approach.
I'm thinking at PWM regulator driver. I worked lately on adding code to PWM
framework in order to support what our PWM controller calls PWM triggers.
These are external input received directly by the PWM controller which let
it change the PWM output waveform, as long as the trigger is active, only
at the controller level (no interaction with kernel code interaction -
no IRQ received on trigger activation).
This could be used, e.g., in regulators with feedback. I'm thinking that this
could be useful for PWM regulator driver when it is using a PWM channel
in push-pull mode and with trigger configuration.

I have create that code on top of push-pull mode modifications since
I want to take advantage of push-pull mode while using PWM triggers.
Of course this is something I didn't introduce (I can give more details
on this if you want).

Thank you,
Claudiu

Ideally you would try to
> implement both at once so we can get a better feeling about whether or
> not the framework changes are adequate to cover at least the first two
> cases, which is much more reassuring than just being able to cover a
> single case.
> 
> One thing to consider is still that if you only use this from sysfs,
> it'll be quite a lot of work to add in-kernel infrastructure when it's
> never needed. There's still an advantage to do it anyway because it
> allows us to abstract everything away properly from the start and avoid
> fragmentation of the sysfs interface.
> 
> Thierry
> 

[toc] | [prev] | [next] | [standalone]

#1717367

2017-08-22 14:11 GMT+02:00 m18063 <Claudiu.Beznea@microchip.com>:
> Hi Thierry,
>
> I added few other comments below. Please let me know what you think.
>
> Thank you,
> Claudiu
>
> On 21.08.2017 14:25, Thierry Reding wrote:
>> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote:
>>> Hi Thierry,
>>>
>>> Thank you for your response. I added few comments below.
>>>
>>> Thank you,
>>> Claudiu
>>>
>>> On 21.08.2017 11:25, Thierry Reding wrote:
>>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote:
>>>>> Hi all,
>>>>>
>>>>> Please give feedback on these patches which extends the PWM
>>>>> framework in order to support multiple PWM signal types.
>>>>> Since I didn't receive any inputs on RFC series I'm resending it as
>>>>> normal patch series.
>>>>>
>>>>> The current patch series add the following PWM signal types:
>>>>> - PWM complementary signals
>>>>> - PWM push-pull signal
>>>>>
>>>>> Definition of PWM complementary mode:
>>>>> For a PWM controllers with more than one outputs per PWM channel,
>>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals
>>>>> have opposite levels, same duration and same starting times,
>>>>> as in the following diagram:
>>>>>
>>>>>         __    __    __    __
>>>>> PWMH __|  |__|  |__|  |__|  |__
>>>>>      __    __    __    __    __
>>>>> PWML   |__|  |__|  |__|  |__|
>>>>>        <--T-->
>>>>> Where T is the signal period.
>>>>>
>>>>> Definition of PWM push-pull mode:
>>>>> For PWM controllers with more than one outputs per PWM channel,
>>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals
>>>>> have same levels, same duration and are delayed until the begining
>>>>> of the next period, as in the following diagram:
>>>>>
>>>>>         __          __
>>>>> PWMH __|  |________|  |________
>>>>>               __          __
>>>>> PWML ________|  |________|  |__
>>>>>        <--T-->
>>>>>
>>>>> Where T is the signal period.
>>>>>
>>>>> These output signals could be configured by setting PWM mode
>>>>> (a new input in sysfs has been added in order to support this
>>>>> operation).
>>>>>
>>>>> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
>>>>> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode
>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
>>>>> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
>>>>>
>>>>> The PWM push-pull mode could be usefull in applications like
>>>>> half bridge converters.
>>>>
>>>> Sorry for taking an absurdly long time to get back to you on this.
>>>>
>>>> One problem I see with this is that the PWM framework is built on the
>>>> assumption that we have a single output per PWM channel. This becomes
>>>> important when you start adding features such as this because now the
>>>> users have no way of determining whether or not the PWM they retrieve
>>>> will actually be able to do what they want.
>>> I was thinking that the framework is there to offer support in configuring
>>> the underlying hardware without taking into account how many outputs per
>>> PWM channels are actually present. It is true I only worked with Atmel/Microchip
>>> PWM controllers which for instance, SAMA5 SoC has a PWM controller
>>> with 2 outputs per PWM channel. Taking into account that the driver is
>>> already mainlined I though that the user is aware of what he is using
>>> (either a one output per PWM channel controller, or 2 outputs or
>>> more than 2) and about the underlying hardware support. I understand your
>>> position on this. I'm just explaining myself on the approach I've chose
>>> for this implementation.
>>
>> So currently we assume that there will be (at least) one output per PWM
>> channel. However there are currently no drivers that actually use any
>> output other than the "primary" one.
>>
>> That means, even if there are currently PWM controllers that support
>> multiple outputs per PWM channel, we have no code making assumptions
>> about it.
>>
>> Before we can add code that makes any assumptions about the number of
>> outputs per PWM channel, we have to add facitilities for such code to
>> detect capabilities, so that they can deal with PWMs that don't match
>> what they need.
>>
>>>> For example, let's say you have a half bridge converter driver in the
>>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing
>>>> preventing it from using a simple one-output PWM and there's no way to
>>>> check that we're actually doing something wrong.
>>> I understand your position here. I've chose this approach taking into
>>> account that the user of the PWM will be aware of the capabilities
>>> of the underlying hardware because I worked on a controller which already
>>> has a mainlined driver and which has more than one output per PWM channel
>>> and I believe there are also other controllers with this kind of capability
>>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver
>>> I saw a bool type variable in driver specific data structure (struct stm32_pwm):
>>> "bool have_complementary_output" which let me think that their controller
>>> also could support more than one output per PWM channel (I will also try
>>> to find the controller specific datasheet). At a first look I saw that also
>>> TI ECAP PWM controller supports this (it is true that I am not aware of how
>>> it is initialized in kernel, I need to check the datasheet, if it is public).
>>
>> Yes, it's quite possible that many of the controllers we currently
>> support have this feature or not. But that's not really relevant. The
>> only important bit is that none of the users know about it. We don't
>> have the means to configure anything beyond just one output. So the only
>> guarantee you will get with the current framework is that there will be
>> one output signal per PWM channel.
>>
>>>> I think any in-kernel API would have to be more fully-featured,
>>>> otherwise we're bound to run into problems. At the very least I think
>>>> we'd have to expose some sort of capabilities list.
>>> About the exposing of these capabilities would you prefer to have the
>>> supported PWM modes registered in driver probe as a new field mask
>>> in "struct pwm_chip". e.g.:
>>> struct pwm_chip {
>>>         struct device *dev;
>>>         struct list_head list;
>>>         const struct pwm_ops *ops;
>>>         int base;
>>>         unsigned int npwm;
>>>      unsigned int pwm_modes_mask;    /* bitfield of supported signal types */
>>>
>>>         struct pwm_device *pwms;
>>>
>>>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>>                                         const struct of_phandle_args *args);
>>>         unsigned int of_pwm_n_cells;
>>> };
>>>
>>> And having in driver_probe() something like this:
>>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE;
>>> pwmchip_add(&driver_data->chip);
>>> Since the PWM push-pull mode imply more than one output per PWM channel. And
>>> validate the supported PWM modes when trying to configure one.
>>>
>>> Or registering, as a field in pwm_chip, how many outputs per channel are actually
>>> supported by the PWM controller and then validate the supported PWM mode based
>>> on this?
>>> e.g.:
>>> in "struct pwm_chip". e.g.:
>>> struct pwm_chip {
>>>         struct device *dev;
>>>         struct list_head list;
>>>         const struct pwm_ops *ops;
>>>         int base;
>>>         unsigned int npwm;
>>>      unsigned int pwm_outputs;       /* Number of supported outputs per channel */>>
>>>         struct pwm_device *pwms;
>>>
>>>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>>                                         const struct of_phandle_args *args);
>>>         unsigned int of_pwm_n_cells;
>>> };
>>>
>>> In driver_probe():
>>> //...
>>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */
>>> pwmchip_add(&driver_data->chip);
>>
>> I think I'd prefer something more flexible. I also think this can't be
>> per-chip, but really has to be at the granularity of single PWM
>> channels. How about something like this:
>>
>>       struct pwm_caps {
>>               ...
>>       };
>>
>>       int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps);
>>
>> From a high-level perspective that would give users an easy way to
>> obtain the capabilities of the PWM they're handed.
> I forgot to mention, I don't know if you had time to look over the other
> patch series I send to extend the PWM framework:
> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times"
> (v2 since the auto build robot prompted me some compilation errors).
> In that series I proposed some extensions to be able to configure
> dead-times for PWM channels. Those changes are also specific to PWM
> controllers which has more than 1 output per PWM channel. Taking into
> account that:
> - this is also specific to PWM controllers with more than 1 output per
> PWM channel and
> - your proposed below to introduce capabilities specific to more than
> one output per PWM channel and not to introduce something to specify that
> the PWM chip or PWM device supports a number of X outputs per PWM channel and
> - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with
> lower delays b/w the edges of complementary outputs:
> Do you think it would be ok (in order to also have a way to configure
> PWM dead-times) to merge the changes of
> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the
> changes that I will do for push-pull mode (with your below proposals)
> and have a single PWM mode for both PWM push-pull and PWM dead-times
> (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used
> to configure the dead-times that will work only when the PWM device supports
> push-pull mode and is configured in push-pull mode?
> Something like this:
>
> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 deadtime_re    <<<< interface for configuring dead-time rising edge delay
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 deadtime_fe    <<<< interface for configuring dead-time falling edge delay
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode           <<<< interface for PWM mode configuration
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
>
> When user switch to push-pull mode it could start configure
> the dead-times (in nanoseconds) to obtain a signal of a form similar to:
>                       ____0    D____P     ____      ____      ____
>         PWM signal __|    |____|    |____|    |____|    |____|    |___
>                         __        __        __        __        __
>               PWMH ____|  |____re|  |______|  |______|  |______|  |___
>                    __        __        __        __        __        __
>               PWML   |______|  |____fe|  |______|  |______|  |______|
>
> (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover
> letter),
> and starting from a driver specific dead-time value (e.g. on Atmel/Microchip
> PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM
> outputs to actually switch to PWM push-pull mode (in this mode there is a delay
> of a period introduce on a PWM ouptut) to obtain a signal similar to this one:
>          __          __
>  PWMH __|  |________|  |________
>                __          __
>  PWML ________|  |________|  |__
>         <--T-->
>
> Then we can start
>> filling in that structure.
>>
>>       struct pwm_caps {
>>               unsigned long modes;
>>       };
>>
>>       #define PWM_MODE_NORMAL (1 << 0)
>>       #define PWM_MODE_COMPLEMENTARY (1 << 1)
>>       #define PWM_MODE_PUSH_PULL (1 << 2)
>>
>>       static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps,
>>                                                 unsigned long mode)
>>       {
>>               return (caps->modes & mode) != 0;
>>       }
>>
>> and maybe something like this as a shortcut:
>>
>>       static inline bool pwm_supports_mode(struct pwm_device *pwm,
>>                                            unsigned long mode)
>>       {
>>               struct pwm_caps caps;
>>               int err;
>>
>>               err = pwm_get_caps(pwm, &caps);
>>               if (err < 0)
>>                       return false;
>>
>>               return pwm_caps_supports_mode(&caps, mode);
>>       }
>>
>> I think that gives us a lot of flexibility and easy extensibility for
>> other capabilities. The implementation of pwm_get_caps() should probably
>> call back into the driver.>
>> But that would cause a lot of boilerplate for simple cases, and would be
>> a lot of work to convert all existing drivers up front. So I think we
>> can add helpers to solve the normal cases. Something along these lines
>> perhaps:
>>
>>       struct pwm_chip {
>>               ...
>>
>>               int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps);
>>
>>               const struct pwm_caps *caps;
>>       };
>>
>>       static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
>>       {
>>               if (!pwm || !caps || !pwm->chip->caps)
>>                       return -EINVAL;
>>
>>               *caps = *pwm->chip->caps;
>>
>>               return 0;
>>       }
>>
>> And then perhaps even do something like this:
>>
>>       const struct pwm_caps pwm_default_caps = {
>>               .modes = PWM_MODE_NORMAL,
>>       };
>> > Now we can either have a patch that sets pwm_default_caps and
>> pwm_simple_get_caps() for all existing drivers, or even easier add this
>> to the pwmchip_add_with_polarity() function:
>>
>>       int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>                                     enum pwm_polarity polarity)
>>       {
>>               ...
>>
>>               if (!chip->get_caps && !chip->caps) {
>>                       chip->get_caps = pwm_simple_get_caps;
>>                       chip->caps = pwm_default_caps;
>>               }
>>
>>               ...
>>       }
>>
> What about having a new member in "struct pwm_ops" something like this:
> struct pwm_ops {
> // ...
>         int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps);
> // ...
> };
> to get from the driver the per PWM capabilities.
>
> And maybe to have, in core.c:
> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
> {
>         struct pwm_chip *chip = pwm->chip;
>         int ret = 0;
>
>         if (!pwm || !chip || !chip->ops || !caps)
>                 return -EINVAL;
>
>         if (chip->ops->get_caps)
>                 ret = chip->ops->get_caps(chip, pwm, caps);
>         else
>                 caps = &pwm_default_caps;
>
>         return ret;
> }
>
>> That way, every driver, unless upgraded with a custom ->get_caps() would
>> get the defaults, which encode the current assumptions of the framework.
>>
>> I think what we'd also want to do is make sure that every driver always
>> at least implement NORMAL mode and perhaps even fail to register those
>> that don't. Not sure we want to go that far, but those are the current
>> assumptions, so devices must be supporting it already anyway.
>>
>>>> A possibly simpler approach might be to restrict this to the driver that
>>>> you need this for. It looks like you will be mainly using this via sysfs
>>>> and in that case you might be able to just extend what information is
>>>> exposed in sysfs for the particular PWM you're dealing with.
>>> By this you mean exporting the sysfs attributes only for my driver or possibly
>>> other drivers interested in this new feature? I may have another in kernel driver
>>> which may try to use this feature.
>>
>> If you've got another driver that will want to use this, I'm leaning
>> towards going with the fully featured approach.
> I'm thinking at PWM regulator driver. I worked lately on adding code to PWM
> framework in order to support what our PWM controller calls PWM triggers.
> These are external input received directly by the PWM controller which let
> it change the PWM output waveform, as long as the trigger is active, only
> at the controller level (no interaction with kernel code interaction -
> no IRQ received on trigger activation).
> This could be used, e.g., in regulators with feedback. I'm thinking that this
> could be useful for PWM regulator driver when it is using a PWM channel
> in push-pull mode and with trigger configuration.
>
> I have create that code on top of push-pull mode modifications since
> I want to take advantage of push-pull mode while using PWM triggers.
> Of course this is something I didn't introduce (I can give more details
> on this if you want).

STM32 PWM have the same kind of triggers features but, since the harware
block can do more than PWM, we have implemented the trigger part in IIO.
It allow use to controls (start/stop) PWMs on events or levels.

>
> Thank you,
> Claudiu
>
> Ideally you would try to
>> implement both at once so we can get a better feeling about whether or
>> not the framework changes are adequate to cover at least the first two
>> cases, which is much more reassuring than just being able to cover a
>> single case.
>>
>> One thing to consider is still that if you only use this from sysfs,
>> it'll be quite a lot of work to add in-kernel infrastructure when it's
>> never needed. There's still an advantage to do it anyway because it
>> allows us to abstract everything away properly from the start and avoid
>> fragmentation of the sysfs interface.
>>
>> Thierry
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1717385

On 22.08.2017 15:24, Benjamin Gaignard wrote:
> 2017-08-22 14:11 GMT+02:00 m18063 <Claudiu.Beznea@microchip.com>:
>> Hi Thierry,
>>
>> I added few other comments below. Please let me know what you think.
>>
>> Thank you,
>> Claudiu
>>
>> On 21.08.2017 14:25, Thierry Reding wrote:
>>> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote:
>>>> Hi Thierry,
>>>>
>>>> Thank you for your response. I added few comments below.
>>>>
>>>> Thank you,
>>>> Claudiu
>>>>
>>>> On 21.08.2017 11:25, Thierry Reding wrote:
>>>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please give feedback on these patches which extends the PWM
>>>>>> framework in order to support multiple PWM signal types.
>>>>>> Since I didn't receive any inputs on RFC series I'm resending it as
>>>>>> normal patch series.
>>>>>>
>>>>>> The current patch series add the following PWM signal types:
>>>>>> - PWM complementary signals
>>>>>> - PWM push-pull signal
>>>>>>
>>>>>> Definition of PWM complementary mode:
>>>>>> For a PWM controllers with more than one outputs per PWM channel,
>>>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals
>>>>>> have opposite levels, same duration and same starting times,
>>>>>> as in the following diagram:
>>>>>>
>>>>>>         __    __    __    __
>>>>>> PWMH __|  |__|  |__|  |__|  |__
>>>>>>      __    __    __    __    __
>>>>>> PWML   |__|  |__|  |__|  |__|
>>>>>>        <--T-->
>>>>>> Where T is the signal period.
>>>>>>
>>>>>> Definition of PWM push-pull mode:
>>>>>> For PWM controllers with more than one outputs per PWM channel,
>>>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals
>>>>>> have same levels, same duration and are delayed until the begining
>>>>>> of the next period, as in the following diagram:
>>>>>>
>>>>>>         __          __
>>>>>> PWMH __|  |________|  |________
>>>>>>               __          __
>>>>>> PWML ________|  |________|  |__
>>>>>>        <--T-->
>>>>>>
>>>>>> Where T is the signal period.
>>>>>>
>>>>>> These output signals could be configured by setting PWM mode
>>>>>> (a new input in sysfs has been added in order to support this
>>>>>> operation).
>>>>>>
>>>>>> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
>>>>>> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
>>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
>>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
>>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode
>>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
>>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
>>>>>> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
>>>>>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
>>>>>>
>>>>>> The PWM push-pull mode could be usefull in applications like
>>>>>> half bridge converters.
>>>>>
>>>>> Sorry for taking an absurdly long time to get back to you on this.
>>>>>
>>>>> One problem I see with this is that the PWM framework is built on the
>>>>> assumption that we have a single output per PWM channel. This becomes
>>>>> important when you start adding features such as this because now the
>>>>> users have no way of determining whether or not the PWM they retrieve
>>>>> will actually be able to do what they want.
>>>> I was thinking that the framework is there to offer support in configuring
>>>> the underlying hardware without taking into account how many outputs per
>>>> PWM channels are actually present. It is true I only worked with Atmel/Microchip
>>>> PWM controllers which for instance, SAMA5 SoC has a PWM controller
>>>> with 2 outputs per PWM channel. Taking into account that the driver is
>>>> already mainlined I though that the user is aware of what he is using
>>>> (either a one output per PWM channel controller, or 2 outputs or
>>>> more than 2) and about the underlying hardware support. I understand your
>>>> position on this. I'm just explaining myself on the approach I've chose
>>>> for this implementation.
>>>
>>> So currently we assume that there will be (at least) one output per PWM
>>> channel. However there are currently no drivers that actually use any
>>> output other than the "primary" one.
>>>
>>> That means, even if there are currently PWM controllers that support
>>> multiple outputs per PWM channel, we have no code making assumptions
>>> about it.
>>>
>>> Before we can add code that makes any assumptions about the number of
>>> outputs per PWM channel, we have to add facitilities for such code to
>>> detect capabilities, so that they can deal with PWMs that don't match
>>> what they need.
>>>
>>>>> For example, let's say you have a half bridge converter driver in the
>>>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing
>>>>> preventing it from using a simple one-output PWM and there's no way to
>>>>> check that we're actually doing something wrong.
>>>> I understand your position here. I've chose this approach taking into
>>>> account that the user of the PWM will be aware of the capabilities
>>>> of the underlying hardware because I worked on a controller which already
>>>> has a mainlined driver and which has more than one output per PWM channel
>>>> and I believe there are also other controllers with this kind of capability
>>>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver
>>>> I saw a bool type variable in driver specific data structure (struct stm32_pwm):
>>>> "bool have_complementary_output" which let me think that their controller
>>>> also could support more than one output per PWM channel (I will also try
>>>> to find the controller specific datasheet). At a first look I saw that also
>>>> TI ECAP PWM controller supports this (it is true that I am not aware of how
>>>> it is initialized in kernel, I need to check the datasheet, if it is public).
>>>
>>> Yes, it's quite possible that many of the controllers we currently
>>> support have this feature or not. But that's not really relevant. The
>>> only important bit is that none of the users know about it. We don't
>>> have the means to configure anything beyond just one output. So the only
>>> guarantee you will get with the current framework is that there will be
>>> one output signal per PWM channel.
>>>
>>>>> I think any in-kernel API would have to be more fully-featured,
>>>>> otherwise we're bound to run into problems. At the very least I think
>>>>> we'd have to expose some sort of capabilities list.
>>>> About the exposing of these capabilities would you prefer to have the
>>>> supported PWM modes registered in driver probe as a new field mask
>>>> in "struct pwm_chip". e.g.:
>>>> struct pwm_chip {
>>>>         struct device *dev;
>>>>         struct list_head list;
>>>>         const struct pwm_ops *ops;
>>>>         int base;
>>>>         unsigned int npwm;
>>>>      unsigned int pwm_modes_mask;    /* bitfield of supported signal types */
>>>>
>>>>         struct pwm_device *pwms;
>>>>
>>>>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>>>                                         const struct of_phandle_args *args);
>>>>         unsigned int of_pwm_n_cells;
>>>> };
>>>>
>>>> And having in driver_probe() something like this:
>>>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE;
>>>> pwmchip_add(&driver_data->chip);
>>>> Since the PWM push-pull mode imply more than one output per PWM channel. And
>>>> validate the supported PWM modes when trying to configure one.
>>>>
>>>> Or registering, as a field in pwm_chip, how many outputs per channel are actually
>>>> supported by the PWM controller and then validate the supported PWM mode based
>>>> on this?
>>>> e.g.:
>>>> in "struct pwm_chip". e.g.:
>>>> struct pwm_chip {
>>>>         struct device *dev;
>>>>         struct list_head list;
>>>>         const struct pwm_ops *ops;
>>>>         int base;
>>>>         unsigned int npwm;
>>>>      unsigned int pwm_outputs;       /* Number of supported outputs per channel */>>
>>>>         struct pwm_device *pwms;
>>>>
>>>>         struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>>>                                         const struct of_phandle_args *args);
>>>>         unsigned int of_pwm_n_cells;
>>>> };
>>>>
>>>> In driver_probe():
>>>> //...
>>>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */
>>>> pwmchip_add(&driver_data->chip);
>>>
>>> I think I'd prefer something more flexible. I also think this can't be
>>> per-chip, but really has to be at the granularity of single PWM
>>> channels. How about something like this:
>>>
>>>       struct pwm_caps {
>>>               ...
>>>       };
>>>
>>>       int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps);
>>>
>>> From a high-level perspective that would give users an easy way to
>>> obtain the capabilities of the PWM they're handed.
>> I forgot to mention, I don't know if you had time to look over the other
>> patch series I send to extend the PWM framework:
>> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times"
>> (v2 since the auto build robot prompted me some compilation errors).
>> In that series I proposed some extensions to be able to configure
>> dead-times for PWM channels. Those changes are also specific to PWM
>> controllers which has more than 1 output per PWM channel. Taking into
>> account that:
>> - this is also specific to PWM controllers with more than 1 output per
>> PWM channel and
>> - your proposed below to introduce capabilities specific to more than
>> one output per PWM channel and not to introduce something to specify that
>> the PWM chip or PWM device supports a number of X outputs per PWM channel and
>> - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with
>> lower delays b/w the edges of complementary outputs:
>> Do you think it would be ok (in order to also have a way to configure
>> PWM dead-times) to merge the changes of
>> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the
>> changes that I will do for push-pull mode (with your below proposals)
>> and have a single PWM mode for both PWM push-pull and PWM dead-times
>> (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used
>> to configure the dead-times that will work only when the PWM device supports
>> push-pull mode and is configured in push-pull mode?
>> Something like this:
>>
>> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
>> -r--r--r--    1 root     root          4096 Feb  9 10:54 capture
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 duty_cycle
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 deadtime_re    <<<< interface for configuring dead-time rising edge delay
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 deadtime_fe    <<<< interface for configuring dead-time falling edge delay
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 enable
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 mode           <<<< interface for PWM mode configuration
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 period
>> -rw-r--r--    1 root     root          4096 Feb  9 10:55 polarity
>> drwxr-xr-x    2 root     root             0 Feb  9 10:54 power
>> -rw-r--r--    1 root     root          4096 Feb  9 10:54 uevent
>>
>> When user switch to push-pull mode it could start configure
>> the dead-times (in nanoseconds) to obtain a signal of a form similar to:
>>                       ____0    D____P     ____      ____      ____
>>         PWM signal __|    |____|    |____|    |____|    |____|    |___
>>                         __        __        __        __        __
>>               PWMH ____|  |____re|  |______|  |______|  |______|  |___
>>                    __        __        __        __        __        __
>>               PWML   |______|  |____fe|  |______|  |______|  |______|
>>
>> (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover
>> letter),
>> and starting from a driver specific dead-time value (e.g. on Atmel/Microchip
>> PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM
>> outputs to actually switch to PWM push-pull mode (in this mode there is a delay
>> of a period introduce on a PWM ouptut) to obtain a signal similar to this one:
>>          __          __
>>  PWMH __|  |________|  |________
>>                __          __
>>  PWML ________|  |________|  |__
>>         <--T-->
>>
>> Then we can start
>>> filling in that structure.
>>>
>>>       struct pwm_caps {
>>>               unsigned long modes;
>>>       };
>>>
>>>       #define PWM_MODE_NORMAL (1 << 0)
>>>       #define PWM_MODE_COMPLEMENTARY (1 << 1)
>>>       #define PWM_MODE_PUSH_PULL (1 << 2)
>>>
>>>       static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps,
>>>                                                 unsigned long mode)
>>>       {
>>>               return (caps->modes & mode) != 0;
>>>       }
>>>
>>> and maybe something like this as a shortcut:
>>>
>>>       static inline bool pwm_supports_mode(struct pwm_device *pwm,
>>>                                            unsigned long mode)
>>>       {
>>>               struct pwm_caps caps;
>>>               int err;
>>>
>>>               err = pwm_get_caps(pwm, &caps);
>>>               if (err < 0)
>>>                       return false;
>>>
>>>               return pwm_caps_supports_mode(&caps, mode);
>>>       }
>>>
>>> I think that gives us a lot of flexibility and easy extensibility for
>>> other capabilities. The implementation of pwm_get_caps() should probably
>>> call back into the driver.>
>>> But that would cause a lot of boilerplate for simple cases, and would be
>>> a lot of work to convert all existing drivers up front. So I think we
>>> can add helpers to solve the normal cases. Something along these lines
>>> perhaps:
>>>
>>>       struct pwm_chip {
>>>               ...
>>>
>>>               int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps);
>>>
>>>               const struct pwm_caps *caps;
>>>       };
>>>
>>>       static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
>>>       {
>>>               if (!pwm || !caps || !pwm->chip->caps)
>>>                       return -EINVAL;
>>>
>>>               *caps = *pwm->chip->caps;
>>>
>>>               return 0;
>>>       }
>>>
>>> And then perhaps even do something like this:
>>>
>>>       const struct pwm_caps pwm_default_caps = {
>>>               .modes = PWM_MODE_NORMAL,
>>>       };
>>>> Now we can either have a patch that sets pwm_default_caps and
>>> pwm_simple_get_caps() for all existing drivers, or even easier add this
>>> to the pwmchip_add_with_polarity() function:
>>>
>>>       int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>>                                     enum pwm_polarity polarity)
>>>       {
>>>               ...
>>>
>>>               if (!chip->get_caps && !chip->caps) {
>>>                       chip->get_caps = pwm_simple_get_caps;
>>>                       chip->caps = pwm_default_caps;
>>>               }
>>>
>>>               ...
>>>       }
>>>
>> What about having a new member in "struct pwm_ops" something like this:
>> struct pwm_ops {
>> // ...
>>         int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps);
>> // ...
>> };
>> to get from the driver the per PWM capabilities.
>>
>> And maybe to have, in core.c:
>> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps)
>> {
>>         struct pwm_chip *chip = pwm->chip;
>>         int ret = 0;
>>
>>         if (!pwm || !chip || !chip->ops || !caps)
>>                 return -EINVAL;
>>
>>         if (chip->ops->get_caps)
>>                 ret = chip->ops->get_caps(chip, pwm, caps);
>>         else
>>                 caps = &pwm_default_caps;
>>
>>         return ret;
>> }
>>
>>> That way, every driver, unless upgraded with a custom ->get_caps() would
>>> get the defaults, which encode the current assumptions of the framework.
>>>
>>> I think what we'd also want to do is make sure that every driver always
>>> at least implement NORMAL mode and perhaps even fail to register those
>>> that don't. Not sure we want to go that far, but those are the current
>>> assumptions, so devices must be supporting it already anyway.
>>>
>>>>> A possibly simpler approach might be to restrict this to the driver that
>>>>> you need this for. It looks like you will be mainly using this via sysfs
>>>>> and in that case you might be able to just extend what information is
>>>>> exposed in sysfs for the particular PWM you're dealing with.
>>>> By this you mean exporting the sysfs attributes only for my driver or possibly
>>>> other drivers interested in this new feature? I may have another in kernel driver
>>>> which may try to use this feature.
>>>
>>> If you've got another driver that will want to use this, I'm leaning
>>> towards going with the fully featured approach.
>> I'm thinking at PWM regulator driver. I worked lately on adding code to PWM
>> framework in order to support what our PWM controller calls PWM triggers.
>> These are external input received directly by the PWM controller which let
>> it change the PWM output waveform, as long as the trigger is active, only
>> at the controller level (no interaction with kernel code interaction -
>> no IRQ received on trigger activation).
>> This could be used, e.g., in regulators with feedback. I'm thinking that this
>> could be useful for PWM regulator driver when it is using a PWM channel
>> in push-pull mode and with trigger configuration.
>>
>> I have create that code on top of push-pull mode modifications since
>> I want to take advantage of push-pull mode while using PWM triggers.
>> Of course this is something I didn't introduce (I can give more details
>> on this if you want).
> 
> STM32 PWM have the same kind of triggers features but, since the harware
> block can do more than PWM, we have implemented the trigger part in IIO.
> It allow use to controls (start/stop) PWMs on events or levels.
It is true! I am aware of that and followed latest changes on that STM32 PWM
driver. Atmel/Microchip PWM hardware is only PWM aware. I though at taking advantage
of the IIO subsystem and integrate the PWM in there but it looked complicated
to integrate both subsystems.
Basically, on the Atmel/Microchip PWM controller setting a specific trigger
involves setting a particular register bit. And, yes, as you said, based
on external SoC events, the PWM output waveforms is disabled/enabled (based
on the external events).

Thank you,
Claudiu
> 
>>
>> Thank you,
>> Claudiu
>>
>> Ideally you would try to
>>> implement both at once so we can get a better feeling about whether or
>>> not the framework changes are adequate to cover at least the first two
>>> cases, which is much more reassuring than just being able to cover a
>>> single case.
>>>
>>> One thing to consider is still that if you only use this from sysfs,
>>> it'll be quite a lot of work to add in-kernel infrastructure when it's
>>> never needed. There's still an advantage to do it anyway because it
>>> allows us to abstract everything away properly from the start and avoid
>>> fragmentation of the sysfs interface.
>>>
>>> Thierry
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web