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.5Hi 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
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
© 2016 - 2025 Red Hat, Inc.