[PATCH v2 00/10] soc: aspeed: lpc-snoop: Miscellaneous fixes

[PATCH v2 00/10] soc: aspeed: lpc-snoop: Miscellaneous fixes

[Andrew Jeffery]

Henry's bug[1] and fix[2] prompted some further inspection by
Jean.

This series provides fixes for the remaining issues Jean identified, as
well as reworking the channel paths to reduce cleanup required in error
paths. It is based on v6.16-rc1.

Lightly tested under qemu and on an AST2600 EVB. Further testing on
platforms designed around the snoop device appreciated.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=219934
[2]: https://lore.kernel.org/all/20250401074647.21300-1-bsdhenrymartin@gmail.com/

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
Changes in v2:
- Address comments on v1 from Jean
- Implement channel indexing using enums to avoid unnecessary tests
- Switch to devm_clk_get_enabled()
- Use dev_err_probe() where possible
- Link to v1: https://patch.msgid.link/20250411-aspeed-lpc-snoop-fixes-v1-0-64f522e3ad6f@codeconstruct.com.au

---
Andrew Jeffery (10):
      soc: aspeed: lpc-snoop: Cleanup resources in stack-order
      soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled
      soc: aspeed: lpc-snoop: Ensure model_data is valid
      soc: aspeed: lpc-snoop: Constrain parameters in channel paths
      soc: aspeed: lpc-snoop: Rename 'channel' to 'index' in channel paths
      soc: aspeed: lpc-snoop: Rearrange channel paths
      soc: aspeed: lpc-snoop: Switch to devm_clk_get_enabled()
      soc: aspeed: lpc-snoop: Use dev_err_probe() where possible
      soc: aspeed: lpc-snoop: Consolidate channel initialisation
      soc: aspeed: lpc-snoop: Lift channel config to const structs

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 224 +++++++++++++++++-----------------
 1 file changed, 110 insertions(+), 114 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250401-aspeed-lpc-snoop-fixes-e5d2883da3a3

Best regards,
-- 
Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH v2 01/10] soc: aspeed: lpc-snoop: Cleanup resources in stack-order

[Andrew Jeffery]

Free the kfifo after unregistering the miscdev in
aspeed_lpc_disable_snoop() as the kfifo is initialised before the
miscdev in aspeed_lpc_enable_snoop().

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Cc: stable@vger.kernel.org
Cc: Jean Delvare <jdelvare@suse.de>
Acked-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index ef8f355589a584d76bfac2b130ba9965c9b06ba5..59c18afa649cd672f54f947066f468f2a78c0d5d 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -263,8 +263,8 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
-	kfifo_free(&lpc_snoop->chan[channel].fifo);
 	misc_deregister(&lpc_snoop->chan[channel].miscdev);
+	kfifo_free(&lpc_snoop->chan[channel].fifo);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)

-- 
2.39.5

[PATCH v2 02/10] soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled

[Andrew Jeffery]

Mitigate e.g. the following:

    # echo 1e789080.lpc-snoop > /sys/bus/platform/drivers/aspeed-lpc-snoop/unbind
    ...
    [  120.363594] Unable to handle kernel NULL pointer dereference at virtual address 00000004 when write
    [  120.373866] [00000004] *pgd=00000000
    [  120.377910] Internal error: Oops: 805 [#1] SMP ARM
    [  120.383306] CPU: 1 UID: 0 PID: 315 Comm: sh Not tainted 6.15.0-rc1-00009-g926217bc7d7d-dirty #20 NONE
    ...
    [  120.679543] Call trace:
    [  120.679559]  misc_deregister from aspeed_lpc_snoop_remove+0x84/0xac
    [  120.692462]  aspeed_lpc_snoop_remove from platform_remove+0x28/0x38
    [  120.700996]  platform_remove from device_release_driver_internal+0x188/0x200
    ...

Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")
Cc: stable@vger.kernel.org
Cc: Jean Delvare <jdelvare@suse.de>
Acked-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 59c18afa649cd672f54f947066f468f2a78c0d5d..fc3a2c41cc10739f5f70ded7ac02baab6468d652 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -58,6 +58,7 @@ struct aspeed_lpc_snoop_model_data {
 };
 
 struct aspeed_lpc_snoop_channel {
+	bool enabled;
 	struct kfifo		fifo;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
@@ -190,6 +191,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	const struct aspeed_lpc_snoop_model_data *model_data =
 		of_device_get_match_data(dev);
 
+	if (WARN_ON(lpc_snoop->chan[channel].enabled))
+		return -EBUSY;
+
 	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
 	/* Create FIFO datastructure */
 	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
@@ -236,6 +240,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		regmap_update_bits(lpc_snoop->regmap, HICRB,
 				hicrb_en, hicrb_en);
 
+	lpc_snoop->chan[channel].enabled = true;
+
 	return 0;
 
 err_misc_deregister:
@@ -248,6 +254,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				     int channel)
 {
+	if (!lpc_snoop->chan[channel].enabled)
+		return;
+
 	switch (channel) {
 	case 0:
 		regmap_update_bits(lpc_snoop->regmap, HICR5,
@@ -263,6 +272,8 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
+	lpc_snoop->chan[channel].enabled = false;
+	/* Consider improving safety wrt concurrent reader(s) */
 	misc_deregister(&lpc_snoop->chan[channel].miscdev);
 	kfifo_free(&lpc_snoop->chan[channel].fifo);
 }

-- 
2.39.5

[PATCH v2 03/10] soc: aspeed: lpc-snoop: Ensure model_data is valid

[Andrew Jeffery]

of_device_get_match_data() can return NULL, though shouldn't in current
circumstances. Regardless, initialise model_data closer to use so it's
clear we need to test for validity prior to dereferencing.

Fixes: 2dee584bc9e3 ("drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat")
Acked-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index fc3a2c41cc10739f5f70ded7ac02baab6468d652..ca7536213e0986f737606a52996ffea620df2a7a 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -186,10 +186,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				   struct device *dev,
 				   int channel, u16 lpc_port)
 {
-	int rc = 0;
+	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
-	const struct aspeed_lpc_snoop_model_data *model_data =
-		of_device_get_match_data(dev);
+	int rc = 0;
 
 	if (WARN_ON(lpc_snoop->chan[channel].enabled))
 		return -EBUSY;
@@ -236,9 +235,10 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
 	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
 			   lpc_port << snpwadr_shift);
-	if (model_data->has_hicrb_ensnp)
-		regmap_update_bits(lpc_snoop->regmap, HICRB,
-				hicrb_en, hicrb_en);
+
+	model_data = of_device_get_match_data(dev);
+	if (model_data && model_data->has_hicrb_ensnp)
+		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
 
 	lpc_snoop->chan[channel].enabled = true;
 

-- 
2.39.5

[PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Andrew Jeffery]

Ensure pointers and the channel index are valid before use.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -25,7 +25,6 @@
 
 #define DEVICE_NAME	"aspeed-lpc-snoop"
 
-#define NUM_SNOOP_CHANNELS 2
 #define SNOOP_FIFO_SIZE 2048
 
 #define HICR5	0x80
@@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
 	unsigned int has_hicrb_ensnp;
 };
 
+enum aspeed_lpc_snoop_index {
+	ASPEED_LPC_SNOOP_INDEX_0 = 0,
+	ASPEED_LPC_SNOOP_INDEX_1 = 1,
+	ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
+};
+
 struct aspeed_lpc_snoop_channel {
 	bool enabled;
 	struct kfifo		fifo;
@@ -68,7 +73,7 @@ struct aspeed_lpc_snoop {
 	struct regmap		*regmap;
 	int			irq;
 	struct clk		*clk;
-	struct aspeed_lpc_snoop_channel chan[NUM_SNOOP_CHANNELS];
+	struct aspeed_lpc_snoop_channel chan[ASPEED_LPC_SNOOP_INDEX_MAX + 1];
 };
 
 static struct aspeed_lpc_snoop_channel *snoop_file_to_chan(struct file *file)
@@ -182,9 +187,10 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
 	return 0;
 }
 
+__attribute__((nonnull))
 static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				   struct device *dev,
-				   int channel, u16 lpc_port)
+				   enum aspeed_lpc_snoop_index channel, u16 lpc_port)
 {
 	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
@@ -251,8 +257,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	return rc;
 }
 
