Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Tue, Jun 28, 2011 at 01:35:27PM +0200, Nicolas Ferre wrote:
> Add this Kconfig variable to ease the submission of this chip support.
> As this chip/board inclusion is dealayed due to deep consolidation of
> arm/mach-at91 sources, we include this dummy configuration variable to allow
> submission of SAM9x5 related drivers in other subsystems.

Why are the drivers even depending on this ? They should be portable
enough. Can you share a few drivers so we have a look ?

IMHO, the whole idea of the consolidation is beyond arch/arm, drivers
should be affected too.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Nicolas Ferre]

Add this Kconfig variable to ease the submission of this chip support.
As this chip/board inclusion is dealayed due to deep consolidation of
arm/mach-at91 sources, we include this dummy configuration variable to allow
submission of SAM9x5 related drivers in other subsystems.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/Kconfig |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
index 2248467..ce45f64 100644
--- a/arch/arm/mach-at91/Kconfig
+++ b/arch/arm/mach-at91/Kconfig
@@ -12,6 +12,9 @@ config HAVE_AT91_USART4
 config HAVE_AT91_USART5
 	bool
 
+config ARCH_AT91SAM9X5
+	bool
+
 menu "Atmel AT91 System-on-Chip"
 
 choice
-- 
1.7.3

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Nicolas Ferre]

Le 28/06/2011 12:35, Felipe Balbi :
> On Tue, Jun 28, 2011 at 01:35:27PM +0200, Nicolas Ferre wrote:
>> Add this Kconfig variable to ease the submission of this chip support.
>> As this chip/board inclusion is dealayed due to deep consolidation of
>> arm/mach-at91 sources, we include this dummy configuration variable to allow
>> submission of SAM9x5 related drivers in other subsystems.
> 
> Why are the drivers even depending on this ? They should be portable
> enough. Can you share a few drivers so we have a look ?

Yes sure. The dependence is only on the Kconfig side: I plan to make
some drivers dependent on this configuration variable.
The goal is to submit the final driver addition without having to send
again a correction to the Kconfig after the chip/board support is merged.

This will ease the submission process at the cost of a two lines dummy
patch and will remove interdependence between subsystem trees: it worth
it, is not it?

> IMHO, the whole idea of the consolidation is beyond arch/arm, drivers
> should be affected too.

Yes sure, I also understood like this.
I will not spread ARCH_AT91SAM9X5 ifdef in driver code...

Best regards,
-- 
Nicolas Ferre

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

Hi,

On Tue, Jun 28, 2011 at 02:13:39PM +0200, Nicolas Ferre wrote:
> Le 28/06/2011 12:35, Felipe Balbi :
> > On Tue, Jun 28, 2011 at 01:35:27PM +0200, Nicolas Ferre wrote:
> >> Add this Kconfig variable to ease the submission of this chip support.
> >> As this chip/board inclusion is dealayed due to deep consolidation of
> >> arm/mach-at91 sources, we include this dummy configuration variable to allow
> >> submission of SAM9x5 related drivers in other subsystems.
> > 
> > Why are the drivers even depending on this ? They should be portable
> > enough. Can you share a few drivers so we have a look ?
> 
> Yes sure. The dependence is only on the Kconfig side: I plan to make
> some drivers dependent on this configuration variable.
> The goal is to submit the final driver addition without having to send
> again a correction to the Kconfig after the chip/board support is merged.

my point is that the drivers shouldn't need that ;-) Are the controllers
Atmel's specific or are you guys sourcing from somewhere else ?

> This will ease the submission process at the cost of a two lines dummy
> patch and will remove interdependence between subsystem trees: it worth
> it, is not it?

if you remove any architecture dependency from the driver, why do you
even need these two lines ? ;-)

> > IMHO, the whole idea of the consolidation is beyond arch/arm, drivers
> > should be affected too.
> 
> Yes sure, I also understood like this.
> I will not spread ARCH_AT91SAM9X5 ifdef in driver code...

yet you will prevent the driver from being easily used by other
architectures. What will happen is that a certain amount of:

depends on (ARCH_AT91SAM9X5 || ARCH_FOO || ARCH_BAR || ARCH_BAZ)

will continue to proliferate.

Here are a few questions:

