Received-SPF: pass (google.com: domain of linux-kernel+bounces-212768-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Thu, 13 Jun 2024 10:57:41 +0300 (EEST)
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, 
    Sebastian Reichel <sre@kernel.org>, linux-pm@vger.kernel.org
cc: Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, 
    Conor Dooley <conor+dt@kernel.org>, Bjorn Andersson <andersson@kernel.org>, 
    Hans de Goede <hdegoede@redhat.com>, 
    Bryan O'Donoghue <bryan.odonoghue@linaro.org>, 
    Heikki Krogerus <heikki.krogerus@linux.intel.com>, 
    Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
    Konrad Dybcio <konrad.dybcio@linaro.org>, devicetree@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>, platform-driver-x86@vger.kernel.org, 
    linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
    Nikita Travkin <nikita@trvn.ru>
Subject: Re: [PATCH v6 4/6] power: supply: lenovo_yoga_c630_battery: add
 Lenovo C630 driver
In-Reply-To: <20240612-yoga-ec-driver-v6-4-8e76ba060439@linaro.org>
Message-ID: <4fc43c56-f801-909a-9178-166d275a5fee@linux.intel.com>
References: <20240612-yoga-ec-driver-v6-0-8e76ba060439@linaro.org> <20240612-yoga-ec-driver-v6-4-8e76ba060439@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Wed, 12 Jun 2024, Dmitry Baryshkov wrote:

> On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> and battery status. Add the driver to read power supply status on the
> laptop.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

> +/* the mutex should already be locked */

Enforce this with lockdep_assert_held() (and remove the comment).

> +static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int val;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
> +	if (val < 0)
> +		return val;
> +	ecbat->bat_present = !!(val & LENOVO_EC_BAT_PRESENT_IS_PRESENT);
> +	if (!ecbat->bat_present)
> +		return val;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> +	if (val < 0)
> +		return val;
> +	ecbat->unit_mA = val & LENOVO_EC_BAT_ATTRIBUTES_UNIT_IS_MA;
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> +	if (val < 0)
> +		return val;
> +	ecbat->design_capacity = val * 1000;
> +
> +	/*
> +	 * DSDT has delays after most of EC reads in these methods.
> +	 * Having no documentation for the EC we have to follow and sleep here.
> +	 */
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE);
> +	if (val < 0)
> +		return val;
> +	ecbat->design_voltage = val;
> +
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER);
> +	if (val < 0)
> +		return val;
> +	val = yoga_c630_ec_read16(ec,
> +				  val & LENOVO_EC_BAT_FULL_REGISTER_IS_FACTORY ?
> +				  LENOVO_EC_BAT_FULL_FACTORY :
> +				  LENOVO_EC_BAT_FULL_CAPACITY);
> +	if (val < 0)
> +		return val;
> +
> +	ecbat->full_charge_capacity = val * 1000;
> +
> +	if (!ecbat->unit_mA) {
> +		ecbat->design_capacity *= 10;
> +		ecbat->full_charge_capacity *= 10;
> +	}
> +
> +	return 0;
> +}
> +
> +static int yoga_c630_psy_maybe_update_bat_status(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int current_mA;
> +	int val;
> +
> +	scoped_guard(mutex, &ecbat->lock) {

This too could be simply guard() to bring down the indentation level.

> +		if (time_before(jiffies, ecbat->last_status_update + LENOVO_EC_CACHE_TIME))
> +			return 0;
> +
> +		val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_STATUS);
> +		if (val < 0)
> +			return val;
> +		ecbat->bat_status = val;
> +
> +		msleep(50);
> +
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_REMAIN_CAPACITY);
> +		if (val < 0)
> +			return val;
> +		ecbat->capacity_now = val * 1000;
> +
> +		msleep(50);
> +
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
> +		if (val < 0)
> +			return val;
> +		ecbat->voltage_now = val * 1000;
> +
> +		msleep(50);
> +
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
> +		if (val < 0)
> +			return val;
> +		current_mA = sign_extend32(val, 15);
> +		ecbat->current_now = current_mA * 1000;
> +		ecbat->rate_now = current_mA * (ecbat->voltage_now / 1000);
> +
> +		msleep(50);
> +
> +		if (!ecbat->unit_mA)
> +			ecbat->capacity_now *= 10;
> +
> +		ecbat->last_status_update = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int yoga_c630_psy_update_adapter_status(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int val;
> +
> +	scoped_guard(mutex, &ecbat->lock) {

Ditto.

> +		val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
> +		if (val < 0)
> +			return val;
> +
> +		ecbat->adapter_online = !!(val & LENOVO_EC_ADPT_STATUS_PRESENT);
> +	}
> +
> +	return 0;
> +}


