[PATCH v4 0/8] Introduce initial AMD ASF Controller driver support

[PATCH v4 0/8] Introduce initial AMD ASF Controller driver support

[Shyam Sundar S K]

The AMD ASF (Alert Standard Format) function block is essentially an SMBus
controller with built-in ASF functionality. It features two pins SCL1 and
SDA1 that facilitate communication with other SMBus devices. This dual
capability allows the ASF controller to issue generic SMBus packets and
communicate with the DASH controller using MCTP over ASF. Additionally,
the ASF controller supports remote commands defined by the ASF
specification, such as shutdown, reset, power-up, and power-down, without
requiring any software interaction.

The concept is to enable a remote system to communicate with the target
system over the network. The local network controller, such as an Ethernet
MAC, receives remote packets and relays the commands to the FCH
(Fusion Controller Hub) through the ASF. Examples of these commands
include shutdown and reset. Since ASF uses the SMBus protocol, this
controller can be configured as a secondary SMBus controller.

This series of updates focuses on extending the i2c-piix4 driver to
support the ASF driver by exporting several functions from the i2c-piix4
driver, allowing the AMD ASF driver to leverage existing functionalities.
Additionally, this change incorporates core ASF functionality, including
ACPI integration and the implementation of i2c_algorithm callbacks for ASF
operations.

v4:
----
 - Carve out a separate _HID driver for ASF
 - Export i2c_piix4 driver functions as library
 - Make function signature changes within i2c-pixx4 driver
 - Use dev_err_probe() in probe()
 - Address other remarks from Andy.

v3:
----
 - Fix LKP reported issue by adding 'depends on X86'
 - Drop callback when using acpi_dev_get_resources()
 - Address other remarks from Andy on v2.

v2:
----
 - Change function signature from u8 to enum
 - Use default case in switch
 - Use acpi_dev_get_resources() and drop devm_kzalloc() usage
 - Fix LKP reported issues
 - Address other minor remarks from Andy and Andi Shyti

Shyam Sundar S K (8):
  i2c: piix4: Change the signature of piix4_transaction function
  i2c: piix4: Move i2c_piix4 macros and structures to common header
  i2c: piix4: Export i2c_piix4 driver functions as library
  i2c: amd-asf: Add ACPI support for AMD ASF Controller
  i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with
    SMBus
  i2c: amd-asf: Add routine to handle the ASF slave process
  i2c: amd-asf: Clear remote IRR bit to get successive interrupt
  MAINTAINERS: Add AMD ASF driver entry

 MAINTAINERS                           |   8 +-
 drivers/i2c/busses/Kconfig            |  17 ++
 drivers/i2c/busses/Makefile           |   1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 398 ++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-piix4.c        |  54 ++--
 drivers/i2c/busses/i2c-piix4.h        |  45 +++
 6 files changed, 489 insertions(+), 34 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-amd-asf-plat.c
 create mode 100644 drivers/i2c/busses/i2c-piix4.h

-- 
2.25.1

[PATCH v4 1/8] i2c: piix4: Change the signature of piix4_transaction function

[Shyam Sundar S K]

Currently, `piix4_transaction()` accepts only one parameter, which is the
`i2c_adapter` information. This approach works well as long as SB800 SMBus
port accesses are confined to the piix4 driver. However, with the
implementation of a separate ASF driver and the varying address spaces
across drivers, it is necessary to change the function signature of
`piix4_transaction()` to include the port address. This modification
allows other drivers that use piix4 to pass the specific port details they
need to operate on.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 4e32d57ae0bf..69b362db6d0c 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -536,10 +536,8 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
-static int piix4_transaction(struct i2c_adapter *piix4_adapter)
+static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
 {
-	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
-	unsigned short piix4_smba = adapdata->smba;
 	int temp;
 	int result = 0;
 	int timeout = 0;
@@ -675,7 +673,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	status = piix4_transaction(adap);
+	status = piix4_transaction(adap, piix4_smba);
 	if (status)
 		return status;
 
-- 
2.25.1

[PATCH v4 2/8] i2c: piix4: Move i2c_piix4 macros and structures to common header

[Shyam Sundar S K]

Add a separate header file to relocate the common code from the i2c_piix4
driver, allowing the AMD ASF driver to utilize the same code.

Update the MAINTAINERS file to include information about the new common
header file.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS                    |  2 +-
 drivers/i2c/busses/i2c-piix4.c | 24 +-------------------
 drivers/i2c/busses/i2c-piix4.h | 40 ++++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 24 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-piix4.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 10430778c998..3f348b71b9c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10611,7 +10611,7 @@ F:	drivers/i2c/busses/i2c-i801.c
 F:	drivers/i2c/busses/i2c-isch.c
 F:	drivers/i2c/busses/i2c-nforce2-s4985.c
 F:	drivers/i2c/busses/i2c-nforce2.c
-F:	drivers/i2c/busses/i2c-piix4.c
+F:	drivers/i2c/busses/i2c-piix4.*
 F:	drivers/i2c/busses/i2c-sis5595.c
 F:	drivers/i2c/busses/i2c-sis630.c
 F:	drivers/i2c/busses/i2c-sis96x.c
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 69b362db6d0c..2c2a466e2f85 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -35,23 +35,7 @@
 #include <linux/acpi.h>
 #include <linux/io.h>
 
-
-/* PIIX4 SMBus address offsets */
-#define SMBHSTSTS	(0 + piix4_smba)
-#define SMBHSLVSTS	(1 + piix4_smba)
-#define SMBHSTCNT	(2 + piix4_smba)
-#define SMBHSTCMD	(3 + piix4_smba)
-#define SMBHSTADD	(4 + piix4_smba)
-#define SMBHSTDAT0	(5 + piix4_smba)
-#define SMBHSTDAT1	(6 + piix4_smba)
-#define SMBBLKDAT	(7 + piix4_smba)
-#define SMBSLVCNT	(8 + piix4_smba)
-#define SMBSHDWCMD	(9 + piix4_smba)
-#define SMBSLVEVT	(0xA + piix4_smba)
-#define SMBSLVDAT	(0xC + piix4_smba)
-
-/* count for request_region */
-#define SMBIOSIZE	9
+#include "i2c-piix4.h"
 
 /* PCI Address Constants */
 #define SMBBA		0x090
@@ -70,7 +54,6 @@
 #define PIIX4_BYTE		0x04
 #define PIIX4_BYTE_DATA		0x08
 #define PIIX4_WORD_DATA		0x0C
-#define PIIX4_BLOCK_DATA	0x14
 
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS	4
@@ -160,11 +143,6 @@ static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 };
 static const char *piix4_aux_port_name_sb800 = " port 1";
 
-struct sb800_mmio_cfg {
-	void __iomem *addr;
-	bool use_mmio;
-};
-
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
diff --git a/drivers/i2c/busses/i2c-piix4.h b/drivers/i2c/busses/i2c-piix4.h
new file mode 100644
index 000000000000..c4c20edacb00
--- /dev/null
+++ b/drivers/i2c/busses/i2c-piix4.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * PIIX4/SB800 SMBus Interfaces
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *	    Sanket Goswami <Sanket.Goswami@amd.com>
+ */
+
+#ifndef I2C_PIIX4_H
+#define I2C_PIIX4_H
+
+/* PIIX4 SMBus address offsets */
+#define SMBHSTSTS	(0 + piix4_smba)
+#define SMBHSLVSTS	(1 + piix4_smba)
+#define SMBHSTCNT	(2 + piix4_smba)
+#define SMBHSTCMD	(3 + piix4_smba)
+#define SMBHSTADD	(4 + piix4_smba)
+#define SMBHSTDAT0	(5 + piix4_smba)
+#define SMBHSTDAT1	(6 + piix4_smba)
+#define SMBBLKDAT	(7 + piix4_smba)
+#define SMBSLVCNT	(8 + piix4_smba)
+#define SMBSHDWCMD	(9 + piix4_smba)
+#define SMBSLVEVT	(0xA + piix4_smba)
+#define SMBSLVDAT	(0xC + piix4_smba)
+
+/* Count for request_region */
+#define SMBIOSIZE	9
+
+/* PIIX4 constants */
+#define PIIX4_BLOCK_DATA	0x14
+
+struct sb800_mmio_cfg {
+	void __iomem *addr;
+	bool use_mmio;
+};
+
+#endif /* I2C_PIIX4_H */
-- 
2.25.1

[PATCH v4 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Shyam Sundar S K]

Export the following i2c_piix4 driver functions as a library so that the
AMD ASF driver can utilize these core functionalities from the i2c_piix4
driver:

- piix4_sb800_region_request(): Request access to a specific SMBus region
on the SB800 chipset.

- piix4_sb800_region_release(): Release the previously requested SMBus
region on the SB800 chipset.

- piix4_transaction(): Handle SMBus transactions between the SMBus
controller and connected devices.

- piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
chipset.

By making these functions available as a library, enable the AMD ASF
driver to leverage the established mechanisms in the i2c_piix4 driver,
promoting code reuse and consistency across different drivers.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 14 ++++++++------
 drivers/i2c/busses/i2c-piix4.h |  5 +++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 2c2a466e2f85..174cce254e96 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -153,8 +153,7 @@ struct i2c_piix4_adapdata {
 	struct sb800_mmio_cfg mmio_cfg;
 };
 
-static int piix4_sb800_region_request(struct device *dev,
-				      struct sb800_mmio_cfg *mmio_cfg)
+int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
 {
 	if (mmio_cfg->use_mmio) {
 		void __iomem *addr;
@@ -192,9 +191,9 @@ static int piix4_sb800_region_request(struct device *dev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(piix4_sb800_region_request);
 
-static void piix4_sb800_region_release(struct device *dev,
-				       struct sb800_mmio_cfg *mmio_cfg)
+void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
 {
 	if (mmio_cfg->use_mmio) {
 		iounmap(mmio_cfg->addr);
@@ -205,6 +204,7 @@ static void piix4_sb800_region_release(struct device *dev,
 
 	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
 }
+EXPORT_SYMBOL_GPL(piix4_sb800_region_release);
 
 static bool piix4_sb800_use_mmio(struct pci_dev *PIIX4_dev)
 {
@@ -514,7 +514,7 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
-static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
+int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
 {
 	int temp;
 	int result = 0;
@@ -587,6 +587,7 @@ static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short p
 		inb_p(SMBHSTDAT1));
 	return result;
 }
+EXPORT_SYMBOL_GPL(piix4_transaction);
 
 /* Return negative errno on error. */
 static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
@@ -740,7 +741,7 @@ static void piix4_imc_wakeup(void)
 	release_region(KERNCZ_IMC_IDX, 2);
 }
 
-static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
+int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
 {
 	u8 smba_en_lo, val;
 
@@ -762,6 +763,7 @@ static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
 
 	return (smba_en_lo & piix4_port_mask_sb800);
 }
+EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);
 
 /*
  * Handles access to multiple SMBus ports on the SB800.
diff --git a/drivers/i2c/busses/i2c-piix4.h b/drivers/i2c/busses/i2c-piix4.h
index c4c20edacb00..9a5faac3eedd 100644
--- a/drivers/i2c/busses/i2c-piix4.h
+++ b/drivers/i2c/busses/i2c-piix4.h
@@ -37,4 +37,9 @@ struct sb800_mmio_cfg {
 	bool use_mmio;
 };
 
+int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg);
+int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba);
+int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg);
+void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg);
+
 #endif /* I2C_PIIX4_H */
-- 
2.25.1

[PATCH v4 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Shyam Sundar S K]

The AMD ASF controller is presented to the operating system as an ACPI
device. The AMD ASF driver can use ACPI to obtain information about the
ASF controller's attributes, such as the ASF address space and interrupt
number, and to handle ASF interrupts.

Currently, the piix4 driver assumes that a specific port address is
designated for AUX operations. However, with the introduction of ASF, the
same port address may also be used by the ASF controller. Therefore, a
check needs to be added to ensure that if ASF is advertised and enabled in
ACPI, the AUX port should not be configured.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

 drivers/i2c/busses/Kconfig            |  16 ++++
 drivers/i2c/busses/Makefile           |   1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 109 ++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-piix4.c        |  12 ++-
 4 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i2c/busses/i2c-amd-asf-plat.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..262a8193c0bc 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -95,6 +95,22 @@ config I2C_AMD_MP2
 	  This driver can also be built as modules.  If so, the modules will
 	  be called i2c-amd-mp2-pci and i2c-amd-mp2-plat.
 
+config I2C_AMD_ASF
+	tristate "AMD ASF I2C Controller Support"
+	depends on ACPI && I2C_PIIX4
+	help
+	  This option enables support for the AMD ASF (Alert Standard Format)
+	  I2C controller. The AMD ASF controller is an SMBus controller with
+	  built-in ASF functionality, allowing it to issue generic SMBus
+	  packets and communicate with the DASH controller using MCTP over
+	  ASF.
+
+	  If you have an AMD system with ASF support and want to enable this
+	  functionality, say Y or M here. If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called i2c_amd_asf_plat.
+
 config I2C_HIX5HD2
 	tristate "Hix5hd2 high-speed I2C driver"
 	depends on ARCH_HISI || ARCH_HIX5HD2 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..74920380a337 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
 obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
+obj-$(CONFIG_I2C_AMD_ASF)	+= i2c-amd-asf-plat.o
 obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 i2c-at91-objs			:= i2c-at91-core.o i2c-at91-master.o
diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
new file mode 100644
index 000000000000..eb3cd166850c
--- /dev/null
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AMD Alert Standard Format Platform Driver
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *	    Sanket Goswami <Sanket.Goswami@amd.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/i2c-smbus.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "i2c-piix4.h"
+
+static const char *sb800_asf_port_name = " port 1";
+
+struct amd_asf_dev {
+	struct device *dev;
+	struct i2c_adapter adap;
+	struct sb800_mmio_cfg mmio_cfg;
+	unsigned short port_addr;
+};
+
+static int amd_asf_probe(struct platform_device *pdev)
+{
+	struct resource_entry *rentry;
+	struct amd_asf_dev *asf_dev;
+	struct acpi_device *adev;
+	LIST_HEAD(res_list);
+	int ret;
+
+	adev = ACPI_COMPANION(&pdev->dev);
+	if (!adev)
+		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n");
+
+	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
+	if (!asf_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
+
+	asf_dev->dev = &pdev->dev;
+	platform_set_drvdata(pdev, asf_dev);
+
+	asf_dev->adap.owner = THIS_MODULE;
+	asf_dev->mmio_cfg.use_mmio = true;
+	asf_dev->adap.class = I2C_CLASS_HWMON;
+
+	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret);
+
+	list_for_each_entry(rentry, &res_list, node) {
+		switch (resource_type(rentry->res)) {
+		case IORESOURCE_IO:
+			asf_dev->port_addr = rentry->res->start;
+			break;
+		default:
+			dev_warn(&adev->dev, "Invalid ASF resource\n");
+			break;
+		}
+	}
+
+	acpi_dev_free_resource_list(&res_list);
+	/* Set up the sysfs linkage to our parent device */
+	asf_dev->adap.dev.parent = &pdev->dev;
+
+	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
+		 "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr);
+
+	i2c_set_adapdata(&asf_dev->adap, asf_dev);
+	ret = i2c_add_adapter(&asf_dev->adap);
+	if (ret) {
+		release_region(asf_dev->port_addr, SMBIOSIZE);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void amd_asf_remove(struct platform_device *pdev)
+{
+	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
+
+	if (dev->port_addr) {
+		i2c_del_adapter(&dev->adap);
+		release_region(dev->port_addr, SMBIOSIZE);
+	}
+}
+
+static const struct acpi_device_id amd_asf_acpi_ids[] = {
+	{"AMDI001A", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
+
+static struct platform_driver amd_asf_driver = {
+	.driver = {
+		.name = "i2c-amd-asf",
+		.acpi_match_table = amd_asf_acpi_ids,
+	},
+	.probe = amd_asf_probe,
+	.remove_new = amd_asf_remove,
+};
+module_platform_driver(amd_asf_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Alert Standard Format Driver");
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 174cce254e96..ff1e378a7198 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -84,6 +84,7 @@
 
 #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE			8
+#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"
 
 /* insmod parameters */
 
@@ -1021,6 +1022,9 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
 	bool is_sb800 = false;
+	bool is_asf = false;
+	acpi_status status;
+	acpi_handle handle;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
@@ -1083,10 +1087,16 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		}
 	}
 
+	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
+	if (ACPI_SUCCESS(status))
+		is_asf = true;
+
 	if (dev->vendor == PCI_VENDOR_ID_AMD &&
 	    (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
 	     dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)) {
-		retval = piix4_setup_sb800(dev, id, 1);
+		/* Do not setup AUX port if ASF is enabled */
+		if (!is_asf)
+			retval = piix4_setup_sb800(dev, id, 1);
 	}
 
 	if (retval > 0) {
-- 
2.25.1

[PATCH v4 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K]

Implement the i2c_algorithm operations to enable support for AMD ASF
(Alert Standard Format) with SMBus. This enhancement includes:

- Adding functionality to identify and select the supported ASF functions.
- Implementing mechanisms for registering and deregistering I2C slave
  devices.
- Providing support for data transfer operations over ASF.

Additionally, include a 'select' Kconfig entry as the current patch
utilizes reg_slave and unreg_slave callbacks, which are controlled by
IS_ENABLED(CONFIG_I2C_SLAVE).

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/Kconfig            |   1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 189 +++++++++++++++++++++++++-
 2 files changed, 188 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 262a8193c0bc..b8c6428d24ea 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -98,6 +98,7 @@ config I2C_AMD_MP2
 config I2C_AMD_ASF
 	tristate "AMD ASF I2C Controller Support"
 	depends on ACPI && I2C_PIIX4
+	select I2C_SLAVE
 	help
 	  This option enables support for the AMD ASF (Alert Standard Format)
 	  I2C controller. The AMD ASF controller is an SMBus controller with
diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
index eb3cd166850c..7afaf8a9440e 100644
--- a/drivers/i2c/busses/i2c-amd-asf-plat.c
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -15,15 +15,199 @@
 #include <linux/slab.h>
 #include "i2c-piix4.h"
 
-static const char *sb800_asf_port_name = " port 1";
+/* ASF register bits */
+#define ASF_SLV_LISTN	0
+#define ASF_SLV_INTR	1
+#define ASF_SLV_RST	4
+#define ASF_PEC_SP	5
+#define ASF_DATA_EN	7
+#define ASF_MSTR_EN	16
+#define ASF_CLK_EN	17
+
+/* ASF address offsets */
+#define ASFLISADDR	(9 + piix4_smba)
+#define ASFSTA		(0xA + piix4_smba)
+#define ASFSLVSTA	(0xD + piix4_smba)
+#define ASFDATABNKSEL	(0x13 + piix4_smba)
+#define ASFSLVEN	(0x15 + piix4_smba)
+
+#define ASF_BLOCK_MAX_BYTES		72
+
+static const char *amd_asf_port_name = " port 1";
 
 struct amd_asf_dev {
 	struct device *dev;
 	struct i2c_adapter adap;
+	struct i2c_client *target;
 	struct sb800_mmio_cfg mmio_cfg;
 	unsigned short port_addr;
 };
 
+static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
+				unsigned long offset, bool set)
+{
+	unsigned long reg;
+
+	reg = inb_p(offset);
+	if (set)
+		set_bit(bit, &reg);
+	else
+		clear_bit(bit, &reg);
+	outb_p(reg, offset);
+}
+
+static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
+{
+	unsigned long reg;
+
+	reg = ioread32(dev->mmio_cfg.addr);
+	if (set)
+		set_bit(bit, &reg);
+	else
+		clear_bit(bit, &reg);
+	iowrite32(reg, dev->mmio_cfg.addr);
+}
+
+static void amd_asf_setup_target(struct amd_asf_dev *dev)
+{
+	unsigned short piix4_smba = dev->port_addr;
+
+	/* Reset both host and target before setting up */
+	outb_p(0, SMBHSTSTS);
+	outb_p(0, ASFSLVSTA);
+	outb_p(0, ASFSTA);
+
+	/* Update target address */
+	amd_asf_update_bits(piix4_smba, ASF_SLV_LISTN, ASFLISADDR, true);
+	/* Enable target and set the clock */
+	amd_asf_update_bytes(dev, ASF_MSTR_EN, false);
+	amd_asf_update_bytes(dev, ASF_CLK_EN, true);
+	/* Enable target interrupt */
+	amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, ASFSLVEN, true);
+	amd_asf_update_bits(piix4_smba, ASF_SLV_RST, ASFSLVEN, false);
+	/* Enable PEC and PEC append */
+	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, SMBHSTCNT, true);
+	amd_asf_update_bits(piix4_smba, ASF_PEC_SP, SMBHSTCNT, true);
+}
+
+static s32 amd_asf_access(struct i2c_adapter *adap, u16 addr, u8 command, u8 *data)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = dev->port_addr;
+	u8 i, len;
+
+	outb_p((addr << 1), SMBHSTADD);
+	outb_p(command, SMBHSTCMD);
+	len = data[0];
+	if (len == 0 || len > ASF_BLOCK_MAX_BYTES)
+		return -EINVAL;
+
+	outb_p(len, SMBHSTDAT0);
+	/* Reset SMBBLKDAT */
+	inb_p(SMBHSTCNT);
+	for (i = 1; i <= len; i++)
+		outb_p(data[i], SMBBLKDAT);
+
+	outb_p(PIIX4_BLOCK_DATA, SMBHSTCNT);
+	/* Enable PEC and PEC append */
+	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, SMBHSTCNT, true);
+	amd_asf_update_bits(piix4_smba, ASF_PEC_SP, SMBHSTCNT, true);
+
+	return piix4_transaction(adap, piix4_smba);
+}
+
+static int amd_asf_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = dev->port_addr;
+	u8 asf_data[ASF_BLOCK_MAX_BYTES];
+	struct i2c_msg *dev_msgs = msgs;
+	u8 prev_port;
+	int ret;
+
+	if (msgs->flags & I2C_M_RD) {
+		dev_err(&adap->dev, "Read not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	/* Exclude the receive header and PEC */
+	if (msgs->len > ASF_BLOCK_MAX_BYTES - 3) {
+		dev_err(&adap->dev, "ASF max message length exceeded\n");
+		return -EOPNOTSUPP;
+	}
+
+	asf_data[0] = dev_msgs->len;
+	memcpy(asf_data + 1, dev_msgs[0].buf, dev_msgs->len);
+
+	ret = piix4_sb800_region_request(&adap->dev, &dev->mmio_cfg);
+	if (ret)
+		return ret;
+
+	amd_asf_update_bits(piix4_smba, ASF_SLV_RST, ASFSLVEN, true);
+	amd_asf_update_bits(piix4_smba, ASF_SLV_LISTN, ASFLISADDR, false);
+	/* Clear ASF target status */
+	outb_p(0, ASFSLVSTA);
+
+	/* Enable ASF SMBus controller function */
+	amd_asf_update_bytes(dev, ASF_MSTR_EN, true);
+	prev_port = piix4_sb800_port_sel(0, &dev->mmio_cfg);
+	ret = amd_asf_access(adap, msgs->addr, msgs[0].buf[0], asf_data);
+	piix4_sb800_port_sel(prev_port, &dev->mmio_cfg);
+	amd_asf_setup_target(dev);
+	piix4_sb800_region_release(&adap->dev, &dev->mmio_cfg);
+	return ret;
+}
+
+static int amd_asf_reg_target(struct i2c_client *target)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
+	unsigned short piix4_smba = dev->port_addr;
+	int ret;
+	u8 reg;
+
+	if (dev->target)
+		return -EBUSY;
+
+	ret = piix4_sb800_region_request(&target->dev, &dev->mmio_cfg);
+	if (ret)
+		return ret;
+
+	reg = (target->addr << 1) | BIT(0);
+	outb_p(reg, ASFLISADDR);
+
+	amd_asf_setup_target(dev);
+	dev->target = target;
+	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, ASFDATABNKSEL, false);
+	piix4_sb800_region_release(&target->dev, &dev->mmio_cfg);
+
+	return 0;
+}
+
+static int amd_asf_unreg_target(struct i2c_client *target)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
+	unsigned short piix4_smba = dev->port_addr;
+
+	amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, ASFSLVEN, false);
+	amd_asf_update_bits(piix4_smba, ASF_SLV_RST, ASFSLVEN, true);
+	dev->target = NULL;
+
+	return 0;
+}
+
+static u32 amd_asf_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SLAVE | I2C_FUNC_SMBUS_WRITE_BLOCK_DATA | I2C_FUNC_SMBUS_PEC;
+}
+
+static const struct i2c_algorithm amd_asf_smbus_algorithm = {
+	.master_xfer = amd_asf_xfer,
+	.reg_target = amd_asf_reg_target,
+	.unreg_target = amd_asf_unreg_target,
+	.functionality = amd_asf_func,
+};
+
 static int amd_asf_probe(struct platform_device *pdev)
 {
 	struct resource_entry *rentry;
@@ -46,6 +230,7 @@ static int amd_asf_probe(struct platform_device *pdev)
 	asf_dev->adap.owner = THIS_MODULE;
 	asf_dev->mmio_cfg.use_mmio = true;
 	asf_dev->adap.class = I2C_CLASS_HWMON;
+	asf_dev->adap.algo = &amd_asf_smbus_algorithm;
 
 	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
 	if (ret < 0)
@@ -67,7 +252,7 @@ static int amd_asf_probe(struct platform_device *pdev)
 	asf_dev->adap.dev.parent = &pdev->dev;
 
 	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
-		 "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr);
+		 "SMBus ASF adapter%s at %04x", amd_asf_port_name, asf_dev->port_addr);
 
 	i2c_set_adapdata(&asf_dev->adap, asf_dev);
 	ret = i2c_add_adapter(&asf_dev->adap);
