[1/4] media: staging: rkisp1: remove two unused fields in uapi struct
diff mbox series

Message ID 20200625185017.16493-2-dafna.hirschfeld@collabora.com
State New
Headers show
Series
  • media: staging: rkisp1: fixes related to the uapi lsc struct
Related show

Commit Message

Dafna Hirschfeld June 25, 2020, 6:50 p.m. UTC
The fields 'config_width', 'config_height' in struct
'rkisp1_cif_isp_lsc_config' are not used by the driver and
therefore are not needed. This patch removes them.
In later patch, documentation of the fields in struct
'rkisp1_cif_isp_lsc_config' will be added.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
 1 file changed, 2 deletions(-)

Comments

Ezequiel Garcia June 26, 2020, 12:47 p.m. UTC | #1
Hi Dafna,

Thanks for all the rkisp1 patches, I hope we can get
the driver out of staging soon.

On Thu, 2020-06-25 at 20:50 +0200, Dafna Hirschfeld wrote:
> The fields 'config_width', 'config_height' in struct
> 'rkisp1_cif_isp_lsc_config' are not used by the driver and
> therefore are not needed. This patch removes them.
> In later patch, documentation of the fields in struct
> 'rkisp1_cif_isp_lsc_config' will be added.
> 

If I understand correctly, you are affecting the uAPI here.

The fact that the driver doesn't use it now, doesn't mean
it won't need it at some later point.

I'm not saying the change is wrong, but that the "not currently
in use" reason might be insufficient for a uAPI. If you
want to remove this field, I suggest you make sure
the field is inappropriate for this interface.

Also, it would be better if you could add a cover letter
on all the series, there are a bunch of rkisp1 patches now
and having a cover letter helps by adding some context.

Thanks,
Ezequiel

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index ca0d031b14ac..7331bacf7dfd 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
>  
>  	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>  	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
> -	__u16 config_width;
> -	__u16 config_height;
>  } __packed;
>  
>  /**
Dafna Hirschfeld July 6, 2020, 2:16 p.m. UTC | #2
Hi,

On 26.06.20 14:47, Ezequiel Garcia wrote:
> Hi Dafna,
> 
> Thanks for all the rkisp1 patches, I hope we can get
> the driver out of staging soon.

me too.

> 
> On Thu, 2020-06-25 at 20:50 +0200, Dafna Hirschfeld wrote:
>> The fields 'config_width', 'config_height' in struct
>> 'rkisp1_cif_isp_lsc_config' are not used by the driver and
>> therefore are not needed. This patch removes them.
>> In later patch, documentation of the fields in struct
>> 'rkisp1_cif_isp_lsc_config' will be added.
>>
> 
> If I understand correctly, you are affecting the uAPI here.
> 
> The fact that the driver doesn't use it now, doesn't mean
> it won't need it at some later point.
> 
> I'm not saying the change is wrong, but that the "not currently
> in use" reason might be insufficient for a uAPI. If you
> want to remove this field, I suggest you make sure
> the field is inappropriate for this interface.
> 
I looked at the documentation and didn't find
any width/height values related to the LSC interface.
So I it seems to be ok to remove those fields.

> Also, it would be better if you could add a cover letter
> on all the series, there are a bunch of rkisp1 patches now
> and having a cover letter helps by adding some context.
> 

I did add a cover-letter: https://patchwork.kernel.org/cover/11625921/

Thanks,
Dafna

> Thanks,
> Ezequiel
> 
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> index ca0d031b14ac..7331bacf7dfd 100644
>> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> @@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
>>   
>>   	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>>   	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>> -	__u16 config_width;
>> -	__u16 config_height;
>>   } __packed;
>>   
>>   /**
> 
>
Helen Koike July 24, 2020, 3:21 p.m. UTC | #3
Hi Dafna,


On 6/25/20 3:50 PM, Dafna Hirschfeld wrote:
> The fields 'config_width', 'config_height' in struct
> 'rkisp1_cif_isp_lsc_config' are not used by the driver and
> therefore are not needed. This patch removes them.
> In later patch, documentation of the fields in struct
> 'rkisp1_cif_isp_lsc_config' will be added.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index ca0d031b14ac..7331bacf7dfd 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
>  
>  	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>  	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
> -	__u16 config_width;
> -	__u16 config_height;
>  } __packed;
>  
>  /**
> 


I was checking the Rockchip 3A library API, and they use these fields on:

https://github.com/rockchip-linux/camera_engine_rkisp/blob/master/rkisp/ia-engine/include/ia/rk_aiq_types.h#L409

Which I'm not sure what it means tbh, so I would rather leave these fields (with a comment to say it is not used
by the driver) just in case it is useful in the future.
Ideally we should try to understand what this field does to make a decision.

Regards,
Helen

Patch
diff mbox series

diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
index ca0d031b14ac..7331bacf7dfd 100644
--- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
+++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
@@ -285,8 +285,6 @@  struct rkisp1_cif_isp_lsc_config {
 
 	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
 	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
-	__u16 config_width;
-	__u16 config_height;
 } __packed;
 
 /**