[PATCH v14 2/6] media: imagination: Add E5010 JPEG Encoder driver

[Devarsh Thakkar]

Hi Sebastian,

On 21/06/24 18:56, Sebastian Fricke wrote:
> Hey Devarsh,
> 
> On 21.06.2024 18:14, Devarsh Thakkar wrote:
>> Hi Sebastian
>>
>> On 21/06/24 18:07, Sebastian Fricke wrote:
>>> Hey Devarsh,
>>>
>>> This doesn't compile without errors for me, curious, it probably did
>>> compile without problems for you right?
>>>
>>> drivers/media/platform/imagination/e5010-jpeg-enc.c:1622:19: error:
>>> initialization of ‘int (*)(struct platform_device *)’ from incompatible
>>> pointer type ‘void (*)(struct platform_device *)’
>>> [-Werror=incompatible-pointer-types]
>>>  1622 |         .remove = e5010_remove,
>>>       |                   ^~~~~~~~~~~~
>>> drivers/media/platform/imagination/e5010-jpeg-enc.c:1622:19: note: (near
>>> initialization for ‘e5010_driver.remove’)
>>> cc1: some warnings being treated as errors
>>>
>>
>> Yes I think it did compile fine for me.
>> Did you try this on tip of linux-next ?
> 
> Nope the media subsystem is not based on linux-next, but instead on:
> https://git.linuxtv.org/media_stage.git/log/
> Please make sure that your patches compile on top of that.
> 

Sure, will take care of this in future.

>>
>> As mentioned in changelog, there was update in platform driver for return type
>> of remove function which got changed to void return type. Please check if you
>> have this patch [1] in your tree which got recently merged.
> 
> No that patch is not part of the tree yet.
> 
> Please note also the following section in that patch:
> 

I think the said patch removes below comment.

>      /*
>      * Traditionally the remove callback returned an int which however is
>      * ignored by the driver core. This led to wrong expectations by driver
>      * authors who thought returning an error code was a valid error
>      * handling strategy. To convert to a callback returning void, new
>      * drivers should implement .remove_new() until the conversion it done
>      * that eventually makes .remove() return void.
> 
> So your driver is expected to implement remove_new().
> 

If I look at the tip of linux-next (which is rc4+) [1], the above comment is
now removed and it says that new drivers are supposed to implement .remove()
only. But yeah, as media_tree doesn't have this patch it may require to have
int remove() and void remove_new() as you mention, but eventually when
media_stage tree gets synced with above patch pulled in, we may require to
update again to what is present in this patch.

> I will take care of this for the PR, but please note for the future:
> It has to work with media_stage, unless you have a very good reason for
> something different.
> 

Sure, thanks for taking care of this. Feel free to let me know if any issues
faced.

Regards
Devarsh

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/platform_device.h?h=next-20240620#n240