[PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Javier Martinez Canillas]

The Exynos5420 Peach Pit and Exynos5800 Peach Pi boards have a built-in
Silicon Motion USB UVC WebCam. Enable support for the USB Video Class
driver and its needed media Kconfig symbols so the camera is supported.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 arch/arm/configs/exynos_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/configs/exynos_defconfig b/arch/arm/configs/exynos_defconfig
index 1ff2bfa2e183..62f677f76a0b 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -126,6 +126,10 @@ CONFIG_REGULATOR_S2MPA01=y
 CONFIG_REGULATOR_S2MPS11=y
 CONFIG_REGULATOR_S5M8767=y
 CONFIG_REGULATOR_TPS65090=y
+CONFIG_MEDIA_SUPPORT=y
+CONFIG_MEDIA_CAMERA_SUPPORT=y
+CONFIG_MEDIA_USB_SUPPORT=y
+CONFIG_USB_VIDEO_CLASS=y
 CONFIG_DRM=y
 CONFIG_DRM_NXP_PTN3460=y
 CONFIG_DRM_PARADE_PS8622=y
-- 
2.4.3

Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Krzysztof Kozlowski]

On 08.09.2015 06:45, Javier Martinez Canillas wrote:
> The Exynos5420 Peach Pit and Exynos5800 Peach Pi boards have a built-in
> Silicon Motion USB UVC WebCam. Enable support for the USB Video Class
> driver and its needed media Kconfig symbols so the camera is supported.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
>  arch/arm/configs/exynos_defconfig | 4 ++++
>  1 file changed, 4 insertions(+)

The patch itself looks good but now I wonder whether we are not putting
to much stuff built-in. The exynos_defconfig does not replace the
distribution distro. For a fully working board the distro should prepare
it's own config.

I understand that in this case the USB webcams are parts of device (like
on all laptops)... a little bit similar as camera sensors on mobile
phones. Yet on mobile phone usually the camera itself is part of SoC,
only the sensor is external.

Actually what we need is a kind of policy for exynos_defconfig - what
should be inside as built-in and what as module?

Best regards,
Krzysztof


> 
> diff --git a/arch/arm/configs/exynos_defconfig b/arch/arm/configs/exynos_defconfig
> index 1ff2bfa2e183..62f677f76a0b 100644
> --- a/arch/arm/configs/exynos_defconfig
> +++ b/arch/arm/configs/exynos_defconfig
> @@ -126,6 +126,10 @@ CONFIG_REGULATOR_S2MPA01=y
>  CONFIG_REGULATOR_S2MPS11=y
>  CONFIG_REGULATOR_S5M8767=y
>  CONFIG_REGULATOR_TPS65090=y
> +CONFIG_MEDIA_SUPPORT=y
> +CONFIG_MEDIA_CAMERA_SUPPORT=y
> +CONFIG_MEDIA_USB_SUPPORT=y
> +CONFIG_USB_VIDEO_CLASS=y
>  CONFIG_DRM=y
>  CONFIG_DRM_NXP_PTN3460=y
>  CONFIG_DRM_PARADE_PS8622=y
> 

Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Javier Martinez Canillas]

[adding Bartlomiej to cc]

Hello Krzysztof,

On 09/08/2015 10:11 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 06:45, Javier Martinez Canillas wrote:
>> The Exynos5420 Peach Pit and Exynos5800 Peach Pi boards have a built-in
>> Silicon Motion USB UVC WebCam. Enable support for the USB Video Class
>> driver and its needed media Kconfig symbols so the camera is supported.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>>
>>  arch/arm/configs/exynos_defconfig | 4 ++++
>>  1 file changed, 4 insertions(+)
> 
> The patch itself looks good but now I wonder whether we are not putting
> to much stuff built-in. The exynos_defconfig does not replace the
> distribution distro. For a fully working board the distro should prepare
> it's own config.
>

Agreed that exynos_defconfig is not meant to replace a distro config.

> I understand that in this case the USB webcams are parts of device (like
> on all laptops)... a little bit similar as camera sensors on mobile
> phones. Yet on mobile phone usually the camera itself is part of SoC,
> only the sensor is external.
> 
> Actually what we need is a kind of policy for exynos_defconfig - what
> should be inside as built-in and what as module?
>

I had the same conversation with Bartlomiej before in [0] when I tried to
enable the SBS battery driver as module. I save you a click and quote him:

"the current most popular use case for exynos_defconfig
(not multi_v7_defconfig) seems to be to build kernel image
alone and use it without any modules"

