Re: [Patch net-next 00/13] net: dsa: microchip: common spi probe for the ksz series switches - part 2

[Vladimir Oltean]

On Wed, Jun 22, 2022 at 02:34:12PM +0530, Arun Ramadoss wrote:
> This patch series aims to refactor the ksz_switch_register routine to have the
> common flow for the ksz series switch. And this is the follow up patch series.
> 
> First, it tries moves the common implementation in the setup from individual
> files to ksz_setup. Then implements the common dsa_switch_ops structure instead
> of independent registration. And then moves the ksz_dev_ops to ksz_common.c,
> it allows the dynamic detection of which ksz_dev_ops to be used based on
> the switch detection function.
> 
> Finally, the patch updates the ksz_spi probe function to be same for all the
> ksz_switches.

Sorry for being late to the party again. I've looked over the resulting
code and it appears that there is still some cleanup to do.

We now have a stray struct ksz8 pointer being allocated by the common
ksz_spi_probe(), and passed as dev->priv to ksz_switch_alloc() by this
generic code.

Only ksz8 accesses dev->priv, although it is interesting to note that
ksz9477_i2c_probe() calls ksz_switch_alloc() with a type-incompatible
struct i2c_client *i2c that is unused but bogus.

The concept of struct ksz8 was added by commit 9f73e11250fb ("net: dsa:
microchip: ksz8795: move register offsets and shifts to separate
struct"), and in essence it isn't a bad idea, it's just that I wasn't
aware of it, and only ksz8 makes use of it.

You've added some register offsets yourself to ksz_chip_data
(stp_ctrl_reg, broadcast_ctrl_reg, multicast_ctrl_reg, start_ctrl_reg),
and it looks like struct ksz8 shares more or less the same purpose -
regs, masks, shifts etc. Would you mind doing some more consolidation
work and trying to figure out if we could eliminate a data structure
unique for ksz8 and integrate that information into struct ksz_chip_data
(and perhaps use it in more places)?