Re: [RFC PATCH v3 1/3] rust: core abstractions for network PHY drivers

[Trevor Gross <tmgross@umich.edu>]

On Wed, Oct 11, 2023 at 11:17 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> >> 3. multiple phy drivers has one id in device_table.
> >>
> >> How these approach can handle the case?
> >>
> >> Also note that there is a driver doesn't define PHY_ID, uses
> >> match_phy_device() instead.
> >
> > In this case you would just need to put the same `DeviceId` in the
> > array for both drivers, perhaps as a const. I think this proposal
>
> I'm not sure modpost can handle it.
>
> I guess that you would have two exact same lines in modules.alias,
> which might works but I don't think that it's not designed to handle
> such.
>
> Or the macro needs to find the same id and keep only one.

Good point, scratch that. What about this logic:

1. Leave `Driver` as you have it, one `DeviceId` with a null default
2. Use the macro to create a `const fn` to build the mdio_device_table
so you can add logic (like in the playground I linked)
3. Make this function skip anything that doesn't specify a `DeviceId`,
i.e. if the `DeviceID` is null don't add it to the table
4. Any devices that don't specify a `DeviceId` must specify
`match_phy_device` (I think, since it will never get matched
otherwise?). This could be verified in `create_phy_driver`
4. In the macro, allow specifying extra `DeviceId`s that aren't
specific to any phy.

Something complex like the icplus driver [1] is probably a good test
to see how any of these work out. I think that would look something
like:

    const IP175C_PHY_ID: u32 = 0x02430d80;
    const IP1001_PHY_ID: u32 = 0x02430d90;
    const IP101A_PHY_ID: u32 = 0x02430c54;

    impl Driver for Ip175c {
        const PHY_DEVICE_ID: phy::DeviceId =
phy::DeviceId::match_model(IP175C_PHY_ID);
        // ...
    }

    impl Driver for Ip1001 {
        const PHY_DEVICE_ID: phy::DeviceId =
phy::DeviceId::match_model(IP1001_PHY_ID);
        // ...
    }

    impl Driver for Ip101a {
        // no override of PHY_DEVICE_ID
        fn match_phy_device(_dev: &mut Device) -> bool { /* ... */ }
        // ...
    }

    impl Driver for Ip101g {
        // no override of PHY_DEVICE_ID
        fn match_phy_device(_dev: &mut Device) -> bool { /* ... */ }
        // ...
    }

    kernel::module_phy_driver! {
        // the first two drivers provide match_model for IP175C_PHY_ID
and IP1001_PHY_ID
        drivers: [Ip175, Ip1001, Ip101a, Ip101g],
        // this provides the extra match_exact
        // this field is optional, most drivers won't need it
        additional_matches: [phy::DeviceId::match_exact(IP101A_PHY_ID)],
        name: "...",
        author: "...",
        description: "ICPlus IP175C/IP101A/IP101G/IC1001 PHY drivers",
        license: "...",
    }

Nice because the easy default behavior is to add PHY_DEVICE_ID to the
table if it is specified. But you get the flexibility to not provide
it, or add extra entries that aren't specific to a device. Any
thoughts?

If this works, maybe PHY_DEVICE_ID should be Option<DeviceId> to make
it more clear that you don't have to specify anything?

---

I know you have the `DeviceId` functions called `new_with_exact_mask`
and similar. Maybe rename them to `match_exact`, `match_vendor`,
`match_model` so they are easier to discover based on the C macros?
Also more terse.

[1]: https://elixir.bootlin.com/linux/v6.6-rc5/source/drivers/net/phy/icplus.c