Which seems to be true, so my understanding is that exynos_defconfig is a
minimal defconfig for Exynos platforms and for easy of test/use, everything
should be built-in while multi_v7_defconfig would be more similar to a conf
used by distros where most things would be built as a module when possible.

Other SoC specific deconfig do it differently, OMAP for example does the
opposite and tries to build as much stuff as possible as a module.

> Best regards,
> Krzysztof
> 
>

[0]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/278757.html

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Krzysztof Kozlowski]

W dniu 08.09.2015 o 17:40, Javier Martinez Canillas pisze:
> [adding Bartlomiej to cc]
> 
> Hello Krzysztof,
> 
> On 09/08/2015 10:11 AM, Krzysztof Kozlowski wrote:
>> On 08.09.2015 06:45, Javier Martinez Canillas wrote:
>>> The Exynos5420 Peach Pit and Exynos5800 Peach Pi boards have a built-in
>>> Silicon Motion USB UVC WebCam. Enable support for the USB Video Class
>>> driver and its needed media Kconfig symbols so the camera is supported.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>
>>> ---
>>>
>>>  arch/arm/configs/exynos_defconfig | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>
>> The patch itself looks good but now I wonder whether we are not putting
>> to much stuff built-in. The exynos_defconfig does not replace the
>> distribution distro. For a fully working board the distro should prepare
>> it's own config.
>>
> 
> Agreed that exynos_defconfig is not meant to replace a distro config.
> 
>> I understand that in this case the USB webcams are parts of device (like
>> on all laptops)... a little bit similar as camera sensors on mobile
>> phones. Yet on mobile phone usually the camera itself is part of SoC,
>> only the sensor is external.
>>
>> Actually what we need is a kind of policy for exynos_defconfig - what
>> should be inside as built-in and what as module?
>>
> 
> I had the same conversation with Bartlomiej before in [0] when I tried to
> enable the SBS battery driver as module. I save you a click and quote him:
> 
> "the current most popular use case for exynos_defconfig
> (not multi_v7_defconfig) seems to be to build kernel image
> alone and use it without any modules"
> 
> Which seems to be true, so my understanding is that exynos_defconfig is a
> minimal defconfig for Exynos platforms and for easy of test/use, everything
> should be built-in while multi_v7_defconfig would be more similar to a conf
> used by distros where most things would be built as a module when possible.

Yeah, but this actually does not entirely cover my doubts. We cannot
enable everything built-in because boot partitions on some devices have
limited size. So enabling everything would break that use case (use case
of easy testing).

Let me rephrase my question into:
1. What is worth enabling in exynos_defconfig? USB devices? I would
argue, except they are needed to boot.
So maybe enable everything which Exynos boards have hard-wired? That
would make some sense... but we're making kernel larger.

2. Maybe enable only what is a typical use case (including typical
testing cases)? Then we would have to define what "typical" means. For
example battery would be typical but camera would not.

3. Argh, so maybe, if we agree that not everything is worth being
enabled, that additional stuff could be build as module?


> Other SoC specific deconfig do it differently, OMAP for example does the
> opposite and tries to build as much stuff as possible as a module.

I don't argue about switching from "y" to "m". That's not the case.
Rather I am thinking where/when we should stop enabling stuff?

Best regards,
Krzysztof

> 
>> Best regards,
>> Krzysztof
>>
>>
> 
> [0]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/278757.html
> 
> Best regards,
> 

Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Javier Martinez Canillas]

Hello Krzysztof,

