Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
HI Arnd,
Em Mon, 9 Apr 2018 10:50:13 +0200
Arnd Bergmann escreveu:
> >> > That hardly seems to be an arch-specific iommu solution, but, instead,
> >> > some
> >> > hack used by only three drivers or some legacy iommu binding.
> >>
> >> It's more complex than that. There are multiple IOMMU-related APIs on ARM,
> >> so
> >> more recent than others, with different feature sets. While I agree that
> >> drivers should move away from arm_iommu_create_mapping(), doing so requires
> >> coordination between the IOMMU driver and the bus master driver (for
> >> instance
> >> the omap3isp driver). It's not a trivial matter, but I'd love if someone
> >> submitted patches :-)
> >
> > If someone steps up to do that, it would be really helpful, but we
> > should not trust that this will happen. OMAP3 is an old hardware,
> > and not many developers are working on improving its support.
>
> Considering its age, I still see a lot of changes on the arch/arm side of
> it, so I wouldn't give up the hope yet.
Yeah, someone might still work on such fix.
> > Arnd,
> >
> > What do you think?
>
> I think including a foreign architecture header is worse than your
> earlier patch, I'd rather see a local hack in the driver.
>
> I haven't tried it, but how about something simpler like what
> I have below.
Actually, another #ifdef was needed, before include arch-specifi
header :-)
>
> Arnd
>
> (in case it works and you want to pick it up with a proper
> changelog):
>
> Signed-off-by: Arnd Bergmann
Sounds a reasonable approach. Instead of using CONFIG_ARM, I would,
instead check for CONFIG_ARM_DMA_USE_IOMMU, with is the actual
dependency for such code, as otherwise it could cause some
compilation breakages on ARM with COMPILE_TEST and some randconfig.
An advantage is that it properly annotates the part of the code
that depends on ARM_DMA_USE_IOMMU.
Thanks,
Mauro
From: Arnd Bergmann
media: omap3isp: allow it to build with COMPILE_TEST
There aren't much things required for it to build with COMPILE_TEST.
It just needs to not compile the code that depends on arm-specific
iommu implementation.
Co-developed-by: Mauro Carvalho Chehab
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 1ee915b794c0..2757b621091c 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -63,12 +63,10 @@ config VIDEO_MUX
config VIDEO_OMAP3
tristate "OMAP 3 Camera support"
depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
- depends on ARCH_OMAP3 || COMPILE_TEST
- depends on ARM
+ depends on ((ARCH_OMAP3 && OMAP_IOMMU) || COMPILE_TEST)
depends on COMMON_CLK
depends on HAS_DMA && OF
- depends on OMAP_IOMMU
- select ARM_DMA_USE_IOMMU
+ select ARM_DMA_USE_IOMMU if OMAP_IOMMU
select VIDEOBUF2_DMA_CONTIG
select MFD_SYSCON
select V4L2_FWNODE
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 16c50099cccd..b8c8761a76b6 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -61,7 +61,9 @@
#include
#include
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
#include
+#endif
#include
#include
@@ -1938,12 +1940,15 @@ static int isp_initialize_modules(struct isp_device
*isp)
static void isp_detach_iommu(struct isp_device *isp)
{
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
arm_iommu_release_mapping(isp->mapping);
isp->mapping = NULL;
+#endif
}
static int isp_attach_iommu(struct isp_device *isp)
{
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
struct dma_iommu_mapping *mapping;
int ret;
@@ -1972,6 +1977,9 @@ static int isp_attach_iommu(struct isp_device *isp)
error:
isp_detach_iommu(isp);
return ret;
+#else
+ return -ENODEV;
+#endif
}
/*
Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
On Sat, Apr 7, 2018 at 3:14 PM, Mauro Carvalho Chehab
wrote:
> Em Sat, 07 Apr 2018 14:56:59 +0300
> Laurent Pinchart escreveu:
>> On Thursday, 5 April 2018 22:44:44 EEST Mauro Carvalho Chehab wrote:
>> > Em Thu, 05 Apr 2018 21:30:27 +0300 Laurent Pinchart escreveu:
>> > > If you want to make drivers compile for all architectures, the APIs they
>> > > use must be defined in linux/, not in asm/. They can be stubbed there
>> > > when not implemented in a particular architecture, but not in the driver.
>> >
>> > In this specific case, the same approach taken here is already needed
>> > by the Renesas VMSA-compatible IPMMU driver, with, btw, is inside
>> > drivers/iommu:
>> >
>> > drivers/iommu/ipmmu-vmsa.c
>>
>> The reason there is different, the driver is shared by ARM32 and ARM64
>> platforms. Furthermore, there's an effort (or at least there was) to move
>> away
>> from those APIs in the driver, but I think it has stalled.
>
> Anyway, the approach sticks at the driver. As this was accepted even
> inside drivers/iommu, I fail to see why not doing the same on media,
> specially since it really helps us to find bugs at omap3isp driver.
>
> Even without pushing upstream (where Coverity would analyze it),
> we got lots of problems by simply letting omap3isp to use the
> usual tools we use for all other drivers.
>
>> > Also, this API is used only by 3 drivers [1]:
>> >
>> > drivers/iommu/ipmmu-vmsa.c
>> > drivers/iommu/mtk_iommu_v1.c
>> > drivers/media/platform/omap3isp/isp.c
>> >
>> > [1] as blamed by
>> > git grep -l arm_iommu_create_mapping
>>
>> The exynos driver also uses it.
>>
>> > That hardly seems to be an arch-specific iommu solution, but, instead, some
>> > hack used by only three drivers or some legacy iommu binding.
>>
>> It's more complex than that. There are multiple IOMMU-related APIs on ARM, so
>> more recent than others, with different feature sets. While I agree that
>> drivers should move away from arm_iommu_create_mapping(), doing so requires
>> coordination between the IOMMU driver and the bus master driver (for instance
>> the omap3isp driver). It's not a trivial matter, but I'd love if someone
>> submitted patches :-)
>
> If someone steps up to do that, it would be really helpful, but we
> should not trust that this will happen. OMAP3 is an old hardware,
> and not many developers are working on improving its support.
Considering its age, I still see a lot of changes on the arch/arm side of
it, so I wouldn't give up the hope yet.
>> > The omap3isp is, btw, the only driver outside drivers/iommu that needs it.
>> >
>> > So, it sounds that other driver uses some other approach, but hardly
>> > it would be worth to change this driver to use something else.
>> >
>> > So, better to stick with the same solution the Renesas driver used.
>>
>> I'm not responsible for that solution and I didn't think it was a good one at
>> the time it was introduced, but in any case it is not meant at all to support
>> COMPILE_TEST. I still don't think the omap3isp driver should declare stubs
>> for
>> these functions for the purpose of supporting compile-testing on x86.
>
> Well, there is another alternative. We could do add this to its Makefile:
>
> ifndef CONFIG_ARCH_ARM
> ccflags-y += -I./arch/arm/include/
> endif
>
> And add those stubs to arch/arm/include/asm/dma-iommu.h,
> in order to be used when CONFIG_IOMMU_DMA isn't defined:
>
> #define arm_iommu_create_mapping(...) NULL
> #define arm_iommu_attach_device(...)-ENODEV
> #define arm_iommu_release_mapping(...) do {} while (0)
> #define arm_iommu_detach_device(...)do {} while (0)
>
> If done right, such solution could also be used to remove
> the #ifdef inside drivers/iommu/ipmmu-vmsa.c.
>
> Yet, I think that the approach I proposed before is better,
> but maybe arm maintainers may have a different opinion.
>
> Arnd,
>
> What do you think?
I think including a foreign architecture header is worse than your
earlier patch, I'd rather see a local hack in the driver.
I haven't tried it, but how about something simpler like what
I have below.
Arnd
(in case it works and you want to pick it up with a proper
changelog):
Signed-off-by: Arnd Bergmann
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..625f2e407929 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,12 +1945,15 @@ static int isp_initialize_modules(struct
isp_device *isp)
static void isp_detach_iommu(struct isp_device *isp)
{
+#ifdef CONFIG_ARM
arm_iommu_release_mapping(isp->mapping);
isp->mapping = NULL;
+#endif
}
static int isp_attach_iommu(struct isp_device *isp)
{
+#ifdef CONFIG_ARM
struct dma_iommu_mapping *mapping;
int ret;
@@ -1979,6 +1982,9 @@ static int isp_attach_iommu(struct isp_device *isp)
error:
isp_detach_iommu(isp);
return ret;
+#else
+ return -ENODEV;
+#endif
}
/*
Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
Em Sat, 07 Apr 2018 14:56:59 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Thursday, 5 April 2018 22:44:44 EEST Mauro Carvalho Chehab wrote:
> > Em Thu, 05 Apr 2018 21:30:27 +0300 Laurent Pinchart escreveu:
> > > On Thursday, 5 April 2018 20:54:02 EEST Mauro Carvalho Chehab wrote:
> > > > There aren't much things required for it to build with COMPILE_TEST.
> > > > It just needs to provide stub for an arm-dependent include.
> > > >
> > > > Let's replicate the same solution used by ipmmu-vmsa, in order
> > > > to allow building omap3 with COMPILE_TEST.
> > > >
> > > > The actual logic here came from this driver:
> > > >drivers/iommu/ipmmu-vmsa.c
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab
> > > > ---
> > > >
> > > > drivers/media/platform/Kconfig| 8
> > > > drivers/media/platform/omap3isp/isp.c | 7 +++
> > > > 2 files changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/Kconfig
> > > > b/drivers/media/platform/Kconfig index c7a1cf8a1b01..03c9dfeb7781
> > > > 100644
> > > > --- a/drivers/media/platform/Kconfig
> > > > +++ b/drivers/media/platform/Kconfig
> > > > @@ -62,12 +62,12 @@ config VIDEO_MUX
> > > >
> > > > config VIDEO_OMAP3
> > > > tristate "OMAP 3 Camera support"
> > > > - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API &&
> > > > ARCH_OMAP3
> > > > + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> > > > depends on HAS_DMA && OF
> > > > - depends on OMAP_IOMMU
> > > > - select ARM_DMA_USE_IOMMU
> > > > + depends on ((ARCH_OMAP3 && OMAP_IOMMU) || COMPILE_TEST)
> > > > + select ARM_DMA_USE_IOMMU if OMAP_IOMMU
> > > > select VIDEOBUF2_DMA_CONTIG
> > > > - select MFD_SYSCON
> > > > + select MFD_SYSCON if ARCH_OMAP3
> > > > select V4L2_FWNODE
> > > > ---help---
> > > > Driver for an OMAP 3 camera controller.
> > > > diff --git a/drivers/media/platform/omap3isp/isp.c
> > > > b/drivers/media/platform/omap3isp/isp.c index 8eb000e3d8fd..2a11a709aa4f
> > > > 100644
> > > > --- a/drivers/media/platform/omap3isp/isp.c
> > > > +++ b/drivers/media/platform/omap3isp/isp.c
> > > > @@ -61,7 +61,14 @@
> > > > #include
> > > > #include
> > > >
> > > > +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> > > > #include
> > > > +#else
> > > > +#define arm_iommu_create_mapping(...) NULL
> > > > +#define arm_iommu_attach_device(...) -ENODEV
> > > > +#define arm_iommu_release_mapping(...) do {} while (0)
> > > > +#define arm_iommu_detach_device(...) do {} while (0)
> > > > +#endif
> > >
> > > I don't think it's the job of a driver to define those stubs, sorry.
> > > Otherwise where do you stop ? If you have half of the code that is
> > > architecture- dependent, would you stub it ? And what if the stubs you
> > > define here generate warnings in static analyzers ?
> >
> > I agree that we should avoid doing that as a general case, but see
> > below.
> >
> > > If you want to make drivers compile for all architectures, the APIs they
> > > use must be defined in linux/, not in asm/. They can be stubbed there
> > > when not implemented in a particular architecture, but not in the driver.
> > >
> >
> > In this specific case, the same approach taken here is already needed
> > by the Renesas VMSA-compatible IPMMU driver, with, btw, is inside
> > drivers/iommu:
> >
> > drivers/iommu/ipmmu-vmsa.c
>
> The reason there is different, the driver is shared by ARM32 and ARM64
> platforms. Furthermore, there's an effort (or at least there was) to move
> away
> from those APIs in the driver, but I think it has stalled.
Anyway, the approach sticks at the driver. As this was accepted even
inside drivers/iommu, I fail to see why not doing the same on media,
specially since it really helps us to find bugs at omap3isp driver.
Even without pushing upstream (where Coverity would analyze it),
we got lots of problems by simply letting omap3isp to use the
usual tools we use for all other drivers.
> > Also, this API is used only by 3 drivers [1]:
> >
> > drivers/iommu/ipmmu-vmsa.c
> > drivers/iommu/mtk_iommu_v1.c
> > drivers/media/platform/omap3isp/isp.c
> >
> > [1] as blamed by
> > git grep -l arm_iommu_create_mapping
>
> The exynos driver also uses it.
>
> > That hardly seems to be an arch-specific iommu solution, but, instead, some
> > hack used by only three drivers or some legacy iommu binding.
>
> It's more complex than that. There are multiple IOMMU-related APIs on ARM, so
> more recent than others, with different feature sets. While I agree that
> drivers should move away from arm_iommu_create_mapping(), doing so requires
> coordination between the IOMMU driver and the bus master driver (for instance
> the omap3isp driver). It's not a trivial matter, but I'd love if someone
> submitted patches :-)
If someone steps up to do that, it would be really helpful, but we
Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
Hi Mauro,
On Thursday, 5 April 2018 22:44:44 EEST Mauro Carvalho Chehab wrote:
> Em Thu, 05 Apr 2018 21:30:27 +0300 Laurent Pinchart escreveu:
> > On Thursday, 5 April 2018 20:54:02 EEST Mauro Carvalho Chehab wrote:
> > > There aren't much things required for it to build with COMPILE_TEST.
> > > It just needs to provide stub for an arm-dependent include.
> > >
> > > Let's replicate the same solution used by ipmmu-vmsa, in order
> > > to allow building omap3 with COMPILE_TEST.
> > >
> > > The actual logic here came from this driver:
> > >drivers/iommu/ipmmu-vmsa.c
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > >
> > > drivers/media/platform/Kconfig| 8
> > > drivers/media/platform/omap3isp/isp.c | 7 +++
> > > 2 files changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/Kconfig
> > > b/drivers/media/platform/Kconfig index c7a1cf8a1b01..03c9dfeb7781
> > > 100644
> > > --- a/drivers/media/platform/Kconfig
> > > +++ b/drivers/media/platform/Kconfig
> > > @@ -62,12 +62,12 @@ config VIDEO_MUX
> > >
> > > config VIDEO_OMAP3
> > > tristate "OMAP 3 Camera support"
> > > - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> > > + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> > > depends on HAS_DMA && OF
> > > - depends on OMAP_IOMMU
> > > - select ARM_DMA_USE_IOMMU
> > > + depends on ((ARCH_OMAP3 && OMAP_IOMMU) || COMPILE_TEST)
> > > + select ARM_DMA_USE_IOMMU if OMAP_IOMMU
> > > select VIDEOBUF2_DMA_CONTIG
> > > - select MFD_SYSCON
> > > + select MFD_SYSCON if ARCH_OMAP3
> > > select V4L2_FWNODE
> > > ---help---
> > > Driver for an OMAP 3 camera controller.
> > > diff --git a/drivers/media/platform/omap3isp/isp.c
> > > b/drivers/media/platform/omap3isp/isp.c index 8eb000e3d8fd..2a11a709aa4f
> > > 100644
> > > --- a/drivers/media/platform/omap3isp/isp.c
> > > +++ b/drivers/media/platform/omap3isp/isp.c
> > > @@ -61,7 +61,14 @@
> > > #include
> > > #include
> > >
> > > +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> > > #include
> > > +#else
> > > +#define arm_iommu_create_mapping(...)NULL
> > > +#define arm_iommu_attach_device(...) -ENODEV
> > > +#define arm_iommu_release_mapping(...) do {} while (0)
> > > +#define arm_iommu_detach_device(...) do {} while (0)
> > > +#endif
> >
> > I don't think it's the job of a driver to define those stubs, sorry.
> > Otherwise where do you stop ? If you have half of the code that is
> > architecture- dependent, would you stub it ? And what if the stubs you
> > define here generate warnings in static analyzers ?
>
> I agree that we should avoid doing that as a general case, but see
> below.
>
> > If you want to make drivers compile for all architectures, the APIs they
> > use must be defined in linux/, not in asm/. They can be stubbed there
> > when not implemented in a particular architecture, but not in the driver.
>
> In this specific case, the same approach taken here is already needed
> by the Renesas VMSA-compatible IPMMU driver, with, btw, is inside
> drivers/iommu:
>
> drivers/iommu/ipmmu-vmsa.c
The reason there is different, the driver is shared by ARM32 and ARM64
platforms. Furthermore, there's an effort (or at least there was) to move away
from those APIs in the driver, but I think it has stalled.
> Also, this API is used only by 3 drivers [1]:
>
> drivers/iommu/ipmmu-vmsa.c
> drivers/iommu/mtk_iommu_v1.c
> drivers/media/platform/omap3isp/isp.c
>
> [1] as blamed by
> git grep -l arm_iommu_create_mapping
The exynos driver also uses it.
> That hardly seems to be an arch-specific iommu solution, but, instead, some
> hack used by only three drivers or some legacy iommu binding.
It's more complex than that. There are multiple IOMMU-related APIs on ARM, so
more recent than others, with different feature sets. While I agree that
drivers should move away from arm_iommu_create_mapping(), doing so requires
coordination between the IOMMU driver and the bus master driver (for instance
the omap3isp driver). It's not a trivial matter, but I'd love if someone
submitted patches :-)
> The omap3isp is, btw, the only driver outside drivers/iommu that needs it.
>
> So, it sounds that other driver uses some other approach, but hardly
> it would be worth to change this driver to use something else.
>
> So, better to stick with the same solution the Renesas driver used.
I'm not responsible for that solution and I didn't think it was a good one at
the time it was introduced, but in any case it is not meant at all to support
COMPILE_TEST. I still don't think the omap3isp driver should declare stubs for
these functions for the purpose of supporting compile-testing on x86.
--
Regards,
Laurent Pinchart
Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
Hi Mauro,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.16 next-20180406]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/Make-all-drivers-under-drivers-media-to-build-with-COMPILE_TEST/20180406-164215
base: git://linuxtv.org/media_tree.git master
config: openrisc-allyesconfig (attached as .config)
compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc
All errors (new ones prefixed by >>):
In file included from drivers/media/platform/omap3isp/isp.c:78:0:
drivers/media/platform/omap3isp/isp.h:130:16: error: field 'hw' has
incomplete type
struct clk_hw hw;
^~
In file included from include/asm-generic/bug.h:5:0,
from ./arch/openrisc/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from arch/openrisc/include/asm/cacheflush.h:21,
from drivers/media/platform/omap3isp/isp.c:45:
drivers/media/platform/omap3isp/isp.c: In function 'isp_xclk_prepare':
include/linux/kernel.h:938:32: error: dereferencing pointer to incomplete
type 'struct clk_hw'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~
include/linux/compiler.h:316:19: note: in definition of macro
'__compiletime_assert'
bool __cond = !(condition);\
^
include/linux/compiler.h:339:2: note: in expansion of macro
'_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~
include/linux/build_bug.h:45:37: note: in expansion of macro
'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
include/linux/kernel.h:938:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~
include/linux/kernel.h:938:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~
drivers/media/platform/omap3isp/isp.c:167:26: note: in expansion of macro
'container_of'
#define to_isp_xclk(_hw) container_of(_hw, struct isp_xclk, hw)
^~~~
drivers/media/platform/omap3isp/isp.c:187:26: note: in expansion of macro
'to_isp_xclk'
struct isp_xclk *xclk = to_isp_xclk(hw);
^~~
drivers/media/platform/omap3isp/isp.c: At top level:
drivers/media/platform/omap3isp/isp.c:282:21: error: variable 'isp_xclk_ops'
has initializer but incomplete type
static const struct clk_ops isp_xclk_ops = {
^~~
>> drivers/media/platform/omap3isp/isp.c:283:2: error: unknown field 'prepare'
>> specified in initializer
.prepare = isp_xclk_prepare,
^
drivers/media/platform/omap3isp/isp.c:283:13: warning: excess elements in
struct initializer
.prepare = isp_xclk_prepare,
^~~~
drivers/media/platform/omap3isp/isp.c:283:13: note: (near initialization for
'isp_xclk_ops')
>> drivers/media/platform/omap3isp/isp.c:284:2: error: unknown field
>> 'unprepare' specified in initializer
.unprepare = isp_xclk_unprepare,
^
drivers/media/platform/omap3isp/isp.c:284:15: warning: excess elements in
struct initializer
.unprepare = isp_xclk_unprepare,
^~
drivers/media/platform/omap3isp/isp.c:284:15: note: (near initialization for
'isp_xclk_ops')
>> drivers/media/platform/omap3isp/isp.c:285:2: error: unknown field 'enable'
>> specified in initializer
.enable = isp_xclk_enable,
^
drivers/media/platform/omap3isp/isp.c:285:12: warning: excess elements in
struct initializer
.enable = isp_xclk_enable,
^~~
drivers/media/platform/omap3isp/isp.c:285:12: note: (near initialization for
'isp_xclk_ops')
>> drivers/media/platform/omap3isp/isp.c:286:2: error: unknown field 'disable'
>> specified in initializer
.disable = isp_xclk_disable,
^
drivers/media/platform/omap3isp/isp.c:286:13: warning: excess elements in
struct initializer
.disable = isp_xclk_disable,
^~~~
drivers/media/platform/omap3isp/isp.c:286:13: note: (near initialization for
'isp_xclk_ops')
>> drivers/media/platform/omap3isp/isp.c:287:2: error: un
Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
Em Thu, 05 Apr 2018 21:30:27 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Thursday, 5 April 2018 20:54:02 EEST Mauro Carvalho Chehab wrote:
> > There aren't much things required for it to build with COMPILE_TEST.
> > It just needs to provide stub for an arm-dependent include.
> >
> > Let's replicate the same solution used by ipmmu-vmsa, in order
> > to allow building omap3 with COMPILE_TEST.
> >
> > The actual logic here came from this driver:
> >
> >drivers/iommu/ipmmu-vmsa.c
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/platform/Kconfig| 8
> > drivers/media/platform/omap3isp/isp.c | 7 +++
> > 2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index c7a1cf8a1b01..03c9dfeb7781 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -62,12 +62,12 @@ config VIDEO_MUX
> >
> > config VIDEO_OMAP3
> > tristate "OMAP 3 Camera support"
> > - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> > + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> > depends on HAS_DMA && OF
> > - depends on OMAP_IOMMU
> > - select ARM_DMA_USE_IOMMU
> > + depends on ((ARCH_OMAP3 && OMAP_IOMMU) || COMPILE_TEST)
> > + select ARM_DMA_USE_IOMMU if OMAP_IOMMU
> > select VIDEOBUF2_DMA_CONTIG
> > - select MFD_SYSCON
> > + select MFD_SYSCON if ARCH_OMAP3
> > select V4L2_FWNODE
> > ---help---
> > Driver for an OMAP 3 camera controller.
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 8eb000e3d8fd..2a11a709aa4f
> > 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -61,7 +61,14 @@
> > #include
> > #include
> >
> > +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> > #include
> > +#else
> > +#define arm_iommu_create_mapping(...) NULL
> > +#define arm_iommu_attach_device(...) -ENODEV
> > +#define arm_iommu_release_mapping(...) do {} while (0)
> > +#define arm_iommu_detach_device(...) do {} while (0)
> > +#endif
>
> I don't think it's the job of a driver to define those stubs, sorry.
> Otherwise
> where do you stop ? If you have half of the code that is architecture-
> dependent, would you stub it ? And what if the stubs you define here generate
> warnings in static analyzers ?
I agree that we should avoid doing that as a general case, but see
below.
> If you want to make drivers compile for all architectures, the APIs they use
> must be defined in linux/, not in asm/. They can be stubbed there when not
> implemented in a particular architecture, but not in the driver.
In this specific case, the same approach taken here is already needed
by the Renesas VMSA-compatible IPMMU driver, with, btw, is inside
drivers/iommu:
drivers/iommu/ipmmu-vmsa.c
Also, this API is used only by 3 drivers [1]:
drivers/iommu/ipmmu-vmsa.c
drivers/iommu/mtk_iommu_v1.c
drivers/media/platform/omap3isp/isp.c
[1] as blamed by
git grep -l arm_iommu_create_mapping
That hardly seems to be an arch-specific iommu solution, but, instead, some
hack used by only three drivers or some legacy iommu binding.
The omap3isp is, btw, the only driver outside drivers/iommu that needs it.
So, it sounds that other driver uses some other approach, but hardly
it would be worth to change this driver to use something else.
So, better to stick with the same solution the Renesas driver used.
Thanks,
Mauro
Re: [PATCH 02/16] media: omap3isp: allow it to build with COMPILE_TEST
Hi Mauro,
Thank you for the patch.
On Thursday, 5 April 2018 20:54:02 EEST Mauro Carvalho Chehab wrote:
> There aren't much things required for it to build with COMPILE_TEST.
> It just needs to provide stub for an arm-dependent include.
>
> Let's replicate the same solution used by ipmmu-vmsa, in order
> to allow building omap3 with COMPILE_TEST.
>
> The actual logic here came from this driver:
>
>drivers/iommu/ipmmu-vmsa.c
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/platform/Kconfig| 8
> drivers/media/platform/omap3isp/isp.c | 7 +++
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c7a1cf8a1b01..03c9dfeb7781 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -62,12 +62,12 @@ config VIDEO_MUX
>
> config VIDEO_OMAP3
> tristate "OMAP 3 Camera support"
> - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> depends on HAS_DMA && OF
> - depends on OMAP_IOMMU
> - select ARM_DMA_USE_IOMMU
> + depends on ((ARCH_OMAP3 && OMAP_IOMMU) || COMPILE_TEST)
> + select ARM_DMA_USE_IOMMU if OMAP_IOMMU
> select VIDEOBUF2_DMA_CONTIG
> - select MFD_SYSCON
> + select MFD_SYSCON if ARCH_OMAP3
> select V4L2_FWNODE
> ---help---
> Driver for an OMAP 3 camera controller.
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 8eb000e3d8fd..2a11a709aa4f
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -61,7 +61,14 @@
> #include
> #include
>
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> #include
> +#else
> +#define arm_iommu_create_mapping(...)NULL
> +#define arm_iommu_attach_device(...) -ENODEV
> +#define arm_iommu_release_mapping(...) do {} while (0)
> +#define arm_iommu_detach_device(...) do {} while (0)
> +#endif
I don't think it's the job of a driver to define those stubs, sorry. Otherwise
where do you stop ? If you have half of the code that is architecture-
dependent, would you stub it ? And what if the stubs you define here generate
warnings in static analyzers ?
If you want to make drivers compile for all architectures, the APIs they use
must be defined in linux/, not in asm/. They can be stubbed there when not
implemented in a particular architecture, but not in the driver.
> #include
> #include
--
Regards,
Laurent Pinchart
