netdev - Re: [PATCH v2 3/5] drivers: net: phy: Add MDIO driver

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: <CAKh23FmaQy8J=f-eyfBeNndz0oT5R0icvwJ3vrw1DMJnj9uv_g@mail.gmail.com>
Date:	Mon, 6 Jun 2016 10:12:35 -0700
From:	Iyappan Subramanian <isubramanian@....com>
To:	Andrew Lunn <andrew@...n.ch>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, patches <patches@....com>,
	Matthias Brugger <matthias.bgg@...il.com>
Subject: Re: [PATCH v2 3/5] drivers: net: phy: Add MDIO driver

Hi Andrew,

Thanks for the review.

On Tue, May 31, 2016 at 6:11 PM, Andrew Lunn <andrew@...n.ch> wrote:
> On Tue, May 31, 2016 at 05:10:38PM -0700, Iyappan Subramanian wrote:
>> +static int xgene_mdio_reset(struct xgene_mdio_pdata *pdata)
>> +{
>> +     int ret;
>> +
>> +     if (pdata->mdio_id == XGENE_MDIO_RGMII) {
>> +             if (pdata->dev->of_node) {
>> +                     clk_prepare_enable(pdata->clk);
>> +                     clk_disable_unprepare(pdata->clk);
>> +                     clk_prepare_enable(pdata->clk);
>
> Hi Iyappan
>
> Is that a workaround for a hardware problem? If so, i would suggest
> adding a comment, to stop people submitting a patch simplifying it.

Hardware expects this clock sequence.  I'll add comment as you suggested.

>
>
>> +static int xgene_mdio_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct mii_bus *mdio_bus;
>> +     const struct of_device_id *of_id;
>> +     struct resource *res;
>> +     struct xgene_mdio_pdata *pdata;
>> +     void __iomem *csr_addr;
>> +     int mdio_id = 0, ret = 0;
>> +
>
>
>> +     of_id = of_match_device(xgene_mdio_of_match, &pdev->dev);
>> +     if (mdio_id == XGENE_MDIO_RGMII) {
>> +             mdio_bus->read = xgene_mdio_rgmii_read;
>> +             mdio_bus->write = xgene_mdio_rgmii_write;
>> +     } else {
>> +             mdio_bus->read = xgene_xfi_mdio_read;
>> +             mdio_bus->write = xgene_xfi_mdio_write;
>> +     }
>
>> +static const struct of_device_id xgene_mdio_of_match[] = {
>> +     {
>> +             .compatible = "apm,xgene-mdio-rgmii",
>> +             .data = (void *)XGENE_MDIO_RGMII
>> +     },
>> +     {
>> +             .compatible = "apm,xgene-mdio-xfi",
>> +             .data = (void *)XGENE_MDIO_XFI},
>> +     {},
>> +};
>
>
> This all makes me think you should have two separate MDIO drivers, one
> for each compatible string. There is not that much shared code.

I would like to keep the driver consistent with the ethernet driver.
Only the mdio read and write functions are hardware specific, and that
too implemented using function pointers.  Other parts of the code are
shared and much cleaner that way.

>
>     Andrew