* Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes
From: Dmitry Torokhov @ 2024-09-09 1:31 UTC (permalink / raw)
To: Jeff LaBundy
Cc: linux-input, Michael Hennerich, Ville Syrjala, Support Opensource,
Eddie James, Andrey Moiseev, Hans de Goede, Javier Carrasco,
linux-kernel
In-Reply-To: <Zt47Ic059YbOSbGy@nixie71>
Hi Jeff,
On Sun, Sep 08, 2024 at 07:02:41PM -0500, Jeff LaBundy wrote:
> Hi Dmitry,
>
> On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote:
> > Use __free(fwnode_handle) cleanup facility to ensure that references to
> > acquired fwnodes are dropped at appropriate times automatically.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/misc/iqs626a.c | 22 ++++++----------------
> > 1 file changed, 6 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..7a6e6927f331 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > {
> > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> > struct i2c_client *client = iqs626->client;
> > - struct fwnode_handle *ev_node;
> > const char *ev_name;
> > u8 *thresh, *hyst;
> > unsigned int val;
> > @@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > if (!iqs626_channels[ch_id].events[i])
> > continue;
> >
> > + struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;
>
> This seems to deviate from what I understand to be a more conventional
> style of declaring variables at the top of the scope, and separate from
> actual code, like below:
This is follows Linus' guidance on combining declaration and
initialization for variables using __free() cleanup annotations (where
possible). These annotations are the reason for dropping
-Wdeclaration-after-statement from our makefiles. See b5ec6fd286df
("kbuild: Drop -Wdeclaration-after-statement") and discussion in
https://lore.kernel.org/all/CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com
>
>
> for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) {
> struct fwnode_handle *ev_node __free(fwnode_handle);
>
> if (!iqs626_channels[ch_id].events[i])
> continue;
This will result in attempt to "put" a garbage pointer if we follow
"continue" code path. In general __free() pointers have to be
initialized, either to NULL or to a valid object (assuming that cleanup
function can deal with NULLs).
>
> I also did not see any reason to explicitly declare the variable as NULL;
> let me know in case I have misunderstood.
See the above. Yes, in this particular case it will get a value in both
branches, but I feel it is too fragile and may get messed up if someone
refactors code.
>
> > if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
> > /*
> > * Trackpad touch events are simply described under the
> > @@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > dev_err(&client->dev,
> > "Invalid input type: %u\n",
> > val);
> > - fwnode_handle_put(ev_node);
> > return -EINVAL;
> > }
> >
> > @@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > dev_err(&client->dev,
> > "Invalid %s channel hysteresis: %u\n",
> > fwnode_get_name(ch_node), val);
> > - fwnode_handle_put(ev_node);
> > return -EINVAL;
> > }
> >
> > @@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > dev_err(&client->dev,
> > "Invalid %s channel threshold: %u\n",
> > fwnode_get_name(ch_node), val);
> > - fwnode_handle_put(ev_node);
> > return -EINVAL;
> > }
> >
> > @@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > else
> > *(thresh + iqs626_events[i].th_offs) = val;
> > }
> > -
> > - fwnode_handle_put(ev_node);
> > }
> >
> > return 0;
> > @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
> > for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
> > u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
> > u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
> > - struct fwnode_handle *tc_node;
> > char tc_name[10];
> >
> > snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
> >
> > - tc_node = fwnode_get_named_child_node(ch_node, tc_name);
> > + struct fwnode_handle *tc_node __free(fwnode_handle) =
> > + fwnode_get_named_child_node(ch_node, tc_name);
>
> Same here.
Yes, combining declaration and initialization is preferred for such
pointers.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH -next v2 15/15] hwmon: (nzxt-smart2) Use devm_hid_hw_start_and_open in nzxt_smart2_hid_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the nzxt-smart2 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the out_hw_close and out_hw_stop
lables, and directly return the error code when an error occurs.
Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the drvdata no longer needs to hold hwmon device
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2:
1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
2) Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-20-lizetao1@huawei.com/
drivers/hwmon/nzxt-smart2.c | 38 +++++--------------------------------
1 file changed, 5 insertions(+), 33 deletions(-)
diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
index df6fa72a6b59..9c6d020ac896 100644
--- a/drivers/hwmon/nzxt-smart2.c
+++ b/drivers/hwmon/nzxt-smart2.c
@@ -172,7 +172,6 @@ struct set_fan_speed_report {
struct drvdata {
struct hid_device *hid;
- struct device *hwmon;
u8 fan_duty_percent[FAN_CHANNELS];
u16 fan_rpm[FAN_CHANNELS];
@@ -730,6 +729,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
struct drvdata *drvdata;
+ struct device *hwmon;
int ret;
drvdata = devm_kzalloc(&hdev->dev, sizeof(struct drvdata), GFP_KERNEL);
@@ -750,44 +750,17 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto out_hw_stop;
-
hid_device_io_start(hdev);
init_device(drvdata, UPDATE_INTERVAL_DEFAULT_MS);
- drvdata->hwmon =
- hwmon_device_register_with_info(&hdev->dev, "nzxtsmart2", drvdata,
- &nzxt_smart2_chip_info, NULL);
- if (IS_ERR(drvdata->hwmon)) {
- ret = PTR_ERR(drvdata->hwmon);
- goto out_hw_close;
- }
-
- return 0;
-
-out_hw_close:
- hid_hw_close(hdev);
-
-out_hw_stop:
- hid_hw_stop(hdev);
- return ret;
-}
-
-static void nzxt_smart2_hid_remove(struct hid_device *hdev)
-{
- struct drvdata *drvdata = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(drvdata->hwmon);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ hwmon = devm_hwmon_device_register_with_info(&hdev->dev, "nzxtsmart2", drvdata,
+ &nzxt_smart2_chip_info, NULL);
+ return PTR_ERR_OR_ZERO(hwmon);
}
static const struct hid_device_id nzxt_smart2_hid_id_table[] = {
@@ -807,7 +780,6 @@ static struct hid_driver nzxt_smart2_hid_driver = {
.name = "nzxt-smart2",
.id_table = nzxt_smart2_hid_id_table,
.probe = nzxt_smart2_hid_probe,
- .remove = nzxt_smart2_hid_remove,
.raw_event = nzxt_smart2_hid_raw_event,
#ifdef CONFIG_PM
.reset_resume = nzxt_smart2_hid_reset_resume,
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 08/15] hwmon: (aquacomputer_d5next) Use devm_hid_hw_start_and_open in aqc_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the aquacomputer_d5next module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-13-lizetao1@huawei.com/
drivers/hwmon/aquacomputer_d5next.c | 39 +++++++----------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 8e55cd2f46f5..9b66ff0fe6e1 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -1556,14 +1556,10 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto fail_and_stop;
-
switch (hdev->product) {
case USB_PRODUCT_ID_AQUAERO:
/*
@@ -1577,10 +1573,8 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
* they present. The two other devices have the type of the second element in
* their respective collections set to 1, while the real device has it set to 0.
*/
- if (hdev->collection[1].type != 0) {
- ret = -ENODEV;
- goto fail_and_close;
- }
+ if (hdev->collection[1].type != 0)
+ return -ENODEV;
priv->kind = aquaero;
@@ -1740,10 +1734,8 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
* Choose the right Leakshield device, because
* the other one acts as a keyboard
*/
- if (hdev->type != 2) {
- ret = -ENODEV;
- goto fail_and_close;
- }
+ if (hdev->type != 2)
+ return -ENODEV;
priv->kind = leakshield;
@@ -1865,30 +1857,20 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
priv->name = aqc_device_names[priv->kind];
priv->buffer = devm_kzalloc(&hdev->dev, priv->buffer_size, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->mutex);
priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, priv->name, priv,
&aqc_chip_info, NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- goto fail_and_close;
- }
+ if (IS_ERR(priv->hwmon_dev))
+ return PTR_ERR(priv->hwmon_dev);
aqc_debugfs_init(priv);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void aqc_remove(struct hid_device *hdev)
@@ -1897,9 +1879,6 @@ static void aqc_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id aqc_table[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 09/15] hwmon: Use devm_hid_hw_start_and_open in rog_ryujin_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the rog_ryujin module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the rog_ryujin_data structure no longer needs to hold
hwmon device.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2:
1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
2) Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-14-lizetao1@huawei.com/
drivers/hwmon/asus_rog_ryujin.c | 47 +++++----------------------------
1 file changed, 7 insertions(+), 40 deletions(-)
diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
index f8b20346a995..ef0d9b72a824 100644
--- a/drivers/hwmon/asus_rog_ryujin.c
+++ b/drivers/hwmon/asus_rog_ryujin.c
@@ -80,7 +80,6 @@ static const char *const rog_ryujin_speed_label[] = {
struct rog_ryujin_data {
struct hid_device *hdev;
- struct device *hwmon_dev;
/* For locking access to buffer */
struct mutex buffer_lock;
/* For queueing multiple readers */
@@ -497,6 +496,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct rog_ryujin_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -520,23 +520,13 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
}
/* Enable hidraw so existing user-space tools can continue to work */
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->status_report_request_mutex);
mutex_init(&priv->buffer_lock);
@@ -548,31 +538,9 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
init_completion(&priv->cooler_duty_set);
init_completion(&priv->controller_duty_set);
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
+ hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
priv, &rog_ryujin_chip_info, NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
- }
-
- return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
-}
-
-static void rog_ryujin_remove(struct hid_device *hdev)
-{
- struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
}
static const struct hid_device_id rog_ryujin_table[] = {
@@ -586,7 +554,6 @@ static struct hid_driver rog_ryujin_driver = {
.name = "rog_ryujin",
.id_table = rog_ryujin_table,
.probe = rog_ryujin_probe,
- .remove = rog_ryujin_remove,
.raw_event = rog_ryujin_raw_event,
};
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 10/15] hwmon: (corsair-cpro) Use devm_hid_hw_start_and_open in ccp_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the corsair-cpro module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the out_hw_close and hid_hw_stop
lables, and directly return the error code when an error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-15-lizetao1@huawei.com/
drivers/hwmon/corsair-cpro.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index e1a7f7aa7f80..7bba30840f32 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -601,14 +601,10 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto out_hw_stop;
-
ccp->hdev = hdev;
hid_set_drvdata(hdev, ccp);
@@ -621,28 +617,20 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
/* temp and fan connection status only updates when device is powered on */
ret = get_temp_cnct(ccp);
if (ret)
- goto out_hw_close;
+ return ret;
ret = get_fan_cnct(ccp);
if (ret)
- goto out_hw_close;
+ return ret;
ccp_debugfs_init(ccp);
ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
ccp, &ccp_chip_info, NULL);
- if (IS_ERR(ccp->hwmon_dev)) {
- ret = PTR_ERR(ccp->hwmon_dev);
- goto out_hw_close;
- }
+ if (IS_ERR(ccp->hwmon_dev))
+ return PTR_ERR(ccp->hwmon_dev);
return 0;
-
-out_hw_close:
- hid_hw_close(hdev);
-out_hw_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void ccp_remove(struct hid_device *hdev)
@@ -651,8 +639,6 @@ static void ccp_remove(struct hid_device *hdev)
debugfs_remove_recursive(ccp->debugfs);
hwmon_device_unregister(ccp->hwmon_dev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id ccp_devices[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 07/15] HID: playstation: Use devm_hid_hw_start_and_open in ps_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the playstation module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the err_close and err_stop lables, and
directly return the error code when an error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-10-lizetao1@huawei.com/
drivers/hid/hid-playstation.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 0d90d7ee693c..6dddb9451a37 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2704,41 +2704,25 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "Failed to start HID device\n");
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "Failed to open HID device\n");
- goto err_stop;
- }
if (id->driver_data == PS_TYPE_PS4_DUALSHOCK4) {
dev = dualshock4_create(hdev);
if (IS_ERR(dev)) {
hid_err(hdev, "Failed to create dualshock4.\n");
- ret = PTR_ERR(dev);
- goto err_close;
+ return PTR_ERR(dev);
}
} else if (id->driver_data == PS_TYPE_PS5_DUALSENSE) {
dev = dualsense_create(hdev);
if (IS_ERR(dev)) {
hid_err(hdev, "Failed to create dualsense.\n");
- ret = PTR_ERR(dev);
- goto err_close;
+ return PTR_ERR(dev);
}
}
return ret;
-
-err_close:
- hid_hw_close(hdev);
-err_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void ps_remove(struct hid_device *hdev)
@@ -2750,9 +2734,6 @@ static void ps_remove(struct hid_device *hdev)
if (dev->remove)
dev->remove(dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id ps_devices[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 05/15] HID: mcp2221: Use devm_hid_hw_start_and_open in mcp2221_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the mcp2221 module use devm_add_action_or_reset() to manage
device resource for HID unregistration, now that a universal interface
has been provided, use a universal interface to replace it.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: None
v1:
https://lore.kernel.org/all/20240904123607.3407364-6-lizetao1@huawei.com/
drivers/hid/hid-mcp2221.c | 26 ++------------------------
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 0f93c22a479f..3b8269f7e923 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -932,15 +932,6 @@ static int mcp2221_raw_event(struct hid_device *hdev,
return 1;
}
-/* Device resource managed function for HID unregistration */
-static void mcp2221_hid_unregister(void *ptr)
-{
- struct hid_device *hdev = ptr;
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
-}
-
/* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
static void mcp2221_remove(struct hid_device *hdev)
{
@@ -1141,31 +1132,18 @@ static int mcp2221_probe(struct hid_device *hdev,
* This driver uses the .raw_event callback and therefore does not need any
* HID_CONNECT_xxx flags.
*/
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "can't start hardware\n");
+ ret = devm_hid_hw_start_and_open(hdev, 0);
+ if (ret)
return ret;
- }
hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
hdev->version & 0xff, hdev->name, hdev->phys);
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "can't open device\n");
- hid_hw_stop(hdev);
- return ret;
- }
-
mutex_init(&mcp->lock);
init_completion(&mcp->wait_in_report);
hid_set_drvdata(hdev, mcp);
mcp->hdev = hdev;
- ret = devm_add_action_or_reset(&hdev->dev, mcp2221_hid_unregister, hdev);
- if (ret)
- return ret;
-
hid_device_io_start(hdev);
/* Set I2C bus clock diviser */
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 06/15] HID: nintendo: Use devm_hid_hw_start_and_open in nintendo_hid_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the nintendo module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the err_close and err_stop lables.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-7-lizetao1@huawei.com/
drivers/hid/hid-nintendo.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 58cd0506e431..45ac4fd3c7ea 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2673,31 +2673,23 @@ static int nintendo_hid_probe(struct hid_device *hdev,
*/
hdev->version |= 0x8000;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "HW start failed\n");
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
goto err_wq;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "cannot start hardware I/O\n");
- goto err_stop;
- }
hid_device_io_start(hdev);
ret = joycon_init(hdev);
if (ret) {
hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
- goto err_close;
+ goto err_wq;
}
/* Initialize the leds */
ret = joycon_leds_create(ctlr);
if (ret) {
hid_err(hdev, "Failed to create leds; ret=%d\n", ret);
- goto err_close;
+ goto err_wq;
}
/* Initialize the battery power supply */
@@ -2720,10 +2712,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
err_ida:
ida_free(&nintendo_player_id_allocator, ctlr->player_id);
-err_close:
- hid_hw_close(hdev);
-err_stop:
- hid_hw_stop(hdev);
err_wq:
destroy_workqueue(ctlr->rumble_queue);
err:
@@ -2745,9 +2733,6 @@ static void nintendo_hid_remove(struct hid_device *hdev)
destroy_workqueue(ctlr->rumble_queue);
ida_free(&nintendo_player_id_allocator, ctlr->player_id);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
#ifdef CONFIG_PM
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 14/15] hwmon: (nzxt-kraken3) Use devm_hid_hw_start_and_open in kraken3_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the nzxt-kraken2 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-19-lizetao1@huawei.com/
drivers/hwmon/nzxt-kraken3.c | 34 +++++++---------------------------
1 file changed, 7 insertions(+), 27 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
index 00f3ac90a290..71b8c21cfe1b 100644
--- a/drivers/hwmon/nzxt-kraken3.c
+++ b/drivers/hwmon/nzxt-kraken3.c
@@ -897,17 +897,9 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
}
/* Enable hidraw so existing user-space tools can continue to work */
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
switch (hdev->product) {
case USB_PRODUCT_ID_X53:
@@ -928,15 +920,12 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
device_name = "kraken2023elite";
break;
default:
- ret = -ENODEV;
- goto fail_and_close;
+ return -ENODEV;
}
priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->buffer_lock);
mutex_init(&priv->z53_status_request_lock);
@@ -948,7 +937,7 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
ret = kraken3_init_device(hdev);
if (ret < 0) {
hid_err(hdev, "device init failed with %d\n", ret);
- goto fail_and_close;
+ return ret;
}
ret = kraken3_get_fw_ver(hdev);
@@ -960,18 +949,12 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
if (IS_ERR(priv->hwmon_dev)) {
ret = PTR_ERR(priv->hwmon_dev);
hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
+ return ret;
}
kraken3_debugfs_init(priv, device_name);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void kraken3_remove(struct hid_device *hdev)
@@ -980,9 +963,6 @@ static void kraken3_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id kraken3_table[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 04/15] HID: mcp2200: Use devm_hid_hw_start_and_open in mcp2200_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the mcp2200 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. So there is no need to close and
stop hid when an error occurs. At the same time, since there is no need to
do any operations in mcp2200_remove() now, so delete .remote operation.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-5-lizetao1@huawei.com/
drivers/hid/hid-mcp2200.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
index bf57f7f6caa0..56d72fc5623d 100644
--- a/drivers/hid/hid-mcp2200.c
+++ b/drivers/hid/hid-mcp2200.c
@@ -329,22 +329,13 @@ static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id
return ret;
}
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "can't start hardware\n");
+ ret = devm_hid_hw_start_and_open(hdev, 0);
+ if (ret)
return ret;
- }
hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
hdev->version & 0xff, hdev->name, hdev->phys);
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "can't open device\n");
- hid_hw_stop(hdev);
- return ret;
- }
-
mutex_init(&mcp->lock);
init_completion(&mcp->wait_in_report);
hid_set_drvdata(hdev, mcp);
@@ -356,20 +347,12 @@ static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id
ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
if (ret < 0) {
hid_err(hdev, "Unable to register gpiochip\n");
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
return ret;
}
return 0;
}
-static void mcp2200_remove(struct hid_device *hdev)
-{
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
-}
-
static const struct hid_device_id mcp2200_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
{ }
@@ -380,7 +363,6 @@ static struct hid_driver mcp2200_driver = {
.name = "mcp2200",
.id_table = mcp2200_devices,
.probe = mcp2200_probe,
- .remove = mcp2200_remove,
.raw_event = mcp2200_raw_event,
};
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 13/15] hwmon: (nzxt-kraken2) Use devm_hid_hw_start_and_open in kraken2_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the nzxt-kraken2 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the kraken2_priv_data no longer needs to hold hwmon device.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2:
1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
2) Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-18-lizetao1@huawei.com/
drivers/hwmon/nzxt-kraken2.c | 45 ++++++------------------------------
1 file changed, 7 insertions(+), 38 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
index 7caf387eb144..5b618fc2c17f 100644
--- a/drivers/hwmon/nzxt-kraken2.c
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -29,7 +29,6 @@ static const char *const kraken2_fan_label[] = {
struct kraken2_priv_data {
struct hid_device *hid_dev;
- struct device *hwmon_dev;
s32 temp_input[1];
u16 fan_input[2];
unsigned long updated; /* jiffies */
@@ -133,6 +132,7 @@ static int kraken2_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
struct kraken2_priv_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -158,44 +158,14 @@ static int kraken2_probe(struct hid_device *hdev,
/*
* Enable hidraw so existing user-space tools can continue to work.
*/
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
-
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
- priv, &kraken2_chip_info,
- NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
- }
-
- return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
-}
-
-static void kraken2_remove(struct hid_device *hdev)
-{
- struct kraken2_priv_data *priv = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(priv->hwmon_dev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "kraken2",
+ priv, &kraken2_chip_info,
+ NULL);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
}
static const struct hid_device_id kraken2_table[] = {
@@ -209,7 +179,6 @@ static struct hid_driver kraken2_driver = {
.name = "nzxt-kraken2",
.id_table = kraken2_table,
.probe = kraken2_probe,
- .remove = kraken2_remove,
.raw_event = kraken2_raw_event,
};
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 11/15] hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in corsairpsu_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the corsair-psu module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Reviewed-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-15-lizetao1@huawei.com/
drivers/hwmon/corsair-psu.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index f8f22b8a67cd..b574ec9cd00f 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -787,14 +787,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto fail_and_stop;
-
priv->hdev = hdev;
hid_set_drvdata(hdev, priv);
mutex_init(&priv->lock);
@@ -805,13 +801,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
ret = corsairpsu_init(priv);
if (ret < 0) {
dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
- goto fail_and_stop;
+ return ret;
}
ret = corsairpsu_fwinfo(priv);
if (ret < 0) {
dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
- goto fail_and_stop;
+ return ret;
}
corsairpsu_get_criticals(priv);
@@ -820,20 +816,12 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
&corsairpsu_chip_info, NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- goto fail_and_close;
- }
+ if (IS_ERR(priv->hwmon_dev))
+ return PTR_ERR(priv->hwmon_dev);
corsairpsu_debugfs_init(priv);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void corsairpsu_remove(struct hid_device *hdev)
@@ -842,8 +830,6 @@ static void corsairpsu_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data,
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 03/15] HID: ft260: Use devm_hid_hw_start_and_open in ft260_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the ft260 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the err_hid_close, err_hid_stop
and err_i2c_free lables, and directly return the error code when an
error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-4-lizetao1@huawei.com/
drivers/hid/hid-ft260.c | 32 +++++++-------------------------
1 file changed, 7 insertions(+), 25 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 333341e80b0e..272165ebf46c 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -976,23 +976,15 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "failed to start HID HW\n");
+ ret = devm_hid_hw_start_and_open(hdev, 0);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "failed to open HID HW\n");
- goto err_hid_stop;
- }
ret = ft260_hid_feature_report_get(hdev, FT260_CHIP_VERSION,
(u8 *)&version, sizeof(version));
if (ret < 0) {
hid_err(hdev, "failed to retrieve chip version\n");
- goto err_hid_close;
+ return ret;
}
hid_info(hdev, "chip code: %02x%02x %02x%02x\n",
@@ -1001,7 +993,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
ret = ft260_is_interface_enabled(hdev);
if (ret <= 0)
- goto err_hid_close;
+ return ret;
hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n",
hdev->version >> 8, hdev->version & 0xff, hdev->name,
@@ -1028,24 +1020,17 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
ret = i2c_add_adapter(&dev->adap);
if (ret) {
hid_err(hdev, "failed to add i2c adapter\n");
- goto err_hid_close;
+ return ret;
}
ret = sysfs_create_group(&hdev->dev.kobj, &ft260_attr_group);
if (ret < 0) {
hid_err(hdev, "failed to create sysfs attrs\n");
- goto err_i2c_free;
+ i2c_del_adapter(&dev->adap);
+ return ret;
}
return 0;
-
-err_i2c_free:
- i2c_del_adapter(&dev->adap);
-err_hid_close:
- hid_hw_close(hdev);
-err_hid_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void ft260_remove(struct hid_device *hdev)
@@ -1057,9 +1042,6 @@ static void ft260_remove(struct hid_device *hdev)
sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
i2c_del_adapter(&dev->adap);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 12/15] hwmon: (gigabyte_waterforce) Use devm_hid_hw_start_and_open in waterforce_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the waterforce module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-16-lizetao1@huawei.com/
drivers/hwmon/gigabyte_waterforce.c | 29 +++++------------------------
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index 8129d7b3ceaf..9052d1c3d5aa 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -337,23 +337,13 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
/*
* Enable hidraw so existing user-space tools can continue to work.
*/
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->status_report_request_mutex);
mutex_init(&priv->buffer_lock);
@@ -371,18 +361,12 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
if (IS_ERR(priv->hwmon_dev)) {
ret = PTR_ERR(priv->hwmon_dev);
hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
+ return ret;
}
waterforce_debugfs_init(priv);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void waterforce_remove(struct hid_device *hdev)
@@ -391,9 +375,6 @@ static void waterforce_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id waterforce_table[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 00/15] HID: convert to devm_hid_hw_start_and_open()
From: Li Zetao @ 2024-09-09 1:22 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
v1 -> v2:
1) drop some risky patches, such as patch 7, which may have race issues
2) Some patches can be further optimized. By replacing
hwmon_device_register_with_info with devm_hwmon_device_register_with_info,
the .remove operation can be completely deleted.
3) Adjust some commit information and use "Use" to replace
"Consider using"
v1:
https://lore.kernel.org/all/20240904123607.3407364-1-lizetao1@huawei.com/
Hi, all
This patchset is dedicated to using the life cycle approach to manage
hid resources. By keeping hid resources consistent with the life cycle
of the device, we ensure that resources are available during the life
cycle and the hid resources can be released before device release.
Going one step further, since the module does not need to recycle hid
resources by itself, the goto-release resource release coding can be
avoided. It also reduces the risk of resources not being released.
Thanks,
Li Zetao.
Li Zetao (15):
HID: core: Use devm_add_action_or_reset helper to manage hid resources
HID: cp2112: Use devm_hid_hw_start_and_open in cp2112_probe()
HID: ft260: Use devm_hid_hw_start_and_open in ft260_probe()
HID: mcp2200: Use devm_hid_hw_start_and_open in mcp2200_probe()
HID: mcp2221: Use devm_hid_hw_start_and_open in mcp2221_probe()
HID: nintendo: Use devm_hid_hw_start_and_open in nintendo_hid_probe()
HID: playstation: Use devm_hid_hw_start_and_open in ps_probe()
hwmon: (aquacomputer_d5next) Use devm_hid_hw_start_and_open in
aqc_probe()
hwmon: Use devm_hid_hw_start_and_open in rog_ryujin_probe()
hwmon: (corsair-cpro) Use devm_hid_hw_start_and_open in ccp_probe()
hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in
corsairpsu_probe()
hwmon: (gigabyte_waterforce) Use devm_hid_hw_start_and_open in
waterforce_probe()
hwmon: (nzxt-kraken2) Use devm_hid_hw_start_and_open in
kraken2_probe()
hwmon: (nzxt-kraken3) Use devm_hid_hw_start_and_open in
kraken3_probe()
hwmon: (nzxt-smart2) Use devm_hid_hw_start_and_open in
nzxt_smart2_hid_probe()
drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++
drivers/hid/hid-cp2112.c | 26 ++--------------
drivers/hid/hid-ft260.c | 32 +++++---------------
drivers/hid/hid-mcp2200.c | 22 ++------------
drivers/hid/hid-mcp2221.c | 26 ++--------------
drivers/hid/hid-nintendo.c | 23 +++-----------
drivers/hid/hid-playstation.c | 27 +++--------------
drivers/hwmon/aquacomputer_d5next.c | 39 ++++++------------------
drivers/hwmon/asus_rog_ryujin.c | 47 +++++------------------------
drivers/hwmon/corsair-cpro.c | 24 +++------------
drivers/hwmon/corsair-psu.c | 24 +++------------
drivers/hwmon/gigabyte_waterforce.c | 29 +++---------------
drivers/hwmon/nzxt-kraken2.c | 45 +++++----------------------
drivers/hwmon/nzxt-kraken3.c | 34 +++++----------------
drivers/hwmon/nzxt-smart2.c | 38 +++--------------------
include/linux/hid.h | 2 ++
16 files changed, 114 insertions(+), 364 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH -next v2 01/15] HID: core: Use devm_add_action_or_reset helper to manage hid resources
From: Li Zetao @ 2024-09-09 1:22 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
By adding a custom action to the device, it can bind the hid resource
to the hid_device life cycle. The framework automatically close and stop
the hid resources before hid_device is released, and the users do not
need to pay attention to the timing of hid resource release.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Add function usage constraints in comments
v1:
https://lore.kernel.org/all/cyils23bh4xaiw7bydlpapz4ngqpya3c4kesifrdpnme2t4bib@6elk7u3wvhh2/
drivers/hid/hid-core.c | 44 ++++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 2 ++
2 files changed, 46 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 30de92d0bf0f..132c81639753 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2416,6 +2416,50 @@ void hid_hw_close(struct hid_device *hdev)
}
EXPORT_SYMBOL_GPL(hid_hw_close);
+static void hid_hw_close_and_stop(void *data)
+{
+ struct hid_device *hdev = data;
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+/**
+ * devm_hid_hw_start_and_open - manage hid resources through custom action
+ *
+ * @hdev: hid device
+ * @connect_mask: which outputs to connect, see HID_CONNECT_*
+ *
+ * Bind the hid resource to the hid_device life cycle and register
+ * an action to release the hid resource. The users do not need to
+ * pay attention to the release of hid.
+ *
+ * Some usage constraints of this function: hid_device also needs to be
+ * allocated through the Devres API, such as devm_kzalloc; hid_hw_stop should
+ * be followed immediately by hid_hw_close in the remove operation.
+ */
+
+int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
+{
+ int ret;
+
+ ret = hid_hw_start(hdev, connect_mask);
+ if (ret) {
+ hid_err(hdev, "hw start failed with %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "hw open failed with %d\n", ret);
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
+}
+EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
+
/**
* hid_hw_request - send report request to device
*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 121d5b8bc867..0ce217aa5f62 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
void hid_hw_stop(struct hid_device *hdev);
int __must_check hid_hw_open(struct hid_device *hdev);
void hid_hw_close(struct hid_device *hdev);
+int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
+ unsigned int connect_mask);
void hid_hw_request(struct hid_device *hdev,
struct hid_report *report, enum hid_class_request reqtype);
int __hid_hw_raw_request(struct hid_device *hdev,
--
2.34.1
^ permalink raw reply related
* [PATCH -next v2 02/15] HID: cp2112: Use devm_hid_hw_start_and_open in cp2112_probe()
From: Li Zetao @ 2024-09-09 1:23 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, gupt21, djogorchock,
roderick.colenbrander, savicaleksa83, me, jdelvare, linux, mail,
wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240909012313.500341-1-lizetao1@huawei.com>
Currently, the cp2112 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the err_hid_close and err_hid_stop
lables.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-3-lizetao1@huawei.com/
drivers/hid/hid-cp2112.c | 26 +++-----------------------
1 file changed, 3 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 20a0d1315d90..6d65c65f1b83 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1215,22 +1215,14 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hw start failed\n");
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hw open failed\n");
- goto err_hid_stop;
- }
ret = hid_hw_power(hdev, PM_HINT_FULLON);
if (ret < 0) {
hid_err(hdev, "power management error: %d\n", ret);
- goto err_hid_close;
+ return ret;
}
ret = cp2112_hid_get(hdev, CP2112_GET_VERSION_INFO, buf, sizeof(buf),
@@ -1334,10 +1326,6 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
i2c_del_adapter(&dev->adap);
err_power_normal:
hid_hw_power(hdev, PM_HINT_NORMAL);
-err_hid_close:
- hid_hw_close(hdev);
-err_hid_stop:
- hid_hw_stop(hdev);
return ret;
}
@@ -1354,14 +1342,6 @@ static void cp2112_remove(struct hid_device *hdev)
}
gpiochip_remove(&dev->gc);
- /* i2c_del_adapter has finished removing all i2c devices from our
- * adapter. Well behaved devices should no longer call our cp2112_xfer
- * and should have waited for any pending calls to finish. It has also
- * waited for device_unregister(&adap->dev) to complete. Therefore we
- * can safely free our struct cp2112_device.
- */
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes
From: Jeff LaBundy @ 2024-09-09 0:12 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Michael Hennerich, Ville Syrjala, Support Opensource,
Eddie James, Andrey Moiseev, Hans de Goede, Javier Carrasco,
linux-kernel
In-Reply-To: <20240904044825.1048256-1-dmitry.torokhov@gmail.com>
Hi Dmitry,
On Tue, Sep 03, 2024 at 09:48:25PM -0700, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> index 9ca5a743f19f..d9b87606ff7a 100644
> --- a/drivers/input/misc/iqs7222.c
> +++ b/drivers/input/misc/iqs7222.c
> @@ -2385,9 +2385,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> for (i = 0; i < ARRAY_SIZE(iqs7222_kp_events); i++) {
> const char *event_name = iqs7222_kp_events[i].name;
> u16 event_enable = iqs7222_kp_events[i].enable;
> - struct fwnode_handle *event_node;
>
> - event_node = fwnode_get_named_child_node(chan_node, event_name);
> + struct fwnode_handle *event_node __free(fwnode_handle) =
> + fwnode_get_named_child_node(chan_node, event_name);
This leaves a newline amongst the declarations, but not in between the
declarations and code. I don't feel strongly against this, but it's
opposite of what I understand to be more common; please let me know in
case I have misunderstood. This comment applies to the other chunks as
well.
> if (!event_node)
> continue;
>
> @@ -2408,7 +2408,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> dev_err(&client->dev,
> "Invalid %s press timeout: %u\n",
> fwnode_get_name(event_node), val);
> - fwnode_handle_put(event_node);
> return -EINVAL;
> }
>
> @@ -2418,7 +2417,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> dev_err(&client->dev,
> "Failed to read %s press timeout: %d\n",
> fwnode_get_name(event_node), error);
> - fwnode_handle_put(event_node);
> return error;
> }
>
> @@ -2429,7 +2427,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> dev_desc->touch_link - (i ? 0 : 2),
> &iqs7222->kp_type[chan_index][i],
> &iqs7222->kp_code[chan_index][i]);
> - fwnode_handle_put(event_node);
> if (error)
> return error;
>
> @@ -2604,10 +2601,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>
> for (i = 0; i < ARRAY_SIZE(iqs7222_sl_events); i++) {
> const char *event_name = iqs7222_sl_events[i].name;
> - struct fwnode_handle *event_node;
> enum iqs7222_reg_key_id reg_key;
>
> - event_node = fwnode_get_named_child_node(sldr_node, event_name);
> + struct fwnode_handle *event_node __free(fwnode_handle) =
> + fwnode_get_named_child_node(sldr_node, event_name);
> if (!event_node)
> continue;
>
> @@ -2639,7 +2636,6 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
> : sldr_setup[4 + reg_offset],
> NULL,
> &iqs7222->sl_code[sldr_index][i]);
> - fwnode_handle_put(event_node);
> if (error)
> return error;
>
> @@ -2742,9 +2738,9 @@ static int iqs7222_parse_tpad(struct iqs7222_private *iqs7222,
>
> for (i = 0; i < ARRAY_SIZE(iqs7222_tp_events); i++) {
> const char *event_name = iqs7222_tp_events[i].name;
> - struct fwnode_handle *event_node;
>
> - event_node = fwnode_get_named_child_node(tpad_node, event_name);
> + struct fwnode_handle *event_node __free(fwnode_handle) =
> + fwnode_get_named_child_node(tpad_node, event_name);
> if (!event_node)
> continue;
>
> @@ -2760,7 +2756,6 @@ static int iqs7222_parse_tpad(struct iqs7222_private *iqs7222,
> iqs7222_tp_events[i].link, 1566,
> NULL,
> &iqs7222->tp_code[i]);
> - fwnode_handle_put(event_node);
> if (error)
> return error;
>
> @@ -2818,9 +2813,9 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
> int reg_grp_index)
> {
> struct i2c_client *client = iqs7222->client;
> - struct fwnode_handle *reg_grp_node;
> int error;
>
> + struct fwnode_handle *reg_grp_node __free(fwnode_handle) = NULL;
> if (iqs7222_reg_grp_names[reg_grp]) {
> char reg_grp_name[16];
>
> @@ -2838,14 +2833,17 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
>
> error = iqs7222_parse_props(iqs7222, reg_grp_node, reg_grp_index,
> reg_grp, IQS7222_REG_KEY_NONE);
> + if (error)
> + return error;
>
> - if (!error && iqs7222_parse_extra[reg_grp])
> + if (iqs7222_parse_extra[reg_grp]) {
> error = iqs7222_parse_extra[reg_grp](iqs7222, reg_grp_node,
> reg_grp_index);
> + if (error)
> + return error;
> + }
>
> - fwnode_handle_put(reg_grp_node);
> -
> - return error;
> + return 0;
> }
>
> static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
> --
> 2.46.0.469.g59c65b2a67-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes
From: Jeff LaBundy @ 2024-09-09 0:02 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Michael Hennerich, Ville Syrjala, Support Opensource,
Eddie James, Andrey Moiseev, Hans de Goede, Javier Carrasco,
linux-kernel
In-Reply-To: <20240904044814.1048062-1-dmitry.torokhov@gmail.com>
Hi Dmitry,
On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/misc/iqs626a.c | 22 ++++++----------------
> 1 file changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..7a6e6927f331 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> {
> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> struct i2c_client *client = iqs626->client;
> - struct fwnode_handle *ev_node;
> const char *ev_name;
> u8 *thresh, *hyst;
> unsigned int val;
> @@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> if (!iqs626_channels[ch_id].events[i])
> continue;
>
> + struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;
This seems to deviate from what I understand to be a more conventional
style of declaring variables at the top of the scope, and separate from
actual code, like below:
for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) {
struct fwnode_handle *ev_node __free(fwnode_handle);
if (!iqs626_channels[ch_id].events[i])
continue;
I also did not see any reason to explicitly declare the variable as NULL;
let me know in case I have misunderstood.
> if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
> /*
> * Trackpad touch events are simply described under the
> @@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid input type: %u\n",
> val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s channel hysteresis: %u\n",
> fwnode_get_name(ch_node), val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s channel threshold: %u\n",
> fwnode_get_name(ch_node), val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> else
> *(thresh + iqs626_events[i].th_offs) = val;
> }
> -
> - fwnode_handle_put(ev_node);
> }
>
> return 0;
> @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
> for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
> u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
> u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
> - struct fwnode_handle *tc_node;
> char tc_name[10];
>
> snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
>
> - tc_node = fwnode_get_named_child_node(ch_node, tc_name);
> + struct fwnode_handle *tc_node __free(fwnode_handle) =
> + fwnode_get_named_child_node(ch_node, tc_name);
Same here.
> if (!tc_node)
> continue;
>
> @@ -790,7 +785,6 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s %s ATI base: %u\n",
> fwnode_get_name(ch_node), tc_name, val);
> - fwnode_handle_put(tc_node);
> return -EINVAL;
> }
>
> @@ -803,14 +797,11 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s %s threshold: %u\n",
> fwnode_get_name(ch_node), tc_name, val);
> - fwnode_handle_put(tc_node);
> return -EINVAL;
> }
>
> *thresh = val;
> }
> -
> - fwnode_handle_put(tc_node);
> }
>
> if (!fwnode_property_present(ch_node, "linux,keycodes"))
> @@ -1233,7 +1224,6 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
> {
> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> struct i2c_client *client = iqs626->client;
> - struct fwnode_handle *ch_node;
> unsigned int val;
> int error, i;
> u16 general;
> @@ -1375,13 +1365,13 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
> sys_reg->active = 0;
>
> for (i = 0; i < ARRAY_SIZE(iqs626_channels); i++) {
> - ch_node = device_get_named_child_node(&client->dev,
> - iqs626_channels[i].name);
> + struct fwnode_handle *ch_node __free(fwnode_handle) =
> + device_get_named_child_node(&client->dev,
> + iqs626_channels[i].name);
> if (!ch_node)
> continue;
>
> error = iqs626_parse_channel(iqs626, ch_node, i);
> - fwnode_handle_put(ch_node);
> if (error)
> return error;
>
> --
> 2.46.0.469.g59c65b2a67-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 13/22] Input: iqs269a - use cleanup facility for fwnodes
From: Jeff LaBundy @ 2024-09-08 22:08 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Michael Hennerich, Ville Syrjala, Support Opensource,
Eddie James, Andrey Moiseev, Hans de Goede, Javier Carrasco,
linux-kernel
In-Reply-To: <20240904044806.1047847-1-dmitry.torokhov@gmail.com>
Hi Dmitry,
On Tue, Sep 03, 2024 at 09:48:05PM -0700, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/misc/iqs269a.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index c34d847fa4af..1851848e2cd3 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -550,7 +550,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> const struct fwnode_handle *ch_node)
> {
> struct i2c_client *client = iqs269->client;
> - struct fwnode_handle *ev_node;
> struct iqs269_ch_reg *ch_reg;
> u16 engine_a, engine_b;
> unsigned int reg, val;
> @@ -727,8 +726,9 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> }
>
> for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
> - ev_node = fwnode_get_named_child_node(ch_node,
> - iqs269_events[i].name);
> + struct fwnode_handle *ev_node __free(fwnode_handle) =
> + fwnode_get_named_child_node(ch_node,
> + iqs269_events[i].name);
> if (!ev_node)
> continue;
>
> @@ -737,7 +737,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> dev_err(&client->dev,
> "Invalid channel %u threshold: %u\n",
> reg, val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -751,7 +750,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> dev_err(&client->dev,
> "Invalid channel %u hysteresis: %u\n",
> reg, val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -767,7 +765,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> }
>
> error = fwnode_property_read_u32(ev_node, "linux,code", &val);
> - fwnode_handle_put(ev_node);
> if (error == -EINVAL) {
> continue;
> } else if (error) {
> --
> 2.46.0.469.g59c65b2a67-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex
From: Jeff LaBundy @ 2024-09-08 22:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Michael Hennerich, Ville Syrjala, Support Opensource,
Eddie James, Andrey Moiseev, Hans de Goede, Javier Carrasco,
linux-kernel
In-Reply-To: <20240904044756.1047629-1-dmitry.torokhov@gmail.com>
Hi Dmitry,
On Tue, Sep 03, 2024 at 09:47:55PM -0700, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that mutexes are released in all code paths
> when control leaves critical section.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/misc/iqs269a.c | 46 +++++++++++++-----------------------
> 1 file changed, 16 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index 843f8a3f3410..c34d847fa4af 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -365,7 +365,7 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
> if (mode > IQS269_CHx_ENG_A_ATI_MODE_MAX)
> return -EINVAL;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> engine_a = be16_to_cpu(ch_reg[ch_num].engine_a);
>
> @@ -375,8 +375,6 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
> ch_reg[ch_num].engine_a = cpu_to_be16(engine_a);
> iqs269->ati_current = false;
>
> - mutex_unlock(&iqs269->lock);
> -
> return 0;
> }
>
> @@ -389,9 +387,9 @@ static int iqs269_ati_mode_get(struct iqs269_private *iqs269,
> if (ch_num >= IQS269_NUM_CH)
> return -EINVAL;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
> +
> engine_a = be16_to_cpu(ch_reg[ch_num].engine_a);
> - mutex_unlock(&iqs269->lock);
>
> engine_a &= IQS269_CHx_ENG_A_ATI_MODE_MASK;
> *mode = (engine_a >> IQS269_CHx_ENG_A_ATI_MODE_SHIFT);
> @@ -429,7 +427,7 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269,
> return -EINVAL;
> }
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
>
> @@ -439,8 +437,6 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269,
> ch_reg[ch_num].engine_b = cpu_to_be16(engine_b);
> iqs269->ati_current = false;
>
> - mutex_unlock(&iqs269->lock);
> -
> return 0;
> }
>
> @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269,
> if (ch_num >= IQS269_NUM_CH)
> return -EINVAL;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
> +
> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
> - mutex_unlock(&iqs269->lock);
>
> switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) {
> case IQS269_CHx_ENG_B_ATI_BASE_75:
> @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269,
> if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX)
> return -EINVAL;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
>
> @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269,
> ch_reg[ch_num].engine_b = cpu_to_be16(engine_b);
> iqs269->ati_current = false;
>
> - mutex_unlock(&iqs269->lock);
> -
> return 0;
> }
>
> @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269,
> if (ch_num >= IQS269_NUM_CH)
> return -EINVAL;
>
> - mutex_lock(&iqs269->lock);
> - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
> - mutex_unlock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
> *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32;
>
> return 0;
> @@ -1199,7 +1192,7 @@ static int iqs269_dev_init(struct iqs269_private *iqs269)
> {
> int error;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> /*
> * Early revisions of silicon require the following workaround in order
> @@ -1210,19 +1203,19 @@ static int iqs269_dev_init(struct iqs269_private *iqs269)
> error = regmap_multi_reg_write(iqs269->regmap, iqs269_tws_init,
> ARRAY_SIZE(iqs269_tws_init));
> if (error)
> - goto err_mutex;
> + return error;
> }
>
> error = regmap_update_bits(iqs269->regmap, IQS269_HALL_UI,
> IQS269_HALL_UI_ENABLE,
> iqs269->hall_enable ? ~0 : 0);
> if (error)
> - goto err_mutex;
> + return error;
>
> error = regmap_raw_write(iqs269->regmap, IQS269_SYS_SETTINGS,
> &iqs269->sys_reg, sizeof(iqs269->sys_reg));
> if (error)
> - goto err_mutex;
> + return error;
>
> /*
> * The following delay gives the device time to deassert its RDY output
> @@ -1232,10 +1225,7 @@ static int iqs269_dev_init(struct iqs269_private *iqs269)
>
> iqs269->ati_current = true;
>
> -err_mutex:
> - mutex_unlock(&iqs269->lock);
> -
> - return error;
> + return 0;
> }
>
> static int iqs269_input_init(struct iqs269_private *iqs269)
> @@ -1580,13 +1570,11 @@ static ssize_t hall_enable_store(struct device *dev,
> if (error)
> return error;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> iqs269->hall_enable = val;
> iqs269->ati_current = false;
>
> - mutex_unlock(&iqs269->lock);
> -
> return count;
> }
>
> @@ -1643,13 +1631,11 @@ static ssize_t rx_enable_store(struct device *dev,
> if (val > 0xFF)
> return -EINVAL;
>
> - mutex_lock(&iqs269->lock);
> + guard(mutex)(&iqs269->lock);
>
> ch_reg[iqs269->ch_num].rx_enable = val;
> iqs269->ati_current = false;
>
> - mutex_unlock(&iqs269->lock);
> -
> return count;
> }
>
> --
> 2.46.0.469.g59c65b2a67-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH v8 2/9] mfd: core: make platform_data pointer const in struct mfd_cell
From: Heiko Stuebner @ 2024-09-08 21:07 UTC (permalink / raw)
To: lee, jikos, jic23
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, srinivas.pandruvada,
bentiss, dmitry.torokhov, pavel, ukleinek, devicetree,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-rockchip,
linux-input, linux-iio, linux-leds, Heiko Stuebner
In-Reply-To: <20240908210803.3339919-1-heiko@sntech.de>
The content of the platform_data of a struct mfd_cell is simply passed on
to the platform_device_add_data() call in mfd_add_device() .
platform_device_add_data() already handles the data behind that pointer
as const and also uses kmemdup to create a copy of the data before
handing that copy over to the newly created platform-device,
so there is no reason to not extend this to struct mfd_cell, as the old
copy in the mfd_cell will be stale anyway.
This allows to pass structs gathered from of_device_get_match_data()
as platform-data to sub-devices - which is retrieved as const already.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
include/linux/mfd/core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index e8bcad641d8c..faeea7abd688 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -72,7 +72,7 @@ struct mfd_cell {
int (*resume)(struct platform_device *dev);
/* platform data passed to the sub devices drivers */
- void *platform_data;
+ const void *platform_data;
size_t pdata_size;
/* Matches ACPI */
--
2.43.0
^ permalink raw reply related
* [PATCH v8 1/9] HID: hid-sensor-hub: don't use stale platform-data on remove
From: Heiko Stuebner @ 2024-09-08 21:07 UTC (permalink / raw)
To: lee, jikos, jic23
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, srinivas.pandruvada,
bentiss, dmitry.torokhov, pavel, ukleinek, devicetree,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-rockchip,
linux-input, linux-iio, linux-leds, Heiko Stuebner
In-Reply-To: <20240908210803.3339919-1-heiko@sntech.de>
The hid-sensor-hub creates the individual device structs and transfers them
to the created mfd platform-devices via the platform_data in the mfd_cell.
Before commit e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
the sensor-hub was managing access centrally, with one "completion" in the
hub's data structure, which needed to be finished on removal at the latest.
The mentioned commit then moved this central management to each hid sensor
device, resulting on a completion in each struct hid_sensor_hub_device.
The remove procedure was adapted to go through all sensor devices and
finish any pending "completion".
What this didn't take into account was, platform_device_add_data() that is
used by mfd_add{_hotplug}_devices() does a kmemdup on the submitted
platform-data. So the data the platform-device gets is a copy of the
original data, meaning that the device worked on a different completion
than what sensor_hub_remove() currently wants to access.
To fix that, use device_for_each_child() to go through each child-device
similar to how mfd_remove_devices() unregisters the devices later and
with that get the live platform_data to finalize the correct completion.
Fixes: e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/hid/hid-sensor-hub.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 26e93a331a51..3cd00afa453a 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -730,23 +730,30 @@ static int sensor_hub_probe(struct hid_device *hdev,
return ret;
}
+static int sensor_hub_finalize_pending_fn(struct device *dev, void *data)
+{
+ struct hid_sensor_hub_device *hsdev = dev->platform_data;
+
+ if (hsdev->pending.status)
+ complete(&hsdev->pending.ready);
+
+ return 0;
+}
+
static void sensor_hub_remove(struct hid_device *hdev)
{
struct sensor_hub_data *data = hid_get_drvdata(hdev);
unsigned long flags;
- int i;
hid_dbg(hdev, " hardware removed\n");
hid_hw_close(hdev);
hid_hw_stop(hdev);
+
spin_lock_irqsave(&data->lock, flags);
- for (i = 0; i < data->hid_sensor_client_cnt; ++i) {
- struct hid_sensor_hub_device *hsdev =
- data->hid_sensor_hub_client_devs[i].platform_data;
- if (hsdev->pending.status)
- complete(&hsdev->pending.ready);
- }
+ device_for_each_child(&hdev->dev, NULL,
+ sensor_hub_finalize_pending_fn);
spin_unlock_irqrestore(&data->lock, flags);
+
mfd_remove_devices(&hdev->dev);
mutex_destroy(&data->mutex);
}
--
2.43.0
^ permalink raw reply related
* [PATCH v8 3/9] dt-bindings: mfd: add binding for qnap,ts433-mcu devices
From: Heiko Stuebner @ 2024-09-08 21:07 UTC (permalink / raw)
To: lee, jikos, jic23
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, srinivas.pandruvada,
bentiss, dmitry.torokhov, pavel, ukleinek, devicetree,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-rockchip,
linux-input, linux-iio, linux-leds, Heiko Stuebner, Conor Dooley
In-Reply-To: <20240908210803.3339919-1-heiko@sntech.de>
These MCUs can be found in network attached storage devices made by QNAP.
They are connected to a serial port of the host device and provide
functionality like LEDs, power-control and temperature monitoring.
LEDs, buttons, etc are all elements of the MCU firmware itself, so don't
need devicetree input, though the fan gets its cooling settings from
a fan-0 subnode.
A binding for the LEDs for setting the linux-default-trigger may come
later, once all the LEDs are understood and ATA controllers actually
can address individual port-LEDs, but are really optional.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
.../bindings/mfd/qnap,ts433-mcu.yaml | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
diff --git a/Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml b/Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
new file mode 100644
index 000000000000..877078ac172f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/qnap,ts433-mcu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: QNAP NAS on-board Microcontroller
+
+maintainers:
+ - Heiko Stuebner <heiko@sntech.de>
+
+description:
+ QNAP embeds a microcontroller on their NAS devices adding system feature
+ as PWM Fan control, additional LEDs, power button status and more.
+
+properties:
+ compatible:
+ enum:
+ - qnap,ts433-mcu
+
+patternProperties:
+ "^fan-[0-9]+$":
+ $ref: /schemas/hwmon/fan-common.yaml#
+ unevaluatedProperties: false
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ uart {
+ mcu {
+ compatible = "qnap,ts433-mcu";
+
+ fan-0 {
+ #cooling-cells = <2>;
+ cooling-levels = <0 64 89 128 166 204 221 238>;
+ };
+ };
+ };
--
2.43.0
^ permalink raw reply related
* [PATCH v8 0/9] Drivers to support the MCU on QNAP NAS devices
From: Heiko Stuebner @ 2024-09-08 21:07 UTC (permalink / raw)
To: lee, jikos, jic23
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, srinivas.pandruvada,
bentiss, dmitry.torokhov, pavel, ukleinek, devicetree,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-rockchip,
linux-input, linux-iio, linux-leds, Heiko Stuebner
This implements a set of drivers for the MCU used on QNAP NAS devices.
Of course no documentation for the serial protocol is available, so
thankfully QNAP has a tool on their rescue-inird to talk to the MCU and
I found interceptty [0] to listen to what goes over the serial connection.
In general it looks like there are two different generations in general,
an "EC" device and now this "MCU" - referenced in the strings of the
userspace handlers for those devices.
For the MCU "SPEC3" and "SPEC4" are listed which is configured in
the model.conf of the device. When setting the value from SPEC4 to
SPEC3 on my TS433, the supported commands change, but the command
interface stays the same and especially the version command is the
same.
The binding also does not expose any interals of the device that
might change, so hopefully there shouldn't be big roadblocks to
support different devices, apart from possibly adapting the commands.
changes in v8:
- patch for hid-sensor hub to not do wonky stuff with an old
platform-data copy
I hope my reading of the situation is correct here, but that
initial platform_data really seemed wrong
mfd:
- flush serial before writing a new command
- wait for send to complete before starting the receive wait-timeout
- set expected length to 0 directly when the reply is complete
not after leaving the receive callback
changes in v7:
- use ASCII representation in commands where possible instead of hex vals
- drop get_variant function and use mfd platform-data instead
mfd:
- a lot of style improvements
leds:
- name variables better (value -> brightness, num -> num_err_led)
- handle preservation of blink mode more effectively
- snprintf -> scnprintf
- drop duplicate "failed to register ... LED" messages
changes in v6:
- format mcu commands arrays in single lines (Lee)
mfd:
- drop obsolete remain kdoc for the removed
reply_lock (kernel test robot)
changes in v5:
binding:
- add Conor's Reviewed-by
mfd:
Address comments from Lee
- improve commit message
- improve Kconfig help text
- sort headers alphabetical
- style and spelling improvements
- constants for magic numbers
- drop reply assignment, the mcu only replies to commands sent to it,
so there should only ever be one command in fligth.
hwmon:
Add Acked-by from Guenter and address some remarks
- don't allow empty fan subnode
- use num var directly when getting cooling levels, without using ret
intermediate
- use dev_err_probe in thermal init function
changes in v4:
binding:
- move cooling properties into a fan subnode and reference
fan-common.yaml (Rob)
- dropped Krzysztof's Ack because of this
mfd:
- use correct format-string for size_t (kernel test robot)
input:
- added Dmitry's Ack
hwmon:
- adapted to fan-subnode when reading cooling properties
- dropped Guenter's Ack because of this
changes in v3:
mfd
- use correct power-off priority: default
- constify the cmd-data array in command functions (Dmitry)
leds:
- don't point to temporary buffers for cdev->name (Florian Eckert)
hwmon:
- use clamp_val(), don't try to reimplement (Guenter)
- add Guenter's Ack
input:
address Dmitry's comments
- constify some cmd arrays
- add input-close callback to cancel beep worker
- drop initial input event report
changes in v2:
binding:
- rename to qnap,ts433-mcu.yaml (Krzysztof)
- drop "preserve formatting" indicator (Krzysztof)
- add Krzysztof's Review tag
mfd:
- fix checkpatch --strict CHECKs
- add a MAINTAINERS entry for all qnap-mcu-parts
Heiko Stuebner (9):
HID: hid-sensor-hub: don't use stale platform-data on remove
mfd: core: make platform_data pointer const in struct mfd_cell
dt-bindings: mfd: add binding for qnap,ts433-mcu devices
mfd: add base driver for qnap-mcu devices
leds: add driver for LEDs from qnap-mcu devices
Input: add driver for the input part of qnap-mcu devices
hwmon: add driver for the hwmon parts of qnap-mcu devices
arm64: dts: rockchip: hook up the MCU on the QNAP TS433
arm64: dts: rockchip: set hdd led labels on qnap-ts433
.../bindings/mfd/qnap,ts433-mcu.yaml | 42 ++
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/qnap-mcu-hwmon.rst | 27 ++
MAINTAINERS | 9 +
.../boot/dts/rockchip/rk3568-qnap-ts433.dts | 61 +++
drivers/hid/hid-sensor-hub.c | 21 +-
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/qnap-mcu-hwmon.c | 364 ++++++++++++++++++
drivers/input/misc/Kconfig | 12 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/qnap-mcu-input.c | 153 ++++++++
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-qnap-mcu.c | 227 +++++++++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 2 +
drivers/mfd/qnap-mcu.c | 332 ++++++++++++++++
include/linux/mfd/core.h | 2 +-
include/linux/mfd/qnap-mcu.h | 26 ++
20 files changed, 1310 insertions(+), 8 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
create mode 100644 Documentation/hwmon/qnap-mcu-hwmon.rst
create mode 100644 drivers/hwmon/qnap-mcu-hwmon.c
create mode 100644 drivers/input/misc/qnap-mcu-input.c
create mode 100644 drivers/leds/leds-qnap-mcu.c
create mode 100644 drivers/mfd/qnap-mcu.c
create mode 100644 include/linux/mfd/qnap-mcu.h
--
2.43.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox