Hi Dan,
On Thu, May 23, 2024 at 02:44:06PM +0100, Dan Scally wrote:
> Hi Sakari - thanks for the review. Snipping some bits for which I have no comment...
>
> On 23/05/2024 09:03, Sakari Ailus wrote:
>
> <snip>
> > > +
> > > +static unsigned int mali_c55_calculate_bank_num(struct mali_c55 *mali_c55,
> > > + unsigned int crop,
> > > + unsigned int scale)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned int i;
> > > +
> > > + tmp = (scale * 1000) / crop;
> > This looks like something that can overflow. Can it?
>
>
> Shouldn't be able to; maximum scale width is 8192.
Ok.
1000U in that case?
> > > + for (i = 0; i < MALI_C55_RESIZER_COEFS_NUM_BANKS; i++) {
> > > + for (j = 0; j < MALI_C55_RESIZER_COEFS_NUM_ENTRIES; j++) {
> > > + mali_c55_write(mali_c55, haddr,
> > > + mali_c55_scaler_h_filter_coefficients[i][j]);
> > > + mali_c55_write(mali_c55, vaddr,
> > > + mali_c55_scaler_v_filter_coefficients[i][j]);
> > > +
> > > + haddr += 4;
> > > + vaddr += 4;
> > sizeof(u32) ?
> >
> > Up to you.
>
>
> I think I'll keep it if it's all the same to you
Well, not the same but I'll let you decide. :-)
...
> > > +static int mali_c55_tpg_init_state(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *sd_state)
> > > +{
> > > + struct v4l2_mbus_framefmt *fmt;
> > > +
> > > + fmt = v4l2_subdev_state_get_format(sd_state, MALI_C55_TPG_SRC_PAD);
> > Can be assigned in the declaration.
>
>
> How would you make it fit that way?
struct v4l2_mbus_framefmt *fmt =
v4l2_subdev_state_get_format(sd_state, MALI_C55_TPG_SRC_PAD);
--
Regards,
Sakari Ailus
