Message-ID: <CAE-0n51eSxxvnJXwnfPrXx1=rei=8OGGEtCAgw6nhCktZ0iQDw@mail.gmail.com>
Date: Tue, 3 Sep 2024 18:20:14 -0400
From: Stephen Boyd <swboyd@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	patches@...ts.linux.dev, devicetree@...r.kernel.org, 
	Douglas Anderson <dianders@...omium.org>, Pin-yen Lin <treapking@...omium.org>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Benson Leung <bleung@...omium.org>, 
	Conor Dooley <conor+dt@...nel.org>, Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...il.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, dri-devel@...ts.freedesktop.org, 
	Guenter Roeck <groeck@...omium.org>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	Jonas Karlman <jonas@...boo.se>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Lee Jones <lee@...nel.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Prashant Malani <pmalani@...omium.org>, 
	Robert Foss <rfoss@...nel.org>, Rob Herring <robh+dt@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, Daniel Scally <djrscally@...il.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>, Ivan Orlov <ivan.orlov0322@...il.com>, 
	linux-acpi@...r.kernel.org, linux-usb@...r.kernel.org, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH v4 06/18] drm/bridge: aux-hpd: Support USB Type-C DP
 altmodes via DRM lane assignment

Quoting Andy Shevchenko (2024-09-02 04:35:46)
> On Sat, Aug 31, 2024 at 09:06:44PM -0700, Stephen Boyd wrote:
> > Extend the aux-hpd bridge driver to support assigning DP lanes to USB
> > type-c pins based on typec mux state entry. Existing users of this
> > driver only need the HPD signaling support, so leave that in place and
> > wrap the code with a variant that supports more features of USB type-c
>
> Isn't the proper spelling "USB Type-C"?

Perhaps in a title?

>
> > DP altmode, i.e. pin configurations. Prefix that code with
> > 'drm_dp_typec_bridge' to differentiate it from the existing
> > 'drm_aux_hpd_bridge' code.
> >
> > Parse the struct typec_mux_state members to determine if DP altmode has
> > been entered and if HPD is asserted or not. Signal HPD to the drm bridge
> > chain when HPD is asserted. Similarly, parse the pin assignment and map
> > the DP lanes to the usb-c output lanes, taking into account any lane
> > remapping from the data-lanes endpoint property. Pass that lane mapping
> > to the previous drm_bridge in the bridge chain during the atomic check
> > phase.
>
> ...
>
> > +static inline struct drm_dp_typec_bridge_data *
> > +hpd_bridge_to_typec_bridge_data(struct drm_aux_hpd_bridge_data *hpd_data)
> > +{
> > +     return container_of(hpd_data, struct drm_dp_typec_bridge_data, hpd_bridge);
>
> + container_of.h ?
>
> With that said, can the argument be const here?

You mean 'hpd_data'? Don't think so because then we would want
container_of_const(), and to return a const pointer from this function
when drm_dp_typec_bridge_assign_pins() wants to modify struct
drm_dp_typec_bridge_data::num_lanes.

>
> > +}
>
> ...
>
> Ditto for the two more helpers, added in this change.

Ditto.

>
> ...
>
> > +static void drm_dp_typec_bridge_release(struct device *dev)
> > +{
> > +     struct drm_dp_typec_bridge_dev *typec_bridge_dev;
> > +     struct auxiliary_device *adev;
> > +
> > +     typec_bridge_dev = to_drm_dp_typec_bridge_dev(dev);
> > +     adev = &typec_bridge_dev->adev;
> > +
> > +     ida_free(&drm_aux_hpd_bridge_ida, adev->id);
>
> > +     of_node_put(adev->dev.platform_data);
> > +     of_node_put(adev->dev.of_node);
>
> I'm wondering why it's not made fwnode to begin with...
> From the file / function names it's not obvious that it's OF-only code. Neither
> there is no explanations why this must be OF-only code (among all fwnode types
> supported).

When in Rome? The drm_bridge code doesn't work with fwnode today, and
making it support fwnode is a much larger change. I'm not going to make
drm_bridge work with fwnode. Maybe when ACPI describes elements in the
display chain we can convert drm_bridge to use fwnode.

>
> > +     kfree(typec_bridge_dev);
> > +}
>
> ...
>
> > +             return ERR_PTR(dev_err_probe(parent, -ENODEV, "Missing typec endpoint(s) port@0\n"));
>
> We have a new helper for such cases.

