diff mbox series

[v2,13/13] ARM: s3c24xx: camif: include header with prototypes and unify declaration

Message ID 20200804192654.12783-14-krzk@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series clk/watchdog/ARM: Cleanup of various S3C bits | expand

Commit Message

Krzysztof Kozlowski Aug. 4, 2020, 7:26 p.m. UTC
The s3c_camif_gpio_put() declaration in
include/media/drv-intf/s3c_camif.h header was different than definition.
Fixing this allows to include that header to also fix the W=1 compile
warnings:

    arch/arm/mach-s3c24xx/setup-camif.c:28:5: warning: no previous prototype for 's3c_camif_gpio_get' [-Wmissing-prototypes]
       28 | int s3c_camif_gpio_get(void)
    arch/arm/mach-s3c24xx/setup-camif.c:56:6: warning: no previous prototype for 's3c_camif_gpio_put' [-Wmissing-prototypes]
       56 | void s3c_camif_gpio_put(void)

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v1:
1. New patch
---
 arch/arm/mach-s3c24xx/setup-camif.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Stephen Boyd Aug. 12, 2020, 7:59 a.m. UTC | #1
Quoting Krzysztof Kozlowski (2020-08-04 12:26:54)
> The s3c_camif_gpio_put() declaration in
> include/media/drv-intf/s3c_camif.h header was different than definition.
> Fixing this allows to include that header to also fix the W=1 compile
> warnings:
> 
>     arch/arm/mach-s3c24xx/setup-camif.c:28:5: warning: no previous prototype for 's3c_camif_gpio_get' [-Wmissing-prototypes]
>        28 | int s3c_camif_gpio_get(void)
>     arch/arm/mach-s3c24xx/setup-camif.c:56:6: warning: no previous prototype for 's3c_camif_gpio_put' [-Wmissing-prototypes]
>        56 | void s3c_camif_gpio_put(void)

Maybe it should have been void all along?
Arnd Bergmann Aug. 12, 2020, 9:14 a.m. UTC | #2
On Wed, Aug 12, 2020 at 9:59 AM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Krzysztof Kozlowski (2020-08-04 12:26:54)
> > The s3c_camif_gpio_put() declaration in
> > include/media/drv-intf/s3c_camif.h header was different than definition.
> > Fixing this allows to include that header to also fix the W=1 compile
> > warnings:
> >
> >     arch/arm/mach-s3c24xx/setup-camif.c:28:5: warning: no previous prototype for 's3c_camif_gpio_get' [-Wmissing-prototypes]
> >        28 | int s3c_camif_gpio_get(void)
> >     arch/arm/mach-s3c24xx/setup-camif.c:56:6: warning: no previous prototype for 's3c_camif_gpio_put' [-Wmissing-prototypes]
> >        56 | void s3c_camif_gpio_put(void)
>
> Maybe it should have been void all along?

It seems there have never been any callers and the entire file
can just be removed, with the rest of that platform_data header
file moved to drivers/media/platform/s3c-camif/.

     Arnd
On 12.08.2020 11:14, Arnd Bergmann wrote:
> On Wed, Aug 12, 2020 at 9:59 AM Stephen Boyd <sboyd@kernel.org> wrote:
>> Quoting Krzysztof Kozlowski (2020-08-04 12:26:54)
>>> The s3c_camif_gpio_put() declaration in
>>> include/media/drv-intf/s3c_camif.h header was different than definition.
>>> Fixing this allows to include that header to also fix the W=1 compile
>>> warnings:
>>>
>>>     arch/arm/mach-s3c24xx/setup-camif.c:28:5: warning: no previous prototype for 's3c_camif_gpio_get' [-Wmissing-prototypes]
>>>        28 | int s3c_camif_gpio_get(void)
>>>     arch/arm/mach-s3c24xx/setup-camif.c:56:6: warning: no previous prototype for 's3c_camif_gpio_put' [-Wmissing-prototypes]
>>>        56 | void s3c_camif_gpio_put(void)
>>
>> Maybe it should have been void all along?
> 
> It seems there have never been any callers and the entire file
> can just be removed, with the rest of that platform_data header
> file moved to drivers/media/platform/s3c-camif/.

Yes, it seems that patch never made it to mainline:
https://git.linuxtv.org/snawrocki/media.git/commit/?h=testing/s3c-camif&id=355cbf835aff2aabf78390931cbbaa608af77967

I doubt there are still users of camera on the s3c2440 boards
with current mainline kernels, if any at all, there are much
better HW alternatives right now.
Arnd Bergmann Aug. 12, 2020, 11:28 a.m. UTC | #4
On Wed, Aug 12, 2020 at 12:46 PM Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 12.08.2020 11:14, Arnd Bergmann wrote:
> >
> > It seems there have never been any callers and the entire file
> > can just be removed, with the rest of that platform_data header
> > file moved to drivers/media/platform/s3c-camif/.
>
> Yes, it seems that patch never made it to mainline:
> https://git.linuxtv.org/snawrocki/media.git/commit/?h=testing/s3c-camif&id=355cbf835aff2aabf78390931cbbaa608af77967

