| Message ID | 20220810030500.2793882-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | power: supply: Lenovo Yoga C630 EC | expand |
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! > 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.
On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote: > Hi, > > [+Cc Lee Jones, DRI devel] > > 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. > > > > 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> > > --- > > [...] > > + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES); > > + if (val < 0) > > + goto out_unlock; > > + ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA; > > + if (!ec->unit_ma) > > + ec->scale = 1000; > > + else > > + ec->scale = 1; > > Since I'm not sure how much of information was gained by reverse > engineering: Is this really milliamps vs microamps and not milliamps > vs milliwatt? SBS batteries usually report either mA or mW. > Unfortunately I don't know the answer to this. > > [...] > > + case POWER_SUPPLY_PROP_SERIAL_NUMBER: > > + val->strval = "05072018"; > > + break; > > why is this hardcoded? :) > I don't know, but as Daniel suggests, it would make sense to just drop it. > > [...] > > + device_for_each_child_node(dev, fwnode) { > > + ret = fwnode_property_read_u32(fwnode, "reg", &port); > > + if (ret < 0) > > + continue; > > + > > + /* Got multiple ports, but altmode is only possible on port 1 */ > > + if (port != 1) > > + continue; > > + > > + ec->bridge.funcs = &yoga_c630_ec_bridge_funcs; > > + ec->bridge.of_node = to_of_node(fwnode); > > + ec->bridge.ops = DRM_BRIDGE_OP_HPD; > > + ec->bridge.type = DRM_MODE_CONNECTOR_USB; > > + > > + ret = devm_drm_bridge_add(dev, &ec->bridge); > > + if (ret) { > > + dev_err(dev, "failed to register drm bridge\n"); > > + fwnode_handle_put(fwnode); > > + return ret; > > + } > > I wonder if DRM people want to see this in drivers/gpu/drm/bridge. > Maybe it's better to make this a MFD driver? > I did consider it, but it adds a whole bunch of boiler plate code without a lot of benefit. There are a few other cases where I think it would make sense to have drm bridges outside of drivers/gpu/drm, such as drivers/usb/typec/altmodes/ and drivers/platform/chrome/... > > [...] > > -- Sebastian Thanks, Bjorn
On Fri, 16 Sept 2022 at 00:25, Bjorn Andersson <andersson@kernel.org> wrote: > > On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote: > > Hi, > > > > [+Cc Lee Jones, DRI devel] > > > > 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. > > > > > > 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> > > > --- > > > [...] > > > + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES); > > > + if (val < 0) > > > + goto out_unlock; > > > + ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA; > > > + if (!ec->unit_ma) > > > + ec->scale = 1000; > > > + else > > > + ec->scale = 1; > > > > Since I'm not sure how much of information was gained by reverse > > engineering: Is this really milliamps vs microamps and not milliamps > > vs milliwatt? SBS batteries usually report either mA or mW. > > > > Unfortunately I don't know the answer to this. > > > > [...] > > > + case POWER_SUPPLY_PROP_SERIAL_NUMBER: > > > + val->strval = "05072018"; > > > + break; > > > > why is this hardcoded? :) > > > > I don't know, but as Daniel suggests, it would make sense to just drop > it. > > > > [...] > > > + device_for_each_child_node(dev, fwnode) { > > > + ret = fwnode_property_read_u32(fwnode, "reg", &port); > > > + if (ret < 0) > > > + continue; > > > + > > > + /* Got multiple ports, but altmode is only possible on port 1 */ > > > + if (port != 1) > > > + continue; > > > + > > > + ec->bridge.funcs = &yoga_c630_ec_bridge_funcs; > > > + ec->bridge.of_node = to_of_node(fwnode); > > > + ec->bridge.ops = DRM_BRIDGE_OP_HPD; > > > + ec->bridge.type = DRM_MODE_CONNECTOR_USB; > > > + > > > + ret = devm_drm_bridge_add(dev, &ec->bridge); > > > + if (ret) { > > > + dev_err(dev, "failed to register drm bridge\n"); > > > + fwnode_handle_put(fwnode); > > > + return ret; > > > + } > > > > I wonder if DRM people want to see this in drivers/gpu/drm/bridge. > > Maybe it's better to make this a MFD driver? > > > > I did consider it, but it adds a whole bunch of boiler plate code > without a lot of benefit. > > There are a few other cases where I think it would make sense to have > drm bridges outside of drivers/gpu/drm, such as > drivers/usb/typec/altmodes/ and drivers/platform/chrome/... What about a solution which might sound simpler than MFD: from your driver's probe() register a platform device which represents a drm bridge. Add drm_bridge driver for it into drivers/gpu/drm/bridges/.
On Fri, Sep 16, 2022 at 12:53:42AM +0300, Dmitry Baryshkov wrote: > On Fri, 16 Sept 2022 at 00:25, Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote: > > > Hi, > > > > > > [+Cc Lee Jones, DRI devel] > > > > > > 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. > > > > > > > > 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> > > > > --- > > > > [...] > > > > + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES); > > > > + if (val < 0) > > > > + goto out_unlock; > > > > + ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA; > > > > + if (!ec->unit_ma) > > > > + ec->scale = 1000; > > > > + else > > > > + ec->scale = 1; > > > > > > Since I'm not sure how much of information was gained by reverse > > > engineering: Is this really milliamps vs microamps and not milliamps > > > vs milliwatt? SBS batteries usually report either mA or mW. > > > > > > > Unfortunately I don't know the answer to this. > > > > > > [...] > > > > + case POWER_SUPPLY_PROP_SERIAL_NUMBER: > > > > + val->strval = "05072018"; > > > > + break; > > > > > > why is this hardcoded? :) > > > > > > > I don't know, but as Daniel suggests, it would make sense to just drop > > it. > > > > > > [...] > > > > + device_for_each_child_node(dev, fwnode) { > > > > + ret = fwnode_property_read_u32(fwnode, "reg", &port); > > > > + if (ret < 0) > > > > + continue; > > > > + > > > > + /* Got multiple ports, but altmode is only possible on port 1 */ > > > > + if (port != 1) > > > > + continue; > > > > + > > > > + ec->bridge.funcs = &yoga_c630_ec_bridge_funcs; > > > > + ec->bridge.of_node = to_of_node(fwnode); > > > > + ec->bridge.ops = DRM_BRIDGE_OP_HPD; > > > > + ec->bridge.type = DRM_MODE_CONNECTOR_USB; > > > > + > > > > + ret = devm_drm_bridge_add(dev, &ec->bridge); > > > > + if (ret) { > > > > + dev_err(dev, "failed to register drm bridge\n"); > > > > + fwnode_handle_put(fwnode); > > > > + return ret; > > > > + } > > > > > > I wonder if DRM people want to see this in drivers/gpu/drm/bridge. > > > Maybe it's better to make this a MFD driver? > > > > > > > I did consider it, but it adds a whole bunch of boiler plate code > > without a lot of benefit. > > > > There are a few other cases where I think it would make sense to have > > drm bridges outside of drivers/gpu/drm, such as > > drivers/usb/typec/altmodes/ and drivers/platform/chrome/... > > What about a solution which might sound simpler than MFD: from your > driver's probe() register a platform device which represents a drm > bridge. Add drm_bridge driver for it into drivers/gpu/drm/bridges/. > That is indeed simpler But then rather than just calling drm_bridge_hpd_notify(status) we need a new custom interface for this driver to call the other drive when status changes and we waste 840 bytes just to track another struct platform_device. Perhaps that could be made generic, so we'd have "proxy_drm_bridge" driver for drivers who implements a HPD drm_bridge but don't want to implement that outside drivers/gpu/drm. But this is just based on the assumption that the "DRM people" require drm_bridges to only live in drivers/gpu/drm/. It's all one kernel and the API is well defined, so I don't think there's a concrete problem to solve. Regards, Bjorn