| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0a43b5a2-dc2f-402a-8ec7-1d5c88602ffc@google.com>
Date: Tue, 25 Nov 2025 18:32:43 -0800
From: Amit Sunil Dhamne <amitsd@...gle.com>
To: André Draszik <andre.draszik@...aro.org>,
Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Lee Jones <lee@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Badhri Jagan Sridharan <badhri@...gle.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Peter Griffin <peter.griffin@...aro.org>,
Tudor Ambarus <tudor.ambarus@...aro.org>,
Alim Akhtar <alim.akhtar@...sung.com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-usb@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
RD Babiera <rdbabiera@...gle.com>, Kyle Tso <kyletso@...gle.com>
Subject: Re: [PATCH 6/6] usb: typec: tcpm/tcpci_maxim: deprecate WAR for
setting charger mode
On 11/24/25 1:09 AM, André Draszik wrote:
> Hi Amit,
>
> On Sun, 2025-11-23 at 08:35 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>> From: Amit Sunil Dhamne <amitsd@...gle.com>
>>
>> TCPCI maxim driver directly writes to the charger's register space to
>> set charger mode depending on the power role. As MAX77759 chg driver
>> exists, this WAR is not required.
>>
>> Instead, use a regulator interface to set OTG Boost mode.
>>
>> Signed-off-by: Amit Sunil Dhamne <amitsd@...gle.com>
>> ---
>> drivers/usb/typec/tcpm/tcpci_maxim.h | 1 +
>> drivers/usb/typec/tcpm/tcpci_maxim_core.c | 48 +++++++++++++++++++++----------
>> 2 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
>> index b33540a42a95..6c82a61efe46 100644
>> --- a/drivers/usb/typec/tcpm/tcpci_maxim.h
>> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
>> @@ -60,6 +60,7 @@ struct max_tcpci_chip {
>> struct tcpm_port *port;
>> enum contamiant_state contaminant_state;
>> bool veto_vconn_swap;
>> + struct regulator *otg_reg;
>> };
>>
>> static inline int max_tcpci_read16(struct max_tcpci_chip *chip, unsigned int reg, u16 *val)
>> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
>> index 19f638650796..6d819a762fa1 100644
>> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
>> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
>> @@ -10,6 +10,7 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> #include <linux/usb/pd.h>
>> #include <linux/usb/tcpci.h>
>> #include <linux/usb/tcpm.h>
>> @@ -202,32 +203,49 @@ static void process_rx(struct max_tcpci_chip *chip, u16 status)
>> tcpm_pd_receive(chip->port, &msg, rx_type);
>> }
>>
>> +static int get_otg_regulator_handle(struct max_tcpci_chip *chip)
>> +{
>> + if (IS_ERR_OR_NULL(chip->otg_reg)) {
>> + chip->otg_reg = devm_regulator_get_exclusive(chip->dev,
>> + "otg-vbus");
>> + if (IS_ERR_OR_NULL(chip->otg_reg)) {
>> + dev_err(chip->dev,
>> + "Failed to get otg regulator handle");
>> + return -ENODEV;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int max_tcpci_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, bool source, bool sink)
>> {
>> struct max_tcpci_chip *chip = tdata_to_max_tcpci(tdata);
>> - u8 buffer_source[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_SOURCE};
>> - u8 buffer_sink[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_SINK};
>> - u8 buffer_none[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_OFF};
> You should also remove the corresponding #defines at the top of this file.
Ack!
>
>> - struct i2c_client *i2c = chip->client;
>> int ret;
>>
>> - struct i2c_msg msgs[] = {
>> - {
>> - .addr = MAX_BUCK_BOOST_SID,
>> - .flags = i2c->flags & I2C_M_TEN,
>> - .len = 2,
>> - .buf = source ? buffer_source : sink ? buffer_sink : buffer_none,
>> - },
>> - };
>> -
>> if (source && sink) {
>> dev_err(chip->dev, "Both source and sink set\n");
>> return -EINVAL;
>> }
>>
>> - ret = i2c_transfer(i2c->adapter, msgs, 1);
>> + ret = get_otg_regulator_handle(chip);
>> + if (ret) {
>> + /*
>> + * Regulator is not necessary for sink only applications. Return
>> + * success in cases where sink mode is being modified.
>> + */
>> + return source ? ret : 1;
>> + }
>> +
>> + if (source) {
>> + if (!regulator_is_enabled(chip->otg_reg))
>> + ret = regulator_enable(chip->otg_reg);
>> + } else {
>> + if (regulator_is_enabled(chip->otg_reg))
>> + ret = regulator_disable(chip->otg_reg);
>> + }
> Given otg_reg is the fake regulator created by a previous patch in this
> series, this means that the regulator API is really used to configure two
> out of 16 possible charger modes here. That doesn't look right.
I understand the concern, but I believe the regulator framework is the
correct interface here for three reasons:
1. The TCPCI driver's responsibility is only to signal the intent to
source VBUS (OTG) or not. It should not be aware of the specific
register values or mode bits of the companion charger chip. The charger
driver (via the regulator enable op) is responsible for translating that
intent into the correct hardware-specific register value (e.g.,
selecting the correct OTG mode).
2. While the charger supports multiple modes, sourcing VBUS via OTG and
sinking VBUS (charging) are mutually exclusive states on Type-C and
there are no modes to support both. If complex scenarios arise (like
wireless charging + USB OTG), the logic to select the specific register
mode belongs in the charger driver (the regulator provider), not the
TCPCI consumer. Now say theoretically, we support wireless charging and
want to turn it on (sink) while USB OTG mode (source) is on. The charger
driver would set an appropriate mode based on this info (wireless
power-supply on, USB otg regulator on, so select XX mode). We can game
this for any 10 of the *valid* modes (refer [1]) supported by the chip
and this would work. Using a regulator framework here will not pose a
restriction on the number of charger modes now or in the future.
3. This design pattern is standard in the kernel from my study. Drivers
such as bq24190_charger and rt9471 expose USB OTG functionality via the
regulator framework.
[1]
https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max77759_regs.h#52
BR,
Amit
>
> Cheers,
> Andre'
Powered by blists - more mailing lists