Messages in this thread | | | Date | Sun, 4 Sep 2022 23:34:58 +0200 | From | Christian Marangi <> | Subject | Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API |
| |
On Sat, Aug 27, 2022 at 04:00:40PM +0200, Andrew Lunn wrote: > > static struct regmap_config qca8k_regmap_config = { > > - .reg_bits = 16, > > + .reg_bits = 32, > > Does this change really allow you to access more registers? > > > .val_bits = 32, > > .reg_stride = 4, > > .max_register = 0x16ac, /* end MIB - Port6 range */ > > - .reg_read = qca8k_regmap_read, > > - .reg_write = qca8k_regmap_write, > > + .read = qca8k_bulk_read, > > + .write = qca8k_bulk_write, > > .reg_update_bits = qca8k_regmap_update_bits, > > .rd_table = &qca8k_readable_table, > > .disable_locking = true, /* Locking is handled by qca8k read/write */ > > .cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */ > > + .max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */ > > + .max_raw_write = 16, > > I think the word 'bytes' in the comment is wrong. I assume you can > access 4 registers, each register is one 32-bit work in size. > > > static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb) > > { > > - u32 reg[3]; > > + u32 reg[QCA8K_ATU_TABLE_SIZE]; > > int ret; > > > > /* load the ARL table into an array */ > > - ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg)); > > + ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg, > > + QCA8K_ATU_TABLE_SIZE); > > if (ret) > > return ret; > > Please split the 3 -> QCA8K_ATU_TABLE_SIZE change out into a patch of > its own. > > Ideally you want lots of small, obviously correct patches. A change > which replaces 3 for QCA8K_ATU_TABLE_SIZE should be small and > obviously correct, meaning it is quick and easy to review, and makes > the more complex to review change smaller and also simpler to review. > > Andrew
I'm working on v3. I manage to make everything work with 16 bit. (It was set as the max reg for this switch is 0x16ac)
Anyway on second look, this change is necessary and I think split this in another patch would be redundant and problematic. (for backporting purpose and to correctly write a sane commit description)
The regmap bulk implementation take as the 4th arg the count of reg to read/write. Current implementation instead take the actual size of the array. So I think this has to be changed in the bulk conversion patch.
I can split it but it will be something like
qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, QCA8K_ATU_TABLE_SIZE * sizeof(u32));
and on the bulk conversion commit I have to drop it again to
regmap_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, QCA8K_ATU_TABLE_SIZE);
Seems a bad thing to make a change and than fix that change on the next commit.
Will wait for a response about this and propose v3 with what is the correct approach.
-- Ansuel
| |