[PATCH 1/3] [media] exynos4-is: Staticize local symbol

[PATCH 1/3] [media] exynos4-is: Staticize local symbol

[Sachin Kamat]

__fimc_is_hw_update_param is used only in this file. Make it static.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos4-is/fimc-is-param.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-is-param.c b/drivers/media/platform/exynos4-is/fimc-is-param.c
index c7e7f69..a353be0 100644
--- a/drivers/media/platform/exynos4-is/fimc-is-param.c
+++ b/drivers/media/platform/exynos4-is/fimc-is-param.c
@@ -56,7 +56,7 @@ static void __fimc_is_hw_update_param_sensor_framerate(struct fimc_is *is)
 	__hw_param_copy(dst, src);
 }
 
-int __fimc_is_hw_update_param(struct fimc_is *is, u32 offset)
+static int __fimc_is_hw_update_param(struct fimc_is *is, u32 offset)
 {
 	struct is_param_region *par = &is->is_p_region->parameter;
 	struct chain_config *cfg = &is->config[is->config_index];
-- 
1.7.9.5

[PATCH 2/3] [media] exynos4-is: Annotate unused functions

[Sachin Kamat]

__is_set_init_isp_aa and fimc_is_hw_set_tune currently do not have
any callers. However these functions may be used in the future. Hence
instead of deleting them, staticize and annotate them with __maybe_unused
flag to avoid compiler warnings.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos4-is/fimc-is-param.c |    2 +-
 drivers/media/platform/exynos4-is/fimc-is-regs.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-is-param.c b/drivers/media/platform/exynos4-is/fimc-is-param.c
index a353be0..9bf3ddd 100644
--- a/drivers/media/platform/exynos4-is/fimc-is-param.c
+++ b/drivers/media/platform/exynos4-is/fimc-is-param.c
@@ -287,7 +287,7 @@ void __is_set_sensor(struct fimc_is *is, int fps)
 	fimc_is_set_param_bit(is, PARAM_ISP_OTF_INPUT);
 }
 
-void __is_set_init_isp_aa(struct fimc_is *is)
+static void __maybe_unused __is_set_init_isp_aa(struct fimc_is *is)
 {
 	struct isp_param *isp;
 
diff --git a/drivers/media/platform/exynos4-is/fimc-is-regs.c b/drivers/media/platform/exynos4-is/fimc-is-regs.c
index 63c68ec..cf2e13a 100644
--- a/drivers/media/platform/exynos4-is/fimc-is-regs.c
+++ b/drivers/media/platform/exynos4-is/fimc-is-regs.c
@@ -96,7 +96,7 @@ int fimc_is_hw_set_param(struct fimc_is *is)
 	return 0;
 }
 
-int fimc_is_hw_set_tune(struct fimc_is *is)
+static int __maybe_unused fimc_is_hw_set_tune(struct fimc_is *is)
 {
 	fimc_is_hw_wait_intmsr0_intmsd0(is);
 
-- 
1.7.9.5

[PATCH 3/3] [media] exynos4-is: Fix potential NULL pointer dereference

[Sachin Kamat]

dev->of_node could be NULL. Hence check for the same and return before
dereferencing it in the subsequent error message.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos4-is/fimc-lite.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index 08fbfed..214bde2 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -1513,6 +1513,9 @@ static int fimc_lite_probe(struct platform_device *pdev)
 		if (of_id)
 			drv_data = (struct flite_drvdata *)of_id->data;
 		fimc->index = of_alias_get_id(dev->of_node, "fimc-lite");
+	} else {
+		dev_err(dev, "device node not found\n");
+		return -EINVAL;
 	}
 
 	if (!drv_data || fimc->index >= drv_data->num_instances ||
-- 
1.7.9.5

Re: [PATCH 3/3] [media] exynos4-is: Fix potential NULL pointer dereference

[Sylwester Nawrocki]

Hi Sachin,

On 08/02/2013 08:32 AM, Sachin Kamat wrote:
> dev->of_node could be NULL. Hence check for the same and return before
> dereferencing it in the subsequent error message.
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/media/platform/exynos4-is/fimc-lite.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
> index 08fbfed..214bde2 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -1513,6 +1513,9 @@ static int fimc_lite_probe(struct platform_device *pdev)
>  		if (of_id)
>  			drv_data = (struct flite_drvdata *)of_id->data;
>  		fimc->index = of_alias_get_id(dev->of_node, "fimc-lite");
> +	} else {
> +		dev_err(dev, "device node not found\n");
> +		return -EINVAL;
>  	}

Thanks for the patch. I would prefer to add a check at very beginning
of fimc_lite_probe() like:

	if (!dev->of_node)
		return -ENODEV;

Those devices are only used on DT platforms.

--
Regards,
Sylwester

Re: [PATCH 3/3] [media] exynos4-is: Fix potential NULL pointer dereference

[Sachin Kamat]

Hi Sylwester,

