linux-kernel - Re: [PATCH v2 6/6] usb: typec: mux: Add On Semi fsa4480 driver

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Feb 2022 15:24:00 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.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>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        USB <linux-usb@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Subject: Re: [PATCH v2 6/6] usb: typec: mux: Add On Semi fsa4480 driver

On Tue, Feb 8, 2022 at 1:26 PM Bjorn Andersson
<bjorn.andersson@...aro.org> wrote:
>
> The ON Semiconductor FSA4480 is a USB Type-C port multimedia switch with
> support for analog audio headsets. It allows sharing a common USB Type-C
> port to pass USB2.0 signal, analog audio, sideband use wires and analog
> microphone signal.
>
> Due to lacking upstream audio support for testing, the audio muxing is
> left untouched, but implementation of muxing the SBU lines is provided
> as a pair of TypeC mux and switch devices. This provides the necessary
> support for enabling the DisplayPort altmode on devices with this
> circuit.

...

> +config TYPEC_MUX_FSA4480
> +       tristate "ON Semi FSA4480 Analog Audio Switch driver"
> +       depends on I2C
> +       select REGMAP_I2C
> +       help
> +         Driver for the ON Semiconductor FSA4480 Analog Audio Switch, which
> +         provides support for muxing analog audio and sideband signals on a
> +         common USB Type-C connector.

What would be the module name?

...

> +/*
> + * Copyright (C) 2021 Linaro Ltd.

2021-2022 ?

> + * Copyright (C) 2018-2020 The Linux Foundation
> + */

...

Missed mod_devicetable.h ?

...

> +#define FSA4480_ENABLE_DEVICE  BIT(7)
> +#define FSA4480_ENABLE_SBU     GENMASK(6, 5)
> +#define FSA4480_ENABLE_USB     GENMASK(4, 3)

Don't forget to include bits.h

...

> +       /* used to serialize concurrect change requests */

concurrent

...

> +static const struct regmap_config fsa4480_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,

> +       .max_register = FSA4480_RESET,

I would create a specific macro, to avoid confusion in case if there
will be more hw revisions with slightly different register layouts.

> +};


...

> +       fsa->regmap = devm_regmap_init_i2c(client, &fsa4480_regmap_config);
> +       if (IS_ERR(fsa->regmap)) {

> +               dev_err(dev, "failed to initialize regmap\n");
> +               return PTR_ERR(fsa->regmap);

return dev_err_probe();

> +       }

...

> +       fsa->sw = typec_switch_register(dev, &sw_desc);
> +       if (IS_ERR(fsa->sw)) {
> +               dev_err(dev, "failed to register typec switch: %ld\n", PTR_ERR(fsa->sw));
> +               return PTR_ERR(fsa->sw);

Ditto.

> +       }

...

> +       mux_desc.fwnode = dev->fwnode;

Please, avoid dereferencing fwnode, use dev_fwnode() instead.

...

> +       fsa->mux = typec_mux_register(dev, &mux_desc);
> +       if (IS_ERR(fsa->mux)) {
> +               typec_switch_unregister(fsa->sw);
> +               dev_err(dev, "failed to register typec mux: %ld\n", PTR_ERR(fsa->mux));
> +               return PTR_ERR(fsa->mux);

return dev_err_probe();

> +       }

...

> +static struct i2c_driver fsa4480_driver = {
> +       .driver = {
> +               .name = "fsa4480",
> +               .of_match_table = fsa4480_of_table,
> +       },
> +       .probe_new      = fsa4480_probe,
> +       .remove         = fsa4480_remove,
> +       .id_table       = fsa4480_table,
> +};

> +

Redundant blank line.

> +module_i2c_driver(fsa4480_driver);

-- 
With Best Regards,
Andy Shevchenko