i) The drivers you're willing to send, are those for Atmel's IPs or are
	the IPs sourced from some other company ?

ii) Even if they are Atmel-specific, do you see the possibility of Atmel
	licensing them ?

iii) Does your driver current depend on asm/ or mach/ headers ?

iv) Is there a generic header which you could use instead of asm/ mach/ ?

If you could share the driver, it would be easier to review on that one.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:26 Tue 28 Jun     , Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 28, 2011 at 02:13:39PM +0200, Nicolas Ferre wrote:
> > Le 28/06/2011 12:35, Felipe Balbi :
> > > On Tue, Jun 28, 2011 at 01:35:27PM +0200, Nicolas Ferre wrote:
> > >> Add this Kconfig variable to ease the submission of this chip support.
> > >> As this chip/board inclusion is dealayed due to deep consolidation of
> > >> arm/mach-at91 sources, we include this dummy configuration variable to allow
> > >> submission of SAM9x5 related drivers in other subsystems.
> > > 
> > > Why are the drivers even depending on this ? They should be portable
> > > enough. Can you share a few drivers so we have a look ?
> > 
> > Yes sure. The dependence is only on the Kconfig side: I plan to make
> > some drivers dependent on this configuration variable.
> > The goal is to submit the final driver addition without having to send
> > again a correction to the Kconfig after the chip/board support is merged.
> 
> my point is that the drivers shouldn't need that ;-) Are the controllers
> Atmel's specific or are you guys sourcing from somewhere else ?
> 
> > This will ease the submission process at the cost of a two lines dummy
> > patch and will remove interdependence between subsystem trees: it worth
> > it, is not it?
> 
> if you remove any architecture dependency from the driver, why do you
> even need these two lines ? ;-)
> 
> > > IMHO, the whole idea of the consolidation is beyond arch/arm, drivers
> > > should be affected too.
> > 
> > Yes sure, I also understood like this.
> > I will not spread ARCH_AT91SAM9X5 ifdef in driver code...
> 
> yet you will prevent the driver from being easily used by other
> architectures. What will happen is that a certain amount of:
> 
> depends on (ARCH_AT91SAM9X5 || ARCH_FOO || ARCH_BAR || ARCH_BAZ)
no I disagree this is done to allow only the drivers on proper arch

and we do not need the multiple depend we usally create a HAVE_xxx config
that the ARCH select and we just depend on it


Best Regards,
J.

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:35 Tue 28 Jun     , Nicolas Ferre wrote:
> Add this Kconfig variable to ease the submission of this chip support.
> As this chip/board inclusion is dealayed due to deep consolidation of
> arm/mach-at91 sources, we include this dummy configuration variable to allow
> submission of SAM9x5 related drivers in other subsystems.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

Hi,

On Tue, Jun 28, 2011 at 06:02:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > yet you will prevent the driver from being easily used by other
> > architectures. What will happen is that a certain amount of:
> > 
> > depends on (ARCH_AT91SAM9X5 || ARCH_FOO || ARCH_BAR || ARCH_BAZ)
> no I disagree this is done to allow only the drivers on proper arch
> 
> and we do not need the multiple depend we usally create a HAVE_xxx config
> that the ARCH select and we just depend on it

I still think it isn't good enough. Ideally, drivers will always be
portable without any arch dependency or the HAVE_XXX trick. Still, it's
not worth to continue this discussion, I rather look at a driver which
now depends on ARCH_AT91SAM9X5.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Nicolas Ferre]

