Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Sun, 22 Oct 2023 17:34:04 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sun, Oct 22, 2023 at 01:37:33PM +0200, Miguel Ojeda wrote:
>> On Sun, Oct 22, 2023 at 11:47 AM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>> >
>> > Agreed that the first three paragraphs at the top of the file are
>> > implementation comments. Are there any other comments in the file,
>> > which look implementation comments to you? To me, the rest look the
>> > docs for Rust API users.
>> 
>> I think some should be improved with that in mind, yeah. For instance,
>> this one seems good to me:
>> 
>>     /// An instance of a PHY driver.
>> 
>> But this one is not:
>> 
>>     /// Creates the kernel's `phy_driver` instance.
>> 
>> It is especially bad because the first line of the docs is the "short
>> description" used for lists by `rustdoc`.
>> 
>> For similar reasons, this one is bad (and in this case it is the only line!):
>> 
>>     /// Corresponds to the kernel's `enum phy_state`.
>> 
>> That line could be part of the documentation if you think it is
>> helpful for a reader as a practical note explaining what it is
>> supposed to map in the C side. But it should really not be the very
>> first line / short description.
>> 
>> Instead, the documentation should answer the question "What is this?".
>> And the answer should be something like "The state of the PHY ......"
> 
> Its the state of the state machine, not the state of the PHY. It is
> already documented in kernel doc, so we don't really want to duplicate
> it. So maybe just cross reference to the kdoc:
> 
> https://docs.kernel.org/networking/kapi.html#c.phy_state

I added links to the kdoc like:

/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state).

But the first line needs to a short description so I copy the C
description:

/// PHY state machine states.

I revised all the comments.