[v9,8/8] exynos4-is: Add support for v4l2-flash subdevs

  This patch adds support for external v4l2-flash devices.
The support includes parsing "camera-flashes" DT property
and asynchronous sub-device 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 |   64 ++++++++++++++++++++++++-
 drivers/media/platform/exynos4-is/media-dev.h |    4 ++
 2 files changed, 66 insertions(+), 2 deletions(-)
  

On 25/05/15 17:14, Jacek Anaszewski wrote:
> This patch adds support for external v4l2-flash devices.
> The support includes parsing "camera-flashes" DT property

"samsung,camera-flashes" ?

> and asynchronous sub-device registration.

> +static int fimc_md_register_flash_entities(struct fimc_md *fmd)
> +{
> +	struct device_node *parent = fmd->pdev->dev.of_node, *np_sensor,
> +		*np_flash;
> +	struct v4l2_async_notifier *notifier = &fmd->subdev_notifier;
> +	struct v4l2_async_subdev *asd;
> +	int i, j, num_flashes = 0, num_elems;
> +
> +	num_elems = of_property_count_elems_of_size(parent,
> +				"samsung,camera-flashes", sizeof(np_flash));

I think this should be of_property_count_u32_elems(), phandle is always
a 32-bit value [1], while size of a pointer depends on the architecture.


> +	/* samsung,camera-flashes property is optional */
> +	if (num_elems < 0)
> +		return 0;
> +
> +	/* samsung,camera-flashes array must have even number of elements */
> +	if ((num_elems & 1) || (num_elems > FIMC_MAX_SENSORS * 2))
> +		return -EINVAL;
> +
> +	for (i = 0; i < num_elems; i += 2) {
> +		np_sensor = of_parse_phandle(parent,
> +					     "samsung,camera-flashes", i);
> +
> +		for (j = 0; j < fmd->num_sensors; j++)
> +			if (fmd->async_subdevs.sensors[j].match.of.node ==
> +			    np_sensor)
> +				break;
> +
> +		of_node_put(np_sensor);

Would be good to add some comment here, why is the sensor required.
It's just a DT correctness check? Couldn't we carry on with the flash
registration after just emitting some warning?

> +		if (j == fmd->num_sensors)
> +			continue;
> +
> +		np_flash = of_parse_phandle(parent, "samsung,camera-flashes",
> +						i + 1);
> +
> +		asd = &fmd->async_subdevs.flashes[num_flashes++];
> +		asd->match_type = V4L2_ASYNC_MATCH_OF;
> +		asd->match.of.node = np_flash;
> +		notifier->subdevs[notifier->num_subdevs++] = asd;
> +
> +		of_node_put(np_flash);
> +	}
> +
> +	return 0;
> +}

Otherwise looks good to me.
  

Hi Sylwester,

Thanks for the review.

On 05/26/2015 11:04 AM, Sylwester Nawrocki wrote:
> On 25/05/15 17:14, Jacek Anaszewski wrote:
>> This patch adds support for external v4l2-flash devices.
>> The support includes parsing "camera-flashes" DT property
>
> "samsung,camera-flashes" ?

Right.

>> and asynchronous sub-device registration.
>
>> +static int fimc_md_register_flash_entities(struct fimc_md *fmd)
>> +{
>> +	struct device_node *parent = fmd->pdev->dev.of_node, *np_sensor,
>> +		*np_flash;
>> +	struct v4l2_async_notifier *notifier = &fmd->subdev_notifier;
>> +	struct v4l2_async_subdev *asd;
>> +	int i, j, num_flashes = 0, num_elems;
>> +
>> +	num_elems = of_property_count_elems_of_size(parent,
>> +				"samsung,camera-flashes", sizeof(np_flash));
>
> I think this should be of_property_count_u32_elems(), phandle is always
> a 32-bit value [1], while size of a pointer depends on the architecture.

Thanks for spotting this.

>
>> +	/* samsung,camera-flashes property is optional */
>> +	if (num_elems < 0)
>> +		return 0;
>> +
>> +	/* samsung,camera-flashes array must have even number of elements */
>> +	if ((num_elems & 1) || (num_elems > FIMC_MAX_SENSORS * 2))
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < num_elems; i += 2) {
>> +		np_sensor = of_parse_phandle(parent,
>> +					     "samsung,camera-flashes", i);
>> +
>> +		for (j = 0; j < fmd->num_sensors; j++)
>> +			if (fmd->async_subdevs.sensors[j].match.of.node ==
>> +			    np_sensor)
>> +				break;
>> +
>> +		of_node_put(np_sensor);
>
> Would be good to add some comment here, why is the sensor required.
> It's just a DT correctness check?

Yes, it checks whether the phandle points to the sensor node
which was previously registered.

> Couldn't we carry on with the flash
> registration after just emitting some warning?

Hmm, I've just realized that with this code the flash phandle associated
with the sensor phandle that hasn't been previously registered would be
silently ignored.

I agree that we should register the flash and emit warning in case
sensor phandle doesn't point the known sensor.

>> +		if (j == fmd->num_sensors)
>> +			continue;
>> +
>> +		np_flash = of_parse_phandle(parent, "samsung,camera-flashes",
>> +						i + 1);
>> +
>> +		asd = &fmd->async_subdevs.flashes[num_flashes++];
>> +		asd->match_type = V4L2_ASYNC_MATCH_OF;
>> +		asd->match.of.node = np_flash;
>> +		notifier->subdevs[notifier->num_subdevs++] = asd;
>> +
>> +		of_node_put(np_flash);
>> +	}
>> +
>> +	return 0;
>> +}
>
> Otherwise looks good to me.
>