I suppose it would still apply if anyone was interested, but I see your
point.

> I doubt there are still users of camera on the s3c2440 boards
> with current mainline kernels, if any at all, there are much
> better HW alternatives right now.

I see two board files (and no DT) instantiate the camif device:
NexVision Nexcoder 2440 and the FriendlyARM mini2440.

Can you say whether the camif on those would actually work
at all without your patch? If not, we know that there are no
users of that driver and could either drop it completely or move
it to staging for a release or two.

     Arnd
On 12.08.2020 13:28, Arnd Bergmann wrote:
> On Wed, Aug 12, 2020 at 12:46 PM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 12.08.2020 11:14, Arnd Bergmann wrote:
>>>
>>> It seems there have never been any callers and the entire file
>>> can just be removed, with the rest of that platform_data header
>>> file moved to drivers/media/platform/s3c-camif/.
>>
>> Yes, it seems that patch never made it to mainline:
>> https://protect2.fireeye.com/v1/url?k=abe5f73a-f6293cfe-abe47c75-0cc47a314e9a-7fafe832d055d852&q=1&e=2596ffb6-c4cb-492a-8c6f-a0e261567842&u=https%3A%2F%2Fgit.linuxtv.org%2Fsnawrocki%2Fmedia.git%2Fcommit%2F%3Fh%3Dtesting%2Fs3c-camif%26id%3D355cbf835aff2aabf78390931cbbaa608af77967
> 
> I suppose it would still apply if anyone was interested, but I see your
> point.
> 
>> I doubt there are still users of camera on the s3c2440 boards
>> with current mainline kernels, if any at all, there are much
>> better HW alternatives right now.
> 
> I see two board files (and no DT) instantiate the camif device:
> NexVision Nexcoder 2440 and the FriendlyARM mini2440.
> 
> Can you say whether the camif on those would actually work
> at all without your patch? If not, we know that there are no
> users of that driver and could either drop it completely or move
> it to staging for a release or two.

Without additional patches the camif will not work, the driver 
needs an instance of struct s3c_camif_plat_data which specifies
what image sensor is attached.

I think we can drop the driver, together with the s3c_camif_device
platform device definitions. It can always be added again if anyone
ever needs it or converts the platform to DT.

IMO all non-DT code in arch/arm/mach-s3c24xx is a candidate for
removal, it just adds to the maintenance effort and I seriously
doubt there are now any users of it.
Krzysztof Kozlowski Aug. 12, 2020, 1:31 p.m. UTC | #6
On Wed, Aug 12, 2020 at 03:11:41PM +0200, Sylwester Nawrocki wrote:
> On 12.08.2020 13:28, Arnd Bergmann wrote:
> > On Wed, Aug 12, 2020 at 12:46 PM Sylwester Nawrocki
> > <s.nawrocki@samsung.com> wrote:
> >> On 12.08.2020 11:14, Arnd Bergmann wrote:
> >>>
> >>> It seems there have never been any callers and the entire file
> >>> can just be removed, with the rest of that platform_data header
> >>> file moved to drivers/media/platform/s3c-camif/.
> >>
> >> Yes, it seems that patch never made it to mainline:
> >> https://protect2.fireeye.com/v1/url?k=abe5f73a-f6293cfe-abe47c75-0cc47a314e9a-7fafe832d055d852&q=1&e=2596ffb6-c4cb-492a-8c6f-a0e261567842&u=https%3A%2F%2Fgit.linuxtv.org%2Fsnawrocki%2Fmedia.git%2Fcommit%2F%3Fh%3Dtesting%2Fs3c-camif%26id%3D355cbf835aff2aabf78390931cbbaa608af77967
> > 
> > I suppose it would still apply if anyone was interested, but I see your
> > point.
> > 
> >> I doubt there are still users of camera on the s3c2440 boards
> >> with current mainline kernels, if any at all, there are much
> >> better HW alternatives right now.
> > 
> > I see two board files (and no DT) instantiate the camif device:
> > NexVision Nexcoder 2440 and the FriendlyARM mini2440.
> > 
> > Can you say whether the camif on those would actually work
> > at all without your patch? If not, we know that there are no
> > users of that driver and could either drop it completely or move
> > it to staging for a release or two.
> 
> Without additional patches the camif will not work, the driver 
> needs an instance of struct s3c_camif_plat_data which specifies
> what image sensor is attached.
> 
> I think we can drop the driver, together with the s3c_camif_device
> platform device definitions. It can always be added again if anyone
> ever needs it or converts the platform to DT.

Since the header was in /include/media I assumed there might be some
user-space tools using it. But if it is not the case, I'll drop the code
then.

 
> IMO all non-DT code in arch/arm/mach-s3c24xx is a candidate for
> removal, it just adds to the maintenance effort and I seriously
> doubt there are now any users of it.

That is quite tricky... I really do not know whether there are any real
world users of S3C24xx and S3C64xx platforms. Evalkits are mostly not
available for buying so I do not expect new designs. However still
existing ones might be somewhere... Few years ago, back in Samsung, I
mentioned removing them. That time I think Marek or you Sylwester, said
that there are industrial applications using S3C24xx. I believe, why
not. The trouble is - how to find such users? How to get in touch for
testing or at least for bug reports if something is broken?

Or even more important - is it worth to spend effort and time on this?
If there is no single production system using recent Linux kernel, the
answer should be negative...

Best regards,
Krzysztof
On 12.08.2020 15:31, Krzysztof Kozlowski wrote:
> On Wed, Aug 12, 2020 at 03:11:41PM +0200, Sylwester Nawrocki wrote:
>> On 12.08.2020 13:28, Arnd Bergmann wrote:
>>> On Wed, Aug 12, 2020 at 12:46 PM Sylwester Nawrocki
>>> <s.nawrocki@samsung.com> wrote:
>>>> On 12.08.2020 11:14, Arnd Bergmann wrote:

>>> I see two board files (and no DT) instantiate the camif device:
>>> NexVision Nexcoder 2440 and the FriendlyARM mini2440.
>>>
>>> Can you say whether the camif on those would actually work
>>> at all without your patch? If not, we know that there are no
>>> users of that driver and could either drop it completely or move
>>> it to staging for a release or two.
>>
>> Without additional patches the camif will not work, the driver 
>> needs an instance of struct s3c_camif_plat_data which specifies
>> what image sensor is attached.
>>
>> I think we can drop the driver, together with the s3c_camif_device
>> platform device definitions. It can always be added again if anyone
>> ever needs it or converts the platform to DT.
> 
> Since the header was in /include/media I assumed there might be some
> user-space tools using it. But if it is not the case, I'll drop the code
> then.

That's a kernel internal header, only for board files, it should really 
have been added to include/linux/platform_data.
  
>> IMO all non-DT code in arch/arm/mach-s3c24xx is a candidate for
>> removal, it just adds to the maintenance effort and I seriously
>> doubt there are now any users of it.
> 
> That is quite tricky... I really do not know whether there are any real
> world users of S3C24xx and S3C64xx platforms. Evalkits are mostly not
> available for buying so I do not expect new designs. However still
> existing ones might be somewhere... Few years ago, back in Samsung, I
> mentioned removing them. That time I think Marek or you Sylwester, said
> that there are industrial applications using S3C24xx. I believe, why
> not. The trouble is - how to find such users? How to get in touch for
> testing or at least for bug reports if something is broken?

I believe if there any such applications of the S3C24XX SoCs still existing 
somewhere their long term support doesn't include updating to new kernels. 
I used to keep a running S3C2440 SoC based board just for the purpose of
testing patches touching the common code, but I stopped it, I think it is
not worth to waste time and health on it any more. For example support for 
the OSELAS.BSP-Pengutronix-Mini2440 BSP I used for tests ended 5 years ago
[1].

> Or even more important - is it worth to spend effort and time on this?
> If there is no single production system using recent Linux kernel, the
> answer should be negative...

I suspect nobody cares about that code (non-DT s3c24xx) any more for other
than sentimental reasons.

[1] https://git.pengutronix.de/cgit/OSELAS.BSP-Pengutronix-Mini2440
Krzysztof Kozlowski Aug. 12, 2020, 5:13 p.m. UTC | #8
On Wed, Aug 12, 2020 at 05:58:52PM +0200, Sylwester Nawrocki wrote:
> On 12.08.2020 15:31, Krzysztof Kozlowski wrote:
> > On Wed, Aug 12, 2020 at 03:11:41PM +0200, Sylwester Nawrocki wrote:
> >> On 12.08.2020 13:28, Arnd Bergmann wrote:
> >>> On Wed, Aug 12, 2020 at 12:46 PM Sylwester Nawrocki
> >>> <s.nawrocki@samsung.com> wrote:
> >>>> On 12.08.2020 11:14, Arnd Bergmann wrote:
> 
> >>> I see two board files (and no DT) instantiate the camif device:
> >>> NexVision Nexcoder 2440 and the FriendlyARM mini2440.
> >>>
> >>> Can you say whether the camif on those would actually work
> >>> at all without your patch? If not, we know that there are no
> >>> users of that driver and could either drop it completely or move
> >>> it to staging for a release or two.
> >>
> >> Without additional patches the camif will not work, the driver 
> >> needs an instance of struct s3c_camif_plat_data which specifies
> >> what image sensor is attached.
> >>
> >> I think we can drop the driver, together with the s3c_camif_device
> >> platform device definitions. It can always be added again if anyone
> >> ever needs it or converts the platform to DT.
> > 
> > Since the header was in /include/media I assumed there might be some
> > user-space tools using it. But if it is not the case, I'll drop the code
> > then.
> 
> That's a kernel internal header, only for board files, it should really 
> have been added to include/linux/platform_data.
>   
> >> IMO all non-DT code in arch/arm/mach-s3c24xx is a candidate for
> >> removal, it just adds to the maintenance effort and I seriously
> >> doubt there are now any users of it.
> > 
> > That is quite tricky... I really do not know whether there are any real
> > world users of S3C24xx and S3C64xx platforms. Evalkits are mostly not
> > available for buying so I do not expect new designs. However still
> > existing ones might be somewhere... Few years ago, back in Samsung, I
> > mentioned removing them. That time I think Marek or you Sylwester, said
> > that there are industrial applications using S3C24xx. I believe, why
> > not. The trouble is - how to find such users? How to get in touch for
> > testing or at least for bug reports if something is broken?
> 
> I believe if there any such applications of the S3C24XX SoCs still existing 
> somewhere their long term support doesn't include updating to new kernels. 
> I used to keep a running S3C2440 SoC based board just for the purpose of
> testing patches touching the common code, but I stopped it, I think it is
> not worth to waste time and health on it any more. For example support for 
> the OSELAS.BSP-Pengutronix-Mini2440 BSP I used for tests ended 5 years ago
> [1].
> 
> > Or even more important - is it worth to spend effort and time on this?
> > If there is no single production system using recent Linux kernel, the
> > answer should be negative...
> 
> I suspect nobody cares about that code (non-DT s3c24xx) any more for other
> than sentimental reasons.

I'll start then with S3C camif driver. :) I guess
drivers/media/platform/s3c-camif/ still should be left?

Best regards,
Krzysztof
On 12.08.2020 19:13, Krzysztof Kozlowski wrote:
> I'll start then with S3C camif driver. :) I guess
> drivers/media/platform/s3c-camif/ still should be left?

No, if you want to remove the driver then this whole directory
should be removed. The driver also supports the S3C6410 CAMIF
but there is no s3c6410-camif devices instantiated in 
arch/arm/mach-s3c64xx either.
Cedric Roux Aug. 27, 2020, 8:52 p.m. UTC | #10
On 8/12/20 3:31 PM, Krzysztof Kozlowski wrote:
> Or even more important - is it worth to spend effort and time on this?
> If there is no single production system using recent Linux kernel, the
> answer should be negative...

Well, I have a server running on mini2440 with a not-too-young
but not-too-old kernel. I don't have much time to test recent
kernels though so I guess that doesn't count.
Krzysztof Kozlowski Sept. 6, 2020, 3:41 p.m. UTC | #11
On Thu, 27 Aug 2020 at 22:51, Cedric Roux <sed@free.fr> wrote:
>
> On 8/12/20 3:31 PM, Krzysztof Kozlowski wrote:
> > Or even more important - is it worth to spend effort and time on this?
> > If there is no single production system using recent Linux kernel, the
> > answer should be negative...
>
> Well, I have a server running on mini2440 with a not-too-young
> but not-too-old kernel. I don't have much time to test recent
> kernels though so I guess that doesn't count.

Actually good to hear. It counts a little bit :)

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm/mach-s3c24xx/setup-camif.c b/arch/arm/mach-s3c24xx/setup-camif.c
index 2b262fae3f61..4046afaad645 100644
--- a/arch/arm/mach-s3c24xx/setup-camif.c
+++ b/arch/arm/mach-s3c24xx/setup-camif.c
@@ -7,6 +7,7 @@ 
 #include <linux/gpio.h>
 #include <plat/gpio-cfg.h>
 #include <mach/gpio-samsung.h>
+#include <media/drv-intf/s3c_camif.h>
 
 /* Number of camera port pins, without FIELD */
 #define S3C_CAMIF_NUM_GPIOS	13
@@ -53,7 +54,7 @@  int s3c_camif_gpio_get(void)
 	return 0;
 }
 
-void s3c_camif_gpio_put(void)
+int s3c_camif_gpio_put(void)
 {
 	int i, gpio_start, gpio_reset;
 
@@ -64,4 +65,6 @@  void s3c_camif_gpio_put(void)
 		if (gpio != gpio_reset)
 			gpio_free(gpio);
 	}
+
+	return 0;
 }