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

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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.

-- 
Thanks,
Sylwester

[1]
http://lxr.free-electrons.com/source/Documentation/devicetree/booting-without-of.txt#L545