Message-ID: <7c915d5b-56c9-430d-05ac-544f76966eb1@arinc9.com>
Date:   Thu, 25 May 2023 09:20:08 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Daniel Golle <daniel@...rotopia.org>,
        Andrew Lunn <andrew@...n.ch>,
        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>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Russell King <linux@...linux.org.uk>,
        Richard van Schagen <richard@...terhints.com>,
        Richard van Schagen <vschagen@...com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
        erkin.bozoglu@...ont.com, mithat.guner@...ont.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from
 correct register

On 24.05.2023 19:57, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal@...il.com wrote:
>> From: Arınç ÜNAL <arinc.unal@...nc9.com>
>>
>> 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@...nc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@...nc9.com>
>> ---
> 
> 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@arinc9.com/

> 
> 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ç

Powered by blists - more mailing lists