lkml.org 
[lkml]   [2022]   [Sep]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-09-04 23:38    [W:0.074 / U:1.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site