-- 
2.25.1

[PATCH v4 6/8] i2c: amd-asf: Add routine to handle the ASF slave process

[Shyam Sundar S K]

Add support for handling ASF slave process events as described in the AMD
ASF databook. This involves implementing the correct programming sequence
to manage each ASF packet appropriately.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-amd-asf-plat.c | 95 ++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
index 7afaf8a9440e..65904fa895dd 100644
--- a/drivers/i2c/busses/i2c-amd-asf-plat.c
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -25,13 +25,17 @@
 #define ASF_CLK_EN	17
 
 /* ASF address offsets */
+#define ASFINDEX	(7 + piix4_smba)
 #define ASFLISADDR	(9 + piix4_smba)
 #define ASFSTA		(0xA + piix4_smba)
 #define ASFSLVSTA	(0xD + piix4_smba)
+#define ASFDATARWPTR	(0x11 + piix4_smba)
+#define ASFSETDATARDPTR	(0x12 + piix4_smba)
 #define ASFDATABNKSEL	(0x13 + piix4_smba)
 #define ASFSLVEN	(0x15 + piix4_smba)
 
 #define ASF_BLOCK_MAX_BYTES		72
+#define SB800_ASF_ERROR_STATUS		0xE
 
 static const char *amd_asf_port_name = " port 1";
 
