diff mbox

[v1,06/11] exynos4-is: Add support for v4l2-flash subdevs

Message ID 1426863811-12516-7-git-send-email-j.anaszewski@samsung.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Jacek Anaszewski March 20, 2015, 3:03 p.m. UTC 
  This patch adds support for external v4l2-flash devices.
The support includes parsing "flashes" DT property
and asynchronous subdevice registration.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/media/platform/exynos4-is/media-dev.c |   36 +++++++++++++++++++++++--
 drivers/media/platform/exynos4-is/media-dev.h |   13 ++++++++-
 2 files changed, 46 insertions(+), 3 deletions(-)

Comments

Sakari Ailus March 22, 2015, 1:21 a.m. UTC | #1 
Hi Jacek,

Some comments below. Please also get an ack from Sylwester! :-)

On Fri, Mar 20, 2015 at 04:03:26PM +0100, Jacek Anaszewski wrote:
> This patch adds support for external v4l2-flash devices.
> The support includes parsing "flashes" DT property
> and asynchronous subdevice registration.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c |   36 +++++++++++++++++++++++--
>  drivers/media/platform/exynos4-is/media-dev.h |   13 ++++++++-
>  2 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index f315ef9..8dd0e5d 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -451,6 +451,25 @@ rpm_put:
>  	return ret;
>  }
>  
> +static void fimc_md_register_flash_entities(struct fimc_md *fmd)
> +{
> +	struct device_node *parent = fmd->pdev->dev.of_node;
> +	struct device_node *np;
> +	int i = 0;
> +
> +	do {
> +		np = of_parse_phandle(parent, "flashes", i);
> +		if (np) {

if (!np)
	break;

And you can remove checking np another time in the loop condition.

> +			fmd->flash[fmd->num_flashes].asd.match_type =
> +							V4L2_ASYNC_MATCH_OF;
> +			fmd->flash[fmd->num_flashes].asd.match.of.node = np;
> +			fmd->num_flashes++;
> +			fmd->async_subdevs[fmd->num_sensors + i] =
> +						&fmd->flash[i].asd;

Have all the sensors been already registered by this point?

> +		}
> +	} while (np && (++i < FIMC_MAX_FLASHES));

How about instead:

fmd->num_flashes < FIMC_MAX_FLASHES

And drop i. Also move incrementing num_flashes as last in the if.

> +}
> +
>  static int __of_get_csis_id(struct device_node *np)
>  {
>  	u32 reg = 0;
> @@ -1275,6 +1294,15 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
>  	struct fimc_sensor_info *si = NULL;
>  	int i;
>  
> +	/* Register flash subdev if detected any */
> +	for (i = 0; i < ARRAY_SIZE(fmd->flash); i++) {
> +		if (fmd->flash[i].asd.match.of.node == subdev->dev->of_node) {
> +			fmd->flash[i].subdev = subdev;
> +			fmd->num_flashes++;
> +			return 0;
> +		}
> +	}
> +
>  	/* Find platform data for this sensor subdev */
>  	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
>  		if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
> @@ -1385,6 +1413,8 @@ static int fimc_md_probe(struct platform_device *pdev)
>  		goto err_m_ent;
>  	}
>  
> +	fimc_md_register_flash_entities(fmd);
> +
>  	mutex_unlock(&fmd->media_dev.graph_mutex);
>  
>  	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
> @@ -1401,12 +1431,14 @@ static int fimc_md_probe(struct platform_device *pdev)
>  		goto err_attr;
>  	}
>  
> -	if (fmd->num_sensors > 0) {
> +	if (fmd->num_sensors > 0 || fmd->num_flashes > 0) {
>  		fmd->subdev_notifier.subdevs = fmd->async_subdevs;
> -		fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
> +		fmd->subdev_notifier.num_subdevs = fmd->num_sensors +
> +							fmd->num_flashes;
>  		fmd->subdev_notifier.bound = subdev_notifier_bound;
>  		fmd->subdev_notifier.complete = subdev_notifier_complete;
>  		fmd->num_sensors = 0;
> +		fmd->num_flashes = 0;
>  
>  		ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
>  						&fmd->subdev_notifier);
> diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
> index 0321454..feff9c8 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.h
> +++ b/drivers/media/platform/exynos4-is/media-dev.h
> @@ -34,6 +34,8 @@
>  
>  #define FIMC_MAX_SENSORS	4
>  #define FIMC_MAX_CAMCLKS	2
> +#define FIMC_MAX_FLASHES	2
> +#define FIMC_MAX_ASYNC_SUBDEVS (FIMC_MAX_SENSORS + FIMC_MAX_FLASHES)
>  #define DEFAULT_SENSOR_CLK_FREQ	24000000U
>  
>  /* LCD/ISP Writeback clocks (PIXELASYNCMx) */
> @@ -93,6 +95,11 @@ struct fimc_sensor_info {
>  	struct fimc_dev *host;
>  };
>  
> +struct fimc_flash_info {
> +	struct v4l2_subdev *subdev;
> +	struct v4l2_async_subdev asd;
> +};
> +
>  struct cam_clk {
>  	struct clk_hw hw;
>  	struct fimc_md *fmd;
> @@ -104,6 +111,8 @@ struct cam_clk {
>   * @csis: MIPI CSIS subdevs data
>   * @sensor: array of registered sensor subdevs
>   * @num_sensors: actual number of registered sensors
> + * @flash: array of registered flash subdevs
> + * @num_flashes: actual number of registered flashes
>   * @camclk: external sensor clock information
>   * @fimc: array of registered fimc devices
>   * @fimc_is: fimc-is data structure
> @@ -123,6 +132,8 @@ struct fimc_md {
>  	struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
>  	struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
>  	int num_sensors;
> +	struct fimc_flash_info flash[FIMC_MAX_FLASHES];
> +	int num_flashes;
>  	struct fimc_camclk_info camclk[FIMC_MAX_CAMCLKS];
>  	struct clk *wbclk[FIMC_MAX_WBCLKS];
>  	struct fimc_lite *fimc_lite[FIMC_LITE_MAX_DEVS];
> @@ -149,7 +160,7 @@ struct fimc_md {
>  	} clk_provider;
>  
>  	struct v4l2_async_notifier subdev_notifier;
> -	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
> +	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_ASYNC_SUBDEVS];
>  
>  	bool user_subdev_api;
>  	spinlock_t slock;
Jacek Anaszewski March 23, 2015, 3:32 p.m. UTC | #2 
Hi Sakari,

Thanks for the review.

On 03/22/2015 02:21 AM, Sakari Ailus wrote:
> Hi Jacek,
>
> Some comments below. Please also get an ack from Sylwester! :-)

No doubt about that :)

> On Fri, Mar 20, 2015 at 04:03:26PM +0100, Jacek Anaszewski wrote:
>> This patch adds support for external v4l2-flash devices.
>> The support includes parsing "flashes" DT property
>> and asynchronous subdevice registration.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>>   drivers/media/platform/exynos4-is/media-dev.c |   36 +++++++++++++++++++++++--
>>   drivers/media/platform/exynos4-is/media-dev.h |   13 ++++++++-
>>   2 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index f315ef9..8dd0e5d 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -451,6 +451,25 @@ rpm_put:
>>   	return ret;
>>   }
>>
>> +static void fimc_md_register_flash_entities(struct fimc_md *fmd)
>> +{
>> +	struct device_node *parent = fmd->pdev->dev.of_node;
>> +	struct device_node *np;
>> +	int i = 0;
>> +
>> +	do {
>> +		np = of_parse_phandle(parent, "flashes", i);
>> +		if (np) {
>
> if (!np)
> 	break;
>
> And you can remove checking np another time in the loop condition.

Thanks, this will be cleaner indeed.

>> +			fmd->flash[fmd->num_flashes].asd.match_type =
>> +							V4L2_ASYNC_MATCH_OF;
>> +			fmd->flash[fmd->num_flashes].asd.match.of.node = np;
>> +			fmd->num_flashes++;
>> +			fmd->async_subdevs[fmd->num_sensors + i] =
>> +						&fmd->flash[i].asd;
>
> Have all the sensors been already registered by this point?

Function fimc_md_register_sensor_entities is called before
this one.

>> +		}
>> +	} while (np && (++i < FIMC_MAX_FLASHES));
>
> How about instead:
>
> fmd->num_flashes < FIMC_MAX_FLASHES
>
> And drop i. Also move incrementing num_flashes as last in the if.

Dropping i will enforce referring to fmd->num_flashes 7 times
in this short fragment of code.
Maybe it would be better to use a pointer to it?
int *nf = &fmd=>num_flashes ?

>> +}
>> +
>>   static int __of_get_csis_id(struct device_node *np)
>>   {
>>   	u32 reg = 0;
>> @@ -1275,6 +1294,15 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
>>   	struct fimc_sensor_info *si = NULL;
>>   	int i;
>>
>> +	/* Register flash subdev if detected any */
>> +	for (i = 0; i < ARRAY_SIZE(fmd->flash); i++) {
>> +		if (fmd->flash[i].asd.match.of.node == subdev->dev->of_node) {
>> +			fmd->flash[i].subdev = subdev;
>> +			fmd->num_flashes++;
>> +			return 0;
>> +		}
>> +	}
>> +
>>   	/* Find platform data for this sensor subdev */
>>   	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
>>   		if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
>> @@ -1385,6 +1413,8 @@ static int fimc_md_probe(struct platform_device *pdev)
>>   		goto err_m_ent;
>>   	}
>>
>> +	fimc_md_register_flash_entities(fmd);
>> +
>>   	mutex_unlock(&fmd->media_dev.graph_mutex);
>>
>>   	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
>> @@ -1401,12 +1431,14 @@ static int fimc_md_probe(struct platform_device *pdev)
>>   		goto err_attr;
>>   	}
>>
>> -	if (fmd->num_sensors > 0) {
>> +	if (fmd->num_sensors > 0 || fmd->num_flashes > 0) {
>>   		fmd->subdev_notifier.subdevs = fmd->async_subdevs;
>> -		fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
>> +		fmd->subdev_notifier.num_subdevs = fmd->num_sensors +
>> +							fmd->num_flashes;
>>   		fmd->subdev_notifier.bound = subdev_notifier_bound;
>>   		fmd->subdev_notifier.complete = subdev_notifier_complete;
>>   		fmd->num_sensors = 0;
>> +		fmd->num_flashes = 0;
>>
>>   		ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
>>   						&fmd->subdev_notifier);
>> diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
>> index 0321454..feff9c8 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.h
>> +++ b/drivers/media/platform/exynos4-is/media-dev.h
>> @@ -34,6 +34,8 @@
>>
>>   #define FIMC_MAX_SENSORS	4
>>   #define FIMC_MAX_CAMCLKS	2
>> +#define FIMC_MAX_FLASHES	2
>> +#define FIMC_MAX_ASYNC_SUBDEVS (FIMC_MAX_SENSORS + FIMC_MAX_FLASHES)
>>   #define DEFAULT_SENSOR_CLK_FREQ	24000000U
>>
>>   /* LCD/ISP Writeback clocks (PIXELASYNCMx) */
>> @@ -93,6 +95,11 @@ struct fimc_sensor_info {
>>   	struct fimc_dev *host;
>>   };
>>
>> +struct fimc_flash_info {
>> +	struct v4l2_subdev *subdev;
>> +	struct v4l2_async_subdev asd;
>> +};
>> +
>>   struct cam_clk {
>>   	struct clk_hw hw;
>>   	struct fimc_md *fmd;
>> @@ -104,6 +111,8 @@ struct cam_clk {
>>    * @csis: MIPI CSIS subdevs data
>>    * @sensor: array of registered sensor subdevs
>>    * @num_sensors: actual number of registered sensors
>> + * @flash: array of registered flash subdevs
>> + * @num_flashes: actual number of registered flashes
>>    * @camclk: external sensor clock information
>>    * @fimc: array of registered fimc devices
>>    * @fimc_is: fimc-is data structure
>> @@ -123,6 +132,8 @@ struct fimc_md {
>>   	struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
>>   	struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
>>   	int num_sensors;
>> +	struct fimc_flash_info flash[FIMC_MAX_FLASHES];
>> +	int num_flashes;
>>   	struct fimc_camclk_info camclk[FIMC_MAX_CAMCLKS];
>>   	struct clk *wbclk[FIMC_MAX_WBCLKS];
>>   	struct fimc_lite *fimc_lite[FIMC_LITE_MAX_DEVS];
>> @@ -149,7 +160,7 @@ struct fimc_md {
>>   	} clk_provider;
>>
>>   	struct v4l2_async_notifier subdev_notifier;
>> -	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
>> +	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_ASYNC_SUBDEVS];
>>
>>   	bool user_subdev_api;
>>   	spinlock_t slock;
>
Sakari Ailus March 23, 2015, 10:39 p.m. UTC | #3 
Hi Jacek,

On Mon, Mar 23, 2015 at 04:32:12PM +0100, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> Thanks for the review.
> 
> On 03/22/2015 02:21 AM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >Some comments below. Please also get an ack from Sylwester! :-)
> 
> No doubt about that :)
> 
> >On Fri, Mar 20, 2015 at 04:03:26PM +0100, Jacek Anaszewski wrote:
> >>This patch adds support for external v4l2-flash devices.
> >>The support includes parsing "flashes" DT property
> >>and asynchronous subdevice registration.
> >>
> >>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> >>Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>---
> >>  drivers/media/platform/exynos4-is/media-dev.c |   36 +++++++++++++++++++++++--
> >>  drivers/media/platform/exynos4-is/media-dev.h |   13 ++++++++-
> >>  2 files changed, 46 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> >>index f315ef9..8dd0e5d 100644
> >>--- a/drivers/media/platform/exynos4-is/media-dev.c
> >>+++ b/drivers/media/platform/exynos4-is/media-dev.c
> >>@@ -451,6 +451,25 @@ rpm_put:
> >>  	return ret;
> >>  }
> >>
> >>+static void fimc_md_register_flash_entities(struct fimc_md *fmd)
> >>+{
> >>+	struct device_node *parent = fmd->pdev->dev.of_node;
> >>+	struct device_node *np;
> >>+	int i = 0;
> >>+
> >>+	do {
> >>+		np = of_parse_phandle(parent, "flashes", i);
> >>+		if (np) {
> >
> >if (!np)
> >	break;
> >
> >And you can remove checking np another time in the loop condition.
> 
> Thanks, this will be cleaner indeed.
> 
> >>+			fmd->flash[fmd->num_flashes].asd.match_type =
> >>+							V4L2_ASYNC_MATCH_OF;
> >>+			fmd->flash[fmd->num_flashes].asd.match.of.node = np;
> >>+			fmd->num_flashes++;
> >>+			fmd->async_subdevs[fmd->num_sensors + i] =
> >>+						&fmd->flash[i].asd;
> >
> >Have all the sensors been already registered by this point?
> 
> Function fimc_md_register_sensor_entities is called before
> this one.

Ok. Then it's fine. I just thing this would be much cleaner if there was no
assumption that fmd->num_flashes is necessarily zero (and all sensors have
been registered).

> >>+		}
> >>+	} while (np && (++i < FIMC_MAX_FLASHES));
> >
> >How about instead:
> >
> >fmd->num_flashes < FIMC_MAX_FLASHES
> >
> >And drop i. Also move incrementing num_flashes as last in the if.
> 
> Dropping i will enforce referring to fmd->num_flashes 7 times
> in this short fragment of code.
> Maybe it would be better to use a pointer to it?
> int *nf = &fmd=>num_flashes ?

You could also do

const int nf = fmd->num_flashes;

in the beginning of the loop.

Up to you. Either is IMO better than an unrelated counter variable i. :-)

