From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.14]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EFCE5435539; Wed, 5 Aug 2026 11:47:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.14 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785930466; cv=none; b=gmvcHCv41J3kZOk7cSTvAJB2SC4a21BOt5oiGYw+9w2XFqu5q13fWYjiIwbPanrV2da0JDhX9J+SrhjIgBI6L3DyYuqjfi/8hbnMNYIUyJQ5WRNqhlE+pu10D7uQoCOStkcB7Iyb5fvtzpxB87yxqNDQ5LmKzhB+cLhAhG5YL0M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785930466; c=relaxed/simple; bh=GHEQA7zE2Q7m0lCPj2/Gzzf6Dy30RMpWXQ1Kz2fOrjA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=cdqua3FE5ntBg4QMhIpbhJw2jSTbG10ix5RefU8x5YqyrHYCIIRmIACI8r6tTEF7YLF1tvAD1v1F1XwoTQaEYV5FPKs55Qu9z1SH1D2hL50SbVizawl4eEfMwj6WJQDUWvGWh/0jBAbuRJr6uKzy9pnlaZ5RYzt+NFUUqI1PhVk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=la5iqx/S; arc=none smtp.client-ip=198.175.65.14 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="la5iqx/S" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1785930464; x=1817466464; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=GHEQA7zE2Q7m0lCPj2/Gzzf6Dy30RMpWXQ1Kz2fOrjA=; b=la5iqx/SV9vj7JVQPTUnYvZ6/k3Bg08i6d8MwM7T/ax5E+lhWqhgKLpn NfSF/pmGBJwPZin4Y2sexOLPJFVCWBHx9FE1e/8BFngM88yo7iFxWQ4ms 4gaD5P5zUashThpSat5UORiwQ2XT9g8MhscUiGD/Nw+B7mB8OrO5YRBsG bAIK+BLXakBINJmV0aa2ViBADx2jPS+0sxrtBvKMTioFJc2JKnXGJM5ZG sP160YTvqMtjglMeNkQjJTESWiEiycvopEgGy7n8qQ634MYQIgbI6tBJj 4Ha298BUIJPeLa172ZrlBOaRw65YG9PZP+KtkWG0cyuxNbT8XFS5JNW3b w==; X-CSE-ConnectionGUID: 9dvkXQ/5STaAE5LentCqgA== X-CSE-MsgGUID: BSW+p0n5RVqsGPMBE7Klng== X-IronPort-AV: E=McAfee;i="6800,10657,11865"; a="90385036" X-IronPort-AV: E=Sophos;i="6.25,206,1779174000"; d="scan'208";a="90385036" Received: from fmviesa003.fm.intel.com ([10.60.135.143]) by orvoesa106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Aug 2026 04:47:43 -0700 X-CSE-ConnectionGUID: mW0NGWteQ6WLVnKOlmafdQ== X-CSE-MsgGUID: BKtAXhwDT+m59ThNsdWdSQ== X-ExtLoop1: 1 Received: from black.igk.intel.com ([10.91.253.5]) by fmviesa003.fm.intel.com with ESMTP; 05 Aug 2026 04:47:40 -0700 Received: by black.igk.intel.com (Postfix, from userid 1008) id AB16999; Wed, 05 Aug 2026 13:47:38 +0200 (CEST) Date: Wed, 5 Aug 2026 13:47:38 +0200 From: Heikki Krogerus To: Sasha Finkelstein Cc: Sven Peter , Janne Grunau , Neal Gompa , Greg Kroah-Hartman , Rob Herring , Krzysztof Kozlowski , Conor Dooley , asahi@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Alyssa Milburn Subject: Re: [PATCH v2 2/3] usb: typec: tipd: Factor out i2c specifics Message-ID: References: <20260727-tipd-ace3-v2-0-1345957bd635@chaosmail.tech> <20260727-tipd-ace3-v2-2-1345957bd635@chaosmail.tech> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260727-tipd-ace3-v2-2-1345957bd635@chaosmail.tech> Hi, Sorry to keep you waiting. On Mon, Jul 27, 2026 at 06:09:29PM +0200, Sasha Finkelstein wrote: > From: Alyssa Milburn > > Make the core driver more bus-agnostic to prepare for SPMI variants of > the tipd chip > > Signed-off-by: Alyssa Milburn > Signed-off-by: Sasha Finkelstein > --- > drivers/usb/typec/tipd/Kconfig | 8 ++++-- > drivers/usb/typec/tipd/Makefile | 11 ++++--- > drivers/usb/typec/tipd/core.c | 224 ++++++++++++++++++++++------------------------------------------------------------------------------------------------------------------------- > drivers/usb/typec/tipd/i2c.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/usb/typec/tipd/tps6598x.h | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 265 insertions(+), 196 deletions(-) > > diff --git a/drivers/usb/typec/tipd/Kconfig b/drivers/usb/typec/tipd/Kconfig > index b82715293072..c87cbe6109ca 100644 > --- a/drivers/usb/typec/tipd/Kconfig > +++ b/drivers/usb/typec/tipd/Kconfig > @@ -1,9 +1,13 @@ > +config TYPEC_TPS6598X_CORE > + tristate > + select USB_ROLE_SWITCH > + select POWER_SUPPLY > + > config TYPEC_TPS6598X > tristate "TI TPS6598x USB Power Delivery controller driver" > depends on I2C > - select POWER_SUPPLY > select REGMAP_I2C > - select USB_ROLE_SWITCH > + select TYPEC_TPS6598X_CORE > help > Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power > Delivery controller. Looks better now. > diff --git a/drivers/usb/typec/tipd/Makefile b/drivers/usb/typec/tipd/Makefile > index aa439f80a889..c741bf44f52f 100644 > --- a/drivers/usb/typec/tipd/Makefile > +++ b/drivers/usb/typec/tipd/Makefile > @@ -1,6 +1,9 @@ > # SPDX-License-Identifier: GPL-2.0 > -CFLAGS_trace.o := -I$(src) > +CFLAGS_trace.o := -I$(src) > > -obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o > -tps6598x-y := core.o > -tps6598x-$(CONFIG_TRACING) += trace.o > +obj-$(CONFIG_TYPEC_TPS6598X_CORE) += tps6598x-core.o > +tps6598x-core-y := core.o > +tps6598x-core-$(CONFIG_TRACING) += trace.o > + > +obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o > +tps6598x-y := i2c.o > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c > index d5ee0af9058b..aadadfdc6ed1 100644 > --- a/drivers/usb/typec/tipd/core.c > +++ b/drivers/usb/typec/tipd/core.c > @@ -9,7 +9,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -92,31 +91,6 @@ struct tps6598x_rx_identity_reg { > struct usb_pd_identity identity; > } __packed; > > -/* TPS_REG_USB4_STATUS */ > -struct tps6598x_usb4_status_reg { > - u8 mode_status; > - __le32 eudo; > - __le32 unknown; > -} __packed; > - > -/* TPS_REG_DP_SID_STATUS */ > -struct tps6598x_dp_sid_status_reg { > - u8 mode_status; > - __le32 status_tx; > - __le32 status_rx; > - __le32 configure; > - __le32 mode_data; > -} __packed; > - > -/* TPS_REG_INTEL_VID_STATUS */ > -struct tps6598x_intel_vid_status_reg { > - u8 mode_status; > - __le32 attention_vdo; > - __le16 enter_vdo; > - __le16 device_mode; > - __le16 cable_mode; > -} __packed; > - > /* Standard Task return codes */ > #define TPS_TASK_TIMEOUT 1 > #define TPS_TASK_REJECTED 3 > @@ -143,81 +117,6 @@ static const char *const modes[] = { > /* Unrecognized commands will be replaced with "!CMD" */ > #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321) > > -struct tps6598x; > - > -struct tipd_data { > - irq_handler_t irq_handler; > - u64 irq_mask1; > - size_t tps_struct_size; > - void (*remove)(struct tps6598x *tps); > - int (*register_port)(struct tps6598x *tps, struct fwnode_handle *node); > - void (*unregister_port)(struct tps6598x *tps); > - void (*trace_data_status)(u32 status); > - void (*trace_power_status)(u16 status); > - void (*trace_status)(u32 status); > - int (*apply_patch)(struct tps6598x *tps); > - int (*init)(struct tps6598x *tps); > - int (*switch_power_state)(struct tps6598x *tps, u8 target_state); > - bool (*read_data_status)(struct tps6598x *tps); > - int (*reset)(struct tps6598x *tps); > - int (*connect)(struct tps6598x *tps, u32 status); > -}; > - > -struct tps6598x { > - struct device *dev; > - struct regmap *regmap; > - struct mutex lock; /* device lock */ > - u8 i2c_protocol:1; > - > - struct gpio_desc *reset; > - struct typec_port *port; > - struct typec_partner *partner; > - struct usb_pd_identity partner_identity; > - struct usb_role_switch *role_sw; > - struct typec_capability typec_cap; > - > - struct power_supply *psy; > - struct power_supply_desc psy_desc; > - enum power_supply_usb_type usb_type; > - > - int wakeup; > - u32 status; /* status reg */ > - u32 data_status; > - u16 pwr_status; > - struct delayed_work wq_poll; > - > - const struct tipd_data *data; > -}; > - > -struct cd321x_status { > - u32 status; > - u32 pwr_status; > - u32 data_status; > - u32 status_changed; > - struct usb_pd_identity partner_identity; > - struct tps6598x_dp_sid_status_reg dp_sid_status; > - struct tps6598x_intel_vid_status_reg intel_vid_status; > - struct tps6598x_usb4_status_reg usb4_status; > -}; > - > -struct cd321x { > - struct tps6598x tps; > - > - struct tps6598x_dp_sid_status_reg dp_sid_status; > - struct tps6598x_intel_vid_status_reg intel_vid_status; > - struct tps6598x_usb4_status_reg usb4_status; > - > - struct typec_altmode *port_altmode_dp; > - struct typec_altmode *port_altmode_tbt; > - > - struct typec_mux *mux; > - struct typec_mux_state state; > - > - struct cd321x_status update_status; > - struct delayed_work update_work; > - struct usb_pd_identity cur_partner_identity; > -}; > - > static enum power_supply_property tps6598x_psy_props[] = { > POWER_SUPPLY_PROP_USB_TYPE, > POWER_SUPPLY_PROP_ONLINE, > @@ -1087,11 +986,12 @@ static int tps6598x_check_mode(struct tps6598x *tps) > return -ENODEV; > } > > -static const struct regmap_config tps6598x_regmap_config = { > +const struct regmap_config tps6598x_regmap_config = { > .reg_bits = 8, > .val_bits = 8, > .max_register = 0x7F, > }; > +EXPORT_SYMBOL_GPL(tps6598x_regmap_config); Don't export that. Just move it to i2c.c > static int tps6598x_psy_get_online(struct tps6598x *tps, > union power_supply_propval *val) > @@ -1738,27 +1638,13 @@ static void cd321x_remove(struct tps6598x *tps) > cancel_delayed_work_sync(&cd321x->update_work); > } > > -static int tps6598x_probe(struct i2c_client *client) > +int tipd_init(struct tps6598x *tps) > { > - const struct tipd_data *data; > - struct tps6598x *tps; > struct fwnode_handle *fwnode; > u32 status; > u32 vid; > int ret; > > - data = i2c_get_match_data(client); > - if (!data) > - return -EINVAL; > - > - tps = devm_kzalloc(&client->dev, data->tps_struct_size, GFP_KERNEL); > - if (!tps) > - return -ENOMEM; > - > - mutex_init(&tps->lock); > - tps->dev = &client->dev; > - tps->data = data; > - > tps->reset = devm_gpiod_get_optional(tps->dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(tps->reset)) > return dev_err_probe(tps->dev, PTR_ERR(tps->reset), > @@ -1766,23 +1652,12 @@ static int tps6598x_probe(struct i2c_client *client) > if (tps->reset) > msleep(TPS_SETUP_MS); > > - tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config); > - if (IS_ERR(tps->regmap)) > - return PTR_ERR(tps->regmap); > - > if (!device_is_compatible(tps->dev, "ti,tps25750")) { > ret = tps6598x_read32(tps, TPS_REG_VID, &vid); > if (ret < 0 || !vid) > return -ENODEV; > } > > - /* > - * Checking can the adapter handle SMBus protocol. If it can not, the > - * driver needs to take care of block reads separately. > - */ > - if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > - tps->i2c_protocol = true; > - > if (tps->data->switch_power_state) { > ret = tps->data->switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0); > if (ret) > @@ -1816,7 +1691,7 @@ static int tps6598x_probe(struct i2c_client *client) > * with existing DT files, we work around this by deleting any > * fwnode_links to/from this fwnode. > */ > - fwnode = device_get_named_child_node(&client->dev, "connector"); > + fwnode = device_get_named_child_node(tps->dev, "connector"); > if (fwnode) > fw_devlink_purge_absent_suppliers(fwnode); > > @@ -1842,14 +1717,14 @@ static int tps6598x_probe(struct i2c_client *client) > goto err_unregister_port; > ret = tps->data->connect(tps, status); > if (ret) > - dev_err(&client->dev, "failed to register partner\n"); > + dev_err(tps->dev, "failed to register partner\n"); > } > > - if (client->irq) { > - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + if (tps->irq) { > + ret = devm_request_threaded_irq(tps->dev, tps->irq, NULL, > tps->data->irq_handler, > IRQF_SHARED | IRQF_ONESHOT, > - dev_name(&client->dev), tps); > + dev_name(tps->dev), tps); > } else { > dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n"); > INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work); > @@ -1860,13 +1735,12 @@ static int tps6598x_probe(struct i2c_client *client) > if (ret) > goto err_disconnect; > > - i2c_set_clientdata(client, tps); > fwnode_handle_put(fwnode); > > tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source"); > - if (tps->wakeup && client->irq) { > - devm_device_init_wakeup(&client->dev); > - enable_irq_wake(client->irq); > + if (tps->wakeup && tps->irq) { > + devm_device_init_wakeup(tps->dev); > + enable_irq_wake(tps->irq); > } > > return 0; > @@ -1887,15 +1761,14 @@ static int tps6598x_probe(struct i2c_client *client) > > return ret; > } > +EXPORT_SYMBOL_GPL(tipd_init); > > -static void tps6598x_remove(struct i2c_client *client) > +void tipd_remove(struct tps6598x *tps) > { > - struct tps6598x *tps = i2c_get_clientdata(client); > - > - if (!client->irq) > + if (!tps->irq) > cancel_delayed_work_sync(&tps->wq_poll); > else > - devm_free_irq(tps->dev, client->irq, tps); > + devm_free_irq(tps->dev, tps->irq, tps); > > if (tps->data->remove) > tps->data->remove(tps); > @@ -1910,29 +1783,26 @@ static void tps6598x_remove(struct i2c_client *client) > if (tps->reset) > gpiod_set_value_cansleep(tps->reset, 1); > } > +EXPORT_SYMBOL_GPL(tipd_remove); > > -static int __maybe_unused tps6598x_suspend(struct device *dev) > +int tipd_suspend(struct tps6598x *tps) > { > - struct i2c_client *client = to_i2c_client(dev); > - struct tps6598x *tps = i2c_get_clientdata(client); > - > if (tps->wakeup) { > - disable_irq(client->irq); > - enable_irq_wake(client->irq); > + disable_irq(tps->irq); > + enable_irq_wake(tps->irq); > } else if (tps->reset) { > gpiod_set_value_cansleep(tps->reset, 1); > } > > - if (!client->irq) > + if (!tps->irq) > cancel_delayed_work_sync(&tps->wq_poll); > > return 0; > } > +EXPORT_SYMBOL_GPL(tipd_suspend); > > -static int __maybe_unused tps6598x_resume(struct device *dev) > +int tipd_resume(struct tps6598x *tps) > { > - struct i2c_client *client = to_i2c_client(dev); > - struct tps6598x *tps = i2c_get_clientdata(client); > int ret; > > ret = tps6598x_check_mode(tps); > @@ -1946,25 +1816,22 @@ static int __maybe_unused tps6598x_resume(struct device *dev) > } > > if (tps->wakeup) { > - disable_irq_wake(client->irq); > - enable_irq(client->irq); > + disable_irq_wake(tps->irq); > + enable_irq(tps->irq); > } else if (tps->reset) { > gpiod_set_value_cansleep(tps->reset, 0); > msleep(TPS_SETUP_MS); > } > > - if (!client->irq) > + if (!tps->irq) > queue_delayed_work(system_power_efficient_wq, &tps->wq_poll, > msecs_to_jiffies(POLL_INTERVAL)); > > return 0; > } > +EXPORT_SYMBOL_GPL(tipd_resume); > > -static const struct dev_pm_ops tps6598x_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(tps6598x_suspend, tps6598x_resume) > -}; > - > -static const struct tipd_data cd321x_data = { > +const struct tipd_data tipd_cd321x_data = { > .irq_handler = cd321x_interrupt, > .irq_mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE | > APPLE_CD_REG_INT_DATA_STATUS_UPDATE | > @@ -1982,8 +1849,9 @@ static const struct tipd_data cd321x_data = { > .switch_power_state = cd321x_switch_power_state, > .connect = cd321x_connect, > }; > +EXPORT_SYMBOL_GPL(tipd_cd321x_data); There shouldn't be need to export these either. You can add a variant flag member to struct tps6598x for now, and pick the correct data for example from a lookup table. Ideally IMO each variant would be handled in its own file. But that would require a bit more refactoring of course. > -static const struct tipd_data tps6598x_data = { > +const struct tipd_data tipd_tps6598x_data = { > .irq_handler = tps6598x_interrupt, > .irq_mask1 = TPS_REG_INT_POWER_STATUS_UPDATE | > TPS_REG_INT_DATA_STATUS_UPDATE | > @@ -2000,8 +1868,9 @@ static const struct tipd_data tps6598x_data = { > .reset = tps6598x_reset, > .connect = tps6598x_connect, > }; > +EXPORT_SYMBOL_GPL(tipd_tps6598x_data); > > -static const struct tipd_data tps25750_data = { > +const struct tipd_data tipd_tps25750_data = { > .irq_handler = tps25750_interrupt, > .irq_mask1 = TPS_REG_INT_POWER_STATUS_UPDATE | > TPS_REG_INT_DATA_STATUS_UPDATE | > @@ -2018,33 +1887,8 @@ static const struct tipd_data tps25750_data = { > .reset = tps25750_reset, > .connect = tps6598x_connect, > }; > - > -static const struct of_device_id tps6598x_of_match[] = { > - { .compatible = "ti,tps6598x", &tps6598x_data}, > - { .compatible = "apple,cd321x", &cd321x_data}, > - { .compatible = "ti,tps25750", &tps25750_data}, > - {} > -}; > -MODULE_DEVICE_TABLE(of, tps6598x_of_match); > - > -static const struct i2c_device_id tps6598x_id[] = { > - { .name = "tps6598x", .driver_data = (kernel_ulong_t)&tps6598x_data }, > - { } > -}; > -MODULE_DEVICE_TABLE(i2c, tps6598x_id); > - > -static struct i2c_driver tps6598x_i2c_driver = { > - .driver = { > - .name = "tps6598x", > - .pm = &tps6598x_pm_ops, > - .of_match_table = tps6598x_of_match, > - }, > - .probe = tps6598x_probe, > - .remove = tps6598x_remove, > - .id_table = tps6598x_id, > -}; > -module_i2c_driver(tps6598x_i2c_driver); > +EXPORT_SYMBOL_GPL(tipd_tps25750_data); > > MODULE_AUTHOR("Heikki Krogerus "); > -MODULE_LICENSE("GPL v2"); > -MODULE_DESCRIPTION("TI TPS6598x USB Power Delivery Controller Driver"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("TI TPS6598x USB Power Delivery Controller Core Functions"); Thanks, -- heikki