LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: "Modi, Geet" <geet.modi@ti.com>
Cc: "Nagalla, Hari" <hnagalla@ti.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Sharma, Vikram" <vikram.sharma@ti.com>
Subject: Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization
Date: Tue, 7 Sep 2021 16:02:44 +0200	[thread overview]
Message-ID: <YTdxBMVeqZVyO4Tf@lunn.ch> (raw)
In-Reply-To: <99232B33-1C2F-45AF-A259-0868AC7D3FBC@ti.com>

On Mon, Sep 06, 2021 at 08:51:53PM +0000, Modi, Geet wrote:
> Hi Andrew,
> 
> This feature is not used by our mainstream customers as they have additional mechanism to monitor the supply at System level. 
> 
> Hence want to keep it disable by default.

So we are slowly getting closer to a usable commit message. One that
actually makes sense.

Now, this is not really your driver, it is the Linux kernel
driver. Could somebody be using this feature? Not one of your
mainstream customer, but a Linux kernel user?

Lets look at the driver, what interrupts actually get enabled?

                misr_status |= (DP83811_RX_ERR_HF_INT_EN |
                                DP83811_MS_TRAINING_INT_EN |
                                DP83811_ANEG_COMPLETE_INT_EN |
                                DP83811_ESD_EVENT_INT_EN |
                                DP83811_WOL_INT_EN |
                                DP83811_LINK_STAT_INT_EN |
                                DP83811_ENERGY_DET_INT_EN |
                                DP83811_LINK_QUAL_INT_EN);

		misr_status |= (DP83811_JABBER_DET_INT_EN |
                                DP83811_POLARITY_INT_EN |
                                DP83811_SLEEP_MODE_INT_EN |
                                DP83811_OVERTEMP_INT_EN |
                                DP83811_OVERVOLTAGE_INT_EN |
                                DP83811_UNDERVOLTAGE_INT_EN);

Some of these i have no idea what they even do. Why would i be
interested in RX_ERR_HF_INT_EN or MS_TRAINING_INT_EN?
ESD_EVENT_INT_EN? Do we need to wake up the phy driver and update the
status because these interrupts have fired?

ANEG_COMPLETE_INT_EN, ANEG_COMPLETE_INT_EN, LINK_STAT_INT_EN make
sense.

LINK_QUAL_INT_EN and POLARITY_INT_EN could in theory make sense, but
the driver is missing the code to report quality and MDIX/MDX. 

If the driver ever gets HWMON support, OVERTEMP_INT_EN,
OVERVOLTAGE_INT_EN and UNDERVOLTAGE_INT_EN could be interesting. But
there is no HWMON support.

So it looks like there are lots of interrupts which could be removed
because nothing happens when they fire. But then i wonder, why did you
pick just one? Does it cause some sort of problem for one of your
mainstream customers? What sort of problem? Does it affect all other
Linux users, not just your customer?

So please expand your commit message to try to answer some of these
questions.

Thanks
	Andrew

  reply	other threads:[~2021-09-07 14:02 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02 19:09 hnagalla
2021-09-02 23:23 ` Andrew Lunn
2021-09-06 20:51   ` [EXTERNAL] " Modi, Geet
2021-09-07 14:02     ` Andrew Lunn [this message]
     [not found]       ` <E61A9519-DBA6-4931-A2A0-78856819C362@ti.com>
2021-09-09 20:37         ` [EXTERNAL] " Andrew Lunn
2021-09-10  0:41           ` [EXTERNAL] " Modi, Geet
2021-09-10  1:30             ` Andrew Lunn
2021-09-10  1:42               ` [EXTERNAL] " Modi, Geet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YTdxBMVeqZVyO4Tf@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=geet.modi@ti.com \
    --cc=hnagalla@ti.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vikram.sharma@ti.com \
    --subject='Re: [EXTERNAL] Re: [PATCH] net: phy: dp83tc811: modify list of interrupts enabled at initialization' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).