Re: [PATCH net-next v6 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Trevor Gross <tmgross@umich.edu>]

On Fri, Aug 23, 2024 at 8:37 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> > At this point the vendor driver looks like it does some verification:
> > it attempts to read 3.d7fd until it returns something other than 0x10
> > or 0, or times out. Could that be done here?
>
> Yeah, we better to wait here until the hw becomes ready (since the
> 8051 has just started) and check if it works correctly. A new Rust
> abstraction for msleep() is necessary.
>
> Even without the logic, the driver starts to work eventually (if the
> hw isn't broken) so I didn't include it in the patchset. I'll work on
> the abstraction and update the driver after this is merged.

It sounds okay to me to not block on this as long as it isn't glitchy
- It should probably be a FIXME?

> > Consistency nit: this file uses a mix of upper and lowercase hex
> > (mostly uppercase here) - we should probably be consistent. A quick
> > regex search looks like lowercase hex is about twice as common in the
> > kernel as uppercase so I think this may as well be updated.
>
> Ah, I'll use lowercase for all the hex in the driver.
>
> It will be a new coding rule for rust code in kernel? If so, can a
> checker tool warn this?

I don't know of any rule for this, I just noticed that the patch had
both and it made me take a look at what is used elsewhere. rustfmt has
an option to just commonize it for you, `hex_literal_case` [1], but it
is unstable. That would probably be nice at some point.

> > Overall this looks pretty good to me, checking against both the
> > datasheet and the vendor driver we have. Mostly small suggestions
> > here, I'm happy to add a RB with my verification question addressed
> > and some rewording of the 0xd001 (phy revision) comment.
>
> Thanks a lot! I'll send v7 soon.

Thanks!

- Trevor

[1]: https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#hex_literal_case