[PATCH v4 3/5] media: mali-c55: Add Mali-C55 ISP driver

[Sakari Ailus]

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