Message-ID: <20220718184017.o2ogalgjt6zwwhq3@skbuf>
Date:   Mon, 18 Jul 2022 21:40:17 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop
 qca8k_read/write/rmw for regmap variant

On Mon, Jul 18, 2022 at 07:55:26PM +0200, Christian Marangi wrote:
> Sure.
> When the regmap conversion was done at times, to limit patch delta it
> was suggested to keep these function. This was to not get crazy with
> eventual backports and fixes.
> 
> The logic here is:
> As we are moving these function AND the function will use regmap api
> anyway, we can finally drop them and user the regmap api directly
> instead of these additional function.
> 
> When the regmap conversion was done, I pointed out that in the future
> the driver had to be split in specific and common code and it was said
> that only at that times there was a good reason to make all these
> changes and drop these special functions.
> 
> Now these function are used by both setup function for qca8k and by
> common function that will be moved to a different file.
> 
> 
> If we really want I can skip the dropping of these function and move
> them to qca8k common code.

I don't really have a preference, I just want to understand why you want
to call regmap_read(priv->regmap) directly every time as opposed to
qca8k_read(priv) which is shorter to type and allows more stuff to fit
on one line.

I think if you run "make drivers/net/dsa/qca/qca8k.lst" and you look at
the generated code listing before and after, you'll find it is identical
(note, I haven't actually done that).

> An alternative is to keep them for qca8k specific code and migrate the
> common function to regmap api.

No, that's silly and I can't even find a reason to do that.
It's not like you're trying to create a policy to not call qca8k-common.c
functions from qca8k-8xxx.c, right? That should work just fine (in this
case, qca8k_read etc).

In fact, while typing this I realized that in your code structure,
you'll have one struct dsa_switch_ops in qca8k-8xxx.c and another one in
qca8k-ipq4019.c. But the vast majority of dsa_switch_ops are common,
with the exception of .setup() which is switch-specific, correct?

Wouldn't you consider, as an alternative, to move the dsa_switch_ops
structure to the common C file as well, and have a switch-specific
(*setup) operation in the match_data structure? Or even much better,
make the switch-specific ops as fine-grained as possible, rather than
reimplementing the entire qca8k_setup() (note, I don't know how similar
they are, but there should be as little duplication of logic as possible,
the common code should dictate what there is to do, and the switch
specific code just how to do it).

> So it's really a choice of drop these additional function or keep using
> them for the sake of not modifying too much source.
> 
> Hope it's clear now the reason of this change.

If I were to summarize your reason, it would be "because I prefer it
that way and because now is a good time", right? That's fine with me,
but I honestly didn't understand that while reading the commit message.

Powered by blists - more mailing lists