> +static const struct power_supply_desc yoga_c630_psy_bat_psy_desc_mA = {
> +	.name = "yoga-c630-battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.properties = yoga_c630_psy_bat_mA_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_bat_mA_properties),
> +	.get_property = yoga_c630_psy_bat_get_property,
> +};
> +
> +static const struct power_supply_desc yoga_c630_psy_bat_psy_desc_mWh = {
> +	.name = "yoga-c630-battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.properties = yoga_c630_psy_bat_mWh_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_bat_mWh_properties),
> +	.get_property = yoga_c630_psy_bat_get_property,
> +};
> +
> +static int yoga_c630_psy_adpt_get_property(struct power_supply *psy,
> +					  enum power_supply_property psp,
> +					  union power_supply_propval *val)
> +{
> +	struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
> +	int ret = 0;
> +
> +	ret = yoga_c630_psy_update_adapter_status(ecbat);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = ecbat->adapter_online;
> +		break;
> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		val->intval = POWER_SUPPLY_USB_TYPE_C;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property yoga_c630_psy_adpt_properties[] = {
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +};
> +
> +static const enum power_supply_usb_type yoga_c630_psy_adpt_usb_type[] = {
> +	POWER_SUPPLY_USB_TYPE_C,
> +};
> +
> +static const struct power_supply_desc yoga_c630_psy_adpt_psy_desc = {
> +	.name = "yoga-c630-adapter",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.usb_types = yoga_c630_psy_adpt_usb_type,
> +	.num_usb_types = ARRAY_SIZE(yoga_c630_psy_adpt_usb_type),
> +	.properties = yoga_c630_psy_adpt_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_adpt_properties),
> +	.get_property = yoga_c630_psy_adpt_get_property,
> +};
> +
> +static int yoga_c630_psy_register_bat_psy(struct yoga_c630_psy *ecbat)
> +{
> +	struct power_supply_config bat_cfg = {};
> +
> +	bat_cfg.drv_data = ecbat;
> +	bat_cfg.fwnode = ecbat->fwnode;
> +	ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev,
> +						     ecbat->unit_mA ?
> +						     &yoga_c630_psy_bat_psy_desc_mA :
> +						     &yoga_c630_psy_bat_psy_desc_mWh,
> +						     &bat_cfg);
> +	if (IS_ERR(ecbat->bat_psy)) {
> +		dev_err(ecbat->dev, "failed to register battery supply\n");
> +		return PTR_ERR(ecbat->bat_psy);
> +	}
> +
> +	return 0;
> +}
> +
> +static void yoga_c630_ec_refresh_bat_info(struct yoga_c630_psy *ecbat)
> +{
> +	bool current_unit;
> +
> +	scoped_guard(mutex, &ecbat->lock) {

guard()

> +		current_unit = ecbat->unit_mA;
> +
> +		yoga_c630_psy_update_bat_info(ecbat);
> +
> +		if (current_unit != ecbat->unit_mA) {
> +			power_supply_unregister(ecbat->bat_psy);
> +			yoga_c630_psy_register_bat_psy(ecbat);
> +		}
> +	}
> +}


> +	adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name;

This is not problem with your patch but I'm wondering why supplied_to 
needs to be non-const char *. Are those strings expected to be altered by 
something, I couldn't find anything to that effect (the pointer itself 
does not become const if supplied_to is changed to const char **)?

--
 i.