Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver

[Andy Shevchenko]

On Thu, Jun 13, 2013 at 11:25 PM, Alexey Brodkin
<Alexey.Brodkin@xxxxxxxxxxxx> wrote:
> On 06/13/2013 10:25 PM, Andy Shevchenko wrote:
>> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
>> <Alexey.Brodkin@xxxxxxxxxxxx> wrote:
>>> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>>> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>>> ARCAngel4/ML50x.

>>> +++ b/drivers/net/ethernet/arc/arc_emac.h

> And what about function names? Do you think it worth to shorten them too
> since most of them aren't visible outside (static).

It's better to keep them in their namespace. So, leave them as is.

>>> +struct arc_emac_priv {

>>> +};

>> It seems you missed my comments against the names of the members. Can
>> you address them or comment why not?
>
> You mean to add description in kerneldoc format for all the fields in
> structures?

Not only that one. About member names as well.

> Well while in general it could be "a proper way" of documenting sources
> I found it not that convenient especially in case of really long structures.

> In my case "arc_emac_priv" structure has 21 members, so right before
> structure itself there will be another at least 21 line of comments.

Not an argument, you understand.

> Moreover: "The kernel-doc function comments describe each parameter to
> the function, in order, with the @name lines."

> While I don't think that each and every member needs description.

Describe them in couple of words.

> At
> least some pairs like Tx/Rx I believe may share the only comment saying
> "Pointers to BD rings - CPU side".

What BD means? May be it worth to describe as well?

> Also I barely can find an example of strict usage of kernel-doc format
> for data structures in drivers nearby.
>
> For example take a look at STMMAC - drivers/net/ethernet/stmicro/stmmac/
> Lots of structures defined, non with kernel-doc description.

Again, not an excuse :-)

> Still you think the only way to go is to add kernel-doc description then
> I'll add it ASAP, might be it will be a good example for other developers.

Right.

>>> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
>>

>>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>>> +{

>>> +       unsigned int i, loop, work_done = 0;

>>> +       for (loop = 0; loop < RX_BD_NUM; loop++) {

>>> +               work_done++;
>>> +               if (work_done >= budget)
>>> +                       break;
>>
>> Those three could easily go to the for () on the top of this function.
>
> Correct. It should be like this on top of the "arc_emac_poll":
> ====
>                 if (work_done >= budget)
>                         break;
>                 work_done++;
> ====

I meant something like
for (loop = 0; loop < RX_BD_NUM && work_done < budget; loop++, work_done++)

>>> +static int arc_emac_open(struct net_device *ndev)
>>> +{
>>
>>> +       /* Set Poll rate so that it polls every 1 ms */
>>> +       arc_reg_set(priv, R_POLLRATE,
>>> +                    priv->clock_frequency / 1000000);
>>
>> I don't understand how you end up with 1ms here. 1000000 is just a
>> magic number, clock_frequency generally is an arbitrary value.

I meant how do you guarantee this is 1ms? What if clock_frequency is not 100MHz?

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/