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

[Andy Shevchenko]

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