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

[Lee Jones <lee@kernel.org>]

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;

-- 
Lee Jones