| Message ID | 20240528-yoga-ec-driver-v4-0-4fa8dfaae7b6@linaro.org |
|---|---|
| Headers | show |
| Series | power: supply: Lenovo Yoga C630 EC | expand |
On Tue, 28 May 2024, Dmitry Baryshkov wrote: > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in > the onboard EC. Add glue driver to interface the platform's UCSI > implementation. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/usb/typec/ucsi/Kconfig | 9 ++ > drivers/usb/typec/ucsi/Makefile | 1 + > drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > index bdcb1764cfae..680e1b87b152 100644 > --- a/drivers/usb/typec/ucsi/Kconfig > +++ b/drivers/usb/typec/ucsi/Kconfig > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK > To compile the driver as a module, choose M here: the module will be > called ucsi_glink. > > +config UCSI_LENOVO_YOGA_C630 > + tristate "UCSI Interface Driver for Lenovo Yoga C630" > + depends on EC_LENOVO_YOGA_C630 > + help > + This driver enables UCSI support on the Lenovo Yoga C630 laptop. > + > + To compile the driver as a module, choose M here: the module will be > + called ucsi_yoga_c630. > + > endif > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > index b4679f94696b..aed41d23887b 100644 > --- a/drivers/usb/typec/ucsi/Makefile > +++ b/drivers/usb/typec/ucsi/Makefile > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > new file mode 100644 > index 000000000000..ca1ab5c81b87 > --- /dev/null > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > @@ -0,0 +1,189 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2024, Linaro Ltd > + * Authors: > + * Bjorn Andersson > + * Dmitry Baryshkov > + */ > +#include <linux/auxiliary_bus.h> > +#include <linux/module.h> > +#include <linux/platform_data/lenovo-yoga-c630.h> > + > +#include "ucsi.h" > + > +struct yoga_c630_ucsi { > + struct yoga_c630_ec *ec; > + struct ucsi *ucsi; > + struct notifier_block nb; > + struct completion complete; Add includes for what you used here. > + unsigned long flags; > +#define UCSI_C630_COMMAND_PENDING 0 > +#define UCSI_C630_ACK_PENDING 1 > + u16 version; > +}; > + > +static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset, extra space > + void *val, size_t val_len) > +{ > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); Missing include for ucsi_get_drvdata > + u8 buf[YOGA_C630_UCSI_READ_SIZE]; > + int ret; > + > + ret = yoga_c630_ec_ucsi_read(uec->ec, buf); > + if (ret) > + return ret; > + > + if (offset == UCSI_VERSION) { > + memcpy(val, &uec->version, min(val_len, sizeof(uec->version))); > + return 0; > + } > + > + if (offset == UCSI_CCI) > + memcpy(val, buf, > + min(val_len, YOGA_C630_UCSI_CCI_SIZE)); Fits to one line. > + else if (offset == UCSI_MESSAGE_IN) > + memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE, > + min(val_len, YOGA_C630_UCSI_DATA_SIZE)); > + else > + return -EINVAL; > + > + return 0; > +} > + > +static int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, extra space, there seems to be more of them below but I won't mark them. > + const void *val, size_t val_len) > +{ > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > + > + if (offset != UCSI_CONTROL || > + val_len != YOGA_C630_UCSI_WRITE_SIZE) > + return -EINVAL; > + > + return yoga_c630_ec_ucsi_write(uec->ec, val); > +} > + > +static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset, > + const void *val, size_t val_len) > +{ > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; > + int ret; > + > + if (ack) > + set_bit(UCSI_C630_ACK_PENDING, &uec->flags); > + else > + set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); Include for set_bit() > + reinit_completion(&uec->complete); > + > + ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len); > + if (ret) > + goto out_clear_bit; > + > + if (!wait_for_completion_timeout(&uec->complete, 5 * HZ)) > + ret = -ETIMEDOUT; > + > +out_clear_bit: > + if (ack) > + clear_bit(UCSI_C630_ACK_PENDING, &uec->flags); > + else > + clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > + > + return ret; > +} > + > +const struct ucsi_operations yoga_c630_ucsi_ops = { Include for ucsi_operations. > + .read = yoga_c630_ucsi_read, > + .sync_write = yoga_c630_ucsi_sync_write, > + .async_write = yoga_c630_ucsi_async_write, > +}; > + > +static int yoga_c630_ucsi_notify(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb); Include for container_of
On Wed, 29 May 2024 at 17:20, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Tue, 28 May 2024, Dmitry Baryshkov wrote: > > > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in > > the onboard EC. Add glue driver to interface the platform's UCSI > > implementation. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/usb/typec/ucsi/Kconfig | 9 ++ > > drivers/usb/typec/ucsi/Makefile | 1 + > > drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++ > > 3 files changed, 199 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > index bdcb1764cfae..680e1b87b152 100644 > > --- a/drivers/usb/typec/ucsi/Kconfig > > +++ b/drivers/usb/typec/ucsi/Kconfig > > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK > > To compile the driver as a module, choose M here: the module will be > > called ucsi_glink. > > > > +config UCSI_LENOVO_YOGA_C630 > > + tristate "UCSI Interface Driver for Lenovo Yoga C630" > > + depends on EC_LENOVO_YOGA_C630 > > + help > > + This driver enables UCSI support on the Lenovo Yoga C630 laptop. > > + > > + To compile the driver as a module, choose M here: the module will be > > + called ucsi_yoga_c630. > > + > > endif > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > > index b4679f94696b..aed41d23887b 100644 > > --- a/drivers/usb/typec/ucsi/Makefile > > +++ b/drivers/usb/typec/ucsi/Makefile > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o > > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > new file mode 100644 > > index 000000000000..ca1ab5c81b87 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > @@ -0,0 +1,189 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2022-2024, Linaro Ltd > > + * Authors: > > + * Bjorn Andersson > > + * Dmitry Baryshkov > > + */ > > +#include <linux/auxiliary_bus.h> > > +#include <linux/module.h> > > +#include <linux/platform_data/lenovo-yoga-c630.h> > > + > > +#include "ucsi.h" > > + > > +struct yoga_c630_ucsi { > > + struct yoga_c630_ec *ec; > > + struct ucsi *ucsi; > > + struct notifier_block nb; > > + struct completion complete; > > Add includes for what you used here. > > > + unsigned long flags; > > +#define UCSI_C630_COMMAND_PENDING 0 > > +#define UCSI_C630_ACK_PENDING 1 > > + u16 version; > > +}; > > + > > +static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset, > > extra space > > > + void *val, size_t val_len) > > +{ > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > Missing include for ucsi_get_drvdata I'll review my includes, but this comment and the comment for ucsi_operations are clearly wrong. There is a corresponding include. > > > + u8 buf[YOGA_C630_UCSI_READ_SIZE]; > > + int ret; > > + > > + ret = yoga_c630_ec_ucsi_read(uec->ec, buf); > > + if (ret) > > + return ret; > > + > > + if (offset == UCSI_VERSION) { > > + memcpy(val, &uec->version, min(val_len, sizeof(uec->version))); > > + return 0; > > + } > > + > > + if (offset == UCSI_CCI) > > + memcpy(val, buf, > > + min(val_len, YOGA_C630_UCSI_CCI_SIZE)); > > Fits to one line. > > > + else if (offset == UCSI_MESSAGE_IN) > > + memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE, > > + min(val_len, YOGA_C630_UCSI_DATA_SIZE)); > > + else > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > > extra space, there seems to be more of them below but I won't mark them. > > > + const void *val, size_t val_len) > > +{ > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > + > > + if (offset != UCSI_CONTROL || > > + val_len != YOGA_C630_UCSI_WRITE_SIZE) > > + return -EINVAL; > > + > > + return yoga_c630_ec_ucsi_write(uec->ec, val); > > +} > > + > > +static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset, > > + const void *val, size_t val_len) > > +{ > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; > > + int ret; > > + > > + if (ack) > > + set_bit(UCSI_C630_ACK_PENDING, &uec->flags); > > + else > > + set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > > Include for set_bit() > > > + reinit_completion(&uec->complete); > > + > > + ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len); > > + if (ret) > > + goto out_clear_bit; > > + > > + if (!wait_for_completion_timeout(&uec->complete, 5 * HZ)) > > + ret = -ETIMEDOUT; > > + > > +out_clear_bit: > > + if (ack) > > + clear_bit(UCSI_C630_ACK_PENDING, &uec->flags); > > + else > > + clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > > + > > + return ret; > > +} > > + > > +const struct ucsi_operations yoga_c630_ucsi_ops = { > > Include for ucsi_operations. > > > + .read = yoga_c630_ucsi_read, > > + .sync_write = yoga_c630_ucsi_sync_write, > > + .async_write = yoga_c630_ucsi_async_write, > > +}; > > + > > +static int yoga_c630_ucsi_notify(struct notifier_block *nb, > > + unsigned long action, void *data) > > +{ > > + struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb); > > Include for container_of > > -- > i. >
On Wed, 29 May 2024, Dmitry Baryshkov wrote: > On Wed, 29 May 2024 at 17:20, Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > > > On Tue, 28 May 2024, Dmitry Baryshkov wrote: > > > > > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in > > > the onboard EC. Add glue driver to interface the platform's UCSI > > > implementation. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/usb/typec/ucsi/Kconfig | 9 ++ > > > drivers/usb/typec/ucsi/Makefile | 1 + > > > drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++ > > > 3 files changed, 199 insertions(+) > > > > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > > index bdcb1764cfae..680e1b87b152 100644 > > > --- a/drivers/usb/typec/ucsi/Kconfig > > > +++ b/drivers/usb/typec/ucsi/Kconfig > > > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK > > > To compile the driver as a module, choose M here: the module will be > > > called ucsi_glink. > > > > > > +config UCSI_LENOVO_YOGA_C630 > > > + tristate "UCSI Interface Driver for Lenovo Yoga C630" > > > + depends on EC_LENOVO_YOGA_C630 > > > + help > > > + This driver enables UCSI support on the Lenovo Yoga C630 laptop. > > > + > > > + To compile the driver as a module, choose M here: the module will be > > > + called ucsi_yoga_c630. > > > + > > > endif > > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > > > index b4679f94696b..aed41d23887b 100644 > > > --- a/drivers/usb/typec/ucsi/Makefile > > > +++ b/drivers/usb/typec/ucsi/Makefile > > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > > > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > > > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > > > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > > > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o > > > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > > new file mode 100644 > > > index 000000000000..ca1ab5c81b87 > > > --- /dev/null > > > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > > @@ -0,0 +1,189 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Copyright (c) 2022-2024, Linaro Ltd > > > + * Authors: > > > + * Bjorn Andersson > > > + * Dmitry Baryshkov > > > + */ > > > +#include <linux/auxiliary_bus.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_data/lenovo-yoga-c630.h> > > > + > > > +#include "ucsi.h" > > > + > > > +struct yoga_c630_ucsi { > > > + struct yoga_c630_ec *ec; > > > + struct ucsi *ucsi; > > > + struct notifier_block nb; > > > + struct completion complete; > > > > Add includes for what you used here. > > > > > + unsigned long flags; > > > +#define UCSI_C630_COMMAND_PENDING 0 > > > +#define UCSI_C630_ACK_PENDING 1 > > > + u16 version; > > > +}; > > > + > > > +static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset, > > > > extra space > > > > > + void *val, size_t val_len) > > > +{ > > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > > > Missing include for ucsi_get_drvdata > > I'll review my includes, but this comment and the comment for > ucsi_operations are clearly wrong. There is a corresponding include. Ah, sorry about that. I completely missed there was that local include.
On Tue, 28 May 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> > --- > drivers/power/supply/Kconfig | 9 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++ > 3 files changed, 489 insertions(+) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 3e31375491d5..55ab8e90747d 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3 > help > Say Y here to enable support for the LEGO MINDSTORMS EV3 battery. > > +config BATTERY_LENOVO_YOGA_C630 > + tristate "Lenovo Yoga C630 battery" > + depends on OF && EC_LENOVO_YOGA_C630 > + help > + This driver enables battery support on the Lenovo Yoga C630 laptop. > + > + To compile the driver as a module, choose M here: the module will be > + called lenovo_yoga_c630_battery. > + > config BATTERY_PMU > tristate "Apple PMU battery" > depends on PPC32 && ADB_PMU > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 58b567278034..8ebbdcf92dac 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782) += ds2782_battery.o > obj-$(CONFIG_BATTERY_GAUGE_LTC2941) += ltc2941-battery-gauge.o > obj-$(CONFIG_BATTERY_GOLDFISH) += goldfish_battery.o > obj-$(CONFIG_BATTERY_LEGO_EV3) += lego_ev3_battery.o > +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o > obj-$(CONFIG_BATTERY_PMU) += pmu_battery.o > obj-$(CONFIG_BATTERY_QCOM_BATTMGR) += qcom_battmgr.o > obj-$(CONFIG_BATTERY_OLPC) += olpc_battery.o > diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c > new file mode 100644 > index 000000000000..76152ad38d46 > --- /dev/null > +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c > @@ -0,0 +1,479 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2024, Linaro Ltd > + * Authors: > + * Bjorn Andersson > + * Dmitry Baryshkov > + */ > +#include <linux/auxiliary_bus.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/platform_data/lenovo-yoga-c630.h> > +#include <linux/power_supply.h> > + > +struct yoga_c630_psy { > + struct yoga_c630_ec *ec; > + struct device *dev; > + struct device_node *of_node; > + struct notifier_block nb; > + struct mutex lock; Missing a few includes, please check them here as well. > + struct power_supply *adp_psy; > + struct power_supply *bat_psy; > + > + unsigned long last_status_update; > + > + bool adapter_online; > + > + bool unit_mA; > + > + bool bat_present; > + unsigned int bat_status; > + unsigned int design_capacity; > + unsigned int design_voltage; > + unsigned int full_charge_capacity; > + > + unsigned int capacity_now; > + unsigned int voltage_now; > + > + int current_now; > + int rate_now; > +}; > + > +#define LENOVO_EC_CACHE_TIME (10 * HZ) > + > +#define LENOVO_EC_ADPT_STATUS 0xa3 > +#define LENOVO_EC_ADPT_PRESENT BIT(7) Add include for BIT() > +#define LENOVO_EC_BAT_ATTRIBUTES 0xc0 > +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA BIT(1) > +#define LENOVO_EC_BAT_STATUS 0xc1 > +#define LENOVO_EC_BAT_REMAIN_CAPACITY 0xc2 > +#define LENOVO_EC_BAT_VOLTAGE 0xc6 > +#define LENOVO_EC_BAT_DESIGN_VOLTAGE 0xc8 > +#define LENOVO_EC_BAT_DESIGN_CAPACITY 0xca > +#define LENOVO_EC_BAT_FULL_CAPACITY 0xcc > +#define LENOVO_EC_BAT_CURRENT 0xd2 > +#define LENOVO_EC_BAT_FULL_FACTORY 0xd6 > +#define LENOVO_EC_BAT_PRESENT 0xda > +#define LENOVO_EC_BAT_FULL_REGISTER 0xdb > +#define LENOVO_EC_BAT_FULL_IS_FACTORY BIT(0) > + > +/* the mutex should already be locked */ > +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 & BIT(0)); Name the bit with a define. > + 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_ATTR_UNIT_IS_MA; > + > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY); > + if (val < 0) > + return val; > + ecbat->design_capacity = val * 1000; Check linux/units.h if some WATT related one matches to that literal 1000. > + 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; > + if (val & LENOVO_EC_BAT_FULL_IS_FACTORY) > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_FACTORY); > + else > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_CAPACITY); You could consider doing this instead of branching: val = yoga_c630_ec_read16(ec, val & LENOVO_EC_BAT_FULL_IS_FACTORY ? LENOVO_EC_BAT_FULL_FACTORY : LENOVO_EC_BAT_FULL_CAPACITY); > + if (val < 0) > + return val; > + > + ecbat->full_charge_capacity = val * 1000; Something from linux/units.h ? > + if (!ecbat->unit_mA) { > + ecbat->design_capacity *= 10; > + ecbat->full_charge_capacity *= 10; > + } > + > + return 0; > +} > + > +/* the mutex should already be locked */ > +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; > + > + 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; units.h ? > + msleep(50); > + > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE); > + if (val < 0) > + return val; > + ecbat->voltage_now = val * 1000; Ditto. > + 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); Ditto. > + msleep(50); > + > + if (!ecbat->unit_mA) > + ecbat->capacity_now *= 10; > + > + ecbat->last_status_update = jiffies; Add jiffies.h > + return 0; > +} > + > +static int yoga_c630_psy_update_adapter_status(struct yoga_c630_psy *ecbat) > +{ > + struct yoga_c630_ec *ec = ecbat->ec; > + int val; > + > + mutex_lock(&ecbat->lock); This kind of functions coul use guard to take the mutex so unlock will be handled for you by the cleanup automation. > + val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS); > + if (val > 0) > + ecbat->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val); > + > + mutex_unlock(&ecbat->lock); > + > + return val; > +} > + > +static bool yoga_c630_psy_is_charged(struct yoga_c630_psy *ecbat) > +{ > + if (ecbat->bat_status != 0) > + return false; > + > + if (ecbat->full_charge_capacity <= ecbat->capacity_now) > + return true; > + > + if (ecbat->design_capacity <= ecbat->capacity_now) > + return true; > + > + return false; > +} > + > +static int yoga_c630_psy_bat_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 rc = 0; > + > + if (!ecbat->bat_present && > + psp != POWER_SUPPLY_PROP_PRESENT) Fits to one line. > + return -ENODEV; > + > + mutex_lock(&ecbat->lock); > + rc = yoga_c630_psy_maybe_update_bat_status(ecbat); > + mutex_unlock(&ecbat->lock); > + > + if (rc) Remove empty line in between since this is the error handling. > + return rc; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + if (ecbat->bat_status & BIT(0)) Name bits with defines. > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + else if (ecbat->bat_status & BIT(1)) Ditto. > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else if (yoga_c630_psy_is_charged(ecbat)) > + val->intval = POWER_SUPPLY_STATUS_FULL; > + else > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > + break; > + case POWER_SUPPLY_PROP_PRESENT: > + val->intval = ecbat->bat_present; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + val->intval = ecbat->design_voltage; > + break; > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: > + val->intval = ecbat->design_capacity; > + break; > + case POWER_SUPPLY_PROP_CHARGE_FULL: > + case POWER_SUPPLY_PROP_ENERGY_FULL: > + val->intval = ecbat->full_charge_capacity; > + break; > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + case POWER_SUPPLY_PROP_ENERGY_NOW: > + val->intval = ecbat->capacity_now; > + break; > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + val->intval = ecbat->current_now; > + break; > + case POWER_SUPPLY_PROP_POWER_NOW: > + val->intval = ecbat->rate_now; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = ecbat->voltage_now; > + break; > + 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; > + default: > + rc = -EINVAL; > + break; > + } > + > + return rc; > +} > +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.of_node = ecbat->of_node; > + if (ecbat->unit_mA) > + ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mA, &bat_cfg); > + else > + ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mWh, &bat_cfg); Again, it might be easier to see what's different in here if the relevant parameter just uses ?: instead of full blown if/else. > + if (IS_ERR(ecbat->bat_psy)) { > + dev_err(ecbat->dev, "failed to register battery supply\n"); > + return PTR_ERR(ecbat->bat_psy); > + } > + > + return 0; > +}
On 28/05/2024 21:44, Dmitry Baryshkov wrote: > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in > the onboard EC. Add glue driver to interface the platform's UCSI > implementation. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/usb/typec/ucsi/Kconfig | 9 ++ > drivers/usb/typec/ucsi/Makefile | 1 + > drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > index bdcb1764cfae..680e1b87b152 100644 > --- a/drivers/usb/typec/ucsi/Kconfig > +++ b/drivers/usb/typec/ucsi/Kconfig > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK > To compile the driver as a module, choose M here: the module will be > called ucsi_glink. > > +config UCSI_LENOVO_YOGA_C630 > + tristate "UCSI Interface Driver for Lenovo Yoga C630" > + depends on EC_LENOVO_YOGA_C630 > + help > + This driver enables UCSI support on the Lenovo Yoga C630 laptop. > + > + To compile the driver as a module, choose M here: the module will be > + called ucsi_yoga_c630. > + > endif > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > index b4679f94696b..aed41d23887b 100644 > --- a/drivers/usb/typec/ucsi/Makefile > +++ b/drivers/usb/typec/ucsi/Makefile > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > new file mode 100644 > index 000000000000..ca1ab5c81b87 > --- /dev/null > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > @@ -0,0 +1,189 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2024, Linaro Ltd > + * Authors: > + * Bjorn Andersson > + * Dmitry Baryshkov > + */ > +#include <linux/auxiliary_bus.h> > +#include <linux/module.h> > +#include <linux/platform_data/lenovo-yoga-c630.h> > + > +#include "ucsi.h" > + > +struct yoga_c630_ucsi { > + struct yoga_c630_ec *ec; > + struct ucsi *ucsi; > + struct notifier_block nb; > + struct completion complete; > + unsigned long flags; > +#define UCSI_C630_COMMAND_PENDING 0 > +#define UCSI_C630_ACK_PENDING 1 > + u16 version; > +}; > + > +static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset, > + void *val, size_t val_len) > +{ > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > + u8 buf[YOGA_C630_UCSI_READ_SIZE]; > + int ret; > + > + ret = yoga_c630_ec_ucsi_read(uec->ec, buf); > + if (ret) > + return ret; > + > + if (offset == UCSI_VERSION) { > + memcpy(val, &uec->version, min(val_len, sizeof(uec->version))); > + return 0; > + } > + > + if (offset == UCSI_CCI) > + memcpy(val, buf, > + min(val_len, YOGA_C630_UCSI_CCI_SIZE)); > + else if (offset == UCSI_MESSAGE_IN) > + memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE, > + min(val_len, YOGA_C630_UCSI_DATA_SIZE)); For some reason I believe multi-lines like this, including function calls that are split over lines should be encapsulated with {} else if(x) { memcpy(x,y, z); } If checkpatch doesn't complain about it feel free not to do that though. > + else > + return -EINVAL; > + > + return 0; > +} > + > +static int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > + const void *val, size_t val_len) > +{ > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > + > + if (offset != UCSI_CONTROL || > + val_len != YOGA_C630_UCSI_WRITE_SIZE) > + return -EINVAL; > + > + return yoga_c630_ec_ucsi_write(uec->ec, val); > +} > + > +static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset, > + const void *val, size_t val_len) > +{ > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; > + int ret; > + > + if (ack) > + set_bit(UCSI_C630_ACK_PENDING, &uec->flags); > + else > + set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > + > + reinit_completion(&uec->complete); > + > + ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len); > + if (ret) > + goto out_clear_bit; > + > + if (!wait_for_completion_timeout(&uec->complete, 5 * HZ)) > + ret = -ETIMEDOUT; > + > +out_clear_bit: > + if (ack) > + clear_bit(UCSI_C630_ACK_PENDING, &uec->flags); > + else > + clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > + > + return ret; > +} > + > +const struct ucsi_operations yoga_c630_ucsi_ops = { > + .read = yoga_c630_ucsi_read, > + .sync_write = yoga_c630_ucsi_sync_write, > + .async_write = yoga_c630_ucsi_async_write, > +}; > + > +static int yoga_c630_ucsi_notify(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb); > + u32 cci; > + int ret; > + > + if (action == LENOVO_EC_EVENT_USB || action == LENOVO_EC_EVENT_HPD) { > + ucsi_connector_change(uec->ucsi, 1); > + return NOTIFY_OK; > + } > + > + if (action != LENOVO_EC_EVENT_UCSI) > + return NOTIFY_DONE; Is this disjunction on action a good candidate for a switch(){} > + > + ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci)); > + if (ret) > + return NOTIFY_DONE; > + > + if (UCSI_CCI_CONNECTOR(cci)) > + ucsi_connector_change(uec->ucsi, UCSI_CCI_CONNECTOR(cci)); > + > + if (cci & UCSI_CCI_ACK_COMPLETE && > + test_bit(UCSI_C630_ACK_PENDING, &uec->flags)) > + complete(&uec->complete); > + if (cci & UCSI_CCI_COMMAND_COMPLETE && > + test_bit(UCSI_C630_COMMAND_PENDING, &uec->flags)) > + complete(&uec->complete); IMO these multi-line clauses should end up with a {} around the complete even though its not required. Emphasis on the O. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 28/05/2024 21:44, 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> > --- > drivers/power/supply/Kconfig | 9 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++ > 3 files changed, 489 insertions(+) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 3e31375491d5..55ab8e90747d 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3 > help > Say Y here to enable support for the LEGO MINDSTORMS EV3 battery. > > +config BATTERY_LENOVO_YOGA_C630 > + tristate "Lenovo Yoga C630 battery" > + depends on OF && EC_LENOVO_YOGA_C630 > + help > + This driver enables battery support on the Lenovo Yoga C630 laptop. > + > + To compile the driver as a module, choose M here: the module will be > + called lenovo_yoga_c630_battery. > + > config BATTERY_PMU > tristate "Apple PMU battery" > depends on PPC32 && ADB_PMU > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 58b567278034..8ebbdcf92dac 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782) += ds2782_battery.o > obj-$(CONFIG_BATTERY_GAUGE_LTC2941) += ltc2941-battery-gauge.o > obj-$(CONFIG_BATTERY_GOLDFISH) += goldfish_battery.o > obj-$(CONFIG_BATTERY_LEGO_EV3) += lego_ev3_battery.o > +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o > obj-$(CONFIG_BATTERY_PMU) += pmu_battery.o > obj-$(CONFIG_BATTERY_QCOM_BATTMGR) += qcom_battmgr.o > obj-$(CONFIG_BATTERY_OLPC) += olpc_battery.o > diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c > new file mode 100644 > index 000000000000..76152ad38d46 > --- /dev/null > +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c > @@ -0,0 +1,479 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2024, Linaro Ltd > + * Authors: > + * Bjorn Andersson > + * Dmitry Baryshkov > + */ > +#include <linux/auxiliary_bus.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/platform_data/lenovo-yoga-c630.h> > +#include <linux/power_supply.h> > + > +struct yoga_c630_psy { > + struct yoga_c630_ec *ec; > + struct device *dev; > + struct device_node *of_node; > + struct notifier_block nb; > + struct mutex lock; Do locks still not require a struct mutex lock; /* this mutex locks this thing */ > + > + struct power_supply *adp_psy; > + struct power_supply *bat_psy; > + > + unsigned long last_status_update; > + > + bool adapter_online; > + > + bool unit_mA; > + > + bool bat_present; > + unsigned int bat_status; > + unsigned int design_capacity; > + unsigned int design_voltage; > + unsigned int full_charge_capacity; > + > + unsigned int capacity_now; > + unsigned int voltage_now; > + > + int current_now; > + int rate_now; > +}; > + > +#define LENOVO_EC_CACHE_TIME (10 * HZ) > + > +#define LENOVO_EC_ADPT_STATUS 0xa3 > +#define LENOVO_EC_ADPT_PRESENT BIT(7) > +#define LENOVO_EC_BAT_ATTRIBUTES 0xc0 > +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA BIT(1) > +#define LENOVO_EC_BAT_STATUS 0xc1 > +#define LENOVO_EC_BAT_REMAIN_CAPACITY 0xc2 > +#define LENOVO_EC_BAT_VOLTAGE 0xc6 > +#define LENOVO_EC_BAT_DESIGN_VOLTAGE 0xc8 > +#define LENOVO_EC_BAT_DESIGN_CAPACITY 0xca > +#define LENOVO_EC_BAT_FULL_CAPACITY 0xcc > +#define LENOVO_EC_BAT_CURRENT 0xd2 > +#define LENOVO_EC_BAT_FULL_FACTORY 0xd6 > +#define LENOVO_EC_BAT_PRESENT 0xda > +#define LENOVO_EC_BAT_FULL_REGISTER 0xdb > +#define LENOVO_EC_BAT_FULL_IS_FACTORY BIT(0) > + > +/* the mutex should already be locked */ > +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 & BIT(0)); > + 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_ATTR_UNIT_IS_MA; > + > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY); > + if (val < 0) > + return val; > + ecbat->design_capacity = val * 1000; > + > + msleep(50); What's this for ? Also do you really want to hold a mutex for 50 milliseconds ? > + > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE); > + if (val < 0) > + return val; > + ecbat->design_voltage = val; > + > + msleep(50); And again ? I guess it doesn't really matter how long you hold your mutex but, some description of the delay in the code would be nice from a reader's perspective. Same comment for the rest of the msleeps(); > + > + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER); > + if (val < 0) > + return val; > + if (val & LENOVO_EC_BAT_FULL_IS_FACTORY) > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_FACTORY); > + else > + val = yoga_c630_ec_read16(ec, 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; > +} > + > +/* the mutex should already be locked */ > +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; > + > + 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; > + > + mutex_lock(&ecbat->lock); > + > + val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS); > + if (val > 0) > + ecbat->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val); > + > + mutex_unlock(&ecbat->lock); > + > + return val; > +} > + > +static bool yoga_c630_psy_is_charged(struct yoga_c630_psy *ecbat) > +{ > + if (ecbat->bat_status != 0) > + return false; > + > + if (ecbat->full_charge_capacity <= ecbat->capacity_now) > + return true; > + > + if (ecbat->design_capacity <= ecbat->capacity_now) > + return true; > + > + return false; > +} > + > +static int yoga_c630_psy_bat_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 rc = 0; > + > + if (!ecbat->bat_present && > + psp != POWER_SUPPLY_PROP_PRESENT) > + return -ENODEV; > + > + mutex_lock(&ecbat->lock); > + rc = yoga_c630_psy_maybe_update_bat_status(ecbat); > + mutex_unlock(&ecbat->lock); > + > + if (rc) > + return rc; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + if (ecbat->bat_status & BIT(0)) > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + else if (ecbat->bat_status & BIT(1)) For preference I'd name these bits. > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else if (yoga_c630_psy_is_charged(ecbat)) > + val->intval = POWER_SUPPLY_STATUS_FULL; > + else > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > + break; > + case POWER_SUPPLY_PROP_PRESENT: > + val->intval = ecbat->bat_present; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + val->intval = ecbat->design_voltage; > + break; > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: > + val->intval = ecbat->design_capacity; > + break; > + case POWER_SUPPLY_PROP_CHARGE_FULL: > + case POWER_SUPPLY_PROP_ENERGY_FULL: > + val->intval = ecbat->full_charge_capacity; > + break; > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + case POWER_SUPPLY_PROP_ENERGY_NOW: > + val->intval = ecbat->capacity_now; > + break; > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + val->intval = ecbat->current_now; > + break; > + case POWER_SUPPLY_PROP_POWER_NOW: > + val->intval = ecbat->rate_now; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = ecbat->voltage_now; > + break; > + 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; > + default: > + rc = -EINVAL; > + break; > + } > + > + return rc; > +} > + > +static enum power_supply_property yoga_c630_psy_bat_mA_properties[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_FULL, > + POWER_SUPPLY_PROP_CHARGE_NOW, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_POWER_NOW, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_MANUFACTURER, > +}; > + > +static enum power_supply_property yoga_c630_psy_bat_mWh_properties[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, > + POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN, > + POWER_SUPPLY_PROP_ENERGY_FULL, > + POWER_SUPPLY_PROP_ENERGY_NOW, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_POWER_NOW, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_MANUFACTURER, > +}; > + > +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 rc = 0; > + > + yoga_c630_psy_update_adapter_status(ecbat); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + val->intval = ecbat->adapter_online; > + break; > + default: > + rc = -EINVAL; > + break; > + } > + > + return rc; > +} > + > +static enum power_supply_property yoga_c630_psy_adpt_properties[] = { > + POWER_SUPPLY_PROP_ONLINE, > +}; > + > +static const struct power_supply_desc yoga_c630_psy_adpt_psy_desc = { > + .name = "yoga-c630-adapter", > + .type = POWER_SUPPLY_TYPE_USB_TYPE_C, > + .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.of_node = ecbat->of_node; > + if (ecbat->unit_mA) > + ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mA, &bat_cfg); > + else > + ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mWh, &bat_cfg); These look a bit long, in the other driver in this series you're capping at whatever it is 75 or 80 characters. TBH I think I prefer the longer line above maybe consider elongating the length of the line in the previous driver or curtailing to 80 here. But I think you should have a consistent line lenght in the same series. > + 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; > + > + mutex_lock(&ecbat->lock); > + 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); > + } > + > + mutex_unlock(&ecbat->lock); > +} > + > +static int yoga_c630_psy_notify(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct yoga_c630_psy *ecbat = container_of(nb, struct yoga_c630_psy, nb); > + > + switch (action) { > + case LENOVO_EC_EVENT_BAT_INFO: > + yoga_c630_ec_refresh_bat_info(ecbat); > + break; > + case LENOVO_EC_EVENT_BAT_ADPT_STATUS: > + power_supply_changed(ecbat->adp_psy); > + fallthrough; > + case LENOVO_EC_EVENT_BAT_STATUS: > + power_supply_changed(ecbat->bat_psy); > + break; > + } > + > + return NOTIFY_OK; > +} > + > +static int yoga_c630_psy_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct yoga_c630_ec *ec = adev->dev.platform_data; > + struct power_supply_config adp_cfg = {}; > + struct device *dev = &adev->dev; > + struct yoga_c630_psy *ecbat; > + int ret; > + > + ecbat = devm_kzalloc(&adev->dev, sizeof(*ecbat), GFP_KERNEL); > + if (!ecbat) > + return -ENOMEM; > + > + ecbat->ec = ec; > + ecbat->dev = dev; > + mutex_init(&ecbat->lock); > + ecbat->of_node = adev->dev.parent->of_node; > + ecbat->nb.notifier_call = yoga_c630_psy_notify; > + > + auxiliary_set_drvdata(adev, ecbat); > + > + adp_cfg.drv_data = ecbat; > + adp_cfg.of_node = ecbat->of_node; > + adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name; > + adp_cfg.num_supplicants = 1; > + ecbat->adp_psy = devm_power_supply_register_no_ws(dev, &yoga_c630_psy_adpt_psy_desc, &adp_cfg); > + if (IS_ERR(ecbat->adp_psy)) { > + dev_err(dev, "failed to register AC adapter supply\n"); > + return PTR_ERR(ecbat->adp_psy); > + } > + > + mutex_lock(&ecbat->lock); Do you really need this lock here in your probe() function ? What's the parallel path of execution you are mitigating against here ? > + > + ret = yoga_c630_psy_update_bat_info(ecbat); > + if (ret) > + goto err_unlock; > + > + ret = yoga_c630_psy_register_bat_psy(ecbat); > + if (ret) > + goto err_unlock; > + > + mutex_unlock(&ecbat->lock); > + > + ret = yoga_c630_ec_register_notify(ecbat->ec, &ecbat->nb); > + if (ret) > + goto err_unreg_bat; > + > + return 0; > + > +err_unlock: > + mutex_unlock(&ecbat->lock); > + > +err_unreg_bat: > + power_supply_unregister(ecbat->bat_psy); > + return ret; > +} > + > +static void yoga_c630_psy_remove(struct auxiliary_device *adev) > +{ > + struct yoga_c630_psy *ecbat = auxiliary_get_drvdata(adev); > + > + yoga_c630_ec_unregister_notify(ecbat->ec, &ecbat->nb); > + power_supply_unregister(ecbat->bat_psy); > +} > + > +static const struct auxiliary_device_id yoga_c630_psy_id_table[] = { > + { .name = YOGA_C630_MOD_NAME "." YOGA_C630_DEV_PSY, }, > + {} > +}; > +MODULE_DEVICE_TABLE(auxiliary, yoga_c630_psy_id_table); > + > +static struct auxiliary_driver yoga_c630_psy_driver = { > + .name = YOGA_C630_DEV_PSY, > + .id_table = yoga_c630_psy_id_table, > + .probe = yoga_c630_psy_probe, > + .remove = yoga_c630_psy_remove, > +}; > + > +module_auxiliary_driver(yoga_c630_psy_driver); > + > +MODULE_DESCRIPTION("Lenovo Yoga C630 psy"); > +MODULE_LICENSE("GPL"); >
On Wed, May 29, 2024 at 04:41:40PM +0100, Bryan O'Donoghue wrote: > On 28/05/2024 21:44, Dmitry Baryshkov wrote: > > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in > > the onboard EC. Add glue driver to interface the platform's UCSI > > implementation. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/usb/typec/ucsi/Kconfig | 9 ++ > > drivers/usb/typec/ucsi/Makefile | 1 + > > drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++ > > 3 files changed, 199 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > index bdcb1764cfae..680e1b87b152 100644 > > --- a/drivers/usb/typec/ucsi/Kconfig > > +++ b/drivers/usb/typec/ucsi/Kconfig > > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK > > To compile the driver as a module, choose M here: the module will be > > called ucsi_glink. > > +config UCSI_LENOVO_YOGA_C630 > > + tristate "UCSI Interface Driver for Lenovo Yoga C630" > > + depends on EC_LENOVO_YOGA_C630 > > + help > > + This driver enables UCSI support on the Lenovo Yoga C630 laptop. > > + > > + To compile the driver as a module, choose M here: the module will be > > + called ucsi_yoga_c630. > > + > > endif > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > > index b4679f94696b..aed41d23887b 100644 > > --- a/drivers/usb/typec/ucsi/Makefile > > +++ b/drivers/usb/typec/ucsi/Makefile > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o > > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > new file mode 100644 > > index 000000000000..ca1ab5c81b87 > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > @@ -0,0 +1,189 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2022-2024, Linaro Ltd > > + * Authors: > > + * Bjorn Andersson > > + * Dmitry Baryshkov > > + */ > > +#include <linux/auxiliary_bus.h> > > +#include <linux/module.h> > > +#include <linux/platform_data/lenovo-yoga-c630.h> > > + > > +#include "ucsi.h" > > + > > +struct yoga_c630_ucsi { > > + struct yoga_c630_ec *ec; > > + struct ucsi *ucsi; > > + struct notifier_block nb; > > + struct completion complete; > > + unsigned long flags; > > +#define UCSI_C630_COMMAND_PENDING 0 > > +#define UCSI_C630_ACK_PENDING 1 > > + u16 version; > > +}; > > + > > +static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset, > > + void *val, size_t val_len) > > +{ > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > + u8 buf[YOGA_C630_UCSI_READ_SIZE]; > > + int ret; > > + > > + ret = yoga_c630_ec_ucsi_read(uec->ec, buf); > > + if (ret) > > + return ret; > > + > > + if (offset == UCSI_VERSION) { > > + memcpy(val, &uec->version, min(val_len, sizeof(uec->version))); > > + return 0; > > + } > > + > > + if (offset == UCSI_CCI) > > + memcpy(val, buf, > > + min(val_len, YOGA_C630_UCSI_CCI_SIZE)); > > + else if (offset == UCSI_MESSAGE_IN) > > + memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE, > > + min(val_len, YOGA_C630_UCSI_DATA_SIZE)); > > For some reason I believe multi-lines like this, including function calls > that are split over lines should be encapsulated with {} > > else if(x) { > memcpy(x,y, > z); > } > > If checkpatch doesn't complain about it feel free not to do that though. No, checkpatch --strict doesn't complain > > > + else > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > > + const void *val, size_t val_len) > > +{ > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > + > > + if (offset != UCSI_CONTROL || > > + val_len != YOGA_C630_UCSI_WRITE_SIZE) > > + return -EINVAL; > > + > > + return yoga_c630_ec_ucsi_write(uec->ec, val); > > +} > > + > > +static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset, > > + const void *val, size_t val_len) > > +{ > > + struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi); > > + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; > > + int ret; > > + > > + if (ack) > > + set_bit(UCSI_C630_ACK_PENDING, &uec->flags); > > + else > > + set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > > + > > + reinit_completion(&uec->complete); > > + > > + ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len); > > + if (ret) > > + goto out_clear_bit; > > + > > + if (!wait_for_completion_timeout(&uec->complete, 5 * HZ)) > > + ret = -ETIMEDOUT; > > + > > +out_clear_bit: > > + if (ack) > > + clear_bit(UCSI_C630_ACK_PENDING, &uec->flags); > > + else > > + clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags); > > + > > + return ret; > > +} > > + > > +const struct ucsi_operations yoga_c630_ucsi_ops = { > > + .read = yoga_c630_ucsi_read, > > + .sync_write = yoga_c630_ucsi_sync_write, > > + .async_write = yoga_c630_ucsi_async_write, > > +}; > > + > > +static int yoga_c630_ucsi_notify(struct notifier_block *nb, > > + unsigned long action, void *data) > > +{ > > + struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb); > > + u32 cci; > > + int ret; > > + > > + if (action == LENOVO_EC_EVENT_USB || action == LENOVO_EC_EVENT_HPD) { > > + ucsi_connector_change(uec->ucsi, 1); > > + return NOTIFY_OK; > > + } > > + > > + if (action != LENOVO_EC_EVENT_UCSI) > > + return NOTIFY_DONE; > > Is this disjunction on action a good candidate for a switch(){} Ack, refactored the function by extracting the UCSI notification code and then using the switch-case. > > + > > + ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci)); > > + if (ret) > > + return NOTIFY_DONE; > > + > > + if (UCSI_CCI_CONNECTOR(cci)) > > + ucsi_connector_change(uec->ucsi, UCSI_CCI_CONNECTOR(cci)); > > + > > + if (cci & UCSI_CCI_ACK_COMPLETE && > > + test_bit(UCSI_C630_ACK_PENDING, &uec->flags)) > > + complete(&uec->complete); > > + if (cci & UCSI_CCI_COMMAND_COMPLETE && > > + test_bit(UCSI_C630_COMMAND_PENDING, &uec->flags)) > > + complete(&uec->complete); > > IMO these multi-line clauses should end up with a {} around the complete > even though its not required. > > Emphasis on the O. I added an empty line inbetween, then it's easier to comprehent event without curly brackets. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Wed, May 29, 2024 at 04:51:54PM +0100, Bryan O'Donoghue wrote: > On 28/05/2024 21:44, 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> > > --- > > drivers/power/supply/Kconfig | 9 + > > drivers/power/supply/Makefile | 1 + > > drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++ > > 3 files changed, 489 insertions(+) > > > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > > index 3e31375491d5..55ab8e90747d 100644 > > --- a/drivers/power/supply/Kconfig > > +++ b/drivers/power/supply/Kconfig > > @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3 > > help > > Say Y here to enable support for the LEGO MINDSTORMS EV3 battery. > > +config BATTERY_LENOVO_YOGA_C630 > > + tristate "Lenovo Yoga C630 battery" > > + depends on OF && EC_LENOVO_YOGA_C630 > > + help > > + This driver enables battery support on the Lenovo Yoga C630 laptop. > > + > > + To compile the driver as a module, choose M here: the module will be > > + called lenovo_yoga_c630_battery. > > + > > config BATTERY_PMU > > tristate "Apple PMU battery" > > depends on PPC32 && ADB_PMU > > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > > index 58b567278034..8ebbdcf92dac 100644 > > --- a/drivers/power/supply/Makefile > > +++ b/drivers/power/supply/Makefile > > @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782) += ds2782_battery.o > > obj-$(CONFIG_BATTERY_GAUGE_LTC2941) += ltc2941-battery-gauge.o > > obj-$(CONFIG_BATTERY_GOLDFISH) += goldfish_battery.o > > obj-$(CONFIG_BATTERY_LEGO_EV3) += lego_ev3_battery.o > > +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o > > obj-$(CONFIG_BATTERY_PMU) += pmu_battery.o > > obj-$(CONFIG_BATTERY_QCOM_BATTMGR) += qcom_battmgr.o > > obj-$(CONFIG_BATTERY_OLPC) += olpc_battery.o > > diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c > > new file mode 100644 > > index 000000000000..76152ad38d46 > > --- /dev/null > > +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c > > @@ -0,0 +1,479 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2022-2024, Linaro Ltd > > + * Authors: > > + * Bjorn Andersson > > + * Dmitry Baryshkov > > + */ > > +#include <linux/auxiliary_bus.h> > > +#include <linux/delay.h> > > +#include <linux/module.h> > > +#include <linux/platform_data/lenovo-yoga-c630.h> > > +#include <linux/power_supply.h> > > + > > +struct yoga_c630_psy { > > + struct yoga_c630_ec *ec; > > + struct device *dev; > > + struct device_node *of_node; > > + struct notifier_block nb; > > + struct mutex lock; > > Do locks still not require a > > struct mutex lock; /* this mutex locks this thing */ Not required, but let me add the doc. > > > + > > + struct power_supply *adp_psy; > > + struct power_supply *bat_psy; > > + > > + unsigned long last_status_update; > > + > > + bool adapter_online; > > + > > + bool unit_mA; > > + > > + bool bat_present; > > + unsigned int bat_status; > > + unsigned int design_capacity; > > + unsigned int design_voltage; > > + unsigned int full_charge_capacity; > > + > > + unsigned int capacity_now; > > + unsigned int voltage_now; > > + > > + int current_now; > > + int rate_now; > > +}; > > + > > +#define LENOVO_EC_CACHE_TIME (10 * HZ) > > + > > +#define LENOVO_EC_ADPT_STATUS 0xa3 > > +#define LENOVO_EC_ADPT_PRESENT BIT(7) > > +#define LENOVO_EC_BAT_ATTRIBUTES 0xc0 > > +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA BIT(1) > > +#define LENOVO_EC_BAT_STATUS 0xc1 > > +#define LENOVO_EC_BAT_REMAIN_CAPACITY 0xc2 > > +#define LENOVO_EC_BAT_VOLTAGE 0xc6 > > +#define LENOVO_EC_BAT_DESIGN_VOLTAGE 0xc8 > > +#define LENOVO_EC_BAT_DESIGN_CAPACITY 0xca > > +#define LENOVO_EC_BAT_FULL_CAPACITY 0xcc > > +#define LENOVO_EC_BAT_CURRENT 0xd2 > > +#define LENOVO_EC_BAT_FULL_FACTORY 0xd6 > > +#define LENOVO_EC_BAT_PRESENT 0xda > > +#define LENOVO_EC_BAT_FULL_REGISTER 0xdb > > +#define LENOVO_EC_BAT_FULL_IS_FACTORY BIT(0) > > + > > +/* the mutex should already be locked */ > > +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 & BIT(0)); > > + 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_ATTR_UNIT_IS_MA; > > + > > + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY); > > + if (val < 0) > > + return val; > > + ecbat->design_capacity = val * 1000; > > + > > + msleep(50); > > What's this for ? Also do you really want to hold a mutex for 50 > milliseconds ? DSDT has these delays after each read, so I can only assume it is required. Sleeping outside of the mutex() would mean that a concurrent thread might break into this delay and query the EC. [skipped] > > +static int yoga_c630_psy_probe(struct auxiliary_device *adev, > > + const struct auxiliary_device_id *id) > > +{ > > + struct yoga_c630_ec *ec = adev->dev.platform_data; > > + struct power_supply_config adp_cfg = {}; > > + struct device *dev = &adev->dev; > > + struct yoga_c630_psy *ecbat; > > + int ret; > > + > > + ecbat = devm_kzalloc(&adev->dev, sizeof(*ecbat), GFP_KERNEL); > > + if (!ecbat) > > + return -ENOMEM; > > + > > + ecbat->ec = ec; > > + ecbat->dev = dev; > > + mutex_init(&ecbat->lock); > > + ecbat->of_node = adev->dev.parent->of_node; > > + ecbat->nb.notifier_call = yoga_c630_psy_notify; > > + > > + auxiliary_set_drvdata(adev, ecbat); > > + > > + adp_cfg.drv_data = ecbat; > > + adp_cfg.of_node = ecbat->of_node; > > + adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name; > > + adp_cfg.num_supplicants = 1; > > + ecbat->adp_psy = devm_power_supply_register_no_ws(dev, &yoga_c630_psy_adpt_psy_desc, &adp_cfg); > > + if (IS_ERR(ecbat->adp_psy)) { > > + dev_err(dev, "failed to register AC adapter supply\n"); > > + return PTR_ERR(ecbat->adp_psy); > > + } > > + > > + mutex_lock(&ecbat->lock); > > Do you really need this lock here in your probe() function ? What's the > parallel path of execution you are mitigating against here ? Notifications from the battery driver can already happen at this point. Also once the fist power supply is registered, userspace can potentially access it, triggering EC access and updates of the PSY registration. > > > + > > + ret = yoga_c630_psy_update_bat_info(ecbat); > > + if (ret) > > + goto err_unlock; > > + > > + ret = yoga_c630_psy_register_bat_psy(ecbat); > > + if (ret) > > + goto err_unlock; > > + > > + mutex_unlock(&ecbat->lock); > > +
This adds binding, driver and the DT support for the Lenovo Yoga C630 Embedded Controller, to provide battery information. Support for this EC was implemented by Bjorn, who later could not work on this driver. I've picked this patchset up and updated it following the pending review comments. DisplayPort support is still not a part of this patchset. It uses EC messages to provide AltMode information rather than implementing corresponding UCSI commands. However to have a cleaner uAPI story, the AltMode should be handled via the same Type-C port. Merge strategy: the driver bits depend on the platform/arm64 patch, which adds interface for the subdrivers. I'd either ask to get that patch merged to the immutable branch, which then can be picked up by power/supply and USB trees or, to make life simpler, ack merging all driver bits e.g. through USB subsystem (I'm biased here since I plan to send more cleanups for the UCSI subsystem, which would otherwise result in cross-subsystem conflicts). --- Changes in v4: - Moved bindings to platform/ to follow example of other Acer Aspire1 EC (Nikita Travkin) - Fixed dt validation for EC interrupt pin (Rob Herring) - Dropped separate 'scale' property (Oliver Neukum) - Link to v3: https://lore.kernel.org/r/20240527-yoga-ec-driver-v3-0-327a9851dad5@linaro.org Changes in v3: - Split the driver into core and power supply drivers, - Added UCSI driver part, handling USB connections, - Fixed Bjorn's address in DT bindings (Brian Masney) - Changed power-role for both ports to be "dual" per UCSI - Link to v2: https://lore.kernel.org/linux-arm-msm/20230205152809.2233436-1-dmitry.baryshkov@linaro.org/ Changes in v2: - Dropped DP support for now, as the bindings are in process of being discussed separately, - Merged dt patch into the same patchseries, - Removed the fixed serial number battery property, - Fixed indentation of dt bindings example, - Added property: reg and unevaluatedProperties to the connector bindings. - Link to v1: https://lore.kernel.org/linux-arm-msm/20220810035424.2796777-1-bjorn.andersson@linaro.org/ --- Bjorn Andersson (2): dt-bindings: platform: Add Lenovo Yoga C630 EC arm64: dts: qcom: c630: Add Embedded Controller node Dmitry Baryshkov (4): platform: arm64: add Lenovo Yoga C630 WOS EC driver usb: typec: ucsi: add Lenovo Yoga C630 glue driver power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver arm64: dts: qcom: sdm845: describe connections of USB/DP port .../bindings/platform/lenovo,yoga-c630-ec.yaml | 83 ++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 53 ++- .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 75 ++++ drivers/platform/arm64/Kconfig | 14 + drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/lenovo-yoga-c630.c | 279 ++++++++++++ drivers/power/supply/Kconfig | 9 + drivers/power/supply/Makefile | 1 + drivers/power/supply/lenovo_yoga_c630_battery.c | 479 +++++++++++++++++++++ drivers/usb/typec/ucsi/Kconfig | 9 + drivers/usb/typec/ucsi/Makefile | 1 + drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++ include/linux/platform_data/lenovo-yoga-c630.h | 42 ++ 13 files changed, 1234 insertions(+), 1 deletion(-) --- base-commit: 6dc544b66971c7f9909ff038b62149105272d26a change-id: 20240527-yoga-ec-driver-76fd7f5ddae8 Best regards,