Le 28/06/2011 14:26, Felipe Balbi :
> Hi,
> 
> On Tue, Jun 28, 2011 at 02:13:39PM +0200, Nicolas Ferre wrote:
>> Le 28/06/2011 12:35, Felipe Balbi :
>>> On Tue, Jun 28, 2011 at 01:35:27PM +0200, Nicolas Ferre wrote:
>>>> Add this Kconfig variable to ease the submission of this chip support.
>>>> As this chip/board inclusion is dealayed due to deep consolidation of
>>>> arm/mach-at91 sources, we include this dummy configuration variable to allow
>>>> submission of SAM9x5 related drivers in other subsystems.
>>>
>>> Why are the drivers even depending on this ? They should be portable
>>> enough. Can you share a few drivers so we have a look ?
>>
>> Yes sure. The dependence is only on the Kconfig side: I plan to make
>> some drivers dependent on this configuration variable.
>> The goal is to submit the final driver addition without having to send
>> again a correction to the Kconfig after the chip/board support is merged.
> 
> my point is that the drivers shouldn't need that ;-) Are the controllers
> Atmel's specific or are you guys sourcing from somewhere else ?
> 
>> This will ease the submission process at the cost of a two lines dummy
>> patch and will remove interdependence between subsystem trees: it worth
>> it, is not it?
> 
> if you remove any architecture dependency from the driver, why do you
> even need these two lines ? ;-)
> 
>>> IMHO, the whole idea of the consolidation is beyond arch/arm, drivers
>>> should be affected too.
>>
>> Yes sure, I also understood like this.
>> I will not spread ARCH_AT91SAM9X5 ifdef in driver code...
> 
> yet you will prevent the driver from being easily used by other
> architectures. What will happen is that a certain amount of:
> 
> depends on (ARCH_AT91SAM9X5 || ARCH_FOO || ARCH_BAR || ARCH_BAZ)

Yes, exactly like:

config USB_GADGET_ATMEL_USBA
[..]
        depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45

or

config MMC_ATMELMCI_DMA
[..]
        depends on MMC_ATMELMCI && (AVR32 || ARCH_AT91SAM9G45) && DMA_ENGINE && EXPERIMENTAL

or 

config TOUCHSCREEN_ATMEL_TSADCC
[..]
        depends on ARCH_AT91SAM9RL || ARCH_AT91SAM9G45


Those are places where I wanted to add my little ARCH_AT91SAM9X5...

And as Jean-Christophe said, when those lines are getting too long, we change this to a nice:
HAVE_xxx config option.

> will continue to proliferate.

So what?
It will:
- ease xconfig/menuconfig reading by user: who cares about my Atmel driver while running OMAPs?
- ease user choice by selecting default values depending on chip availability

> Here are a few questions:
> i) The drivers you're willing to send, are those for Atmel's IPs or are
> 	the IPs sourced from some other company ?
> ii) Even if they are Atmel-specific, do you see the possibility of Atmel
> 	licensing them ?
> iii) Does your driver current depend on asm/ or mach/ headers ?
> iv) Is there a generic header which you could use instead of asm/ mach/ ?

I just want to hide drivers that are not relevant for others: I have the feeling that it is a good practice. This tiny patch will ease this during my publication flow. Do you seriously care?

> If you could share the driver, it would be easier to review on that one.

Kconfig snippets above-quoted.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

Hi,

On Wed, Jun 29, 2011 at 05:24:42PM +0200, Nicolas Ferre wrote:
> > yet you will prevent the driver from being easily used by other
> > architectures. What will happen is that a certain amount of:
> > 
> > depends on (ARCH_AT91SAM9X5 || ARCH_FOO || ARCH_BAR || ARCH_BAZ)
> 
> Yes, exactly like:
> 
> config USB_GADGET_ATMEL_USBA
> [..]
>         depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
> 
> or
> 
> config MMC_ATMELMCI_DMA
> [..]
>         depends on MMC_ATMELMCI && (AVR32 || ARCH_AT91SAM9G45) && DMA_ENGINE && EXPERIMENTAL
> 
> or 
> 
> config TOUCHSCREEN_ATMEL_TSADCC
> [..]
>         depends on ARCH_AT91SAM9RL || ARCH_AT91SAM9G45

are you saying that's correct ? Well, I see those are platform_drivers,
so it's probably some IP integrated inside ATMEL chips. Now, what if the
same IP is used by some other SoC ? I mean, other than AT91* even.

> Those are places where I wanted to add my little ARCH_AT91SAM9X5...

After a couple years, someone else comes with another patch adding
ARCH_AT91SAMXXX and ARCH_AT91SAMYYY and ARCH_AT91_SAMZZZ...

But then again, at the end of the day Russell is the final judge :-) I
just wanted to say that it's far better to remove those dependencies and
allow those drivers to be built as modules rather than keep adding
dependencies to the end of times ;-)

