Re: [PATCH] add Enermax DigiFanless HID/hwmon driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 18, 2015 at 01:38:40AM -0500, Frank Schaefer wrote:
> This driver adds support for the monitoring logic in the Enermax DigiFanless
> 550W (EDF550AWN) power supply.  It spans the HID and hwmon subsystems, so I
> decided it needs review from both maintainer teams.  (I'm still aiming for it
> to filter through the hwmon linux-staging repo).
> 
Not sure what you mean with that last sentence. Either case, this and the review
comment should probably not be in the commit log and thus be after the '---'.

Since this driver will go into drivers/hid, it should go through the hid tree.

Given its experimental nature, I wonder if it should reside in drivers/staging
for a while.

> Signed-off-by: Frank Schaefer <kelledin@xxxxxxxxx>
> ---
>  Documentation/hwmon/zdpms | 107 ++++++
>  drivers/hid/Kconfig       |  16 +
>  drivers/hid/Makefile      |   1 +
>  drivers/hid/hid-core.c    |   1 +
>  drivers/hid/hid-ids.h     |   1 +
>  drivers/hid/hid-zdpms.c   | 828 ++++++++++++++++++++++++++++++++++++++++++++++

Care to add yourself as maintainer ?

>  6 files changed, 954 insertions(+)
>  create mode 100644 Documentation/hwmon/zdpms
>  create mode 100644 drivers/hid/hid-zdpms.c
> 
> diff --git a/Documentation/hwmon/zdpms b/Documentation/hwmon/zdpms
> new file mode 100644
> index 0000000..98a4cdc
> --- /dev/null
> +++ b/Documentation/hwmon/zdpms
> @@ -0,0 +1,107 @@
> +Kernel driver hid-zdpms
> +=======================
> +
> +Supported chips:
> +  * Enermax DigiFanless 550W PIC (Microchip PIC32MX230F064D)
> +    Prefix: 'zdpms'
> +    Addresses scanned: USB HID space
> +    Datasheet: Still not published
> +
> +Authors:
> +        Frank Schaefer <kelledin@xxxxxxxxx>
> +
> +Module Parameters
> +-----------------
> +
> +zdpms_timeout=timeout   Set the ZDPMS transaction timeout in milliseconds
> +                        (default 100 ms).  This is clamped to a minimum of 20
> +                        milliseconds, because accuracy is spotty with anything
> +                        less.
> +                        Example: 'modprobe hid-zdpms zdpms_timeout=500'
> +zdpms_12v_config=<int>  Set the 12V rail configuration (1 for single rail, 2
> +                        for dual rail).  By default the rail configuration is
> +                        left untouched.
> +                        Example: 'modprobe hid-zdpms zdpms_12v_config=2'

Is this a good idea ? Normally such parameters are determined by the hardware,
and changing it from the OS may be harmful.