Thanks!

>
> ...
>
> > +     adev->dev.of_node = of_node_get(parent->of_node);
>
> device_set_node() ?

Or device_set_of_node_from_dev()?

>
> ...
>
> > +     ret = auxiliary_device_init(adev);
> > +     if (ret) {
> > +             of_node_put(adev->dev.platform_data);
> > +             of_node_put(adev->dev.of_node);
> > +             ida_free(&drm_aux_hpd_bridge_ida, adev->id);
> > +             kfree(adev);
> > +             return ERR_PTR(ret);
>
> Can cleanup.h be utilised here and in other error paths in this function?

It looks like we can set these later and save on the of_node_put()s
until after the auxiliary_device_init() is called. Doing that allows
them to be in one place, the release function.

> > +static int dp_lane_to_typec_lane(enum dp_lane lane)
> > +{
> > +     switch (lane) {
> > +     case DP_ML0:
> > +             return USB_SSTX2;
> > +     case DP_ML1:
> > +             return USB_SSRX2;
> > +     case DP_ML2:
> > +             return USB_SSTX1;
> > +     case DP_ML3:
> > +             return USB_SSRX1;
> > +     }
>
> > +     return -EINVAL;
>
> Hmm... This can be simply made as default case.

And then the enum is always "covered" and the compiler doesn't complain
about missing cases (I don't think we have -Wswitch-enum)? Seems worse.

>
> > +}
>
> ...
>
> > +     for (i = 0; i < num_lanes; i++) {
> > +             /* Get physical type-c lane for DP lane */
> > +             typec_lane = dp_lane_to_typec_lane(i);
> > +             if (typec_lane < 0) {
> > +                     dev_err(&adev->dev, "Invalid type-c lane configuration at DP_ML%d\n", i);
> > +                     return -EINVAL;
>
> Most likely typec_lane contains an error code here. If yes, it would be rather
>
>                         return typec_lane;
>
> If no, why not make that happen?

Sure.

>
> > +static int drm_dp_typec_bridge_atomic_check(struct drm_bridge *bridge,
> > +                                        struct drm_bridge_state *bridge_state,
> > +                                        struct drm_crtc_state *crtc_state,
> > +                                        struct drm_connector_state *conn_state)
> > +{
> > +     struct drm_dp_typec_bridge_data *data;
> > +     struct drm_lane_cfg *in_lanes;
> > +     u8 *dp_lanes;
> > +     size_t num_lanes;
>
> > +     int i;
>
> Does it need to be signed? Can it theoretically overflow as num_lanes defined
> as size_t?

I guess it could but seems highly unlikely. Using unsigned is fine.

>
> > +             port->typec_data = typec_data;
> > +             if (of_property_read_u32_array(ep.local_node, "data-lanes",
> > +                                            port->lane_mapping,
> > +                                            ARRAY_SIZE(port->lane_mapping))) {
>
> > +                     memcpy(port->lane_mapping, mapping, sizeof(mapping));
>
> Hmm... I'm wondering if direct assignment will save a few .text bytes
>
>                 port->lane_mapping = ...;
>                 of_property_read_u32_array(ep.local_node, "data-lanes",
>                                            port->lane_mapping,
>                                            ARRAY_SIZE(port->lane_mapping));
>
> Also note that conditional is not needed in this case.
>

Ok. I'm fine with either way here. Maybe Dmitry has an opinion.

Powered by blists - more mailing lists