On Tue, Feb 02, 2016 at 06:44:43PM -0500, David Rivshin (Allworx) wrote: > On Tue, 2 Feb 2016 17:23:30 +0100 > Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > > > On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote: > > > Hi, > > > > > > * David Rivshin (Allworx) <drivshin.allworx@xxxxxxxxx> [160201 > > > 10:23]: > > > > On Sat, 30 Jan 2016 15:51:06 +0100 > > > > Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote: > > > > > > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > > > > > <drivshin.allworx@xxxxxxxxx>: > > > > > > From: David Rivshin <drivshin@xxxxxxxxxxx> > > > > > > > > > > > > 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 <drivshin@xxxxxxxxxxx> > > > > > > --- > > > > > > > > > > > > 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 extra information. To be honest I'm not entirely sure of the usefulness of the pwm_dbg_show() or letting drivers override it. But that's slightly off-topic. However... > > 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. Thierry
Attachment:
signature.asc
Description: PGP signature