> +
> +Description
> +-----------
> +
> +The Enermax DigiFanless 550W power supply has an integrated Microchip
> +32-bit peripheral interface controller (PIC), running a specialized
> +application to monitor internal power-supply sensors and handle some
> +incidental user-controllable configuration (mainly the 12V rail
> +configuration).  This controller application communicates with the host
> +via USB raw Human Interface Device (HID) protocol, using a custom OEM
> +usage table and failing to disclose the required report descriptors via
> +standard HID enumeration.  This controller is the foundation of the
> +Enermax Zero-Delay Power Monitoring System (ZDPMS), which is apparently
> +supported by Enermax only through a proprietary Windows application.
> +
> +The Microchip PIC application implements voltage and current sensors for
> +all internal DC rails (12V1, 12V2, 5V, and 3.3V), an internal temperature
> +sensor, and an AC input voltage sensor, all of which we are able to query.
> +There are also supposedly shutdown thresholds for overvoltage protection
> +and overtemp protection, but as near as i can tell, they are hardcoded and
> +firmly out of software's reach.
> +
> +Sensor values are reported by the PIC in an odd format similar to (but
> +not quite identical to) IEEE 754 half-precision floating-point.  They
> +seem to have a 5-bit exponent and no sign field, and the leading '1' bit
> +implicit in IEEE 754 mantissa fields is apparently explicit here.  The
> +exponent also appears to be a fixed value for each sensor, implying that
> +each sensor has its own fixed range and precision.  The apparent lack of
> +a sign bit implies that all sensor readings are lower-bounded at zero.
> +
> +Voltage sensors (also known as IN sensors) report their values in volts;
> +current sensors (known as CURR sensors) report their values in amperes.
> +The only known temperature sensor reports its value in degrees Celcius.
> +Applications such as the Enermax OEM application are supposedly expected
> +to calculate watts in software using the information supplied in voltage
> +and amperage sensors (although we currently lack enough sensor readings
> +to calculate all AC input properties).
> +
> +Supposedly, if any sensor reading crosses its hardcoded alarm threshold,
> +the power supply will shut down, presumably powering down the embedded
> +Microchip PIC as well (although we have yet to confirm this empirically).
> +So any software alarms are probably superfluous.
> +
> +It is unknown how often the Microchip PIC will measure internal sensor
> +readings, although the "zero delay" terminology implies that it is near
> +instantaneous (which seems appropriate for overvoltage protection).
> +The driver queries the PIC every time userland reads a sensor token via
> +sysfs but does no internal monitoring of its own.
> +
> +Known Issues
> +------------
> +
> +This driver identifies the Enermax PIC controller application by USB
> +vendor and product ID.  Unfortunately, it is specifically advertising a
> +Microchip vendor ID rather than an Enermax vendor ID, and the product ID
> +is an otherwise unknown value.  Even worse, it does not advertise any
> +vendor, product, or serial number string through HID enumeration, so I
> +have some concerns about how well Enermax has ensured uniqueness of
> +enumerated IDs. There's also an open question as to whether Enermax has
> +incorporated similar technology in any of their other power supplies, how
> +those other products enumerate via HID, and how the driver will respond
> +upon seeing one of these other devices.
> +
> +Also, since pretty much all my knowledge of this hardware is based on
> +dissecting USB traffic generated by the Enermax OEM application, I'm not
> +entirely certain of some aspects, such as the sensor-reading format.  For
> +example, I've never seen either of the two high bits of the exponent unset,
> +or the high bit of the mantissa set--so for all I know, these bits may not
> +mean what I think they mean.
> +
> +Furthermore, although the OEM application is able to determine the apparent
> +power usage (volt-amperes), the actual power usage (watts), and the power
> +factor for the AC inputs, I haven't learned how it determines any of those
> +values.
> +
> +Finally, there is a possibility that upon non-fatal "alarm" conditions,
> +the controller may notify the host via an unexpected HID event.  I have no
> +idea what this will look like, and if there's a host-initiated query or
> +command in flight, any such unexpected event may interfere with the expected
> +response.  To figure out how to deal with this (or even see what it looks
> +like without vendor documentation), it may be necessary to construct a
> +benchtop load setup specifically to trigger such alarm conditions.  This is
> +difficult and somewhat dangerous, for obvious reasons.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index cc4c664..de01050 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -849,6 +849,22 @@ config HID_XINMO
>  	standard. Currently only supports the Xin-Mo Dual Arcade. Say Y here
>  	if you have a Xin-Mo Dual Arcade controller.
>  
> +config HID_ZDPMS
> +	tristate "Enermax ZDPMS support (EXPERIMENTAL)"
> +	depends on HID && HWMON
> +	default n
> +	---help---
> +	Support for monitoring sensors and software-configurable settings
> +	embedded in the Enermax DigiFanless 550W power supply.  This driver
> +	registers as a hwmon driver to expose these sensors for use by
> +	libsensors-based applications.
> +
> +	This driver is largely based on dissecting captures of USB traffic
> +	generated by the Enermax ZDPMS OEM Windows application.  At least
> +	two sensors are not supported (yet), and it's uncertain how this
> +	driver will behave in the presence of other Enermax products with
> +	similar (but not quite identical) functionality.
> +
"If unsure, say 'n'." seems appropriate here.