> >>+}
> >>+
> >>  static int __of_get_csis_id(struct device_node *np)
> >>  {
> >>  	u32 reg = 0;
> >>@@ -1275,6 +1294,15 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> >>  	struct fimc_sensor_info *si = NULL;
> >>  	int i;
> >>
> >>+	/* Register flash subdev if detected any */
> >>+	for (i = 0; i < ARRAY_SIZE(fmd->flash); i++) {
> >>+		if (fmd->flash[i].asd.match.of.node == subdev->dev->of_node) {
> >>+			fmd->flash[i].subdev = subdev;
> >>+			fmd->num_flashes++;
> >>+			return 0;
> >>+		}
> >>+	}
> >>+
> >>  	/* Find platform data for this sensor subdev */
> >>  	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> >>  		if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
> >>@@ -1385,6 +1413,8 @@ static int fimc_md_probe(struct platform_device *pdev)
> >>  		goto err_m_ent;
> >>  	}
> >>
> >>+	fimc_md_register_flash_entities(fmd);
> >>+
> >>  	mutex_unlock(&fmd->media_dev.graph_mutex);
> >>
> >>  	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
> >>@@ -1401,12 +1431,14 @@ static int fimc_md_probe(struct platform_device *pdev)
> >>  		goto err_attr;
> >>  	}
> >>
> >>-	if (fmd->num_sensors > 0) {
> >>+	if (fmd->num_sensors > 0 || fmd->num_flashes > 0) {
> >>  		fmd->subdev_notifier.subdevs = fmd->async_subdevs;
> >>-		fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
> >>+		fmd->subdev_notifier.num_subdevs = fmd->num_sensors +
> >>+							fmd->num_flashes;
> >>  		fmd->subdev_notifier.bound = subdev_notifier_bound;
> >>  		fmd->subdev_notifier.complete = subdev_notifier_complete;
> >>  		fmd->num_sensors = 0;
> >>+		fmd->num_flashes = 0;
> >>
> >>  		ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
> >>  						&fmd->subdev_notifier);
> >>diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
> >>index 0321454..feff9c8 100644
> >>--- a/drivers/media/platform/exynos4-is/media-dev.h
> >>+++ b/drivers/media/platform/exynos4-is/media-dev.h
> >>@@ -34,6 +34,8 @@
> >>
> >>  #define FIMC_MAX_SENSORS	4
> >>  #define FIMC_MAX_CAMCLKS	2
> >>+#define FIMC_MAX_FLASHES	2
> >>+#define FIMC_MAX_ASYNC_SUBDEVS (FIMC_MAX_SENSORS + FIMC_MAX_FLASHES)
> >>  #define DEFAULT_SENSOR_CLK_FREQ	24000000U
> >>
> >>  /* LCD/ISP Writeback clocks (PIXELASYNCMx) */
> >>@@ -93,6 +95,11 @@ struct fimc_sensor_info {
> >>  	struct fimc_dev *host;
> >>  };
> >>
> >>+struct fimc_flash_info {
> >>+	struct v4l2_subdev *subdev;
> >>+	struct v4l2_async_subdev asd;
> >>+};
> >>+
> >>  struct cam_clk {
> >>  	struct clk_hw hw;
> >>  	struct fimc_md *fmd;
> >>@@ -104,6 +111,8 @@ struct cam_clk {
> >>   * @csis: MIPI CSIS subdevs data
> >>   * @sensor: array of registered sensor subdevs
> >>   * @num_sensors: actual number of registered sensors
> >>+ * @flash: array of registered flash subdevs
> >>+ * @num_flashes: actual number of registered flashes
> >>   * @camclk: external sensor clock information
> >>   * @fimc: array of registered fimc devices
> >>   * @fimc_is: fimc-is data structure
> >>@@ -123,6 +132,8 @@ struct fimc_md {
> >>  	struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
> >>  	struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
> >>  	int num_sensors;
> >>+	struct fimc_flash_info flash[FIMC_MAX_FLASHES];
> >>+	int num_flashes;
> >>  	struct fimc_camclk_info camclk[FIMC_MAX_CAMCLKS];
> >>  	struct clk *wbclk[FIMC_MAX_WBCLKS];
> >>  	struct fimc_lite *fimc_lite[FIMC_LITE_MAX_DEVS];
> >>@@ -149,7 +160,7 @@ struct fimc_md {
> >>  	} clk_provider;
> >>
> >>  	struct v4l2_async_notifier subdev_notifier;
> >>-	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
> >>+	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_ASYNC_SUBDEVS];
> >>
> >>  	bool user_subdev_api;
> >>  	spinlock_t slock;
> >
> 
>
Jacek Anaszewski March 24, 2015, 8:52 a.m. UTC | #4 
Hi Sakari,

On 03/23/2015 11:39 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Mon, Mar 23, 2015 at 04:32:12PM +0100, Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> Thanks for the review.
>>
>> On 03/22/2015 02:21 AM, Sakari Ailus wrote:
>>> Hi Jacek,
>>>
>>> Some comments below. Please also get an ack from Sylwester! :-)
>>
>> No doubt about that :)
>>
>>> On Fri, Mar 20, 2015 at 04:03:26PM +0100, Jacek Anaszewski wrote:
>>>> This patch adds support for external v4l2-flash devices.
>>>> The support includes parsing "flashes" DT property
>>>> and asynchronous subdevice registration.
>>>>
>>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>> ---
>>>>   drivers/media/platform/exynos4-is/media-dev.c |   36 +++++++++++++++++++++++--
>>>>   drivers/media/platform/exynos4-is/media-dev.h |   13 ++++++++-
>>>>   2 files changed, 46 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>>>> index f315ef9..8dd0e5d 100644
>>>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>>>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>>>> @@ -451,6 +451,25 @@ rpm_put:
>>>>   	return ret;
>>>>   }
>>>>
>>>> +static void fimc_md_register_flash_entities(struct fimc_md *fmd)
>>>> +{
>>>> +	struct device_node *parent = fmd->pdev->dev.of_node;
>>>> +	struct device_node *np;
>>>> +	int i = 0;
>>>> +
>>>> +	do {
>>>> +		np = of_parse_phandle(parent, "flashes", i);
>>>> +		if (np) {
>>>
>>> if (!np)
>>> 	break;
>>>
>>> And you can remove checking np another time in the loop condition.
>>
>> Thanks, this will be cleaner indeed.
>>
>>>> +			fmd->flash[fmd->num_flashes].asd.match_type =
>>>> +							V4L2_ASYNC_MATCH_OF;
>>>> +			fmd->flash[fmd->num_flashes].asd.match.of.node = np;
>>>> +			fmd->num_flashes++;
>>>> +			fmd->async_subdevs[fmd->num_sensors + i] =
>>>> +						&fmd->flash[i].asd;
>>>
>>> Have all the sensors been already registered by this point?
>>
>> Function fimc_md_register_sensor_entities is called before
>> this one.
>
> Ok. Then it's fine. I just thing this would be much cleaner if there was no
> assumption that fmd->num_flashes is necessarily zero (and all sensors have
> been registered).

Do you think this should be approached differently? I don't catch your
point here, could you be more specific? :)

>>>> +		}
>>>> +	} while (np && (++i < FIMC_MAX_FLASHES));
>>>
>>> How about instead:
>>>
>>> fmd->num_flashes < FIMC_MAX_FLASHES
>>>
>>> And drop i. Also move incrementing num_flashes as last in the if.
>>
>> Dropping i will enforce referring to fmd->num_flashes 7 times
>> in this short fragment of code.
>> Maybe it would be better to use a pointer to it?
>> int *nf = &fmd=>num_flashes ?
>
> You could also do
>
> const int nf = fmd->num_flashes;

