[PATCH V1] [media] V4L: soc-camera: add SPI device support

[PATCH V1] [media] V4L: soc-camera: add SPI device support

[Kassey Li]

From: Kassey Li <kasseyl@nvidia.com>

This adds support for spi interface sub device for
soc_camera.

Signed-off-by: Kassey Li <kasseyl@nvidia.com>
---
 drivers/media/platform/soc_camera/soc_camera.c |   51 ++++++++++++++++++++++++
 include/media/soc_camera.h                     |   10 +++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index b3db51c..6db2d89 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1599,6 +1599,49 @@ static void scan_async_host(struct soc_camera_host *ici)
 #define soc_camera_i2c_free(icd)	do {} while (0)
 #define scan_async_host(ici)		do {} while (0)
 #endif
+static int soc_camera_init_spi(struct soc_camera_device *icd,
+		struct soc_camera_desc *sdesc)
+{
+	struct spi_device   *spi;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+	struct soc_camera_host_desc *shd = &sdesc->host_desc;
+	struct spi_master *spi_master =
+			spi_busnum_to_master(shd->spi_bus_id);
+	struct v4l2_subdev *subdev;
+
+	if (!spi_master) {
+		dev_err(icd->pdev, "Cannot get spi master #%d. No driver?\n",
+				shd->spi_bus_id);
+		goto espind;
+	}
+
+	shd->board_info_spi->platform_data = &sdesc->subdev_desc;
+
+	subdev = v4l2_spi_new_subdev(&ici->v4l2_dev, spi_master,
+			shd->board_info_spi);
+	if (!subdev)
+		goto espind;
+
+	spi = v4l2_get_subdevdata(subdev);
+
+	/* Use to_i2c_client(dev) to recover the i2c client */
+	icd->control = &spi->dev;
+
+	return 0;
+espind:
+	return -ENODEV;
+}
+
+static void soc_camera_free_spi(struct soc_camera_device *icd)
+{
+	struct spi_device   *spi;
+	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+
+	spi = v4l2_get_subdevdata(sd);
+	icd->control = NULL;
+	v4l2_device_unregister_subdev(sd);
+	spi_unregister_device(spi);
+}
 
 #ifdef CONFIG_OF
 
