| Message ID | 20240829-x1e80100-ps8830-v1-2-bcc4790b1d45@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: Add new driver for Parade PS8830 Type-C Retimer | expand |
On 29.08.2024 8:44 PM, Abel Vesa wrote: > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C. > It provides both altmode and orientation handling. > > Add a driver with support for the following modes: > - DP 4lanes > - USB3 > - DP 2lanes + USB3 > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- [...] > + retimer->supplies[0].supply = "vdd33"; > + retimer->supplies[1].supply = "vdd18"; > + retimer->supplies[2].supply = "vdd15"; 1.15? Konrad
On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote: > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C. > It provides both altmode and orientation handling. > > Add a driver with support for the following modes: > - DP 4lanes > - USB3 > - DP 2lanes + USB3 > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > +struct ps8830_retimer { > + struct i2c_client *client; > + struct regulator_bulk_data supplies[4]; > + struct gpio_desc *reset_gpio; > + struct regmap *regmap; > + struct typec_switch_dev *sw; > + struct typec_retimer *retimer; > + struct clk *xo_clk; > + > + bool needs_update; > + struct typec_switch *typec_switch; > + struct typec_mux *typec_mux; > + > + struct mutex lock; /* protect non-concurrent retimer & switch */ > + > + enum typec_orientation orientation; > + unsigned long mode; > + int cfg[3]; > + Stray newline. > +}; > + > +static int ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2) > +{ > + if (cfg0 == retimer->cfg[0] && > + cfg1 == retimer->cfg[1] && > + cfg2 == retimer->cfg[2]) > + return 0; > + > + retimer->cfg[0] = cfg0; > + retimer->cfg[1] = cfg1; > + retimer->cfg[2] = cfg2; > + > + regmap_write(retimer->regmap, 0x0, cfg0); > + regmap_write(retimer->regmap, 0x1, cfg1); > + regmap_write(retimer->regmap, 0x2, cfg2); > + > + return 0; > +} You always return 0 here so should this be a void function? > + > +static int ps8380_set(struct ps8830_retimer *retimer) > +{ > + int cfg0 = 0x00, cfg1 = 0x00, cfg2 = 0x00; Please avoid doing multiple initialisations like this (one per line is more readable). > + int ret; > + > + retimer->needs_update = false; > + > + switch (retimer->orientation) { > + /* Safe mode */ > + case TYPEC_ORIENTATION_NONE: > + cfg0 = 0x01; > + cfg1 = 0x00; > + cfg2 = 0x00; > + break; > + case TYPEC_ORIENTATION_NORMAL: > + cfg0 = 0x01; > + break; > + case TYPEC_ORIENTATION_REVERSE: > + cfg0 = 0x03; > + break; > + } > + > + switch (retimer->mode) { > + /* Safe mode */ > + case TYPEC_STATE_SAFE: > + cfg0 = 0x01; > + cfg1 = 0x00; > + cfg2 = 0x00; > + break; > + > + /* USB3 Only */ > + case TYPEC_STATE_USB: > + cfg0 |= 0x20; > + break; > + > + /* DP Only */ > + case TYPEC_DP_STATE_C: > + case TYPEC_DP_STATE_E: > + cfg0 &= 0x0f; > + cfg1 = 0x85; > + break; > + > + /* DP + USB */ > + case TYPEC_DP_STATE_D: > + case TYPEC_DP_STATE_F: > + cfg0 |= 0x20; > + cfg1 = 0x85; > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + gpiod_set_value(retimer->reset_gpio, 0); > + msleep(20); > + gpiod_set_value(retimer->reset_gpio, 1); > + > + msleep(60); > + > + ret = ps8830_configure(retimer, 0x01, 0x00, 0x00); > + > + msleep(30); > + > + return ps8830_configure(retimer, cfg0, cfg1, cfg2); As the build bots pointed out, ret is never used, and the configure function always returns 0. Make the function type void and return 0 explicitly here instead? > +} > +static int ps8830_retimer_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct typec_switch_desc sw_desc = { }; > + struct typec_retimer_desc rtmr_desc = { }; > + struct ps8830_retimer *retimer; > + int ret; > + > + retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL); > + if (!retimer) > + return -ENOMEM; > + > + retimer->client = client; > + > + retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap); > + if (IS_ERR(retimer->regmap)) { > + dev_err(dev, "Failed to allocate register map\n"); > + return PTR_ERR(retimer->regmap); > + } > + > + retimer->supplies[0].supply = "vdd33"; > + retimer->supplies[1].supply = "vdd18"; > + retimer->supplies[2].supply = "vdd15"; vdd115? > + retimer->supplies[3].supply = "vcc"; > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(retimer->supplies), > + retimer->supplies); > + if (ret) > + return ret; > + > + retimer->xo_clk = devm_clk_get(dev, "xo"); > + if (IS_ERR(retimer->xo_clk)) > + return PTR_ERR(retimer->xo_clk); > + > + retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(retimer->reset_gpio)) > + return PTR_ERR(retimer->reset_gpio); > + > + retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode); > + if (IS_ERR(retimer->typec_switch)) > + return dev_err_probe(dev, PTR_ERR(retimer->typec_switch), > + "failed to acquire orientation-switch\n"); > + > + retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode); > + if (IS_ERR(retimer->typec_mux)) { > + ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux), > + "failed to acquire mode-mux\n"); > + goto err_switch_put; > + } > + > + ret = regulator_bulk_enable(ARRAY_SIZE(retimer->supplies), > + retimer->supplies); > + if (ret < 0) { > + dev_err(dev, "cannot enable regulators %d\n", ret); Please add a colon after "regulators" to maintain a consistent style of error messages. > + goto err_mux_put; > + } > + > + ret = clk_prepare_enable(retimer->xo_clk); > + if (ret) { > + dev_err(dev, "Failed to enable XO: %d\n", ret); Lower case "failed" for consistency. > + goto err_disable_vreg; > + } > + > + gpiod_set_value(retimer->reset_gpio, 0); > + msleep(20); > + gpiod_set_value(retimer->reset_gpio, 1); > + > + msleep(60); > + mutex_init(&retimer->lock); I'd initialise resources like this before resetting the device (e.g. move above regmap init). > + > + sw_desc.drvdata = retimer; > + sw_desc.fwnode = dev_fwnode(dev); > + sw_desc.set = ps8830_sw_set; > + > + ret = drm_aux_bridge_register(dev); > + if (ret) > + goto err_disable_gpio; > + > + retimer->sw = typec_switch_register(dev, &sw_desc); > + if (IS_ERR(retimer->sw)) { > + ret = dev_err_probe(dev, PTR_ERR(retimer->sw), > + "Error registering typec switch\n"); Switch registration cannot return EPROBE_DEFER so I suggest using dev_err() for clarity (e.g. as you must not call functions that can defer probe after registering child devices like the aux bridge). > + goto err_disable_gpio; > + } > + > + rtmr_desc.drvdata = retimer; > + rtmr_desc.fwnode = dev_fwnode(dev); > + rtmr_desc.set = ps8830_retimer_set; > + > + retimer->retimer = typec_retimer_register(dev, &rtmr_desc); > + if (IS_ERR(retimer->retimer)) { > + ret = dev_err_probe(dev, PTR_ERR(retimer->retimer), > + "Error registering typec retimer\n"); Same here. > + goto err_switch_unregister; > + } > + > + dev_info(dev, "Registered Parade PS8830 retimer\n"); Drop this, drivers shouldn't spam the logs on success. > + return 0; > + > +err_switch_unregister: > + typec_switch_unregister(retimer->sw); > + > +err_disable_gpio: > + gpiod_set_value(retimer->reset_gpio, 0); > + clk_disable_unprepare(retimer->xo_clk); > + > +err_disable_vreg: > + regulator_bulk_disable(ARRAY_SIZE(retimer->supplies), > + retimer->supplies); > +err_mux_put: > + typec_mux_put(retimer->typec_mux); > + > +err_switch_put: > + typec_switch_put(retimer->typec_switch); > + > + return ret; > +} > +static const struct i2c_device_id ps8830_retimer_table[] = { > + { "parade,ps8830" }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, ps8830_retimer_table); This one should not be needed, right? > +static const struct of_device_id ps8830_retimer_of_table[] = { > + { .compatible = "parade,ps8830" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ps8830_retimer_of_table); Johan
On Tue, Sep 03, 2024 at 09:27:45AM +0200, Johan Hovold wrote: > On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote: > > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C. > > It provides both altmode and orientation handling. > > > > Add a driver with support for the following modes: > > - DP 4lanes > > - USB3 > > - DP 2lanes + USB3 > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > + retimer->supplies[0].supply = "vdd33"; > > + retimer->supplies[1].supply = "vdd18"; > > + retimer->supplies[2].supply = "vdd15"; > > vdd115? > > > + retimer->supplies[3].supply = "vcc"; I took a look at the schematics and it seems like all but one of the above supply names are wrong and that some are missing. "vcc" also does not exist in either the binding or dt patches you sent separately. From what I can tell the supplies are: vdd 1.15 V vdd33 3.3 V vdd33_cap 3.3 V vddat 1.15 V vddar 1.15 V vddio 1.8 V Also, have you checked that there are no ordering constraints between the supplies? Johan
diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig index ce7db6ad3057..48613b67f1c5 100644 --- a/drivers/usb/typec/mux/Kconfig +++ b/drivers/usb/typec/mux/Kconfig @@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C redriver chip found on some devices with a Type-C port. +config TYPEC_MUX_PS8830 + tristate "Parade PS8830 Type-C retimer driver" + depends on I2C + depends on DRM || DRM=n + select DRM_AUX_BRIDGE if DRM_BRIDGE && OF + select REGMAP_I2C + help + Say Y or M if your system has a Parade PS8830 Type-C retimer chip + found on some devices with a Type-C port. + config TYPEC_MUX_PTN36502 tristate "NXP PTN36502 Type-C redriver driver" depends on I2C diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile index bb96f30267af..4b23b12cfe45 100644 --- a/drivers/usb/typec/mux/Makefile +++ b/drivers/usb/typec/mux/Makefile @@ -6,5 +6,6 @@ obj-$(CONFIG_TYPEC_MUX_PI3USB30532) += pi3usb30532.o obj-$(CONFIG_TYPEC_MUX_INTEL_PMC) += intel_pmc_mux.o obj-$(CONFIG_TYPEC_MUX_IT5205) += it5205.o obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M) += nb7vpq904m.o +obj-$(CONFIG_TYPEC_MUX_PS8830) += ps8830.o obj-$(CONFIG_TYPEC_MUX_PTN36502) += ptn36502.o obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS) += wcd939x-usbss.o diff --git a/drivers/usb/typec/mux/ps8830.c b/drivers/usb/typec/mux/ps8830.c new file mode 100644 index 000000000000..517ccac5932f --- /dev/null +++ b/drivers/usb/typec/mux/ps8830.c @@ -0,0 +1,347 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Parade PS8830 usb retimer driver + * + * Copyright (C) 2024 Linaro Ltd. + */ + +#include <drm/bridge/aux-bridge.h> +#include <linux/clk.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/usb/typec_altmode.h> +#include <linux/usb/typec_dp.h> +#include <linux/usb/typec_mux.h> +#include <linux/usb/typec_retimer.h> + +struct ps8830_retimer { + struct i2c_client *client; + struct regulator_bulk_data supplies[4]; + struct gpio_desc *reset_gpio; + struct regmap *regmap; + struct typec_switch_dev *sw; + struct typec_retimer *retimer; + struct clk *xo_clk; + + bool needs_update; + struct typec_switch *typec_switch; + struct typec_mux *typec_mux; + + struct mutex lock; /* protect non-concurrent retimer & switch */ + + enum typec_orientation orientation; + unsigned long mode; + int cfg[3]; + +}; + +static int ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2) +{ + if (cfg0 == retimer->cfg[0] && + cfg1 == retimer->cfg[1] && + cfg2 == retimer->cfg[2]) + return 0; + + retimer->cfg[0] = cfg0; + retimer->cfg[1] = cfg1; + retimer->cfg[2] = cfg2; + + regmap_write(retimer->regmap, 0x0, cfg0); + regmap_write(retimer->regmap, 0x1, cfg1); + regmap_write(retimer->regmap, 0x2, cfg2); + + return 0; +} + +static int ps8380_set(struct ps8830_retimer *retimer) +{ + int cfg0 = 0x00, cfg1 = 0x00, cfg2 = 0x00; + int ret; + + retimer->needs_update = false; + + switch (retimer->orientation) { + /* Safe mode */ + case TYPEC_ORIENTATION_NONE: + cfg0 = 0x01; + cfg1 = 0x00; + cfg2 = 0x00; + break; + case TYPEC_ORIENTATION_NORMAL: + cfg0 = 0x01; + break; + case TYPEC_ORIENTATION_REVERSE: + cfg0 = 0x03; + break; + } + + switch (retimer->mode) { + /* Safe mode */ + case TYPEC_STATE_SAFE: + cfg0 = 0x01; + cfg1 = 0x00; + cfg2 = 0x00; + break; + + /* USB3 Only */ + case TYPEC_STATE_USB: + cfg0 |= 0x20; + break; + + /* DP Only */ + case TYPEC_DP_STATE_C: + case TYPEC_DP_STATE_E: + cfg0 &= 0x0f; + cfg1 = 0x85; + break; + + /* DP + USB */ + case TYPEC_DP_STATE_D: + case TYPEC_DP_STATE_F: + cfg0 |= 0x20; + cfg1 = 0x85; + break; + + default: + return -EOPNOTSUPP; + } + + gpiod_set_value(retimer->reset_gpio, 0); + msleep(20); + gpiod_set_value(retimer->reset_gpio, 1); + + msleep(60); + + ret = ps8830_configure(retimer, 0x01, 0x00, 0x00); + + msleep(30); + + return ps8830_configure(retimer, cfg0, cfg1, cfg2); +} + +static int ps8830_sw_set(struct typec_switch_dev *sw, + enum typec_orientation orientation) +{ + struct ps8830_retimer *retimer = typec_switch_get_drvdata(sw); + int ret = 0; + + ret = typec_switch_set(retimer->typec_switch, orientation); + if (ret) + return ret; + + mutex_lock(&retimer->lock); + + if (retimer->orientation != orientation) { + retimer->orientation = orientation; + retimer->needs_update = true; + } + + if (retimer->needs_update) + ret = ps8380_set(retimer); + + mutex_unlock(&retimer->lock); + + return ret; +} + +static int ps8830_retimer_set(struct typec_retimer *rtmr, + struct typec_retimer_state *state) +{ + struct ps8830_retimer *retimer = typec_retimer_get_drvdata(rtmr); + struct typec_mux_state mux_state; + int ret = 0; + + mutex_lock(&retimer->lock); + + if (state->mode != retimer->mode) { + retimer->mode = state->mode; + retimer->needs_update = true; + } + + if (retimer->needs_update) + ret = ps8380_set(retimer); + + mutex_unlock(&retimer->lock); + + if (ret) + return ret; + + mux_state.alt = state->alt; + mux_state.data = state->data; + mux_state.mode = state->mode; + + return typec_mux_set(retimer->typec_mux, &mux_state); +} + +static const struct regmap_config ps8830_retimer_regmap = { + .max_register = 0x1f, + .reg_bits = 8, + .val_bits = 8, +}; + +static int ps8830_retimer_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct typec_switch_desc sw_desc = { }; + struct typec_retimer_desc rtmr_desc = { }; + struct ps8830_retimer *retimer; + int ret; + + retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL); + if (!retimer) + return -ENOMEM; + + retimer->client = client; + + retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap); + if (IS_ERR(retimer->regmap)) { + dev_err(dev, "Failed to allocate register map\n"); + return PTR_ERR(retimer->regmap); + } + + retimer->supplies[0].supply = "vdd33"; + retimer->supplies[1].supply = "vdd18"; + retimer->supplies[2].supply = "vdd15"; + retimer->supplies[3].supply = "vcc"; + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(retimer->supplies), + retimer->supplies); + if (ret) + return ret; + + retimer->xo_clk = devm_clk_get(dev, "xo"); + if (IS_ERR(retimer->xo_clk)) + return PTR_ERR(retimer->xo_clk); + + retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(retimer->reset_gpio)) + return PTR_ERR(retimer->reset_gpio); + + retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode); + if (IS_ERR(retimer->typec_switch)) + return dev_err_probe(dev, PTR_ERR(retimer->typec_switch), + "failed to acquire orientation-switch\n"); + + retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode); + if (IS_ERR(retimer->typec_mux)) { + ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux), + "failed to acquire mode-mux\n"); + goto err_switch_put; + } + + ret = regulator_bulk_enable(ARRAY_SIZE(retimer->supplies), + retimer->supplies); + if (ret < 0) { + dev_err(dev, "cannot enable regulators %d\n", ret); + goto err_mux_put; + } + + ret = clk_prepare_enable(retimer->xo_clk); + if (ret) { + dev_err(dev, "Failed to enable XO: %d\n", ret); + goto err_disable_vreg; + } + + gpiod_set_value(retimer->reset_gpio, 0); + msleep(20); + gpiod_set_value(retimer->reset_gpio, 1); + + msleep(60); + mutex_init(&retimer->lock); + + sw_desc.drvdata = retimer; + sw_desc.fwnode = dev_fwnode(dev); + sw_desc.set = ps8830_sw_set; + + ret = drm_aux_bridge_register(dev); + if (ret) + goto err_disable_gpio; + + retimer->sw = typec_switch_register(dev, &sw_desc); + if (IS_ERR(retimer->sw)) { + ret = dev_err_probe(dev, PTR_ERR(retimer->sw), + "Error registering typec switch\n"); + goto err_disable_gpio; + } + + rtmr_desc.drvdata = retimer; + rtmr_desc.fwnode = dev_fwnode(dev); + rtmr_desc.set = ps8830_retimer_set; + + retimer->retimer = typec_retimer_register(dev, &rtmr_desc); + if (IS_ERR(retimer->retimer)) { + ret = dev_err_probe(dev, PTR_ERR(retimer->retimer), + "Error registering typec retimer\n"); + goto err_switch_unregister; + } + + dev_info(dev, "Registered Parade PS8830 retimer\n"); + return 0; + +err_switch_unregister: + typec_switch_unregister(retimer->sw); + +err_disable_gpio: + gpiod_set_value(retimer->reset_gpio, 0); + clk_disable_unprepare(retimer->xo_clk); + +err_disable_vreg: + regulator_bulk_disable(ARRAY_SIZE(retimer->supplies), + retimer->supplies); +err_mux_put: + typec_mux_put(retimer->typec_mux); + +err_switch_put: + typec_switch_put(retimer->typec_switch); + + return ret; +} + +static void ps8830_retimer_remove(struct i2c_client *client) +{ + struct ps8830_retimer *retimer = i2c_get_clientdata(client); + + typec_retimer_unregister(retimer->retimer); + typec_switch_unregister(retimer->sw); + + gpiod_set_value(retimer->reset_gpio, 0); + + clk_disable_unprepare(retimer->xo_clk); + + regulator_bulk_disable(ARRAY_SIZE(retimer->supplies), + retimer->supplies); + + typec_mux_put(retimer->typec_mux); + typec_switch_put(retimer->typec_switch); +} + +static const struct i2c_device_id ps8830_retimer_table[] = { + { "parade,ps8830" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, ps8830_retimer_table); + +static const struct of_device_id ps8830_retimer_of_table[] = { + { .compatible = "parade,ps8830" }, + { } +}; +MODULE_DEVICE_TABLE(of, ps8830_retimer_of_table); + +static struct i2c_driver ps8830_retimer_driver = { + .driver = { + .name = "ps8830_retimer", + .of_match_table = ps8830_retimer_of_table, + }, + .probe = ps8830_retimer_probe, + .remove = ps8830_retimer_remove, + .id_table = ps8830_retimer_table, +}; + +module_i2c_driver(ps8830_retimer_driver); + +MODULE_DESCRIPTION("Parade PS8830 Type-C Retimer driver"); +MODULE_LICENSE("GPL");
The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C. It provides both altmode and orientation handling. Add a driver with support for the following modes: - DP 4lanes - USB3 - DP 2lanes + USB3 Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/usb/typec/mux/Kconfig | 10 ++ drivers/usb/typec/mux/Makefile | 1 + drivers/usb/typec/mux/ps8830.c | 347 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 358 insertions(+)