On Wed, Mar 28, 2018 at 1:23 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On Wed, Mar 28, 2018 at 12:17:34PM -0700, Tim Harvey wrote: >> On Wed, Mar 28, 2018 at 9:24 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: >> > On Wed, Mar 28, 2018 at 08:14:00AM -0700, Tim Harvey wrote: >> >> This patch adds documentation of device-tree bindings for the >> >> Gateworks System Controller (GSC). >> >> >> >> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx> >> >> --- >> >> v3: >> >> - replaced _ with - >> >> - remove input bindings >> >> - added full description of hwmon >> >> - fix unit address of hwmon child nodes >> >> >> >> --- >> >> .../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++ >> >> 1 file changed, 135 insertions(+) >> >> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt >> >> >> >> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt >> >> new file mode 100644 >> >> index 0000000..8f530ed >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt >> >> @@ -0,0 +1,135 @@ >> >> +Gateworks System Controller multi-function device >> >> + >> >> +The GSC is a Multifunction I2C slave device with the following submodules: >> >> +- WDT >> >> +- GPIO >> >> +- Pushbutton controller >> >> +- HWMON >> >> + >> >> +Required properties: >> >> +- compatible : Must be "gw,gsc" >> >> +- reg: I2C address of the device >> >> +- interrupts: interrupt triggered by GSC_IRQ# signal >> >> +- interrupt-parent: Interrupt controller GSC is connected to >> >> +- #interrupt-cells: should be <1>, index of the interrupt within the >> >> + controller, in accordance with the "one cell" variant of >> >> + <devicetree/bindings/interrupt-controller/interrupt.txt> >> >> + >> >> +Optional nodes: >> >> +* watchdog: >> >> +The GSC provides a Watchdog monitor which can power cycle the board's >> >> +primary power supply on most board models when tripped. >> >> + >> >> +Required watchdog properties: >> >> +- compatible: must be "gw,gsc-watchdog" >> >> + >> >> +* hwmon: >> >> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for >> >> +temperature and/or voltage monitoring. >> >> + >> >> +Required hwmon properties: >> >> +- compatible: must be "gw,gsc-hwmon" >> >> + >> > >> > "hwmon" is a very Linux specific term. It might make sense to find a more >> > generic term. >> >> The 'hwmon' driver supports child nodes that fall into the following category: >> - temperature sensor (GSC internal temperature sensor - i2c registers >> returns value in C*10) >> - voltage rails (two types here; cooked: i2c registers return >> pre-scaled value in mV), raw: i2c registers return a raw ADC value >> that must be scaled based on ADC internal ref voltage and resolution >> and adjusted for a voltage divider to convert to mV >> - fan setpoints (I'll explain these below) >> >> I called the node 'gw,gsc-hwmon' because the driver fits into the >> 'hwmon' API. Isn't that appropriate here for the driver compatible >> string? >> > > Devicetree properties are supposed to be OS independent. > >> > >> >> +Optional hwmon properties: >> >> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs >> > >> > AFAIK devicetree likes to specify voltages in uV. >> >> There are currently plenty of dt props specified in mV (grep -r mV >> Documentation/devicetree/bindings/). >> > > "But so many others are speeding, why do I get a ticket ?" > > Please discuss with Rob. Yes - hoping for feedback on mV vs uV as well as naming of hwmon mfd child node. > >> > >> >> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs >> >> + >> > >> > 4096 what ? >> >> reference-voltage and resolution are used to scale the values from the >> nodes that report a raw ADC value: >> >> V = Vadc * (reference-voltage / resolution) >> >> I can provide that in bits if it makes more sense? I can also hard > > Yes, I think that would make more sense, and please describe what it means. > >> code both the resolution and the vref in the hwmon driver and remove >> it from dt as currently the only GSC that uses raw ADC values is 12bit >> with 2.5V ref. >> > > That would be even better. > >> > >> >> +Each hwmon child node defines an ADC input on the chip which the GSC may >> >> +report cooked values (ie temperature sensor based on thermister), raw values, >> >> +(ie voltage rail with a pre-scaling resistor divider), or a fan controller >> >> +setpoint. >> >> + >> >> +Required hwmon child properties: >> >> +- type: one of the following ADC types: >> >> + "gw,hwmon-temperature" - reports temperature in C*10 >> >> + "gw,hwmon-voltage" - reports a pre-scaled voltage value >> >> + "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with >> >> + vreference, resolution, and optional resistor divider >> >> + "gw,hwmon-fan" - a fan temperature setpoint in C*10 >> > >> > What is a "fan temperature setpoint" ? >> > >> >> The GSC supports a fan controller which drives a PWM signal to vary >> the speed of a fan based on the GSC internal temperature sensor. The >> FAN controller has 6 setpoints each having a fixed PWM duty-cycle but >> the temperature at which those setpoints kick in can be varies via >> registers at the 0x29 slave address (same slave address as the >> temperature sensor and voltage inputs which is why I have it in the >> hwmon driver): >> >> fan0_point - 50% PWM (default 300) >> fan1_point - 60% PWM (default 330) >> fan2_point - 70% PWM (default 360) >> fan3_point - 80% PWM (default 390) >> fan4_point - 90% PWM (default 420) >> fan5_point - 100% PWM (default 450) >> >> The values are C/10 thus if the internal GSC temp sensor is below 30C >> the fan output will be 0% duty cycle and if it hits 30C it will go to >> 50% until it hits 60% at 33C etc. >> > Please do not define your own scaling factors. pwm values are 0..255, > and temperatures are in milli-degrees C. > >> That is the hardware implementation that I'm trying to abstract and >> define here. You pointed out the fact that the fan*_input ABI is >> read-only fan PWM and I see that now. What do you suggest I use for > > No, it isn't. It is the fan speed in RPM. > >> this feature I'm trying to implement driver support for? >> > > pwm[1-*]_auto_point[1-*]_pwm > pwm[1-*]_auto_point[1-*]_temp > pwm[1-*]_auto_point[1-*]_temp_hyst > > may be relevant. From the context, something like > > pwm1_auto_point1_pwm read-only, set to 128 > pwm1_auto_point1_temp 30000 > pwm1_auto_point2_pwm read-only, set to 153 > pwm1_auto_point2_temp 33000 > pwm1_auto_point3_pwm read-only, set to 179 > pwm1_auto_point3_temp 36000 > pwm1_auto_point4_pwm read-only, set to 204 > pwm1_auto_point4_temp 39000 > pwm1_auto_point5_pwm read-only, set to 230 > pwm1_auto_point5_temp 42000 > pwm1_auto_point6_pwm read-only, set to 255 > pwm1_auto_point6_temp 45000 > > might make sense. I like that idea! The details of the setpoints do not need to be in the dts at all. I will need to add a single property to signify that the board has a fan controller as some don't. Thanks, Tim -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
