| Message ID | 1432566843-6391-9-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. >
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index ff36a3b..9fca1d4 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -455,6 +455,52 @@ rpm_put: return ret; } +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)); + /* 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); + + 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; +} + static int __of_get_csis_id(struct device_node *np) { u32 reg = 0; @@ -1280,6 +1326,15 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier, struct fimc_sensor_info *si = NULL; int i; + /* + * Flash sub-devices are controlled independently of ISP, and thus + * verify only that the sub-device being matched was previously + * registered. + */ + for (i = 0; i < ARRAY_SIZE(async_subdevs->flashes); i++) + if (&async_subdevs->flashes[i] == asd) + return 0; + /* Find platform data for this sensor subdev */ for (i = 0; i < ARRAY_SIZE(async_subdevs->sensors); i++) if (fmd->sensor[i].asd == asd) @@ -1326,13 +1381,18 @@ static int fimc_md_register_async_entities(struct fimc_md *fmd) { struct device *dev = fmd->media_dev.dev; struct v4l2_async_notifier *notifier = &fmd->subdev_notifier; + int ret; - notifier->subdevs = devm_kcalloc(dev, FIMC_MAX_SENSORS, + notifier->subdevs = devm_kcalloc(dev, FIMC_MAX_ASYNC_ENTITIES, sizeof(*notifier->subdevs), GFP_KERNEL); if (!notifier->subdevs) return -ENOMEM; - return fimc_md_register_sensor_entities(fmd); + ret = fimc_md_register_sensor_entities(fmd); + if (ret) + return -EINVAL; + + return fimc_md_register_flash_entities(fmd); } static int fimc_md_probe(struct platform_device *pdev) diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h index ff6d020..be9205c 100644 --- a/drivers/media/platform/exynos4-is/media-dev.h +++ b/drivers/media/platform/exynos4-is/media-dev.h @@ -33,7 +33,10 @@ #define PINCTRL_STATE_IDLE "idle" #define FIMC_MAX_SENSORS 4 +#define FIMC_MAX_FLASHES 4 +#define FIMC_MAX_ASYNC_ENTITIES (FIMC_MAX_SENSORS + FIMC_MAX_FLASHES) #define FIMC_MAX_CAMCLKS 2 + #define DEFAULT_SENSOR_CLK_FREQ 24000000U /* LCD/ISP Writeback clocks (PIXELASYNCMx) */ @@ -95,6 +98,7 @@ struct fimc_sensor_info { struct fimc_async_subdevs { struct v4l2_async_subdev sensors[FIMC_MAX_SENSORS]; + struct v4l2_async_subdev flashes[FIMC_MAX_FLASHES]; }; struct cam_clk {