linux-kernel - Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

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]

Date:	Tue, 14 Jun 2016 16:24:48 +0800
From:	Li Dongpo <lidongpo@...ilicon.com>
To:	Andrew Lunn <andrew@...n.ch>
CC:	<f.fainelli@...il.com>, <robh+dt@...nel.org>,
	<mark.rutland@....com>, <davem@...emloft.net>,
	<xuejiancheng@...ilicon.com>, <netdev@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC



On 2016/6/13 21:32, Andrew Lunn wrote:
> On Mon, Jun 13, 2016 at 02:07:54PM +0800, Dongpo Li wrote:
>> This patch adds a separate driver for the MDIO interface of the
>> Hisilicon Fast Ethernet MAC.
>>
>> Reviewed-by: Jiancheng Xue <xuejiancheng@...ilicon.com>
>> Signed-off-by: Dongpo Li <lidongpo@...ilicon.com>
>> ---
>>  .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++
>>  drivers/net/phy/Kconfig                            |   8 +
>>  drivers/net/phy/Makefile                           |   1 +
>>  drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
>>  4 files changed, 196 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
>>  create mode 100644 drivers/net/phy/mdio-hisi-femac.c

[...]
>> +
> 
> Hi Dongpo
> 
> Overall this looks good. Just some minor comments
> 
>> +static int hisi_femac_mdio_wait_ready(struct mii_bus *bus)
>> +{
>> +	struct hisi_femac_mdio_data *data = bus->priv;
> 
> You could just pass data here. Your read and write functions already
> have it.
> 
Thank you, I will fix it in next patch version.

>> +	data->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(data->clk)) {
>> +		ret = -ENODEV;
>> +		goto err_out_free_mdiobus;
>> +	}
> 
> Return the error which devm_clk_get() gives you.
> 
ok, I will fix it.

>> +
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret)
>> +		goto err_out_free_mdiobus;
>> +
>> +	ret = of_mdiobus_register(bus, np);
>> +	if (ret)
>> +		goto err_out_free_mdiobus;
> 
> You leave the clock prepared and enabled on error.
> 
ok, I will fix it.

>     Andrew
> 
> .
>