Messages in this thread | | | From | Pin-yen Lin <> | Date | Fri, 25 Nov 2022 14:58:58 +0800 | Subject | Re: [PATCH v6 5/7] drm/bridge: anx7625: Register Type C mode switches |
| |
Hi Andy,
Thanks for reviewing the patch.
On Thu, Nov 24, 2022 at 8:18 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 24, 2022 at 06:20:54PM +0800, Pin-yen Lin wrote: > > Register USB Type-C mode switches when the "mode-switch" property and > > relevant port are available in Device Tree. Configure the crosspoint > > switch based on the entered alternate mode for a specific Type-C > > connector. > > ... > > > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx) > > +{ > > + if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected) > > + /* Both ports available, do nothing to retain the current one. */ > > + return; > > > + else if (ctx->typec_ports[0].dp_connected) > > This 'else' is redundant. I would rewrite above as > > /* Check if both ports available and do nothing to retain the current one */ > if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected) > return; > > if (ctx->typec_ports[0].dp_connected) > > > + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL); > > + else if (ctx->typec_ports[1].dp_connected) > > + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE); > > +}
Thanks for the detailed suggestion. I'll adapt this in v7. > > ... > > > + data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID && > > + state->alt->mode == USB_TYPEC_DP_MODE); > > Parentheses are not needed.
Will fix this in v7. > > ... > > > + /* > > + * <0 1> refers to SSRX1/SSTX1, and <2 3> refers to SSRX2/SSTX2. > > + */ > > + for (i = 0; i < num_lanes; i++) { > > > + if (port_num != -1 && port_num != dp_lanes[i] / 2) { > > + dev_err(dev, "Invalid data lane numbers\n"); > > + return -EINVAL; > > + } > > According to Rob Linux must not validate device tree. If you need it, use > proper YAML schema. > > > + port_num = dp_lanes[i] / 2; > > + } >
I'll remove this from the driver in v7.
> ... > > > + if (!ctx->num_typec_switches) { > > + dev_warn(dev, "No Type-C switches node found\n"); > > > + return ret; > > Why not to return 0 explicitly?
Will update to just "return 0" in v7.
> > > + } > > ... > > > + ctx->typec_ports = devm_kcalloc( > > Broken indentation.
Will fix in v7 > > > + dev, ctx->num_typec_switches, sizeof(struct anx7625_port_data), > > + GFP_KERNEL); > > + if (!ctx->typec_ports) > > + return -ENOMEM; > > ... > > > +struct anx7625_port_data { > > > + bool dp_connected; > > You can save some bytes on some architectures if move this to be last field.
Thanks for the suggestion. I'll do so in v7 > > > + struct typec_mux_dev *typec_mux; > > + struct anx7625_data *ctx; > > +}; > > -- > With Best Regards, > Andy Shevchenko > >
| |