| Message ID | 20200625185017.16493-2-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: fixes related to the uapi lsc struct | expand |
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; > > /**
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; >> >> /** > >
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
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; /**
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(-)