Re: [PATCH v11 3/9] drm/display: Add Type-C switch helpers

[Pin-yen Lin]

I think I accidentally used HTML mode for the previous email. Sorry about that.

On Mon, Feb 20, 2023 at 4:41 PM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote:
>
> Hi Andi,
>
> Thanks for the review.
>
> On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> wrote:
>>
>> Hi Pin-yen,
>>
>> [...]
>>
>> > +static int drm_dp_register_mode_switch(struct device *dev,
>> > +                                    struct fwnode_handle *fwnode,
>> > +                                    struct drm_dp_typec_switch_desc *switch_desc,
>> > +                                    void *data, typec_mux_set_fn_t mux_set)
>> > +{
>> > +     struct drm_dp_typec_port_data *port_data;
>> > +     struct typec_mux_desc mux_desc = {};
>> > +     char name[32];
>> > +     u32 port_num;
>> > +     int ret;
>> > +
>> > +     ret = fwnode_property_read_u32(fwnode, "reg", &port_num);
>> > +     if (ret) {
>> > +             dev_err(dev, "Failed to read reg property: %d\n", ret);
>> > +             return ret;
>> > +     }
>> > +
>> > +     port_data = &switch_desc->typec_ports[port_num];
>> > +     port_data->data = data;
>> > +     port_data->port_num = port_num;
>> > +     port_data->fwnode = fwnode;
>> > +     mux_desc.fwnode = fwnode;
>> > +     mux_desc.drvdata = port_data;
>> > +     snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num);
>> > +     mux_desc.name = name;
>> > +     mux_desc.set = mux_set;
>> > +
>> > +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
>> > +     if (IS_ERR(port_data->typec_mux)) {
>> > +             ret = PTR_ERR(port_data->typec_mux);
>> > +             dev_err(dev, "Mode switch register for port %d failed: %d\n",
>> > +                     port_num, ret);
>> > +
>> > +             return ret;
>>
>> you don't need this return here...
>>
>> > +     }
>> > +
>> > +     return 0;
>>
>> Just "return ret;" here.

This was actually suggested by Angelo in [1]. I personally don't have
a strong opinion on either approach.

[1]https://lore.kernel.org/all/023519eb-0adb-3b08-71b9-afb92a6cceaf@xxxxxxxxxxxxx/

Pin-yen
>>
>>
>> > +}
>> > +
>> > +/**
>> > + * drm_dp_register_typec_switches() - register Type-C switches
>> > + * @dev: Device that registers Type-C switches
>> > + * @port: Device node for the switch
>> > + * @switch_desc: A Type-C switch descriptor
>> > + * @data: Private data for the switches
>> > + * @mux_set: Callback function for typec_mux_set
>> > + *
>> > + * This function registers USB Type-C switches for DP bridges that can switch
>> > + * the output signal between their output pins.
>> > + *
>> > + * Currently only mode switches are implemented, and the function assumes the
>> > + * given @port device node has endpoints with "mode-switch" property.
>> > + * The port number is determined by the "reg" property of the endpoint.
>> > + */
>> > +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port,
>> > +                                struct drm_dp_typec_switch_desc *switch_desc,
>> > +                                void *data, typec_mux_set_fn_t mux_set)
>> > +{
>> > +     struct fwnode_handle *sw;
>> > +     int ret;
>> > +
>> > +     fwnode_for_each_child_node(port, sw) {
>> > +             if (fwnode_property_present(sw, "mode-switch"))
>> > +                     switch_desc->num_typec_switches++;
>> > +     }
>>
>> no need for brackets here
>>
>> > +
>> > +     if (!switch_desc->num_typec_switches) {
>> > +             dev_dbg(dev, "No Type-C switches node found\n");
>>
>> dev_warn()?
>
>
> I used dev_dbg here because the users might call this without checking if there are mode switch endpoints present, and this is the case for the current users (it6505 and anx7625). If we use dev_warn here, there will be warnings every time even on use cases without Type-C switches.
>
> Thanks and regards,
> Pin-yen
>>
>>
>> > +             return 0;
>> > +     }
>> > +
>> > +     switch_desc->typec_ports = devm_kcalloc(
>> > +             dev, switch_desc->num_typec_switches,
>> > +             sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
>> > +
>> > +     if (!switch_desc->typec_ports)
>> > +             return -ENOMEM;
>> > +
>> > +     /* Register switches for each connector. */
>> > +     fwnode_for_each_child_node(port, sw) {
>> > +             if (!fwnode_property_present(sw, "mode-switch"))
>> > +                     continue;
>> > +             ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
>> > +             if (ret)
>> > +                     goto err_unregister_typec_switches;
>> > +     }
>> > +
>> > +     return 0;
>> > +
>> > +err_unregister_typec_switches:
>> > +     fwnode_handle_put(sw);
>> > +     drm_dp_unregister_typec_switches(switch_desc);
>> > +     dev_err(dev, "Failed to register mode switch: %d\n", ret);
>>
>> there is a bit of dmesg spamming. Please choose where you want to
>> print the error, either in this function or in
>> drm_dp_register_mode_switch().
>>
>> Andi
>>
>> > +     return ret;
>> > +}
>> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
>>
>> [...]