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

[Andrew Jeffery <andrew@codeconstruct.com.au>]

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