On 09/08/2015 03:12 PM, Krzysztof Kozlowski wrote:
> W dniu 08.09.2015 o 17:40, Javier Martinez Canillas pisze:
>> [adding Bartlomiej to cc]
>>
>> Hello Krzysztof,
>>
>> On 09/08/2015 10:11 AM, Krzysztof Kozlowski wrote:
>>> On 08.09.2015 06:45, Javier Martinez Canillas wrote:
>>>> The Exynos5420 Peach Pit and Exynos5800 Peach Pi boards have a built-in
>>>> Silicon Motion USB UVC WebCam. Enable support for the USB Video Class
>>>> driver and its needed media Kconfig symbols so the camera is supported.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>>
>>>> ---
>>>>
>>>>  arch/arm/configs/exynos_defconfig | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>
>>> The patch itself looks good but now I wonder whether we are not putting
>>> to much stuff built-in. The exynos_defconfig does not replace the
>>> distribution distro. For a fully working board the distro should prepare
>>> it's own config.
>>>
>>
>> Agreed that exynos_defconfig is not meant to replace a distro config.
>>
>>> I understand that in this case the USB webcams are parts of device (like
>>> on all laptops)... a little bit similar as camera sensors on mobile
>>> phones. Yet on mobile phone usually the camera itself is part of SoC,
>>> only the sensor is external.
>>>
>>> Actually what we need is a kind of policy for exynos_defconfig - what
>>> should be inside as built-in and what as module?
>>>
>>
>> I had the same conversation with Bartlomiej before in [0] when I tried to
>> enable the SBS battery driver as module. I save you a click and quote him:
>>
>> "the current most popular use case for exynos_defconfig
>> (not multi_v7_defconfig) seems to be to build kernel image
>> alone and use it without any modules"
>>
>> Which seems to be true, so my understanding is that exynos_defconfig is a
>> minimal defconfig for Exynos platforms and for easy of test/use, everything
>> should be built-in while multi_v7_defconfig would be more similar to a conf
>> used by distros where most things would be built as a module when possible.
> 
> Yeah, but this actually does not entirely cover my doubts. We cannot
> enable everything built-in because boot partitions on some devices have
> limited size. So enabling everything would break that use case (use case
> of easy testing).
>

Agreed.
 
> Let me rephrase my question into:
> 1. What is worth enabling in exynos_defconfig? USB devices? I would
> argue, except they are needed to boot.

Ok, I understand your concern. The question is where we draw the line.

> So maybe enable everything which Exynos boards have hard-wired? That
> would make some sense... but we're making kernel larger.
>

In the case of this WebCam, it's not a typical USB device in the sense
that is built in the Chromebook and not something that's plugged on an
external USB port.
 
> 2. Maybe enable only what is a typical use case (including typical
> testing cases)? Then we would have to define what "typical" means. For
> example battery would be typical but camera would not.
>

There are a lot of board specific drivers that we currently enable as
built-in like hwmon sensors or iio devices that are likely only present
on a single board or a family of boards.

So then I think all those drivers should be changed as a module as well,
unless are critical for the board operation (i.e: thermal or fan drivers).

> 3. Argh, so maybe, if we agree that not everything is worth being
> enabled, that additional stuff could be build as module?
>

Yes, I don't see anything wrong to enable more stuff as a module if
that will give more build / test coverage.

The goal of kernelci is to add functional tests so besides testing
if a given kernel booted correctly, it's going to test if for example
USB enumeration is working and has no regressions. For that use case
is interesting to have support for the built-in USB devices like this
camera (either as built-in or as a module).
 
> 
>> Other SoC specific deconfig do it differently, OMAP for example does the
>> opposite and tries to build as much stuff as possible as a module.
> 
> I don't argue about switching from "y" to "m". That's not the case.
> Rather I am thinking where/when we should stop enabling stuff?
>

Yes, sorry for misunderstanding what you meant before.
 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Krzysztof Kozlowski]

On 08.09.2015 22:32, Javier Martinez Canillas wrote:

(...)

>  
>> Let me rephrase my question into:
>> 1. What is worth enabling in exynos_defconfig? USB devices? I would
>> argue, except they are needed to boot.
> 
> Ok, I understand your concern. The question is where we draw the line.
> 
>> So maybe enable everything which Exynos boards have hard-wired? That
>> would make some sense... but we're making kernel larger.
>>
> 
> In the case of this WebCam, it's not a typical USB device in the sense
> that is built in the Chromebook and not something that's plugged on an
> external USB port.

Right, that is the difference from regular USB devices.

>  
>> 2. Maybe enable only what is a typical use case (including typical
>> testing cases)? Then we would have to define what "typical" means. For
>> example battery would be typical but camera would not.
>>
> 
> There are a lot of board specific drivers that we currently enable as
> built-in like hwmon sensors or iio devices that are likely only present
> on a single board or a family of boards.
> 
> So then I think all those drivers should be changed as a module as well,
> unless are critical for the board operation (i.e: thermal or fan drivers).

Actually I think we should not judge by number of board using given
component but its usefulness in general exynos_defconfig case. Even when
something is used on just one board but it is important for that board,
then it should be built-in.

For example hwmon monitoring stuff to get information about board
condition. Other example are leds on Odroid - to get visible condition
of the board.

This don't have to be critical, but just important for testing.

Additionally such components can be accessed usually from limited
user-space, e.g. system booted to console or SSH.

If using a component requires more complex user-space (e.g. any kind of
window system), then probably already modules could be easily used. In
such cases I would expect the boot is not from network but from MMC and
there is a full-blown distro working.


