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

[Dan Scally]

Hi Sakari

On 23/05/2024 18:53, Sakari Ailus wrote:
> 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?


Done

>
>>>> +	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. :-)


OK you're right, the sizeof() is better

>
> ...
>
>>>> +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);


Done - thank you!

>