netdev - Re: [PATCH net-next 2/5] net: phy: mediatek: Move LED helper functions into mtk phy lib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29352dda1b5c647c30e48fbb31e7781fdab43d9f.camel@mediatek.com>
Date: Thu, 31 Oct 2024 04:56:59 +0000
From: SkyLake Huang (黃啟澤)
	<SkyLake.Huang@...iatek.com>
To: "andrew@...n.ch" <andrew@...n.ch>
CC: "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "horms@...nel.org" <horms@...nel.org>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "dqfext@...il.com" <dqfext@...il.com>,
	Steven Liu (劉人豪) <steven.liu@...iatek.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"daniel@...rotopia.org" <daniel@...rotopia.org>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH net-next 2/5] net: phy: mediatek: Move LED helper
 functions into mtk phy lib

On Wed, 2024-10-30 at 22:46 +0100, Andrew Lunn wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +void mtk_phy_leds_state_init(struct phy_device *phydev)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < 2; ++i)
> > +             phydev->drv->led_hw_control_get(phydev, i, NULL);
> > +}
> 
> This does appear to be the same as what it is replacing, but it also
> seems odd.
> 
> Why is an init function doing a get? I assume it is to do with
> setting
> priv->led_state? But led_state is not used in setting *rules, which
> is
> what led_hw_control_get() is all about. So maybe in a follow up
> patch,
> move the actual init code out of led_hw_control_get()?
> 
> Reviewed-by: Andrew Lunn <andrew@...n.ch>
> 
>     Andrew

I think I got your point. In a follow up patch, I'll do the following
change:

[Psuedo code]
/* Currently */
mtk_phy_led_hw_ctrl_get() {
	get_led_hw_settings(); //on & blink

	set/clear &priv->led_state according to led_hw_settings

	get *rules according to led_hw_settings
}

/* Change into */
get_led_hw_settings()

mtk_phy_leds_state_init() { /* Actual led_state init code */
	get_led_hw_settings(); //on & blink

	set/clear &priv->led_state according to led_hw_settings
}

mtk_phy_led_hw_ctrl_get() {
	get_led_hw_settings() in register; //on & blink

	get *rules according to led_hw_settings
}

BRs,
Sky