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

Andy Shevchenko
2022-02-08T13:29:03Z

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