Quoting Jonathan Cameron (2020-09-06 07:02:47) > On Thu, 3 Sep 2020 15:18:28 -0700 > Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > We need to set various bits in the hardware registers for this device to > > operate properly depending on how it is installed. Add a handful of DT > > properties to configure these things. > > > > Cc: Daniel Campello <campello@xxxxxxxxxxxx> > > Cc: Lars-Peter Clausen <lars@xxxxxxxxxx> > > Cc: Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> > > Cc: Rob Herring <robh+dt@xxxxxxxxxx> > > Cc: <devicetree@xxxxxxxxxxxxxxx> > > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx> > > Cc: Gwendal Grignou <gwendal@xxxxxxxxxxxx> > > Cc: Evan Green <evgreen@xxxxxxxxxxxx> > > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > > --- > > > > I haven't written any code to handle these properties yet. I'd rather do > > that once the binding patch is reviewed. Patch based on iio.git testing > > branch. > Makes sense to do docs first for this. Quite a bit feels like it isn't > a feature of the device configuration, but rather of the usecase. That > stuff should probably be done with a userspace interface, but you may > be able to argue me around on some of them! > > Thanks for reviewing! I'm not well read on IIO so please bear with my ignorance. > > > > .../iio/proximity/semtech,sx9310.yaml | 182 ++++++++++++++++++ > > 1 file changed, 182 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml > > index 5739074d3592..e74b81483c14 100644 > > --- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml > > @@ -40,6 +40,169 @@ properties: > > "#io-channel-cells": > > const: 1 > > > > + semtech,cs0-ground: > > + description: Indicates the CS0 sensor is connected to ground. > > + type: boolean > > This one is probably fine. I can't think of a similar interface we need > to match, but maybe Rob or someone else will have a suggestion. Ok. > > > + > > + semtech,combined-sensors: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 1, 2, 3] > > + default: 0 > > + description: > > + Which sensors are combined. 0 for CS3, 1 for CS0+CS1, 2 for CS1+CS2, > > + and 3 for all sensors. > > Make it clear in this description what 'combined' means. > Also, I think this would be better as a set of values with an anyOf match to say > <3> > <0>, <1> > <1>, <2> > <1>, <2>, <3> > > Fine to insist they are in numeric order. Ok, sure. I can make it a list of sensor numbers. > > > + > > + semtech,cs0-gain-factor: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [1, 2, 4, 8] > > + default: 1 > > + description: > > + Gain factor for CS0 (and combined if any) sensor. > > Why is this something that should be in DT as opposed to via > a userspace control? We have hardwaregain for this purpose (I think) Thanks I'm not aware of hardwaregain. That looks like it should work. > > Also we mostly use child nodes to allow us to specify characteristics > of individual channels. Got it. > > > + > > + semtech,cs1-gain-factor: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [1, 2, 4, 8] > > + default: 1 > > + description: > > + Gain factor for CS1 sensor. > > + > > + semtech,cs2-gain-factor: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [1, 2, 4, 8] > > + default: 1 > > + description: > > + Gain factor for CS2 sensor. > > + > > + semtech,resolution: > > + description: > > + Capacitance measure resolution. > > + enum: > > + - coarsest > > + - very-coarse > > + - coarse > > + - medium-coarse > > + - medium > > + - fine > > + - very-fine > > + - finest > I'd normally be very against cases like this where we have something that > feels like it should have a clear definition rather than a random wordy scale > but these are all the information I can find in the datasheet. > > I would suggest adding a specific reference to the datasheet for this one. Are you saying description should say: Capacitance measure resolution. Refer to datasheet for more details. ? > > > + > > + semtech,startup-sensor: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 1, 2, 3] > > + default: 0 > > + description: > > + Sensor used for start-up proximity detection. The combined > > + sensor is represented by 3. > > This feels like it should be a userspace control rather than in DT? I believe this is used during initial compensation, so it needs to be set before sx9310_init_compensation() runs at probe time. Probably can't be moved to userspace. > > > + > > + semtech,proxraw-strength: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 2, 4, 8] > > + default: 2 > > + description: > > + PROXRAW filter strength. A value of 0 represents off, and other values > > + represent 1-1/N. > > Having looked at the datasheet I have little or now idea of what this filter > actually is. However, what is the argument for it being in DT rather than > exposing a userspace control of some type. I only see this equation in the datasheet F(PROXRAW ; PROXUSEFUL[n-1] ; RAWFILT) = (1 - RAWFILT).PROXRAW + RAWFILT.PROXUSEFUL[n-1] and it's talking about updating PROXUSEFUL. "PROXUSEFUL update consists of filtering PROXRAW upfront to remove its high frequencies components". So presumably this filter is used to make proxraw into proxuseful so that it is a meaningful number. Is this a new knob in userspace? > > > + > > + semtech,compensate-common: > > + description: Any sensor triggers compensation of all channels. > > + type: boolean > > Compensation for what? This is for RegProxCtrl6 bit 6 AVGCOMPMETHOD. Defines the average compensation method: 0: Individual. Each sensor triggers only its own compensation 1: Common. Any sensor triggers compensation of all channels. I believe this is for the offset compensation. > > > + > > + semtech,avg-pos-strength: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295] > > + default: 16 > > + description: > > + Average positive filter strength. A value of 0 represents off and > > + UINT_MAX (4294967295) represents infinite. Other values > > + represent 1-1/N. > > I'm not sure about using UINT_MAX to represent infinity. Rob any thoughts on > this? > > Again, why does it make sense to have the filter controls in DT? Is there an IIO property for this? Seems OK to move it to userspace. > > > > + > > + semtech,cs0-prox-threshold: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [2, 4, 6, 8, 12, 16, 20, 24, 28, 32, 40, > > + 48, 56, 64, 72, 80, 88, 96, 112, 128, 144, > > + 160, 192, 224, 256, 320, 384, 512, 640, > > + 768, 1024, 1536] > > + default: 12 > > + description: > > + Proximity detection threshold for CS0 (and combined if any) sensor. > > That is definitely a userspace thing. Why would you put it in DT? > Also same comment as above for channels as child nodes Alright. Presumably this is IIO_EV_TYPE_THRESH? > > > + > > + semtech,cs1-prox-threshold: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [2, 4, 6, 8, 12, 16, 20, 24, 28, 32, 40, > > + 48, 56, 64, 72, 80, 88, 96, 112, 128, 144, > > + 160, 192, 224, 256, 320, 384, 512, 640, > > + 768, 1024, 1536] > > + default: 12 > > + description: > > + Proximity detection threshold for CS1 sensor. > > + > > + semtech,cs2-prox-threshold: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [2, 4, 6, 8, 12, 16, 20, 24, 28, 32, 40, > > + 48, 56, 64, 72, 80, 88, 96, 112, 128, 144, > > + 160, 192, 224, 256, 320, 384, 512, 640, > > + 768, 1024, 1536] > > + default: 12 > > + description: > > + Proximity detection threshold for CS2 sensor. > > + > > + semtech,cs0-body-threshold: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 300, 600, 900, 1200, 1500, 1800, 30000] > > + default: 1800 > > + description: > > + Body detection threshold for CS0 (and combined if any) sensor. > > As before, why DT plus child nodes How should I differentiate body vs. proximity thresholds in userspace? Or should I make it /sys/.../events/in_proximity0_thresh_falling_value vs. /sys/.../events/in_proximity0_thresh_rising_value? > > > + > > + semtech,cs1-body-threshold: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 300, 600, 900, 1200, 1500, 1800, 30000] > > + default: 12 > > + description: > > + Body detection threshold for CS1 sensor. > > + > > + semtech,cs2-body-threshold: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 300, 600, 900, 1200, 1500, 1800, 30000] > > + default: 12 > > + description: > > + Body detection threshold for CS2 sensor. > > + > > + semtech,hysteresis: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 6, 12, 25] > > + default: 0 > > + description: > > + The percentage of hysteresis +/- applied to proximity/body samples. > > Is this hysteresis on an event? If so we have defined ABI to control that > from userspace, though as an absolute value rather than a precentage so some > magic will be needed. Hysteresis is usually defined only the 'not event' > direction rather than +/- Is this IIO_EV_INFO_HYSTERESIS? It looks like it is applied to the threshold by shifting it right by 4, 3, or 2. I think the +/- is actually dependent on the RegProxCtrl10 bit 6 FARCOND value, so maybe that isn't a problem. We could make another value like hysteresis shift or hysteresis percentage? > > > + > > + semtech,close-debounce-samples: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 2, 4, 8] > > + default: 0 > > + description: > > + The number of close samples debounced for proximity/body thresholds. > > This feels like something that has more to do with the object motion than > the sensor setup, so perhaps should be controlled from userspace? Sure. Is there an IIO sample property? Or I should make a custom knob for this? > > > + > > + semtech,far-debounce-samples: > > + allOf: > > + - $ref: /schemas/types.yaml#definitions/uint32 > > + - enum: [0, 2, 4, 8] > > + default: 0 > > + description: > > + The number of far samples debounced for proximity/body thresholds. > > + > > required: > > - compatible > > - reg
