Patch review when upstreaming

[andy-shev]

Not directly an issue, though, please include me to Cc list when you will be about to send this upstream.

(Code looks not bad, though it has a lot of small issues,and the driver registration looks hackish, but this fixable I believe).

[roadrunner2]

Thanks for the initial feedback. @cb22 has the final say here, but for my part: would you mind spelling out some of the issues so we can look into fixing them?

Regarding the registration, yes, that is truly a hack (at least all the appleacpi_* stuff) - I've been assuming it's not upstreamable. But at least it allows folks to use this driver right now without needing to modify the DSDT. For more background on the whole thing, please see #29 and the linux-spi posts mentioned in #21 (in short: support for loading info from _DSM methods needs to be added to the acpi core, and then #29 can be reverted).

[cb22]

Thanks for the feedback @andy-shev

Agreed, there are quite a few hacks floating around in the driver that definitely should be resolved before sending it upstream.

[marbetschar]

@cb22 is there any (rough) ETA for upstreaming this driver? For me it works quite well and we'll going to maintain a custom compile step for adding this driver to Tails (see https://labs.riseup.net/code/issues/15652 for more information).

But doing so rises additional ongoing effort. From this point of view I'd really love seeing this driver upstreamed ;)

[roadrunner2]

@andy-shev, @l1k: This is long overdue, but I'm definitely ready to upstream this thing. I've pushed the candidate commits to my macbook-drivers branch. The first (4-th last) commit is this applespi driver here; the other 3 are for the iBridge and touchbar. If either of you have any time and interest to take a look any of these before I submit them upstream, that would be great. if not, I understand.

One big question about the applespi commit roadrunner2/linux@572b4e7 is how kosher the changing of the Kconfig for DRM_SIL_SII8620 is - I try to justify it in the commit message, but not sure if that's convincing enough, or if such a change is a strict no-no.

[l1k]

I'd move the change of drivers/gpu/drm/bridge/Kconfig to a separate commit preceding the series. That commit should be cc'ed to the folks who signed off on torvalds/linux@d6abe6df706c (Inki Dae <inki.dae@samsung.com>, Andrzej Hajda <a.hajda@samsung.com>) as well as dri-devel@lists.freedesktop.org. The change itself looks legit to me. A quick git grep "select INPUT" and git grep "depends on INPUT" shows that the former is almost never used. Once one of the drm maintainers and/or the Samsung folks has acked the commit, it can go in through the input tree with the other commits.

The #include <linux/dmi.h> seems unnecessary if I'm not mistaken.

I notice struct touchpad_info_protocol is marked __packed, the others aren't. Not sure if intentional.

Love the extensive kerneldoc.

[Dunedan]

@roadrunner2 I had a quick look at the tree and what stands out to me is the repeated mention of "recent MacBook Pros" in the Kconfigs. It might be better to call out the supported generations explicitly as the most recent MacBook Pro's (15,x) are afaik already not supported anymore by the drivers.

And another very minor nitpick: The official notation for the touch bar is "Touch Bar" according to Apple, not "touchbar" or "Touchbar".

[roadrunner2]

@l1k wrote:

I'd move the change of drivers/gpu/drm/bridge/Kconfig to a separate commit preceding the series.
[snip]

Sounds good. Done.

The #include <linux/dmi.h> seems unnecessary if I'm not mistaken.

Ah, good catch - a leftover.

I notice struct touchpad_info_protocol is marked __packed, the others aren't. Not sure if intentional.

Yes, because the one field in there is unaligned (in all other structures things are naturally aligned).

Thanks for the review and suggestions!

[roadrunner2]

@Dunedan wrote:

@roadrunner2 I had a quick look at the tree and what stands out to me is the repeated mention of "recent MacBook Pros" in the Kconfigs. It might be better to call out the supported generations explicitly as the most recent MacBook Pro's (15,x) are afaik already not supported anymore by the drivers.

Ok, so this is a bit tricky. For the keyboard/trackpad I agree with you, and I believe everywhere I explicitly mention the 13,* and 14,* models (plus the MB's). For the iBridge, however, I do believe the drivers can probably be used on the 15,*'s too (with potentially minor changes to the mfd driver), because if you look at the ioreg tree there you'll notice that the T1 chip functionality is essentially still there, just tucked under the T2 chip's PCI-to-USB interface instead of directly on the platform's USB h/w. I.e. I expect that the T2 driver that needs to be written is a PCI driver that exposes a USB interface, on which the iBridge functionality will be found much like now. I could of course be wrong.

And another very minor nitpick: The official notation for the touch bar is "Touch Bar" according to Apple, not "touchbar" or "Touchbar".

Oh, ouch! Ok, I've changed various places to use "Touch Bar" (such as in the Kconfig's), but for most of the code comments etc I decided to use "touch bar" (at some point the capitalization becomes annoying).

[l1k]

I've taken a look at the MFD, Touch Bar and ALS commits. I'm not really familiar with MFD and HID, so it's difficult for me to provide constructive feedback on those portions. However I've worked with the IIO subsystem before, so took a closer look at the ALS commit. One thing that caught my eye was that appleals_config_iio() assigns iio_dev->channels using kmemdup(), but you should be able to assign it directly with iio_dev->channels = appleals_channels;. This obviates the need for kfree() in the probe error path and remove routine.

The MFD, Touch Bar and ALS commits need a Signed-off-by: in the commit message.

[l1k]

I've pushed commit l1k/linux@255df1c91f9d as a little docs fixup on top of your series. Could you fetch, cherry-pick and squash with git rebase --autosquash?

Also, could you cc me when posting to the list? I think at least the applespi driver is ready to go in. Thanks!

[roadrunner2]

I've taken a look at the MFD, Touch Bar and ALS commits. I'm not really familiar with MFD and HID, so it's difficult for me to provide constructive feedback on those portions. However I've worked with the IIO subsystem before, so took a closer look at the ALS commit. One thing that caught my eye was that appleals_config_iio() assigns iio_dev->channels using kmemdup(), but you should be able to assign it directly with iio_dev->channels = appleals_channels;. This obviates the need for kfree() in the probe error path and remove routine.

Thanks for looking at this too! You're right about the kmemdup(): I had copied that from the existing hid-sensor-als driver, but since I don't modify the channels structure later like that one does, and only one sensor is supported anyway, that allocation is not necessary. Fixed now.

The MFD, Touch Bar and ALS commits need a Signed-off-by: in the commit message.

Oops, done.

I've pushed commit l1k/linux@255df1c as a little docs fixup on top of your series. Could you fetch, cherry-pick and squash with git rebase --autosquash?

Done. All updates pushed.

Also, could you cc me when posting to the list? I think at least the applespi driver is ready to go in. Thanks!

Definitely. Thanks so much for looking at these!