| Message ID | 20220307034040.1111107-7-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v4,1/7] device property: Helper to match multiple connections | expand |
On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson 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 Type-C mux and switch devices. This provides the necessary > support for enabling the DisplayPort altmode on devices with this > circuit. ... > +static const struct regmap_config fsa4480_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = FSA4480_MAX_REGISTER, > +}; You are using mutex for accessing hardware. Do you still need a regmap lock? If so, add a comment to explain why. ... > + /* 15us to allow the SBU switch to turn off */ > + usleep_range(15, 1000); This is quite unusual range. If you are fine with the long delay, why to stress the system on it? Otherwise the use of 1000 is unclear. That said, I would expect one of the below: usleep_range(15, 30); usleep_range(500, 1000); ... > + sw_desc.fwnode = dev->fwnode; Please, don't dereference for fwnode explicitly. Use dev_fwnode(). ... > + mux_desc.fwnode = dev->fwnode; Ditto.
On Mon 07 Mar 02:16 PST 2022, Andy Shevchenko wrote: > On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson 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 Type-C mux and switch devices. This provides the necessary > > support for enabling the DisplayPort altmode on devices with this > > circuit. > > ... > > > +static const struct regmap_config fsa4480_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = FSA4480_MAX_REGISTER, > > +}; > > You are using mutex for accessing hardware. Do you still need a regmap lock? > If so, add a comment to explain why. > I've not considered that before, but you're right, there doesn't seem to be any reason to keep the locking in the regmap. > ... > > > + /* 15us to allow the SBU switch to turn off */ > > + usleep_range(15, 1000); > > This is quite unusual range. > > If you are fine with the long delay, why to stress the system on it? > Otherwise the use of 1000 is unclear. > > That said, I would expect one of the below: > > usleep_range(15, 30); > usleep_range(500, 1000); > Glad you asked about that, as you say the typical form is to keep the range within 2x of the lower value, or perhaps lower + 5. But if the purpose is to specify a minimum time and then give a max to give the system some flexibility in it's decision of when to wake up. And in situations such as this, we're talking about someone connecting a cable, so we're in "no rush" and I picked the completely arbitrary 1ms as the max. Do you see any drawback of this much higher number? (Other than it looking "wrong") > ... > > > + sw_desc.fwnode = dev->fwnode; > > Please, don't dereference for fwnode explicitly. Use dev_fwnode(). > Okay, will update accordingly. Thanks, Bjorn > ... > > > + mux_desc.fwnode = dev->fwnode; > > Ditto. > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Mar 07, 2022 at 06:48:25AM -0800, Bjorn Andersson wrote: > On Mon 07 Mar 02:16 PST 2022, Andy Shevchenko wrote: > > On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson wrote: ... > > > + /* 15us to allow the SBU switch to turn off */ > > > + usleep_range(15, 1000); > > > > This is quite unusual range. > > > > If you are fine with the long delay, why to stress the system on it? > > Otherwise the use of 1000 is unclear. > > > > That said, I would expect one of the below: > > > > usleep_range(15, 30); > > usleep_range(500, 1000); > > Glad you asked about that, as you say the typical form is to keep the > range within 2x of the lower value, or perhaps lower + 5. > > But if the purpose is to specify a minimum time and then give a max to > give the system some flexibility in it's decision of when to wake up. > And in situations such as this, we're talking about someone connecting a > cable, so we're in "no rush" and I picked the completely arbitrary 1ms > as the max. > > Do you see any drawback of this much higher number? (Other than it > looking "wrong") I see the drawback of low number. The 1000 makes not much sense to me with the minimum 66x times less. If there is no rush, use some reasonable values, what about usleep_range(100, 1000); ? 10x is way better than 66x.
On Mon 07 Mar 08:13 PST 2022, Andy Shevchenko wrote: > On Mon, Mar 07, 2022 at 06:48:25AM -0800, Bjorn Andersson wrote: > > On Mon 07 Mar 02:16 PST 2022, Andy Shevchenko wrote: > > > On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson wrote: > > ... > > > > > + /* 15us to allow the SBU switch to turn off */ > > > > + usleep_range(15, 1000); > > > > > > This is quite unusual range. > > > > > > If you are fine with the long delay, why to stress the system on it? > > > Otherwise the use of 1000 is unclear. > > > > > > That said, I would expect one of the below: > > > > > > usleep_range(15, 30); > > > usleep_range(500, 1000); > > > > Glad you asked about that, as you say the typical form is to keep the > > range within 2x of the lower value, or perhaps lower + 5. > > > > But if the purpose is to specify a minimum time and then give a max to > > give the system some flexibility in it's decision of when to wake up. > > And in situations such as this, we're talking about someone connecting a > > cable, so we're in "no rush" and I picked the completely arbitrary 1ms > > as the max. > > > > Do you see any drawback of this much higher number? (Other than it > > looking "wrong") > > I see the drawback of low number. 15us is based on the data sheet and if the kernel is ready to serve us after 15us then let's do that. > The 1000 makes not much sense to me with the minimum 66x times less. > If there is no rush, use some reasonable values, > what about > > usleep_range(100, 1000); > > ? 10x is way better than 66x. I don't agree, and in particular putting 100 here because it's 1/10 of the number I just made up doesn't sounds like a good reason. The datasheet says 15us, so that is at least based on something real. In https://www.kernel.org/doc/Documentation/timers/timers-howto.txt I find the following: With the introduction of a range, the scheduler is free to coalesce your wakeup with any other wakeup that may have happened for other reasons, or at the worst case, fire an interrupt for your upper bound. The larger a range you supply, the greater a chance that you will not trigger an interrupt; this should be balanced with what is an acceptable upper bound on delay / performance for your specific code path. Exact tolerances here are very situation specific, thus it is left to the caller to determine a reasonable range. Which to me says that the wider range is perfectly reasonable. In particular 15, 30 (which seems to be quite common) makes the available range to the scheduler unnecessarily narrow. And it's clear that whatever the upper bound it's going to be some arbitrary number, but 1ms should ensure that there are other hrtimer interrupts to piggy back on. Regards, Bjorn
On Mon, Mar 07, 2022 at 01:04:50PM -0800, Bjorn Andersson wrote: > On Mon 07 Mar 08:13 PST 2022, Andy Shevchenko wrote: > > On Mon, Mar 07, 2022 at 06:48:25AM -0800, Bjorn Andersson wrote: > > > On Mon 07 Mar 02:16 PST 2022, Andy Shevchenko wrote: > > > > On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson wrote: ... > > > > > + /* 15us to allow the SBU switch to turn off */ > > > > > + usleep_range(15, 1000); > > > > > > > > This is quite unusual range. > > > > > > > > If you are fine with the long delay, why to stress the system on it? > > > > Otherwise the use of 1000 is unclear. > > > > > > > > That said, I would expect one of the below: > > > > > > > > usleep_range(15, 30); > > > > usleep_range(500, 1000); > > > > > > Glad you asked about that, as you say the typical form is to keep the > > > range within 2x of the lower value, or perhaps lower + 5. > > > > > > But if the purpose is to specify a minimum time and then give a max to > > > give the system some flexibility in it's decision of when to wake up. > > > And in situations such as this, we're talking about someone connecting a > > > cable, so we're in "no rush" and I picked the completely arbitrary 1ms > > > as the max. > > > > > > Do you see any drawback of this much higher number? (Other than it > > > looking "wrong") > > > > I see the drawback of low number. > > 15us is based on the data sheet and if the kernel is ready to serve us > after 15us then let's do that. > > > The 1000 makes not much sense to me with the minimum 66x times less. > > If there is no rush, use some reasonable values, > > what about > > > > usleep_range(100, 1000); > > > > ? 10x is way better than 66x. > > I don't agree, and in particular putting 100 here because it's 1/10 of > the number I just made up doesn't sounds like a good reason. The > datasheet says 15us, so that is at least based on something real. > > In https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > I find the following: > > With the introduction of a range, the scheduler is > free to coalesce your wakeup with any other wakeup > that may have happened for other reasons, or at the > worst case, fire an interrupt for your upper bound. > > The larger a range you supply, the greater a chance > that you will not trigger an interrupt; this should > be balanced with what is an acceptable upper bound on > delay / performance for your specific code path. Exact > tolerances here are very situation specific, thus it > is left to the caller to determine a reasonable range. > > Which to me says that the wider range is perfectly reasonable. In > particular 15, 30 (which seems to be quite common) makes the available > range to the scheduler unnecessarily narrow. > > And it's clear that whatever the upper bound it's going to be some > arbitrary number, but 1ms should ensure that there are other hrtimer > interrupts to piggy back on. Okay, I have grepped for usleep_range(x[x], yyyy) and there are 9 modules use it. A few commit messages call 1000 as "reasonable upper limit".
On Mon 07 Mar 14:13 PST 2022, Andy Shevchenko wrote: > On Mon, Mar 07, 2022 at 01:04:50PM -0800, Bjorn Andersson wrote: > > On Mon 07 Mar 08:13 PST 2022, Andy Shevchenko wrote: > > > On Mon, Mar 07, 2022 at 06:48:25AM -0800, Bjorn Andersson wrote: > > > > On Mon 07 Mar 02:16 PST 2022, Andy Shevchenko wrote: > > > > > On Sun, Mar 06, 2022 at 07:40:40PM -0800, Bjorn Andersson wrote: > > ... > > > > > > > + /* 15us to allow the SBU switch to turn off */ > > > > > > + usleep_range(15, 1000); > > > > > > > > > > This is quite unusual range. > > > > > > > > > > If you are fine with the long delay, why to stress the system on it? > > > > > Otherwise the use of 1000 is unclear. > > > > > > > > > > That said, I would expect one of the below: > > > > > > > > > > usleep_range(15, 30); > > > > > usleep_range(500, 1000); > > > > > > > > Glad you asked about that, as you say the typical form is to keep the > > > > range within 2x of the lower value, or perhaps lower + 5. > > > > > > > > But if the purpose is to specify a minimum time and then give a max to > > > > give the system some flexibility in it's decision of when to wake up. > > > > And in situations such as this, we're talking about someone connecting a > > > > cable, so we're in "no rush" and I picked the completely arbitrary 1ms > > > > as the max. > > > > > > > > Do you see any drawback of this much higher number? (Other than it > > > > looking "wrong") > > > > > > I see the drawback of low number. > > > > 15us is based on the data sheet and if the kernel is ready to serve us > > after 15us then let's do that. > > > > > The 1000 makes not much sense to me with the minimum 66x times less. > > > If there is no rush, use some reasonable values, > > > what about > > > > > > usleep_range(100, 1000); > > > > > > ? 10x is way better than 66x. > > > > I don't agree, and in particular putting 100 here because it's 1/10 of > > the number I just made up doesn't sounds like a good reason. The > > datasheet says 15us, so that is at least based on something real. > > > > In https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > > I find the following: > > > > With the introduction of a range, the scheduler is > > free to coalesce your wakeup with any other wakeup > > that may have happened for other reasons, or at the > > worst case, fire an interrupt for your upper bound. > > > > The larger a range you supply, the greater a chance > > that you will not trigger an interrupt; this should > > be balanced with what is an acceptable upper bound on > > delay / performance for your specific code path. Exact > > tolerances here are very situation specific, thus it > > is left to the caller to determine a reasonable range. > > > > Which to me says that the wider range is perfectly reasonable. In > > particular 15, 30 (which seems to be quite common) makes the available > > range to the scheduler unnecessarily narrow. > > > > And it's clear that whatever the upper bound it's going to be some > > arbitrary number, but 1ms should ensure that there are other hrtimer > > interrupts to piggy back on. > > Okay, I have grepped for usleep_range(x[x], yyyy) and there are 9 modules > use it. A few commit messages call 1000 as "reasonable upper limit". > Right, we usually see a much more narrow range, as you say 2x or perhaps 10x, and this why I said I was glad you asked. I have been wondering about this in a few different cases... Thanks, Bjorn
diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig index edead555835e..5eb2c17d72c1 100644 --- a/drivers/usb/typec/mux/Kconfig +++ b/drivers/usb/typec/mux/Kconfig @@ -2,6 +2,16 @@ menu "USB Type-C Multiplexer/DeMultiplexer Switch support" +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. + If compiled as a module, the module will be named fsa4480. + config TYPEC_MUX_PI3USB30532 tristate "Pericom PI3USB30532 Type-C cross switch driver" depends on I2C diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile index 280a6f553115..e52a56c16bfb 100644 --- a/drivers/usb/typec/mux/Makefile +++ b/drivers/usb/typec/mux/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_TYPEC_MUX_FSA4480) += fsa4480.o obj-$(CONFIG_TYPEC_MUX_PI3USB30532) += pi3usb30532.o obj-$(CONFIG_TYPEC_MUX_INTEL_PMC) += intel_pmc_mux.o diff --git a/drivers/usb/typec/mux/fsa4480.c b/drivers/usb/typec/mux/fsa4480.c new file mode 100644 index 000000000000..ab8d014a6b79 --- /dev/null +++ b/drivers/usb/typec/mux/fsa4480.c @@ -0,0 +1,216 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021-2022 Linaro Ltd. + * Copyright (C) 2018-2020 The Linux Foundation + */ + +#include <linux/bits.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/usb/typec_dp.h> +#include <linux/usb/typec_mux.h> + +#define FSA4480_SWITCH_ENABLE 0x04 +#define FSA4480_SWITCH_SELECT 0x05 +#define FSA4480_SWITCH_STATUS1 0x07 +#define FSA4480_SLOW_L 0x08 +#define FSA4480_SLOW_R 0x09 +#define FSA4480_SLOW_MIC 0x0a +#define FSA4480_SLOW_SENSE 0x0b +#define FSA4480_SLOW_GND 0x0c +#define FSA4480_DELAY_L_R 0x0d +#define FSA4480_DELAY_L_MIC 0x0e +#define FSA4480_DELAY_L_SENSE 0x0f +#define FSA4480_DELAY_L_AGND 0x10 +#define FSA4480_RESET 0x1e +#define FSA4480_MAX_REGISTER 0x1f + +#define FSA4480_ENABLE_DEVICE BIT(7) +#define FSA4480_ENABLE_SBU GENMASK(6, 5) +#define FSA4480_ENABLE_USB GENMASK(4, 3) + +#define FSA4480_SEL_SBU_REVERSE GENMASK(6, 5) +#define FSA4480_SEL_USB GENMASK(4, 3) + +struct fsa4480 { + struct i2c_client *client; + + /* used to serialize concurrent change requests */ + struct mutex lock; + + struct typec_switch_dev *sw; + struct typec_mux_dev *mux; + + struct regmap *regmap; + + u8 cur_enable; + u8 cur_select; +}; + +static const struct regmap_config fsa4480_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = FSA4480_MAX_REGISTER, +}; + +static int fsa4480_switch_set(struct typec_switch_dev *sw, + enum typec_orientation orientation) +{ + struct fsa4480 *fsa = typec_switch_get_drvdata(sw); + u8 new_sel; + + mutex_lock(&fsa->lock); + new_sel = FSA4480_SEL_USB; + if (orientation == TYPEC_ORIENTATION_REVERSE) + new_sel |= FSA4480_SEL_SBU_REVERSE; + + if (new_sel == fsa->cur_select) + goto out_unlock; + + if (fsa->cur_enable & FSA4480_ENABLE_SBU) { + /* Disable SBU output while re-configuring the switch */ + regmap_write(fsa->regmap, FSA4480_SWITCH_ENABLE, + fsa->cur_enable & ~FSA4480_ENABLE_SBU); + + /* 35us to allow the SBU switch to turn off */ + usleep_range(35, 1000); + } + + regmap_write(fsa->regmap, FSA4480_SWITCH_SELECT, new_sel); + fsa->cur_select = new_sel; + + if (fsa->cur_enable & FSA4480_ENABLE_SBU) { + regmap_write(fsa->regmap, FSA4480_SWITCH_ENABLE, fsa->cur_enable); + + /* 15us to allow the SBU switch to turn on again */ + usleep_range(15, 1000); + } + +out_unlock: + mutex_unlock(&fsa->lock); + + return 0; +} + +static int fsa4480_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state) +{ + struct fsa4480 *fsa = typec_mux_get_drvdata(mux); + u8 new_enable; + + mutex_lock(&fsa->lock); + + new_enable = FSA4480_ENABLE_DEVICE | FSA4480_ENABLE_USB; + if (state->mode >= TYPEC_DP_STATE_A) + new_enable |= FSA4480_ENABLE_SBU; + + if (new_enable == fsa->cur_enable) + goto out_unlock; + + regmap_write(fsa->regmap, FSA4480_SWITCH_ENABLE, new_enable); + fsa->cur_enable = new_enable; + + if (new_enable & FSA4480_ENABLE_SBU) { + /* 15us to allow the SBU switch to turn off */ + usleep_range(15, 1000); + } + +out_unlock: + mutex_unlock(&fsa->lock); + + return 0; +} + +static int fsa4480_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct typec_switch_desc sw_desc = { }; + struct typec_mux_desc mux_desc = { }; + struct fsa4480 *fsa; + + fsa = devm_kzalloc(dev, sizeof(*fsa), GFP_KERNEL); + if (!fsa) + return -ENOMEM; + + fsa->client = client; + mutex_init(&fsa->lock); + + fsa->regmap = devm_regmap_init_i2c(client, &fsa4480_regmap_config); + if (IS_ERR(fsa->regmap)) + return dev_err_probe(dev, PTR_ERR(fsa->regmap), "failed to initialize regmap\n"); + + fsa->cur_enable = FSA4480_ENABLE_DEVICE | FSA4480_ENABLE_USB; + fsa->cur_select = FSA4480_SEL_USB; + + /* set default settings */ + regmap_write(fsa->regmap, FSA4480_SLOW_L, 0x00); + regmap_write(fsa->regmap, FSA4480_SLOW_R, 0x00); + regmap_write(fsa->regmap, FSA4480_SLOW_MIC, 0x00); + regmap_write(fsa->regmap, FSA4480_SLOW_SENSE, 0x00); + regmap_write(fsa->regmap, FSA4480_SLOW_GND, 0x00); + regmap_write(fsa->regmap, FSA4480_DELAY_L_R, 0x00); + regmap_write(fsa->regmap, FSA4480_DELAY_L_MIC, 0x00); + regmap_write(fsa->regmap, FSA4480_DELAY_L_SENSE, 0x00); + regmap_write(fsa->regmap, FSA4480_DELAY_L_AGND, 0x09); + regmap_write(fsa->regmap, FSA4480_SWITCH_SELECT, fsa->cur_select); + regmap_write(fsa->regmap, FSA4480_SWITCH_ENABLE, fsa->cur_enable); + + sw_desc.drvdata = fsa; + sw_desc.fwnode = dev->fwnode; + sw_desc.set = fsa4480_switch_set; + + fsa->sw = typec_switch_register(dev, &sw_desc); + if (IS_ERR(fsa->sw)) + return dev_err_probe(dev, PTR_ERR(fsa->sw), "failed to register typec switch\n"); + + mux_desc.drvdata = fsa; + mux_desc.fwnode = dev->fwnode; + mux_desc.set = fsa4480_mux_set; + + fsa->mux = typec_mux_register(dev, &mux_desc); + if (IS_ERR(fsa->mux)) { + typec_switch_unregister(fsa->sw); + return dev_err_probe(dev, PTR_ERR(fsa->mux), "failed to register typec mux\n"); + } + + i2c_set_clientdata(client, fsa); + return 0; +} + +static int fsa4480_remove(struct i2c_client *client) +{ + struct fsa4480 *fsa = i2c_get_clientdata(client); + + typec_mux_unregister(fsa->mux); + typec_switch_unregister(fsa->sw); + + return 0; +} + +static const struct i2c_device_id fsa4480_table[] = { + { "fsa4480" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, fsa4480_table); + +static const struct of_device_id fsa4480_of_table[] = { + { .compatible = "fcs,fsa4480" }, + { } +}; +MODULE_DEVICE_TABLE(of, fsa4480_of_table); + +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, +}; +module_i2c_driver(fsa4480_driver); + +MODULE_DESCRIPTION("ON Semiconductor FSA4480 driver"); +MODULE_LICENSE("GPL v2");
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 Type-C mux and switch devices. This provides the necessary support for enabling the DisplayPort altmode on devices with this circuit. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - None Changes since v2: - Expanded Kconfig help text. - Fixed copyright year - Fixed spelling mistakes in comments - Introduce FSA4480_MAX_REGISTER - Use dev_err_probe() - Removed some useless blank lines. Changes since v1: - None drivers/usb/typec/mux/Kconfig | 10 ++ drivers/usb/typec/mux/Makefile | 1 + drivers/usb/typec/mux/fsa4480.c | 216 ++++++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+) create mode 100644 drivers/usb/typec/mux/fsa4480.c