So, based on the discussion and feedback in D43746, I modified the existing vf_i2c driver to support the LX2160A controller, both in FDT and ACPI mode. It is closer to Val's version from D24917. I have mostly kept the clock divider calculations from the original vf_i2c driver, with one twist: If no parent clock frequency can be extracted AND if the controller's clock divider register is already set, then assume the boot loader has set it, and do not set the register to the lowest speed in the table. I think this should retain the support for the other systems, while allowing proper support for the LX2160A. Otherwise, I have kept the updated code from D43746. I kept the copyright notice from the original vf_i2c driver. I moved the files to the new recommended directory.
Details
Basic testing SolidRun LX2160A CEX7 using the DK2 UEFI firmware in ACPI mode:
dmesg:
vf_i2c_acpi0: <Vybrid Family Inter-Integrated Circuit (I2C)> iomem 0x2000000-0x200ffff irq 5 on acpi0
vf_i2c_acpi0: Using existing bus frequency divider register value (0xA2).
iicbus0: <Philips I2C bus (ACPI-hinted)> on vf_i2c_acpi0
iicbus0: <unknown card> at addr 0x77
iic0: <I2C generic I/O> on iicbus0
vf_i2c_acpi1: <Vybrid Family Inter-Integrated Circuit (I2C)> iomem 0x2010000-0x201ffff irq 6 on acpi0
vf_i2c_acpi1: Bus frequency divider value appears unset, defaulting to low I2C bus speed.
iicbus1: <Philips I2C bus (ACPI-hinted)> on vf_i2c_acpi1
iic1: <I2C generic I/O> on iicbus1
Right after boot:
i2c -s -f /dev/iic0
19 1b 36 37 50 51 53 57 77
After selecting MUX's channel 1 (MUX address is 0x77, I used the code https://github.com/pldrouin/amc6821_controller to select channel 1 on LX2160A and to query the ID of the fan controller):
i2c -s -f /dev/iic0
18 6a 77
TZ0 is still working. Have not witnessed interference between the thermal zone and the I2C controller driver so far:
sysctl hw.acpi.thermal.tz0
hw.acpi.thermal.tz0._TSP: 50
hw.acpi.thermal.tz0._TC2: 1
hw.acpi.thermal.tz0._TC1: 1
hw.acpi.thermal.tz0._ACx: 80.1C -1 -1 -1 -1 -1 -1 -1 -1 -1
hw.acpi.thermal.tz0._CRT: 95.1C
hw.acpi.thermal.tz0._HOT: -1
hw.acpi.thermal.tz0._CR3: -1
hw.acpi.thermal.tz0._PSV: 55.1C
hw.acpi.thermal.tz0.thermal_flags: 0
hw.acpi.thermal.tz0.passive_cooling: 0
hw.acpi.thermal.tz0.active: 1
hw.acpi.thermal.tz0.temperature: 44.1C
Retrying bus scanning:
i2c -s -f /dev/iic0
18 6a 77
Support for some other systems in FDT mode should be tested as well
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 56132 Build 53020: arc lint + arc unit
Event Timeline
If I understand right, the usual Copyright procedure for changes to a file that still has substantial original material but also substantial new material is to add copyright lines (for the new author's material, with years if in dated form), leaving the original Copyright line in place as well.
If new files (nearly just renaming would be: "not new") are created that are not substantially copies of older files, just the new author's Copyright would go in those. Similarly for the file naming not being changed but not much of the original material being left.
(The wording above may only provide a indication of the general direction and not much detail relative to various legal concepts involved.)
[A hint about "substantial changes": if omitting a Copyright means not finding the name of who to ask questions about notable parts of the resulting material, some Copyright notice(s) are likely missing.]
Thanks Mark. I am not the first one making changes though, I think at least @dgr_semihalf.com and @val_packett.cool did significant changes as well.
. . .
Thanks Mark. I am not the first one making changes though, I think at least @dgr_semihalf.com and @val_packett.cool did significant changes as well.
As far as I know, you are only responsible for your own changes and your own Copyright status relative to Copyright notices, not to adjusting other's Copyright notices or lack of them.
I would say that the judgment for your context is about how things compare to what you started from, even if the Copyright(s) status of the prior state is possibly odd/incomplete.
Looks like you adjusted the Copyright lines of Ruslan Bukin (adding: "-2024"). As I understand it, that implies Ruslan made changes to the source in 2024 (and possibly some between years). Did such happen? If not, Ruslan likely could not validly claim a Copyright on any new 2024 material, just on text from 2014 that is still present somewhere. Going in a different direction: you are making such Copyright changes for an author, possibly without their permission.
Also, I'd not be surprised if each Copyright needs its own "Copyright".
So, if I understand right, the following would likely be better notation in files where both of you significantly contributed content:
* Copyright (c) 2014 Ruslan Bukin <br@bsdpad.com> * Copyright (c) 2024 Pierre-Luc Drouin <pldrouin@pldrouin.net>
There are a few style issues. I've commented on examples of them I see, but you should check for more.
Can you upload more context, e.g. if using the web interface use the git command from https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface
| sys/arm/freescale/vybrid/files.vybrid | ||
|---|---|---|
| 12 | This will break i2c on vybrid | |
| sys/conf/files.arm64 | ||
| 634 | No need to create vf_i2c_acpi and vf_i2c_fdt. They already depend on acpi and fdt respectively. | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
| 5 | @br Are you happy with this change? All rights reserved. is unneeded so we have been removing it when possible. | |
| 137 | Missing newline | |
| 371 | This should be 2 lines, and is missing a space after the if | |
| 384 | This should be at the top of the function | |
| 387 | Where did 0x14 come from? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.h | ||
| 42 | You need to rebase past moving dev/extres/* to dev/* | |
| sys/dev/iicbus/controller/vybrid/vf_i2c_acpi.c | ||
| 39 | Is this needed? It was for __FBSDID, but that's been removed. | |
| 97–103 | Are these not needed on fdt? | |
| 105 | DEVMETHOD_END | |
| sys/modules/Makefile | ||
| 222–223 | Why do we need 2 modules? You can include both in one module, e.g. dpaa2 | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 5 | Sure! | |
Implementing most of the recommended changes, pending clarification for the remaining requests.
I think the latest diff addresses everything that has been mentioned so far.
| sys/conf/files.arm64 | ||
|---|---|---|
| 634 | I had it as a split device to minimize memory usage if one does not want ACPI support for this device? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
| 387 | I don't know where it originally came from, I wanted to retain compatibility with the existing code. I just switched it to a hex representation for consistency. I could find barely any documentation about the MVF600. | |
| sys/modules/Makefile | ||
| 222–223 | I had it as a split module to minimize memory usage if one does not want ACPI support for this device? | |
As far as I know, all requests have been addressed. Please let me know if I have missed anything. Thanks!
Really looking forward to this. Also a step towards getting SFP+ support going..
| sys/modules/vf_i2c/Makefile | ||
|---|---|---|
| 8 | Why do we need opt_mmccam? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 16 | Has FreeBSD changed the default template? COPYRIGHT and share/examples/etc/bsd-style-copyright uses different quoting. And on this review it actually shows up like that. Or is this an editor being clever given it is on all files with ""? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 16 | Looks like there was explicit editing of the original legal-template text during the editing to produce 134084. As I understand, generally template legal text should be untouched but for the likes of name and I expect that the quoting around AS IS should revert to match the legal template. | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 16 | I see that I was wrong about what the FreeBSD template has. Ignore my note. (I do not seem to be able to delete or edit it.) | |
I looked through src and it looks like either form of quoting are used for the copyright notice...
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 16 | I had used https://www.freebsd.org/internal/software-license/ following your recommendation | |
| 16 | ||
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 5 | Hey, can we move to SPDX-LICENSE-IDENTIFIER only for the license grant? @bz? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c_acpi.c | ||
| 6 | Here and elsewhere: you must reoeat copyright before the year to comply with the legal notice requirements. Also why is br@ here also for new code? | |
Updating copyright notice to state copyright for each author and also removing the original vf_i2c author for vf_i2c_acpi
| sys/dev/iicbus/controller/vybrid/vf_i2c_acpi.c | ||
|---|---|---|
| 6 | Fixed | |
One comment: I tested this on a LS1088 in FDT mode and it makes me super happy as I can scan bits (and flawlessly read) now I had problems with on another board (which I should test as well once I can again).
And that brings me to a request -- and please say "no" if you feel like it's too much hassle. Can we split this into two parts as it is essentially two things: (i) the move of the code and split for the various bus attachments, and (ii) the fixing/improving of the actual I2C bits/logic. It would certainly help commit messages and explanations of the why these changes are done and working so well.
There's a few style bits and some hard coded values we should probably #define in there still but nothing major. I'll wait on commenting on whether we'll split it or not.
At this point in order to split it into two diffs like that I think the easiest would be to reintroduce the old code into the split files. Maybe I can do it manually if you think it is worth it.