+__attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
-				     int channel)
+				     enum aspeed_lpc_snoop_index channel)
 {
 	if (!lpc_snoop->chan[channel].enabled)
 		return;
@@ -331,16 +338,16 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 	if (rc)
 		goto err;
 
-	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
+	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port);
 	if (rc)
 		goto err;
 
 	/* Configuration of 2nd snoop channel port is optional */
 	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
 				       1, &port) == 0) {
-		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
+		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_1, port);
 		if (rc) {
-			aspeed_lpc_disable_snoop(lpc_snoop, 0);
+			aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
 			goto err;
 		}
 	}
@@ -358,8 +365,8 @@ static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
 	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
 
 	/* Disable both snoop channels */
-	aspeed_lpc_disable_snoop(lpc_snoop, 0);
-	aspeed_lpc_disable_snoop(lpc_snoop, 1);
+	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
+	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
 
 	clk_disable_unprepare(lpc_snoop->clk);
 }

-- 
2.39.5

[PATCH v2 05/10] soc: aspeed: lpc-snoop: Rename 'channel' to 'index' in channel paths

[Andrew Jeffery]

We'll introduce another 'channel' variable shortly

Acked-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 43 ++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 804c6ed9c4c671da73a6c66c1de41c59922c82dc..e9d17239163a8ae5145bd3652fcec572b70bd11c 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -190,37 +190,37 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
 __attribute__((nonnull))
 static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				   struct device *dev,
-				   enum aspeed_lpc_snoop_index channel, u16 lpc_port)
+				   enum aspeed_lpc_snoop_index index, u16 lpc_port)
 {
 	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
 	int rc = 0;
 
-	if (WARN_ON(lpc_snoop->chan[channel].enabled))
+	if (WARN_ON(lpc_snoop->chan[index].enabled))
 		return -EBUSY;
 
-	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
+	init_waitqueue_head(&lpc_snoop->chan[index].wq);
 	/* Create FIFO datastructure */
-	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
+	rc = kfifo_alloc(&lpc_snoop->chan[index].fifo,
 			 SNOOP_FIFO_SIZE, GFP_KERNEL);
 	if (rc)
 		return rc;
 
-	lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
-	lpc_snoop->chan[channel].miscdev.name =
-		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
-	if (!lpc_snoop->chan[channel].miscdev.name) {
+	lpc_snoop->chan[index].miscdev.minor = MISC_DYNAMIC_MINOR;
+	lpc_snoop->chan[index].miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
+	if (!lpc_snoop->chan[index].miscdev.name) {
 		rc = -ENOMEM;
 		goto err_free_fifo;
 	}
-	lpc_snoop->chan[channel].miscdev.fops = &snoop_fops;
-	lpc_snoop->chan[channel].miscdev.parent = dev;
-	rc = misc_register(&lpc_snoop->chan[channel].miscdev);
+	lpc_snoop->chan[index].miscdev.fops = &snoop_fops;
+	lpc_snoop->chan[index].miscdev.parent = dev;
+	rc = misc_register(&lpc_snoop->chan[index].miscdev);
 	if (rc)
 		goto err_free_fifo;
 
 	/* Enable LPC snoop channel at requested port */
-	switch (channel) {
+	switch (index) {
 	case 0:
 		hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W;
 		snpwadr_mask = SNPWADR_CH0_MASK;
@@ -246,25 +246,26 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	if (model_data && model_data->has_hicrb_ensnp)
 		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
 
-	lpc_snoop->chan[channel].enabled = true;
+	lpc_snoop->chan[index].enabled = true;
 
 	return 0;
 
 err_misc_deregister:
-	misc_deregister(&lpc_snoop->chan[channel].miscdev);
+	misc_deregister(&lpc_snoop->chan[index].miscdev);
 err_free_fifo:
-	kfifo_free(&lpc_snoop->chan[channel].fifo);
+	kfifo_free(&lpc_snoop->chan[index].fifo);
 	return rc;
 }
 
 __attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
-				     enum aspeed_lpc_snoop_index channel)
+				     enum aspeed_lpc_snoop_index index)
 {
-	if (!lpc_snoop->chan[channel].enabled)
+	if (!lpc_snoop->chan[index].enabled)
 		return;
 
-	switch (channel) {
+	/* Disable interrupts along with the device */
+	switch (index) {
 	case 0:
 		regmap_update_bits(lpc_snoop->regmap, HICR5,
 				   HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
@@ -279,10 +280,10 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
-	lpc_snoop->chan[channel].enabled = false;
+	lpc_snoop->chan[index].enabled = false;
 	/* Consider improving safety wrt concurrent reader(s) */
-	misc_deregister(&lpc_snoop->chan[channel].miscdev);
-	kfifo_free(&lpc_snoop->chan[channel].fifo);
+	misc_deregister(&lpc_snoop->chan[index].miscdev);
+	kfifo_free(&lpc_snoop->chan[index].fifo);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)

-- 
2.39.5

[PATCH v2 06/10] soc: aspeed: lpc-snoop: Rearrange channel paths

[Andrew Jeffery]

Order assignments such that tests for conditions not involving resource
acquisition are ordered before those testing acquired resources, and
order managed resource acquisition before unmanaged where possible. This
way we minimise the amount of manual cleanup required.

In the process, improve readability of the code by introducing a channel
pointer that takes the place of the repeated object lookups.

Acked-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 ++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index e9d17239163a8ae5145bd3652fcec572b70bd11c..9992212c789d4224edcc0ee1a3bb9c73f9fc661b 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -194,28 +194,30 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 {
 	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
+	struct aspeed_lpc_snoop_channel *channel;
 	int rc = 0;
 
-	if (WARN_ON(lpc_snoop->chan[index].enabled))
+	channel = &lpc_snoop->chan[index];
+
+	if (WARN_ON(channel->enabled))
 		return -EBUSY;
 
-	init_waitqueue_head(&lpc_snoop->chan[index].wq);
-	/* Create FIFO datastructure */
-	rc = kfifo_alloc(&lpc_snoop->chan[index].fifo,
-			 SNOOP_FIFO_SIZE, GFP_KERNEL);
+	init_waitqueue_head(&channel->wq);
+
+	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
+	channel->miscdev.fops = &snoop_fops;
+	channel->miscdev.parent = dev;
+
+	channel->miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
+	if (!channel->miscdev.name)
+		return -ENOMEM;
+
+	rc = kfifo_alloc(&channel->fifo, SNOOP_FIFO_SIZE, GFP_KERNEL);
 	if (rc)
 		return rc;
 
-	lpc_snoop->chan[index].miscdev.minor = MISC_DYNAMIC_MINOR;
-	lpc_snoop->chan[index].miscdev.name =
-		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
-	if (!lpc_snoop->chan[index].miscdev.name) {
-		rc = -ENOMEM;
-		goto err_free_fifo;
-	}
-	lpc_snoop->chan[index].miscdev.fops = &snoop_fops;
-	lpc_snoop->chan[index].miscdev.parent = dev;
-	rc = misc_register(&lpc_snoop->chan[index].miscdev);
+	rc = misc_register(&channel->miscdev);
 	if (rc)
 		goto err_free_fifo;
 
@@ -238,6 +240,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		goto err_misc_deregister;
 	}
 
+	/* Enable LPC snoop channel at requested port */
 	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
 	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
 			   lpc_port << snpwadr_shift);
@@ -246,14 +249,14 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	if (model_data && model_data->has_hicrb_ensnp)
 		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
 
-	lpc_snoop->chan[index].enabled = true;
+	channel->enabled = true;
 
 	return 0;
 
 err_misc_deregister:
-	misc_deregister(&lpc_snoop->chan[index].miscdev);
+	misc_deregister(&channel->miscdev);
 err_free_fifo:
-	kfifo_free(&lpc_snoop->chan[index].fifo);
+	kfifo_free(&channel->fifo);
 	return rc;
 }
 
@@ -261,7 +264,11 @@ __attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				     enum aspeed_lpc_snoop_index index)
 {
-	if (!lpc_snoop->chan[index].enabled)
+	struct aspeed_lpc_snoop_channel *channel;
+
+	channel = &lpc_snoop->chan[index];
+
+	if (!channel->enabled)
 		return;
 
 	/* Disable interrupts along with the device */
@@ -280,10 +287,10 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
-	lpc_snoop->chan[index].enabled = false;
+	channel->enabled = false;
 	/* Consider improving safety wrt concurrent reader(s) */
-	misc_deregister(&lpc_snoop->chan[index].miscdev);
-	kfifo_free(&lpc_snoop->chan[index].fifo);
+	misc_deregister(&channel->miscdev);
+	kfifo_free(&channel->fifo);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)

-- 
2.39.5

[PATCH v2 07/10] soc: aspeed: lpc-snoop: Switch to devm_clk_get_enabled()

[Andrew Jeffery]

Simplify clock handling as done in other drivers.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 9992212c789d4224edcc0ee1a3bb9c73f9fc661b..bd4afa7f258eb3c1e64fe87d2b4be5f8422fbaf7 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -329,26 +329,21 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	lpc_snoop->clk = devm_clk_get(dev, NULL);
+	lpc_snoop->clk = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(lpc_snoop->clk)) {
 		rc = PTR_ERR(lpc_snoop->clk);
 		if (rc != -EPROBE_DEFER)
 			dev_err(dev, "couldn't get clock\n");
 		return rc;
 	}
-	rc = clk_prepare_enable(lpc_snoop->clk);
-	if (rc) {
-		dev_err(dev, "couldn't enable clock\n");
-		return rc;
-	}
 
 	rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev);
 	if (rc)
-		goto err;
+		return rc;
 
 	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port);
 	if (rc)
-		goto err;
+		return rc;
 
 	/* Configuration of 2nd snoop channel port is optional */
 	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
@@ -356,16 +351,11 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_1, port);
 		if (rc) {
 			aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
-			goto err;
+			return rc;
 		}
 	}
 
 	return 0;
-
-err:
-	clk_disable_unprepare(lpc_snoop->clk);
-
-	return rc;
 }
 
 static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
@@ -375,8 +365,6 @@ static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
 	/* Disable both snoop channels */
 	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
 	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
-
-	clk_disable_unprepare(lpc_snoop->clk);
 }
 
 static const struct aspeed_lpc_snoop_model_data ast2400_model_data = {

-- 
2.39.5

[PATCH v2 08/10] soc: aspeed: lpc-snoop: Use dev_err_probe() where possible

[Andrew Jeffery]

Exploit that it returns the provided error to eliminate some lines, and
return the actual error involved rather than -ENODEV.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index bd4afa7f258eb3c1e64fe87d2b4be5f8422fbaf7..8dbc9d4158b89f23bda340f060d205a29bbb43c3 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -12,6 +12,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/dev_printk.h>
 #include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/kfifo.h>
@@ -316,10 +317,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 	}
 
 	lpc_snoop->regmap = syscon_node_to_regmap(np);
-	if (IS_ERR(lpc_snoop->regmap)) {
-		dev_err(dev, "Couldn't get regmap\n");
-		return -ENODEV;
-	}
+	if (IS_ERR(lpc_snoop->regmap))
+		return dev_err_probe(dev, PTR_ERR(lpc_snoop->regmap), "Couldn't get regmap\n");
 
 	dev_set_drvdata(&pdev->dev, lpc_snoop);
 
@@ -330,12 +329,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 	}
 
 	lpc_snoop->clk = devm_clk_get_enabled(dev, NULL);
-	if (IS_ERR(lpc_snoop->clk)) {
-		rc = PTR_ERR(lpc_snoop->clk);
-		if (rc != -EPROBE_DEFER)
-			dev_err(dev, "couldn't get clock\n");
-		return rc;
-	}
+	if (IS_ERR(lpc_snoop->clk))
+		return dev_err_probe(dev, PTR_ERR(lpc_snoop->clk), "couldn't get clock");
 
 	rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev);
 	if (rc)

