* Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver
[not found] ` <20220810030500.2793882-3-bjorn.andersson@linaro.org>
@ 2022-09-13 10:45 ` Sebastian Reichel
2022-09-15 21:25 ` Bjorn Andersson
0 siblings, 1 reply; 4+ messages in thread
From: Sebastian Reichel @ 2022-09-13 10:45 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, linux-pm, linux-arm-msm, Lee Jones, linux-kernel,
dri-devel, Rob Herring, Krzysztof Kozlowski
[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]
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.
> [...]
> + case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> + val->strval = "05072018";
> + break;
why is this hardcoded? :)
> [...]
> + 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?
> [...]
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver
2022-09-13 10:45 ` [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver Sebastian Reichel
@ 2022-09-15 21:25 ` Bjorn Andersson
2022-09-15 21:53 ` Dmitry Baryshkov
0 siblings, 1 reply; 4+ messages in thread
From: Bjorn Andersson @ 2022-09-15 21:25 UTC (permalink / raw)
To: Sebastian Reichel
Cc: devicetree, linux-pm, linux-arm-msm, Lee Jones, linux-kernel,
dri-devel, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver
2022-09-15 21:25 ` Bjorn Andersson
@ 2022-09-15 21:53 ` Dmitry Baryshkov
2022-09-21 22:10 ` Bjorn Andersson
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Baryshkov @ 2022-09-15 21:53 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, linux-pm, linux-kernel, Lee Jones, Sebastian Reichel,
dri-devel, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
linux-arm-msm
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/.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver
2022-09-15 21:53 ` Dmitry Baryshkov
@ 2022-09-21 22:10 ` Bjorn Andersson
0 siblings, 0 replies; 4+ messages in thread
From: Bjorn Andersson @ 2022-09-21 22:10 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: devicetree, linux-pm, linux-kernel, Lee Jones, Sebastian Reichel,
dri-devel, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
linux-arm-msm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-21 22:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220810030500.2793882-1-bjorn.andersson@linaro.org>
[not found] ` <20220810030500.2793882-3-bjorn.andersson@linaro.org>
2022-09-13 10:45 ` [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver Sebastian Reichel
2022-09-15 21:25 ` Bjorn Andersson
2022-09-15 21:53 ` Dmitry Baryshkov
2022-09-21 22:10 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).