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 | 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))
+		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 */
-	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);
+	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);
 
 	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 */
-	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);
 
 	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");
+	lpc_snoop->chan[0].cfg = &channel_cfgs[0];
+	lpc_snoop->chan[1].cfg = &channel_cfgs[1];
+
 	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") &&

-- 
2.39.5

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

Hi Jean,

Thanks for the response and sorry for the delayed reply, I've been on
leave.

On Thu, 2025-04-17 at 12:49 +0200, Jean Delvare wrote:
> 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..b54d8fbf7b83ebadd4fe1b16c
> > bddf07a0bfac868 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.

Happy to change it.

> 
> > +               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.

That may have been the result of shuffling the patches while organising
the series. I'll put it back.

> 
> > -       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.

Ack.

> 
> > +       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().

Ack.

> 
> >  
> >         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? ^^

None, again, might've been lost in some shuffling.

> 
> > -       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.

Agreed.

> 
> >  
> >         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.

I used the literal 2 here as it reflected the two assignments below...

> 
> > +       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.

... but implementing your suggestion here will help. Let me rework it.

Thanks,

Andrew