Re: [PATCH v4] hwmon: Add System76 Thelio Io driver

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

 



On Thu, Aug 24, 2023 at 02:39:02PM -0600, Jeremy Soller wrote:
> The System76 Thelio Io is a fan and power button LED controller. This
> driver fully supports the System76 Thelio Io, by exposing fan controls
> and sending the system suspend/resume state.
> 

You might want to consider reading
Documentation/process/submitting-patches.rst. It would tell you that

... However, for a multi-patch series, it is generally
best to avoid using In-Reply-To: to link to older versions of the
series.  This way multiple versions of the patch don't become an
unmanageable forest of references in email clients.  If a link is
helpful, you can use the https://lore.kernel.org/ redirector (e.g., in
the cover email text) to link to an earlier version of the patch series.

It might possibly make sense to also add a note saying something like
"don't send X revisions of your patch in a row".

> Signed-off-by: Jeremy Soller <jeremy@xxxxxxxxxxxx>
> ---

Missing change log. You are flooding me with patch revisions. Do you
really expect me to look into each of those to figure out what you
changed ?

Random feedback below. Since the is no explanation for what has
changed and what hasn't and why, the feedback is necessarily
incomplete.

>  Documentation/hwmon/system76-thelio-io.rst |  31 ++

Missing reference in Documentation/hwmon/index.rst

>  MAINTAINERS                                |   7 +
>  drivers/hwmon/Kconfig                      |  10 +
>  drivers/hwmon/Makefile                     |   1 +
>  drivers/hwmon/system76-thelio-io.c         | 424 +++++++++++++++++++++
>  5 files changed, 473 insertions(+)
>  create mode 100644 Documentation/hwmon/system76-thelio-io.rst
>  create mode 100644 drivers/hwmon/system76-thelio-io.c
> 
> diff --git a/Documentation/hwmon/system76-thelio-io.rst b/Documentation/hwmon/system76-thelio-io.rst
> new file mode 100644
> index 000000000000..7ca34bb47bbb
> --- /dev/null
> +++ b/Documentation/hwmon/system76-thelio-io.rst
> @@ -0,0 +1,31 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver system76-thelio-io
> +==========================
> +
> +Supported devices:
> +
> +  * System76 Thelio Io (thelio_io_2)
> +
> +Author: Jeremy Soller
> +
> +Description
> +-----------
> +
> +This driver implements the sysfs interface for the System76 Thelio Io.
> +The System76 Thelio Io is a USB device with 4 fan connectors and a
> +power button LED.
> +
> +Usage Notes
> +-----------
> +
> +Since it is a USB device, hotswapping is possible. The device is autodetected.
> +
> +Sysfs entries
> +-------------
> +
> +======================= =====================================================================
> +fan[1-4]_input		Connected fan rpm.
> +fan[1-4]_label		Shows fan connector name.
> +pwm[1-4]		Sets the fan speed. Values from 0-255.
> +======================= =====================================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 48abe1a281f2..f4e8f7bdd1f5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20738,6 +20738,13 @@ L:	platform-driver-x86@xxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	drivers/platform/x86/system76_acpi.c
>  
> +SYSTEM76 THELIO IO DRIVER
> +M:	Jeremy Soller <jeremy@xxxxxxxxxxxx>
> +M:	System76 <info@xxxxxxxxxxxx>
> +L:	linux-hwmon@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	drivers/hwmon/system76-thelio-io.c
> +
>  SYSV FILESYSTEM
>  S:	Orphan
>  F:	Documentation/filesystems/sysv-fs.rst
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 307477b8a371..fdcf0baa2669 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1965,6 +1965,16 @@ config SENSORS_SMM665
>  	  This driver can also be built as a module. If so, the module will
>  	  be called smm665.
>  
> +config SENSORS_SYSTEM76_THELIO_IO
> +	tristate "System76 Thelio Io controller"
> +	depends on HID
> +	help
> +	  If you say yes here you get support for the System76 Thelio Io
> +	  controller.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called system76-thelio-io.
> +
>  config SENSORS_ADC128D818
>  	tristate "Texas Instruments ADC128D818"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3f4b0fda0998..7cfdabb4e66f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -199,6 +199,7 @@ obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>  obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
>  obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
>  obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
> +obj-$(CONFIG_SENSORS_SYSTEM76_THELIO_IO) += system76-thelio-io.o
>  obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>  obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> diff --git a/drivers/hwmon/system76-thelio-io.c b/drivers/hwmon/system76-thelio-io.c
> new file mode 100644
> index 000000000000..4d9c2229cd3d
> --- /dev/null
> +++ b/drivers/hwmon/system76-thelio-io.c
> @@ -0,0 +1,424 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *
> + * system76-thelio-io.c - Linux driver for System76 Thelio Io
> + * Copyright (C) 2023 System76
> + *
> + * Based on:
> + * corsair-cpro.c - Linux driver for Corsair Commander Pro
> + * Copyright (C) 2020 Marius Zachmann <mail@xxxxxxxxxxxxxxxxx>
> + *
> + * This driver uses hid reports to communicate with the device to allow hidraw userspace drivers
> + * still being used. The device does not use report ids. When using hidraw and this driver
> + * simultaniously, reports could be switched.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/hid.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/suspend.h>
> +#include <linux/types.h>
> +
> +#define BUFFER_SIZE	32
> +#define REQ_TIMEOUT	300

