[PATCH 3/3] staging: atomisp: move mipi_info assignment to next line in __get_asd_from_port()

[Daeseok Youn]

The line which is initializing mipi_info variable is too long
to read. It would be placed in next line.

Signed-off-by: Daeseok Youn 
---
This series of patches are related to previous patches:
[1] https://lkml.org/lkml/2017/3/27/159
[2] https://lkml.org/lkml/2017/3/30/1068
[3] https://lkml.org/lkml/2017/3/30/1069

 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 4af76b5..2208477 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -532,9 +532,11 @@ static void clear_irq_reg(struct atomisp_device *isp)
/* Check which isp subdev to send eof */
for (i = 0; i < isp->num_of_streams; i++) {
struct atomisp_sub_device *asd = &isp->asd[i];
-   struct camera_mipi_info *mipi_info =
-   atomisp_to_sensor_mipi_info(
-   isp->inputs[asd->input_curr].camera);
+   struct camera_mipi_info *mipi_info;
+
+   mipi_info = atomisp_to_sensor_mipi_info(
+   isp->inputs[asd->input_curr].camera);
+
if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED &&
__get_mipi_port(isp, mipi_info->port) == port) {
return asd;
-- 
1.9.1

[PATCH 2/3] staging: atomisp: replace "&isp->asd[i]" with "asd" in __get_asd_from_port()

[Daeseok Youn]

The address of isp->asd[i] is already assigned to
local "asd" variable. "&isp->asd[i]" would be replaced with
just "asd".

Signed-off-by: Daeseok Youn 
---
This series of patches are related to previous patches:
[1] https://lkml.org/lkml/2017/3/27/159
[2] https://lkml.org/lkml/2017/3/30/1068
[3] https://lkml.org/lkml/2017/3/30/1069

 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index c3d0596..4af76b5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -535,9 +535,9 @@ static void clear_irq_reg(struct atomisp_device *isp)
struct camera_mipi_info *mipi_info =
atomisp_to_sensor_mipi_info(
isp->inputs[asd->input_curr].camera);
-   if (isp->asd[i].streaming == ATOMISP_DEVICE_STREAMING_ENABLED &&
+   if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED &&
__get_mipi_port(isp, mipi_info->port) == port) {
-   return &isp->asd[i];
+   return asd;
}
}
 
-- 
1.9.1

[PATCH 1/3] staging: atomisp: remove enable_isp_irq function and add disable_isp_irq

[Daeseok Youn]

Enable/Disable ISP irq is switched with "enable" parameter of
enable_isp_irq(). It would be better splited to two such as
enable_isp_irq()/disable_isp_irq().

But the enable_isp_irq() is no use in atomisp_cmd.c file.
So remove the enable_isp_irq() function and add
disable_isp_irq function only.

Signed-off-by: Daeseok Youn 
---
This series of patches are related to previous patches:
[1] https://lkml.org/lkml/2017/3/27/159
[2] https://lkml.org/lkml/2017/3/30/1068
[3] https://lkml.org/lkml/2017/3/30/1069

 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 36 ++
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 0ba5d8b..c3d0596 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -375,34 +375,16 @@ int atomisp_reset(struct atomisp_device *isp)
 }
 
 /*
- * interrupt enable/disable functions
+ * interrupt disable functions
  */
-static void enable_isp_irq(enum hrt_isp_css_irq irq, bool enable)
-{
-   if (enable) {
-   irq_enable_channel(IRQ0_ID, irq);
-   /*sh_css_hrt_irq_enable(irq, true, false);*/
-   switch (irq) { /*We only have sp interrupt right now*/
-   case hrt_isp_css_irq_sp:
-   /*sh_css_hrt_irq_enable_sp(true);*/
-   cnd_sp_irq_enable(SP0_ID, true);
-   break;
-   default:
-   break;
-   }
+static void disable_isp_irq(enum hrt_isp_css_irq irq)
+{
+   irq_disable_channel(IRQ0_ID, irq);
 
-   } else {
-   /*sh_css_hrt_irq_disable(irq);*/
-   irq_disable_channel(IRQ0_ID, irq);
-   switch (irq) {
-   case hrt_isp_css_irq_sp:
-   /*sh_css_hrt_irq_enable_sp(false);*/
-   cnd_sp_irq_enable(SP0_ID, false);
-   break;
-   default:
-   break;
-   }
-   }
+   if (irq != hrt_isp_css_irq_sp)
+   return;
+
+   cnd_sp_irq_enable(SP0_ID, false);
 }
 
 /*
@@ -1415,7 +1397,7 @@ static void __atomisp_css_recover(struct atomisp_device 
*isp, bool isp_timeout)
}
 
/* clear irq */
-   enable_isp_irq(hrt_isp_css_irq_sp, false);
+   disable_isp_irq(hrt_isp_css_irq_sp);
clear_isp_irq(hrt_isp_css_irq_sp);
 
/* Set the SRSE to 3 before resetting */
-- 
1.9.1

[PATCH 2/2 V2] staging: atomisp: use local variable to reduce the number of reference

[Daeseok Youn]

Define new local variable to reduce the number of reference.
The new local variable is added to save the addess of dfs
and used in atomisp_freq_scaling() function.

Signed-off-by: Daeseok Youn 
---
V2: this patch was rebased since the patch 1/2 was improved.

 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 37 --
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 87224d6..9f4041a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
 {
/* FIXME! Only use subdev[0] status yet */
struct atomisp_sub_device *asd = &isp->asd[0];
+   const struct atomisp_dfs_config *dfs;
unsigned int new_freq;
struct atomisp_freq_scaling_rule curr_rules;
int i, ret;
@@ -264,20 +265,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
if (ATOMISP_IS_CHT(isp) && ATOMISP_USE_YUVPP(asd))
isp->dfs = &dfs_config_cht_soc;
 
-   if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
-   isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
-   !isp->dfs->dfs_table) {
+   dfs = isp->dfs;
+
+   if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
+   dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
+   !dfs->dfs_table) {
dev_err(isp->dev, "DFS configuration is invalid.\n");
return -EINVAL;
}
 
if (mode == ATOMISP_DFS_MODE_LOW) {
-   new_freq = isp->dfs->lowest_freq;
+   new_freq = dfs->lowest_freq;
goto done;
}
 
if (mode == ATOMISP_DFS_MODE_MAX) {
-   new_freq = isp->dfs->highest_freq;
+   new_freq = dfs->highest_freq;
goto done;
}
 
@@ -303,26 +306,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
}
 
