The commit message additionally suggests that this might be WM8960 specific.
Fortunately this is AFAICT usable much more generally. 24.576MHz can often be used for various 48kHz based sampling rates with 16 or 32bit. AKM has nice tables showing this in their datasheets.E.g. AK4452 Tables 3+4/6+7 show this is usable up to hex speed. BICK requirements probably further limit it to 16bit @ 384kHz.

Is this unfinished?

It is

I'm not sure this commented code would work. The board crate can't detect an imxrt-hal crate feature called imxrt1010, since they're different crates. And as I write this, board doesn't have its own imxrt1010 feature.

I have two ideas that might work:

1. Each board has its own clock gate collection (one example, another example). Since the SAI instance used by a board depends on the board, we can put the clock gate locator there. Board configuration considers the combination of these common clocks and the board-specific clocks, so it's equivalent to what we're going for.
2. Change each 10xx board to expose a constant, like SAI_INSTANCE, that's used like clock_gate::sai::<{board_impl::SAI_INSTANCE}>() in this collection of common clock gate locators.

I prefer the first approach. That should make it easier to introduce a board that doesn't immediately support an SAI example, and there's precedent.

I'm not convinced adding these arrays (here and for 1010) is sensible.
Other *_CLOCK_GATES contain gates that are fed from the same clock root. That way they can easily be disabled before making changes to the clock root. SAI however has a separate clock root per instance, so we usually don't have to gate all of the instances.

I agree. I remember suggesting / doubling-down on the 1010 approach in an experiment, but I was being hasty. Sorry for misleading!

If each SAI peripheral has its own clock root, then we can skip this collection. ee76313 notes that we're taking a similar approach for FlexIO.

This should follow the example set by other status() methods returning a Status struct generated by bitflags.

This could use some more type safety. I think it would be good if it followed the same API as flexpwm::Submodule::set_interrupts().

Yes it could, will take a closer look at the flexpwm driver for ideas

It seems like the PIT is only used for printing debug information now. Variable/constant names and comments should probably be updated accordingly.

I'm not sure this commented code would work. The board crate can't detect an imxrt-hal crate feature called imxrt1010, since they're different crates. And as I write this, board doesn't have its own imxrt1010 feature.

I have two ideas that might work:

1. Each board has its own clock gate collection (one example, another example). Since the SAI instance used by a board depends on the board, we can put the clock gate locator there. Board configuration considers the combination of these common clocks and the board-specific clocks, so it's equivalent to what we're going for.
2. Change each 10xx board to expose a constant, like SAI_INSTANCE, that's used like clock_gate::sai::<{board_impl::SAI_INSTANCE}>() in this collection of common clock gate locators.

I prefer the first approach. That should make it easier to introduce a board that doesn't immediately support an SAI example, and there's precedent.

I agree. I remember suggesting / doubling-down on the 1010 approach in an experiment, but I was being hasty. Sorry for misleading!

If each SAI peripheral has its own clock root, then we can skip this collection. ee76313 notes that we're taking a similar approach for FlexIO.

CI drew my eye here. Regarding chip portability:

• For the 1010, 1020, and 1060 series, we should target the proper field based on the const generic N. As written, an Sai<3, ...>::split will (re)configure the SAI1 clock direction, not the SAI3 clock direction.
• For the 1170 series, we should target the proper register and field based on the const generic N.

If splitting depends on a chip-specific behavior, we might only want to expose it when a HAL chip feature is turned on. The way we enable DMA APIs for peripherals is a reference for how it could work. Otherwise, I think we'll need a common module to take a dependency on chip, which hasn't been done yet.

Other observations:

• For both the 1010 and the 1060, there are other bit fields in this GPR1 register. A write_reg! will set those other fields to zero. Consider using a modify_reg! here to preserve other bits that are already set high.
• If the user isn't providing a &mut to the IOMUXC_GPR instance, we'll want to know we're in a critical section while we perform our read-modify-write.

Would something like this suffice? Also is this instance() call kosher?

// could this stay in common/sai.rs?
#[cfg(family = "imxrt10xx")]
unsafe fn configure_sai_mclk_dir<const N: u8>(direction: u8) {
    match N {
        1 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR1, SAI1_MCLK_DIR: direction),
        2 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR1, SAI2_MCLK_DIR: direction),
        3 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR1, SAI3_MCLK_DIR: direction),
        _ => panic!("Unsupported SAI instance: {}", N),
    }
}

#[cfg(family = "imxrt11xx")]
unsafe fn configure_sai_mclk_dir<const N: u8>(direction: u8) {
    match N {
        1 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR0, SAI1_MCLK_DIR: direction),
        2 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR1, SAI2_MCLK_DIR: direction),
        3 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR2, SAI3_MCLK_DIR: direction),
        4 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR2, SAI4_MCLK_DIR: direction),
        _ => panic!("Unsupported SAI instance: {}", N),
    }
}

Would something like this suffice?

I haven't tested, but I'm not sure if the family = "imxrt10xx" branch

2 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR1, SAI2_MCLK_DIR: direction),

will compile for all MCUs. The 1010 MCUs don't have a SAI2_MCLK_DIR field. But don't worry; CI should catch that. If that's the only issue, a conditional compile like

#[cfg(not(chip="imxrt1010"))]
2 => ral::modify_reg!(ral::iomuxc_gpr, ral::iomuxc_gpr::IOMUXC_GPR::instance(), GPR1, SAI2_MCLK_DIR: direction),

might fix it.

Separately, I recommend using the instance validity trick shown here to turn those panics into unreachable branches. A 1010 user might appreciate when their accidental configure_sai_mclk_dir::<2>(1) turns into a compile error instead of panicking at runtime.

The input direction: u8 could be is_output: bool to clarify the configuration we're performing. Additionally, a bool ensures that we can only pass two values into this one-bit-wide field.

Also is this instance() call kosher?

That'll have the same chance for race conditions as the previous code. If it's convenient to hide the IOMUXC_GPR dependency from our users, we can wrap the entire match in a critical_section.

could this stay in common/sai.rs?

Let's see how it plays out. If it remains in common/sai.rs, I think we'll need to have a configure_sai_mclk_dir implementation for when there is no family cfg selection. I'm not sure what that implementation would do.