-- 
2.39.5

[PATCH v2 09/10] soc: aspeed: lpc-snoop: Consolidate channel initialisation

[Andrew Jeffery]

Previously, channel initialisation was a bit perilous with respect to
resource cleanup in error paths. While the implementation had issues,
it at least made an effort to eliminate some of its problems by first
testing whether any channels were enabled, and bailing out if not.

Having improved the robustness of resource handling in probe() we can
now rearrange the initial channel test to be located with the subsequent
test, and rework the unrolled conditional logic to use a loop for an
improvement in readability.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 +++++++++++++++++------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 8dbc9d4158b89f23bda340f060d205a29bbb43c3..9f88c5471b1b6d85f6d9e1970240f3d1904d166c 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -294,12 +294,21 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	kfifo_free(&channel->fifo);
 }
 
+static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
+{
+	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
+
+	/* Disable both snoop channels */
+	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
+	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
+}
+
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 {
 	struct aspeed_lpc_snoop *lpc_snoop;
-	struct device *dev;
 	struct device_node *np;
-	u32 port;
+	struct device *dev;
+	int idx;
 	int rc;
 
 	dev = &pdev->dev;
@@ -322,12 +331,6 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(&pdev->dev, lpc_snoop);
 
-	rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0, &port);
-	if (rc) {
-		dev_err(dev, "no snoop ports configured\n");
-		return -ENODEV;
-	}
-
 	lpc_snoop->clk = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(lpc_snoop->clk))
 		return dev_err_probe(dev, PTR_ERR(lpc_snoop->clk), "couldn't get clock");
@@ -336,30 +339,24 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port);
-	if (rc)
-		return rc;
+	for (idx = ASPEED_LPC_SNOOP_INDEX_0; idx <= ASPEED_LPC_SNOOP_INDEX_MAX; idx++) {
+		u32 port;
 
-	/* Configuration of 2nd snoop channel port is optional */
-	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
-				       1, &port) == 0) {
-		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_1, port);
-		if (rc) {
-			aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
-			return rc;
-		}
+		rc = of_property_read_u32_index(dev->of_node, "snoop-ports", idx, &port);
+		if (rc)
+			break;
+
+		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, idx, port);
+		if (rc)
+			goto cleanup_channels;
 	}
 
-	return 0;
-}
+	return idx == ASPEED_LPC_SNOOP_INDEX_0 ? -ENODEV : 0;
 
-static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
-{
-	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
+cleanup_channels:
+	aspeed_lpc_snoop_remove(pdev);
 
-	/* Disable both snoop channels */
-	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
-	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
+	return rc;
 }
 
 static const struct aspeed_lpc_snoop_model_data ast2400_model_data = {

-- 
2.39.5

[PATCH v2 10/10] soc: aspeed: lpc-snoop: Lift channel config to const structs

[Andrew Jeffery]

The shifts and masks for each channel are defined by hardware and
are not something that changes at runtime. Accordingly, describe the
information in an array of const structs and associate elements with
each channel instance, removing the need for the switch and handling of
its default case.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 100 +++++++++++++++-------------------
 1 file changed, 45 insertions(+), 55 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 9f88c5471b1b6d85f6d9e1970240f3d1904d166c..2d97b8d5fb429e215c321c9c2ee3fa35d39f8618 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -63,7 +63,16 @@ enum aspeed_lpc_snoop_index {
 	ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
 };
 
+struct aspeed_lpc_snoop_channel_cfg {
+	enum aspeed_lpc_snoop_index index;
+	u32 hicr5_en;
+	u32 snpwadr_mask;
+	u32 snpwadr_shift;
+	u32 hicrb_en;
+};
+
 struct aspeed_lpc_snoop_channel {
+	const struct aspeed_lpc_snoop_channel_cfg *cfg;
 	bool enabled;
 	struct kfifo		fifo;
 	wait_queue_head_t	wq;
@@ -77,6 +86,23 @@ struct aspeed_lpc_snoop {
 	struct aspeed_lpc_snoop_channel chan[ASPEED_LPC_SNOOP_INDEX_MAX + 1];
 };
 
+static const struct aspeed_lpc_snoop_channel_cfg channel_cfgs[ASPEED_LPC_SNOOP_INDEX_MAX + 1] = {
+	{
+		.index = ASPEED_LPC_SNOOP_INDEX_0,
+		.hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
+		.snpwadr_mask = SNPWADR_CH0_MASK,
+		.snpwadr_shift = SNPWADR_CH0_SHIFT,
+		.hicrb_en = HICRB_ENSNP0D,
+	},
+	{
+		.index = ASPEED_LPC_SNOOP_INDEX_1,
+		.hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W,
+		.snpwadr_mask = SNPWADR_CH1_MASK,
+		.snpwadr_shift = SNPWADR_CH1_SHIFT,
+		.hicrb_en = HICRB_ENSNP1D,
+	},
+};
+
 static struct aspeed_lpc_snoop_channel *snoop_file_to_chan(struct file *file)
 {
 	return container_of(file->private_data,
@@ -189,28 +215,27 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
 }
 
 __attribute__((nonnull))
-static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
-				   struct device *dev,
-				   enum aspeed_lpc_snoop_index index, u16 lpc_port)
+static int aspeed_lpc_enable_snoop(struct device *dev,
+				    struct aspeed_lpc_snoop *lpc_snoop,
+				    struct aspeed_lpc_snoop_channel *channel,
+				    const struct aspeed_lpc_snoop_channel_cfg *cfg,
+				    u16 lpc_port)
 {
 	const struct aspeed_lpc_snoop_model_data *model_data;
-	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
-	struct aspeed_lpc_snoop_channel *channel;
 	int rc = 0;
 
-	channel = &lpc_snoop->chan[index];
-
 	if (WARN_ON(channel->enabled))
 		return -EBUSY;
 
 	init_waitqueue_head(&channel->wq);
 
+	channel->cfg = cfg;
 	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
 	channel->miscdev.fops = &snoop_fops;
 	channel->miscdev.parent = dev;
 
 	channel->miscdev.name =
-		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
+		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, cfg->index);
 	if (!channel->miscdev.name)
 		return -ENOMEM;
 
@@ -223,39 +248,18 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		goto err_free_fifo;
 
 	/* Enable LPC snoop channel at requested port */
-	switch (index) {
-	case 0:
-		hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W;
-		snpwadr_mask = SNPWADR_CH0_MASK;
-		snpwadr_shift = SNPWADR_CH0_SHIFT;
-		hicrb_en = HICRB_ENSNP0D;
-		break;
-	case 1:
-		hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W;
-		snpwadr_mask = SNPWADR_CH1_MASK;
-		snpwadr_shift = SNPWADR_CH1_SHIFT;
-		hicrb_en = HICRB_ENSNP1D;
-		break;
-	default:
-		rc = -EINVAL;
-		goto err_misc_deregister;
-	}
-
-	/* Enable LPC snoop channel at requested port */
-	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
-	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
-			   lpc_port << snpwadr_shift);
+	regmap_set_bits(lpc_snoop->regmap, HICR5, cfg->hicr5_en);
+	regmap_update_bits(lpc_snoop->regmap, SNPWADR, cfg->snpwadr_mask,
+		lpc_port << cfg->snpwadr_shift);
 
 	model_data = of_device_get_match_data(dev);
 	if (model_data && model_data->has_hicrb_ensnp)
-		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
+		regmap_set_bits(lpc_snoop->regmap, HICRB, cfg->hicrb_en);
 
 	channel->enabled = true;
 
 	return 0;
 
-err_misc_deregister:
-	misc_deregister(&channel->miscdev);
 err_free_fifo:
 	kfifo_free(&channel->fifo);
 	return rc;
@@ -263,30 +267,13 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 
 __attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
-				     enum aspeed_lpc_snoop_index index)
+				     struct aspeed_lpc_snoop_channel *channel)
 {
-	struct aspeed_lpc_snoop_channel *channel;
-
-	channel = &lpc_snoop->chan[index];
-
 	if (!channel->enabled)
 		return;
 
 	/* Disable interrupts along with the device */
-	switch (index) {
-	case 0:
-		regmap_update_bits(lpc_snoop->regmap, HICR5,
-				   HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
-				   0);
-		break;
-	case 1:
-		regmap_update_bits(lpc_snoop->regmap, HICR5,
-				   HICR5_EN_SNP1W | HICR5_ENINT_SNP1W,
-				   0);
-		break;
-	default:
-		return;
-	}
+	regmap_clear_bits(lpc_snoop->regmap, HICR5, channel->cfg->hicr5_en);
 
 	channel->enabled = false;
 	/* Consider improving safety wrt concurrent reader(s) */
@@ -299,8 +286,8 @@ static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
 	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
 
 	/* Disable both snoop channels */
-	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
-	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
+	aspeed_lpc_disable_snoop(lpc_snoop, &lpc_snoop->chan[0]);
+	aspeed_lpc_disable_snoop(lpc_snoop, &lpc_snoop->chan[1]);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
@@ -339,6 +326,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
+	static_assert(ARRAY_SIZE(channel_cfgs) == ARRAY_SIZE(lpc_snoop->chan),
+		"Broken implementation assumption regarding cfg count");
 	for (idx = ASPEED_LPC_SNOOP_INDEX_0; idx <= ASPEED_LPC_SNOOP_INDEX_MAX; idx++) {
 		u32 port;
 
@@ -346,7 +335,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 		if (rc)
 			break;
 
-		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, idx, port);
+		rc = aspeed_lpc_enable_snoop(dev, lpc_snoop, &lpc_snoop->chan[idx],
+					     &channel_cfgs[idx], port);
 		if (rc)
 			goto cleanup_channels;
 	}

-- 
2.39.5

Re: [PATCH v2 07/10] soc: aspeed: lpc-snoop: Switch to devm_clk_get_enabled()

[Jean Delvare]

Hello Andrew,

On Mon, 16 Jun 2025 22:43:44 +0930, Andrew Jeffery wrote:
> Simplify clock handling as done in other drivers.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> (...)

LGTM.

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 08/10] soc: aspeed: lpc-snoop: Use dev_err_probe() where possible

[Jean Delvare]

On Mon, 16 Jun 2025 22:43:45 +0930, Andrew Jeffery wrote:
> Exploit that it returns the provided error to eliminate some lines, and
> return the actual error involved rather than -ENODEV.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> (...)

LGTM.

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 09/10] soc: aspeed: lpc-snoop: Consolidate channel initialisation

[Jean Delvare]

Hi Andrew,

On Mon, 16 Jun 2025 22:43:46 +0930, Andrew Jeffery wrote:
> Previously, channel initialisation was a bit perilous with respect to
> resource cleanup in error paths. While the implementation had issues,
> it at least made an effort to eliminate some of its problems by first
> testing whether any channels were enabled, and bailing out if not.
> 
> Having improved the robustness of resource handling in probe() we can
> now rearrange the initial channel test to be located with the subsequent
> test, and rework the unrolled conditional logic to use a loop for an
> improvement in readability.

I like the idea, this indeed improves readability and would make it
much easier to add support for more channels. Three suggestions inline
below.

> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 +++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 8dbc9d4158b89f23bda340f060d205a29bbb43c3..9f88c5471b1b6d85f6d9e1970240f3d1904d166c 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -294,12 +294,21 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	kfifo_free(&channel->fifo);
>  }
>  
> +static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
> +
> +	/* Disable both snoop channels */
> +	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
> +	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);

For consistency with the probe function, I think it would make sense to
use a for loop here as well, instead of hard-coding the channel number
to 2. That way, no change will be needed if a future device supports
more than 2 channels.

> +}
> +
>  static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  {
>  	struct aspeed_lpc_snoop *lpc_snoop;
> -	struct device *dev;
>  	struct device_node *np;
> -	u32 port;
> +	struct device *dev;
> +	int idx;
>  	int rc;
>  
>  	dev = &pdev->dev;
> @@ -322,12 +331,6 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  
>  	dev_set_drvdata(&pdev->dev, lpc_snoop);
>  
> -	rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0, &port);
> -	if (rc) {
> -		dev_err(dev, "no snoop ports configured\n");
> -		return -ENODEV;
> -	}
> -
>  	lpc_snoop->clk = devm_clk_get_enabled(dev, NULL);
>  	if (IS_ERR(lpc_snoop->clk))
>  		return dev_err_probe(dev, PTR_ERR(lpc_snoop->clk), "couldn't get clock");
> @@ -336,30 +339,24 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> -	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port);
> -	if (rc)
> -		return rc;
> +	for (idx = ASPEED_LPC_SNOOP_INDEX_0; idx <= ASPEED_LPC_SNOOP_INDEX_MAX; idx++) {
> +		u32 port;
>  
> -	/* Configuration of 2nd snoop channel port is optional */
> -	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
> -				       1, &port) == 0) {
> -		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_1, port);
> -		if (rc) {
> -			aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
> -			return rc;
> -		}
> +		rc = of_property_read_u32_index(dev->of_node, "snoop-ports", idx, &port);
> +		if (rc)
> +			break;
> +
> +		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, idx, port);
> +		if (rc)
> +			goto cleanup_channels;
>  	}
>  
> -	return 0;
> -}
> +	return idx == ASPEED_LPC_SNOOP_INDEX_0 ? -ENODEV : 0;

The driver used to log an error message when returning -NODEV:
"no snoop ports configured". Maybe you could call dev_err_probe()
here?

It might also be a good idea to add a comment stating that only the
first channel is mandatory, to explain why the ASPEED_LPC_SNOOP_INDEX_0
case is handled differently (there used to be a comment
	/* Configuration of 2nd snoop channel port is optional */
serving that purpose).

>  
> -static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
> -{
> -	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
> +cleanup_channels:
> +	aspeed_lpc_snoop_remove(pdev);
>  
> -	/* Disable both snoop channels */
> -	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
> -	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
> +	return rc;
>  }
>  
>  static const struct aspeed_lpc_snoop_model_data ast2400_model_data = {
> 

None if this is blocking though, so:

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 06/10] soc: aspeed: lpc-snoop: Rearrange channel paths

[Jean Delvare]

On Mon, 16 Jun 2025 22:43:43 +0930, Andrew Jeffery wrote:
> Order assignments such that tests for conditions not involving resource
> acquisition are ordered before those testing acquired resources, and
> order managed resource acquisition before unmanaged where possible. This
> way we minimise the amount of manual cleanup required.
> 
> In the process, improve readability of the code by introducing a channel
> pointer that takes the place of the repeated object lookups.
> 
> Acked-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 ++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> (...)
> @@ -238,6 +240,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  		goto err_misc_deregister;
>  	}
>  
> +	/* Enable LPC snoop channel at requested port */
>  	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
>  	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
>  			   lpc_port << snpwadr_shift);

This duplicates a comment which is already present in the driver a few
lines before.

This duplicated comment gets cleaned up later in patch 10/10 (soc:
aspeed: lpc-snoop: Lift channel config to const structs).

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 10/10] soc: aspeed: lpc-snoop: Lift channel config to const structs

[Jean Delvare]

Hi Andrew,

On Mon, 16 Jun 2025 22:43:47 +0930, Andrew Jeffery wrote:
> The shifts and masks for each channel are defined by hardware and
> are not something that changes at runtime. Accordingly, describe the
> information in an array of const structs and associate elements with
> each channel instance, removing the need for the switch and handling of
> its default case.

I like this.

Note that technically, the removal of the default case in the switch
was already possible since patch 06/10 (soc: aspeed: lpc-snoop:
Constrain parameters in channel paths).

> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 100 +++++++++++++++-------------------
>  1 file changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 9f88c5471b1b6d85f6d9e1970240f3d1904d166c..2d97b8d5fb429e215c321c9c2ee3fa35d39f8618 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -63,7 +63,16 @@ enum aspeed_lpc_snoop_index {
>  	ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
>  };
>  
> +struct aspeed_lpc_snoop_channel_cfg {
> +	enum aspeed_lpc_snoop_index index;
> +	u32 hicr5_en;
> +	u32 snpwadr_mask;
> +	u32 snpwadr_shift;
> +	u32 hicrb_en;
> +};
> +
>  struct aspeed_lpc_snoop_channel {
> +	const struct aspeed_lpc_snoop_channel_cfg *cfg;
>  	bool enabled;
>  	struct kfifo		fifo;
>  	wait_queue_head_t	wq;
> @@ -77,6 +86,23 @@ struct aspeed_lpc_snoop {
>  	struct aspeed_lpc_snoop_channel chan[ASPEED_LPC_SNOOP_INDEX_MAX + 1];
>  };
>  
> +static const struct aspeed_lpc_snoop_channel_cfg channel_cfgs[ASPEED_LPC_SNOOP_INDEX_MAX + 1] = {
> +	{
> +		.index = ASPEED_LPC_SNOOP_INDEX_0,
> +		.hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
> +		.snpwadr_mask = SNPWADR_CH0_MASK,
> +		.snpwadr_shift = SNPWADR_CH0_SHIFT,
> +		.hicrb_en = HICRB_ENSNP0D,
> +	},
> +	{
> +		.index = ASPEED_LPC_SNOOP_INDEX_1,
> +		.hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W,
> +		.snpwadr_mask = SNPWADR_CH1_MASK,
> +		.snpwadr_shift = SNPWADR_CH1_SHIFT,
> +		.hicrb_en = HICRB_ENSNP1D,
> +	},
> +};
> +
>  static struct aspeed_lpc_snoop_channel *snoop_file_to_chan(struct file *file)
>  {
>  	return container_of(file->private_data,
> @@ -189,28 +215,27 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
>  }
>  
>  __attribute__((nonnull))
> -static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
> -				   struct device *dev,
> -				   enum aspeed_lpc_snoop_index index, u16 lpc_port)
> +static int aspeed_lpc_enable_snoop(struct device *dev,
> +				    struct aspeed_lpc_snoop *lpc_snoop,
> +				    struct aspeed_lpc_snoop_channel *channel,
> +				    const struct aspeed_lpc_snoop_channel_cfg *cfg,
> +				    u16 lpc_port)
>  {

I'm confused by this new calling convention. With lpc_snoop and index,
you could already retrieve the aspeed_lpc_snoop_channel struct and the
aspeed_lpc_snoop_channel_cfg struct. I can't see the benefit of the
change. It even forces you to add an index field to struct
aspeed_lpc_snoop_channel_cfg, which would otherwise not be needed.

If you prefer to pass cfg instead of index as a parameter, that does
not imply passing channel too. You can get the index from the cfg (if
you decide to keep it in that struct), and then the channel from index.

Or you could even pass only the channel (to be consistent with
aspeed_lpc_disable_snoop), if you set channel->cfg before calling this
function. Again this implies keeping index in struct
aspeed_lpc_snoop_channel_cfg.

>  	const struct aspeed_lpc_snoop_model_data *model_data;
> -	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
> -	struct aspeed_lpc_snoop_channel *channel;
>  	int rc = 0;
>  
> -	channel = &lpc_snoop->chan[index];
> -
>  	if (WARN_ON(channel->enabled))
>  		return -EBUSY;
>  
>  	init_waitqueue_head(&channel->wq);
>  
> +	channel->cfg = cfg;
>  	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
>  	channel->miscdev.fops = &snoop_fops;
>  	channel->miscdev.parent = dev;
>  
>  	channel->miscdev.name =
> -		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
> +		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, cfg->index);
>  	if (!channel->miscdev.name)
>  		return -ENOMEM;
>  
> @@ -223,39 +248,18 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  		goto err_free_fifo;
>  
>  	/* Enable LPC snoop channel at requested port */
> -	switch (index) {
> -	case 0:
> -		hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W;
> -		snpwadr_mask = SNPWADR_CH0_MASK;
> -		snpwadr_shift = SNPWADR_CH0_SHIFT;
> -		hicrb_en = HICRB_ENSNP0D;
> -		break;
> -	case 1:
> -		hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W;
> -		snpwadr_mask = SNPWADR_CH1_MASK;
> -		snpwadr_shift = SNPWADR_CH1_SHIFT;
> -		hicrb_en = HICRB_ENSNP1D;
> -		break;
> -	default:
> -		rc = -EINVAL;
> -		goto err_misc_deregister;
> -	}
> -
> -	/* Enable LPC snoop channel at requested port */
> -	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
> -	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
> -			   lpc_port << snpwadr_shift);
> +	regmap_set_bits(lpc_snoop->regmap, HICR5, cfg->hicr5_en);
> +	regmap_update_bits(lpc_snoop->regmap, SNPWADR, cfg->snpwadr_mask,
> +		lpc_port << cfg->snpwadr_shift);

It is a good practice to align the second line on the opening
parenthesis of the first line (as was done originally).

>  
>  	model_data = of_device_get_match_data(dev);
>  	if (model_data && model_data->has_hicrb_ensnp)
> -		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
> +		regmap_set_bits(lpc_snoop->regmap, HICRB, cfg->hicrb_en);
>  
>  	channel->enabled = true;
>  
>  	return 0;
>  
> -err_misc_deregister:
> -	misc_deregister(&channel->miscdev);
>  err_free_fifo:
>  	kfifo_free(&channel->fifo);
>  	return rc;
> @@ -263,30 +267,13 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  
>  __attribute__((nonnull))
>  static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
> -				     enum aspeed_lpc_snoop_index index)
> +				     struct aspeed_lpc_snoop_channel *channel)
>  {
> -	struct aspeed_lpc_snoop_channel *channel;
> -
> -	channel = &lpc_snoop->chan[index];
> -
>  	if (!channel->enabled)
>  		return;
>  
>  	/* Disable interrupts along with the device */
> -	switch (index) {
> -	case 0:
> -		regmap_update_bits(lpc_snoop->regmap, HICR5,
> -				   HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
> -				   0);
> -		break;
> -	case 1:
> -		regmap_update_bits(lpc_snoop->regmap, HICR5,
> -				   HICR5_EN_SNP1W | HICR5_ENINT_SNP1W,
> -				   0);
> -		break;
> -	default:
> -		return;
> -	}
> +	regmap_clear_bits(lpc_snoop->regmap, HICR5, channel->cfg->hicr5_en);
>  
>  	channel->enabled = false;
>  	/* Consider improving safety wrt concurrent reader(s) */
> @@ -299,8 +286,8 @@ static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
>  	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
>  
>  	/* Disable both snoop channels */
> -	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
> -	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
> +	aspeed_lpc_disable_snoop(lpc_snoop, &lpc_snoop->chan[0]);
> +	aspeed_lpc_disable_snoop(lpc_snoop, &lpc_snoop->chan[1]);
>  }
>  
>  static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
> @@ -339,6 +326,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> +	static_assert(ARRAY_SIZE(channel_cfgs) == ARRAY_SIZE(lpc_snoop->chan),
> +		"Broken implementation assumption regarding cfg count");

Both also need to be equal to ASPEED_LPC_SNOOP_INDEX_MAX + 1, right?
Otherwise the loop below would break. But it turns out that both arrays
are now declared that way, so it just has to be true. This makes me
believe that this static assert is no longer needed.

>  	for (idx = ASPEED_LPC_SNOOP_INDEX_0; idx <= ASPEED_LPC_SNOOP_INDEX_MAX; idx++) {
>  		u32 port;
>  
> @@ -346,7 +335,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  		if (rc)
>  			break;
>  
> -		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, idx, port);
> +		rc = aspeed_lpc_enable_snoop(dev, lpc_snoop, &lpc_snoop->chan[idx],
> +					     &channel_cfgs[idx], port);
>  		if (rc)
>  			goto cleanup_channels;
>  	}
> 


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Jean Delvare]

On Mon, 16 Jun 2025 22:43:41 +0930, Andrew Jeffery wrote:
> Ensure pointers and the channel index are valid before use.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -25,7 +25,6 @@
>  
>  #define DEVICE_NAME	"aspeed-lpc-snoop"
>  
> -#define NUM_SNOOP_CHANNELS 2
>  #define SNOOP_FIFO_SIZE 2048
>  
>  #define HICR5	0x80
> @@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
>  	unsigned int has_hicrb_ensnp;
>  };
>  
> +enum aspeed_lpc_snoop_index {
> +	ASPEED_LPC_SNOOP_INDEX_0 = 0,
> +	ASPEED_LPC_SNOOP_INDEX_1 = 1,
> +	ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
> +};

I don't have a strong opinion on this (again, I'm neither the driver
maintainer nor the subsystem maintainer so my opinion has little
value), but IMHO the main value of introducing an enum here was to make
it possible to get rid of the default statement in the switch
constructs. With switch constructs being gone in patch 10/10 (soc:
aspeed: lpc-snoop: Lift channel config to const structs), the value of
this enum seems pretty low now. You could use NUM_SNOOP_CHANNELS
instead of ASPEED_LPC_SNOOP_INDEX_MAX + 1 and 0 and 1 instead of
ASPEED_LPC_SNOOP_INDEX_0 and ASPEED_LPC_SNOOP_INDEX_1, respectively,
and the code would work just the same, while being more simple, with no
downside that I can see.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Andrew Jeffery]

Hi Jean,

On Fri, 2025-07-04 at 18:44 +0200, Jean Delvare wrote:
> On Mon, 16 Jun 2025 22:43:41 +0930, Andrew Jeffery wrote:
> > Ensure pointers and the channel index are valid before use.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -25,7 +25,6 @@
> >  
> >  #define DEVICE_NAME    "aspeed-lpc-snoop"
> >  
> > -#define NUM_SNOOP_CHANNELS 2
> >  #define SNOOP_FIFO_SIZE 2048
> >  
> >  #define HICR5  0x80
> > @@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
> >         unsigned int has_hicrb_ensnp;
> >  };
> >  
> > +enum aspeed_lpc_snoop_index {
> > +       ASPEED_LPC_SNOOP_INDEX_0 = 0,
> > +       ASPEED_LPC_SNOOP_INDEX_1 = 1,
> > +       ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
> > +};
> 
> I don't have a strong opinion on this (again, I'm neither the driver
> maintainer nor the subsystem maintainer so my opinion has little
> value), but IMHO the main value of introducing an enum here was to make
> it possible to get rid of the default statement in the switch
> constructs. With switch constructs being gone in patch 10/10 (soc:
> aspeed: lpc-snoop: Lift channel config to const structs), the value of
> this enum seems pretty low now. You could use NUM_SNOOP_CHANNELS
> instead of ASPEED_LPC_SNOOP_INDEX_MAX + 1 and 0 and 1 instead of
> ASPEED_LPC_SNOOP_INDEX_0 and ASPEED_LPC_SNOOP_INDEX_1, respectively,
> and the code would work just the same, while being more simple, with no
> downside that I can see.
> 

Yeah, I agonised over it a bit before posting. However, I'm on leave,
and I'd like to draw a line under this series. This patch is in the
middle of it, and I'd rather not disrupt it too much and go around
again with a v3. I'm going to keep the enum for now, but if I need to
tidy up the driver down again the track I'll reconsider its worth.

Andrew

Re: [PATCH v2 06/10] soc: aspeed: lpc-snoop: Rearrange channel paths

[Andrew Jeffery]

Hi Jean,

On Fri, 2025-07-04 at 17:34 +0200, Jean Delvare wrote:
> On Mon, 16 Jun 2025 22:43:43 +0930, Andrew Jeffery wrote:
> > Order assignments such that tests for conditions not involving resource
> > acquisition are ordered before those testing acquired resources, and
> > order managed resource acquisition before unmanaged where possible. This
> > way we minimise the amount of manual cleanup required.
> > 
> > In the process, improve readability of the code by introducing a channel
> > pointer that takes the place of the repeated object lookups.
> > 
> > Acked-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 ++++++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 22 deletions(-)
> > (...)
> > @@ -238,6 +240,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
> >                 goto err_misc_deregister;
> >         }
> >  
> > +       /* Enable LPC snoop channel at requested port */
> >         regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
> >         regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
> >                            lpc_port << snpwadr_shift);
> 
> This duplicates a comment which is already present in the driver a few
> lines before.
> 
> This duplicated comment gets cleaned up later in patch 10/10 (soc:
> aspeed: lpc-snoop: Lift channel config to const structs).
> 

Thanks, I've dropped the duplicate in the process of applying the
patches.

Andrew

Re: [PATCH v2 09/10] soc: aspeed: lpc-snoop: Consolidate channel initialisation

[Andrew Jeffery]

Hi Jean,

On Fri, 2025-07-04 at 17:13 +0200, Jean Delvare wrote:

> 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 +++++++++++++++++------------------
> >  1 file changed, 24 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index 8dbc9d4158b89f23bda340f060d205a29bbb43c3..9f88c5471b1b6d85f6d9e1970240f3d1904d166c 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -294,12 +294,21 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
> >         kfifo_free(&channel->fifo);
> >  }
> >  
> > +static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
> > +{
> > +       struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
> > +
> > +       /* Disable both snoop channels */
> > +       aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
> > +       aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
> 
> For consistency with the probe function, I think it would make sense to
> use a for loop here as well, instead of hard-coding the channel number
> to 2. That way, no change will be needed if a future device supports
> more than 2 channels.

You're right, but for now I'm not bothered by it. I'm going to leave it
as is, as the motivation for the loop in the probe() path was to
consolidate the logic required for both channels. This one is an easy
thing to fix down the track.

> 
> None if this is blocking though, so:
> 
> Acked-by: Jean Delvare <jdelvare@suse.de>
> 

Thanks.

Andrew

Re: [PATCH v2 10/10] soc: aspeed: lpc-snoop: Lift channel config to const structs

[Andrew Jeffery]

Hi Jean,

On Fri, 2025-07-04 at 18:23 +0200, Jean Delvare wrote:
> 
> > @@ -189,28 +215,27 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
> >  }
> >  
> >  __attribute__((nonnull))
> > -static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
> > -                                  struct device *dev,
> > -                                  enum aspeed_lpc_snoop_index index, u16 lpc_port)
> > +static int aspeed_lpc_enable_snoop(struct device *dev,
> > +                                   struct aspeed_lpc_snoop *lpc_snoop,
> > +                                   struct aspeed_lpc_snoop_channel *channel,
> > +                                   const struct aspeed_lpc_snoop_channel_cfg *cfg,
> > +                                   u16 lpc_port)
> >  {
> 
> I'm confused by this new calling convention. With lpc_snoop and index,
> you could already retrieve the aspeed_lpc_snoop_channel struct and the
> aspeed_lpc_snoop_channel_cfg struct. I can't see the benefit of the
> change. 
> 

My motivation for this choice was to isolate the association between
indexes into the arrays to the call-site of aspeed_lpc_enable_snoop(),
rather than have that information spread through the implementation.

I considered the approaches you outline next before posting v2, so
while they have their merits as well, I'm going to chalk this one up to
personal preference on my part.

> It even forces you to add an index field to struct
> aspeed_lpc_snoop_channel_cfg, which would otherwise not be needed.
> 
> If you prefer to pass cfg instead of index as a parameter, that does
> not imply passing channel too. You can get the index from the cfg (if
> you decide to keep it in that struct), and then the channel from index.
> 
> Or you could even pass only the channel (to be consistent with
> aspeed_lpc_disable_snoop), if you set channel->cfg before calling this
> function. Again this implies keeping index in struct
> aspeed_lpc_snoop_channel_cfg.

*snip*

> 
> > -
> > -       /* Enable LPC snoop channel at requested port */
> > -       regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
> > -       regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
> > -                          lpc_port << snpwadr_shift);
> > +       regmap_set_bits(lpc_snoop->regmap, HICR5, cfg->hicr5_en);
> > +       regmap_update_bits(lpc_snoop->regmap, SNPWADR, cfg->snpwadr_mask,
> > +               lpc_port << cfg->snpwadr_shift);
> 
> It is a good practice to align the second line on the opening
> parenthesis of the first line (as was done originally).

Thanks, I've fixed this up.

*snip*

> >  
> >  static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
> > @@ -339,6 +326,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
> >         if (rc)
> >                 return rc;
> >  
> > +       static_assert(ARRAY_SIZE(channel_cfgs) == ARRAY_SIZE(lpc_snoop->chan),
> > +               "Broken implementation assumption regarding cfg count");
> 
> Both also need to be equal to ASPEED_LPC_SNOOP_INDEX_MAX + 1, right?
> Otherwise the loop below would break. But it turns out that both arrays
> are now declared that way, so it just has to be true. This makes me
> believe that this static assert is no longer needed.

My intent was to convey that we require the arrays to be the same
length, as opposed to being declared such that they happen to have the
same length. It's a property of the design rather than the
implementation. All static_assert()s should be obviously true; IMO
their purpose is to communicate requirements and constrain change.

With the view to getting these patches applied I intend to keep it.

Thanks,

Andrew