@@ -1762,6 +1805,10 @@ static int soc_camera_probe(struct soc_camera_host *ici,
 		ret = soc_camera_i2c_init(icd, sdesc);
 		if (ret < 0 && ret != -EPROBE_DEFER)
 			goto eadd;
+	} else if (shd->board_info_spi) {
+               ret = soc_camera_init_spi(icd, sdesc);
+               if (ret < 0)
+                       goto eadd;
 	} else if (!shd->add_device || !shd->del_device) {
 		ret = -EINVAL;
 		goto eadd;
@@ -1803,6 +1850,8 @@ static int soc_camera_probe(struct soc_camera_host *ici,
 efinish:
 	if (shd->board_info) {
 		soc_camera_i2c_free(icd);
+	} else if (shd->board_info_spi) {
+		soc_camera_free_spi(icd);
 	} else {
 		shd->del_device(icd);
 		module_put(control->driver->owner);
@@ -1843,6 +1892,8 @@ static int soc_camera_remove(struct soc_camera_device *icd)
 
 	if (sdesc->host_desc.board_info) {
 		soc_camera_i2c_free(icd);
+	} else if (sdesc->host_desc.board_info_spi) {
+		soc_camera_free_spi(icd);
 	} else {
 		struct device *dev = to_soc_camera_control(icd);
 		struct device_driver *drv = dev ? dev->driver : NULL;
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 2f6261f..7530893 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -178,6 +178,11 @@ struct soc_camera_host_desc {
 	int i2c_adapter_id;
 	struct i2c_board_info *board_info;
 	const char *module_name;
+	/*
+	 * Add SPI device support.
+	 */
+	struct spi_board_info *board_info_spi;
+	int spi_bus_id;
 
 	/*
 	 * For non-I2C devices platform has to provide methods to add a device
@@ -243,6 +248,11 @@ struct soc_camera_link {
 	int i2c_adapter_id;
 	struct i2c_board_info *board_info;
 	const char *module_name;
+	/*
+	 * Add SPI device support.
+	 */
+	struct spi_board_info *board_info_spi;
+	int spi_bus_id;
 
 	/*
 	 * For non-I2C devices platform has to provide methods to add a device
-- 
1.7.9.5

Re: [PATCH V1] [media] V4L: soc-camera: add SPI device support

[Guennadi Liakhovetski]

Hi Kassey,

Thanks for the patch. Do I understand it right, that this patch only 
supports SPI subdevices, supplied in platform data, no support for 
asynchronous SPI clients / DT? Does your platform not support DT?

I'm not an expert in SPI, so, not really sure how correct is the use of 
the SPI API here.

On Tue, 27 Jan 2015, Kassey Li wrote:

> From: Kassey Li <kasseyl@nvidia.com>
> 
> This adds support for spi interface sub device for
> soc_camera.
> 
> Signed-off-by: Kassey Li <kasseyl@nvidia.com>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |   51 ++++++++++++++++++++++++
>  include/media/soc_camera.h                     |   10 +++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index b3db51c..6db2d89 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -1599,6 +1599,49 @@ static void scan_async_host(struct soc_camera_host *ici)
>  #define soc_camera_i2c_free(icd)	do {} while (0)
>  #define scan_async_host(ici)		do {} while (0)
>  #endif
> +static int soc_camera_init_spi(struct soc_camera_device *icd,
> +		struct soc_camera_desc *sdesc)

The rest of the file _mostly_ uses a different line-breaking style, but... 
This isn't completely incompatible with the rest, so, I wouldn't insist on 
changing this, probably. Just to explain, normally in this file alignment 
is performed in theform

	ret = func(a, b, c,
		   d, e);

i.e. first TABs are used and then spaces to align, say, under the first 
argument in this case. If, however, this alignment style would make the 
second line too long, then yes, additional spaces and, possibly, one or 
more TABs are removed. So, at least this kinf of alignment

	ret = func(a, b, c,
			d, e);

is never used... But, as I said, up to you, would be nice to have a 
somewhat better style compliance.

> +{
> +	struct spi_device   *spi;

Please, just one space.

> +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> +	struct soc_camera_host_desc *shd = &sdesc->host_desc;
> +	struct spi_master *spi_master =
> +			spi_busnum_to_master(shd->spi_bus_id);

You certainly need an additional header for this and other SPI macros, 
functions and types.

> +	struct v4l2_subdev *subdev;
> +
> +	if (!spi_master) {
> +		dev_err(icd->pdev, "Cannot get spi master #%d. No driver?\n",
> +				shd->spi_bus_id);

This is the kind of alignment, that this file otherwise doesn't use. Just 
removing one TAB above would already make it look more consistent:)

> +		goto espind;

You can just return here.

> +	}
> +
> +	shd->board_info_spi->platform_data = &sdesc->subdev_desc;
> +
> +	subdev = v4l2_spi_new_subdev(&ici->v4l2_dev, spi_master,
> +			shd->board_info_spi);
> +	if (!subdev)
> +		goto espind;

Comparing to the I2C subdevice initialisation, this version is lacking 
regulator and clock support... Is it not needed?

> +
> +	spi = v4l2_get_subdevdata(subdev);
> +
> +	/* Use to_i2c_client(dev) to recover the i2c client */
> +	icd->control = &spi->dev;
> +
> +	return 0;
> +espind:
> +	return -ENODEV;
> +}
> +
> +static void soc_camera_free_spi(struct soc_camera_device *icd)
> +{
> +	struct spi_device   *spi;
> +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> +
> +	spi = v4l2_get_subdevdata(sd);
> +	icd->control = NULL;
> +	v4l2_device_unregister_subdev(sd);
> +	spi_unregister_device(spi);
> +}
>  
>  #ifdef CONFIG_OF
>  
> @@ -1762,6 +1805,10 @@ static int soc_camera_probe(struct soc_camera_host *ici,
>  		ret = soc_camera_i2c_init(icd, sdesc);
>  		if (ret < 0 && ret != -EPROBE_DEFER)
>  			goto eadd;
> +	} else if (shd->board_info_spi) {
> +               ret = soc_camera_init_spi(icd, sdesc);
> +               if (ret < 0)
> +                       goto eadd;
>  	} else if (!shd->add_device || !shd->del_device) {
>  		ret = -EINVAL;
>  		goto eadd;
> @@ -1803,6 +1850,8 @@ static int soc_camera_probe(struct soc_camera_host *ici,
>  efinish:
>  	if (shd->board_info) {
>  		soc_camera_i2c_free(icd);
> +	} else if (shd->board_info_spi) {
> +		soc_camera_free_spi(icd);
>  	} else {
>  		shd->del_device(icd);
>  		module_put(control->driver->owner);
> @@ -1843,6 +1892,8 @@ static int soc_camera_remove(struct soc_camera_device *icd)
>  
>  	if (sdesc->host_desc.board_info) {
>  		soc_camera_i2c_free(icd);
> +	} else if (sdesc->host_desc.board_info_spi) {
> +		soc_camera_free_spi(icd);
>  	} else {
>  		struct device *dev = to_soc_camera_control(icd);
>  		struct device_driver *drv = dev ? dev->driver : NULL;
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 2f6261f..7530893 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -178,6 +178,11 @@ struct soc_camera_host_desc {
>  	int i2c_adapter_id;
>  	struct i2c_board_info *board_info;
>  	const char *module_name;
> +	/*
> +	 * Add SPI device support.
> +	 */

This comment documents an action of "adding" SPI support, which isn't 
interesting in the long run. It doesn't provide any additional information 
about the code. Please, remove.

> +	struct spi_board_info *board_info_spi;
> +	int spi_bus_id;
>  
>  	/*
>  	 * For non-I2C devices platform has to provide methods to add a device
> @@ -243,6 +248,11 @@ struct soc_camera_link {
>  	int i2c_adapter_id;
>  	struct i2c_board_info *board_info;
>  	const char *module_name;
> +	/*
> +	 * Add SPI device support.
> +	 */

Ditto, just remove it.

> +	struct spi_board_info *board_info_spi;
> +	int spi_bus_id;

[OT] This reminds me... This struct has to be removed...

>  
>  	/*
>  	 * For non-I2C devices platform has to provide methods to add a device
> -- 
> 1.7.9.5
> 

Thanks
Guennadi

Re: [PATCH V1] [media] V4L: soc-camera: add SPI device support

[Kassey]

2015-02-01 23:27 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi Kassey,
>
> Thanks for the patch. Do I understand it right, that this patch only
> supports SPI subdevices, supplied in platform data, no support for
> asynchronous SPI clients / DT? Does your platform not support DT?
>
   yes, we support DT, add DT in patch V2 later for your to review.
> I'm not an expert in SPI, so, not really sure how correct is the use of
> the SPI API here.
>
   I have a sony sensor which is spi interface, verified on kernel-3.10

> On Tue, 27 Jan 2015, Kassey Li wrote:
>
>> From: Kassey Li <kasseyl@nvidia.com>
>>
>> This adds support for spi interface sub device for
>> soc_camera.
>>
>> Signed-off-by: Kassey Li <kasseyl@nvidia.com>
>> ---
>>  drivers/media/platform/soc_camera/soc_camera.c |   51 ++++++++++++++++++++++++
>>  include/media/soc_camera.h                     |   10 +++++
>>  2 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>> index b3db51c..6db2d89 100644
>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>> @@ -1599,6 +1599,49 @@ static void scan_async_host(struct soc_camera_host *ici)
>>  #define soc_camera_i2c_free(icd)     do {} while (0)
>>  #define scan_async_host(ici)         do {} while (0)
>>  #endif
>> +static int soc_camera_init_spi(struct soc_camera_device *icd,
>> +             struct soc_camera_desc *sdesc)
>
> The rest of the file _mostly_ uses a different line-breaking style, but...
> This isn't completely incompatible with the rest, so, I wouldn't insist on
> changing this, probably. Just to explain, normally in this file alignment
> is performed in theform
>
>         ret = func(a, b, c,
>                    d, e);
>
> i.e. first TABs are used and then spaces to align, say, under the first
> argument in this case. If, however, this alignment style would make the
> second line too long, then yes, additional spaces and, possibly, one or
> more TABs are removed. So, at least this kinf of alignment
>
>         ret = func(a, b, c,
>                         d, e);
>
> is never used... But, as I said, up to you, would be nice to have a
> somewhat better style compliance.
>
>> +{
>> +     struct spi_device   *spi;
>
> Please, just one space.
>
>> +     struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> +     struct soc_camera_host_desc *shd = &sdesc->host_desc;
>> +     struct spi_master *spi_master =
>> +                     spi_busnum_to_master(shd->spi_bus_id);
>
> You certainly need an additional header for this and other SPI macros,
> functions and types.
>
>> +     struct v4l2_subdev *subdev;
>> +
>> +     if (!spi_master) {
>> +             dev_err(icd->pdev, "Cannot get spi master #%d. No driver?\n",
>> +                             shd->spi_bus_id);
>
> This is the kind of alignment, that this file otherwise doesn't use. Just
> removing one TAB above would already make it look more consistent:)
>
>> +             goto espind;
>
> You can just return here.
>
>> +     }
>> +
>> +     shd->board_info_spi->platform_data = &sdesc->subdev_desc;
>> +
>> +     subdev = v4l2_spi_new_subdev(&ici->v4l2_dev, spi_master,
>> +                     shd->board_info_spi);
>> +     if (!subdev)
>> +             goto espind;
>
> Comparing to the I2C subdevice initialisation, this version is lacking
> regulator and clock support... Is it not needed?
>
>> +
>> +     spi = v4l2_get_subdevdata(subdev);
>> +
>> +     /* Use to_i2c_client(dev) to recover the i2c client */
>> +     icd->control = &spi->dev;
>> +
>> +     return 0;
>> +espind:
>> +     return -ENODEV;
>> +}
>> +
>> +static void soc_camera_free_spi(struct soc_camera_device *icd)
>> +{
>> +     struct spi_device   *spi;
>> +     struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> +
>> +     spi = v4l2_get_subdevdata(sd);
>> +     icd->control = NULL;
>> +     v4l2_device_unregister_subdev(sd);
>> +     spi_unregister_device(spi);
>> +}
>>
>>  #ifdef CONFIG_OF
>>
>> @@ -1762,6 +1805,10 @@ static int soc_camera_probe(struct soc_camera_host *ici,
>>               ret = soc_camera_i2c_init(icd, sdesc);
>>               if (ret < 0 && ret != -EPROBE_DEFER)
>>                       goto eadd;
>> +     } else if (shd->board_info_spi) {
>> +               ret = soc_camera_init_spi(icd, sdesc);
>> +               if (ret < 0)
>> +                       goto eadd;
>>       } else if (!shd->add_device || !shd->del_device) {
>>               ret = -EINVAL;
>>               goto eadd;
>> @@ -1803,6 +1850,8 @@ static int soc_camera_probe(struct soc_camera_host *ici,
>>  efinish:
>>       if (shd->board_info) {
>>               soc_camera_i2c_free(icd);
>> +     } else if (shd->board_info_spi) {
>> +             soc_camera_free_spi(icd);
>>       } else {
>>               shd->del_device(icd);
>>               module_put(control->driver->owner);
>> @@ -1843,6 +1892,8 @@ static int soc_camera_remove(struct soc_camera_device *icd)
>>
>>       if (sdesc->host_desc.board_info) {
>>               soc_camera_i2c_free(icd);
>> +     } else if (sdesc->host_desc.board_info_spi) {
>> +             soc_camera_free_spi(icd);
>>       } else {
>>               struct device *dev = to_soc_camera_control(icd);
>>               struct device_driver *drv = dev ? dev->driver : NULL;
>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
>> index 2f6261f..7530893 100644
>> --- a/include/media/soc_camera.h
>> +++ b/include/media/soc_camera.h
>> @@ -178,6 +178,11 @@ struct soc_camera_host_desc {
>>       int i2c_adapter_id;
>>       struct i2c_board_info *board_info;
>>       const char *module_name;
>> +     /*
>> +      * Add SPI device support.
>> +      */
>
> This comment documents an action of "adding" SPI support, which isn't
> interesting in the long run. It doesn't provide any additional information
> about the code. Please, remove.
>
>> +     struct spi_board_info *board_info_spi;
>> +     int spi_bus_id;
>>
>>       /*
>>        * For non-I2C devices platform has to provide methods to add a device
>> @@ -243,6 +248,11 @@ struct soc_camera_link {
>>       int i2c_adapter_id;
>>       struct i2c_board_info *board_info;
>>       const char *module_name;
>> +     /*
>> +      * Add SPI device support.
>> +      */
>
> Ditto, just remove it.
>
>> +     struct spi_board_info *board_info_spi;
>> +     int spi_bus_id;
>
> [OT] This reminds me... This struct has to be removed...
>
>>
>>       /*
>>        * For non-I2C devices platform has to provide methods to add a device
>> --
>> 1.7.9.5
>>
>
> Thanks
> Guennadi



-- 
Best regards
Kassey