| Message ID | 1389469372-17199-2-git-send-email-tomasz.figa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat 2014-01-11 20:42:43, Tomasz Figa wrote: > This patch removes name field from private s3c64xx_pm_domain struct and > moves domain name to dedicated field of generic_pm_domain struct. > > When at it, beautify the names a bit, since they are used by genpd core > as message prefixes. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > --- > arch/arm/mach-s3c64xx/pm.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c > index 8cdb824..5238d66 100644 > --- a/arch/arm/mach-s3c64xx/pm.c > +++ b/arch/arm/mach-s3c64xx/pm.c > @@ -35,7 +35,6 @@ > #include "regs-syscon-power.h" > > struct s3c64xx_pm_domain { > - char *const name; > u32 ena; > u32 pwr_stat; > struct generic_pm_domain pd; > @@ -76,7 +75,7 @@ static int s3c64xx_pd_on(struct generic_pm_domain *domain) > } while (retry--); > > if (!retry) { > - pr_err("Failed to start domain %s\n", pd->name); > + pr_err("Failed to start domain %s\n", pd->pd.name); > return -EBUSY; > } So you changed text "failed to start domain I" to "failed to start domain domain_i". Not sure that's an improvement...? Could we get some more descriptive names for the domains? > static struct s3c64xx_pm_domain s3c64xx_pm_i = { > - .name = "I", > .ena = S3C64XX_NORMALCFG_DOMAIN_I_ON, > .pwr_stat = S3C64XX_BLKPWRSTAT_I, > .pd = { > + .name = "domain_i", > .power_off = s3c64xx_pd_off, > .power_on = s3c64xx_pd_on, > }, > };
Hi Pavel, On 12.01.2014 12:47, Pavel Machek wrote: > On Sat 2014-01-11 20:42:43, Tomasz Figa wrote: >> This patch removes name field from private s3c64xx_pm_domain struct and >> moves domain name to dedicated field of generic_pm_domain struct. >> >> When at it, beautify the names a bit, since they are used by genpd core >> as message prefixes. >> >> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> >> --- >> arch/arm/mach-s3c64xx/pm.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c >> index 8cdb824..5238d66 100644 >> --- a/arch/arm/mach-s3c64xx/pm.c >> +++ b/arch/arm/mach-s3c64xx/pm.c >> @@ -35,7 +35,6 @@ >> #include "regs-syscon-power.h" >> >> struct s3c64xx_pm_domain { >> - char *const name; >> u32 ena; >> u32 pwr_stat; >> struct generic_pm_domain pd; >> @@ -76,7 +75,7 @@ static int s3c64xx_pd_on(struct generic_pm_domain *domain) >> } while (retry--); >> >> if (!retry) { >> - pr_err("Failed to start domain %s\n", pd->name); >> + pr_err("Failed to start domain %s\n", pd->pd.name); >> return -EBUSY; >> } > > > So you changed text "failed to start domain I" to "failed to start > domain domain_i". Not sure that's an improvement...? Right. I apparently missed this, since this error should never happen unless there is something wrong with your hardware (SoC or PMIC). Still, the generic code in drivers/base/power/domain.c does not add the "domain" prefix to the message. Without this patch it was printing messages like I: Power-on latency exceeded, new value 1000 ns I'd say that what's need adjustment are the messages in mach-s3c64xx/pm.c to print "%s: Failed to start\n" and keep things consistent with higher level code. > > Could we get some more descriptive names for the domains? They are listed like this in user's manual, e.g. DOMAIN_F, DOMAIN_I, DOMAIN_G, etc. Best regards, Tomasz
Hi! > >Could we get some more descriptive names for the domains? > > They are listed like this in user's manual, e.g. DOMAIN_F, DOMAIN_I, > DOMAIN_G, etc. I guessed so. So the manual sucks. Would it be feasible to get it more descriptive (like "Domain_G (GPU)")? Pavel
On 12.01.2014 19:53, Pavel Machek wrote: > Hi! > >>> Could we get some more descriptive names for the domains? >> >> They are listed like this in user's manual, e.g. DOMAIN_F, DOMAIN_I, >> DOMAIN_G, etc. > > I guessed so. So the manual sucks. Would it be feasible to get it more > descriptive (like "Domain_G (GPU)")? Does the name really matter that much? I changed original ones in this patch simply to make it easier to grep for domain related messages in kernel output (e.g. dmesg | grep domain). It was not the main part of this patch. Also, there are multiple devices in most domains, so that would rather end up being "Domain_I (JPEG, Cam I/F)", "Domain_P (2D, TV Enc., Scaler)", "Domain_F (Rot, Post, LCD)". If you really insist, I can change this, but I fail to see what effect it would really have. Best regards, Tomasz
On Sat, Jan 11, 2014 at 08:42:43PM +0100, Tomasz Figa wrote: > static struct s3c64xx_pm_domain s3c64xx_pm_irom = { > - .name = "IROM", > .ena = S3C64XX_NORMALCFG_IROM_ON, > .pd = { > + .name = "domain_irom", This is nitpicking a bit but are you sure this is actually a beautification of the name? It's mainly the domain_ bit, mostly since I'd expect that if it's not obvious that this is a power domain the core logging would be changed rather than tweaking the name of every power domain user.
On Sun, Jan 12, 2014 at 08:03:50PM +0100, Tomasz Figa wrote: > Also, there are multiple devices in most domains, so that would > rather end up being "Domain_I (JPEG, Cam I/F)", "Domain_P (2D, TV > Enc., Scaler)", "Domain_F (Rot, Post, LCD)". It's not just that there's multiple devices either, it's also the fact that the groupings are a bit random and probably more to do with chip layout or something than function.
On 12.01.2014 20:20, Mark Brown wrote: > On Sat, Jan 11, 2014 at 08:42:43PM +0100, Tomasz Figa wrote: > >> static struct s3c64xx_pm_domain s3c64xx_pm_irom = { >> - .name = "IROM", >> .ena = S3C64XX_NORMALCFG_IROM_ON, >> .pd = { >> + .name = "domain_irom", > > This is nitpicking a bit but are you sure this is actually a > beautification of the name? It's mainly the domain_ bit, mostly since > I'd expect that if it's not obvious that this is a power domain the core > logging would be changed rather than tweaking the name of every power > domain user. > Hmm, that's a really good point. A separate patch might change this and I could drop the "controversial" part of this patch ;). Best regards, Tomasz
diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c index 8cdb824..5238d66 100644 --- a/arch/arm/mach-s3c64xx/pm.c +++ b/arch/arm/mach-s3c64xx/pm.c @@ -35,7 +35,6 @@ #include "regs-syscon-power.h" struct s3c64xx_pm_domain { - char *const name; u32 ena; u32 pwr_stat; struct generic_pm_domain pd; @@ -76,7 +75,7 @@ static int s3c64xx_pd_on(struct generic_pm_domain *domain) } while (retry--); if (!retry) { - pr_err("Failed to start domain %s\n", pd->name); + pr_err("Failed to start domain %s\n", pd->pd.name); return -EBUSY; } } @@ -85,78 +84,78 @@ static int s3c64xx_pd_on(struct generic_pm_domain *domain) } static struct s3c64xx_pm_domain s3c64xx_pm_irom = { - .name = "IROM", .ena = S3C64XX_NORMALCFG_IROM_ON, .pd = { + .name = "domain_irom", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_etm = { - .name = "ETM", .ena = S3C64XX_NORMALCFG_DOMAIN_ETM_ON, .pwr_stat = S3C64XX_BLKPWRSTAT_ETM, .pd = { + .name = "domain_etm", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_s = { - .name = "S", .ena = S3C64XX_NORMALCFG_DOMAIN_S_ON, .pwr_stat = S3C64XX_BLKPWRSTAT_S, .pd = { + .name = "domain_s", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_f = { - .name = "F", .ena = S3C64XX_NORMALCFG_DOMAIN_F_ON, .pwr_stat = S3C64XX_BLKPWRSTAT_F, .pd = { + .name = "domain_f", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_p = { - .name = "P", .ena = S3C64XX_NORMALCFG_DOMAIN_P_ON, .pwr_stat = S3C64XX_BLKPWRSTAT_P, .pd = { + .name = "domain_p", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_i = { - .name = "I", .ena = S3C64XX_NORMALCFG_DOMAIN_I_ON, .pwr_stat = S3C64XX_BLKPWRSTAT_I, .pd = { + .name = "domain_i", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_g = { - .name = "G", .ena = S3C64XX_NORMALCFG_DOMAIN_G_ON, .pd = { + .name = "domain_g", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, }, }; static struct s3c64xx_pm_domain s3c64xx_pm_v = { - .name = "V", .ena = S3C64XX_NORMALCFG_DOMAIN_V_ON, .pwr_stat = S3C64XX_BLKPWRSTAT_V, .pd = { + .name = "domain_v", .power_off = s3c64xx_pd_off, .power_on = s3c64xx_pd_on, },
This patch removes name field from private s3c64xx_pm_domain struct and moves domain name to dedicated field of generic_pm_domain struct. When at it, beautify the names a bit, since they are used by genpd core as message prefixes. Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> --- arch/arm/mach-s3c64xx/pm.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)