AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C
Hello Val, my name is Johannes Hahn from Siemens AG in Germany. Our product Open Controller II (OCII)[1] uses the Realtime Clock RX6110SA from SEIKO EPSON. Currently there is a merge request ongoing for the Linux Kernel master branch[2] which adds I²C and ACPI support to your original driver implementation. Simultaneously there is an already merged patch-set for coreboot[3] available creating the ACPI (SSDT) table entries for the RX6110SA. The OCII uses coreboot for firmware initialization. During the merge request the eligible objection arose that the ACPI ID used in the Linux driver patch is not conforming the ACPI Specification. Indeed it does not. But when searching for a product identifier of RX6110SA I was not able to find a sufficient one with respect to the ACPI Specification (see [4] chapter 6.1.5 _HID (Hardware ID),[5]). According to the fact that there are other Linux RTC drivers on the Kernel mainline[6] which support ACPI matching that also do not have ACPI Specification compatible IDs we used that as an example for our first patch attempt. A PNP ID for SEIKO EPSON is already registered at UEFI database[7]. What I kindly ask your for is an ACPI Specification conforming Product Identifier for the RX6110SA RTC ? According to [5] this Product Identifier should be "... always four-character hexadecimal numbers (0-9 and A-F)". In case you do not know it our can not acquire/create one could you please redirect me to someone from SEIKO EPSON who can help me with that demand ? [1]: (https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7677-2DB42-0GB0) [2]: https://lkml.org/lkml/2020/11/12/561 [3]: https://review.coreboot.org/c/coreboot/+/47235 [4]: https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf [5]: https://www.uefi.org/PNP_ACPI_Registry [6]: https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1142 [7]: https://www.uefi.org/PNP_ID_List?search=SEIKO+EPSON Best Regards, Johannes Hahn -Ursprüngliche Nachricht- Von: Henning Schild Gesendet: Montag, 16. November 2020 16:30 An: Andy Shevchenko Cc: Claudius Heine ; Alessandro Zummo ; Alexandre Belloni ; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Hahn, Johannes (DI FA CTR PLC PRC3) ; Zeh, Werner (DI MC MTS R HW 1) Betreff: Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C Am Mon, 16 Nov 2020 16:46:31 +0200 schrieb Andy Shevchenko : > On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote: > > From: Johannes Hahn > > > > This allows the RX6110 driver to be automatically assigned to the > > right device on the I2C bus. > > Before adding new ACPI ID, can you provide an evidence (either from > vendor of the component, or a real snapshot of DSDT from device on > market) that this is real ID? > > Before that happens, NAK. > > P.S. Seems to me that this is kinda cargo cult patch because proposed > ID is against ACPI and PNP registry and ACPI specification. In fact we pushed it in coreboot and Linux at the same time. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3Dreserved=0 That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already. Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind. Henning
AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C
Hello Andy, when comparing the ACPI IDs used in rtc-ds1307.c, which is already on mainline https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1141 for example. Every ID listed there is also not formatted the ACPI ID , PNP ID way defined in the ACPI spec. How about that ? Best Regards, Johannes -Ursprüngliche Nachricht- Von: Henning Schild Gesendet: Montag, 16. November 2020 16:30 An: Andy Shevchenko Cc: Claudius Heine ; Alessandro Zummo ; Alexandre Belloni ; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Hahn, Johannes (DI FA CTR PLC PRC3) ; Zeh, Werner (DI MC MTS R HW 1) Betreff: Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C Am Mon, 16 Nov 2020 16:46:31 +0200 schrieb Andy Shevchenko : > On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote: > > From: Johannes Hahn > > > > This allows the RX6110 driver to be automatically assigned to the > > right device on the I2C bus. > > Before adding new ACPI ID, can you provide an evidence (either from > vendor of the component, or a real snapshot of DSDT from device on > market) that this is real ID? > > Before that happens, NAK. > > P.S. Seems to me that this is kinda cargo cult patch because proposed > ID is against ACPI and PNP registry and ACPI specification. In fact we pushed it in coreboot and Linux at the same time. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3Dreserved=0 That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already. Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind. Henning