>  config HID_ZEROPLUS
>  	tristate "Zeroplus based game controller support"
>  	depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 2f8a41d..dd538dd 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_HID_TOPSEED)	+= hid-topseed.o
>  obj-$(CONFIG_HID_TWINHAN)	+= hid-twinhan.o
>  obj-$(CONFIG_HID_UCLOGIC)	+= hid-uclogic.o
>  obj-$(CONFIG_HID_XINMO)		+= hid-xinmo.o
> +obj-$(CONFIG_HID_ZDPMS)		+= hid-zdpms.o
>  obj-$(CONFIG_HID_ZEROPLUS)	+= hid-zpff.o
>  obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
>  
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index e6fce23..5d4062b 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1895,6 +1895,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_ZDPMS) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b3b225b..a9eedea 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -650,6 +650,7 @@
>  #define USB_DEVICE_ID_PICOLCD		0xc002
>  #define USB_DEVICE_ID_PICOLCD_BOOTLOADER	0xf002
>  #define USB_DEVICE_ID_PICK16F1454	0x0042
> +#define USB_DEVICE_ID_ZDPMS		0xf590
>  
>  #define USB_VENDOR_ID_MICROSOFT		0x045e
>  #define USB_DEVICE_ID_SIDEWINDER_GV	0x003b
> diff --git a/drivers/hid/hid-zdpms.c b/drivers/hid/hid-zdpms.c
> new file mode 100644
> index 0000000..905ef6a
> --- /dev/null
> +++ b/drivers/hid/hid-zdpms.c
> @@ -0,0 +1,828 @@
> +/*
> + *  Linux HID/hwmon driver for Enermax ZDPMS controller
> + *
> + *  Copyright (c) 2015 Frank Schaefer <kelledin@xxxxxxxxx>
> + *
> + *  The name "Enermax" (and possibly "DigiFanless" and "ZDPMS") are trademark
> + *  Enermax Inc (http://www.enermax.com/).
> + */
> +
> +/*

Comment split unnecessary ?

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/hid.h>
> +#include <linux/delay.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +#include <linux/mutex.h>
> +#include <linux/jiffies.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/atomic.h>
> +#include "hid-ids.h"
> +
> +#define ZDPMS_VERSION  "0.1.1"

I would suggest to drop the version number. It tends to be never updated.

> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Enermax ZDPMS controller");
> +MODULE_AUTHOR("Frank Schaefer <kelledin@xxxxxxxxx>");
> +MODULE_VERSION(ZDPMS_VERSION);
> +
> +static const struct hid_device_id zdpms_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_ZDPMS) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, zdpms_devices);
> +
> +static int zdpms_timeout = 100;
> +module_param(zdpms_timeout, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(zdpms_timeout,
> +		 "Transaction timeout in milliseconds (minimum 20)");
> +
> +static int zdpms_12v_config;
> +module_param(zdpms_12v_config, int, S_IRUGO);
> +MODULE_PARM_DESC(zdpms_12v_config,
> +		 "Set 12V rail configuration (1: single rail, 2: dual rail)");
> +
> +enum {
> +	/* Raw size for HID messages, excluding the URB header */
> +	ZDPMS_MSG_SIZE = 64,
> +
> +	/*
> +	 * No idea what these mean, but they are part of the initialization
> +	 * sequence used by the Enermax ZDPMS OEM application, with the
> +	 * single byte value 0x5 passed to each one.  I suspect they set
> +	 * something, I just don't know what.
> +	 */
> +	ZDPMS_INIT1 = 0x25,
> +	ZDPMS_INIT2 = 0x24,
> +	ZDPMS_INIT3 = 0x23,

Why the odd numbering ?

> +
> +	/*
> +	 * 12V rail configuration setting.  Passing byte value 0x1 sets
> +	 * single-rail mode, while passing byte value 0x2 sets dual-rail
> +	 * mode.  Some PSU models may support more values for more rail
> +	 * configurations, but this is what we get on our one sample.
> +	 */
> +	ZDPMS_12V_CONFIG = 0x26,
> +
> +	/* Sensor values, as near as we can tell. */
> +	ZDPMS_SENSOR_MIN = 0x27,      /* Minimum sensor value */
> +	ZDPMS_AC_V = 0x27,            /* AC input voltage level */
> +	ZDPMS_DC_12V1 = 0x28,         /*
> +				       * One of the 12V rail voltage levels
> +				       * (probably 12V1)
> +				       */
> +	ZDPMS_DC_5V = 0x29,           /* DC 5V rail voltage level */
> +	ZDPMS_DC_3V = 0x2a,           /* DC 3.3V rail voltage level */
> +	ZDPMS_DC_12V2 = 0x2b,         /*
> +				       * One of the 21V rail voltage levels.
> +				       * (probably 12V1)
> +				       */
> +	ZDPMS_DC_12V1_CURRENT = 0x2c, /* 12V1 current level in amperes */
> +	ZDPMS_DC_5V_CURRENT = 0x2d,   /* 5V current level in amperes */
> +	ZDPMS_DC_3V_CURRENT = 0x2e,   /* 3.3V current level in amperes */
> +	ZDPMS_DC_12V2_CURRENT = 0x2f, /* 12V2 current level in amperes */
> +	ZDPMS_TEMP = 0x30,            /* PSU temperature in degrees Celcius */
> +	ZDPMS_SENSOR_MAX = 0x30,      /* Maximum sensor value */
> +
> +	ZDPMS_MODEL_NUMBER = 0x32,    /*
> +				       * PSU model number string length
> +				       * (variable length)
> +				       */
> +
> +	/* Message types (a.k.a. HID report IDs) that we've seen. */
> +	ZDPMS_SET_CONFIG = 0x82,      /* Used for setting configuration. */
> +	ZDPMS_READ_SENSOR = 0x83,     /* Used for reading values. */
> +	ZDPMS_WTF_1 = 0x90,           /* No idea at all.  Doesn't get a valid
> +				       * response.
> +				      */
> +	ZDPMS_WTF_2 = 0x91,           /* As above, no idea at all. */
> +};
> +
> +static const char * const zdpms_labels[] = {
> +	"AC input voltage",
> +	"DC 12V1",
> +	"DC 5V",
> +	"DC 3.3V",
> +	"DC 12V2",
> +	"DC 12V1 current",
> +	"DC 5V current",
> +	"DC 3.3V current",
> +	"DC 12V2 current",
> +	"PSU temp",
> +};
> +
> +struct zdpms_device {
> +	/* The hid_device we belong to. */
> +	struct hid_device *hdev;
> +
> +	/* The hwmon device pointer. */
> +	struct device *hwmon_dev;
> +
> +	/* The wait queue used to track transaction replies. */
> +	wait_queue_head_t io_wait;
> +
> +	/*
> +	 * The atomic condition variable to indicate that a message is
> +	 * received.
> +	 */
> +	atomic_t msg_received;
> +
> +	/*
> +	 * The last data read by the raw event handler.  This is expected to be
> +	 * a command or query response (up to ZDPMS_MSG_SIZE bytes), and to the
> +	 * best of my knowledge, only one pending command or query is supported
> +	 * at any time.
> +	 */
> +	uint8_t data[ZDPMS_MSG_SIZE];
> +	unsigned int sz;
> +
> +	/*
> +	 * The ZDPMS model number (i.e. "EDF550AWN" for the Enermax DigiFanless
> +	 * 550W).  We don't expect this to be more than 32 bytes.
> +	 */
> +	char model[32];
> +
> +	/*
> +	 * Transaction lock mutex.  This keeps us from launching a second
> +	 * command/query while one is still pending.
> +	 */
> +	struct mutex io_lock;
> +
> +};
> +
> +/**
> + * Perform a command/response or query/response exchange with an Enermax
> + * ZDPMS controller.  The command is launched immediately through the
> + * controller's interrupt endpoint; the response is copied back for the
> + * caller to perform further analysis.  A minimum response timeout
> + * (maintained in the zdpms_timeout variable) is implicit to avoid
> + * potential deadlocks.  No validation is performed on the response
> + * content.
> + *
> + * Note that commands, queries, and responses are generally zero padded to
> + * ZDPMS_MSG_SIZE bytes.  The first byte of any message must be the report
> + * ID (typically either ZDPMS_SET_CONFIG or ZDPMS_READ_SENSOR); the second
> + * byte is supposed to be the length of the message payload, minus the first
> + * two bytes and any zero padding.  For successful commands, the ZDPMS
> + * controller appears to copy the report ID to the end of the message; for
> + * unsupported commands, the controller appears to set the length byte to
> + * 0xFF in the response.
> + *
> + * @param hdev
> + *     The hid_device to which we're sending the command/query
> + *
> + * @param msg
> + *     The command/query to send to the ZDPMS controller, starting with the
> + *     report ID and message-length byte
> + *
> + * @param sz
> + *     The size of the command/query in bytes, including the report ID and
> + *     message-length byte
> + *
> + * @param resp
> + *     A user-supplied buffer to store the response.  This MUST be at least
> + *     ZDPMS_MSG_SIZE bytes in order to avoid potential buffer overflows.
> + *
> + * @return
> + *     Returns the number of bytes read on success (usually ZDPMS_MSG_SIZE),
> + *     or a negative errno value on failure.
> + */
> +static int zdpms_xchg(struct hid_device *hdev,
> +			  const uint8_t *data,
> +			   unsigned int  sz,
> +				uint8_t *resp)