/* search for the target frequency by looping freq rules*/
-   for (i = 0; i < isp->dfs->dfs_table_size; i++) {
-   if (curr_rules.width != isp->dfs->dfs_table[i].width &&
-   isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
+   for (i = 0; i < dfs->dfs_table_size; i++) {
+   if (curr_rules.width != dfs->dfs_table[i].width &&
+   dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
continue;
-   if (curr_rules.height != isp->dfs->dfs_table[i].height &&
-   isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
+   if (curr_rules.height != dfs->dfs_table[i].height &&
+   dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
continue;
-   if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
-   isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
+   if (curr_rules.fps != dfs->dfs_table[i].fps &&
+   dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
continue;
-   if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
-   isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
+   if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
+   dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
continue;
break;
}
 
-   if (i == isp->dfs->dfs_table_size)
-   new_freq = isp->dfs->max_freq_at_vmin;
+   if (i == dfs->dfs_table_size)
+   new_freq = dfs->max_freq_at_vmin;
else
-   new_freq = isp->dfs->dfs_table[i].isp_freq;
+   new_freq = dfs->dfs_table[i].isp_freq;
 
 done:
dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);
-- 
2.7.4

[PATCH 1/2 V2] staging: atomisp: simplify the if condition in atomisp_freq_scaling()

[Daeseok Youn]

The condition line in if-statement is needed to be shorthen to
improve readability.

Add a new definition to check the CHT with atomisp_device structure.

Signed-off-by: Daeseok Youn 
---
V2: replace the assigment line with macro to check CHT type.

 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c  | 3 +--
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h | 4 
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 94bc793..87224d6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -261,8 +261,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
return -EINVAL;
}
 
-   if ((isp->pdev->device & ATOMISP_PCI_DEVICE_SOC_MASK) ==
-   ATOMISP_PCI_DEVICE_SOC_CHT && ATOMISP_USE_YUVPP(asd))
+   if (ATOMISP_IS_CHT(isp) && ATOMISP_USE_YUVPP(asd))
isp->dfs = &dfs_config_cht_soc;
 
if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
index d366713..97dc5f88 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
@@ -72,6 +72,10 @@
 #define ATOMISP_PCI_DEVICE_SOC_ANN 0x1478
 #define ATOMISP_PCI_DEVICE_SOC_CHT 0x22b8
 
+#define ATOMISP_IS_CHT(isp) \
+   (((isp)->pdev->device & ATOMISP_PCI_DEVICE_SOC_MASK) == \
+   ATOMISP_PCI_DEVICE_SOC_CHT)
+
 #define ATOMISP_PCI_REV_MRFLD_A0_MAX   0
 #define ATOMISP_PCI_REV_BYT_A0_MAX 4
 
-- 
2.7.4

Re: [PATCH 1/2] staging: atomisp: simplify the if condition in atomisp_freq_scaling()

[DaeSeok Youn]

2017-03-30 19:52 GMT+09:00 Alan Cox :
> On Thu, 2017-03-30 at 15:24 +0900, Daeseok Youn wrote:
>> The condition line in if-statement is needed to be shorthen to
>> improve readability.
>>
>> Signed-off-by: Daeseok Youn 
>> ---
>
> How about a define for ATOMISP_IS_CHT(isp) instead - as we will need
hmm.. I think there is another way to get a *device*(unsigned short or
__u32) to mask with "ATOMISP_PCI_DEVICE_SOC_MASK".
In the atomisp_freq_scaling() function, the "device" value is getting
started from "isp" structure.
(isp->pdev->device)

if the function has only "pci_dev" struction as a parameter and it
need to check the CHT. Then we cannot use the definition like
ATOMISP_IS_CHT(isp). it means we have another definition to check the
CHT.

Am I right?

> these tests in other places where there are ISP2400/ISP2401 ifdefs ?
I am not sure whether these tests are needed in other place or not.
(Actually, I didn't find good H/W reference for Atom ISP device - Can
you please share the link to refer document like H/W manual to
develop?) I have tried to clean up the code first. in the meantime, I
will have a look at the document if I have good reference manual.

Thanks.
Regards,
Daeseok.
>
> Alan
>

Re: [PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

[DaeSeok Youn]

2017-03-30 16:19 GMT+09:00 walter harms :
>
>
> Am 30.03.2017 08:25, schrieb Daeseok Youn:
>> Define new local variable to reduce the number of reference.
>> The new local variable is added to save the addess of dfs
>> and used in atomisp_freq_scaling() function.
>>
>> Signed-off-by: Daeseok Youn 
>> ---
>>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 37 
>> --
>>  1 file changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
>> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> index eebfccd..d76a95c 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> @@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>>  {
>>   /* FIXME! Only use subdev[0] status yet */
>>   struct atomisp_sub_device *asd = &isp->asd[0];
>> + const struct atomisp_dfs_config *dfs;
>>   unsigned int new_freq;
>>   struct atomisp_freq_scaling_rule curr_rules;
>>   int i, ret;
>> @@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>>   ATOMISP_USE_YUVPP(asd))
>>   isp->dfs = &dfs_config_cht_soc;
>>
>> - if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
>> - isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
>> - !isp->dfs->dfs_table) {
>> + dfs = isp->dfs;
>> +
>> + if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
>> + dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
>> + !dfs->dfs_table) {
>>   dev_err(isp->dev, "DFS configuration is invalid.\n");
>>   return -EINVAL;
>>   }
>>
>>   if (mode == ATOMISP_DFS_MODE_LOW) {
>> - new_freq = isp->dfs->lowest_freq;
>> + new_freq = dfs->lowest_freq;
>>   goto done;
>>   }
>>
>>   if (mode == ATOMISP_DFS_MODE_MAX) {
>> - new_freq = isp->dfs->highest_freq;
>> + new_freq = dfs->highest_freq;
>>   goto done;
>>   }
>>
>> @@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>>   }
>>
>>   /* search for the target frequency by looping freq rules*/
>> - for (i = 0; i < isp->dfs->dfs_table_size; i++) {
>> - if (curr_rules.width != isp->dfs->dfs_table[i].width &&
>> - isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
>> + for (i = 0; i < dfs->dfs_table_size; i++) {
>> + if (curr_rules.width != dfs->dfs_table[i].width &&
>> + dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
>>   continue;
>> - if (curr_rules.height != isp->dfs->dfs_table[i].height &&
>> - isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
>> + if (curr_rules.height != dfs->dfs_table[i].height &&
>> + dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
>>   continue;
>> - if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
>> - isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
>> + if (curr_rules.fps != dfs->dfs_table[i].fps &&
>> + dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
>>   continue;
>> - if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
>> - isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
>> + if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
>> + dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
>>   continue;
>>   break;
>>   }
>
>>
>> - if (i == isp->dfs->dfs_table_size)
>> - new_freq = isp->dfs->max_freq_at_vmin;
>> + if (i == dfs->dfs_table_size)
>> + new_freq = dfs->max_freq_at_vmin;
>>   else
>> - new_freq = isp->dfs->dfs_table[i].isp_freq;
>> + new_freq = dfs->dfs_table[i].isp_freq;
>>
>
> you can eliminate the last block by setting
>
>  new_freq = dfs->max_freq_at_vmin;
>
>   for(i=0;) {
> 
> new_freq = dfs->dfs_table[i].isp_freq;
> break;
> }
Yes, it could be. I will make another patch to improve it as your comment.
>
> unfortunately i have no good idea how to make the loop more readable.
I am not sure whether the for-loop is possible to improve for
readability or not. :-)

Thanks for comment.
Regards,
Daeseok.
>
>
> re,
>  wh
>
>
>>  done:
>>   dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);

[PATCH 1/2] staging: atomisp: simplify the if condition in atomisp_freq_scaling()

[Daeseok Youn]

The condition line in if-statement is needed to be shorthen to
improve readability.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 94bc793..eebfccd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -255,14 +255,17 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
struct atomisp_freq_scaling_rule curr_rules;
int i, ret;
unsigned short fps = 0;
+   unsigned short masked_dev = 0;
 
if (isp->sw_contex.power_state != ATOM_ISP_POWER_UP) {
dev_err(isp->dev, "DFS cannot proceed due to no power.\n");
return -EINVAL;
}
 
-   if ((isp->pdev->device & ATOMISP_PCI_DEVICE_SOC_MASK) ==
-   ATOMISP_PCI_DEVICE_SOC_CHT && ATOMISP_USE_YUVPP(asd))
+   masked_dev = isp->pdev->device & ATOMISP_PCI_DEVICE_SOC_MASK;
+
+   if (masked_dev == ATOMISP_PCI_DEVICE_SOC_CHT &&
+   ATOMISP_USE_YUVPP(asd))
isp->dfs = &dfs_config_cht_soc;
 
if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
-- 
1.9.1

[PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

[Daeseok Youn]

Define new local variable to reduce the number of reference.
The new local variable is added to save the addess of dfs
and used in atomisp_freq_scaling() function.

Signed-off-by: Daeseok Youn 
---
 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 37 --
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index eebfccd..d76a95c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
 {
/* FIXME! Only use subdev[0] status yet */
struct atomisp_sub_device *asd = &isp->asd[0];
+   const struct atomisp_dfs_config *dfs;
unsigned int new_freq;
struct atomisp_freq_scaling_rule curr_rules;
int i, ret;
@@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
ATOMISP_USE_YUVPP(asd))
isp->dfs = &dfs_config_cht_soc;
 
-   if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
-   isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
-   !isp->dfs->dfs_table) {
+   dfs = isp->dfs;
+
+   if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
+   dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
+   !dfs->dfs_table) {
dev_err(isp->dev, "DFS configuration is invalid.\n");
return -EINVAL;
}
 
if (mode == ATOMISP_DFS_MODE_LOW) {
-   new_freq = isp->dfs->lowest_freq;
+   new_freq = dfs->lowest_freq;
goto done;
}
 
if (mode == ATOMISP_DFS_MODE_MAX) {
-   new_freq = isp->dfs->highest_freq;
+   new_freq = dfs->highest_freq;
goto done;
}
 
@@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
}
 
/* search for the target frequency by looping freq rules*/
-   for (i = 0; i < isp->dfs->dfs_table_size; i++) {
-   if (curr_rules.width != isp->dfs->dfs_table[i].width &&
-   isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
+   for (i = 0; i < dfs->dfs_table_size; i++) {
+   if (curr_rules.width != dfs->dfs_table[i].width &&
+   dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
continue;
-   if (curr_rules.height != isp->dfs->dfs_table[i].height &&
-   isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
+   if (curr_rules.height != dfs->dfs_table[i].height &&
+   dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
continue;
-   if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
-   isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
+   if (curr_rules.fps != dfs->dfs_table[i].fps &&
+   dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
continue;
-   if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
-   isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
+   if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
+   dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
continue;
break;
}
 
-   if (i == isp->dfs->dfs_table_size)
-   new_freq = isp->dfs->max_freq_at_vmin;
+   if (i == dfs->dfs_table_size)
+   new_freq = dfs->max_freq_at_vmin;
else
-   new_freq = isp->dfs->dfs_table[i].isp_freq;
+   new_freq = dfs->dfs_table[i].isp_freq;
 
 done:
dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);
-- 
1.9.1

[PATCH] staging: atomisp: fix an issue timeout value for checking error

[Daeseok Youn]

The timeout variable could be zero even if the bits has expected result.
The checking expected bits again would be better instead whether
the timeout value is zero or not.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 94bc793..f2e5749 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -220,11 +220,11 @@ static int write_target_freq_to_hw(struct atomisp_device 
*isp,
timeout--;
}
 
-   if (timeout != 0)
+   if (!(isp_sspm1 & ISP_FREQ_VALID_MASK))
break;
}
 
-   if (timeout == 0) {
+   if (isp_sspm1 & ISP_FREQ_VALID_MASK) {
dev_err(isp->dev, "DFS failed due to HW error.\n");
return -EINVAL;
}
@@ -238,7 +238,7 @@ static int write_target_freq_to_hw(struct atomisp_device 
*isp,
udelay(100);
timeout--;
}
-   if (timeout == 0) {
+   if ((isp_sspm1 >> ISP_FREQ_STAT_OFFSET) != ratio) {
dev_err(isp->dev, "DFS target freq is rejected by HW.\n");
return -EINVAL;
}
-- 
1.9.1

[PATCH 4/4 V2] staging: atomisp: remove redudant condition in if-statement

[Daeseok Youn]

The V4L2_FIELD_ANY is zero, so the (!field) is same meaning
with (field == V4L2_FIELD_ANY) in if-statement.

Signed-off-by: Daeseok Youn 
---
V2: one(2/4) of this series was updated so I tried to send them again.

 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index f7c0705..943a7ae 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -5084,7 +5084,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct 
v4l2_format *f,
 
depth = get_pixel_depth(pixelformat);
 
-   if (!field || field == V4L2_FIELD_ANY)
+   if (field == V4L2_FIELD_ANY)
field = V4L2_FIELD_NONE;
else if (field != V4L2_FIELD_NONE) {
dev_err(isp->dev, "Wrong output field\n");
-- 
1.9.1

[PATCH 3/4 V2] staging: atomisp: remove useless condition in if-statements

[Daeseok Youn]

The css_pipe_id was checked with 'CSS_PIPE_ID_COPY' in previous if-
statement. In this case, if the css_pipe_id equals to 'CSS_PIPE_ID_COPY',
it could not enter the next if-statement. But the "next" if-statement
has the condition to check whether the css_pipe_id equals to
'CSS_PIPE_ID_COPY' or not. It should be removed.

Signed-off-by: Daeseok Youn 
---
V2: one(2/4) of this series was updated so I tried to send them again.

 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 811331d..f7c0705 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -910,8 +910,7 @@ static struct atomisp_video_pipe *__atomisp_get_pipe(
} else if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
/* For online video or SDV video pipe. */
if (css_pipe_id == CSS_PIPE_ID_VIDEO ||
-   css_pipe_id == CSS_PIPE_ID_COPY ||
-   css_pipe_id == CSS_PIPE_ID_YUVPP) {
+   css_pipe_id == CSS_PIPE_ID_COPY) {
if (buf_type == CSS_BUFFER_TYPE_OUTPUT_FRAME)
return &asd->video_out_video_capture;
return &asd->video_out_preview;
@@ -919,8 +918,7 @@ static struct atomisp_video_pipe *__atomisp_get_pipe(
} else if (asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW) {
/* For online preview or ZSL preview pipe. */
if (css_pipe_id == CSS_PIPE_ID_PREVIEW ||
-   css_pipe_id == CSS_PIPE_ID_COPY ||
-   css_pipe_id == CSS_PIPE_ID_YUVPP)
+   css_pipe_id == CSS_PIPE_ID_COPY)
return &asd->video_out_preview;
}
/* For capture pipe. */
-- 
1.9.1

[PATCH 2/4 V2] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[Daeseok Youn]

If v4l2_subdev_call() gets the global frame interval values,
it returned 0 and it could be checked whether numerator is zero or not.

If the numerator is not zero, the fps could be calculated in this function.
If not, it just returns 0.

Signed-off-by: Daeseok Youn 
---
V2: split error handling, the first check is for the result from 
v4l2_subdev_call(),
another check is for fi.interval.numerator > 0. it is more understandable 
and
simpler than before.

 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 24 ++
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 8bdb224..811331d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -153,21 +153,19 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
video_device *dev)
 
 static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
 {
-   struct v4l2_subdev_frame_interval frame_interval;
+   struct v4l2_subdev_frame_interval fi;
struct atomisp_device *isp = asd->isp;
-   unsigned short fps;
 
-   if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-   video, g_frame_interval, &frame_interval)) {
-   fps = 0;
-   } else {
-   if (frame_interval.interval.numerator)
-   fps = frame_interval.interval.denominator /
-   frame_interval.interval.numerator;
-   else
-   fps = 0;
-   }
-   return fps;
+   int ret;
+
+   ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
+  video, g_frame_interval, &fi);
+
+   if (ret)
+   return 0;
+
+   return (fi.interval.numerator > 0) ?
+  (fi.interval.denominator / fi.interval.numerator) : 0;
 }
 
 /*
-- 
1.9.1

[PATCH 1/4 V2] staging: atomisp: remove else statement after return

[Daeseok Youn]

It doesn't need to have else statement after return.

Signed-off-by: Daeseok Youn 
---
V2: one(2/4) of this series was updated so I tried to send them again.

 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index d97a8df..8bdb224 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -2958,11 +2958,11 @@ int atomisp_get_metadata(struct atomisp_sub_device 
*asd, int flag,
dev_err(isp->dev, "copy to user failed: copied %d bytes\n",
ret);
return -EFAULT;
-   } else {
-   list_del_init(&md_buf->list);
-   list_add_tail(&md_buf->list, &asd->metadata[md_type]);
}
 
+   list_del_init(&md_buf->list);
+   list_add_tail(&md_buf->list, &asd->metadata[md_type]);
+
dev_dbg(isp->dev, "%s: HAL de-queued metadata type %d with exp_id %d\n",
__func__, md_type, md->exp_id);
return 0;
-- 
1.9.1

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[DaeSeok Youn]

2017-03-20 22:11 GMT+09:00 walter harms :
>
>
> Am 20.03.2017 13:51, schrieb DaeSeok Youn:
>> 2017-03-20 21:04 GMT+09:00 walter harms :
>>>
>>>
>>> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>>>> If v4l2_subdev_call() gets the global frame interval values,
>>>> it returned 0 and it could be checked whether numerator is zero or not.
>>>>
>>>> If the numerator is not zero, the fps could be calculated in this function.
>>>> If not, it just returns 0.
>>>>
>>>> Signed-off-by: Daeseok Youn 
>>>> ---
>>>>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 
>>>> ++
>>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
>>>> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>>> index 8bdb224..6bdd19e 100644
>>>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
>>>> video_device *dev)
>>>>
>>>>  static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device 
>>>> *asd)
>>>>  {
>>>> - struct v4l2_subdev_frame_interval frame_interval;
>>>> + struct v4l2_subdev_frame_interval fi;
>>>>   struct atomisp_device *isp = asd->isp;
>>>> - unsigned short fps;
>>>>
>>>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>>> - video, g_frame_interval, &frame_interval)) {
>>>> - fps = 0;
>>>> - } else {
>>>> - if (frame_interval.interval.numerator)
>>>> - fps = frame_interval.interval.denominator /
>>>> - frame_interval.interval.numerator;
>>>> - else
>>>> - fps = 0;
>>>> - }
>>>> + unsigned short fps = 0;
>>>> + int ret;
>>>> +
>>>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>>> +video, g_frame_interval, &fi);
>>>> +
>>>> + if (!ret && fi.interval.numerator)
>>>> + fps = fi.interval.denominator / fi.interval.numerator;
>>>> +
>>>>   return fps;
>>>>  }
>>>
>>>
>>>
>>> do you need to check ret at all ? if an error occurs can 
>>> fi.interval.numerator
>>> be something else than 0 ?
>> the return value from the v4l2_subdev_call() function is zero when it
>> is done without any error. and also I checked
>> the ret value whether is 0 or not. if the ret is 0 then the value of
>> numerator should be checked to avoid for dividing by 0.
>>>
>>> if ret is an ERRNO it would be wise to return ret not fps, but this may 
>>> require
>>> changes at other places also.
>> hmm.., yes, you are right. but I think it is ok because the
>> atomisp_get_sensor_fps() function is needed to get fps value.
>> (originally, zero or calculated fps value was returned.)
>
> maybe its better to divide this in:
> if (ret)
>return 0; // error case
>
> return (fi.interval.numerator>0)?fi.interval.denominator / 
> fi.interval.numerator:0;
>
> So there is a chance that someone will a) understand and b) fix the error 
> return.
yes, it looks better than mine. I will update it and resend it.

Thanks walter,
Regards,
Daeseok Youn.

>
> re,
>  wh
>
>>
>>>
>>> re,
>>>  wh
>>>
>>>>
>>

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[DaeSeok Youn]

2017-03-20 21:04 GMT+09:00 walter harms :
>
>
> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>> If v4l2_subdev_call() gets the global frame interval values,
>> it returned 0 and it could be checked whether numerator is zero or not.
>>
>> If the numerator is not zero, the fps could be calculated in this function.
>> If not, it just returns 0.
>>
>> Signed-off-by: Daeseok Youn 
>> ---
>>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 
>> ++
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
>> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> index 8bdb224..6bdd19e 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
>> video_device *dev)
>>
>>  static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
>>  {
>> - struct v4l2_subdev_frame_interval frame_interval;
>> + struct v4l2_subdev_frame_interval fi;
>>   struct atomisp_device *isp = asd->isp;
>> - unsigned short fps;
>>
>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>> - video, g_frame_interval, &frame_interval)) {
>> - fps = 0;
>> - } else {
>> - if (frame_interval.interval.numerator)
>> - fps = frame_interval.interval.denominator /
>> - frame_interval.interval.numerator;
>> - else
>> - fps = 0;
>> - }
>> + unsigned short fps = 0;
>> + int ret;
>> +
>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>> +video, g_frame_interval, &fi);
>> +
>> + if (!ret && fi.interval.numerator)
>> + fps = fi.interval.denominator / fi.interval.numerator;
>> +
>>   return fps;
>>  }
>
>
>
> do you need to check ret at all ? if an error occurs can fi.interval.numerator
> be something else than 0 ?
the return value from the v4l2_subdev_call() function is zero when it
is done without any error. and also I checked
the ret value whether is 0 or not. if the ret is 0 then the value of
numerator should be checked to avoid for dividing by 0.
>
> if ret is an ERRNO it would be wise to return ret not fps, but this may 
> require
> changes at other places also.
hmm.., yes, you are right. but I think it is ok because the
atomisp_get_sensor_fps() function is needed to get fps value.
(originally, zero or calculated fps value was returned.)

>
> re,
>  wh
>
>>

[PATCH 4/4] staging: atomisp: remove redudant condition in if-statement

[Daeseok Youn]

The V4L2_FIELD_ANY is zero, so the (!field) is same meaning
with (field == V4L2_FIELD_ANY) in if-statement.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 929ed80..2437162 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -5084,7 +5084,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct 
v4l2_format *f,
 
depth = get_pixel_depth(pixelformat);
 
-   if (!field || field == V4L2_FIELD_ANY)
+   if (field == V4L2_FIELD_ANY)
field = V4L2_FIELD_NONE;
else if (field != V4L2_FIELD_NONE) {
dev_err(isp->dev, "Wrong output field\n");
-- 
1.9.1

[PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[Daeseok Youn]

If v4l2_subdev_call() gets the global frame interval values,
it returned 0 and it could be checked whether numerator is zero or not.

If the numerator is not zero, the fps could be calculated in this function.
If not, it just returns 0.

Signed-off-by: Daeseok Youn 
---
 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 8bdb224..6bdd19e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
video_device *dev)
 
 static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
 {
-   struct v4l2_subdev_frame_interval frame_interval;
+   struct v4l2_subdev_frame_interval fi;
struct atomisp_device *isp = asd->isp;
-   unsigned short fps;
 
-   if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-   video, g_frame_interval, &frame_interval)) {
-   fps = 0;
-   } else {
-   if (frame_interval.interval.numerator)
-   fps = frame_interval.interval.denominator /
-   frame_interval.interval.numerator;
-   else
-   fps = 0;
-   }
+   unsigned short fps = 0;
+   int ret;
+
+   ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
+  video, g_frame_interval, &fi);
+
+   if (!ret && fi.interval.numerator)
+   fps = fi.interval.denominator / fi.interval.numerator;
+
return fps;
 }
 
-- 
1.9.1

[PATCH 3/4] staging: atomisp: remove useless condition in if-statements

[Daeseok Youn]

The css_pipe_id was checked with 'CSS_PIPE_ID_COPY' in previous if-
statement. In this case, if the css_pipe_id equals to 'CSS_PIPE_ID_COPY',
it could not enter the next if-statement. But the "next" if-statement
has the condition to check whether the css_pipe_id equals to
'CSS_PIPE_ID_COPY' or not. It should be removed.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 6bdd19e..929ed80 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -910,8 +910,7 @@ static struct atomisp_video_pipe *__atomisp_get_pipe(
} else if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
/* For online video or SDV video pipe. */
if (css_pipe_id == CSS_PIPE_ID_VIDEO ||
-   css_pipe_id == CSS_PIPE_ID_COPY ||
-   css_pipe_id == CSS_PIPE_ID_YUVPP) {
+   css_pipe_id == CSS_PIPE_ID_COPY) {
if (buf_type == CSS_BUFFER_TYPE_OUTPUT_FRAME)
return &asd->video_out_video_capture;
return &asd->video_out_preview;
@@ -919,8 +918,7 @@ static struct atomisp_video_pipe *__atomisp_get_pipe(
} else if (asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW) {
/* For online preview or ZSL preview pipe. */
if (css_pipe_id == CSS_PIPE_ID_PREVIEW ||
-   css_pipe_id == CSS_PIPE_ID_COPY ||
-   css_pipe_id == CSS_PIPE_ID_YUVPP)
+   css_pipe_id == CSS_PIPE_ID_COPY)
return &asd->video_out_preview;
}
/* For capture pipe. */
-- 
1.9.1

[PATCH 1/4] staging: atomisp: remove else statement after return

[Daeseok Youn]

It doesn't need to have else statement after return.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index d97a8df..8bdb224 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -2958,11 +2958,11 @@ int atomisp_get_metadata(struct atomisp_sub_device 
*asd, int flag,
dev_err(isp->dev, "copy to user failed: copied %d bytes\n",
ret);
return -EFAULT;
-   } else {
-   list_del_init(&md_buf->list);
-   list_add_tail(&md_buf->list, &asd->metadata[md_type]);
}
 
+   list_del_init(&md_buf->list);
+   list_add_tail(&md_buf->list, &asd->metadata[md_type]);
+
dev_dbg(isp->dev, "%s: HAL de-queued metadata type %d with exp_id %d\n",
__func__, md_type, md->exp_id);
return 0;
-- 
1.9.1

[PATCH 4/4] staging: atomisp: fix "alignment should match open parenthesis"

[Daeseok Youn]

Fix checkpatch.pl issues in atomisp_cmd.c
 : "CHECK: Alignment should match open parenthesis"

Signed-off-by: Daeseok Youn 
---
 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 179 +++--
 1 file changed, 90 insertions(+), 89 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 82e7382..d97a8df 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -158,7 +158,7 @@ static unsigned short atomisp_get_sensor_fps(struct 
atomisp_sub_device *asd)
unsigned short fps;
 
if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-   video, g_frame_interval, &frame_interval)) {
+   video, g_frame_interval, &frame_interval)) {
fps = 0;
} else {
if (frame_interval.interval.numerator)
@@ -481,7 +481,7 @@ static void atomisp_3a_stats_ready_event(struct 
atomisp_sub_device *asd, uint8_t
 }
 
 static void atomisp_metadata_ready_event(struct atomisp_sub_device *asd,
-   enum atomisp_metadata_type 
md_type)
+enum atomisp_metadata_type md_type)
 {
struct v4l2_event event = {0};
 
@@ -622,14 +622,14 @@ irqreturn_t atomisp_isr(int irq, void *dev)
}
if (irq_infos & CSS_IRQ_INFO_EVENTS_READY)
atomic_set(&asd->sequence,
-   atomic_read(&asd->sequence_temp));
+  atomic_read(&asd->sequence_temp));
}
 
if (irq_infos & CSS_IRQ_INFO_CSS_RECEIVER_SOF)
irq_infos &= ~CSS_IRQ_INFO_CSS_RECEIVER_SOF;
 
if ((irq_infos & CSS_IRQ_INFO_INPUT_SYSTEM_ERROR) ||
-   (irq_infos & CSS_IRQ_INFO_IF_ERROR)) {
+   (irq_infos & CSS_IRQ_INFO_IF_ERROR)) {
/* handle mipi receiver error */
u32 rx_infos;
enum ia_css_csi2_port port;
@@ -684,7 +684,7 @@ void atomisp_clear_css_buffer_counters(struct 
atomisp_sub_device *asd)
memset(asd->s3a_bufs_in_css, 0, sizeof(asd->s3a_bufs_in_css));
for (i = 0; i < ATOMISP_INPUT_STREAM_NUM; i++)
memset(asd->metadata_bufs_in_css[i], 0,
-   sizeof(asd->metadata_bufs_in_css[i]));
+  sizeof(asd->metadata_bufs_in_css[i]));
asd->dis_bufs_in_css = 0;
asd->video_out_capture.buffers_in_css = 0;
asd->video_out_vf.buffers_in_css = 0;
@@ -804,7 +804,7 @@ void atomisp_flush_params_queue(struct atomisp_video_pipe 
*pipe)
 
while (!list_empty(&pipe->per_frame_params)) {
param = list_entry(pipe->per_frame_params.next,
-   struct atomisp_css_params_with_list, list);
+  struct atomisp_css_params_with_list, list);
list_del(¶m->list);
atomisp_free_css_parameters(¶m->params);
atomisp_kernel_free(param);
@@ -983,7 +983,7 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int 
error,
memset(&buffer, 0, sizeof(struct atomisp_css_buffer));
buffer.css_buffer.type = buf_type;
err = atomisp_css_dequeue_buffer(asd, stream_id, css_pipe_id,
-   buf_type, &buffer);
+buf_type, &buffer);
if (err) {
dev_err(isp->dev,
"atomisp_css_dequeue_buffer failed: 0x%x\n", err);
@@ -1000,12 +1000,12 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, 
int error,
switch (buf_type) {
case CSS_BUFFER_TYPE_3A_STATISTICS:
list_for_each_entry_safe(s3a_buf, _s3a_buf_tmp,
-   &asd->s3a_stats_in_css, list) {
+&asd->s3a_stats_in_css, list) {
if (s3a_buf->s3a_data ==
buffer.css_buffer.data.stats_3a) {
list_del_init(&s3a_buf->list);
list_add_tail(&s3a_buf->list,
-   &asd->s3a_stats_ready);
+ &asd->s3a_stats_ready);
break;
}
}
@@ -1021,12 +1021,12 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, 
int error,
 
md_type = atomisp_get_metadata_type(asd, css_pipe_id);
list_for_each_entry_safe(md_buf, _md_buf_tmp,
-   &asd->metadata_in_css[md_type], list) {
+  

[PATCH 3/4] staging: atomisp: remove useless #ifdef ISP2401 on top of atomisp_cmd.c

[Daeseok Youn]

There is no reason to have "#ifdef ISP2401" condition
on top of atomisp_cmd.c file

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 6160119..82e7382 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -1,6 +1,3 @@
-#ifdef ISP2401
-
-#endif
 /*
  * Support for Medifield PNW Camera Imaging ISP subsystem.
  *
-- 
1.9.1

[PATCH 2/4] staging: atomisp: fix inconsistent indenting

[Daeseok Youn]

Fix warnings from the smatch tool

atomisp_cmd.c:5698
   atomisp_set_fmt_to_snr() warn: inconsistent indenting
atomisp_cmd.c:5714
   atomisp_set_fmt_to_snr() warn: inconsistent indenting

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 9c3ba11..6160119 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -5693,7 +5693,7 @@ static int atomisp_set_fmt_to_snr(struct video_device 
*vdev,
/* Disable dvs if resolution can't be supported by sensor */
if (asd->params.video_dis_en &&
source_pad == ATOMISP_SUBDEV_PAD_SOURCE_VIDEO) {
-   vformat.which = V4L2_SUBDEV_FORMAT_TRY;
+   vformat.which = V4L2_SUBDEV_FORMAT_TRY;
ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
pad, set_fmt, &pad_cfg, &vformat);
if (ret)
@@ -5710,7 +5710,7 @@ static int atomisp_set_fmt_to_snr(struct video_device 
*vdev,
}
dev_dbg(isp->dev, "sensor width: %d, height: %d\n",
ffmt->width, ffmt->height);
-vformat.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   vformat.which = V4L2_SUBDEV_FORMAT_ACTIVE;
ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera, pad,
   set_fmt, NULL, &vformat);
if (ret)
-- 
1.9.1

[PATCH 1/4] staging: atomisp: fix unsigned int comparison with less than zero

[Daeseok Youn]

Fix warnings from the smatch tool

atomisp_cmd.c:2649
  atomisp_set_array_res() warn:
  unsigned 'config->width' is never less than zero.

atomisp_cmd.c:2650
  atomisp_set_array_res() warn:
  unsigned 'config->height' is never less than zero.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 1ee99d0..9c3ba11 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -2646,8 +2646,7 @@ int atomisp_set_array_res(struct atomisp_sub_device *asd,
 struct atomisp_resolution  *config)
 {
dev_dbg(asd->isp->dev, ">%s start\n", __func__);
-   if (config == NULL || config->width < 0
-   || config->height < 0) {
+   if (!config) {
dev_err(asd->isp->dev, "Set sensor array size is not valid\n");
return -EINVAL;
}
-- 
1.9.1

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[DaeSeok Youn]

2017-03-14 2:54 GMT+09:00 Alan Cox :
>
> On Mon, 2017-03-13 at 19:54 +0900, Daeseok Youn wrote:
> > If the atomisp_kernel_zalloc() has "true" as a second parameter, it
> > tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
> > But using kzalloc is rather than kmalloc followed by memset with 0.
> > (vzalloc is for same reason with kzalloc)
>
> This is true but please don't apply this. There are about five other
> layers of indirection for memory allocators that want removing first so
> that the driver just uses the correct kmalloc/kzalloc/kv* functions in
> the right places.
right. kvmalloc/kvzalloc would be used after preparing those
interfaces in staging tree.
I will try to change all the atomisp_kernel_m{z}alloc() callers to
correct functions to allocate memory.

Thanks.
Regards,
Jake.

>
> Alan
>

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[DaeSeok Youn]

2017-03-13 23:07 GMT+09:00 DaeSeok Youn :
> 2017-03-13 20:51 GMT+09:00 Dan Carpenter :
>> On Mon, Mar 13, 2017 at 07:54:21PM +0900, Daeseok Youn wrote:
>>> If the atomisp_kernel_zalloc() has "true" as a second parameter, it
>>> tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
>>> But using kzalloc is rather than kmalloc followed by memset with 0.
>>> (vzalloc is for same reason with kzalloc)
>>>
>>> And also atomisp_kernel_malloc() can be used with
>>> atomisp_kernel_zalloc(, false);
>>>
>>
>> We should just change all the callers to kvmalloc() and kvzmalloc().
> ok. I will try to change all the callers to kvmalloc() and kvzalloc().

The kvmalloc() and kvzalloc() are not ready to use in staging-testing
branch on staging tree.
If the kvmalloc and kvzalloc are available to use, I will replace
atomisp_kernel_malloc() and atomisp_kernel_zalloc() with kvmalloc()
and kvzalloc().

Thanks.
Regards,
Daeseok Youn.


>
> Thanks.
> Regards,
> Daeseok Youn
>>
>> regards,
>> dan carpenter
>>

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[DaeSeok Youn]

2017-03-13 20:51 GMT+09:00 Dan Carpenter :
> On Mon, Mar 13, 2017 at 07:54:21PM +0900, Daeseok Youn wrote:
>> If the atomisp_kernel_zalloc() has "true" as a second parameter, it
>> tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
>> But using kzalloc is rather than kmalloc followed by memset with 0.
>> (vzalloc is for same reason with kzalloc)
>>
>> And also atomisp_kernel_malloc() can be used with
>> atomisp_kernel_zalloc(, false);
>>
>
> We should just change all the callers to kvmalloc() and kvzmalloc().
ok. I will try to change all the callers to kvmalloc() and kvzalloc().

Thanks.
Regards,
Daeseok Youn
>
> regards,
> dan carpenter
>

[PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[Daeseok Youn]

If the atomisp_kernel_zalloc() has "true" as a second parameter, it
tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
But using kzalloc is rather than kmalloc followed by memset with 0.
(vzalloc is for same reason with kzalloc)

And also atomisp_kernel_malloc() can be used with
atomisp_kernel_zalloc(, false);

Signed-off-by: Daeseok Youn 
---
I think kvmalloc() or kvzalloc() can be used to allocate memory if there is
no reason to use vmalloc() when the requested bytes is over PAGE_SIZE.

 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 25 --
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index d9a5c24..44b2244 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -86,32 +86,35 @@
 };
 
 /*
- * atomisp_kernel_malloc: chooses whether kmalloc() or vmalloc() is preferable.
+ * atomisp_kernel_malloc:
+ * allocating memory from atomisp_kernel_zalloc() without zeroing memory.
  *
  * It is also a wrap functions to pass into css framework.
  */
 void *atomisp_kernel_malloc(size_t bytes)
 {
-   /* vmalloc() is preferable if allocating more than 1 page */
-   if (bytes > PAGE_SIZE)
-   return vmalloc(bytes);
-
-   return kmalloc(bytes, GFP_KERNEL);
+   return atomisp_kernel_zalloc(bytes, false);
 }
 
 /*
- * atomisp_kernel_zalloc: chooses whether set 0 to the allocated memory.
+ * atomisp_kernel_zalloc: chooses whether set 0 to the allocated memory
+ * with k{z,m}alloc or v{z,m}alloc
  *
  * It is also a wrap functions to pass into css framework.
  */
 void *atomisp_kernel_zalloc(size_t bytes, bool zero_mem)
 {
-   void *ptr = atomisp_kernel_malloc(bytes);
+   /* vmalloc() is preferable if allocating more than 1 page */
+   if (bytes > PAGE_SIZE) {
+   if (zero_mem)
+   return vzalloc(bytes);
+   return vmalloc(bytes);
+   }
 
-   if (ptr && zero_mem)
-   memset(ptr, 0, bytes);
+   if (zero_mem)
+   return kzalloc(bytes, GFP_KERNEL);
 
-   return ptr;
+   return kmalloc(bytes, GFP_KERNEL);
 }
 
 /*
-- 
1.9.1

[PATCH] [media] s2255drv: fix memory leak s2255_probe()

[Daeseok Youn]

smatch says:
 drivers/media/usb/s2255/s2255drv.c:2246 s2255_probe() warn:
possible memory leak of 'dev'

Signed-off-by: Daeseok Youn 
---
V2: use the same pattern for error handling.

 drivers/media/usb/s2255/s2255drv.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/usb/s2255/s2255drv.c 
b/drivers/media/usb/s2255/s2255drv.c
index 1d4ba2b..3193474 100644
--- a/drivers/media/usb/s2255/s2255drv.c
+++ b/drivers/media/usb/s2255/s2255drv.c
@@ -2243,7 +2243,7 @@ static int s2255_probe(struct usb_interface *interface,
dev->cmdbuf = kzalloc(S2255_CMDBUF_SIZE, GFP_KERNEL);
if (dev->cmdbuf == NULL) {
s2255_dev_err(&interface->dev, "out of memory\n");
-   return -ENOMEM;
+   goto errorFWDATA1;
}
 
atomic_set(&dev->num_channels, 0);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] s2255drv: fix memory leak s2255_probe()

[DaeSeok Youn]

Hello,

2014-05-05 17:38 GMT+09:00 Sakari Ailus :
> On Tue, Apr 15, 2014 at 07:54:43PM +0900, DaeSeok Youn wrote:
>> Hi, Sakari
>>
>> 2014-04-15 18:33 GMT+09:00 Sakari Ailus :
>> > Hi Daeseok,
>> >
>> > On Tue, Apr 15, 2014 at 01:49:34PM +0900, Daeseok Youn wrote:
>> >>
>> >> smatch says:
>> >>  drivers/media/usb/s2255/s2255drv.c:2246 s2255_probe() warn:
>> >> possible memory leak of 'dev'
>> >>
>> >> Signed-off-by: Daeseok Youn 
>> >> ---
>> >>  drivers/media/usb/s2255/s2255drv.c |1 +
>> >>  1 files changed, 1 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/drivers/media/usb/s2255/s2255drv.c 
>> >> b/drivers/media/usb/s2255/s2255drv.c
>> >> index 1d4ba2b..8aca3ef 100644
>> >> --- a/drivers/media/usb/s2255/s2255drv.c
>> >> +++ b/drivers/media/usb/s2255/s2255drv.c
>> >> @@ -2243,6 +2243,7 @@ static int s2255_probe(struct usb_interface 
>> >> *interface,
>> >>   dev->cmdbuf = kzalloc(S2255_CMDBUF_SIZE, GFP_KERNEL);
>> >>   if (dev->cmdbuf == NULL) {
>> >>   s2255_dev_err(&interface->dev, "out of memory\n");
>> >> + kfree(dev);
>> >>   return -ENOMEM;
>> >>   }
>> >>
>> >
>> > The rest of the function already uses goto and labels for error handling. I
>> > think it'd take adding one more. dev is correctly released in other error
>> > cases.
>> I am not sure that adding a new label for error handling when
>> allocation for dev->cmdbuf is failed.
>> I think it is ok to me. :-) Because I think it is not good adding a
>> new label and use goto statement for this.
>
> I can ack this if you use the same pattern for error handling that's already
> there.
But it has a redundant kfree() call for dev->cmdbuf when it failed to
allocate, right?
If it is ok, I send this again after fixing as your comment.

Thanks.
Daeseok Youn.
>
> --
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] s2255drv: fix memory leak s2255_probe()

[DaeSeok Youn]

Hi, Sakari

2014-04-15 18:33 GMT+09:00 Sakari Ailus :
> Hi Daeseok,
>
> On Tue, Apr 15, 2014 at 01:49:34PM +0900, Daeseok Youn wrote:
>>
>> smatch says:
>>  drivers/media/usb/s2255/s2255drv.c:2246 s2255_probe() warn:
>> possible memory leak of 'dev'
>>
>> Signed-off-by: Daeseok Youn 
>> ---
>>  drivers/media/usb/s2255/s2255drv.c |1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/usb/s2255/s2255drv.c 
>> b/drivers/media/usb/s2255/s2255drv.c
>> index 1d4ba2b..8aca3ef 100644
>> --- a/drivers/media/usb/s2255/s2255drv.c
>> +++ b/drivers/media/usb/s2255/s2255drv.c
>> @@ -2243,6 +2243,7 @@ static int s2255_probe(struct usb_interface *interface,
>>   dev->cmdbuf = kzalloc(S2255_CMDBUF_SIZE, GFP_KERNEL);
>>   if (dev->cmdbuf == NULL) {
>>   s2255_dev_err(&interface->dev, "out of memory\n");
>> + kfree(dev);
>>   return -ENOMEM;
>>   }
>>
>
> The rest of the function already uses goto and labels for error handling. I
> think it'd take adding one more. dev is correctly released in other error
> cases.
I am not sure that adding a new label for error handling when
allocation for dev->cmdbuf is failed.
I think it is ok to me. :-) Because I think it is not good adding a
new label and use goto statement for this.

Thanks for review.

Regards,
Daeseok Youn.
>
> What do you think?
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] s2255drv: fix memory leak s2255_probe()

[Daeseok Youn]

smatch says:
 drivers/media/usb/s2255/s2255drv.c:2246 s2255_probe() warn:
possible memory leak of 'dev'

Signed-off-by: Daeseok Youn 
---
 drivers/media/usb/s2255/s2255drv.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/usb/s2255/s2255drv.c 
b/drivers/media/usb/s2255/s2255drv.c
index 1d4ba2b..8aca3ef 100644
--- a/drivers/media/usb/s2255/s2255drv.c
+++ b/drivers/media/usb/s2255/s2255drv.c
@@ -2243,6 +2243,7 @@ static int s2255_probe(struct usb_interface *interface,
dev->cmdbuf = kzalloc(S2255_CMDBUF_SIZE, GFP_KERNEL);
if (dev->cmdbuf == NULL) {
s2255_dev_err(&interface->dev, "out of memory\n");
+   kfree(dev);
return -ENOMEM;
}
 
-- 
1.7.4.4


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging: lirc: fix NULL pointer dereference

[DaeSeok Youn]

Please drop this patch.

I made a patch as Dan's comment and sent it.

Thanks.
Daeseok Youn.

2014-04-02 18:41 GMT+09:00 DaeSeok Youn :
> You are right. remove whole thing and send it again.
>
> Thanks.
> Daeseok Youn
>
> 2014-04-02 18:13 GMT+09:00 Dan Carpenter :
>> On Wed, Apr 02, 2014 at 05:18:39PM +0900, Daeseok Youn wrote:
>>>
>>> coccicheck says:
>>>  drivers/staging/media/lirc/lirc_igorplugusb.c:226:15-21:
>>> ERROR: ir is NULL but dereferenced.
>>>
>>> Signed-off-by: Daeseok Youn 
>>> ---
>>>  drivers/staging/media/lirc/lirc_igorplugusb.c |4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/lirc/lirc_igorplugusb.c 
>>> b/drivers/staging/media/lirc/lirc_igorplugusb.c
>>> index f508a13..0ef393b 100644
>>> --- a/drivers/staging/media/lirc/lirc_igorplugusb.c
>>> +++ b/drivers/staging/media/lirc/lirc_igorplugusb.c
>>> @@ -223,8 +223,8 @@ static int unregister_from_lirc(struct igorplug *ir)
>>>   int devnum;
>>>
>>>   if (!ir) {
>>> - dev_err(&ir->usbdev->dev,
>>> - "%s: called with NULL device struct!\n", __func__);
>>> + printk(DRIVER_NAME "%s: called with NULL device struct!\n",
>>> +__func__);
>>
>>
>> It should be pr_err() or something.  But actually "ir" can't be NULL so
>> just delete the whole condition.
>>
>>
>>>   return -EINVAL;
>>>   }
>>
>> regards,
>> dan carpenter
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] staging: lirc: remove redundant NULL check in unregister_from_lirc()

[Daeseok Youn]

"ir" is already checked before calling unregister_from_lirc().

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/lirc/lirc_igorplugusb.c |6 --
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_igorplugusb.c 
b/drivers/staging/media/lirc/lirc_igorplugusb.c
index f508a13..e93e1c2 100644
--- a/drivers/staging/media/lirc/lirc_igorplugusb.c
+++ b/drivers/staging/media/lirc/lirc_igorplugusb.c
@@ -222,12 +222,6 @@ static int unregister_from_lirc(struct igorplug *ir)
struct lirc_driver *d;
int devnum;
 
-   if (!ir) {
-   dev_err(&ir->usbdev->dev,
-   "%s: called with NULL device struct!\n", __func__);
-   return -EINVAL;
-   }
-
devnum = ir->devnum;
d = ir->d;
 
-- 
1.7.4.4


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging: lirc: fix NULL pointer dereference

[DaeSeok Youn]

You are right. remove whole thing and send it again.

Thanks.
Daeseok Youn

2014-04-02 18:13 GMT+09:00 Dan Carpenter :
> On Wed, Apr 02, 2014 at 05:18:39PM +0900, Daeseok Youn wrote:
>>
>> coccicheck says:
>>  drivers/staging/media/lirc/lirc_igorplugusb.c:226:15-21:
>> ERROR: ir is NULL but dereferenced.
>>
>> Signed-off-by: Daeseok Youn 
>> ---
>>  drivers/staging/media/lirc/lirc_igorplugusb.c |4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/lirc/lirc_igorplugusb.c 
>> b/drivers/staging/media/lirc/lirc_igorplugusb.c
>> index f508a13..0ef393b 100644
>> --- a/drivers/staging/media/lirc/lirc_igorplugusb.c
>> +++ b/drivers/staging/media/lirc/lirc_igorplugusb.c
>> @@ -223,8 +223,8 @@ static int unregister_from_lirc(struct igorplug *ir)
>>   int devnum;
>>
>>   if (!ir) {
>> - dev_err(&ir->usbdev->dev,
>> - "%s: called with NULL device struct!\n", __func__);
>> + printk(DRIVER_NAME "%s: called with NULL device struct!\n",
>> +__func__);
>
>
> It should be pr_err() or something.  But actually "ir" can't be NULL so
> just delete the whole condition.
>
>
>>   return -EINVAL;
>>   }
>
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] staging: lirc: fix NULL pointer dereference

[Daeseok Youn]

coccicheck says:
 drivers/staging/media/lirc/lirc_igorplugusb.c:226:15-21:
ERROR: ir is NULL but dereferenced.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/media/lirc/lirc_igorplugusb.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_igorplugusb.c 
b/drivers/staging/media/lirc/lirc_igorplugusb.c
index f508a13..0ef393b 100644
--- a/drivers/staging/media/lirc/lirc_igorplugusb.c
+++ b/drivers/staging/media/lirc/lirc_igorplugusb.c
@@ -223,8 +223,8 @@ static int unregister_from_lirc(struct igorplug *ir)
int devnum;
 
if (!ir) {
-   dev_err(&ir->usbdev->dev,
-   "%s: called with NULL device struct!\n", __func__);
+   printk(DRIVER_NAME "%s: called with NULL device struct!\n",
+  __func__);
return -EINVAL;
}
 
-- 
1.7.4.4


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html