> And as Jean-Christophe said, when those lines are getting too long, we
> change this to a nice: HAVE_xxx config option.
> 
> > will continue to proliferate.
> 
> So what?
> It will:
> - ease xconfig/menuconfig reading by user: who cares about my Atmel
> driver while running OMAPs?

what if in a couple of years comes one OMAP with the same IP that you're
using ? ;-)

it's rather unlikely, but for a simple example look at how many
Cortex-A9 MPCore Interrupt Controller drivers we have under arch/arm/
where it would be simpler to have _one_ driver being re-used by many
archs ;-)

> - ease user choice by selecting default values depending on chip
> availability

IMHO, it's far simpler to answer 'M' to a driver such as a touchscreen
controller without even thinking about it ;-)

> > Here are a few questions:
> > i) The drivers you're willing to send, are those for Atmel's IPs or are
> > 	the IPs sourced from some other company ?
> > ii) Even if they are Atmel-specific, do you see the possibility of Atmel
> > 	licensing them ?
> > iii) Does your driver current depend on asm/ or mach/ headers ?
> > iv) Is there a generic header which you could use instead of asm/ mach/ ?
> 
> I just want to hide drivers that are not relevant for others: I have
> the feeling that it is a good practice. This tiny patch will ease this
> during my publication flow. Do you seriously care?

Well, it's not that I care. I just don't see the point in hiding the
drivers. For starters we loose the very nice linux-next infrastructure
for giving us compile tests ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Arnd Bergmann]

On Wednesday 29 June 2011 17:24:42 Nicolas Ferre wrote:
> > Here are a few questions:
> > i) The drivers you're willing to send, are those for Atmel's IPs or are
> >       the IPs sourced from some other company ?
> > ii) Even if they are Atmel-specific, do you see the possibility of Atmel
> >       licensing them ?
> > iii) Does your driver current depend on asm/ or mach/ headers ?
> > iv) Is there a generic header which you could use instead of asm/ mach/ ?
> 
> I just want to hide drivers that are not relevant for others: I have the feeling
> that it is a good practice. This tiny patch will ease this during my publication
> flow. Do you seriously care?

I think Felipe is right on this one, but both views are common in the kernel
today: Some people want dependencies to mean "you cannot build this driver
unless the dependencies are fulfilled", others like them more broadly to
mean "there is no point to ever enable this driver because I know you won't
need it".

Both views are understandable, but I favor the first one because

* it's the more common view these days and we should be consistent

* it exposes drivers to more build testing. If something changes in
  the kernel that exposes new warnings in your driver or causes a
  build error, that is more likely to get fixed when more people
  find it by doing allyesconfig or randconfig builds.

* If there is an actual build dependency between the driver and the
  platform that causes you to need the explicit Kconfig depends, that
  is in many cases a hint that the driver author is doing something
  wrong, like hardcoding MMIO addresses or referencing custom
  symbols exported by the platform.

I don't think anyone really objects your patch to introduce the extra
Kconfig symbol, but I'd hope that we can eventually get a consensus
on the idea that you shouldn't use Kconfig dependencies based on
whether a driver is relevant or not.

	Arnd

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Nicolas Ferre]

Le 02/07/2011 11:49, Arnd Bergmann :
> On Wednesday 29 June 2011 17:24:42 Nicolas Ferre wrote:
>>> Here are a few questions:
>>> i) The drivers you're willing to send, are those for Atmel's IPs or are
>>>       the IPs sourced from some other company ?
>>> ii) Even if they are Atmel-specific, do you see the possibility of Atmel
>>>       licensing them ?
>>> iii) Does your driver current depend on asm/ or mach/ headers ?
>>> iv) Is there a generic header which you could use instead of asm/ mach/ ?
>>
>> I just want to hide drivers that are not relevant for others: I have the feeling
>> that it is a good practice. This tiny patch will ease this during my publication
>> flow. Do you seriously care?
> 
> I think Felipe is right on this one, but both views are common in the kernel
> today: Some people want dependencies to mean "you cannot build this driver
> unless the dependencies are fulfilled", others like them more broadly to
> mean "there is no point to ever enable this driver because I know you won't
> need it".
> 
> Both views are understandable, but I favor the first one because
> 
> * it's the more common view these days and we should be consistent
> 
> * it exposes drivers to more build testing. If something changes in
>   the kernel that exposes new warnings in your driver or causes a
>   build error, that is more likely to get fixed when more people
>   find it by doing allyesconfig or randconfig builds.
> 
> * If there is an actual build dependency between the driver and the
>   platform that causes you to need the explicit Kconfig depends, that
>   is in many cases a hint that the driver author is doing something
>   wrong, like hardcoding MMIO addresses or referencing custom
>   symbols exported by the platform.
> 
> I don't think anyone really objects your patch to introduce the extra
> Kconfig symbol, but I'd hope that we can eventually get a consensus
> on the idea that you shouldn't use Kconfig dependencies based on
> whether a driver is relevant or not.

