Message ID | 20240612-yoga-ec-driver-v6-0-8e76ba060439@linaro.org |
---|---|
Headers | show |
Series | power: supply: Lenovo Yoga C630 EC | expand |
On Wed, 12 Jun 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. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > +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)); > + else > + return -EINVAL; > + > + return 0; Hmm, the inconsistency when to do return 0 is a bit odd. Also, using switch (offset) would probably be better here anyway to replace all the ifs. > +} > + > +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 void yoga_c630_ucsi_notify_ucsi(struct yoga_c630_ucsi *uec, u32 cci) > +{ > + 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); Is this racy? Can another command start after an ACK in between these two ifs and complete() is called prematurely for the new command? (Or will different value in cci protect against that?) > +} > + > +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; > + > + switch (action) { > + case LENOVO_EC_EVENT_USB: > + case LENOVO_EC_EVENT_HPD: > + ucsi_connector_change(uec->ucsi, 1); > + return NOTIFY_OK; > + > + case LENOVO_EC_EVENT_UCSI: > + ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci)); > + if (ret) > + return NOTIFY_DONE; > + > + yoga_c630_ucsi_notify_ucsi(uec, cci); > + > + return NOTIFY_OK; > + > + default: > + return NOTIFY_DONE; > + } > +} > + > +static int yoga_c630_ucsi_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct yoga_c630_ec *ec = adev->dev.platform_data; > + struct yoga_c630_ucsi *uec; > + int ret; > + > + uec = devm_kzalloc(&adev->dev, sizeof(*uec), GFP_KERNEL); > + if (!uec) > + return -ENOMEM; > + > + uec->ec = ec; > + init_completion(&uec->complete); > + uec->nb.notifier_call = yoga_c630_ucsi_notify; > + > + uec->ucsi = ucsi_create(&adev->dev, &yoga_c630_ucsi_ops); > + if (IS_ERR(uec->ucsi)) > + return PTR_ERR(uec->ucsi); > + > + ucsi_set_drvdata(uec->ucsi, uec); > + > + uec->version = yoga_c630_ec_ucsi_get_version(uec->ec); > + > + auxiliary_set_drvdata(adev, uec); > + > + ret = yoga_c630_ec_register_notify(ec, &uec->nb); > + if (ret) > + return ret; > + > + return ucsi_register(uec->ucsi); > +} > + > +static void yoga_c630_ucsi_remove(struct auxiliary_device *adev) > +{ > + struct yoga_c630_ucsi *uec = auxiliary_get_drvdata(adev); > + > + yoga_c630_ec_unregister_notify(uec->ec, &uec->nb); > + ucsi_unregister(uec->ucsi); Usually, the remove should tear down in reverse order than the probe side. Is the divergence from that here intentional?
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.
On Thu, 13 Jun 2024 at 10:30, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Wed, 12 Jun 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. > > > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > > +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)); > > + else > > + return -EINVAL; > > + > > + return 0; > > Hmm, the inconsistency when to do return 0 is a bit odd. Also, using > switch (offset) would probably be better here anyway to replace all the > ifs. I'll see if I can improve this bit. > > > +} > > + > > +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 void yoga_c630_ucsi_notify_ucsi(struct yoga_c630_ucsi *uec, u32 cci) > > +{ > > + 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); > > Is this racy? Can another command start after an ACK in between these two > ifs and complete() is called prematurely for the new command? (Or will > different value in cci protect against that?) No, there is no race. The UCSI is locked for the duration of the command. > > > +} > > + > > +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; > > + > > + switch (action) { > > + case LENOVO_EC_EVENT_USB: > > + case LENOVO_EC_EVENT_HPD: > > + ucsi_connector_change(uec->ucsi, 1); > > + return NOTIFY_OK; > > + > > + case LENOVO_EC_EVENT_UCSI: > > + ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci)); > > + if (ret) > > + return NOTIFY_DONE; > > + > > + yoga_c630_ucsi_notify_ucsi(uec, cci); > > + > > + return NOTIFY_OK; > > + > > + default: > > + return NOTIFY_DONE; > > + } > > +} > > + > > +static int yoga_c630_ucsi_probe(struct auxiliary_device *adev, > > + const struct auxiliary_device_id *id) > > +{ > > + struct yoga_c630_ec *ec = adev->dev.platform_data; > > + struct yoga_c630_ucsi *uec; > > + int ret; > > + > > + uec = devm_kzalloc(&adev->dev, sizeof(*uec), GFP_KERNEL); > > + if (!uec) > > + return -ENOMEM; > > + > > + uec->ec = ec; > > + init_completion(&uec->complete); > > + uec->nb.notifier_call = yoga_c630_ucsi_notify; > > + > > + uec->ucsi = ucsi_create(&adev->dev, &yoga_c630_ucsi_ops); > > + if (IS_ERR(uec->ucsi)) > > + return PTR_ERR(uec->ucsi); > > + > > + ucsi_set_drvdata(uec->ucsi, uec); > > + > > + uec->version = yoga_c630_ec_ucsi_get_version(uec->ec); > > + > > + auxiliary_set_drvdata(adev, uec); > > + > > + ret = yoga_c630_ec_register_notify(ec, &uec->nb); > > + if (ret) > > + return ret; > > + > > + return ucsi_register(uec->ucsi); > > +} > > + > > +static void yoga_c630_ucsi_remove(struct auxiliary_device *adev) > > +{ > > + struct yoga_c630_ucsi *uec = auxiliary_get_drvdata(adev); > > + > > + yoga_c630_ec_unregister_notify(uec->ec, &uec->nb); > > + ucsi_unregister(uec->ucsi); > > Usually, the remove should tear down in reverse order than the probe side. > Is the divergence from that here intentional? Yes, it's intentional, so that the driver doesn't get a notification while UCSI is being torn down. Consider it to be paired with ucsi_create().
Hi, On Thu, Jun 13, 2024 at 10:57:41AM GMT, Ilpo Järvinen wrote: > > + 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 **)? No, they are not supposed to be modified. It should be fine to make the struct member const char **, patches are welcome :) -- Sebastian