| Message ID | 20100302182939.GI3445@oksana.dev.rtsoft.ru |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello. Anton Vorontsov wrote: > Factor out some ahci_em_messages handling code from ahci_init_one(). > We would like to reuse it for non-PCI devices. > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> > --- > drivers/ata/ahci.c | 41 ++++++++++++++++++++++++----------------- > 1 files changed, 24 insertions(+), 17 deletions(-) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 18443e9..6694b17 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -3040,6 +3040,29 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host) > {} > #endif > > +static void ahci_set_em_messages(struct ahci_host_priv *hpriv, > + struct ata_port_info *pi) > +{ > + u8 messages; > + void __iomem *mmio = hpriv->mmio; > + u32 em_loc = readl(mmio + HOST_EM_LOC); > + u32 em_ctl = readl(mmio + HOST_EM_CTL); > + > + if (!ahci_em_messages || !(hpriv->cap & HOST_CAP_EMS)) > + return; > + > + messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; > + > + /* we only support LED message type right now */ > + if ((messages & 0x01) && (ahci_em_messages == 1)) { > + /* store em_loc */ > + hpriv->em_loc = ((em_loc >> 16) * 4); > Could drop unneeded parens, while at it... MBR, Sergei -- 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 03/02/2010 04:18 PM, Sergei Shtylyov wrote: > Hello. > > Anton Vorontsov wrote: > >> Factor out some ahci_em_messages handling code from ahci_init_one(). >> We would like to reuse it for non-PCI devices. >> >> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> >> --- >> drivers/ata/ahci.c | 41 ++++++++++++++++++++++++----------------- >> 1 files changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >> index 18443e9..6694b17 100644 >> --- a/drivers/ata/ahci.c >> +++ b/drivers/ata/ahci.c >> @@ -3040,6 +3040,29 @@ static inline void >> ahci_gtf_filter_workaround(struct ata_host *host) >> {} >> #endif >> >> +static void ahci_set_em_messages(struct ahci_host_priv *hpriv, >> + struct ata_port_info *pi) >> +{ >> + u8 messages; >> + void __iomem *mmio = hpriv->mmio; >> + u32 em_loc = readl(mmio + HOST_EM_LOC); >> + u32 em_ctl = readl(mmio + HOST_EM_CTL); >> + >> + if (!ahci_em_messages || !(hpriv->cap & HOST_CAP_EMS)) >> + return; >> + >> + messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; >> + >> + /* we only support LED message type right now */ >> + if ((messages & 0x01) && (ahci_em_messages == 1)) { >> + /* store em_loc */ >> + hpriv->em_loc = ((em_loc >> 16) * 4); > > Could drop unneeded parens, while at it... While not strictly necessary, parens often help with readability. I think the above code looks fine as-is. If the reader has to waste a few seconds recalling C's operator precedence, that detracts from the easy reading of the code. Not everyone is like me and has worked on a C compiler, you know ;-) Jeff -- 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. Jeff Garzik wrote: >>> Factor out some ahci_em_messages handling code from ahci_init_one(). >>> We would like to reuse it for non-PCI devices. >>> >>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> >>> --- >>> drivers/ata/ahci.c | 41 ++++++++++++++++++++++++----------------- >>> 1 files changed, 24 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >>> index 18443e9..6694b17 100644 >>> --- a/drivers/ata/ahci.c >>> +++ b/drivers/ata/ahci.c >>> @@ -3040,6 +3040,29 @@ static inline void >>> ahci_gtf_filter_workaround(struct ata_host *host) >>> {} >>> #endif >>> >>> +static void ahci_set_em_messages(struct ahci_host_priv *hpriv, >>> + struct ata_port_info *pi) >>> +{ >>> + u8 messages; >>> + void __iomem *mmio = hpriv->mmio; >>> + u32 em_loc = readl(mmio + HOST_EM_LOC); >>> + u32 em_ctl = readl(mmio + HOST_EM_CTL); >>> + >>> + if (!ahci_em_messages || !(hpriv->cap & HOST_CAP_EMS)) >>> + return; >>> + >>> + messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; >>> + >>> + /* we only support LED message type right now */ >>> + if ((messages & 0x01) && (ahci_em_messages == 1)) { >>> + /* store em_loc */ >>> + hpriv->em_loc = ((em_loc >> 16) * 4); >> >> Could drop unneeded parens, while at it... > > While not strictly necessary, parens often help with readability. I > think the above code looks fine as-is. If the reader has to waste a > few seconds recalling C's operator precedence, that detracts from the > easy reading of the code. Not everyone is like me and has worked on a > C compiler, you know ;-) Come on, parens around the right arg of = are pretty counter-intuitive and so don't add to the readability. :-) > Jeff MBR, Sergei -- 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 03/03/2010 05:40 AM, Sergei Shtylyov wrote: > Hello. > > Jeff Garzik wrote: > >>>> Factor out some ahci_em_messages handling code from ahci_init_one(). >>>> We would like to reuse it for non-PCI devices. >>>> >>>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> >>>> --- >>>> drivers/ata/ahci.c | 41 ++++++++++++++++++++++++----------------- >>>> 1 files changed, 24 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >>>> index 18443e9..6694b17 100644 >>>> --- a/drivers/ata/ahci.c >>>> +++ b/drivers/ata/ahci.c >>>> @@ -3040,6 +3040,29 @@ static inline void >>>> ahci_gtf_filter_workaround(struct ata_host *host) >>>> {} >>>> #endif >>>> >>>> +static void ahci_set_em_messages(struct ahci_host_priv *hpriv, >>>> + struct ata_port_info *pi) >>>> +{ >>>> + u8 messages; >>>> + void __iomem *mmio = hpriv->mmio; >>>> + u32 em_loc = readl(mmio + HOST_EM_LOC); >>>> + u32 em_ctl = readl(mmio + HOST_EM_CTL); >>>> + >>>> + if (!ahci_em_messages || !(hpriv->cap & HOST_CAP_EMS)) >>>> + return; >>>> + >>>> + messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; >>>> + >>>> + /* we only support LED message type right now */ >>>> + if ((messages & 0x01) && (ahci_em_messages == 1)) { >>>> + /* store em_loc */ >>>> + hpriv->em_loc = ((em_loc >> 16) * 4); >>> >>> Could drop unneeded parens, while at it... >> >> While not strictly necessary, parens often help with readability. I >> think the above code looks fine as-is. If the reader has to waste a >> few seconds recalling C's operator precedence, that detracts from the >> easy reading of the code. Not everyone is like me and has worked on a >> C compiler, you know ;-) > > Come on, parens around the right arg of = are pretty counter-intuitive > and so don't add to the readability. :-) <shrug> I respectfully disagree. It is wasteful but does not detract from readability at all, IMO. Jeff -- 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 Wed, Mar 3, 2010 at 6:41 AM, Jeff Garzik <jeff@garzik.org> wrote: > On 03/03/2010 05:40 AM, Sergei Shtylyov wrote: >> >> Hello. >> >> Jeff Garzik wrote: >> >>>>> Factor out some ahci_em_messages handling code from ahci_init_one(). >>>>> We would like to reuse it for non-PCI devices. >>>>> >>>>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> >>>>> --- >>>>> drivers/ata/ahci.c | 41 ++++++++++++++++++++++++----------------- >>>>> 1 files changed, 24 insertions(+), 17 deletions(-) >>>>> >>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >>>>> index 18443e9..6694b17 100644 >>>>> --- a/drivers/ata/ahci.c >>>>> +++ b/drivers/ata/ahci.c >>>>> @@ -3040,6 +3040,29 @@ static inline void >>>>> ahci_gtf_filter_workaround(struct ata_host *host) >>>>> {} >>>>> #endif >>>>> >>>>> +static void ahci_set_em_messages(struct ahci_host_priv *hpriv, >>>>> + struct ata_port_info *pi) >>>>> +{ >>>>> + u8 messages; >>>>> + void __iomem *mmio = hpriv->mmio; >>>>> + u32 em_loc = readl(mmio + HOST_EM_LOC); >>>>> + u32 em_ctl = readl(mmio + HOST_EM_CTL); >>>>> + >>>>> + if (!ahci_em_messages || !(hpriv->cap & HOST_CAP_EMS)) >>>>> + return; >>>>> + >>>>> + messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; >>>>> + >>>>> + /* we only support LED message type right now */ >>>>> + if ((messages & 0x01) && (ahci_em_messages == 1)) { >>>>> + /* store em_loc */ >>>>> + hpriv->em_loc = ((em_loc >> 16) * 4); >>>> >>>> Could drop unneeded parens, while at it... >>> >>> While not strictly necessary, parens often help with readability. I >>> think the above code looks fine as-is. If the reader has to waste a >>> few seconds recalling C's operator precedence, that detracts from the >>> easy reading of the code. Not everyone is like me and has worked on a >>> C compiler, you know ;-) >> >> Come on, parens around the right arg of = are pretty counter-intuitive >> and so don't add to the readability. :-) > > <shrug> I respectfully disagree. It is wasteful but does not detract from > readability at all, IMO. > > Jeff Re: hpriv->em_loc = ((em_loc >> 16) * 4); I think the way it is leaves the reader wondering what used to be there. ie. It makes me think there used to be a function call that was removed, but the parens left in place. I agree with Sergei that it is a readability issue because it sends the readers brain off into tangents. Greg -- 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
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 18443e9..6694b17 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -3040,6 +3040,29 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host) {} #endif +static void ahci_set_em_messages(struct ahci_host_priv *hpriv, + struct ata_port_info *pi) +{ + u8 messages; + void __iomem *mmio = hpriv->mmio; + u32 em_loc = readl(mmio + HOST_EM_LOC); + u32 em_ctl = readl(mmio + HOST_EM_CTL); + + if (!ahci_em_messages || !(hpriv->cap & HOST_CAP_EMS)) + return; + + messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; + + /* we only support LED message type right now */ + if ((messages & 0x01) && (ahci_em_messages == 1)) { + /* store em_loc */ + hpriv->em_loc = ((em_loc >> 16) * 4); + pi->flags |= ATA_FLAG_EM; + if (!(em_ctl & EM_CTL_ALHD)) + pi->flags |= ATA_FLAG_SW_ACTIVITY; + } +} + static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { static int printed_version; @@ -3143,23 +3166,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) if (hpriv->cap & HOST_CAP_PMP) pi.flags |= ATA_FLAG_PMP; - if (ahci_em_messages && (hpriv->cap & HOST_CAP_EMS)) { - u8 messages; - void __iomem *mmio = hpriv->mmio; - u32 em_loc = readl(mmio + HOST_EM_LOC); - u32 em_ctl = readl(mmio + HOST_EM_CTL); - - messages = (em_ctl & EM_CTRL_MSG_TYPE) >> 16; - - /* we only support LED message type right now */ - if ((messages & 0x01) && (ahci_em_messages == 1)) { - /* store em_loc */ - hpriv->em_loc = ((em_loc >> 16) * 4); - pi.flags |= ATA_FLAG_EM; - if (!(em_ctl & EM_CTL_ALHD)) - pi.flags |= ATA_FLAG_SW_ACTIVITY; - } - } + ahci_set_em_messages(hpriv, &pi); if (ahci_broken_system_poweroff(pdev)) { pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
Factor out some ahci_em_messages handling code from ahci_init_one(). We would like to reuse it for non-PCI devices. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> --- drivers/ata/ahci.c | 41 ++++++++++++++++++++++++----------------- 1 files changed, 24 insertions(+), 17 deletions(-)