| Message ID | 1397529703-21165-1-git-send-email-shawn.guo@freescale.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Apr 15, 2014 at 10:41:42AM +0800, Shawn Guo wrote: > Comparing to enums, macros are more conventional to be used for > registers and bits definition. Let's switch to macros. > > While at it, the names of the registers and bit-fields are updated to > have proper namespace prefix and match the hardware reference manual. > > No functional change is involved. > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> Please don't switch to defines. A lot of libata constants use enums and they tend to have more advantages anyway. Thanks.
On Tue, Apr 15, 2014 at 12:03:17PM -0400, Tejun Heo wrote: > On Tue, Apr 15, 2014 at 10:41:42AM +0800, Shawn Guo wrote: > > Comparing to enums, macros are more conventional to be used for > > registers and bits definition. Let's switch to macros. > > > > While at it, the names of the registers and bit-fields are updated to > > have proper namespace prefix and match the hardware reference manual. > > > > No functional change is involved. > > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > > Please don't switch to defines. A lot of libata constants use enums > and they tend to have more advantages anyway. I'm seeing a lot of ata drivers use defines though. And I'm not sure I understand the advantages of using enums over defines in ahci_imx driver, where register offset and bit position are defined in the same 'enum' without a particular enum type. enum { PORT_PHY_CTL = 0x178, /* Port0 PHY Control */ PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ }; To me it's a misuse of enum. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 15, 2014 at 10:41:42AM +0800, Shawn Guo wrote: > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > index 497c7ab..39629b4 100644 > --- a/drivers/ata/ahci_imx.c > +++ b/drivers/ata/ahci_imx.c > @@ -28,11 +28,9 @@ > #include <linux/libata.h> > #include "ahci.h" > > -enum { > - PORT_PHY_CTL = 0x178, /* Port0 PHY Control */ > - PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ > - HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ > -}; > +#define IMX_SATA_TIMER1MS 0x00e0 > +#define IMX_SATA_P0PHYCR 0x0178 > +#define P0PHYCR_TEST_PDDQ (1 << 20) Did you intentionally not namespace the bit mask? Best regards Uwe
On Wed, Apr 16, 2014 at 09:59:57AM +0200, Uwe Kleine-König wrote: > On Tue, Apr 15, 2014 at 10:41:42AM +0800, Shawn Guo wrote: > > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > > index 497c7ab..39629b4 100644 > > --- a/drivers/ata/ahci_imx.c > > +++ b/drivers/ata/ahci_imx.c > > @@ -28,11 +28,9 @@ > > #include <linux/libata.h> > > #include "ahci.h" > > > > -enum { > > - PORT_PHY_CTL = 0x178, /* Port0 PHY Control */ > > - PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ > > - HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ > > -}; > > +#define IMX_SATA_TIMER1MS 0x00e0 > > +#define IMX_SATA_P0PHYCR 0x0178 > > +#define P0PHYCR_TEST_PDDQ (1 << 20) > Did you intentionally not namespace the bit mask? Yes, with the intention to make the bit names a little shorter. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Apr 16, 2014 at 02:35:12PM +0800, Shawn Guo wrote: > I'm seeing a lot of ata drivers use defines though. And I'm not sure I > understand the advantages of using enums over defines in ahci_imx > driver, where register offset and bit position are defined in the same > 'enum' without a particular enum type. > > enum { > PORT_PHY_CTL = 0x178, /* Port0 PHY Control */ > PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ > HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ > }; > > To me it's a misuse of enum. The difference isn't that significant but enums have the advantages of the definitions being evaluated once rather than each time it's used which prevents things like missing parentheses subtly messing up the containing expressions and the compiler, and thus the debugger, being aware of the constants in use. The technical advantages not being enormous, it's a good bike shedding material and argue about it back and forth forever without being productive at all, so, please just stick with enums. Thanks.
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 497c7ab..39629b4 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -28,11 +28,9 @@ #include <linux/libata.h> #include "ahci.h" -enum { - PORT_PHY_CTL = 0x178, /* Port0 PHY Control */ - PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ - HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ -}; +#define IMX_SATA_TIMER1MS 0x00e0 +#define IMX_SATA_P0PHYCR 0x0178 +#define P0PHYCR_TEST_PDDQ (1 << 20) enum ahci_imx_type { AHCI_IMX53, @@ -156,8 +154,8 @@ static void ahci_imx_error_handler(struct ata_port *ap) * without full reset once the pddq mode is enabled making it * impossible to use as part of libata LPM. */ - reg_val = readl(mmio + PORT_PHY_CTL); - writel(reg_val | PORT_PHY_CTL_PDDQ_LOC, mmio + PORT_PHY_CTL); + reg_val = readl(mmio + IMX_SATA_P0PHYCR); + writel(reg_val | P0PHYCR_TEST_PDDQ, mmio + IMX_SATA_P0PHYCR); imx_sata_disable(hpriv); imxpriv->no_device = true; } @@ -248,7 +246,7 @@ static int imx_ahci_probe(struct platform_device *pdev) /* * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL, - * and IP vendor specific register HOST_TIMER1MS. + * and IP vendor specific register IMX_SATA_TIMER1MS. * Configure CAP_SSS (support stagered spin up). * Implement the port0. * Get the ahb clock rate, and configure the TIMER1MS register. @@ -265,7 +263,7 @@ static int imx_ahci_probe(struct platform_device *pdev) } reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000; - writel(reg_val, hpriv->mmio + HOST_TIMER1MS); + writel(reg_val, hpriv->mmio + IMX_SATA_TIMER1MS); ret = ahci_platform_init_host(pdev, hpriv, &ahci_imx_port_info, 0, 0); if (ret)
Comparing to enums, macros are more conventional to be used for registers and bits definition. Let's switch to macros. While at it, the names of the registers and bit-fields are updated to have proper namespace prefix and match the hardware reference manual. No functional change is involved. Signed-off-by: Shawn Guo <shawn.guo@freescale.com> --- drivers/ata/ahci_imx.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)