fmd->num_flashes is incremented in the loop, so the constant
will not work here. There is a tradeoff - counter variable
or many references to the fmd->num_flashes.

>
> in the beginning of the loop.
>
> Up to you. Either is IMO better than an unrelated counter variable i. :-)
>
>>>> +}
>>>> +
>>>>   static int __of_get_csis_id(struct device_node *np)
>>>>   {
>>>>   	u32 reg = 0;
>>>> @@ -1275,6 +1294,15 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
>>>>   	struct fimc_sensor_info *si = NULL;
>>>>   	int i;
>>>>
>>>> +	/* Register flash subdev if detected any */
>>>> +	for (i = 0; i < ARRAY_SIZE(fmd->flash); i++) {
>>>> +		if (fmd->flash[i].asd.match.of.node == subdev->dev->of_node) {
>>>> +			fmd->flash[i].subdev = subdev;
>>>> +			fmd->num_flashes++;
>>>> +			return 0;
>>>> +		}
>>>> +	}
>>>> +
>>>>   	/* Find platform data for this sensor subdev */
>>>>   	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
>>>>   		if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
>>>> @@ -1385,6 +1413,8 @@ static int fimc_md_probe(struct platform_device *pdev)
>>>>   		goto err_m_ent;
>>>>   	}
>>>>
>>>> +	fimc_md_register_flash_entities(fmd);
>>>> +
>>>>   	mutex_unlock(&fmd->media_dev.graph_mutex);
>>>>
>>>>   	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
>>>> @@ -1401,12 +1431,14 @@ static int fimc_md_probe(struct platform_device *pdev)
>>>>   		goto err_attr;
>>>>   	}
>>>>
>>>> -	if (fmd->num_sensors > 0) {
>>>> +	if (fmd->num_sensors > 0 || fmd->num_flashes > 0) {
>>>>   		fmd->subdev_notifier.subdevs = fmd->async_subdevs;
>>>> -		fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
>>>> +		fmd->subdev_notifier.num_subdevs = fmd->num_sensors +
>>>> +							fmd->num_flashes;
>>>>   		fmd->subdev_notifier.bound = subdev_notifier_bound;
>>>>   		fmd->subdev_notifier.complete = subdev_notifier_complete;
>>>>   		fmd->num_sensors = 0;
>>>> +		fmd->num_flashes = 0;
>>>>
>>>>   		ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
>>>>   						&fmd->subdev_notifier);
>>>> diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
>>>> index 0321454..feff9c8 100644
>>>> --- a/drivers/media/platform/exynos4-is/media-dev.h
>>>> +++ b/drivers/media/platform/exynos4-is/media-dev.h
>>>> @@ -34,6 +34,8 @@
>>>>
>>>>   #define FIMC_MAX_SENSORS	4
>>>>   #define FIMC_MAX_CAMCLKS	2
>>>> +#define FIMC_MAX_FLASHES	2
>>>> +#define FIMC_MAX_ASYNC_SUBDEVS (FIMC_MAX_SENSORS + FIMC_MAX_FLASHES)
>>>>   #define DEFAULT_SENSOR_CLK_FREQ	24000000U
>>>>
>>>>   /* LCD/ISP Writeback clocks (PIXELASYNCMx) */
>>>> @@ -93,6 +95,11 @@ struct fimc_sensor_info {
>>>>   	struct fimc_dev *host;
>>>>   };
>>>>
>>>> +struct fimc_flash_info {
>>>> +	struct v4l2_subdev *subdev;
>>>> +	struct v4l2_async_subdev asd;
>>>> +};
>>>> +
>>>>   struct cam_clk {
>>>>   	struct clk_hw hw;
>>>>   	struct fimc_md *fmd;
>>>> @@ -104,6 +111,8 @@ struct cam_clk {
>>>>    * @csis: MIPI CSIS subdevs data
>>>>    * @sensor: array of registered sensor subdevs
>>>>    * @num_sensors: actual number of registered sensors
>>>> + * @flash: array of registered flash subdevs
>>>> + * @num_flashes: actual number of registered flashes
>>>>    * @camclk: external sensor clock information
>>>>    * @fimc: array of registered fimc devices
>>>>    * @fimc_is: fimc-is data structure
>>>> @@ -123,6 +132,8 @@ struct fimc_md {
>>>>   	struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
>>>>   	struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
>>>>   	int num_sensors;
>>>> +	struct fimc_flash_info flash[FIMC_MAX_FLASHES];
>>>> +	int num_flashes;
>>>>   	struct fimc_camclk_info camclk[FIMC_MAX_CAMCLKS];
>>>>   	struct clk *wbclk[FIMC_MAX_WBCLKS];
>>>>   	struct fimc_lite *fimc_lite[FIMC_LITE_MAX_DEVS];
>>>> @@ -149,7 +160,7 @@ struct fimc_md {
>>>>   	} clk_provider;
>>>>
>>>>   	struct v4l2_async_notifier subdev_notifier;
>>>> -	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
>>>> +	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_ASYNC_SUBDEVS];
>>>>
>>>>   	bool user_subdev_api;
>>>>   	spinlock_t slock;
>>>
>>
>>
>
diff mbox

Patch

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index f315ef9..8dd0e5d 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -451,6 +451,25 @@  rpm_put:
 	return ret;
 }
 
+static void fimc_md_register_flash_entities(struct fimc_md *fmd)
+{
+	struct device_node *parent = fmd->pdev->dev.of_node;
+	struct device_node *np;
+	int i = 0;
+
+	do {
+		np = of_parse_phandle(parent, "flashes", i);
+		if (np) {
+			fmd->flash[fmd->num_flashes].asd.match_type =
+							V4L2_ASYNC_MATCH_OF;
+			fmd->flash[fmd->num_flashes].asd.match.of.node = np;
+			fmd->num_flashes++;
+			fmd->async_subdevs[fmd->num_sensors + i] =
+						&fmd->flash[i].asd;
+		}
+	} while (np && (++i < FIMC_MAX_FLASHES));
+}
+
 static int __of_get_csis_id(struct device_node *np)
 {
 	u32 reg = 0;
@@ -1275,6 +1294,15 @@  static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
 	struct fimc_sensor_info *si = NULL;
 	int i;
 
+	/* Register flash subdev if detected any */
+	for (i = 0; i < ARRAY_SIZE(fmd->flash); i++) {
+		if (fmd->flash[i].asd.match.of.node == subdev->dev->of_node) {
+			fmd->flash[i].subdev = subdev;
+			fmd->num_flashes++;
+			return 0;
+		}
+	}
+
 	/* Find platform data for this sensor subdev */
 	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
 		if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
@@ -1385,6 +1413,8 @@  static int fimc_md_probe(struct platform_device *pdev)
 		goto err_m_ent;
 	}
 
+	fimc_md_register_flash_entities(fmd);
+
 	mutex_unlock(&fmd->media_dev.graph_mutex);
 
 	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
@@ -1401,12 +1431,14 @@  static int fimc_md_probe(struct platform_device *pdev)
 		goto err_attr;
 	}
 
-	if (fmd->num_sensors > 0) {
+	if (fmd->num_sensors > 0 || fmd->num_flashes > 0) {
 		fmd->subdev_notifier.subdevs = fmd->async_subdevs;
-		fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
+		fmd->subdev_notifier.num_subdevs = fmd->num_sensors +
+							fmd->num_flashes;
 		fmd->subdev_notifier.bound = subdev_notifier_bound;
 		fmd->subdev_notifier.complete = subdev_notifier_complete;
 		fmd->num_sensors = 0;
+		fmd->num_flashes = 0;
 
 		ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
 						&fmd->subdev_notifier);
diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
index 0321454..feff9c8 100644
--- a/drivers/media/platform/exynos4-is/media-dev.h
+++ b/drivers/media/platform/exynos4-is/media-dev.h
@@ -34,6 +34,8 @@ 
 
 #define FIMC_MAX_SENSORS	4
 #define FIMC_MAX_CAMCLKS	2
+#define FIMC_MAX_FLASHES	2
+#define FIMC_MAX_ASYNC_SUBDEVS (FIMC_MAX_SENSORS + FIMC_MAX_FLASHES)
 #define DEFAULT_SENSOR_CLK_FREQ	24000000U
 
 /* LCD/ISP Writeback clocks (PIXELASYNCMx) */
@@ -93,6 +95,11 @@  struct fimc_sensor_info {
 	struct fimc_dev *host;
 };
 
+struct fimc_flash_info {
+	struct v4l2_subdev *subdev;
+	struct v4l2_async_subdev asd;
+};
+
 struct cam_clk {
 	struct clk_hw hw;
 	struct fimc_md *fmd;
@@ -104,6 +111,8 @@  struct cam_clk {
  * @csis: MIPI CSIS subdevs data
  * @sensor: array of registered sensor subdevs
  * @num_sensors: actual number of registered sensors
+ * @flash: array of registered flash subdevs
+ * @num_flashes: actual number of registered flashes
  * @camclk: external sensor clock information
  * @fimc: array of registered fimc devices
  * @fimc_is: fimc-is data structure
@@ -123,6 +132,8 @@  struct fimc_md {
 	struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
 	struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
 	int num_sensors;
+	struct fimc_flash_info flash[FIMC_MAX_FLASHES];
+	int num_flashes;
 	struct fimc_camclk_info camclk[FIMC_MAX_CAMCLKS];
 	struct clk *wbclk[FIMC_MAX_WBCLKS];
 	struct fimc_lite *fimc_lite[FIMC_LITE_MAX_DEVS];
@@ -149,7 +160,7 @@  struct fimc_md {
 	} clk_provider;
 
 	struct v4l2_async_notifier subdev_notifier;
-	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
+	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_ASYNC_SUBDEVS];
 
 	bool user_subdev_api;
 	spinlock_t slock;