Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register

From: Arınç ÜNAL
Date: Thu May 25 2023 - 02:21:12 EST


On 24.05.2023 19:57, Vladimir Oltean wrote:
On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal@xxxxxxxxx wrote:
From: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>

On commit 7ef6f6f8d237 ("net: dsa: mt7530: Add MT7621 TRGMII mode support")
macros for reading the crystal frequency were added under the MT7530_HWTRAP
register. However, the value given to the xtal variable on
mt7530_pad_clk_setup() is read from the MT7530_MHWTRAP register instead.

Although the document MT7621 Giga Switch Programming Guide v0.3 states that
the value can be read from both registers, use the register where the
macros were defined under.

Tested-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
---

I'm sorry, but I refuse this patch, mainly as a matter of principle -
because that's just not how we do things, and you need to understand why.

The commit title ("read XTAL value from correct register") claims that
the process of reading a field which cannot be changed by software is
any more correct when it is read from HWTRAP rather than MHWTRAP
(modified HWTRAP).

Your justification is that it's confusing to you if two registers have
the same layout, and the driver has a single set of macros to decode the
fields from both. You seem to think it's somehow not correct to decode
fields from the MHWTRAP register using macros which have just HWTRAP in
the name.

No, it doesn't confuse me that two registers share the same layout. My understanding was that the MHWTRAP register should be used for modifying the hardware trap, and the HWTRAP register should be used for reading from the hardware trap. I see that the XTAL constants were defined under the HWTRAP register so I thought it would make sense to change the code to read the XTAL values from the HWTRAP register instead. Let me know if you disagree with this.


While in this very particular case there should be no negative effect
caused by the change (*because* XTAL_FSEL is read-only), your approach
absolutely does not scale to the other situations that you will be faced
with.

If it was any other r/w field from HWTRAP vs MHWTRAP, you would have
needed to get used to that coding pattern (or do something about the
coding pattern itself), and not just decide to change the register to
what you think is correct - which is a *behavior* change and not just
a *coding style* change. You don't change the *behavior* when you don't
like the *coding style* !!! because that's not a punctual fix to your
problem.

I'm sorry that it is like this, but you can't expect the Linux kernel to
be written for the level of understanding of a beginner C programmer.
It's simply too hard for everyone to change, and much easier, and more
beneficial overall, for you to change.

I understand that you're poking sticks at everything in order to become
more familiar with the driver. That's perfectly normal and fine. But not
everything that comes as a result of that is of value for sharing back
to the mainline kernel's mailing lists.

Makes sense.


Seriously, please first share these small rewrites with someone more
senior than you, and ask for a preliminary second opinion.

Would submitting this as an RFC had been a similar action to your describing here? Because I already did that:

https://lore.kernel.org/netdev/20230421143648.87889-6-arinc.unal@xxxxxxxxxx/


As they say, "on the Internet, nobody knows you're a dog". If reviewers
don't take into account that you're a newbie with C (which is a badge
that you don't carry everywhere - because you don't have to), it's easy
for patches like this (and most of this series) to come across as:
"I have no consideration for the fact that the existing code works, and
the way in which it works, I'm just gonna rewrite all of it according to
my own sensibilities and subjective justifications, and throw baseless
words at it such as: fix this, correct that, when in fact all I'm doing
is make silly changes with no effect to the observable behavior".

Because I know that the above isn't the case, I try to be as polite as
possible expressing my frustration that I am looking at a large volume
of worthless and misguided refactoring.

I should've given more effort to explain my reasons for this patch. I disagree that the series is a large volume of worthless and misguided refactoring and am happy to discuss it patch by patch.

Arınç