From: Guenter Roeck <linux@roeck-us.net>
To: Marius Zachmann <mail@mariuszachmann.de>
Cc: Jean Delvare <jdelvare@suse.com>,
Jonathan Corbet <corbet@lwn.net>,
linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: (corsair-cpro) Add firmware and bootloader information
Date: Sun, 12 May 2024 17:10:06 -0700 [thread overview]
Message-ID: <85c0b346-8ab5-4cb1-ae62-d7737500eb36@roeck-us.net> (raw)
In-Reply-To: <20240512231251.72633-3-mail@mariuszachmann.de>
On 5/12/24 16:12, Marius Zachmann wrote:
> Sorry for resending. The last email had missing subsystem in the subject.
>
You actually sent it three times unless I lost count.
Anyway, the above comment should be after "---" because it
should not be part of the description.
> This patch adds:
> - Reading the firmware and bootloader version of the device
> - Debugfs entries: firmware_version and bootloader_version
> - Updated documentation
>
From Documentation/process/submitting-patches.rst:
Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.
With that in mind, something like
Add support for reporting firmware and bootloader version using debugfs.
Update documentation accordingly.
would be a more appropriate. After all, _reading_ the firmware and bootloader
version is implied in making the information available.
> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> ---
> Documentation/hwmon/corsair-cpro.rst | 8 +++
> drivers/hwmon/corsair-cpro.c | 80 ++++++++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
>
> diff --git a/Documentation/hwmon/corsair-cpro.rst b/Documentation/hwmon/corsair-cpro.rst
> index 751f95476b57..11135d7ec6b9 100644
> --- a/Documentation/hwmon/corsair-cpro.rst
> +++ b/Documentation/hwmon/corsair-cpro.rst
> @@ -39,3 +39,11 @@ fan[1-6]_target Sets fan speed target rpm.
> pwm[1-6] Sets the fan speed. Values from 0-255. Can only be read if pwm
> was set directly.
> ======================= =====================================================================
> +
> +Debugfs entries
> +---------------
> +
> +======================= ===========================
> +firmware_version Version of the firmware
Firmware version
> +bootloader_version Version of the bootloader
Bootloader version
> +======================= ===========================
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 3e63666a61bd..4be8a98250a9 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -10,11 +10,13 @@
>
> #include <linux/bitops.h>
> #include <linux/completion.h>
> +#include <linux/debugfs.h>
> #include <linux/hid.h>
> #include <linux/hwmon.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/seq_file.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/types.h>
> @@ -28,6 +30,8 @@
> #define LABEL_LENGTH 11
> #define REQ_TIMEOUT 300
>
> +#define CTL_GET_FW_VER 0x02 /* returns the firmware version in bytes 1-3 */
> +#define CTL_GET_BL_VER 0x06 /* returns the bootloader version in bytes 1-2 */
> #define CTL_GET_TMP_CNCT 0x10 /*
> * returns in bytes 1-4 for each temp sensor:
> * 0 not connected
> @@ -78,6 +82,7 @@
> struct ccp_device {
> struct hid_device *hdev;
> struct device *hwmon_dev;
> + struct dentry *debugfs;
> /* For reinitializing the completion below */
> spinlock_t wait_input_report_lock;
> struct completion wait_input_report;
> @@ -88,6 +93,8 @@ struct ccp_device {
> DECLARE_BITMAP(temp_cnct, NUM_TEMP_SENSORS);
> DECLARE_BITMAP(fan_cnct, NUM_FANS);
> char fan_label[6][LABEL_LENGTH];
> + u8 firmware_ver[3];
> + u8 bootloader_ver[2];
> };
>
> /* converts response error in buffer to errno */
> @@ -496,6 +503,71 @@ static int get_temp_cnct(struct ccp_device *ccp)
> return 0;
> }
>
> +/* read firmware and bootloader version */
> +static int get_fw_version(struct ccp_device *ccp)
> +{
> + int ret;
> +
> + ret = send_usb_cmd(ccp, CTL_GET_FW_VER, 0, 0, 0);
> + if (ret)
> + return ret;
> +
> + ccp->firmware_ver[0] = ccp->buffer[1];
> + ccp->firmware_ver[1] = ccp->buffer[2];
> + ccp->firmware_ver[2] = ccp->buffer[3];
> +
> + ret = send_usb_cmd(ccp, CTL_GET_BL_VER, 0, 0, 0);
> + if (ret)
> + return ret;
> +
> + ccp->bootloader_ver[0] = ccp->buffer[1];
> + ccp->bootloader_ver[1] = ccp->buffer[2];
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +
The conditional isn't needed. Yes, that may mean that the code will
be added even if CONFIG_DEBUG_FS is disabled, but that is 1) unlikely
and 2) better than not compiling the code at all and missing compile
failures.
> +static int firmware_show(struct seq_file *seqf, void *unused)
> +{
> + struct ccp_device *ccp = seqf->private;
> +
> + seq_printf(seqf, "%d.%d.%d\n",
> + ccp->firmware_ver[0],
> + ccp->firmware_ver[1],
> + ccp->firmware_ver[2]);
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(firmware);
> +
> +static int bootloader_show(struct seq_file *seqf, void *unused)
> +{
> + struct ccp_device *ccp = seqf->private;
> +
> + seq_printf(seqf, "%d.%d\n",
> + ccp->bootloader_ver[0],
> + ccp->bootloader_ver[1]);
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(bootloader);
> +
> +static void ccp_debugfs_init(struct ccp_device *ccp)
> +{
> + ccp->debugfs = debugfs_create_dir("corsaircpro", NULL);
> + debugfs_create_file("firmware_version", 0444, ccp->debugfs, ccp, &firmware_fops);
> + debugfs_create_file("bootloader_version", 0444, ccp->debugfs, ccp, &bootloader_fops);
> +}
> +
> +#else
> +
> +static void ccp_debugfs_init(struct ccp_device *ccp)
> +{
> +}
> +
> +#endif
> +
> static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> struct ccp_device *ccp;
> @@ -542,6 +614,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> ret = get_fan_cnct(ccp);
> if (ret)
> goto out_hw_close;
> +
> + ret = get_fw_version(ccp);
> + if (ret)
> + goto out_hw_close;
> +
Why bail out ? That is only informational, and if the information isn't available
the driver would still be operational. On top of that, if debugfs isn't enabled,
the information isn't even used. That means the probe function would bail out
for no good reason.
I'd suggest to move the call to get_fw_version() into ccp_debugfs_init()
and let it fail silently. I would not mind if you add a dev_notice() message
if the call fails, but anything else seems excessive.
Thanks,
Guenter
prev parent reply other threads:[~2024-05-13 0:10 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-12 23:12 [PATCH] hwmon: (corsair-cpro) Add firmware and bootloader information Marius Zachmann
2024-05-13 0:10 ` Guenter Roeck [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85c0b346-8ab5-4cb1-ae62-d7737500eb36@roeck-us.net \
--to=linux@roeck-us.net \
--cc=corbet@lwn.net \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mail@mariuszachmann.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).