Patchwork [09/12] ahci: Introduce ahci_set_em_messages()

login
register
mail settings
Submitter Anton Vorontsov
Date March 2, 2010, 6:29 p.m.
Message ID <20100302182939.GI3445@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/46682/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Anton Vorontsov - March 2, 2010, 6:29 p.m.
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(-)
Sergei Shtylyov - March 2, 2010, 9:18 p.m.
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
Jeff Garzik - March 2, 2010, 11:19 p.m.
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
Sergei Shtylyov - March 3, 2010, 10:40 a.m.
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
Jeff Garzik - March 3, 2010, 11:41 a.m.
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
Greg Freemyer - March 3, 2010, 2:38 p.m.
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

Patch

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;