Allwinner A10/A20 i2c

[manu_bidouilliste.com]

Summary

Add Allwinner i2c (TWI) driver.
Allwinner chips use the TWSI Marvell controller already supported but with different register addresses.
This patch splits the marvell driver into driver and i2c driver methods.

Test Plan

Apply patch, build kernel and verify that i2c is up. On A20 hardware the AXP209 PMU is at address 0x34 :
$ i2c -s
Scanning I2C devices on /dev/iic0: 34

I don't have Marvell hardware so I only tested that the kernel for dreamplug and sheevaplug compiled.

[imp]

sys/arm/allwinner/a10_i2c.c

68–72 ↗

(On Diff #12086)

stylistically, this should be more like:

if (ofw_bus...("allwinner...") == 0)
return (ENXIO);

sc->is_allwinner = 1;

sys/arm/allwinner/files.allwinner

16–17 ↗

(On Diff #12086)

not entirely sure I like this layout, but we don't have to fix this for the commit.

sys/arm/mv/mv_twsi.c

137–141 ↗

(On Diff #12086)

same comments as before. Except is_allwinner will be zero because the softc is zerod when first allocated.

220–263 ↗

(On Diff #12086)

Why do we do this here and not in the allwinner driver? Alternatively, why do we do this here at all? Shouldn't the iic bus code attach the children? or is the MV FDT different :( If so, there should be a comment here because this code doesn't make sense to me at all.

sys/arm/mv/twsi.c

376 ↗

(On Diff #12086)

Not commenting on all of thse, but perhaps it would be better to create twsi_data and twsi_status register offsets in the softc.

[manu_bidouilliste.com]

sys/arm/mv/mv_twsi.c

220–263 ↗

(On Diff #12086)

This is the original code from twsi.c I haven't touched this part.

[manu_bidouilliste.com]

Move register offset into the softc struct.

[manu_bidouilliste.com]

Rename driver to iichb.
Add bus and ofw interface methods.

Now i2c kernel device drivers are probed and attached correctly.

[manu_bidouilliste.com]

Update patch due to the last commit on mv_twsi.c and a10_clk.c on HEAD.

[manu_bidouilliste.com]

Update diff to add change introduce into rev 295567
Also now that A10 kernel config is into -HEAD, add iic and iicbus into it.

[andrew]

I think the driver should be moved to be, e.g. sys/dev/iicbus/twsi/.

sys/arm/allwinner/a10_i2c.c

71–77 ↗

(On Diff #13251)

This should be moved to the attach function, and have the magic numbers changed to macros.

147–169 ↗

(On Diff #13251)

I think it would make sense to create a base class for the common code, then subclass them here. You can see an example in the arm64 GIC driver. The code is under sys/arm64/arm64/ in gic.c, gic_fdt.c, and gic_acpi.c.

[manu_bidouilliste.com]

In D4846#112237, @andrew wrote:

I think the driver should be moved to be, e.g. sys/dev/iicbus/twsi/.

Do you mean all the drivers or the common function ? (the ones currently in arm/mv/twsi.c)

[manu_bidouilliste.com]

Convert twsi to a driver class and move generic function to dev/iicbus/twsi/
Add defines for allwinner and marvell controller reg offset.
Move reg offset assignment to attach function.
Add twsi driver to files.arm since it's used in more that one SoC.

[manu_bidouilliste.com]

While converting to class the driver name changed from iichb so iic child device couldn't attach.
Fix that in this diff.

[manu_bidouilliste.com]

Update diff since the twsi device is now in -HEAD

[andrew]

sys/dev/iicbus/twsi/a10_twsi.c

34 ↗

(On Diff #13303)

Needed?

45 ↗

(On Diff #13303)

Needed?

50 ↗

(On Diff #13303)

Sorting. Should be after the other iicbus headers.

54–62 ↗

(On Diff #13303)

#define<tab>TWI...

85–100 ↗

(On Diff #13303)

This looks to be common.

108–116 ↗

(On Diff #13303)

This looks like it could be moved into a common attach function.

134–140 ↗

(On Diff #13303)

Also common. I'd create twsi_attach, then call it from here, and where this code is in the Marvell attachment.

sys/dev/iicbus/twsi/mv_twsi.c

234 ↗

(On Diff #13303)

This is unneeded, and may overflow dname. OF_getprop already includes the null terminator.

261 ↗

(On Diff #13303)

And here

sys/dev/iicbus/twsi/twsi.c

226 ↗

(On Diff #13303)

Could this (and the functions below) be static? It doesn't seem to be used outside this file.

[manu_bidouilliste.com]

sys/dev/iicbus/twsi/twsi.c

226 ↗

(On Diff #13303)

Now that the device is a class yes, I'll change that.

[manu_bidouilliste.com]

Update diff to take care of andrew comments.