Re: [PATCH v3 1/1] hwmon: (sht3x) read out sensor serial number

[Guenter Roeck]

On Mon, Jan 01, 2024 at 11:18:21PM +0100, Stefan Gloor wrote:
> On Sun, Dec 31, 2023 at 10:32:56AM -0800, Guenter Roeck wrote:
> > This creates i2c<bus>-<address>/serial_number when the device is instantiated.
> > That debugfs entry is not removed when the device is removed, only when the
> > driver is unloaded. This means that de-instantiating the device will leave
> > stray debugfs directories and files behind until the driver is unloaded.
> > 
> > We had this before, and I understand that you claimed that this doesn't happen.
> > To get me to believe you, you'll have to provide a log of
> > 
> > - instantiating the driver 
> > - Showing the debufs tree
> > - de-instantiating the driver
> > - Showing the debugfs tree
> > 
> > ... but even then I'll want to be able to test it myself. Not sure if I
> > have an eval board, but either case that will take some time. Frankly,
> > I don't understand why you refuse to remove
> > i2c<bus>-<address>/serial_number on device removal.
> > 
> > Guenter
> > 
> 
> Hi Guenter,
> 
> Thank you for your patience. As this is my first patch set for Linux I still
> need to learn a lot.
> 
> You are right. I was confused about driver instantiation and driver
> loading/unloading. The i2cX-XX directory needs to be removed explicitly.
> 
> If I understood correctly, the following changes should achieve this:
> 
> +static void sht3x_remove(struct i2c_client *client)
> +{
> +	struct sht3x_data *data;
> +
> +	data = dev_get_drvdata(&client->dev);
> +	debugfs_remove_recursive(data->sensor_dir);
> +}
> +
>  static struct i2c_driver sht3x_i2c_driver = {
>  	.driver.name = "sht3x",
>  	.probe       = sht3x_probe,
> +	.remove      = sht3x_remove,
>  	.id_table    = sht3x_ids,
>  };
> 
> Of course data->sensor_dir needs to be set to the i2X-XX directory when it is
> created.
> 
> If there is nothing obviously wrong with it I'll submit v4 shortly.
> 

This is correct. I personally would prefer the use of devm_add_action_or_reset()
instead of a remove function, but the above works as well. Make sure though
that debugfs_remove_recursive() is also called if hwmon registration fails.

Thanks,
Guenter