Hi Hans,
Thank you for the review.
On Mon, Sep 30, 2013 at 6:13 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 09/27/2013 12:59 PM, Arun Kumar K wrote:
>> This patch adds the crucial hardware pipeline control for the
>> fimc-is driver. All the subdev nodes will call this pipeline
>> interfaces to reach the hardware. Responsibilities of this module
>> involves configuring and maintaining the hardware pipeline involving
>> multiple sub-ips like ISP, DRC, Scalers, ODC, 3DNR, FD etc.
>>
>> Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
>> Signed-off-by: Kilyeon Im <kilyeon.im@xxxxxxxxxxx>
>> Reviewed-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>> ---
>> .../media/platform/exynos5-is/fimc-is-pipeline.c | 1708 ++++++++++++++++++++
>> .../media/platform/exynos5-is/fimc-is-pipeline.h | 129 ++
>> 2 files changed, 1837 insertions(+)
>> create mode 100644 drivers/media/platform/exynos5-is/fimc-is-pipeline.c
>> create mode 100644 drivers/media/platform/exynos5-is/fimc-is-pipeline.h
>>
>> diff --git a/drivers/media/platform/exynos5-is/fimc-is-pipeline.c b/drivers/media/platform/exynos5-is/fimc-is-pipeline.c
>> new file mode 100644
>> index 0000000..a73d952
>> --- /dev/null
>> +++ b/drivers/media/platform/exynos5-is/fimc-is-pipeline.c
>
> <snip>
>
>> +int fimc_is_pipeline_open(struct fimc_is_pipeline *pipeline,
>> + struct fimc_is_sensor *sensor)
>> +{
>> + struct fimc_is *is = pipeline->is;
>> + struct is_region *region;
>> + unsigned long index[2] = {0};
>> + int ret;
>> +
>> + if (!sensor)
>> + return -EINVAL;
>> +
>> + mutex_lock(&pipeline->pipe_lock);
>> +
>> + if (test_bit(PIPELINE_OPEN, &pipeline->state)) {
>> + dev_err(pipeline->dev, "Pipeline already open\n");
>> + ret = -EINVAL;
>> + goto err_exit;
>> + }
>> +
>> + pipeline->fcount = 0;
>> + pipeline->sensor = sensor;
>> +
>> + if (is->num_pipelines == 0) {
>> + /* Init memory */
>> + ret = fimc_is_pipeline_initmem(pipeline);
>> + if (ret) {
>> + dev_err(pipeline->dev, "Pipeline memory init failed\n");
>> + goto err_exit;
>> + }
>> +
>> + /* Load firmware */
>> + ret = fimc_is_pipeline_load_firmware(pipeline);
>> + if (ret) {
>> + dev_err(pipeline->dev, "Firmware load failed\n");
>> + goto err_fw;
>> + }
>> +
>> + /* Power ON */
>> + ret = fimc_is_pipeline_power(pipeline, 1);
>> + if (ret) {
>> + dev_err(pipeline->dev, "A5 power on failed\n");
>> + goto err_fw;
>> + }
>> +
>> + /* Wait for FW Init to complete */
>> + ret = fimc_is_itf_wait_init_state(&is->interface);
>> + if (ret) {
>> + dev_err(pipeline->dev, "FW init failed\n");
>> + goto err_fw;
>> + }
>> + }
>> +
>> + /* Open Sensor */
>> + region = pipeline->is_region;
>> + ret = fimc_is_itf_open_sensor(&is->interface,
>> + pipeline->instance,
>> + sensor->drvdata->id,
>> + sensor->i2c_bus,
>> + pipeline->minfo->shared.paddr);
>> + if (ret) {
>> + dev_err(pipeline->dev, "Open sensor failed\n");
>> + goto err_exit;
>> + }
>> +
>> + /* Load setfile */
>> + ret = fimc_is_pipeline_setfile(pipeline);
>> + if (ret)
>> + goto err_exit;
>> +
>> + /* Stream off */
>> + ret = fimc_is_itf_stream_off(&is->interface, pipeline->instance);
>> + if (ret)
>> + goto err_exit;
>> +
>> + /* Process off */
>> + ret = fimc_is_itf_process_off(&is->interface, pipeline->instance);
>> + if (ret)
>> + goto err_exit;
>> +
>> + if (is->num_pipelines == 0) {
>> + /* Copy init params to FW region */
>> + memset(®ion->parameter, 0x0, sizeof(struct is_param_region));
>> +
>> + memcpy(®ion->parameter.sensor, &init_sensor_param,
>> + sizeof(struct sensor_param));
>
> How about:
>
> region->parameter.sensor = init_sensor_param;
>
> Shorter and type-safe.
>
> Ditto for the memcpy's below.
>
Yes this would make nicer code.
Will make this change.
Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
