From: Byungho An <bh74.an@samsung•com>
To: 'Tomasz Figa' <tomasz.figa@gmail•com>,
netdev@vger•kernel.org, linux-samsung-soc@vger•kernel.org,
devicetree@vger•kernel.org
Cc: 'David Miller' <davem@davemloft•net>,
'GIRISH K S' <ks.giri@samsung•com>,
'SIVAREDDY KALLAM' <siva.kallam@samsung•com>,
'Vipul Chandrakant' <vipul.pandya@samsung•com>,
'Ilho Lee' <ilho215.lee@samsung•com>
Subject: RE: [PATCH V11 2/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Date: Sat, 22 Mar 2014 14:55:43 -0700 [thread overview]
Message-ID: <008801cf4619$7b443cb0$71ccb610$@samsung.com> (raw)
In-Reply-To: <532D9032.1040209@gmail.com>
Tomasz Figa <tomasz.figa@gmail•com> :
> Hi,
>
> I have reviewed the non-net-specific parts of this driver, e.g. platform
driver
> and Device Tree code. Please see my comments inline.
>
> On 22.03.2014 07:23, Byungho An wrote:
> > From: Siva Reddy <siva.kallam@samsung•com>
> >
> > This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
> >
[snip]
> > + struct device_node *np = pdev->dev.of_node;
> > + struct sxgbe_dma_cfg *dma_cfg;
> > +
> > + if (!np)
> > + return -ENODEV;
> > +
> > + *mac = of_get_mac_address(np);
> > + plat->interface = of_get_phy_mode(np);
> > +
> > + plat->bus_id = of_alias_get_id(np, "ethernet");
> > + if (plat->bus_id < 0)
> > + plat->bus_id = 0;
> > +
> > + plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> > + sizeof(struct sxgbe_mdio_bus_data),
> > + GFP_KERNEL);
>
> If plat->mdio_bus_data is assumed to be of the same type as the data
> allocated here, then the following would be preferred:
>
> sizeof(*plat->mdio_bus_data)
>
> Also you should probably check for allocation failure.
OK and it is same type.
>
> > +
> > + dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
> GFP_KERNEL);
> > + if (!dma_cfg)
> > + return -ENOMEM;
> > +
> > + plat->dma_cfg = dma_cfg;
> > + of_property_read_u32(np, "samsung,pbl", &dma_cfg->pbl);
> > + if (of_property_read_u32(np, "samsung,burst-map", &dma_cfg-
> >burst_map) == 0)
> > + dma_cfg->fixed_burst = true;
> > +
> > + return 0;
> > +}
>
> [snip]
>
> > +static int sxgbe_platform_probe(struct platform_device *pdev) {
> > + int ret;
> > + int loop = 0;
> > + int i, chan;
> > + struct resource *res;
> > + struct device *dev = &pdev->dev;
> > + void __iomem *addr;
> > + struct sxgbe_priv_data *priv = NULL;
> > + struct sxgbe_plat_data *plat_dat = NULL;
> > + const char *mac = NULL;
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct device_node *node = dev->of_node;
> > +
> > + /* Get memory resource */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENODEV;
> > +
> > + addr = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(addr))
> > + return PTR_ERR(addr);
> > +
> > + if (pdev->dev.of_node) {
> > + plat_dat = devm_kzalloc(&pdev->dev,
> > + sizeof(struct sxgbe_plat_data),
> > + GFP_KERNEL);
> > + if (!plat_dat)
> > + return -ENOMEM;
> > +
> > + ret = sxgbe_probe_config_dt(pdev, plat_dat, &mac);
> > + if (ret) {
> > + pr_err("%s: main dt probe failed\n", __func__);
> > + return ret;
> > + }
> > + }
> > +
> > + priv = sxgbe_drv_probe(&(pdev->dev), plat_dat, addr);
> > + if (!priv) {
> > + pr_err("%s: main driver probe failed\n", __func__);
> > + return -ENODEV;
> > + }
> > +
> > + /* Get MAC address if available (DT) */
> > + if (mac)
> > + ether_addr_copy(priv->dev->dev_addr, mac);
> > +
> > + /* Get the SXGBE common INT information */
> > + priv->irq = platform_get_irq(pdev, loop++);
>
> The name "loop" of the variable is quite misleading here. Probably something
> like "irq_num", would be more meaningful.
>
> Anyway, it doesn't look like it's used anywhere else in this function, so
> platform_get_irq(pdev, 0) could be simply used.
>
> > + if (priv->irq <= 0) {
> > + dev_err(dev, "sxgbe common irq parsing failed\n");
> > + sxgbe_drv_remove(ndev);
> > + return -EINVAL;
> > + }
> > +
> > + /* Get the TX/RX IRQ numbers */
> > + for (i = 0, chan = 0; i < SXGBE_TX_QUEUES; i++) {
> > + priv->txq[i]->irq_no = irq_of_parse_and_map(node, chan++);
>
> Hmm, this call looks suspicious. The "chan" variable starts here as 0 and so
the
> first call to irq_of_parse_and_map() will end up with parsing the first
(zeroth)
> entry of "interrupts" property, which would be the same as returned by
> platform_get_irq(..., 0) above. Maybe this was the point where the "loop"
> variable should be used?
OK. it will be chan instead of loop.
thanks I missed.
>
> Anyway, why you couldn't simply use platform_get_irq() here as well?
I'll change platform_get_irq to irq_of_parse_and_map because latter can
support PCI and nonPCI
>
> > + if (priv->txq[i]->irq_no <= 0) {
> > + dev_err(dev, "sxgbe tx irq parsing failed\n");
>
> Shouldn't you do some clean-up here, like calling sxgbe_drv_remove()?
OK. I'll add it
> Maybe moving the call to sxgbe_drv_probe() after all the resources are
> successfully retrieved would be a better idea?
OK, I'll consider.
>
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + for (i = 0; i < SXGBE_RX_QUEUES; i++) {
> > + priv->rxq[i]->irq_no = irq_of_parse_and_map(node, chan++);
> > + if (priv->rxq[i]->irq_no <= 0) {
> > + dev_err(dev, "sxgbe rx irq parsing failed\n");
>
> Same comments as for TX IRQs above.
OK.
>
> Best regards,
> Tomasz
next prev parent reply other threads:[~2014-03-22 21:55 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-22 6:23 [PATCH V11 2/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver Byungho An
2014-03-22 11:45 ` Francois Romieu
2014-03-22 21:12 ` Byungho An
2014-03-22 13:29 ` Tomasz Figa
2014-03-22 21:55 ` Byungho An [this message]
2014-03-22 21:59 ` Tomasz Figa
2014-03-22 23:15 ` Byungho An
2014-03-22 19:01 ` Vince Bridgers
2014-03-22 20:04 ` Joe Perches
2014-03-23 0:39 ` Byungho An
2014-03-22 19:39 ` Vince Bridgers
2014-03-22 22:23 ` Byungho An
2014-03-22 19:50 ` Vince Bridgers
2014-03-22 20:07 ` Vince Bridgers
2014-03-23 0:39 ` Byungho An
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='008801cf4619$7b443cb0$71ccb610$@samsung.com' \
--to=bh74.an@samsung$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=devicetree@vger$(echo .)kernel.org \
--cc=ilho215.lee@samsung$(echo .)com \
--cc=ks.giri@samsung$(echo .)com \
--cc=linux-samsung-soc@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=siva.kallam@samsung$(echo .)com \
--cc=tomasz.figa@gmail$(echo .)com \
--cc=vipul.pandya@samsung$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox