Re: [PATCH net-next v25 04/10] mfd: an8855: Add support for Airoha AN8855 Switch

[Christian Marangi <ansuelsmth@gmail.com>]

On Mon, Oct 19, 2026 at 05:44:30PM +0100, Lee Jones wrote:
> On Mon, 12 Oct 2026, Christian Marangi wrote:
> 
> > On Sat, Oct 10, 2026 at 04:11:40PM +0000, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > > - [Medium] mfd: overlapping regmap readable ranges for General Purpose and GPU timers
> > > - [Low] mfd: structure and variable naming violates MFD API guidelines
> > > --
> > > 
> > > commit 05d4dc95b969f0fcf66d11398aafad3e7eb7c7f1
> > > Author: Christian Marangi <ansuelsmth@gmail.com>
> > > 
> > > mfd: an8855: Add support for Airoha AN8855 Switch
> > > 
> > > This commit adds support for the Airoha AN8855 Switch which provides
> > > a DSA switch and a NVMEM provider. It utilizes the mdio-regmap driver
> > > and registers a regmap for each internal PHY.
> > > 
> > > > diff --git a/drivers/mfd/airoha-an8855.c b/drivers/mfd/airoha-an8855.c
> > > > new file mode 100644
> > > > index 0000000000000..a2778511dfefc
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/airoha-an8855.c
> > > [ ... ]
> > > > +static const struct regmap_range an8855_readable_ranges[] = {
> > > [ ... ]
> > > > +	regmap_reg_range(0x10009000, 0x10009fff), /* PDMA */
> > > > +	regmap_reg_range(0x1000a100, 0x1000a2ff), /* General Purpose Timer */
> > > > +	regmap_reg_range(0x1000a200, 0x1000a2ff), /* GPU timer */
> > > 
> > > [Severity: Medium]
> > > Is this range definition correct? The General Purpose Timer range
> > > (0x1000a100 to 0x1000a2ff) fully overlaps with the GPU timer range
> > > (0x1000a200 to 0x1000a2ff).
> > > 
> > > Could this be a typo in the hardware memory map definition? It seems like
> > > the General Purpose Timer range might have been intended to end at
> > > 0x1000a1ff to avoid overlapping and potential register access boundary
> > > issues.
> > 
> > I have to double check this from documentation, but if possible I would
> > like to fix this with a followup patch.
> >  
> > > [ ... ]
> > > > +static int an8855_core_probe(struct mdio_device *mdiodev)
> > > > +{
> > > > +	struct an8855_phy_bus phy_bus_infos[AN8855_MAX_PHY_PORT] = { };
> > > > +	struct device *dev = &mdiodev->dev;
> > > > +	struct an8855_core *priv;
> > > > +	struct regmap *regmap;
> > > > +	int num_phys;
> > > > +	int i, ret;
> > > > +
> > > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > 
> > > [Severity: Low]
> > > This isn't a bug, but does this variable and structure naming align with MFD
> > > subsystem conventions?
> > > 
> > > The MFD guidelines suggest avoiding generic variable names like priv, and
> > > instead recommend using ddata for the instance variable. Also, should the
> > > struct name itself avoid hardcoding implementation details like core?
> > > Naming it something closer to struct an8855 might better adhere to the
> > > subsystem guidelines.
> > > 
> > 
> > I already updated the struct, is it needed to also update the local
> > variables?
> 
> Ah, I was just about to apply this!
> 
> This would be preferred:
> 
>   struct an8855 *ddata;
>

Ahhhhhh! You got me!

Do you think there might be a faster method to submit this so I can quickly
rename the variable and you can pick it? 

-- 
	Ansuel