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

[Jonathan Cameron]

On Sat, 8 Aug 2026 23:10:11 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

> On Thu, Aug 06, 2026 at 05:55:49PM +0530, Sanjay Chitroda via B4 Relay wrote:
> 
> > 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  
> 
> Store
> 
> > dedicated struct sensor_hub_cb_devres, and register a devres action to
> > handle to release resource with devres framework and helper API.  
> 
> ...
> 
> > +static void sensor_hub_remove_callback_helper(void *ptr)
> > +{
> > +	struct sensor_hub_cb_devres *res = ptr;
> > +
> > +	sensor_hub_remove_callback(res->hsdev, res->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 *res;
> > +	int ret;
> > +
> > +	ret = sensor_hub_register_callback(hsdev, usage_id, usage_callback);
> > +	if (ret)
> > +		return ret;
> > +
> > +	res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL);
> > +	if (!res) {
> > +		sensor_hub_remove_callback(hsdev, usage_id);
> > +		return -ENOMEM;
> > +	}  
> 
> Why is this order? What's wrong with the memory allocation first?

Why do we have a devm_kmalloc() in here at all rather than devres_alloc()
/devres_add()  When there is an allocation needed that tends to
end up at a similar level of complexity but with one fewer allocation and
similar unwind.  Fun thought, maybe we can have a DEFINE_FREE() to do
the cleanup (devres_free) on error in sensor_hub_register_callback()
with devres_add(no_free_ptr(devres)) to hand over ownership.


> 
> > +	res->hsdev = hsdev;
> > +	res->usage_id = usage_id;
> > +
> > +	return devm_add_action_or_reset(dev, sensor_hub_remove_callback_helper, res);
> > +}  
> 
> ...
> 
> > +/**
> > + * devm_sensor_hub_register_callback() - Managed register client callbacks
> > + * @dev: Device for resource management
> > + * @hsdev: Hub device instance
> > + * @usage_id: Usage id of the client (e.g. 0x200076 for Gyro)
> > + * @usage_callback: Callback function storage
> > + *
> > + * This is the devres (managed) version of sensor_hub_register_callback().
> > + * The callback will be automatically unregistered when the device is detached.  
> 
> Here, and everywhere else, validate kernel-doc. Here you introduced a new warning
> (missing return section).
> 
> > + */  
> 
> Also, why kernel-doc is here?! It should accompany the implementation, here is
> a declaration.
> 
> > +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);  
>