Arnd, Felipe,

You have convinced me.
But I will have to remove the other dependencies that I mentioned before
in the thread.

We can drop this patch.

Bye,
-- 
Nicolas Ferre

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:23 Mon 04 Jul     , Nicolas Ferre wrote:
> Le 02/07/2011 11:49, Arnd Bergmann :
> > On Wednesday 29 June 2011 17:24:42 Nicolas Ferre wrote:
> >>> Here are a few questions:
> >>> i) The drivers you're willing to send, are those for Atmel's IPs or are
> >>>       the IPs sourced from some other company ?
> >>> ii) Even if they are Atmel-specific, do you see the possibility of Atmel
> >>>       licensing them ?
> >>> iii) Does your driver current depend on asm/ or mach/ headers ?
> >>> iv) Is there a generic header which you could use instead of asm/ mach/ ?
> >>
> >> I just want to hide drivers that are not relevant for others: I have the feeling
> >> that it is a good practice. This tiny patch will ease this during my publication
> >> flow. Do you seriously care?
> > 
> > I think Felipe is right on this one, but both views are common in the kernel
> > today: Some people want dependencies to mean "you cannot build this driver
> > unless the dependencies are fulfilled", others like them more broadly to
> > mean "there is no point to ever enable this driver because I know you won't
> > need it".
> > 
> > Both views are understandable, but I favor the first one because
> > 
> > * it's the more common view these days and we should be consistent
> > 
> > * it exposes drivers to more build testing. If something changes in
> >   the kernel that exposes new warnings in your driver or causes a
> >   build error, that is more likely to get fixed when more people
> >   find it by doing allyesconfig or randconfig builds.
> > 
> > * If there is an actual build dependency between the driver and the
> >   platform that causes you to need the explicit Kconfig depends, that
> >   is in many cases a hint that the driver author is doing something
> >   wrong, like hardcoding MMIO addresses or referencing custom
> >   symbols exported by the platform.
> > 
> > I don't think anyone really objects your patch to introduce the extra
> > Kconfig symbol, but I'd hope that we can eventually get a consensus
> > on the idea that you shouldn't use Kconfig dependencies based on
> > whether a driver is relevant or not.
> 
> Arnd, Felipe,
> 
> You have convinced me.
> But I will have to remove the other dependencies that I mentioned before
> in the thread.
> 
> We can drop this patch.
I prefer to hide the platform specific driver other wise we will have a huge
menu entry in Kconfig with unrelated drivers that can not be used at all on
the selected mach

This is really annoying

Best Regards,
J.

Re: [PATCH] AT91: add AT91SAM9X5 dummy configuration variable

[Arnd Bergmann]

On Tuesday 05 July 2011, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > You have convinced me.
> > But I will have to remove the other dependencies that I mentioned before
> > in the thread.
> > 
> > We can drop this patch.
> I prefer to hide the platform specific driver other wise we will have a huge
> menu entry in Kconfig with unrelated drivers that can not be used at all on
> the selected mach

I fear it's too late for that. You already have to decide on many thousands
of configuration options, most of which are totally irrelevant for a
particular system.

IMHO, the two much better ways to deal with SOC-specific device drivers
are defconfig files and/or specific defaults, like

config ABX500_CORE
        bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions"
        default ARCH_U300 || ARCH_U8500

This lets everybody choose whether they would like to build the driver
or not, which is very useful for allyesconfig/allnoconfig/randconfig
builds. At the same time, the default gets it right in almost all
other cases.

	Arnd