Continuation line alignment is a bit odd here. Please
use a consistent alignment, and don't unnecessarily split lines.

> +{
> +	uint8_t msg[ZDPMS_MSG_SIZE];
> +	struct zdpms_device *dev;
> +	unsigned long last_jiffy;
> +	int ret;
> +
> +	dev = hid_get_drvdata(hdev);
> +
> +	/* Copy the message and clear any padding. */
> +	memcpy(msg, data, sz);
> +
> +	if (sz < sizeof(msg))
> +		memset(msg + sz, 0, sizeof(msg) - sz);
> +
> +	mutex_lock(&(dev->io_lock));

Unnecessary ( ). See checkpatch --strict.

> +
> +	/*
> +	 * Send the command/query.  We use hid_hw_output_report() specifically
> +	 * because it will send the raw message to an interrupt endpoint.
> +	 * NOTE: No return type (WTF really?).  If it fails, we'll supposedly
> +	 * find out when we time out waiting for the response...
> +	 */
> +	hid_hw_output_report(hdev, msg, sizeof(msg));
> +
> +	/*
> +	 * Clamp the timeout to 20ms.  Any smaller and it's dicey to represent
> +	 * it in jiffies...
> +	 */
> +	if (zdpms_timeout < 20)
> +		zdpms_timeout = 20;
> +
> +	/*
> +	 * We track the last allowable jiffy, in order to avoid our timeout
> +	 * getting mistakenly prolonged by a signal storm or similar event.
> +	 */
> +	last_jiffy = jiffies + msecs_to_jiffies(zdpms_timeout);
> +
> +	do {
> +		ret = wait_event_interruptible_timeout(
> +				dev->io_wait,
> +				atomic_read(&(dev->msg_received)),

Unnecessary ( ).

> +				last_jiffy - jiffies);
> +
> +		if (ret == -ERESTARTSYS)
> +			/* Interrupted by a signal.  Keep trying... */
> +			continue;
> +
> +		if (ret < 0)
> +			/* Unspecified error.  Punt! */
> +			goto fail_mutex_locked;
> +
> +		if (ret == 0) {
> +			/* Timed out! */
> +			ret = -ETIMEDOUT;
> +			goto fail_mutex_locked;
> +		}
> +
> +		if (ret > 0)
> +			/* Message is received. */
> +			break;
> +	} while (time_before(jiffies, last_jiffy));
> +
> +	/*
> +	 * Now see about copying the received data to the user-supplied buffer.
> +	 */
> +	memcpy(resp, dev->data, dev->sz);
> +	ret = dev->sz;
> +	atomic_set(&(dev->msg_received), 0);

Unnecessary (). Around many & in the code, as it looks like.

> +
> +fail_mutex_locked:
> +	mutex_unlock(&(dev->io_lock));
> +	return ret;
> +}
> +
> +/**
> + * Sets a specific ZDPMS configuration item.  Currently-known configuration
> + * commands only take a one-byte configuration value.  Response validation
> + * is performed.
> + *
> + * @param hdev
> + *     Pointer to the hid_device structure to configure
> + *
> + * @param cmd
> + *     Device setting to configure
> + *
> + * @param val
> + *     Configuration setting to apply
> + *
> + * @return
> + *     Returns the size of the return message on success, or a negative
> + *     errno value on failure
> + */
> +static int zdpms_set(struct hid_device *hdev, uint8_t cmd, uint8_t val)
> +{
> +	uint8_t msg[8], resp[ZDPMS_MSG_SIZE];
> +	int result;
> +
> +	msg[0] = ZDPMS_SET_CONFIG;
> +	msg[1] = 0x03; /* 3-byte message */
> +	msg[2] = 0x40; /* no idea what this is supposed to mean */
> +	msg[3] = cmd;  /* Command value */
> +	msg[4] = val;
> +
> +	result = zdpms_xchg(hdev, msg, 5, resp);
> +
> +	if (result < 0)
> +		return result;
> +
> +	if (result < 6)
> +		return -EMSGSIZE;
> +
> +	if (resp[1] == 0xff)
> +		/* Unsupported command? */
> +		return -EINVAL;
> +
> +	if (resp[0] != msg[0] || resp[1] != 4 || resp[2] != msg[2] ||
> +	    resp[3] != msg[3] || resp[4] != msg[4] || resp[5] != resp[0])
> +		return -EPROTO;
> +
> +	return result;
> +}
> +
> +/**
> + * Reads the ASCII model number string from the ZDPMS controller and stores it
> + * in the caller-supplied buffer and appends a NULL terminator if possible.  If
> + * the buffer is not large enough to hold the entire string, it will be
> + * truncated to fit (but no NULL terminator will be added).
> + *
> + * @param hdev
> + *     The hid_device we're attempting to query
> + *
> + * @param buf
> + *     The caller-supplied buffer to store the model number string
> + *
> + * @param sz
> + *     The maximum buffer size (in bytes)
> + *
> + * @return
> + *     Returns the length of the model-number string in bytes (not counting
> + *     the NULL terminator), or a negative errno value on failure.  The
> + *     returned string length may be larger than the supplied buffer size.
> + *
> + */
> +static int zdpms_read_model(struct hid_device *hdev, char *buf, size_t sz)
> +{
> +	uint8_t msg[8], resp[ZDPMS_MSG_SIZE];
> +	int result;
> +
> +	msg[0] = ZDPMS_READ_SENSOR;
> +	msg[1] = 0x2; /* 2-byte message */
> +	msg[2] = 0x40; /* nope, still no idea what this is for. */
> +	msg[3] = ZDPMS_MODEL_NUMBER;
> +	result = zdpms_xchg(hdev, msg, 4, resp);
> +
> +	if (result < 0)
> +		return result;
> +
> +	if (result < 6)
> +		return -EMSGSIZE;
> +
> +	if (resp[1] == 0xff)
> +		return -EINVAL;
> +
> +	if (resp[1] + 2 > ZDPMS_MSG_SIZE || resp[0] != msg[0] ||
> +	    resp[2] != msg[2] || resp[3] != msg[3] ||
> +	    resp[resp[1]+1] != resp[0])
> +		return -EPROTO;
> +
> +	result = resp[1] - 3;
> +
> +	if (result < sz) {
> +		/*
> +		 * Good news, we can fit the entire model name in the supplied
> +		 * buffer, complete with a NULL terminator.
> +		 */
> +		memcpy(buf, resp + 4, result);
> +		buf[result] = '\0';

You are setting the terminating 0 several times, here and in the calling code.
Also, in the calling code, you dump a warning message if the
result is too large.

Given that,
	memcpy(buf, resp + 4, min(result, sz));
would be much simpler.

> +	} else
> +		/* Whoops, we can only fit in part of it. */
> +		memcpy(buf, resp + 4, sz);
> +
checkpatch should complain here. Yes, it does, with --strict,
and --strict points out a few other problems. Please fix.

> +	return result;
> +}
> +
> +/**
> + * Query a sensor reading from a ZDPMS controller.  This reading is reported
> + * in millivolts, milliamperes, or millidegrees Celcius, in order to conform
> + * with the Linux hwmon subsystem expectations.
> + *
> + * @param hdev
> + *     The hid_device we're attempting to query
> + *
> + * @param sensor
> + *     The sensor index we're attempting to query
> + *
> + * @return
> + *     Returns the sensor value (currently always >=0) on success, or a
> + *     negative errno value on failure.
> + *
> + */
> +static int zdpms_read_sensor(struct hid_device *hdev, uint8_t sensor)
> +{
> +	uint8_t msg[8], resp[ZDPMS_MSG_SIZE];
> +	int result;
> +	uint32_t val;
> +	unsigned int exponent;
> +
> +	msg[0] = ZDPMS_READ_SENSOR;
> +	msg[1] = 0x2; /* 2-byte message */
> +	msg[2] = 0x40; /* no idea what this is supposed to mean */
> +	msg[3] = sensor; /* Sensor value */
> +	result = zdpms_xchg(hdev, msg, 4, resp);
> +
> +	if (result < 4)
> +		return result;

So 0..3 are acceptable return values and indicate a valid sensor value ?

> +
> +	if (result < 7)
> +		return -EMSGSIZE;
> +
> +	if (resp[1] == 0xff)
> +		/* Nonexistent sensor? */
> +		return -ENODEV;
> +
> +	if (resp[0] != msg[0] || resp[1] != 5 || resp[2] != msg[2] ||
> +	    resp[3] != msg[3] || resp[6] != resp[0])
> +		return -EPROTO;
> +
> +	/*
> +	 * The sensor value is encoded as little-endian quasi-IEEE 754, in
> +	 * bytes 4 and 5.
> +	 */
> +	val = ((resp[5] & 7) << 8) | resp[4];
> +	exponent = resp[5] >> 3;
> +
> +	/*
> +	 * All hwmon sensors we support are supposed to be reported in
> +	 * milliunits (millivolts, millidegrees Celcius, or milliamperes).
> +	 * Given that the maximum possible value for the mantissa is 2^11
> +	 * (2048), we'll remain well within 32-bit integer range even after
> +	 * applying this multiplier.
> +	 */
> +	val *= 1000;
> +
> +	/*
> +	 * Apply the exponent shift *after* applying the units multiplier, so we
> +	 * preserve as much accuracy as possible.
> +	 */
> +	val >>= (32 - exponent);
> +	return val;
> +}
> +
> +/**
> + * Callback for reporting ZDPMS sensor labels via sysfs, in accordance with
> + * standard hwmon conventions.
> + *
> + * @param dev
> + *     hwmon device pointer.  The corresponding zdpms_device structure is
> + *     referenced by the dev_get_drvdata() result and contains a pointer to
> + *     the associated hid_device structure of the ZDPMS controller.
> + *
> + * @param attr
> + *     sysfs device attribute pointer.  The corresponding sensor device
> + *     attribute can be deduced from this and supplies the desired ZDPMS
> + *     sensor ID.
> + *
> + * @param buf
> + *     buffer for storing a string representation of the sensor reading.
> + *
> + * @return
> + *     Returns the size of the string representation stored in the buffer,
> + *     not including the NULL terminator, or a negative errno value on
> + *     failure.
> + *
> + */
> +static ssize_t zdpms_show_label(struct device *dev,
> +		      struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct sensor_device_attribute *sens_attr;
> +	struct zdpms_device            *zdev;
> +	struct hid_device              *hdev;
> +	unsigned int                    idx;

I am personally not a friend of aligned variable names.
Either case, you use it inconsistently. I would suggest to drop
the alignment in variable declarations. Change the longest one later,
and you end up with a merge mess.

> +
> +	sens_attr = to_sensor_dev_attr(attr);
> +	zdev = dev_get_drvdata(dev);
> +	hdev = zdev->hdev;
> +	idx = sens_attr->index;
> +
> +	/* We use a straight sequential array as a lookup table. */
> +	idx -= ZDPMS_SENSOR_MIN;
> +
> +	return sprintf(buf, "%s\n", zdpms_labels[idx]);
> +}
> +
> +/**
> + * Callback for reporting ZDPMS sensor values via sysfs, in accordance with
> + * standard hwmon conventions.
> + *
> + * @param dev
> + *     hwmon device pointer.  The corresponding zdpms_device structure is
> + *     referenced by the dev_get_drvdata() result and contains a pointer to
> + *     the associated hid_device structure of the ZDPMS controller.
> + *
> + * @param attr
> + *     sysfs device attribute pointer.  The corresponding sensor device
> + *     attribute can be deduced from this and supplies the desired ZDPMS
> + *     sensor ID.
> + *
> + * @param buf
> + *     buffer for storing a string representation of the sensor reading.
> + *
> + * @return
> + *     Returns the size of the string representation stored in the buffer,
> + *     not including the NULL terminator, or a negative errno value on
> + *     failure.
> + *
> + */
> +static ssize_t zdpms_show_sensor(struct device *dev,
> +		       struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct sensor_device_attribute *sens_attr;
> +	struct zdpms_device            *zdev;
> +	struct hid_device              *hdev;
> +	unsigned int                    idx;
> +	int                             val;
> +
> +	sens_attr = to_sensor_dev_attr(attr);
> +	zdev = dev_get_drvdata(dev);
> +	hdev = zdev->hdev;
> +	idx = sens_attr->index;
> +	val = zdpms_read_sensor(hdev, idx);
> +
> +	if (val < 0)
> +		return val;
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +/**
> + * Callback for reporting raw HID events from a ZDPMS controller.  ZDPMS
> + * controllers generally only send HID events in response to commands or
> + * queries sent to their interrupt endpoints; generally only one such
> + * command or query is expected to be pending at any time.
> + *
> + * @param hdev
> + *     Pointer to the HID device (ZDPMS controller) responsible for this
> + *     HID event
> + *
> + * @param report
> + *     Pointer to the HID report structure for this event.  This is
> + *     generally ignored, since we prefer to parse raw messages.
> + *
> + * @param data
> + *     Pointer to the HID data payload from this event (not including any
> + *     headers such as the USB Request Block (URB)
> + *
> + * @param sz
> + *     Size of the HID data payload from this event (usually ZDPMS_MSG_SIZE
> + *     bytes).
> + *
> + * @return
> + *     In accordance with the kernel HID API, returns 0 when the event has
> + *     been successfully parsed, or -1 if a parse error occurs.
> + *
> + */
> +static int zdpms_raw_event(struct hid_device *hdev,
> +			   struct hid_report *report,
> +					  u8 *data,
> +					 int  sz)
> +{
> +	struct zdpms_device *dev;
> +
> +	dev = hid_get_drvdata(hdev);
> +
> +	/*
> +	 * We shouldn't receive any messages larger than ZDPMS_MSG_SIZE bytes.
> +	 * If we do, we need to clip it so it fits in our buffer.
> +	 */
> +	if (sz > ZDPMS_MSG_SIZE) {
> +		hid_warn(hdev,
> +			 "truncated oversize %d-byte event message!\n",
> +			 sz);
> +
> +		sz = ZDPMS_MSG_SIZE;
> +	}
> +
> +	memcpy(dev->data, data, sz);
> +	dev->sz = sz;
> +
> +	if (atomic_inc_return(&(dev->msg_received)) > 1)
> +		/*
> +		 * This implies we already had a message pending, and we just
> +		 * overwrote it.  D'oh!
> +		 */
> +		hid_warn(hdev, "overwriting unacknowledged message\n");
> +
> +	/* Now wake up anyone waiting on a transaction to complete. */
> +	wake_up_interruptible(&(dev->io_wait));
> +	return 0;
> +}
> +
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_12V1);
> +static SENSOR_DEVICE_ATTR(in1_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_12V1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_5V);
> +static SENSOR_DEVICE_ATTR(in2_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_5V);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_3V);
> +static SENSOR_DEVICE_ATTR(in3_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_3V);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_12V2);
> +static SENSOR_DEVICE_ATTR(in4_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_12V2);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_AC_V);
> +static SENSOR_DEVICE_ATTR(in5_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_AC_V);
> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_12V1_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr1_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_12V1_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_5V_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr2_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_5V_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr3_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_3V_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr3_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_3V_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr4_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_DC_12V2_CURRENT);
> +static SENSOR_DEVICE_ATTR(curr4_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_DC_12V2_CURRENT);
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, zdpms_show_sensor, NULL,
> +			  ZDPMS_TEMP);
> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, zdpms_show_label, NULL,
> +			  ZDPMS_TEMP);
> +
> +static struct attribute *zdpms_attrs[] = {
> +	&sensor_dev_attr_in1_label.dev_attr.attr,
> +	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	&sensor_dev_attr_in2_label.dev_attr.attr,
> +	&sensor_dev_attr_in2_input.dev_attr.attr,
> +	&sensor_dev_attr_in3_label.dev_attr.attr,
> +	&sensor_dev_attr_in3_input.dev_attr.attr,
> +	&sensor_dev_attr_in4_label.dev_attr.attr,
> +	&sensor_dev_attr_in4_input.dev_attr.attr,
> +	&sensor_dev_attr_in5_label.dev_attr.attr,
> +	&sensor_dev_attr_in5_input.dev_attr.attr,
> +	&sensor_dev_attr_curr1_label.dev_attr.attr,
> +	&sensor_dev_attr_curr1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr2_label.dev_attr.attr,
> +	&sensor_dev_attr_curr2_input.dev_attr.attr,
> +	&sensor_dev_attr_curr3_label.dev_attr.attr,
> +	&sensor_dev_attr_curr3_input.dev_attr.attr,
> +	&sensor_dev_attr_curr4_label.dev_attr.attr,
> +	&sensor_dev_attr_curr4_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(zdpms);
> +
> +static int zdpms_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct zdpms_device *dev;
> +	int ret;
> +
> +	ret = hid_parse(hdev);
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "parse failed\n");
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "hw start failed\n");
> +		return ret;
> +	}
> +
> +	ret = hid_hw_open(hdev);
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "hw open failed\n");
> +		goto fail_hid_started;
> +	}
> +
> +	ret = hid_hw_power(hdev, PM_HINT_FULLON);
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "power management error: %d\n", ret);
> +		goto fail_hid_open;
> +	}
> +
> +	dev = devm_kzalloc(&(hdev->dev),
> +			   sizeof(struct zdpms_device),
> +			   GFP_KERNEL);
> +
> +	if (dev == NULL) {
> +		ret = -ENOMEM;
> +		goto fail_hid_power_on;
> +	}
> +
> +	/* Initialize the private device data structure. */
> +	dev->hdev = hdev;
> +	init_waitqueue_head(&(dev->io_wait));
> +	atomic_set(&(dev->msg_received), 0);
> +	mutex_init(&(dev->io_lock));
> +	hid_set_drvdata(hdev, dev);
> +
> +	/* Allow event processing during probe. */
> +	hid_device_io_start(hdev);
> +
> +	/* Perform the initialization sequence */
> +	ret = zdpms_set(hdev, ZDPMS_INIT1, 0x5);
> +
> +	if (ret >= 0) {
> +		ret = zdpms_set(hdev, ZDPMS_INIT2, 0x5);
> +
> +		if (ret >= 0)
> +			ret = zdpms_set(hdev, ZDPMS_INIT3, 0x5);
> +	}
> +
> +	if (ret < 0) {
> +		hid_err(hdev,
> +			"Primary initialization sequence FAILED (error %d)\n",
> +			ret);
> +
> +		goto fail_io_started;
> +	}
> +
> +	if (zdpms_12v_config > 0 && zdpms_12v_config < 3) {
> +		hid_info(hdev, "Setting 12V %s-rail configuration...",
> +			 zdpms_12v_config == 1 ? "single" : "dual");
> +
> +		ret = zdpms_set(hdev, ZDPMS_12V_CONFIG, zdpms_12v_config);
> +
> +		if (ret < 0) {
> +			hid_err(hdev,
> +				"12V rail configuration FAILED (error %d)\n",
> +				ret);
> +
> +			goto fail_io_started;
> +		}
> +	}
> +
> +	ret = zdpms_read_model(hdev, dev->model, sizeof(dev->model));
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "Model number query FAILED (error %d)\n", ret);
> +		goto fail_io_started;
> +	}
> +
> +	if (ret >= sizeof(dev->model)) {
> +		hid_warn(hdev,
> +			 "model number truncated to %ld bytes (full size %d bytes)\n",
> +			 sizeof(dev->model) - 1, ret);

Is this really worth a warning ?

> +
> +		dev->model[sizeof(dev->model) - 1] = '\0';
> +	} else
> +		dev->model[ret] = '\0';
> +
> +	hid_info(hdev, "Detected model number: %s\n", dev->model);
> +
> +	/* Undo hid_device_io_start(). */
> +	hid_device_io_stop(hdev);
> +
> +	/* Now register this as an hwmon device. */
> +	dev->hwmon_dev = devm_hwmon_device_register_with_groups(&(hdev->dev),
> +								"zdpms",
> +								dev,
> +								zdpms_groups);
> +
> +	ret = PTR_ERR_OR_ZERO(dev->hwmon_dev);
> +
> +	if (ret < 0) {
> +		hid_err(hdev, "hwmon device registration FAILED (error %d)\n",
> +			ret);
> +
> +		goto fail_data_alloced;
> +	}
> +
> +	return 0;
> +
> +fail_io_started:
> +	hid_device_io_stop(hdev);
> +fail_data_alloced:
> +	hid_set_drvdata(hdev, NULL);
Is this necessary ?

> +	devm_kfree(&(hdev->dev), dev);

Unnecessary.

> +fail_hid_power_on:
> +	hid_hw_power(hdev, PM_HINT_NORMAL);
> +fail_hid_open:
> +	hid_hw_close(hdev);
> +fail_hid_started:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void zdpms_remove(struct hid_device *hdev)
> +{
> +	struct zdpms_device *dev;
> +
> +	dev = hid_get_drvdata(hdev);
> +	mutex_destroy(&(dev->io_lock));
> +	hid_hw_power(hdev, PM_HINT_NORMAL);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static struct hid_driver zdpms_driver = {
> +	.name       = "zdpms",
> +	.id_table   = zdpms_devices,
> +	.probe      = zdpms_probe,
> +	.remove     = zdpms_remove,
> +	.raw_event  = zdpms_raw_event,
> +};
> +
> +module_hid_driver(zdpms_driver);
> +
> -- 
> 2.1.4
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@xxxxxxxxxxxxxx
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors



[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux