Re: [PATCH] media: atmel: atmel-isc: split driver into driver base and isc

[Hans Verkuil]

On 5/9/19 11:09 AM, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> 
> This splits the Atmel ISC driver into a common base: atmel-isc-base.c
> and the driver probe/dt part , atmel-isc.c
> This is needed to keep a common ground for the sensor controller which will
> be reused.
> The atmel-isc will use the common symbols inside the atmel-isc-base
> Future driver will also use the same symbols and redefine different aspects,
> for a different version of the ISC.
> This is done to avoid complete code duplication by creating a totally
> different driver for the new variant of the ISC.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> ---
> 
> Hello,
> 
> This patch applies on top of my other patches:
> 
> media: atmel: atmel-isc: make try_fmt error less verbose
> media: atmel: atmel-isc: add support for DO_WHITE_BALANCE
> media: atmel: atmel-isc: reworked white balance feature
> 
> Thanks !
> 
>  MAINTAINERS                                   |    2 +
>  drivers/media/platform/atmel/Makefile         |    1 +
>  drivers/media/platform/atmel/atmel-isc-base.c | 2144 +++++++++++++++++++++++
>  drivers/media/platform/atmel/atmel-isc.c      | 2311 +------------------------
>  drivers/media/platform/atmel/atmel-isc.h      |  210 +++
>  5 files changed, 2372 insertions(+), 2296 deletions(-)
>  create mode 100644 drivers/media/platform/atmel/atmel-isc-base.c
>  create mode 100644 drivers/media/platform/atmel/atmel-isc.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 878588c..8799779 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10110,6 +10110,8 @@ M:	Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>  L:	linux-media@xxxxxxxxxxxxxxx
>  S:	Supported
>  F:	drivers/media/platform/atmel/atmel-isc.c
> +F:	drivers/media/platform/atmel/atmel-isc.h
> +F:	drivers/media/platform/atmel/atmel-isc-base.c
>  F:	drivers/media/platform/atmel/atmel-isc-regs.h
>  F:	Documentation/devicetree/bindings/media/atmel-isc.txt
>  
> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
> index 27000d0..9c8566e 100644
> --- a/drivers/media/platform/atmel/Makefile
> +++ b/drivers/media/platform/atmel/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>  obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
> +obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc-base.o

This doesn't work. If a module consists of two sources, then you must make sure
those sources do not have the same name as the module.

So you have to rename atmel-isc.c.

See e.g. drivers/media/platform/rcar-vin/Makefile on how that's done for
CONFIG_VIDEO_RCAR_VIN.

<snip>

> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
> new file mode 100644
> index 0000000..1e285f1
> --- /dev/null
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -0,0 +1,210 @@

<snip>

> +
> +#if defined(CONFIG_VIDEO_ATMEL_ISC)

Why this #if???

> +#define ATMEL_ISC_NAME "atmel-isc"
> +#endif
> +
> +#endif
> 

If I configure atmel-isc as a module, then I get these errors:

  CC [M]  drivers/media/platform/atmel/atmel-isc-base.o
drivers/media/platform/atmel/atmel-isc-base.c: In function âisc_querycapâ:
drivers/media/platform/atmel/atmel-isc-base.c:906:23: error: âATMEL_ISC_NAMEâ undeclared (first use in this function); did you mean âCTL_MAXNAMEâ?

  strscpy(cap->driver, ATMEL_ISC_NAME, sizeof(cap->driver));
                       ^~~~~~~~~~~~~~
                       CTL_MAXNAME
drivers/media/platform/atmel/atmel-isc-base.c:906:23: note: each undeclared identifier is reported only once for each function it appears in

drivers/media/platform/atmel/atmel-isc-base.c: In function âisc_async_completeâ:
drivers/media/platform/atmel/atmel-isc-base.c:2063:22: error: âATMEL_ISC_NAMEâ undeclared (first use in this function); did you mean âCTL_MAXNAMEâ?

  strscpy(vdev->name, ATMEL_ISC_NAME, sizeof(vdev->name));
                      ^~~~~~~~~~~~~~
                      CTL_MAXNAME
drivers/media/platform/atmel/atmel-isc.c: In function âatmel_isc_probeâ:
drivers/media/platform/atmel/atmel-isc.c:180:11: error: âATMEL_ISC_NAMEâ undeclared (first use in this function); did you mean âCTL_MAXNAMEâ?

           ATMEL_ISC_NAME, isc);
           ^~~~~~~~~~~~~~
           CTL_MAXNAME

Probably related to one or both of my comments above.

Regards,

	Hans