[PATCH 1/1] v4l2-device: Don't unregister ACPI/Device Tree based devices

[PATCH 1/1] v4l2-device: Don't unregister ACPI/Device Tree based devices

[Sakari Ailus]

From: Tommi Franttila <tommi.franttila@intel.com>

When a V4L2 sub-device backed by a DT or ACPI based device was removed,
the device was unregistered as well which certainly was not intentional,
as the client device would not be re-created by simply reinstating the
V4L2 sub-device (indeed the device would have to be there first!).

Skip unregistering the device in case it has non-NULL of_node or fwnode.

Signed-off-by: Tommi Franttila <tommi.franttila@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 5b0a30b..3c8cc9a 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -122,7 +122,8 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 			   We cannot rely on i2c_del_adapter to always
 			   unregister clients for us, since if the i2c bus
 			   is a platform bus, then it is never deleted. */
-			if (client)
+			if (client &&
+			    !client->dev.of_node && !client->dev.fwnode)
 				i2c_unregister_device(client);
 			continue;
 		}
@@ -131,7 +132,7 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 		if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) {
 			struct spi_device *spi = v4l2_get_subdevdata(sd);
 
-			if (spi)
+			if (spi && !spi->dev.of_node && !spi->dev.fwnode)
 				spi_unregister_device(spi);
 			continue;
 		}
-- 
2.1.0.231.g7484e3b

Re: [PATCH 1/1] v4l2-device: Don't unregister ACPI/Device Tree based devices

[Hans Verkuil]

On 11/11/15 14:50, Sakari Ailus wrote:
> From: Tommi Franttila <tommi.franttila@intel.com>
> 
> When a V4L2 sub-device backed by a DT or ACPI based device was removed,
> the device was unregistered as well which certainly was not intentional,
> as the client device would not be re-created by simply reinstating the
> V4L2 sub-device (indeed the device would have to be there first!).
> 
> Skip unregistering the device in case it has non-NULL of_node or fwnode.
> 
> Signed-off-by: Tommi Franttila <tommi.franttila@intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-device.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 5b0a30b..3c8cc9a 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -122,7 +122,8 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
>  			   We cannot rely on i2c_del_adapter to always
>  			   unregister clients for us, since if the i2c bus
>  			   is a platform bus, then it is never deleted. */

Can this comment be extended? This is non-trivial and it is helpful to
document this. Other than that it looks good.

Thanks,

	Hans

> -			if (client)
> +			if (client &&
> +			    !client->dev.of_node && !client->dev.fwnode)
>  				i2c_unregister_device(client);
>  			continue;
>  		}
> @@ -131,7 +132,7 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
>  		if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) {
>  			struct spi_device *spi = v4l2_get_subdevdata(sd);
>  
> -			if (spi)
> +			if (spi && !spi->dev.of_node && !spi->dev.fwnode)
>  				spi_unregister_device(spi);
>  			continue;
>  		}
> 

[PATCH v2 1/1] v4l2-device: Don't unregister ACPI/Device Tree based devices

[Sakari Ailus]

From: Tommi Franttila <tommi.franttila@intel.com>

When a V4L2 sub-device backed by a DT or ACPI based device was removed,
the device was unregistered as well which certainly was not intentional,
as the client device would not be re-created by simply reinstating the
V4L2 sub-device (indeed the device would have to be there first!).

Skip unregistering the device in case it has non-NULL of_node or fwnode.

Signed-off-by: Tommi Franttila <tommi.franttila@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---

Hi Hans,

Thanks for the comment! How about this one?

Regards,
Sakari

 drivers/media/v4l2-core/v4l2-device.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 5b0a30b..7129e43 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -118,11 +118,20 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 		if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) {
 			struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-			/* We need to unregister the i2c client explicitly.
-			   We cannot rely on i2c_del_adapter to always
-			   unregister clients for us, since if the i2c bus
-			   is a platform bus, then it is never deleted. */
-			if (client)
+			/*
+			 * We need to unregister the i2c client
+			 * explicitly. We cannot rely on
+			 * i2c_del_adapter to always unregister
+			 * clients for us, since if the i2c bus is a
+			 * platform bus, then it is never deleted.
+			 *
+			 * Device tree or ACPI based devices must not
+			 * be unregistered as they have not been
+			 * registered by us, and would not be
+			 * re-created by just probing the V4L2 driver.
+			 */
+			if (client &&
+			    !client->dev.of_node && !client->dev.fwnode)
 				i2c_unregister_device(client);
 			continue;
 		}
@@ -131,7 +140,7 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 		if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) {
 			struct spi_device *spi = v4l2_get_subdevdata(sd);
 
-			if (spi)
+			if (spi && !spi->dev.of_node && !spi->dev.fwnode)
 				spi_unregister_device(spi);
 			continue;
 		}
-- 
2.1.0.231.g7484e3b

Re: [PATCH v2 1/1] v4l2-device: Don't unregister ACPI/Device Tree based devices

[Hans Verkuil]

On 11/12/15 10:01, Sakari Ailus wrote:
> From: Tommi Franttila <tommi.franttila@intel.com>
> 
> When a V4L2 sub-device backed by a DT or ACPI based device was removed,
> the device was unregistered as well which certainly was not intentional,
> as the client device would not be re-created by simply reinstating the
> V4L2 sub-device (indeed the device would have to be there first!).
> 
> Skip unregistering the device in case it has non-NULL of_node or fwnode.
> 
> Signed-off-by: Tommi Franttila <tommi.franttila@intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> 
> Hi Hans,
> 
> Thanks for the comment! How about this one?

Lovely, thanks!

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> Regards,
> Sakari
> 
>  drivers/media/v4l2-core/v4l2-device.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 5b0a30b..7129e43 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -118,11 +118,20 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
>  		if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) {
>  			struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
> -			/* We need to unregister the i2c client explicitly.
> -			   We cannot rely on i2c_del_adapter to always
> -			   unregister clients for us, since if the i2c bus
> -			   is a platform bus, then it is never deleted. */
> -			if (client)
> +			/*
> +			 * We need to unregister the i2c client
> +			 * explicitly. We cannot rely on
> +			 * i2c_del_adapter to always unregister
> +			 * clients for us, since if the i2c bus is a
> +			 * platform bus, then it is never deleted.
> +			 *
> +			 * Device tree or ACPI based devices must not
> +			 * be unregistered as they have not been
> +			 * registered by us, and would not be
> +			 * re-created by just probing the V4L2 driver.
> +			 */
> +			if (client &&
> +			    !client->dev.of_node && !client->dev.fwnode)
>  				i2c_unregister_device(client);
>  			continue;
>  		}
> @@ -131,7 +140,7 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
>  		if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) {
>  			struct spi_device *spi = v4l2_get_subdevdata(sd);
>  
> -			if (spi)
> +			if (spi && !spi->dev.of_node && !spi->dev.fwnode)
>  				spi_unregister_device(spi);
>  			continue;
>  		}
>