From: Hans de Goede > Sent: 06 September 2024 08:53 > > Hi Mauro, > > On 9/6/24 8:15 AM, Mauro Carvalho Chehab wrote: > > Em Sat, 27 Jul 2024 14:51:56 +0200 > > Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> escreveu: > > > >> Using clamp_t() instead of min_t(max_t()) is easier to read. > >> > >> It also reduces the size of the preprocessed files by ~ 193 ko. > >> (see [1] for a discussion about it) > >> > >> $ ls -l ia_css_eed1_8.host*.i > >> 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i > >> 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i > >> > >> [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@xxxxxxxxxxxxxxxx/ > >> > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> > >> --- > >> .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- > >> 1 file changed, 12 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > >> index e4fc90f88e24..96c13ebc4331 100644 > >> --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > >> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > >> @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( > >> base = shuffle_block * i; > >> > >> for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > >> - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, > >> - from->dew_enhance_seg_x[j], 0), > >> - 8191); > >> - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, > >> - from->dew_enhance_seg_y[j], -8192), > >> - 8191); > >> + to->e_dew_enh_x[0][base + j] = clamp_t(int, > >> + from->dew_enhance_seg_x[j], > >> + 0, 8191); > >> + to->e_dew_enh_y[0][base + j] = clamp_t(int, > >> + from->dew_enhance_seg_y[j], > >> + -8192, 8191); > > > > Such change introduces two warnings on smatch: > > > > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 > ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + > j]' > > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 > ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + > j]' > > > > Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? > > These already are s32, the problem is that e_dew_enh_a is of type t_vmem_elem which is: > > typedef u16 t_vmem_elem; Ugg... :-) > > And that type is used in a lot of places, so we cannot > just change that. > > I guess we could add a t_signed_vmem_elem (s16) and use that for these vmem-arrays ? > > I tried fixing it like this: > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > b/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > index 1e01d354152b..7c0195d15f53 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > @@ -428,18 +428,13 @@ static int t4ka3_s_stream(struct v4l2_subdev *sd, int enable) > goto error_unlock; > } > > - ret = cci_multi_reg_write(sensor->regmap, t4ka3_init_config, > - ARRAY_SIZE(t4ka3_init_config), NULL); > - if (ret) > - goto error_powerdown; > - > + cci_multi_reg_write(sensor->regmap, t4ka3_init_config, > + ARRAY_SIZE(t4ka3_init_config), &ret); > /* enable group hold */ > - ret = cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, > - ARRAY_SIZE(t4ka3_param_hold), NULL); > - if (ret) > - goto error_powerdown; > - > - ret = cci_multi_reg_write(sensor->regmap, sensor->res->regs, sensor->res->regs_len, NULL); > + cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, > + ARRAY_SIZE(t4ka3_param_hold), &ret); > + cci_multi_reg_write(sensor->regmap, sensor->res->regs, > + sensor->res->regs_len, &ret); > if (ret) > goto error_powerdown; Isn't that unrelated? > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > index b79d78e5b77f..c9043d516192 100644 > --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > @@ -172,21 +172,21 @@ ia_css_eed1_8_vmem_encode( > base = shuffle_block * i; > > for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > - to->e_dew_enh_x[0][base + j] = clamp(from->dew_enhance_seg_x[j], > - 0, 8191); > - to->e_dew_enh_y[0][base + j] = clamp(from->dew_enhance_seg_y[j], > - -8192, 8191); > + to->e_dew_enh_x[0][base + j] = (u16)clamp(from->dew_enhance_seg_x[j], > + 0, 8191); > + to->e_dew_enh_y[0][base + j] = (u16)clamp(from->dew_enhance_seg_y[j], > + -8192, 8191); How about an explicit clamp(...) & 0xffffu? > > for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { > - to->e_dew_enh_a[0][base + j] = clamp(from->dew_enhance_seg_slope[j], > - -8192, 8191); > + to->e_dew_enh_a[0][base + j] = (u16)clamp(from->dew_enhance_seg_slope[j], > + -8192, 8191); > /* Convert dew_enhance_seg_exp to flag: > * 0 -> 0 > * 1...13 -> 1 > */ > - to->e_dew_enh_f[0][base + j] = clamp(from->dew_enhance_seg_exp[j], > - 0, 13) > 0; > + to->e_dew_enh_f[0][base + j] = (u16)clamp(from->dew_enhance_seg_exp[j], > + 0, 13) > 0; Isn't the RHS just from->dew_enhance_seg_exp[j] > 0 ? That shouldn't be generating any kind of warning anyway. David > } > > /* Hard-coded to 0, in order to be able to handle out of > > but smatch still complains after this... > > Regards, > > Hans > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
