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.
