pca954x: harmonize pca9547 and pca954x and add pca9540 support
ClosedPublic
Actions

Authored by bz on Jul 2 2022, 11:41 PM.

Details

Reviewers

• ian
avg
manu
mmel
mw

Commits

rGab82f64eb5b3: pca954x: harmonize pca9547 and pca954x and add pca9540 support
rG97dbd37753ae: pca954x: harmonize pca9547 and pca954x and add pca9540 support

Summary

The two implementations for the pca9548 switch and the pca9547 mux
seemed close enough so we can put them together and with a bit more
abstraction add pca9540 support.

While here apply a bit of consistency in variable and driver naming and
use device_has_property instead of the FDT-only OF_ variant.

This disconnects pca9547 from the build but does not yet delete it.

Test Plan

I cannot test either pca9547 or pca9548. And frankly I did this by
visual inspection and not much thinking in order to not add yet another
almost similar file for the pca9540.

Would be great if you could test things still work as expected in addition
to reviewing.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bz created this revision.Jul 2 2022, 11:41 PM

Herald added a reviewer: manu. · View Herald TranscriptJul 2 2022, 11:41 PM

Herald added subscribers: emaste, andrew, imp. · View Herald Transcript

bz requested review of this revision.Jul 2 2022, 11:41 PM

Harbormaster completed remote builds in B46235: Diff 107696.Jul 2 2022, 11:41 PM

bz added reviewers: mmel, mw.Jul 3 2022, 1:57 PM

mmel added inline comments.Jul 3 2022, 4:00 PM

sys/dev/iicbus/mux/pca954x.c
161	I don't think it should be #ifdefed. The "okay" test is obviously missing in the original probe method.

bz added inline comments.Jul 3 2022, 4:06 PM

sys/dev/iicbus/mux/pca954x.c
161	Is that status inherited? I never saw a '''status = "okay"''' inside the '''i2c-switch''' blocks, only in the upper '''i2c'' node.

imp added inline comments.Jul 3 2022, 5:23 PM

sys/dev/iicbus/mux/pca954x.c
12	this drops a comma.
161	Generally only direct children of ofwbus/fdtbus need to check this. Also, I have a huge patch set I've been rebasing forward for years that hoists this check into the bus code and now seems like a good time to solicit comments on that :)

bz added inline comments.Jul 3 2022, 5:49 PM

sys/dev/iicbus/mux/pca954x.c
12	Given this got merged from two files, ... also the comma is not in /usr/share/etc/bsd-style-copyright . I can add this back is needed but eventually we'll need to align this "templates" (also with the one for the SPDX tag)
161	Right but please on a separate review... I'll just remove the #if 0 block..., right?

remove the #if 0 block for status valid check.

Harbormaster completed remote builds in B46242: Diff 107711.Jul 3 2022, 10:25 PM

mmel accepted this revision.Jul 4 2022, 5:00 AM

This revision is now accepted and ready to land.Jul 4 2022, 5:00 AM

I think that this is a very good change. Some minor code style / improvement suggestions from me.
Thank you.

sys/dev/iicbus/mux/pca954x.c
121	Style nit: I'd add an empty line before the multi-line comment.
141–144	Can this be "collapsed" into 'else if'? Also, there is no default /catch-all else clause... maybe it's worthwhile adding it with some error and diagnostic? Or panic?
145	I think that what should be asserted here is that en is a power of 2, not just an even number. Also, if en == 0 is allowed here, then this clause would cover the next clause as well.
148	What does the XXX comment suggest here? It wasn't clear to me. I do not see how anything from iic_reqbus_data::flags could affect iic_msg::flags.

avg added inline comments.Jul 4 2022, 6:23 AM

sys/dev/iicbus/mux/pca954x.c
161	I think that adding the okay status was a correct change. A mux / switch is a device tree node, so it can be disabled via its status property. This was an oversight in my code.

bz marked 7 inline comments as done.Jul 4 2022, 4:02 PM

bz added inline comments.

sys/dev/iicbus/mux/pca954x.c
145	I cannot follow the en == 0 logic as busidx != (1 << busdix); I am probably misunderstanding here. I'll do the power2 check though.

Correct all but on coments from avg; KASSERT to be discussed still. I didn't understand.

This revision now requires review to proceed.Jul 4 2022, 4:03 PM

Harbormaster completed remote builds in B46252: Diff 107739.Jul 4 2022, 4:03 PM

avg added inline comments.Jul 4 2022, 4:06 PM

sys/dev/iicbus/mux/pca954x.c
145	Apologies, I misread the code.

Does that mean we are good now (again)? Sorry the approved went away with the other corrections.

One final super-small nit: I am not sure what style(9) says about it, but my personal preference is that if one block of an if-else chain needs curly braces then all blocks in the chain should have them.
To my eye, that looks prettier.
But feel free to ignore this if it looks uglier or just the same to you.

Thank you!

This revision is now accepted and ready to land.Jul 4 2022, 4:18 PM

bz added inline comments.Jul 4 2022, 5:28 PM