Messages in this thread | | | Date | Fri, 24 Jun 2022 14:23:04 +0300 | From | Vladimir Oltean <> | Subject | Re: [Patch net-next 00/13] net: dsa: microchip: common spi probe for the ksz series switches - part 2 |
| |
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)?
| |