Re: [PATCH v6 03/14] HID: hid-sensor-hub: introduce device managed API

[Jonathan Cameron]

On Sat, 19 Sep 2026 16:06:26 +0530
Sanjay Chitroda via B4 Relay <devnull+sanjayembeddedse.gmail.com@xxxxxxxxxx> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>
> 
> sensor_hub_register_callback() is common API used for the HID IIO drivers,
> prepare devm API devm_sensor_hub_register_callback() to acquire resource
> during setup and release using device managed framework during drivers
> fail, unbind or remove path.
> 
> Store the required callback removal context (hsdev and usage_id) in a
> dedicated struct sensor_hub_cb_devres, and register resources with
> devres framework and helper API.
> 
> Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>

One thing in here. If it's all that comes up I might just tweak it
whilst applying.  I do want to give a bit of time for others to look
at this series anyway before applying so won't do that immediately.

Thanks,

Jonathan

> ---
> Changes in v6:
> - Add kernel-doc for return statement for the new devm_*() API, as
>   suggested by Andy.
> - Keep kernel-doc for new devm_*() API in public header file, along
>   side where existing API documentation is available.
> - With suggestion/input from Jonathan, use devres and cleanup frameworks
>   together to implement device managed API support with less complexity.
> ---
>  drivers/hid/hid-sensor-hub.c   | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/hid-sensor-hub.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 6470a290ebfc..de35e72b4d88 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -4,7 +4,9 @@
>   * Copyright (c) 2012, Intel Corporation.
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/device.h>
> +#include <linux/device/devres.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -187,6 +189,39 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
>  }
>  EXPORT_SYMBOL_GPL(sensor_hub_remove_callback);
>  
> +DEFINE_FREE(devres_free, struct sensor_hub_cb_devres *, if (_T) devres_free(_T))
> +
> +static void devm_sensor_hub_remove_callback(struct device *dev, void *res)
> +{
> +	struct sensor_hub_cb_devres *dr = res;
> +
> +	sensor_hub_remove_callback(dr->hsdev, dr->usage_id);
> +}
> +
> +int devm_sensor_hub_register_callback(struct device *dev,
> +				      struct hid_sensor_hub_device *hsdev,
> +				      u32 usage_id,
> +				      struct hid_sensor_hub_callbacks *usage_callback)
> +{
> +	struct sensor_hub_cb_devres *dr __free(devres_free) = NULL;

This must be as tightly coupled as possible with the allocation.
	int ret;

	struct sensor_hb_cb_devres *dr __free(devres_free) = 
		devres_alloc(devm_sensor_hub_remove_callback, sizeof(*dr), GFP_KERNEL);
	if (!dr)
		return -ENOMEM;

No idea why we see so many folk doing the = NULL thing given the guidance in
cleanup.h that says never to do that!  If you are bored, it is a fun saga of
weird things compilers can do and not warn you about + fragility of resulting
code and the desire to keep the code readable.

> +	int ret;
> +
> +	dr = devres_alloc(devm_sensor_hub_remove_callback, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	ret = sensor_hub_register_callback(hsdev, usage_id, usage_callback);
> +	if (ret)
> +		return ret;
> +
> +	dr->hsdev = hsdev;
> +	dr->usage_id = usage_id;
> +	devres_add(dev, no_free_ptr(dr));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_sensor_hub_register_callback);