| Message ID | 1390422036-31947-5-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Warren |
| Headers | show |
On Wed, Jan 22, 2014 at 01:20:35PM -0700, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Since all code that sets or interprets MASK_BITS_* now uses the enums > to define/compare the values, there is no need for MASK_BITS_* to have > a specific integer value. In fact, having a specific integer value may > encourage people to hard-code those values, or interpret the values in > incorrect ways. > > As such, remove the logic that assigns a specific value to the enum > values in order to make it completely clear that it's just an enum, not > something that directly represents some integer value. Ah yes, that's a nice way to clean it up as well, so my earlier comment about turning these into defines can be considered obsolete. > diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h > index cb89bd91f40c..357d9c592b55 100644 > --- a/arch/arm/include/asm/arch-tegra/clock.h > +++ b/arch/arm/include/asm/arch-tegra/clock.h > @@ -30,7 +30,7 @@ enum clock_osc_freq { > * them all together and pretends they're all 31:28. > */ > enum { > - MASK_BITS_31_30 = 2, /* num of bits used to specify clock source */ > + MASK_BITS_31_30, /* num of bits used to specify clock source */ Should the comment not be removed as well now? Thierry
On 01/24/2014 06:50 AM, Thierry Reding wrote: > On Wed, Jan 22, 2014 at 01:20:35PM -0700, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> Since all code that sets or interprets MASK_BITS_* now uses the enums >> to define/compare the values, there is no need for MASK_BITS_* to have >> a specific integer value. In fact, having a specific integer value may >> encourage people to hard-code those values, or interpret the values in >> incorrect ways. >> >> As such, remove the logic that assigns a specific value to the enum >> values in order to make it completely clear that it's just an enum, not >> something that directly represents some integer value. > > Ah yes, that's a nice way to clean it up as well, so my earlier comment > about turning these into defines can be considered obsolete. I guess I should read all the replies first:-) >> diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h >> enum { >> - MASK_BITS_31_30 = 2, /* num of bits used to specify clock source */ >> + MASK_BITS_31_30, /* num of bits used to specify clock source */ > > Should the comment not be removed as well now? Well, it the enum name still defines the position (and size), just not the value any more:-) But yes, it's obvious enough without the commment, so I'll remove that.
diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h index cb89bd91f40c..357d9c592b55 100644 --- a/arch/arm/include/asm/arch-tegra/clock.h +++ b/arch/arm/include/asm/arch-tegra/clock.h @@ -30,7 +30,7 @@ enum clock_osc_freq { * them all together and pretends they're all 31:28. */ enum { - MASK_BITS_31_30 = 2, /* num of bits used to specify clock source */ + MASK_BITS_31_30, /* num of bits used to specify clock source */ MASK_BITS_31_29, MASK_BITS_31_28, };