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

[Jean Delvare <jdelvare@suse.de>]

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