From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755967AbbFRQhv (ORCPT <rfc822;w@1wt.eu>);
	Thu, 18 Jun 2015 12:37:51 -0400
Received: from mail-la0-f41.google.com ([209.85.215.41]:35046 "EHLO
	mail-la0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754505AbbFRQhn (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 18 Jun 2015 12:37:43 -0400
MIME-Version: 1.0
In-Reply-To: <55827400.2050904@arm.com>
References: <1434610052-602-1-git-send-email-bjorn.andersson@sonymobile.com>
	<1434610052-602-8-git-send-email-bjorn.andersson@sonymobile.com>
	<55827400.2050904@arm.com>
Date: Thu, 18 Jun 2015 09:37:41 -0700
Message-ID: <CAJAp7OgfkFjFi7_V93WRT+ySHOgvoCH4DpNKwrPgaOjggoSeww@mail.gmail.com>
Subject: Re: [PATCH 7/8] mfd: pm8921: Implement irq_get_irqchip_state
From: Bjorn Andersson <bjorn@kryo.se>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com>,
        Kumar Gala <galak@codeaurora.org>, Andy Gross <agross@codeaurora.org>,
        Samuel Ortiz <sameo@linux.intel.com>, Lee Jones <lee.jones@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "linux-soc@vger.kernel.org" <linux-soc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jun 18, 2015 at 12:32 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Bjorn,
>
[..]
>> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
[..]
>> +static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
>> +                                     enum irqchip_irq_state which,
>> +                                     bool *state)
>> +{
>> +     struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +     unsigned int pmirq = irqd_to_hwirq(d);
>> +     unsigned int bits;
>> +     int irq_bit;
>> +     u8 block;
>> +     int rc;
>> +
>> +     if (!chip) {
>> +             pr_err("Failed to resolve pm_irq_chip\n");
>> +             return -EINVAL;
>> +     }
>
> Why do you need to check this? Is there any code path that could
> actually trigger this?
>

This is a remnant of times before our new fancy api, from what I can
see it should be dropped.

>> +
>> +     if (which != IRQCHIP_STATE_LINE_LEVEL)
>> +             return -EINVAL;
>> +
>> +     block = pmirq / 8;
>> +     irq_bit = pmirq % 8;
>> +
>> +     spin_lock(&chip->pm_irq_lock);
>> +     rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> +     if (rc) {
>> +             pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
>> +             goto bail;
>> +     }
>> +
>> +     rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> +     if (rc) {
>> +             pr_err("Failed Reading Status rc=%d\n", rc);
>> +             goto bail;
>> +     }
>> +
>> +     *state = !!(bits & BIT(irq_bit));
>> +bail:
>> +     spin_unlock(&chip->pm_irq_lock);
>> +
>> +     return rc ? rc : 0;
>
> I think you can just have "return rc;" here.
>

You're right.

Thanks for the review!

Regards,
Bjorn