Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220208031944.3444-1-bjorn.andersson@linaro.org> <20220208031944.3444-7-bjorn.andersson@linaro.org>
In-Reply-To: <20220208031944.3444-7-bjorn.andersson@linaro.org>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 8 Feb 2022 15:24:00 +0200
Message-ID: <CAHp75VcQ+HAmaH6eS7vX-hxVFbBS50iYFpNTJRjs6Q40t7=1ww@mail.gmail.com>
Subject: Re: [PATCH v2 6/6] usb: typec: mux: Add On Semi fsa4480 driver
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Daniel Scally <djrscally@gmail.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        USB <linux-usb@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Feb 8, 2022 at 1:26 PM Bjorn Andersson
<bjorn.andersson@linaro.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