On Wed, 3 Feb 2016 15:14:54 +0100
Thierry Reding [off-list ref] wrote:
On Tue, Feb 02, 2016 at 06:44:43PM -0500, David Rivshin (Allworx)
wrote:
quoted
On Tue, 2 Feb 2016 17:23:30 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
quoted
Hi,
* David Rivshin (Allworx) [off-list ref] [160201
10:23]:
quoted
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
quoted
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to
compute load and match values, it is helpful to know what
the resultant period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I
included it in case it may be of use to someone in the
future. I would have no issues with dropping this if it's
not considered worthwhile.
It's useful, but converting it as a sysfs attribute would be
great !
Hrm, yes that is an interesting thought. I imagine that many
PWM devices have similar constraints, so perhaps that would
be best handled generically in the pwm core? Maybe as new
optional get_*() ops, a modification to the config() op to
add output params, or just updating new fields in the struct
pwm directly?
Yeah for /sys entry it should be Linux generic. The other option
is to use debugfs for driver specific things.
Let's go with debugfs for this one. This is used for diagnostic
I had looked at using the dbg_show() op to add some additional
data. One thing that discouraged me from going down that path was
that it replaced the call to pwm_dbg_show() for that chip. I
wouldn't want to loose the existing output, as it is useful, but
also didn't like the thought of duplicating the logic/formatting. I
think some way to have both the standard output and add extra
output somewhere (e.g. same line, line after each pwm_device, block
after all pwm_devices on the chip) would make that path more
attractive.
Or were you thinking of a separate (per-chip) debugfs file for this
type of information?
Yes, generally I'd prefer a separate, chip-specific debugfs file for
Makes sense.
extra information. To be honest I'm not entirely sure of the
usefulness of the pwm_dbg_show() or letting drivers override it. But
I did notice that no (in-tree) drivers were currently using
pwm_dbg_show()...
that's slightly off-topic. However...
quoted
quoted
purposes and not typically needed when configuring a PWM. While
other drivers may compute similar hardware-specific values,
there's nothing generic to them. The period and duty cycle are
the generic input values and those are already exposed via
sysfs.
Just to clarify, what I was thinking of as generic was the concept
that "period/duty I asked for" and "period/duty I got" may be
different, sometimes to a substantial degree. I could imagine
userspace wanting to know the latter, which is what I think Neil
was suggesting.
For the sake of an example, the input clock for a dmtimer is
sometimes (often?) 32768Hz, which means that the real period and
duty_cycle output are limited to being a multiple of ~61035.2ns
(16384Hz). So an attempt to set a period of 100000ns (10KHz) would
result in either 61035.2ns, or 122070.4ns (8192Hz), depending on
the implementation of the driver (and patch 2 changes behavior from
the former to the latter). You can also have cases where you
desired a 33% duty cycle, but got a 25% duty cycle instead.
In a quick look, I see substantially similar calculations (i.e.
clk_rate*period_ns/10^9) in most of the other pwm drivers, which
makes sense for any that are programmed in terms of some input
clock. This type of calculation almost guarantees that requested
and actual period/ duty_cycle are going to be different to some
degree. Some drivers look like they have even more complicated
behavior, apparently due to other hardware constraints. For
instance, fsl-ftm (among others) looks to be using a power-of-2
prescaler to fit the period_cycles into a 16bit field.
Of course if the input clock rate for these types of devices is
sufficiently high (enough that the desired period is many input
clock cycles), then the difference between "desired" and "actual"
is probably small enough that noone cares. I'm not sure how common
it is that
a) there is a substantial difference, and
b) userspace cares about it, and
c) userspace didn't carefully select an achievable value already
If noone has brought it up before, then I'd guess the answer is
"not very".
... I think perhaps a better way yet would be to have drivers update
the period and duty cycle to the actual values. That way there won't
be a delta between what's really being programmed to hardware and
what shows up in sysfs. It would also give users a chance to check if
what's been programmed is within an acceptable range or not.
From a userspace ABI point of view, I was unsure whether it was
considered OK for a R/W sysfs attribute of this nature to read back
differently than it was set. To follow the previous example:
# echo 100000 > period
# cat period
122070
I didn't find any obvious precedents for that. Would it potentially
confuse userspace? Is the answer different for new drivers vs
existing ones? The obvious alternative would be a separate file (e.g.
"actual_period", or somesuch name), which avoids those questions, but
I can see how that could be considered less clean. I don't have any
preference either way.
If just updating the existing period/duty_cycle files with the
actual values is the way to go, then there is a second question:
should the struct pwm_device track both requested and actual
values in separate fields for kernel-internal users? Or should
the existing period/duty_cycle fields just be modified to hold
the actual values?
Either way, I think any changes along these lines would probably be
cleaner ontop of the atomic update work (assuming some form of that
gets merged). And since it would also be outside the scope of this
patchset, you may still want to consider the simple debug message
in this patch as at least an interim state.