On Tue, 8 Dec 2020 15:19:56 +0200 Cristian Pop <[email protected]> wrote:
> New interface is proposed for dither functionality. This future allows > composing an external signals to the selected output channel. > The dither signal can be turned on/off, scaled, inverted, or it can be > selected from different sources. > > Signed-off-by: Cristian Pop <[email protected]> Great - much easier to discuss with docs. Some comments inline. Note we need to always construct ABI with the intent that it be easy to generalize to other devices. Sometimes that leads to slightly different decisions than we might make for a single driver. > --- > .../ABI/testing/sysfs-bus-iio-dac-ad5766 | 45 +++++++++++++++++++ > 1 file changed, 45 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766 > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766 > b/Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766 > new file mode 100644 > index 000000000000..361bbd0862df > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766 > @@ -0,0 +1,45 @@ > +What: /sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_pwr > +KernelVersion: > +Contact: [email protected] > +Description: > + Dither power on/off. Write 0 to power on dither or 1 to power > it off. _enable is what we tend to have for similar cases of turning things on or off. > + > +What: > /sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_invert > +KernelVersion: > +Contact: [email protected] > +Description: > + Inverts the dither applied to the selected DAC channel. Dither > signal is > + not inverted (default) Dither signal is inverted. This one is good. Clear docs and good naming. > + > +What: > /sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_scale_available > +KernelVersion: > +Contact: [email protected] > +Description: > + Returns possible scalings available for the current channel: > + "NO_SCALING 75%_SCALING 50%_SCALING 25%_SCALING" Needs to be numbers. 75 50 25 + I'm going to guess that no scaling == 100%? If so then use 100 to represent that. > + > +What: > /sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_scale > +KernelVersion: > +Contact: [email protected] > +Description: > + Scales the dither before it is applied to the selected channel. > + NO_SCALING - No scaling > + 75%_SCALING - 75% scaling > + 50%_SCALING - 50% scaling > + 25%_SCALING - 25% scaling As above and in previous review - needs to be just a number, not a string. > + > +What: > /sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_source_available > +KernelVersion: > +Contact: [email protected] > +Description: > + Returns possible dither sources available for the selected > channel: > + "NO_DITHER N0 N1" > + > +What: > /sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_source > +KernelVersion: > +Contact: [email protected] > +Description: > + Selects dither source applied to the selected channel. > + NO_DITHER - No dither applied > + N0 - N0 dither signal applied > + N1 - N1 dither signal applied So you already have an enable. If the enable isn't set I'd assume that is same as no dither applied? If so just don't support NO_DITHER here it doesn't give us anything extra. I wondered about whether this should just be 0 or 1, but as these are effectively arbitrary labels (in general, with assumption this interface may get used for other devices!) N0, N1 is fine for this.