On 2 August 2013 14:15, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> Hi Sachin,
>
> On 08/02/2013 08:32 AM, Sachin Kamat wrote:
>> dev->of_node could be NULL. Hence check for the same and return before
>> dereferencing it in the subsequent error message.
>>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  drivers/media/platform/exynos4-is/fimc-lite.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
>> index 08fbfed..214bde2 100644
>> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
>> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
>> @@ -1513,6 +1513,9 @@ static int fimc_lite_probe(struct platform_device *pdev)
>>               if (of_id)
>>                       drv_data = (struct flite_drvdata *)of_id->data;
>>               fimc->index = of_alias_get_id(dev->of_node, "fimc-lite");
>> +     } else {
>> +             dev_err(dev, "device node not found\n");
>> +             return -EINVAL;
>>       }
>
> Thanks for the patch. I would prefer to add a check at very beginning
> of fimc_lite_probe() like:
>
>         if (!dev->of_node)
>                 return -ENODEV;
>
> Those devices are only used on DT platforms.

OK. Sounds good. I will re-spin this one.

-- 
With warm regards,
Sachin

Re: [PATCH 2/3] [media] exynos4-is: Annotate unused functions

[Sachin Kamat]

Hi Sylwester,

On 2 August 2013 12:02, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> __is_set_init_isp_aa and fimc_is_hw_set_tune currently do not have
> any callers. However these functions may be used in the future. Hence
> instead of deleting them, staticize and annotate them with __maybe_unused
> flag to avoid compiler warnings.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>

Thanks for applying the other 2 patches in this series. What is your
opinion about this one?
Does this look good or do you prefer to delete the code altogether?

> ---
>  drivers/media/platform/exynos4-is/fimc-is-param.c |    2 +-
>  drivers/media/platform/exynos4-is/fimc-is-regs.c  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/exynos4-is/fimc-is-param.c b/drivers/media/platform/exynos4-is/fimc-is-param.c
> index a353be0..9bf3ddd 100644
> --- a/drivers/media/platform/exynos4-is/fimc-is-param.c
> +++ b/drivers/media/platform/exynos4-is/fimc-is-param.c
> @@ -287,7 +287,7 @@ void __is_set_sensor(struct fimc_is *is, int fps)
>         fimc_is_set_param_bit(is, PARAM_ISP_OTF_INPUT);
>  }
>
> -void __is_set_init_isp_aa(struct fimc_is *is)
> +static void __maybe_unused __is_set_init_isp_aa(struct fimc_is *is)
>  {
>         struct isp_param *isp;
>
> diff --git a/drivers/media/platform/exynos4-is/fimc-is-regs.c b/drivers/media/platform/exynos4-is/fimc-is-regs.c
> index 63c68ec..cf2e13a 100644
> --- a/drivers/media/platform/exynos4-is/fimc-is-regs.c
> +++ b/drivers/media/platform/exynos4-is/fimc-is-regs.c
> @@ -96,7 +96,7 @@ int fimc_is_hw_set_param(struct fimc_is *is)
>         return 0;
>  }
>
> -int fimc_is_hw_set_tune(struct fimc_is *is)
> +static int __maybe_unused fimc_is_hw_set_tune(struct fimc_is *is)
>  {
>         fimc_is_hw_wait_intmsr0_intmsd0(is);
>
> --
> 1.7.9.5
>



-- 
With warm regards,
Sachin

Re: [PATCH 2/3] [media] exynos4-is: Annotate unused functions

[Sylwester Nawrocki]

Hi Sachin,

On 08/05/2013 07:12 AM, Sachin Kamat wrote:
> On 2 August 2013 12:02, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> > __is_set_init_isp_aa and fimc_is_hw_set_tune currently do not have
>> > any callers. However these functions may be used in the future. Hence
>> > instead of deleting them, staticize and annotate them with __maybe_unused
>> > flag to avoid compiler warnings.
>> >
>> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Thanks for applying the other 2 patches in this series. What is your
> opinion about this one?
> Does this look good or do you prefer to delete the code altogether?

Thanks for your work on this. I think it would be better to call those
functions somewhere instead, e.g. in the fimc-is initialization routine,
until there is a user interface available for this 3A control.
fimc_is_hw_set_tune() just needs a private control a think. Let me see
if I can come up with at least some intermediate patch to achieve this,
so the warnings can be eliminated. I wouldn't like to take such steps
backwards, marking those functions static an unused.

Regards,
Sylwester

Re: [PATCH 2/3] [media] exynos4-is: Annotate unused functions

[Sachin Kamat]

Hi Sylwester,

On 5 August 2013 16:35, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> Hi Sachin,
>
> On 08/05/2013 07:12 AM, Sachin Kamat wrote:
>> On 2 August 2013 12:02, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>>> > __is_set_init_isp_aa and fimc_is_hw_set_tune currently do not have
>>> > any callers. However these functions may be used in the future. Hence
>>> > instead of deleting them, staticize and annotate them with __maybe_unused
>>> > flag to avoid compiler warnings.
>>> >
>>> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> Thanks for applying the other 2 patches in this series. What is your
>> opinion about this one?
>> Does this look good or do you prefer to delete the code altogether?
>
> Thanks for your work on this. I think it would be better to call those
> functions somewhere instead, e.g. in the fimc-is initialization routine,
> until there is a user interface available for this 3A control.
> fimc_is_hw_set_tune() just needs a private control a think. Let me see
> if I can come up with at least some intermediate patch to achieve this,
> so the warnings can be eliminated. I wouldn't like to take such steps
> backwards, marking those functions static an unused.

Marking it unused is just a stop gap solution to silence unnecessary warnings.
However if you can come up with some users for these functions, then
that would be great
and right thing to do.

-- 
With warm regards,
Sachin