Nit: It is customary to either add a comment describing the unit
(here: milli-seconds) or to indicate it in the define (e.g.
REQ_TIMEOUT_MS).
> +
> +#define HID_CMD		0
> +#define HID_RES		1
> +#define HID_DATA	2
> +
> +#define CMD_FAN_GET		7
> +#define CMD_FAN_SET		8
> +#define CMD_LED_SET_MODE	16
> +#define CMD_FAN_TACH		22
> +
> +struct thelio_io_device {
> +	struct hid_device *hdev;
> +	struct device *hwmon_dev;
> +#ifdef CONFIG_PM_SLEEP
> +	struct notifier_block pm_notifier;
> +#endif
> +	struct completion wait_input_report;
> +	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
> +	u8 *buffer;
> +};
> +
> +/* converts response error in buffer to errno */
> +static int thelio_io_get_errno(struct thelio_io_device *thelio_io)
> +{
> +	switch (thelio_io->buffer[HID_RES]) {
> +	case 0x00: /* success */
> +		return 0;
> +	default:
> +		return -EIO;
> +	}
> +}
> +
> +/* send command, check for error in response, response in thelio_io->buffer */
> +static int send_usb_cmd(struct thelio_io_device *thelio_io, u8 command,
> +			u8 byte1, u8 byte2, u8 byte3)
> +{
> +	int ret;
> +
> +	memset(thelio_io->buffer, 0x00, BUFFER_SIZE);
> +	thelio_io->buffer[HID_CMD] = command;
> +	thelio_io->buffer[HID_DATA] = byte1;
> +	thelio_io->buffer[HID_DATA + 1] = byte2;
> +	thelio_io->buffer[HID_DATA + 2] = byte3;
> +
> +	reinit_completion(&thelio_io->wait_input_report);
> +
> +	ret = hid_hw_output_report(thelio_io->hdev, thelio_io->buffer, BUFFER_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!wait_for_completion_timeout(&thelio_io->wait_input_report,
> +					 msecs_to_jiffies(REQ_TIMEOUT)))
> +		return -ETIMEDOUT;
> +
> +	return thelio_io_get_errno(thelio_io);
> +}
> +
> +static int thelio_io_raw_event(struct hid_device *hdev, struct hid_report *report,
> +			       u8 *data, int size)
> +{
> +	struct thelio_io_device *thelio_io = hid_get_drvdata(hdev);
> +
> +	/* only copy buffer when requested */
> +	if (completion_done(&thelio_io->wait_input_report))
> +		return 0;
> +
> +	memcpy(thelio_io->buffer, data, min(BUFFER_SIZE, size));
> +	complete(&thelio_io->wait_input_report);
> +
> +	return 0;
> +}
> +
> +/* requests and returns single data values depending on channel */
> +static int get_data(struct thelio_io_device *thelio_io, int command, int channel,
> +		    bool two_byte_data)
> +{
> +	int ret;
> +
> +	mutex_lock(&thelio_io->mutex);
> +
> +	ret = send_usb_cmd(thelio_io, command, channel, 0, 0);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = thelio_io->buffer[HID_DATA + 1];
> +	if (two_byte_data)
> +		ret |= thelio_io->buffer[HID_DATA + 2] << 8;
> +
> +out_unlock:
> +	mutex_unlock(&thelio_io->mutex);
> +	return ret;
> +}
> +
> +static int set_pwm(struct thelio_io_device *thelio_io, int channel, long val)
> +{
> +	int ret;
> +
> +	if (val < 0 || val > 255)
> +		return -EINVAL;
> +
> +	mutex_lock(&thelio_io->mutex);
> +
> +	ret = send_usb_cmd(thelio_io, CMD_FAN_SET, channel, val, 0);
> +
> +	mutex_unlock(&thelio_io->mutex);
> +	return ret;
> +}
> +
> +static int thelio_io_read_string(struct device *dev, enum hwmon_sensor_types type,
> +				 u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_label:
> +			switch (channel) {
> +			case 0:
> +				*str = "CPU Fan";
> +				return 0;
> +			case 1:
> +				*str = "Intake Fan";
> +				return 0;
> +			case 2:
> +				*str = "GPU Fan";
> +				return 0;
> +			case 3:
> +				*str = "Aux Fan";
> +				return 0;
> +			default:
> +				break;
> +			}
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int thelio_io_read(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long *val)
> +{
> +	struct thelio_io_device *thelio_io = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			ret = get_data(thelio_io, CMD_FAN_TACH, channel, true);
> +			if (ret < 0)
> +				return ret;
> +			*val = ret;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			ret = get_data(thelio_io, CMD_FAN_GET, channel, false);
> +			if (ret < 0)
> +				return ret;
> +			*val = ret;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +};
> +
> +static int thelio_io_write(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long val)
> +{
> +	struct thelio_io_device *thelio_io = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			return set_pwm(thelio_io, channel, val);
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +};
> +
> +static umode_t thelio_io_is_visible(const void *data, enum hwmon_sensor_types type,
> +				    u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			return 0444;
> +		case hwmon_fan_label:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +};
> +
> +static const struct hwmon_ops thelio_io_hwmon_ops = {
> +	.is_visible = thelio_io_is_visible,
> +	.read = thelio_io_read,
> +	.read_string = thelio_io_read_string,
> +	.write = thelio_io_write,
> +};
> +
> +static const struct hwmon_channel_info *thelio_io_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL
> +			   ),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT
> +			   ),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info thelio_io_chip_info = {
> +	.ops = &thelio_io_hwmon_ops,
> +	.info = thelio_io_info,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP

You suggested in one of your replies that is would by my responsibility
to prove that standard PM calls are insufficient. That is not the case.

> +static int thelio_io_pm(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct thelio_io_device *thelio_io = container_of(nb, struct thelio_io_device, pm_notifier);
> +
> +	switch (action) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +		mutex_lock(&thelio_io->mutex);
> +		send_usb_cmd(thelio_io, CMD_LED_SET_MODE, 0, 1, 0);
> +		mutex_unlock(&thelio_io->mutex);
> +		break;
> +
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_SUSPEND:
> +		mutex_lock(&thelio_io->mutex);
> +		send_usb_cmd(thelio_io, CMD_LED_SET_MODE, 0, 0, 0);
> +		mutex_unlock(&thelio_io->mutex);
> +		break;
> +
> +	case PM_POST_RESTORE:
> +	case PM_RESTORE_PREPARE:
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +#endif
> +
> +static int thelio_io_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct thelio_io_device *thelio_io;
> +	int ret;
> +
> +	thelio_io = devm_kzalloc(&hdev->dev, sizeof(*thelio_io), GFP_KERNEL);
> +	if (!thelio_io)
> +		return -ENOMEM;
> +
> +	thelio_io->buffer = devm_kmalloc(&hdev->dev, BUFFER_SIZE, GFP_KERNEL);
> +	if (!thelio_io->buffer)
> +		return -ENOMEM;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		goto out_hw_stop;
> +
> +	thelio_io->hdev = hdev;
> +	hid_set_drvdata(hdev, thelio_io);
> +	mutex_init(&thelio_io->mutex);
> +	init_completion(&thelio_io->wait_input_report);
> +
> +	hid_device_io_start(hdev);
> +

Just in case the below is _really_ necessary, which seems unlikely to me,
why call hid_device_io_start() even if no hwmon device is going to be
registered ? Why start IO on a device that isn't doing anything ?

> +	if (hdev->maxcollection == 1 && hdev->collection[0].usage == 0xFF600061) {
> +		thelio_io->hwmon_dev = hwmon_device_register_with_info(&hdev->dev,
> +								       "system76_thelio_io",
> +								       thelio_io,
> +								       &thelio_io_chip_info,
> +								       0);
> +		if (IS_ERR(thelio_io->hwmon_dev)) {
> +			ret = PTR_ERR(thelio_io->hwmon_dev);
> +			goto out_hw_close;
> +		}
> +
> +	#ifdef CONFIG_PM_SLEEP
> +		thelio_io->pm_notifier.notifier_call = thelio_io_pm;
> +		register_pm_notifier(&thelio_io->pm_notifier);
> +	#endif
> +	}

Ok, whatever your explanation for this is: I am not going to accept it,
period. It doesn't make sense to instantiate a driver that doesn't do
anything. If this would for whatever reason _really_ _really_ _really_
be necessary, I'd expect a lengthy comment describing why this is the
case and why it would be necessary to keep possibly multiple instances
of this driver around that don't do anything.

And, no, I am not going to dig through all your replies trying
to figure out your rationale for doing this. Note that, given that
there is not a single driver in the kernel doing anything similar,
you might have a hard time convincing me that this makes sense
and is necessary (both the check and successfully instantiating the
driver without registering a hwmon device).

> +
> +	return 0;
> +
> +out_hw_close:
> +	hid_hw_close(hdev);
> +out_hw_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void thelio_io_remove(struct hid_device *hdev)
> +{
> +	struct thelio_io_device *thelio_io = hid_get_drvdata(hdev);
> +
> +	if (thelio_io->hwmon_dev) {
> +		hwmon_device_unregister(thelio_io->hwmon_dev);
> +
> +	#ifdef CONFIG_PM_SLEEP
> +		unregister_pm_notifier(&thelio_io->pm_notifier);
> +	#endif
> +	}
> +
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id thelio_io_devices[] = {
> +	{ HID_USB_DEVICE(0x3384, 0x000B) }, /* thelio_io_2 */
> +	{ }
> +};
> +
> +static struct hid_driver thelio_io_driver = {
> +	.name = "system76-thelio-io",
> +	.id_table = thelio_io_devices,
> +	.probe = thelio_io_probe,
> +	.remove = thelio_io_remove,
> +	.raw_event = thelio_io_raw_event,
> +};
> +
> +MODULE_DEVICE_TABLE(hid, thelio_io_devices);
> +MODULE_LICENSE("GPL");
> +
> +static int __init thelio_io_init(void)
> +{
> +	return hid_register_driver(&thelio_io_driver);
> +}
> +
> +static void __exit thelio_io_exit(void)
> +{
> +	hid_unregister_driver(&thelio_io_driver);
> +}
> +
> +/*
> + * When compiling this driver as built-in, hwmon initcalls will get called before the
> + * hid driver and this driver would fail to register. late_initcall solves this.
> + */
> +late_initcall(thelio_io_init);
> +module_exit(thelio_io_exit);
> -- 
> 2.34.1



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux