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

[Jean Delvare <jdelvare@suse.de>]

Hi Andrew,

On Fri, 11 Apr 2025 10:38:37 +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 the idea very much. A few comments on the implementations below.

> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 82 +++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 0b2044fd79b1be08dfa33bfcaf249b020c909bb9..b54d8fbf7b83ebadd4fe1b16cbddf07a0bfac868 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -10,6 +10,7 @@
>   * 0x80 writes made by the BIOS during the boot process.
>   */
>  
> +#include "linux/ratelimit.h"
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/interrupt.h>
> @@ -57,7 +58,15 @@ struct aspeed_lpc_snoop_model_data {
>  	unsigned int has_hicrb_ensnp;
>  };
>  
> +struct aspeed_lpc_snoop_channel_cfg {
> +	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;
> @@ -188,7 +197,6 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  				   int index, 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;
>  
> @@ -200,6 +208,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	if (channel->enabled)
>  		return -EBUSY;
>  
> +	if (WARN_ONCE(!channel->cfg, "snoop channel %d lacks required config", index))

Why not just WARN? WARN_ONCE has a higher cost, and I don't expect this
code path to be taken more than twice.

> +		return -EINVAL;
> +
>  	init_waitqueue_head(&channel->wq);
>  
>  	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
> @@ -220,39 +231,20 @@ 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 */

Strange that you discard a comment which you added yourself in the
previous patch.

> -	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_update_bits(lpc_snoop->regmap, HICR5, channel->cfg->hicr5_en,
> +		channel->cfg->hicr5_en);

Not caused by your patch, but I think regmap_set_bits() could be used
here to improve readability.

> +	regmap_update_bits(lpc_snoop->regmap, SNPWADR, channel->cfg->snpwadr_mask,
> +		lpc_port << channel->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_update_bits(lpc_snoop->regmap, HICRB, channel->cfg->hicrb_en,
> +			channel->cfg->hicrb_en);

Could also use regmap_set_bits().

>  
>  	channel->enabled = true;
>  
>  	return 0;
>  
> -err_misc_deregister:
> -	misc_deregister(&lpc_snoop->chan[index].miscdev);
>  err_free_fifo:
>  	kfifo_free(&lpc_snoop->chan[index].fifo);
>  	return rc;
> @@ -272,21 +264,7 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	if (!channel->enabled)
>  		return;
>  
> -	/* Disable interrupts along with the device */

Any reason for killing this poor innocent comment? ^^

> -	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_update_bits(lpc_snoop->regmap, HICR5, channel->cfg->hicr5_en, 0);

Could use regmap_clear_bits() if I'm not mistaken.

>  
>  	channel->enabled = false;
>  	/* Consider improving safety wrt concurrent reader(s) */
> @@ -294,6 +272,21 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	kfifo_free(&channel->fifo);
>  }
>  
> +static const struct aspeed_lpc_snoop_channel_cfg channel_cfgs[] = {
> +	{
> +		.hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
> +		.snpwadr_mask = SNPWADR_CH0_MASK,
> +		.snpwadr_shift = SNPWADR_CH0_SHIFT,
> +		.hicrb_en = HICRB_ENSNP0D,
> +	},
> +	{
> +		.hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W,
> +		.snpwadr_mask = SNPWADR_CH1_MASK,
> +		.snpwadr_shift = SNPWADR_CH1_SHIFT,
> +		.hicrb_en = HICRB_ENSNP1D,
> +	},
> +};
> +
>  static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  {
>  	struct aspeed_lpc_snoop *lpc_snoop;
> @@ -308,6 +301,13 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>  	if (!lpc_snoop)
>  		return -ENOMEM;
>  
> +	static_assert(ARRAY_SIZE(channel_cfgs) == ARRAY_SIZE(lpc_snoop->chan),
> +		"Broken implementation assumption regarding cfg count");
> +	static_assert(ARRAY_SIZE(lpc_snoop->chan) == 2,
> +		"Broken implementation assumption regarding channel count");

Wouldn't it be good (and maybe sufficient) to declare
aspeed_lpc_snoop_channel_cfg as channel_cfgs[NUM_SNOOP_CHANNELS]?

If you insist on keeping the second assert then you should at least use
NUM_SNOOP_CHANNELS instead of hard-coding 2.

> +	lpc_snoop->chan[0].cfg = &channel_cfgs[0];
> +	lpc_snoop->chan[1].cfg = &channel_cfgs[1];

Could this be done at the beginning of aspeed_lpc_enable_snoop()? So
that you don't have to duplicate the statement (and don't set
lpc_snoop->chan[1].cfg if there's no second port). Would save a WARN as
well.

> +
>  	np = pdev->dev.parent->of_node;
>  	if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
>  	    !of_device_is_compatible(np, "aspeed,ast2500-lpc-v2") &&
> 


-- 
Jean Delvare
SUSE L3 Support