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

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

Hi Jean,

On Fri, 2025-07-04 at 17:13 +0200, Jean Delvare wrote:

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

You're right, but for now I'm not bothered by it. I'm going to leave it
as is, as the motivation for the loop in the probe() path was to
consolidate the logic required for both channels. This one is an easy
thing to fix down the track.

> 
> None if this is blocking though, so:
> 
> Acked-by: Jean Delvare <jdelvare@suse.de>
> 

Thanks.

Andrew