Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 08:17:46 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Bjorn Andersson <andersson@kernel.org>
Cc:     Sebastian Reichel <sre@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver
Message-ID: <20220913081746.wjj5jne6fjbzcjxr@bullseye-11-arm64.lan>
References: <20220810030500.2793882-1-bjorn.andersson@linaro.org>
 <20220810030500.2793882-3-bjorn.andersson@linaro.org>
 <20220912165802.dvlf3eeqx5g3bzdm@maple.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220912165802.dvlf3eeqx5g3bzdm@maple.lan>
Precedence: bulk

On Mon, Sep 12, 2022 at 05:00:17PM +0000, Daniel Thompson wrote:
> On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > providing AC-adapter and battery status, as well as USB Type-C altmode
> > notifications for Displayport operation.
>
> There's a couple of minor review comments but before we get to that:
> woo hoo!

... and now with the correct address for Bjorn too (comments still below
and indented > one level).


Daniel.


> > The Yoga C630 ships with Windows, where these operations primarily are
> > implemented in ACPI, but due to various issues with the hardware
> > representation therein it's not possible to run Linux on this
> > information. As such this is a best-effort re-implementation of these
> > operations, based on the register map expressed in ACPI and a fair
> > amount of trial and error.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >  drivers/power/supply/Kconfig        |  11 +
> >  drivers/power/supply/Makefile       |   1 +
> >  drivers/power/supply/yoga-c630-ec.c | 547 ++++++++++++++++++++++++++++
> >  3 files changed, 559 insertions(+)
> >  create mode 100644 drivers/power/supply/yoga-c630-ec.c
> >
> > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> > index 1aa8323ad9f6..6e706e948ad2 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -897,4 +897,15 @@ config BATTERY_UG3105
> >  	  device is off or suspended, the functionality of this driver is
> >  	  limited to reporting capacity only.
> >
> > +config LENOVO_YOGA_C630_EC
> > +	tristate "Lenovo Yoga C630 EC battery driver"
> > +	depends on DRM
> > +	depends on I2C
>
> This needs a "depends on TYPEC" in order to avoid linker errors.
>
>
> > +	help
> > +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> > +	  Lenovo Yoga C630, which provides battery information and USB Type-C
> > +	  altmode notifications.
> > +
> > +	  Say M or Y here to include this support.
> > +
> >  endif # POWER_SUPPLY
> > diff --git a/drivers/power/supply/yoga-c630-ec.c b/drivers/power/supply/yoga-c630-ec.c
> > new file mode 100644
> > index 000000000000..1fa0b5844e01
> > --- /dev/null
> > +++ b/drivers/power/supply/yoga-c630-ec.c
> > @@ -0,0 +1,547 @@
> > <snip>
> > +static int yoga_c630_ec_bat_get_property(struct power_supply *psy,
> > +					 enum power_supply_property psp,
> > +					 union power_supply_propval *val)
> > +{
> > +	struct yoga_c630_ec *ec = power_supply_get_drvdata(psy);
> > +	int rc = 0;
> > +
> > +	if (ec->bat_present)
> > +		yoga_c630_ec_maybe_update_bat_status(ec);
> > +	else if (psp != POWER_SUPPLY_PROP_PRESENT)
> > +		return -ENODEV;
> > +
> > +	switch (psp) {
> >       <snip>
> > +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> > +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> > +		break;
> > +	case POWER_SUPPLY_PROP_MODEL_NAME:
> > +		val->strval = "PABAS0241231";
> > +		break;
> > +	case POWER_SUPPLY_PROP_MANUFACTURER:
> > +		val->strval = "Compal";
> > +		break;
> > +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > +		val->strval = "05072018";
> > +		break;
>
> I'm a little sceptical that hardcoding a serial number into the
> driver provides anybody any benefit (regardless of whether the
> AML code does this). AFAICT this is not a commonly implemented property
> in other power supplies so I'm not clear why this is needed.
>
>
> Daniel.