Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [linux-sunxi] [PATCH v2 2/9] power: supply: axp20x_ac_power: Fix
 reporting online status
To:     Julian Calaby <julian.calaby@gmail.com>
Cc:     Chen-Yu Tsai <wens@csie.org>, Sebastian Reichel <sre@kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Oskari Lemmela <oskari@lemmela.net>,
        Quentin Schulz <quentin.schulz@bootlin.com>,
        linux-pm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        linux-sunxi <linux-sunxi@googlegroups.com>,
        stable@vger.kernel.org
References: <20200105012416.23296-1-samuel@sholland.org>
 <20200105012416.23296-3-samuel@sholland.org>
 <CAGRGNgXeenNYMNXY0dewQaeG2QecUPgE_MOofURg7HzcND782w@mail.gmail.com>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <6e299dd3-3d70-9306-8581-798927241779@sholland.org>
Date:   Sun, 5 Jan 2020 09:27:07 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <CAGRGNgXeenNYMNXY0dewQaeG2QecUPgE_MOofURg7HzcND782w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Julian,

On 1/5/20 7:00 AM, Julian Calaby wrote:
> On Sun, Jan 5, 2020 at 12:24 PM Samuel Holland <samuel@sholland.org> wrote:
>>
>> AXP803/AXP813 have a flag that enables/disables the AC power supply
>> input. This flag does not affect the status bits in PWR_INPUT_STATUS.
>> Its effect can be verified by checking the battery charge/discharge
>> state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on
>> the AC input.
>>
>> Take this flag into account when getting the ONLINE property of the AC
>> input, on PMICs where this flag is present.
>>
>> Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
>> index 0d34a932b6d5..ca0a28f72a27 100644
>> --- a/drivers/power/supply/axp20x_ac_power.c
>> +++ b/drivers/power/supply/axp20x_ac_power.c
>> @@ -23,6 +23,8 @@
>>  #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
>>  #define AXP20X_PWR_STATUS_ACIN_AVAIL   BIT(6)
>>
>> +#define AXP813_ACIN_PATH_SEL           BIT(7)
>> +
>>  #define AXP813_VHOLD_MASK              GENMASK(5, 3)
>>  #define AXP813_VHOLD_UV_TO_BIT(x)      ((((x) / 100000) - 40) << 3)
>>  #define AXP813_VHOLD_REG_TO_UV(x)      \
>> @@ -40,6 +42,7 @@ struct axp20x_ac_power {
>>         struct power_supply *supply;
>>         struct iio_channel *acin_v;
>>         struct iio_channel *acin_i;
>> +       bool has_acin_path_sel;
>>  };
>>
>>  static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
>> @@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy,
>>                         return ret;
>>
>>                 val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL);
>> +
>> +               /* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */
>> +               if (power->has_acin_path_sel) {
> 
> Do we need to check this bit if ACIN_AVAIL is not set?

No, we don't. However due to regcache this won't actually cause another read
from the device. If I send a v3, I'll move the && to  the if statement.

>> +                       ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL,
>> +                                         &reg);
>> +                       if (ret)
>> +                               return ret;
>> +
>> +                       val->intval &= !!(reg & AXP813_ACIN_PATH_SEL);
> 
> If we only check this bit if ACIN_AVAIL is set, then we don't need the
> "&" in the "&=". (I'm assuming that val->intval is an int, not a bool,
> otherwise this is the wrong operator)

val->intval is an int, but it only ever takes the values 0 or 1. The !!
expression coerces an integer to the range of a boolean. So the two ways of
deriving the value ("&=" here vs "&& val->intval" in the if statement) are
equivalent.

> Thanks,

Thanks!
Samuel