| Message ID | 4002702.FticUbImag@flatron (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 31, 2011 at 02:34:15PM +0200, Tomasz Figa wrote: > This patch is a prerequisite to adding generic time support for S3C64xx. > It makes possible to differentiate SoCs, required to exclude timers 3 and 4 > from PWM driver only on S3C64xx. Now we have the cpu_is_foo() support for Samsung CPUs can we use that instead?
2011/8/31 Mark Brown <broonie@opensource.wolfsonmicro.com>: > On Wed, Aug 31, 2011 at 02:34:15PM +0200, Tomasz Figa wrote: > >> This patch is a prerequisite to adding generic time support for S3C64xx. >> It makes possible to differentiate SoCs, required to exclude timers 3 and 4 >> from PWM driver only on S3C64xx. > > Now we have the cpu_is_foo() support for Samsung CPUs can we use that > instead? > Well, I thought about it, but I based my patches on latest stable sources and cpu_is_s3c64xx wasn't there yet. Should I rebase them to Linus' tree? I'm not yet fully used to submitting patches here, so things like choosing the base for patches are sometimes a bit confusing to me and I couldn't find any guide. Should fixes be based on stable tree and new features implemented on top of current Linus' tree? Could you give me some advice on this?
Tomasz Figa wrote: > > 2011/8/31 Mark Brown <broonie@opensource.wolfsonmicro.com>: > > On Wed, Aug 31, 2011 at 02:34:15PM +0200, Tomasz Figa wrote: > > > >> This patch is a prerequisite to adding generic time support for S3C64xx. > >> It makes possible to differentiate SoCs, required to exclude timers 3 and 4 > >> from PWM driver only on S3C64xx. > > > > Now we have the cpu_is_foo() support for Samsung CPUs can we use that > > instead? > > > > Well, I thought about it, but I based my patches on latest stable sources and > cpu_is_s3c64xx wasn't there yet. Should I rebase them to Linus' tree? > > I'm not yet fully used to submitting patches here, so things like choosing the > base for patches are sometimes a bit confusing to me and I couldn't find any > guide. Should fixes be based on stable tree and new features implemented on > top of current Linus' tree? Could you give me some advice on this? Hi Tomasz, You can use "soc_is_s3c64xx()" based on Samsung -next tree. git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git for-next In addition, would be better to me if you could work about Samsung stuff based on it. As a note, your previous patches are in there :) If any problems, please let me know... Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
On Thu, Sep 1, 2011 at 11:26 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: > Tomasz Figa wrote: >> >> 2011/8/31 Mark Brown <broonie@opensource.wolfsonmicro.com>: >> > On Wed, Aug 31, 2011 at 02:34:15PM +0200, Tomasz Figa wrote: >> > >> >> This patch is a prerequisite to adding generic time support for > S3C64xx. >> >> It makes possible to differentiate SoCs, required to exclude timers 3 > and 4 >> >> from PWM driver only on S3C64xx. >> > >> > Now we have the cpu_is_foo() support for Samsung CPUs can we use that >> > instead? >> > >> >> Well, I thought about it, but I based my patches on latest stable sources > and >> cpu_is_s3c64xx wasn't there yet. Should I rebase them to Linus' tree? >> >> I'm not yet fully used to submitting patches here, so things like choosing > the >> base for patches are sometimes a bit confusing to me and I couldn't find > any >> guide. Should fixes be based on stable tree and new features implemented > on >> top of current Linus' tree? Could you give me some advice on this? > > Hi Tomasz, > > You can use "soc_is_s3c64xx()" based on Samsung -next tree. > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > for-next +static struct platform_device_id s3c_pwm_driver_ids[] = { + { + .name = "s3c24xx-pwm", + .driver_data = TYPE_GENERIC, + }, { + .name = "s3c64xx-pwm", + .driver_data = TYPE_S3C64XX, + }, +}; +MODULE_DEVICE_TABLE(platform, s3c_pwm_driver_ids); doesn't it right way? do you want to use the soc_is_* at device? > > In addition, would be better to me if you could work about Samsung stuff > based on it. > > As a note, your previous patches are in there :) > > If any problems, please let me know... > > Thanks. > > Best regards, > Kgene. > -- > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, > SW Solution Development Team, Samsung Electronics Co., Ltd. > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thursday 01 of September 2011 at 12:33:10, Kyungmin Park wrote: > On Thu, Sep 1, 2011 at 11:26 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: > > Tomasz Figa wrote: > >> > >> 2011/8/31 Mark Brown <broonie@opensource.wolfsonmicro.com>: > >> > On Wed, Aug 31, 2011 at 02:34:15PM +0200, Tomasz Figa wrote: > >> > > >> >> This patch is a prerequisite to adding generic time support for > > S3C64xx. > >> >> It makes possible to differentiate SoCs, required to exclude timers 3 > > and 4 > >> >> from PWM driver only on S3C64xx. > >> > > >> > Now we have the cpu_is_foo() support for Samsung CPUs can we use that > >> > instead? > >> > >> [snip] > > > > Hi Tomasz, > > > > You can use "soc_is_s3c64xx()" based on Samsung -next tree. > > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > > for-next > > +static struct platform_device_id s3c_pwm_driver_ids[] = { > + { > + .name = "s3c24xx-pwm", > + .driver_data = TYPE_GENERIC, > + }, { > + .name = "s3c64xx-pwm", > + .driver_data = TYPE_S3C64XX, > + }, > +}; > +MODULE_DEVICE_TABLE(platform, s3c_pwm_driver_ids); > > doesn't it right way? > do you want to use the soc_is_* at device? > Well, now we have three alternatives: - to leave it as is (i.e. changing platform device names) - to use cpu_is_* (does it ever exist for S3C64xx or soc_is_s3c64xx is its equivalent?) - to use soc_is_* Waiting for more comments then. Best regards, Tom
On Thu, Sep 01, 2011 at 01:18:32PM +0200, Tomasz Figa wrote: > On Thursday 01 of September 2011 at 12:33:10, Kyungmin Park wrote: > Well, now we have three alternatives: > - to leave it as is (i.e. changing platform device names) > - to use cpu_is_* (does it ever exist for S3C64xx or soc_is_s3c64xx is its equivalent?) > - to use soc_is_* soc_is is the same thing.
On Thursday 01 of September 2011 at 13:18:32, Tomasz Figa wrote: > On Thursday 01 of September 2011 at 12:33:10, Kyungmin Park wrote: > > On Thu, Sep 1, 2011 at 11:26 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: > > > Tomasz Figa wrote: > > >> > > >> 2011/8/31 Mark Brown <broonie@opensource.wolfsonmicro.com>: > > >> > On Wed, Aug 31, 2011 at 02:34:15PM +0200, Tomasz Figa wrote: > > >> > > > >> >> This patch is a prerequisite to adding generic time support for > > > S3C64xx. > > >> >> It makes possible to differentiate SoCs, required to exclude timers 3 > > > and 4 > > >> >> from PWM driver only on S3C64xx. > > >> > > > >> > Now we have the cpu_is_foo() support for Samsung CPUs can we use that > > >> > instead? > > >> > > >> [snip] > > > > > > Hi Tomasz, > > > > > > You can use "soc_is_s3c64xx()" based on Samsung -next tree. > > > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > > > for-next > > > > +static struct platform_device_id s3c_pwm_driver_ids[] = { > > + { > > + .name = "s3c24xx-pwm", > > + .driver_data = TYPE_GENERIC, > > + }, { > > + .name = "s3c64xx-pwm", > > + .driver_data = TYPE_S3C64XX, > > + }, > > +}; > > +MODULE_DEVICE_TABLE(platform, s3c_pwm_driver_ids); > > > > doesn't it right way? > > do you want to use the soc_is_* at device? > > > > Well, now we have three alternatives: > - to leave it as is (i.e. changing platform device names) > - to use cpu_is_* (does it ever exist for S3C64xx or soc_is_s3c64xx is its equivalent?) > - to use soc_is_* > > Waiting for more comments then. > Ping. Should I keep it as is or rather change it to use soc_is_s3c64xx instead? Best regards, Tom
On Tue, Sep 06, 2011 at 12:41:07PM +0200, Tomasz Figa wrote: > On Thursday 01 of September 2011 at 13:18:32, Tomasz Figa wrote: > > Well, now we have three alternatives: > > - to leave it as is (i.e. changing platform device names) > > - to use cpu_is_* (does it ever exist for S3C64xx or soc_is_s3c64xx is its equivalent?) > > - to use soc_is_* > > > > Waiting for more comments then. > > > Ping. > > Should I keep it as is or rather change it to use soc_is_s3c64xx instead? I don't know what question you're asking (the questions don't make sense in terms of the quoted context.) If you're asking whether you should use a cpu_ prefix instead of a soc_ prefix for identifying s3c64xx, then consider the question. What is the CPU? What are S3C64xx / OMAP / PXA310 / SA-11x0 / AT91RM9200? CPU or SoC ? What are Cortex A8/A9 / Xscale / StrongARM / ARM920? CPU or SoC ?
On Tuesday 06 of September 2011 at 12:03:11, Russell King - ARM Linux wrote: > On Tue, Sep 06, 2011 at 12:41:07PM +0200, Tomasz Figa wrote: > > On Thursday 01 of September 2011 at 13:18:32, Tomasz Figa wrote: > > > Well, now we have three alternatives: > > > - to leave it as is (i.e. changing platform device names) > > > - to use cpu_is_* (does it ever exist for S3C64xx or soc_is_s3c64xx is its equivalent?) > > > - to use soc_is_* > > > > > > Waiting for more comments then. > > > > > Ping. > > > > Should I keep it as is or rather change it to use soc_is_s3c64xx instead? > > I don't know what question you're asking (the questions don't make sense > in terms of the quoted context.) > > If you're asking whether you should use a cpu_ prefix instead of a soc_ > prefix for identifying s3c64xx, then consider the question. > > What is the CPU? > > What are S3C64xx / OMAP / PXA310 / SA-11x0 / AT91RM9200? CPU or SoC ? > What are Cortex A8/A9 / Xscale / StrongARM / ARM920? CPU or SoC ? Well, I believe that the context I quoted explains everything, but let me explain that again: The patch is about a driver being able to check to what SoC a device belongs, but there are at least two ways to achieve that. I can see many drivers using the way I used, i.e. distinct platform device names for each SoC that has to be differentiated plus a generic one. However, I was suggested that using cpu_is_*/soc_is_* might be a better approach, while at the same time I received an opinion that the original method is the preferred one. Best regards, Tom
On Tue, Sep 06, 2011 at 01:35:58PM +0200, Tomasz Figa wrote: > Well, I believe that the context I quoted explains everything, but let me > explain that again: > > The patch is about a driver being able to check to what SoC a device belongs, > but there are at least two ways to achieve that. > > I can see many drivers using the way I used, i.e. distinct platform device names > for each SoC that has to be differentiated plus a generic one. However, I was > suggested that using cpu_is_*/soc_is_* might be a better approach, while at the > same time I received an opinion that the original method is the preferred one. Ok. There's pros and cons here: Pros to soc_is_*: 1. Doesn't require additional storage in every driver but does require code. 2. Obvious what's going on for a specific SoC when reading the code. Cons: 1. Needs to be updated each time a new SoC is added. 2. Doesn't scale well - number of soc_is_* will grow to unmanageable levels over time. Pros to platform driver name: 1. Can re-use existing names if feature compatible without driver mods. 2. Scales well with an increasing number of SoCs. Cons: 1. People will hate having SoC names which don't refer to their exact SoC. 2. Probably requires storage of a set of flags in driver private data to identify SoC specific features. 3. Requires additional string space to identify each driver name. However, thinking about this more wrt DT, there's another aspect to this. Rather than encoding into the driver "this SoC has features and quirks X,Y,Z" maybe that information should be in the device tree itself. For example, SoC 1 has X and Z, SoC 2 has Y. Then a new SoC 3 comes along with X and Y but not Z. If X, Y, Z are encoded into DT then there's no need to touch the kernel to support SoC 3, not even to change driver names or soc_is_xxx macros etc. So maybe the answer is: platform driver name determines a set of feature flags for the driver in lieu of DT support, and when DT support comes along the flags are controlled by DT properties instead.
On Tue, Sep 06, 2011 at 01:04:30PM +0100, Russell King - ARM Linux wrote: > Pros to platform driver name: > 1. Can re-use existing names if feature compatible without driver mods. > 2. Scales well with an increasing number of SoCs. > Cons: > 1. People will hate having SoC names which don't refer to their exact SoC. > 2. Probably requires storage of a set of flags in driver private data > to identify SoC specific features. > 3. Requires additional string space to identify each driver name. There's also an issue with actually getting the devices together to register which causes fragility here. > However, thinking about this more wrt DT, there's another aspect to this. > Rather than encoding into the driver "this SoC has features and quirks > X,Y,Z" maybe that information should be in the device tree itself. For > example, SoC 1 has X and Z, SoC 2 has Y. Then a new SoC 3 comes along > with X and Y but not Z. If X, Y, Z are encoded into DT then there's no > need to touch the kernel to support SoC 3, not even to change driver > names or soc_is_xxx macros etc. This does depend pretty strongly on making sure that the SoC DT is distributed separately to the board DT - that should be solved now.
diff --git a/arch/arm/mach-s3c64xx/s3c6400.c b/arch/arm/mach-s3c64xx/s3c6400.c index 5e93fe3..6418832 100644 --- a/arch/arm/mach-s3c64xx/s3c6400.c +++ b/arch/arm/mach-s3c64xx/s3c6400.c @@ -38,6 +38,7 @@ #include <plat/sdhci.h> #include <plat/iic-core.h> #include <plat/onenand-core.h> +#include <plat/pwm-core.h> #include <mach/s3c6400.h> void __init s3c6400_map_io(void) @@ -55,6 +56,7 @@ void __init s3c6400_map_io(void) s3c_onenand_setname("s3c6400-onenand"); s3c64xx_onenand1_setname("s3c6400-onenand"); + s3c_pwm_setname("s3c64xx-pwm"); } void __init s3c6400_init_clocks(int xtal) diff --git a/arch/arm/mach-s3c64xx/s3c6410.c b/arch/arm/mach-s3c64xx/s3c6410.c index 312aa6b..4f66c25 100644 --- a/arch/arm/mach-s3c64xx/s3c6410.c +++ b/arch/arm/mach-s3c64xx/s3c6410.c @@ -41,6 +41,7 @@ #include <plat/adc-core.h> #include <plat/iic-core.h> #include <plat/onenand-core.h> +#include <plat/pwm-core.h> #include <mach/s3c6400.h> #include <mach/s3c6410.h> @@ -60,6 +61,7 @@ void __init s3c6410_map_io(void) s3c_onenand_setname("s3c6410-onenand"); s3c64xx_onenand1_setname("s3c6410-onenand"); s3c_cfcon_setname("s3c64xx-pata"); + s3c_pwm_setname("s3c64xx-pwm"); } void __init s3c6410_init_clocks(int xtal) diff --git a/arch/arm/plat-samsung/dev-pwm.c b/arch/arm/plat-samsung/dev-pwm.c index dab47b0..751286a 100644 --- a/arch/arm/plat-samsung/dev-pwm.c +++ b/arch/arm/plat-samsung/dev-pwm.c @@ -20,6 +20,7 @@ #include <mach/irqs.h> #include <plat/devs.h> +#include <plat/pwm-core.h> #define TIMER_RESOURCE_SIZE (1) @@ -51,3 +52,10 @@ struct platform_device s3c_device_timer[] = { [4] = { DEFINE_S3C_TIMER(4, IRQ_TIMER4) }, }; EXPORT_SYMBOL(s3c_device_timer); + +void s3c_pwm_setname(const char *name) +{ + int i; + for (i = 0; i < ARRAY_SIZE(s3c_device_timer); ++i) + s3c_device_timer[i].name = name; +} diff --git a/arch/arm/plat-samsung/include/plat/pwm-core.h b/arch/arm/plat-samsung/include/plat/pwm-core.h new file mode 100644 index 0000000..651d7a4 --- /dev/null +++ b/arch/arm/plat-samsung/include/plat/pwm-core.h @@ -0,0 +1,21 @@ +/* arch/arm/plat-samsung/include/plat/pwm-core.h + * + * Copyright 2011 Tomasz Figa <tomasz.figa at gmail.com> + * + * S3C - PWM Controller core functions + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. +*/ + +#ifndef __ASM_ARCH_PWM_CORE_H +#define __ASM_ARCH_PWM_CORE_H __FILE__ + +/* These functions are only for use with the core support code, such as + * the cpu specific initialisation code + */ + +extern void s3c_pwm_setname(const char *name); + +#endif /* __ASM_ARCH_PWM_H */ diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c index f37457c..27b5353 100644 --- a/arch/arm/plat-samsung/pwm.c +++ b/arch/arm/plat-samsung/pwm.c @@ -24,6 +24,11 @@ #include <plat/regs-timer.h> +enum soc_type { + TYPE_GENERIC, + TYPE_S3C64XX, +}; + struct pwm_device { struct list_head list; struct platform_device *pdev; @@ -380,11 +385,23 @@ static int s3c_pwm_resume(struct platform_device *pdev) #define s3c_pwm_resume NULL #endif +static struct platform_device_id s3c_pwm_driver_ids[] = { + { + .name = "s3c24xx-pwm", + .driver_data = TYPE_GENERIC, + }, { + .name = "s3c64xx-pwm", + .driver_data = TYPE_S3C64XX, + }, +}; +MODULE_DEVICE_TABLE(platform, s3c_pwm_driver_ids); + static struct platform_driver s3c_pwm_driver = { .driver = { - .name = "s3c24xx-pwm", + .name = "samsung-pwm", .owner = THIS_MODULE, }, + .id_table = s3c_pwm_driver_ids, .probe = s3c_pwm_probe, .remove = __devexit_p(s3c_pwm_remove), .suspend = s3c_pwm_suspend,