| Message ID | 1309260927-11411-1-git-send-email-nicolas.ferre@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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.
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.
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.
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.
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,
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 ;-)
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
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,
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
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
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(-)