@@ -39,10 +43,71 @@ struct amd_asf_dev {
 	struct device *dev;
 	struct i2c_adapter adap;
 	struct i2c_client *target;
+	struct delayed_work work_buf;
 	struct sb800_mmio_cfg mmio_cfg;
 	unsigned short port_addr;
 };
 
+static void amd_asf_process_target(struct work_struct *work)
+{
+	struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work);
+	unsigned short piix4_smba = dev->port_addr;
+	u8 data[ASF_BLOCK_MAX_BYTES];
+	u8 len, idx, val = 0;
+	u8 bank, reg, cmd;
+
+	/* Read target status register */
+	reg = inb_p(ASFSLVSTA);
+
+	/* Check if no error bits are set in target status register */
+	if (reg & SB800_ASF_ERROR_STATUS) {
+		/* Set bank as full */
+		cmd = 0;
+		reg = reg | GENMASK(3, 2);
+		outb_p(reg, ASFDATABNKSEL);
+	} else {
+		/* Read data bank */
+		reg = inb_p(ASFDATABNKSEL);
+		bank = (reg & BIT(3)) ? 1 : 0;
+
+		/* Set read data bank */
+		if (bank) {
+			reg = reg | BIT(4);
+			reg = reg & ~BIT(3);
+		} else {
+			reg = reg & ~BIT(4);
+			reg = reg & ~BIT(2);
+		}
+
+		/* Read command register */
+		outb_p(reg, ASFDATABNKSEL);
+		cmd = inb_p(ASFINDEX);
+		len = inb_p(ASFDATARWPTR);
+		for (idx = 0; idx < len; idx++)
+			data[idx] = inb_p(ASFINDEX);
+
+		/* Clear data bank status */
+		if (bank) {
+			reg = reg | BIT(3);
+			outb_p(reg, ASFDATABNKSEL);
+		} else {
+			reg = reg | BIT(2);
+			outb_p(reg, ASFDATABNKSEL);
+		}
+	}
+
+	outb_p(0, ASFSETDATARDPTR);
+	if (cmd & BIT(0))
+		return;
+
+	i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
+	for (idx = 0; idx < len; idx++) {
+		val = data[idx];
+		i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
+	}
+	i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val);
+}
+
 static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
 				unsigned long offset, bool set)
 {
@@ -208,13 +273,31 @@ static const struct i2c_algorithm amd_asf_smbus_algorithm = {
 	.functionality = amd_asf_func,
 };
 
+static irqreturn_t amd_asf_irq_handler(int irq, void *ptr)
+{
+	struct amd_asf_dev *dev = ptr;
+	unsigned short piix4_smba = dev->port_addr;
+	u8 target_int = inb_p(ASFSTA);
+
+	if (target_int & BIT(6)) {
+		/* Target Interrupt */
+		outb_p(target_int | BIT(6), ASFSTA);
+		schedule_delayed_work(&dev->work_buf, HZ);
+	} else {
+		/* Controller Interrupt */
+		amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, SMBHSTSTS, true);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int amd_asf_probe(struct platform_device *pdev)
 {
 	struct resource_entry *rentry;
 	struct amd_asf_dev *asf_dev;
 	struct acpi_device *adev;
 	LIST_HEAD(res_list);
-	int ret;
+	int ret, irq;
 
 	adev = ACPI_COMPANION(&pdev->dev);
 	if (!adev)
@@ -241,6 +324,9 @@ static int amd_asf_probe(struct platform_device *pdev)
 		case IORESOURCE_IO:
 			asf_dev->port_addr = rentry->res->start;
 			break;
+		case IORESOURCE_IRQ:
+			irq = rentry->res->start;
+			break;
 		default:
 			dev_warn(&adev->dev, "Invalid ASF resource\n");
 			break;
@@ -254,6 +340,12 @@ static int amd_asf_probe(struct platform_device *pdev)
 	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
 		 "SMBus ASF adapter%s at %04x", amd_asf_port_name, asf_dev->port_addr);
 
+	INIT_DELAYED_WORK(&asf_dev->work_buf, amd_asf_process_target);
+	ret = devm_request_irq(&pdev->dev, irq, amd_asf_irq_handler, IRQF_SHARED,
+			       "amd_smbus_asf", asf_dev);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Unable to request irq: %d for use\n", irq);
+
 	i2c_set_adapdata(&asf_dev->adap, asf_dev);
 	ret = i2c_add_adapter(&asf_dev->adap);
 	if (ret) {
@@ -267,6 +359,7 @@ static int amd_asf_probe(struct platform_device *pdev)
 static void amd_asf_remove(struct platform_device *pdev)
 {
 	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
+	cancel_delayed_work_sync(&dev->work_buf);
 
 	if (dev->port_addr) {
 		i2c_del_adapter(&dev->adap);
-- 
2.25.1

[PATCH v4 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Shyam Sundar S K]

To ensure successive interrupts upon packet reception, it is necessary to
clear the remote IRR bit by writing the interrupt number to the EOI
register. The base address for this operation is provided by the BIOS and
retrieved by the driver by traversing the ASF object's namespace.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-amd-asf-plat.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
index 65904fa895dd..c1b1846dd2b0 100644
--- a/drivers/i2c/busses/i2c-amd-asf-plat.c
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -40,6 +40,7 @@
 static const char *amd_asf_port_name = " port 1";
 
 struct amd_asf_dev {
+	void __iomem *eoi_base;
 	struct device *dev;
 	struct i2c_adapter adap;
 	struct i2c_client *target;
@@ -288,11 +289,13 @@ static irqreturn_t amd_asf_irq_handler(int irq, void *ptr)
 		amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, SMBHSTSTS, true);
 	}
 
+	iowrite32(irq, dev->eoi_base);
 	return IRQ_HANDLED;
 }
 
 static int amd_asf_probe(struct platform_device *pdev)
 {
+	resource_size_t eoi_addr, eoi_sz;
 	struct resource_entry *rentry;
 	struct amd_asf_dev *asf_dev;
 	struct acpi_device *adev;
@@ -327,6 +330,10 @@ static int amd_asf_probe(struct platform_device *pdev)
 		case IORESOURCE_IRQ:
 			irq = rentry->res->start;
 			break;
+		case IORESOURCE_MEM:
+			eoi_addr = rentry->res->start;
+			eoi_sz = resource_size(rentry->res);
+			break;
 		default:
 			dev_warn(&adev->dev, "Invalid ASF resource\n");
 			break;
@@ -346,6 +353,10 @@ static int amd_asf_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "Unable to request irq: %d for use\n", irq);
 
+	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr, eoi_sz);
+	if (!asf_dev->eoi_base)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "Invalid remapped address\n");
+
 	i2c_set_adapdata(&asf_dev->adap, asf_dev);
 	ret = i2c_add_adapter(&asf_dev->adap);
 	if (ret) {
-- 
2.25.1

[PATCH v4 8/8] MAINTAINERS: Add AMD ASF driver entry

[Shyam Sundar S K]

Update the MAINTAINERS file with AMD ASF driver details.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3f348b71b9c9..18b8975dd058 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1082,6 +1082,12 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/busses/i2c-amd-mp2*
 
+AMD ASF I2C DRIVER
+M:	Shyam Sundar S K <shyam-sundar.s-k@amd.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	drivers/i2c/busses/i2c-amd-asf-plat.c
+
 AMD PDS CORE DRIVER
 M:	Shannon Nelson <shannon.nelson@amd.com>
 M:	Brett Creeley <brett.creeley@amd.com>
-- 
2.25.1

Re: [PATCH v4 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 05:24:03PM +0530, Shyam Sundar S K wrote:
> The AMD ASF controller is presented to the operating system as an ACPI
> device. The AMD ASF driver can use ACPI to obtain information about the
> ASF controller's attributes, such as the ASF address space and interrupt
> number, and to handle ASF interrupts.
> 
> Currently, the piix4 driver assumes that a specific port address is
> designated for AUX operations. However, with the introduction of ASF, the
> same port address may also be used by the ASF controller. Therefore, a
> check needs to be added to ensure that if ASF is advertised and enabled in
> ACPI, the AUX port should not be configured.

With brief look this is much better than the previous version(s).
Thank you for rewriting it this way!

Some comments below.

...

> +#include <linux/acpi.h>

No need (see below)

+ device.h
+ errno.h
+ gfp_types.h

> +#include <linux/i2c-smbus.h>

This should be i2c.h

+ mod_devicetable.h
+ module.h

> +#include <linux/platform_device.h>

> +#include <linux/slab.h>

Not in use.

+ sprintf.h

> +#include "i2c-piix4.h"
> +
> +static const char *sb800_asf_port_name = " port 1";
> +
> +struct amd_asf_dev {
> +	struct device *dev;
> +	struct i2c_adapter adap;

Make it first member, it might help if we ever do a container_of() against
this.

> +	struct sb800_mmio_cfg mmio_cfg;
> +	unsigned short port_addr;

What you probably want is to have

	void __iomem *addr;

and use devm_ioport_map() somewhere (see
drivers/pinctl/intel/pinctrl-lynxpoint.c, for example)

> +};

> +static int amd_asf_probe(struct platform_device *pdev)
> +{
> +	struct resource_entry *rentry;
> +	struct amd_asf_dev *asf_dev;
> +	struct acpi_device *adev;
> +	LIST_HEAD(res_list);
> +	int ret;

> +	adev = ACPI_COMPANION(&pdev->dev);
> +	if (!adev)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n");

No need. You will get here only if enumerated via ACPI (or if it's out-of-tree
board file which we do not care about at all).

> +	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
> +	if (!asf_dev)
> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
> +
> +	asf_dev->dev = &pdev->dev;

> +	platform_set_drvdata(pdev, asf_dev);

Is it used?

> +	asf_dev->adap.owner = THIS_MODULE;
> +	asf_dev->mmio_cfg.use_mmio = true;
> +	asf_dev->adap.class = I2C_CLASS_HWMON;

> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret);
> +
> +	list_for_each_entry(rentry, &res_list, node) {
> +		switch (resource_type(rentry->res)) {
> +		case IORESOURCE_IO:
> +			asf_dev->port_addr = rentry->res->start;
> +			break;
> +		default:
> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
> +			break;
> +		}
> +	}
> +
> +	acpi_dev_free_resource_list(&res_list);

Now this is a duplicate of what ACPI glue layer does. You have these already
available as platform device resources.

> +	/* Set up the sysfs linkage to our parent device */
> +	asf_dev->adap.dev.parent = &pdev->dev;
> +
> +	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
> +		 "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr);

> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);

Is it used?

> +	ret = i2c_add_adapter(&asf_dev->adap);

Use devm variant of this casll.

> +	if (ret) {

> +		release_region(asf_dev->port_addr, SMBIOSIZE);

Why?

> +		return ret;
> +	}
> +
> +	return 0;

	return devm_i2c_add_adapter(...);

> +}
> +
> +static void amd_asf_remove(struct platform_device *pdev)
> +{
> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);

> +	if (dev->port_addr) {

Redundant.

> +		i2c_del_adapter(&dev->adap);

With devm this should be removed.

> +		release_region(dev->port_addr, SMBIOSIZE);

Why?

> +	}
> +}
> +
> +static const struct acpi_device_id amd_asf_acpi_ids[] = {
> +	{"AMDI001A", 0},

	{ "AMDI001A" },

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
> +
> +static struct platform_driver amd_asf_driver = {
> +	.driver = {
> +		.name = "i2c-amd-asf",
> +		.acpi_match_table = amd_asf_acpi_ids,
> +	},
> +	.probe = amd_asf_probe,
> +	.remove_new = amd_asf_remove,
> +};
> +module_platform_driver(amd_asf_driver);

...

> +	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);