>> 3. Argh, so maybe, if we agree that not everything is worth being
>> enabled, that additional stuff could be build as module?
>>
> 
> Yes, I don't see anything wrong to enable more stuff as a module if
> that will give more build / test coverage.
> 
> The goal of kernelci is to add functional tests so besides testing
> if a given kernel booted correctly, it's going to test if for example
> USB enumeration is working and has no regressions. For that use case
> is interesting to have support for the built-in USB devices like this
> camera (either as built-in or as a module).

Okay, so we have some agreement that other stuff which is not important
but still hard-wired on Exynos boards (built into the board), can be
enabled as a module. So now we we have to draw the line which is
"important enough" to built-in and which is not so it could be as module.

>From my point of view media in general (cameras, tuners etc.) should be
put in the second category (module), especially that in usual to test
them you would have to boot system to a full graphical mode. Can you
test them from SSH connection? Maybe you could test DVB tuners by
reading status of packets but still output won't be visible.

Any comments from other interested parties?

Best regards,
Krzysztof

Re: [PATCH] ARM: exynos_defconfig: Enable USB Video Class support

[Javier Martinez Canillas]

Hello Krzysztof,

On 09/09/2015 02:06 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 22:32, Javier Martinez Canillas wrote:
> 
> (...)
> 
>>  
>>> Let me rephrase my question into:
>>> 1. What is worth enabling in exynos_defconfig? USB devices? I would
>>> argue, except they are needed to boot.
>>
>> Ok, I understand your concern. The question is where we draw the line.
>>
>>> So maybe enable everything which Exynos boards have hard-wired? That
>>> would make some sense... but we're making kernel larger.
>>>
>>
>> In the case of this WebCam, it's not a typical USB device in the sense
>> that is built in the Chromebook and not something that's plugged on an
>> external USB port.
> 
> Right, that is the difference from regular USB devices.
> 
>>  
>>> 2. Maybe enable only what is a typical use case (including typical
>>> testing cases)? Then we would have to define what "typical" means. For
>>> example battery would be typical but camera would not.
>>>
>>
>> There are a lot of board specific drivers that we currently enable as
>> built-in like hwmon sensors or iio devices that are likely only present
>> on a single board or a family of boards.
>>
>> So then I think all those drivers should be changed as a module as well,
>> unless are critical for the board operation (i.e: thermal or fan drivers).
> 
> Actually I think we should not judge by number of board using given
> component but its usefulness in general exynos_defconfig case. Even when
> something is used on just one board but it is important for that board,
> then it should be built-in.
> 
> For example hwmon monitoring stuff to get information about board
> condition. Other example are leds on Odroid - to get visible condition
> of the board.
> 
> This don't have to be critical, but just important for testing.
> 
> Additionally such components can be accessed usually from limited
> user-space, e.g. system booted to console or SSH.
> 
> If using a component requires more complex user-space (e.g. any kind of
> window system), then probably already modules could be easily used. In
> such cases I would expect the boot is not from network but from MMC and
> there is a full-blown distro working.
> 

Ok, fair enough.
 
>>> 3. Argh, so maybe, if we agree that not everything is worth being
>>> enabled, that additional stuff could be build as module?
>>>
>>
>> Yes, I don't see anything wrong to enable more stuff as a module if
>> that will give more build / test coverage.
>>
>> The goal of kernelci is to add functional tests so besides testing
>> if a given kernel booted correctly, it's going to test if for example
>> USB enumeration is working and has no regressions. For that use case
>> is interesting to have support for the built-in USB devices like this
>> camera (either as built-in or as a module).
> 
> Okay, so we have some agreement that other stuff which is not important
> but still hard-wired on Exynos boards (built into the board), can be
> enabled as a module. So now we we have to draw the line which is
> "important enough" to built-in and which is not so it could be as module.
> 
>>From my point of view media in general (cameras, tuners etc.) should be
> put in the second category (module), especially that in usual to test
> them you would have to boot system to a full graphical mode. Can you
> test them from SSH connection? Maybe you could test DVB tuners by
> reading status of packets but still output won't be visible.
>

Ok, my take on it was that if is wired into the board, then it should be
supported out-of-the-box with a zImage build using exynos_defconfig but
I see your point and looks reasonable to me.

I'll wait a couple of days to see if others have more opinions and respin
the patch with these options enabled as a module.

Thanks a lot for your feedback!
 
> Any comments from other interested parties?
> 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America