Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
What the heck is going on here?!?!?! Gyus, stop this, please! Can somebody in Coreboot community read the ACPI spec (in particular chapter 6.1.5) and forbid such changes?
This MUST be reverted!
Could you please calm down and explain *why* you think this change is wrong?
Sure. Sorry for hysteria from my side, because I fed up with abused ACPI IDs in Linux kernel [1][2] (and more). Here we have a chance to do the right things right. what I'm complaining about is solely this (the rest of the patch is absolutely fine with me):
#define RX6110SA_HID_NAME "RX6110SA"
The problem here is that this is completely "invented" ACPI HID against ACPI specification [3], chapter 6.1.5 and against ACPI/PNP registry [4].
The component is produced by Seiko Epson company which has it's own record in the registry [4] 'SEC' and according to specifications [3] the ID should be something like SEChhhh (where h is capitalized hex digit) issued by the vendor. Above doesn't correspond this and must be fixed.
Thank you for the excellent description of the issue at hand. I agree that using an invented ACPI ID isn't a good idea, but I don't know where one would find the correct ID to use. I see that you described the options here: https://lore.kernel.org/linux-rtc/CAHp75Vdxj0tgn6P8Nfi5mMd=e9Q1+hzt4bquzB93z...
Hey Andy.
Thank you for the pointer. We had an internal discussion regarding this HID for the RTC _before_ pushing this patch with exact the same arguments you have pointed out, namely the ACPI spec. At this point in time we had a deep search for the right ID but were not able to find anything (just like you already have discovered that there is none). We then took a closer look to other examples like that and figured out, that there are different HIDs out there, even in Linux kernel, which does not match the spec naming convention. And yes, you are right, finding a bad example should not be used as a justification to do it wrong once again. The point where I underestimated this issue is: I never though or was aware of that the kernel folks already started to clean up that mess in the kernel. Sorry for that! Here is what I propose in this situation: We have already reached out to Seico Epson requesting a proper HID created by the manufacturer of the RTC. As soon as we get a reaction there, I will correct the HID here to match the one from Seico Epson with a patch. For now I do not see an urgent case to revert this patch as, as long there is no one using this ID in the kernel, it stays unused in the system. But at least we can test with this ID. Any objections from your side regarding this plan?