Re: [PATCH 07/10] platform/x86: toshiba_acpi: use device-managed functions for accelerometer

From: Jonathan Cameron <jic23@kernel.org>
To: Alexandru Ardelean <aardelean@deviqon.com>
Cc: platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	coproscefalo@gmail.com, hdegoede@redhat.com,
	mgross@linux.intel.com, linux@deviqon.com
Subject: Re: [PATCH 07/10] platform/x86: toshiba_acpi: use device-managed functions for accelerometer
Date: Mon, 29 Mar 2021 15:54:37 +0100	[thread overview]
Message-ID: <20210329155437.65932679@jic23-huawei> (raw)
In-Reply-To: <20210324125548.45983-8-aardelean@deviqon.com>

On Wed, 24 Mar 2021 14:55:45 +0200
Alexandru Ardelean <aardelean@deviqon.com> wrote:

> This change converts the IIO registration to use devm_iio_device_alloc()
> and devm_iio_device_register().
> With this change we can remove the manual deregistrations an freeing of the
> IIO data.
> 
> This also makes the deregistration symmetrical with the registration.
> 
> One side-effect (that is undesired), is that if devm_iio_device_register()
> fails, then the IIO object will not be free'd and will stick around until
> the parent object is free'd. This is because there is no
> devm_iio_device_free() function anymore in IIO.
> However, this is a pretty bad corner-case that should not happen under
> normal operation.

Hmm. The way this driver papers over failed elements is rather irritating,
though I'm sure there are reasons for it.  Indeed, I'd not worry about the
left over iio_dev structures

> 
> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/platform/x86/toshiba_acpi.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index e787c140eec2..12860ef60e4d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2992,11 +2992,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>  
>  	remove_toshiba_proc_entries(dev);
>  
> -	if (dev->accelerometer_supported && dev->indio_dev) {
> -		iio_device_unregister(dev->indio_dev);
> -		iio_device_free(dev->indio_dev);
> -	}
> -
>  	if (dev->sysfs_created)
>  		sysfs_remove_group(&dev->acpi_dev->dev.kobj,
>  				   &toshiba_attr_group);
> @@ -3149,7 +3144,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  
>  	toshiba_accelerometer_available(dev);
>  	if (dev->accelerometer_supported) {
> -		dev->indio_dev = iio_device_alloc(&acpi_dev->dev, sizeof(*dev));
> +		dev->indio_dev = devm_iio_device_alloc(parent, sizeof(*dev));
>  		if (!dev->indio_dev) {
>  			pr_err("Unable to allocate iio device\n");
>  			goto iio_error;
> @@ -3164,10 +3159,10 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  		dev->indio_dev->num_channels =
>  					ARRAY_SIZE(toshiba_iio_accel_channels);
>  
> -		ret = iio_device_register(dev->indio_dev);
> +		ret = devm_iio_device_register(parent, dev->indio_dev);
>  		if (ret < 0) {
>  			pr_err("Unable to register iio device\n");
> -			iio_device_free(dev->indio_dev);
> +			dev->indio_dev = NULL;
>  		}
>  	}
>  iio_error: