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

[Christian Marangi <ansuelsmth@gmail.com>]

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?

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261009161102.38961-1-ansuelsmth@gmail.com?part=4

-- 
	Ansuel