[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151104200631.GG14575@oracle.com>
Date: Wed, 4 Nov 2015 15:06:31 -0500
From: Sowmini Varadhan <sowmini.varadhan@...cle.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
jeffrey.t.kirsher@...el.com, jesse.brandeburg@...el.com,
shannon.nelson@...el.com, carolyn.wyborny@...el.com,
donald.c.skidmore@...el.com, matthew.vick@...el.com,
john.ronciak@...el.com, mitch.a.williams@...el.com
Subject: Re: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM
On (11/04/15 21:59), Andy Shevchenko wrote:
>
> Usually the structure of kernel doc is something like following
>
> /**
> * func - summary
> * @paramx: desc
> *
> * Description:
> * Long description in many lines and / or paragraphs
> *
> * Returns:
> * 0 on success or errno otherwise.
> */
>
>
> > + **/
>
> No need two stars.
I was actually following the exact comment style of
the function just before i40e_macaddr_init, namely:;
/**
* i40e_vsi_setup - Set up a VSI by a given type
* @pf: board private structure
* @type: VSI type
* @uplink_seid: the switch element to link to
* @param1: usage depends upon VSI type. For VF types, indicates VF id
*
* This allocates the sw VSI structure and its queue resources, then add a VSI
* to the identified VEB.
*
* Returns pointer to the successfully allocated and configure VSI sw struct on
* success, otherwise returns NULL on failure.
**/
struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
u16 uplink_seid, u32 param1)
So I'm not sure we need to really bike-shed this one?
> > + macaddr, NULL);
> > + if (ret) {
> > + dev_info(&vsi->back->pdev->dev,
> > + "Addr change for VSI failed: %d\n", ret);
>
> dev_err() or dev_warn() I would say.
again, this was a cut/paste of code from i40e_set_mac()
which does netdev_info.
> > + ret = i40e_aq_add_macvlan(&vsi->back->hw, vsi->seid, &element, 1, NULL);
> > + aq_err = vsi->back->hw.aq.asq_last_status;
>
> Do you really need a separate variable (aq_err)?
That seems to be the convention used elsewhere, where ret is
distinguished from aq_err, see i40e_sync_vsi_filters()
> > + if (aq_err != I40E_AQ_RC_OK) {
> > + dev_info(&vsi->back->pdev->dev,
> > + "add filter failed err %s aq_err %s\n",
> > + i40e_stat_str(&vsi->back->hw, ret),
> > + i40e_aq_str(&vsi->back->hw, aq_err));
> > + }
> > + return ret;
> Same about kernel doc.
See earlier response.
--Sowmini
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists