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

From: Devarsh Thakkar <devarsht@ti.com>
To: Sebastian Fricke <sebastian.fricke@collabora.com>
Cc: <mchehab@kernel.org>, <robh@kernel.org>, <krzk+dt@kernel.org>,
	<conor+dt@kernel.org>, <hverkuil-cisco@xs4all.nl>,
	<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-rockchip@lists.infradead.org>,
	<benjamin.gaignard@collabora.com>,
	<laurent.pinchart@ideasonboard.com>, <praneeth@ti.com>,
	<nm@ti.com>, <vigneshr@ti.com>, <a-bhatia1@ti.com>,
	<j-luthra@ti.com>, <b-brnich@ti.com>, <detheridge@ti.com>,
	<p-mantena@ti.com>, <vijayp@ti.com>, <andrzej.p@collabora.com>,
	<nicolas@ndufresne.ca>, <afd@ti.com>
Subject: Re: [PATCH v14 2/6] media: imagination: Add E5010 JPEG Encoder driver
Date: Fri, 21 Jun 2024 19:38:16 +0530	[thread overview]
Message-ID: <8c10a338-c2c2-64b8-40db-1cbc20822c39@ti.com> (raw)
In-Reply-To: <20240621132658.m2utqnvwaicgiwqr@basti-XPS-13-9310>

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip