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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <CAEXTbpe_DGzF+M6Cm041Qkec-FZTXn5G8oCa_P-1cU355MA0JA@mail.gmail.com>
Date:   Mon, 20 Feb 2023 17:02:50 +0800
From:   Pin-yen Lin <treapking@...omium.org>
To:     Andi Shyti <andi.shyti@...ux.intel.com>
Cc:     Andrzej Hajda <andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Prashant Malani <pmalani@...omium.org>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        linux-kernel@...r.kernel.org,
        Nícolas F . R . A . Prado 
        <nfraprado@...labora.com>, Hsin-Yi Wang <hsinyi@...omium.org>,
        devicetree@...r.kernel.org, Allen Chen <allen.chen@....com.tw>,
        Lyude Paul <lyude@...hat.com>, linux-acpi@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, Marek Vasut <marex@...x.de>,
        Xin Ji <xji@...logixsemi.com>,
        Stephen Boyd <swboyd@...omium.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Javier Martinez Canillas <javierm@...hat.com>,
        chrome-platform@...ts.linux.dev,
        Alex Deucher <alexander.deucher@....com>,
        Douglas Anderson <dianders@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Imre Deak <imre.deak@...el.com>,
        Jani Nikula <jani.nikula@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Thierry Reding <treding@...dia.com>,
        Robert Foss <rfoss@...nel.org>
Subject: Re: [PATCH v11 3/9] drm/display: Add Type-C switch helpers

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@...omium.org> wrote:
>
> Hi Andi,
>
> Thanks for the review.
>
> On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti <andi.shyti@...ux.intel.com> 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@collabora.com/

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);
>>
>> [...]