Does it compile with CONFIG_ACPI=n?

Also don't you need to include acpi.h for this? Or is it already there?
(I haven't checked).

> +	if (ACPI_SUCCESS(status))
> +		is_asf = true;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Shyam Sundar S K]

On 9/11/2024 20:46, Andy Shevchenko wrote:
> On Wed, Sep 11, 2024 at 05:24:03PM +0530, Shyam Sundar S K wrote:
>> The AMD ASF controller is presented to the operating system as an ACPI
>> device. The AMD ASF driver can use ACPI to obtain information about the
>> ASF controller's attributes, such as the ASF address space and interrupt
>> number, and to handle ASF interrupts.
>>
>> Currently, the piix4 driver assumes that a specific port address is
>> designated for AUX operations. However, with the introduction of ASF, the
>> same port address may also be used by the ASF controller. Therefore, a
>> check needs to be added to ensure that if ASF is advertised and enabled in
>> ACPI, the AUX port should not be configured.
> 
> With brief look this is much better than the previous version(s).
> Thank you for rewriting it this way!
> 
> Some comments below.
> 
> ...
> 
>> +#include <linux/acpi.h>
> 
> No need (see below)
> 
> + device.h
> + errno.h
> + gfp_types.h
> 
>> +#include <linux/i2c-smbus.h>
> 
> This should be i2c.h
> 
> + mod_devicetable.h
> + module.h
> 
>> +#include <linux/platform_device.h>
> 
>> +#include <linux/slab.h>
> 
> Not in use.
> 
> + sprintf.h
> 
>> +#include "i2c-piix4.h"
>> +
>> +static const char *sb800_asf_port_name = " port 1";
>> +
>> +struct amd_asf_dev {
>> +	struct device *dev;
>> +	struct i2c_adapter adap;
> 
> Make it first member, it might help if we ever do a container_of() against
> this.
> 
>> +	struct sb800_mmio_cfg mmio_cfg;
>> +	unsigned short port_addr;
> 
> What you probably want is to have
> 
> 	void __iomem *addr;
> 

I will address the above remarks in the next patch.

I believe this should remain "unsigned short" because

- it is defined a unsigned short in i2c_piix4
- this is just a port address (like 0xb00, 0xb20) and not a real iomem
address.


> and use devm_ioport_map() somewhere (see
> drivers/pinctl/intel/pinctrl-lynxpoint.c, for example)
> 
>> +};
> 
>> +static int amd_asf_probe(struct platform_device *pdev)
>> +{
>> +	struct resource_entry *rentry;
>> +	struct amd_asf_dev *asf_dev;
>> +	struct acpi_device *adev;
>> +	LIST_HEAD(res_list);
>> +	int ret;
> 
>> +	adev = ACPI_COMPANION(&pdev->dev);
>> +	if (!adev)
>> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n");
> 
> No need. You will get here only if enumerated via ACPI (or if it's out-of-tree
> board file which we do not care about at all).

Not sure if I understand your comment correctly. But I used
ACPI_COMPANION to retrieve the acpi device that needs to be passed to
acpi_dev_get_resources(struct acpi_device *, ...) to address your
previous remarks.

> 
>> +	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
>> +	if (!asf_dev)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
>> +
>> +	asf_dev->dev = &pdev->dev;
> 
>> +	platform_set_drvdata(pdev, asf_dev);
> 
> Is it used?
> 
>> +	asf_dev->adap.owner = THIS_MODULE;
>> +	asf_dev->mmio_cfg.use_mmio = true;
>> +	asf_dev->adap.class = I2C_CLASS_HWMON;
> 
>> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret);
>> +
>> +	list_for_each_entry(rentry, &res_list, node) {
>> +		switch (resource_type(rentry->res)) {
>> +		case IORESOURCE_IO:
>> +			asf_dev->port_addr = rentry->res->start;
>> +			break;
>> +		default:
>> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	acpi_dev_free_resource_list(&res_list);
> 
> Now this is a duplicate of what ACPI glue layer does. You have these already
> available as platform device resources.

looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions
that the caller should call acpi_dev_free_resource_list(). Is that not
the case?

> 
>> +	/* Set up the sysfs linkage to our parent device */
>> +	asf_dev->adap.dev.parent = &pdev->dev;
>> +
>> +	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
>> +		 "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr);
> 
>> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
> 
> Is it used?

Yes, in the subsequent patches.

> 
>> +	ret = i2c_add_adapter(&asf_dev->adap);
> 
> Use devm variant of this casll.
> 
>> +	if (ret) {
> 
>> +		release_region(asf_dev->port_addr, SMBIOSIZE);
> 
> Why?
> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
> 
> 	return devm_i2c_add_adapter(...);
> 
>> +}
>> +
>> +static void amd_asf_remove(struct platform_device *pdev)
>> +{
>> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
> 
>> +	if (dev->port_addr) {
> 
> Redundant.
> 
>> +		i2c_del_adapter(&dev->adap);
> 
> With devm this should be removed.
> 
>> +		release_region(dev->port_addr, SMBIOSIZE);
> 
> Why?

My bad :-( Will remove it.

> 
>> +	}
>> +}
>> +
>> +static const struct acpi_device_id amd_asf_acpi_ids[] = {
>> +	{"AMDI001A", 0},
> 
> 	{ "AMDI001A" },
> 
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
>> +
>> +static struct platform_driver amd_asf_driver = {
>> +	.driver = {
>> +		.name = "i2c-amd-asf",
>> +		.acpi_match_table = amd_asf_acpi_ids,
>> +	},
>> +	.probe = amd_asf_probe,
>> +	.remove_new = amd_asf_remove,
>> +};
>> +module_platform_driver(amd_asf_driver);
> 
> ...
> 
>> +	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
> 
> Does it compile with CONFIG_ACPI=n?

I have used a explicit 'depends on' ACPI to this driver soon that LKP
does not complain with a randconfig.

> 
> Also don't you need to include acpi.h for this? Or is it already there?
> (I haven't checked).

acpi_get_handle() is defined in acpi.h.

please assume the rest of the unanswered remarks as "I agree" :-)

Thanks,
Shyam

> 
>> +	if (ACPI_SUCCESS(status))
>> +		is_asf = true;
> 

Re: [PATCH v4 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 10:07:42PM +0530, Shyam Sundar S K wrote:
> On 9/11/2024 20:46, Andy Shevchenko wrote:
> > On Wed, Sep 11, 2024 at 05:24:03PM +0530, Shyam Sundar S K wrote:

...

> >> +	struct i2c_adapter adap;
> > 
> > Make it first member, it might help if we ever do a container_of() against
> > this.
> > 
> >> +	struct sb800_mmio_cfg mmio_cfg;
> >> +	unsigned short port_addr;
> > 
> > What you probably want is to have
> > 
> > 	void __iomem *addr;
> 
> I will address the above remarks in the next patch.
> 
> I believe this should remain "unsigned short" because
> 
> - it is defined a unsigned short in i2c_piix4
> - this is just a port address (like 0xb00, 0xb20) and not a real iomem
> address.

It all depends on how you use that. The devm_ioport_map() is just a trick (in
combination with ioreadXX()/iowriteXX() APIs) to have it in "mapped" address
space.

> > and use devm_ioport_map() somewhere (see
> > drivers/pinctl/intel/pinctrl-lynxpoint.c, for example)

...

> >> +	LIST_HEAD(res_list);

> >> +	adev = ACPI_COMPANION(&pdev->dev);
> >> +	if (!adev)
> >> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n");
> > 
> > No need. You will get here only if enumerated via ACPI (or if it's out-of-tree
> > board file which we do not care about at all).
> 
> Not sure if I understand your comment correctly. But I used
> ACPI_COMPANION to retrieve the acpi device that needs to be passed to
> acpi_dev_get_resources(struct acpi_device *, ...) to address your
> previous remarks.

With platform device driver you don't need all this as you are repeating what
drivers/acpi/acpi_platform.c already does it for all ACPI devices.

> >> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
> >> +	if (ret < 0)
> >> +		return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret);
> >> +
> >> +	list_for_each_entry(rentry, &res_list, node) {
> >> +		switch (resource_type(rentry->res)) {
> >> +		case IORESOURCE_IO:
> >> +			asf_dev->port_addr = rentry->res->start;
> >> +			break;
> >> +		default:
> >> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	acpi_dev_free_resource_list(&res_list);
> > 
> > Now this is a duplicate of what ACPI glue layer does. You have these already
> > available as platform device resources.
> 
> looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions
> that the caller should call acpi_dev_free_resource_list(). Is that not
> the case?

I meant that entire block is just a dup of the existing code. See above.
Instead use simple platform_get_resource() and similar to retrieve this
information.

...

> >> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
> > 
> > Is it used?
> 
> Yes, in the subsequent patches.

I wasn't Cc'ed on the series. Please, make sure you Cc me in the entire series.

...

> >> +	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
> > 
> > Does it compile with CONFIG_ACPI=n?
> 
> I have used a explicit 'depends on' ACPI to this driver soon that LKP
> does not complain with a randconfig.
> 
> > Also don't you need to include acpi.h for this? Or is it already there?
> > (I haven't checked).
> 
> acpi_get_handle() is defined in acpi.h.

Yes, and I meant piix4 driver where you call this API.

> please assume the rest of the unanswered remarks as "I agree" :-)

Noted!

> >> +	if (ACPI_SUCCESS(status))
> >> +		is_asf = true;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Shyam Sundar S K]

On 9/11/2024 22:07, Shyam Sundar S K wrote:
> 
> 
> On 9/11/2024 20:46, Andy Shevchenko wrote:
>> On Wed, Sep 11, 2024 at 05:24:03PM +0530, Shyam Sundar S K wrote:
>>> The AMD ASF controller is presented to the operating system as an ACPI
>>> device. The AMD ASF driver can use ACPI to obtain information about the
>>> ASF controller's attributes, such as the ASF address space and interrupt
>>> number, and to handle ASF interrupts.
>>>
>>> Currently, the piix4 driver assumes that a specific port address is
>>> designated for AUX operations. However, with the introduction of ASF, the
>>> same port address may also be used by the ASF controller. Therefore, a
>>> check needs to be added to ensure that if ASF is advertised and enabled in
>>> ACPI, the AUX port should not be configured.
>>
>> With brief look this is much better than the previous version(s).
>> Thank you for rewriting it this way!
>>
>> Some comments below.
>>
>> ...
>>
>>> +#include <linux/acpi.h>
>>
>> No need (see below)
>>
>> + device.h
>> + errno.h
>> + gfp_types.h
>>
>>> +#include <linux/i2c-smbus.h>
>>
>> This should be i2c.h
>>
>> + mod_devicetable.h
>> + module.h
>>
>>> +#include <linux/platform_device.h>
>>
>>> +#include <linux/slab.h>
>>
>> Not in use.
>>
>> + sprintf.h
>>
>>> +#include "i2c-piix4.h"
>>> +
>>> +static const char *sb800_asf_port_name = " port 1";
>>> +
>>> +struct amd_asf_dev {
>>> +	struct device *dev;
>>> +	struct i2c_adapter adap;
>>
>> Make it first member, it might help if we ever do a container_of() against
>> this.
>>
>>> +	struct sb800_mmio_cfg mmio_cfg;
>>> +	unsigned short port_addr;
>>
>> What you probably want is to have
>>
>> 	void __iomem *addr;
>>
> 
> I will address the above remarks in the next patch.
> 
> I believe this should remain "unsigned short" because
> 
> - it is defined a unsigned short in i2c_piix4
> - this is just a port address (like 0xb00, 0xb20) and not a real iomem
> address.
> 
> 
>> and use devm_ioport_map() somewhere (see
>> drivers/pinctl/intel/pinctrl-lynxpoint.c, for example)
>>
>>> +};
>>
>>> +static int amd_asf_probe(struct platform_device *pdev)
>>> +{
>>> +	struct resource_entry *rentry;
>>> +	struct amd_asf_dev *asf_dev;
>>> +	struct acpi_device *adev;
>>> +	LIST_HEAD(res_list);
>>> +	int ret;
>>
>>> +	adev = ACPI_COMPANION(&pdev->dev);
>>> +	if (!adev)
>>> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n");
>>
>> No need. You will get here only if enumerated via ACPI (or if it's out-of-tree
>> board file which we do not care about at all).
> 
> Not sure if I understand your comment correctly. But I used
> ACPI_COMPANION to retrieve the acpi device that needs to be passed to
> acpi_dev_get_resources(struct acpi_device *, ...) to address your
> previous remarks.
> 
>>
>>> +	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
>>> +	if (!asf_dev)
>>> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
>>> +
>>> +	asf_dev->dev = &pdev->dev;
>>
>>> +	platform_set_drvdata(pdev, asf_dev);
>>
>> Is it used?
>>
>>> +	asf_dev->adap.owner = THIS_MODULE;
>>> +	asf_dev->mmio_cfg.use_mmio = true;
>>> +	asf_dev->adap.class = I2C_CLASS_HWMON;
>>
>>> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
>>> +	if (ret < 0)
>>> +		return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret);
>>> +
>>> +	list_for_each_entry(rentry, &res_list, node) {
>>> +		switch (resource_type(rentry->res)) {
>>> +		case IORESOURCE_IO:
>>> +			asf_dev->port_addr = rentry->res->start;
>>> +			break;
>>> +		default:
>>> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	acpi_dev_free_resource_list(&res_list);
>>
>> Now this is a duplicate of what ACPI glue layer does. You have these already
>> available as platform device resources.
> 
> looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions
> that the caller should call acpi_dev_free_resource_list(). Is that not
> the case?

Ignore this. I understand what you mean now..

Thanks,
Shyam

> 
>>
>>> +	/* Set up the sysfs linkage to our parent device */
>>> +	asf_dev->adap.dev.parent = &pdev->dev;
>>> +
>>> +	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
>>> +		 "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr);
>>
>>> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
>>
>> Is it used?
> 
> Yes, in the subsequent patches.
> 
>>
>>> +	ret = i2c_add_adapter(&asf_dev->adap);
>>
>> Use devm variant of this casll.
>>
>>> +	if (ret) {
>>
>>> +		release_region(asf_dev->port_addr, SMBIOSIZE);
>>
>> Why?
>>
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>
>> 	return devm_i2c_add_adapter(...);
>>
>>> +}
>>> +
>>> +static void amd_asf_remove(struct platform_device *pdev)
>>> +{
>>> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
>>
>>> +	if (dev->port_addr) {
>>
>> Redundant.
>>
>>> +		i2c_del_adapter(&dev->adap);
>>
>> With devm this should be removed.
>>
>>> +		release_region(dev->port_addr, SMBIOSIZE);
>>
>> Why?
> 
> My bad :-( Will remove it.
> 
>>
>>> +	}
>>> +}
>>> +
>>> +static const struct acpi_device_id amd_asf_acpi_ids[] = {
>>> +	{"AMDI001A", 0},
>>
>> 	{ "AMDI001A" },
>>
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
>>> +
>>> +static struct platform_driver amd_asf_driver = {
>>> +	.driver = {
>>> +		.name = "i2c-amd-asf",
>>> +		.acpi_match_table = amd_asf_acpi_ids,
>>> +	},
>>> +	.probe = amd_asf_probe,
>>> +	.remove_new = amd_asf_remove,
>>> +};
>>> +module_platform_driver(amd_asf_driver);
>>
>> ...
>>
>>> +	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
>>
>> Does it compile with CONFIG_ACPI=n?
> 
> I have used a explicit 'depends on' ACPI to this driver soon that LKP
> does not complain with a randconfig.
> 
>>
>> Also don't you need to include acpi.h for this? Or is it already there?
>> (I haven't checked).
> 
> acpi_get_handle() is defined in acpi.h.
> 
> please assume the rest of the unanswered remarks as "I agree" :-)
> 
> Thanks,
> Shyam
> 
>>
>>> +	if (ACPI_SUCCESS(status))
>>> +		is_asf = true;
>>

Re: [PATCH v4 1/8] i2c: piix4: Change the signature of piix4_transaction function

[Andi Shyti]

Hi Shyam,

what does it mean signature? Do you mean "parameter list"?

Andi

On Wed, Sep 11, 2024 at 05:24:00PM GMT, Shyam Sundar S K wrote:
> Currently, `piix4_transaction()` accepts only one parameter, which is the
> `i2c_adapter` information. This approach works well as long as SB800 SMBus
> port accesses are confined to the piix4 driver. However, with the
> implementation of a separate ASF driver and the varying address spaces
> across drivers, it is necessary to change the function signature of
> `piix4_transaction()` to include the port address. This modification
> allows other drivers that use piix4 to pass the specific port details they
> need to operate on.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 4e32d57ae0bf..69b362db6d0c 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -536,10 +536,8 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
>  	return piix4_smba;
>  }
>  
> -static int piix4_transaction(struct i2c_adapter *piix4_adapter)
> +static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>  {
> -	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
> -	unsigned short piix4_smba = adapdata->smba;
>  	int temp;
>  	int result = 0;
>  	int timeout = 0;
> @@ -675,7 +673,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>  
>  	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
>  
> -	status = piix4_transaction(adap);
> +	status = piix4_transaction(adap, piix4_smba);
>  	if (status)
>  		return status;
>  
> -- 
> 2.25.1
> 

Re: [PATCH v4 1/8] i2c: piix4: Change the signature of piix4_transaction function

[Shyam Sundar S K]

Hi Andi

On 9/12/2024 11:45, Andi Shyti wrote:
> Hi Shyam,
> 
> what does it mean signature? Do you mean "parameter list"?

Yes. I mean "parameter list". I can amend it in the next version.

Thanks,
Shyam

> 
> Andi
> 
> On Wed, Sep 11, 2024 at 05:24:00PM GMT, Shyam Sundar S K wrote:
>> Currently, `piix4_transaction()` accepts only one parameter, which is the
>> `i2c_adapter` information. This approach works well as long as SB800 SMBus
>> port accesses are confined to the piix4 driver. However, with the
>> implementation of a separate ASF driver and the varying address spaces
>> across drivers, it is necessary to change the function signature of
>> `piix4_transaction()` to include the port address. This modification
>> allows other drivers that use piix4 to pass the specific port details they
>> need to operate on.
>>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/i2c/busses/i2c-piix4.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 4e32d57ae0bf..69b362db6d0c 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -536,10 +536,8 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
>>  	return piix4_smba;
>>  }
>>  
>> -static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>> +static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>>  {
>> -	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
>> -	unsigned short piix4_smba = adapdata->smba;
>>  	int temp;
>>  	int result = 0;
>>  	int timeout = 0;
>> @@ -675,7 +673,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>>  
>>  	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
>>  
>> -	status = piix4_transaction(adap);
>> +	status = piix4_transaction(adap, piix4_smba);
>>  	if (status)
>>  		return status;
>>  
>> -- 
>> 2.25.1
>>

Re: [PATCH v4 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Andi Shyti]

Hi Shyam,

On Wed, Sep 11, 2024 at 05:24:02PM GMT, Shyam Sundar S K wrote:
> Export the following i2c_piix4 driver functions as a library so that the
> AMD ASF driver can utilize these core functionalities from the i2c_piix4
> driver:
> 
> - piix4_sb800_region_request(): Request access to a specific SMBus region
> on the SB800 chipset.
> 
> - piix4_sb800_region_release(): Release the previously requested SMBus
> region on the SB800 chipset.
> 
> - piix4_transaction(): Handle SMBus transactions between the SMBus
> controller and connected devices.
> 
> - piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
> chipset.
> 
> By making these functions available as a library, enable the AMD ASF
> driver to leverage the established mechanisms in the i2c_piix4 driver,
> promoting code reuse and consistency across different drivers.

...

> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 2c2a466e2f85..174cce254e96 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -153,8 +153,7 @@ struct i2c_piix4_adapdata {
>  	struct sb800_mmio_cfg mmio_cfg;
>  };
>  
> -static int piix4_sb800_region_request(struct device *dev,
> -				      struct sb800_mmio_cfg *mmio_cfg)
> +int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)

I’m not entirely happy with this change, or the others above. If
someone runs a git bisect, they would be confused by not seeing
this change described in the commit log.

While it's true that the accepted line length is now 100
characters, the 80-character limit is still preferred (and
personally, I prefer 80, though that’s just my opinion).

This change doesn’t seem necessary, so please amend it along with
the others below in the next version.

Thanks,
Andi

>  {
>  	if (mmio_cfg->use_mmio) {
>  		void __iomem *addr;
> @@ -192,9 +191,9 @@ static int piix4_sb800_region_request(struct device *dev,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(piix4_sb800_region_request);
>  
> -static void piix4_sb800_region_release(struct device *dev,
> -				       struct sb800_mmio_cfg *mmio_cfg)
> +void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
>  {
>  	if (mmio_cfg->use_mmio) {
>  		iounmap(mmio_cfg->addr);
> @@ -205,6 +204,7 @@ static void piix4_sb800_region_release(struct device *dev,
>  
>  	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
>  }
> +EXPORT_SYMBOL_GPL(piix4_sb800_region_release);
>  
>  static bool piix4_sb800_use_mmio(struct pci_dev *PIIX4_dev)
>  {
> @@ -514,7 +514,7 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
>  	return piix4_smba;
>  }
>  
> -static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
> +int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>  {
>  	int temp;
>  	int result = 0;
> @@ -587,6 +587,7 @@ static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short p
>  		inb_p(SMBHSTDAT1));
>  	return result;
>  }
> +EXPORT_SYMBOL_GPL(piix4_transaction);
>  
>  /* Return negative errno on error. */
>  static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
> @@ -740,7 +741,7 @@ static void piix4_imc_wakeup(void)
>  	release_region(KERNCZ_IMC_IDX, 2);
>  }
>  
> -static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
> +int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>  {
>  	u8 smba_en_lo, val;
>  
> @@ -762,6 +763,7 @@ static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>  
>  	return (smba_en_lo & piix4_port_mask_sb800);
>  }
> +EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);
>  
>  /*
>   * Handles access to multiple SMBus ports on the SB800.

...

Re: [PATCH v4 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Shyam Sundar S K]

Hi Andi,

On 9/12/2024 13:10, Andi Shyti wrote:
> Hi Shyam,
> 
> On Wed, Sep 11, 2024 at 05:24:02PM GMT, Shyam Sundar S K wrote:
>> Export the following i2c_piix4 driver functions as a library so that the
>> AMD ASF driver can utilize these core functionalities from the i2c_piix4
>> driver:
>>
>> - piix4_sb800_region_request(): Request access to a specific SMBus region
>> on the SB800 chipset.
>>
>> - piix4_sb800_region_release(): Release the previously requested SMBus
>> region on the SB800 chipset.
>>
>> - piix4_transaction(): Handle SMBus transactions between the SMBus
>> controller and connected devices.
>>
>> - piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
>> chipset.
>>
>> By making these functions available as a library, enable the AMD ASF
>> driver to leverage the established mechanisms in the i2c_piix4 driver,
>> promoting code reuse and consistency across different drivers.
> 
> ...
> 
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 2c2a466e2f85..174cce254e96 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -153,8 +153,7 @@ struct i2c_piix4_adapdata {
>>  	struct sb800_mmio_cfg mmio_cfg;
>>  };
>>  
>> -static int piix4_sb800_region_request(struct device *dev,
>> -				      struct sb800_mmio_cfg *mmio_cfg)
>> +int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
> 
> I’m not entirely happy with this change, or the others above. If
> someone runs a git bisect, they would be confused by not seeing
> this change described in the commit log.

Can you elaborate a bit more on the expectation? The commit message
has the details on the each of the functions that has to be exported.

Can you please take a look at it again?

> 
> While it's true that the accepted line length is now 100
> characters, the 80-character limit is still preferred (and
> personally, I prefer 80, though that’s just my opinion).
> 
> This change doesn’t seem necessary, so please amend it along with
> the others below in the next version.

Ok. I will move to 80 char length.

Thanks,
Shyam

> 
> Thanks,
> Andi
> 
>>  {
>>  	if (mmio_cfg->use_mmio) {
>>  		void __iomem *addr;
>> @@ -192,9 +191,9 @@ static int piix4_sb800_region_request(struct device *dev,
>>  
>>  	return 0;
>>  }
>> +EXPORT_SYMBOL_GPL(piix4_sb800_region_request);
>>  
>> -static void piix4_sb800_region_release(struct device *dev,
>> -				       struct sb800_mmio_cfg *mmio_cfg)
>> +void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
>>  {
>>  	if (mmio_cfg->use_mmio) {
>>  		iounmap(mmio_cfg->addr);
>> @@ -205,6 +204,7 @@ static void piix4_sb800_region_release(struct device *dev,
>>  
>>  	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
>>  }
>> +EXPORT_SYMBOL_GPL(piix4_sb800_region_release);
>>  
>>  static bool piix4_sb800_use_mmio(struct pci_dev *PIIX4_dev)
>>  {
>> @@ -514,7 +514,7 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
>>  	return piix4_smba;
>>  }
>>  
>> -static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>> +int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>>  {
>>  	int temp;
>>  	int result = 0;
>> @@ -587,6 +587,7 @@ static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short p
>>  		inb_p(SMBHSTDAT1));
>>  	return result;
>>  }
>> +EXPORT_SYMBOL_GPL(piix4_transaction);
>>  
>>  /* Return negative errno on error. */
>>  static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>> @@ -740,7 +741,7 @@ static void piix4_imc_wakeup(void)
>>  	release_region(KERNCZ_IMC_IDX, 2);
>>  }
>>  
>> -static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>> +int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>>  {
>>  	u8 smba_en_lo, val;
>>  
>> @@ -762,6 +763,7 @@ static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>>  
>>  	return (smba_en_lo & piix4_port_mask_sb800);
>>  }
>> +EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);
>>  
>>  /*
>>   * Handles access to multiple SMBus ports on the SB800.
> 
> ...

Re: [PATCH v4 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Andi Shyti]

Hi Shyam,

On Fri, Sep 13, 2024 at 11:54:55AM GMT, Shyam Sundar S K wrote:
> On 9/12/2024 13:10, Andi Shyti wrote:
> > On Wed, Sep 11, 2024 at 05:24:02PM GMT, Shyam Sundar S K wrote:
> >> Export the following i2c_piix4 driver functions as a library so that the
> >> AMD ASF driver can utilize these core functionalities from the i2c_piix4
> >> driver:
> >>
> >> - piix4_sb800_region_request(): Request access to a specific SMBus region
> >> on the SB800 chipset.
> >>
> >> - piix4_sb800_region_release(): Release the previously requested SMBus
> >> region on the SB800 chipset.
> >>
> >> - piix4_transaction(): Handle SMBus transactions between the SMBus
> >> controller and connected devices.
> >>
> >> - piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
> >> chipset.
> >>
> >> By making these functions available as a library, enable the AMD ASF
> >> driver to leverage the established mechanisms in the i2c_piix4 driver,
> >> promoting code reuse and consistency across different drivers.
> > 
> > ...
> > 
> >> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> >> index 2c2a466e2f85..174cce254e96 100644
> >> --- a/drivers/i2c/busses/i2c-piix4.c
> >> +++ b/drivers/i2c/busses/i2c-piix4.c
> >> @@ -153,8 +153,7 @@ struct i2c_piix4_adapdata {
> >>  	struct sb800_mmio_cfg mmio_cfg;
> >>  };
> >>  
> >> -static int piix4_sb800_region_request(struct device *dev,
> >> -				      struct sb800_mmio_cfg *mmio_cfg)
> >> +int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
> > 
> > I’m not entirely happy with this change, or the others above. If
> > someone runs a git bisect, they would be confused by not seeing
> > this change described in the commit log.
> 
> Can you elaborate a bit more on the expectation? The commit message
> has the details on the each of the functions that has to be exported.
> 
> Can you please take a look at it again?

The change I am referring to is that style change I described
here below, that consists in putting in one line the functions.

You are describing the logical changes, but there is no mention
of the fact that you are moving the second parameter of the
function in the same line with the other.

> > While it's true that the accepted line length is now 100
> > characters, the 80-character limit is still preferred (and
> > personally, I prefer 80, though that’s just my opinion).
> > 
> > This change doesn’t seem necessary, so please amend it along with
> > the others below in the next version.
> 
> Ok. I will move to 80 char length.

Thanks,
Andi