MIME-Version: 1.0
References: <20230309225041.477440-1-sre@kernel.org> <20230309225041.477440-3-sre@kernel.org>
In-Reply-To: <20230309225041.477440-3-sre@kernel.org>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Fri, 10 Mar 2023 09:20:09 +0100
Message-ID: <CACRpkdZofL-cuYcyNAwMAshoQAr3z7-boJoHftVnjt80YQmAOQ@mail.gmail.com>
Subject: Re: [PATCHv1 02/11] power: supply: core: auto-exposure of
 simple-battery data
To:     Sebastian Reichel <sre@kernel.org>
Cc:     Matti Vaittinen <mazziesaccount@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Sebastian,

thanks for your patch!

On Thu, Mar 9, 2023 at 11:50=E2=80=AFPM Sebastian Reichel <sre@kernel.org> =
wrote:

> +       /*
> +        * Set if constant battery information from firmware should be
> +        * exposed automatically. No driver specific code is required
> +        * in that case. If the driver also handles a property provided
> +        * by constant firmware data, the driver's handler is preferred.
> +        */
> +       bool expose_battery_info;

Playing it safe with opt-in I see! But I would probably invert it and add
a hide_battery_info for those that don't wanna expose it. It seems
pretty useful to just expose this in general.

However I have no insight in what happens on laptops etc for this
so I guess you have your reasons, either way:

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> +extern bool power_supply_battery_info_has_prop(struct power_supply_batte=
ry_info *info,
> +                                              enum power_supply_property=
 psp);
> +extern int power_supply_battery_info_get_prop(struct power_supply_batter=
y_info *info,
> +                                             enum power_supply_property =
psp,
> +                                             union power_supply_propval =
*val);

I think the build robots complain because you need to add some stubs
for the not enabled case.

Yours,
Linus Walleij