| Message ID | 20210506081619.2443-1-campion.kang@advantech.com.tw (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v7,1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry | expand |
Hi, I'm replying here since this series has no cover-letter, for the next version for a series touching so many different sub-systems it would be good to start with a cover-letter providing some background info on the series. I see this is binding to an ACPI device, yet it is also using devicetree bindings and properties. So I take it this means that your ACPI tables are using the optional capability of embedded device-tree blobs inside the ACPI tables ? That is an unusual combination on a x86 device, note it is not wrong but AFAIK you are the first to do this on x86. Other then that question, for the next version please give all commits a proper commit message and not just a short 1 line Subject and put the changelog after a scissors line after your Signed-off-by like this: Signed-off-by: Campion Kang <campion.kang@advantech.com.tw> --- Changed in V7: 1. According to the reviewer's comment, add two files: Documentation/hwmon/ahc1ec0-hwmon.rst and drivers/platform/x86/ahc1ec0-core.c And please also include older changelog entries. Regards, Hans On 5/6/21 10:16 AM, Campion Kang wrote: > Changed in V7: > 1. According to the reviewer's comment, add two files: > Documentation/hwmon/ahc1ec0-hwmon.rst and > drivers/platform/x86/ahc1ec0-core.c > > Signed-off-by: Campion Kang <campion.kang@advantech.com.tw> > --- > MAINTAINERS | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 83c2b1867586..984795eb6b5d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -572,6 +572,19 @@ S: Maintained > F: Documentation/scsi/advansys.rst > F: drivers/scsi/advansys.c > > +ADVANTECH AHC1EC0 EMBEDDED CONTROLLER DRIVER > +M: Campion Kang <campion.kang@advantech.com.tw> > +L: linux-kernel@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/mfd/ahc1ec0.yaml > +F: Documentation/hwmon/ahc1ec0-hwmon.rst > +F: drivers/hwmon/ahc1ec0-hwmon.c > +F: drivers/mfd/ahc1ec0.c > +F: drivers/platform/x86/ahc1ec0-core.c > +F: drivers/watchdog/ahc1ec0-wdt.c > +F: include/dt-bindings/mfd/ahc1ec0-dt.h > +F: include/linux/platform_data/ahc1ec0.h > + > ADVANTECH SWBTN DRIVER > M: Andrea Ho <Andrea.Ho@advantech.com.tw> > L: platform-driver-x86@vger.kernel.org >
On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com> wrote: > I'm replying here since this series has no cover-letter, for > the next version for a series touching so many different > sub-systems it would be good to start with a cover-letter > providing some background info on the series. > > I see this is binding to an ACPI device, yet it is also using > devicetree bindings and properties. > > So I take it this means that your ACPI tables are using the > optional capability of embedded device-tree blobs inside the > ACPI tables ? > > That is an unusual combination on a x86 device, note it is > not wrong It's actually not okay. We have agreed at some point with DT people, that ACPI should not use non-native variants of natively supported things. For example, it shouldn't use "interrupt" property for IOxAPIC (or xIC) provided interrupts, rather Interrupt() has to be used and so on. > but AFAIK you are the first to do this on x86. No, not the first. Once Intel tried to invent the pin control configuration and muxing properties in ACPI, it was luckily rejected (ACPI 6.x OTOH provides a set of special resources for that). So, NAK from me, *if* it's really the case. ACPI tables must be revisited.
Hi, On 5/6/21 11:23 AM, Andy Shevchenko wrote: > On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com> wrote: >> I'm replying here since this series has no cover-letter, for >> the next version for a series touching so many different >> sub-systems it would be good to start with a cover-letter >> providing some background info on the series. >> >> I see this is binding to an ACPI device, yet it is also using >> devicetree bindings and properties. >> >> So I take it this means that your ACPI tables are using the >> optional capability of embedded device-tree blobs inside the >> ACPI tables ? >> >> That is an unusual combination on a x86 device, note it is >> not wrong > > It's actually not okay. We have agreed at some point with DT people, > that ACPI should not use non-native variants of natively supported > things. For example, it shouldn't use "interrupt" property for IOxAPIC > (or xIC) provided interrupts, rather Interrupt() has to be used and so > on. Right, but that is not the case here, they are using 2 device-tree properties (1), from patch 3/7: +properties: + compatible: + const: advantech,ahc1ec0 + + advantech,hwmon-profile: + description: + The number of sub-devices specified in the platform. Defines for the + hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt. + $ref: /schemas/types.yaml#/definitions/uint32 + maxItems: 1 + + advantech,has-watchdog: + description: + Some implementations of the EC include a watchdog used to monitor the + system. This boolean flag is used to specify whether this watchdog is + present or not. Default is true, otherwise set to false. + type: boolean >> but AFAIK you are the first to do this on x86. > > No, not the first. Once Intel tried to invent the pin control > configuration and muxing properties in ACPI, it was luckily rejected > (ACPI 6.x OTOH provides a set of special resources for that). > > So, NAK from me, *if* it's really the case. ACPI tables must be revisited. AFAIK Advantech are not defining things for which an ACPI standard exists, although these 2 properties might just as well may be 2 simple ACPI integer methods, which would actually make things a bit simpler (.e.g it would allow dropping patch 2/7 and 3/7 from the set). Campion, any reason why you went this route; and can the ACPI tables still be changed? Regards, Hans
On Thu, May 6, 2021 at 12:38 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/6/21 11:23 AM, Andy Shevchenko wrote: ... > Campion, any reason why you went this route; and can the ACPI tables > still be changed? Yes, the main problem with the series is the absence of a cover letter that should basically answer the (obvious) questions and give a justification.
On Thu, May 06, 2021 at 10:47:22AM +0200, Hans de Goede wrote: > Hi, > > I'm replying here since this series has no cover-letter, for > the next version for a series touching so many different > sub-systems it would be good to start with a cover-letter > providing some background info on the series. > > I see this is binding to an ACPI device, yet it is also using > devicetree bindings and properties. > > So I take it this means that your ACPI tables are using the > optional capability of embedded device-tree blobs inside the > ACPI tables ? Ugg, really. I would have stopped caring if I had realized.
Hi, Very thanks your time for reviewing. >-----Original Message----- >From: Hans de Goede <hdegoede@redhat.com> >Sent: Thursday, May 6, 2021 5:39 PM >Subject: Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded >controller entry > >Hi, > >On 5/6/21 11:23 AM, Andy Shevchenko wrote: >> On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com> >wrote: >>> I'm replying here since this series has no cover-letter, for >>> the next version for a series touching so many different >>> sub-systems it would be good to start with a cover-letter >>> providing some background info on the series. >>> Sorry about that, i will study what is cover-letter and its content. Would you kindly provide me a good reference? Can I resend a [Patch v7 0/7] for these patch or provide it in next version? >>> I see this is binding to an ACPI device, yet it is also using >>> devicetree bindings and properties. >>> >>> So I take it this means that your ACPI tables are using the >>> optional capability of embedded device-tree blobs inside the >>> ACPI tables ? >>> >>> That is an unusual combination on a x86 device, note it is >>> not wrong >> >> It's actually not okay. We have agreed at some point with DT people, >> that ACPI should not use non-native variants of natively supported >> things. For example, it shouldn't use "interrupt" property for IOxAPIC >> (or xIC) provided interrupts, rather Interrupt() has to be used and so >> on. In our experience, most risc platforms are using devicetree, and x86/64 platforms are using ACPI table or grub configure for their specific settings in different HW paltform. In this case, EC chip is a LPC interface that can be integrated in whenever risc or x86/64. So in my understand, I think it is not conflict. (please correct me if i am misunderstanding, i will try to describe more) If the EC chip is connected to the risc processor, we will bind its properties in the device-tree without modifing the source. If the EC chip is connected to the X86/64 processor, we bind its the properties in the ACPI table and also without modifing the source. Why do we need to bind the properties in ACPI or in the device-tree? Because it is an LPC interface, it cannot automatically load the driver like a USB or PCI device. In the early days, we had to install the EC driver module in our HW platform and manually load it at every boot. Different Advantech HW platforms have different properties for HWMON and others sub-systems. This causes the EC source to be a bit dirty. It is necessary to obtain the hardware platform name from the BIOS DMI table and determine its attributes according to its platform name. Now bind the attributes to ACPI table or device-tree, the EC source is more clear and universal for Advantech devices, and it is important that if the ACPI table matches, it can be automatically loaded. > >Right, but that is not the case here, they are using 2 device-tree >properties (1), from patch 3/7: > >+properties: >+ compatible: >+ const: advantech,ahc1ec0 >+ >+ advantech,hwmon-profile: >+ description: >+ The number of sub-devices specified in the platform. Defines for the >+ hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt. >+ $ref: /schemas/types.yaml#/definitions/uint32 >+ maxItems: 1 >+ >+ advantech,has-watchdog: >+ description: >+ Some implementations of the EC include a watchdog used to monitor >the >+ system. This boolean flag is used to specify whether this watchdog is >+ present or not. Default is true, otherwise set to false. >+ type: boolean > > >>> but AFAIK you are the first to do this on x86. >> >> No, not the first. Once Intel tried to invent the pin control >> configuration and muxing properties in ACPI, it was luckily rejected >> (ACPI 6.x OTOH provides a set of special resources for that). >> >> So, NAK from me, *if* it's really the case. ACPI tables must be revisited. > I am not sure it supports vendor self-defined attributes for ACPI table? >AFAIK Advantech are not defining things for which an ACPI standard exists, >although these 2 properties might just as well may be 2 simple ACPI integer >methods, which would actually make things a bit simpler (.e.g it would >allow dropping patch 2/7 and 3/7 from the set). > >Campion, any reason why you went this route; and can the ACPI tables >still be changed? > If patches 2/7 and 3/7 are removed, it will be even simpler. This means that there is no device-tree binding designed, in fact, the EC chip only be integrated in the x86/64 platform at present. Sorry, ACPI table now is integrated in the BIOS for Advantech UNO device, it may be revert to previous rule, that is, there is no ACPI table binding and manually loaded the EC driver. If you have any suggestons I would be very grateful. >Regards, > >Hans
>-----Original Message----- >From: Rob Herring <robh@kernel.org> >Sent: Friday, May 7, 2021 8:50 AM >Subject: Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 >embedded controller entry > >On Thu, May 06, 2021 at 10:47:22AM +0200, Hans de Goede wrote: >> Hi, >> >> I'm replying here since this series has no cover-letter, for >> the next version for a series touching so many different >> sub-systems it would be good to start with a cover-letter >> providing some background info on the series. >> >> I see this is binding to an ACPI device, yet it is also using >> devicetree bindings and properties. >> >> So I take it this means that your ACPI tables are using the >> optional capability of embedded device-tree blobs inside the >> ACPI tables ? > >Ugg, really. I would have stopped caring if I had realized. I am very grateful for any comments you have made in the past, and please feel free to give friendly guidance.
Hi, On 5/7/21 1:53 PM, Campion Kang wrote: > Hi, Very thanks your time for reviewing. > >> -----Original Message----- >> From: Hans de Goede <hdegoede@redhat.com> >> Sent: Thursday, May 6, 2021 5:39 PM >> Subject: Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded >> controller entry >> >> Hi, >> >> On 5/6/21 11:23 AM, Andy Shevchenko wrote: >>> On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com> >> wrote: >>>> I'm replying here since this series has no cover-letter, for >>>> the next version for a series touching so many different >>>> sub-systems it would be good to start with a cover-letter >>>> providing some background info on the series. >>>> > > Sorry about that, i will study what is cover-letter and its content. > Would you kindly provide me a good reference? > Can I resend a [Patch v7 0/7] for these patch or provide it in next version? Please add a cover letter to the next version, which will hopefully also address some of the other remarks already made. Regards, Hans > > >>>> I see this is binding to an ACPI device, yet it is also using >>>> devicetree bindings and properties. >>>> >>>> So I take it this means that your ACPI tables are using the >>>> optional capability of embedded device-tree blobs inside the >>>> ACPI tables ? >>>> >>>> That is an unusual combination on a x86 device, note it is >>>> not wrong >>> >>> It's actually not okay. We have agreed at some point with DT people, >>> that ACPI should not use non-native variants of natively supported >>> things. For example, it shouldn't use "interrupt" property for IOxAPIC >>> (or xIC) provided interrupts, rather Interrupt() has to be used and so >>> on. > > In our experience, most risc platforms are using devicetree, and x86/64 platforms > are using ACPI table or grub configure for their specific settings in different HW paltform. > In this case, EC chip is a LPC interface that can be integrated in whenever risc or x86/64. > So in my understand, I think it is not conflict. > (please correct me if i am misunderstanding, i will try to describe more) > > If the EC chip is connected to the risc processor, we will bind its properties in the device-tree without modifing the source. > If the EC chip is connected to the X86/64 processor, we bind its the properties in the ACPI table and also without modifing the source. > Why do we need to bind the properties in ACPI or in the device-tree? Because it is an LPC interface, it cannot automatically load the driver like a USB or PCI device. > In the early days, we had to install the EC driver module in our HW platform and manually load it at every boot. Different Advantech HW platforms have different properties for HWMON and others sub-systems. This causes the EC source to be a bit dirty. It is necessary to obtain the hardware platform name from the BIOS DMI table and determine its attributes according to its platform name. > Now bind the attributes to ACPI table or device-tree, the EC source is more clear and universal for Advantech devices, and it is important that if the ACPI table matches, it can be automatically loaded. > >> >> Right, but that is not the case here, they are using 2 device-tree >> properties (1), from patch 3/7: >> >> +properties: >> + compatible: >> + const: advantech,ahc1ec0 >> + >> + advantech,hwmon-profile: >> + description: >> + The number of sub-devices specified in the platform. Defines for the >> + hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + maxItems: 1 >> + >> + advantech,has-watchdog: >> + description: >> + Some implementations of the EC include a watchdog used to monitor >> the >> + system. This boolean flag is used to specify whether this watchdog is >> + present or not. Default is true, otherwise set to false. >> + type: boolean >> >> >>>> but AFAIK you are the first to do this on x86. >>> >>> No, not the first. Once Intel tried to invent the pin control >>> configuration and muxing properties in ACPI, it was luckily rejected >>> (ACPI 6.x OTOH provides a set of special resources for that). >>> >>> So, NAK from me, *if* it's really the case. ACPI tables must be revisited. >> > > I am not sure it supports vendor self-defined attributes for ACPI table? > >> AFAIK Advantech are not defining things for which an ACPI standard exists, >> although these 2 properties might just as well may be 2 simple ACPI integer >> methods, which would actually make things a bit simpler (.e.g it would >> allow dropping patch 2/7 and 3/7 from the set). >> >> Campion, any reason why you went this route; and can the ACPI tables >> still be changed? >> > > If patches 2/7 and 3/7 are removed, it will be even simpler. > This means that there is no device-tree binding designed, in fact, the EC chip only be integrated in the x86/64 platform at present. > Sorry, ACPI table now is integrated in the BIOS for Advantech UNO device, > it may be revert to previous rule, that is, there is no ACPI table binding and manually loaded the EC driver. If you have any suggestons I would be very grateful. > >> Regards, >> >> Hans >
diff --git a/MAINTAINERS b/MAINTAINERS index 83c2b1867586..984795eb6b5d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -572,6 +572,19 @@ S: Maintained F: Documentation/scsi/advansys.rst F: drivers/scsi/advansys.c +ADVANTECH AHC1EC0 EMBEDDED CONTROLLER DRIVER +M: Campion Kang <campion.kang@advantech.com.tw> +L: linux-kernel@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/mfd/ahc1ec0.yaml +F: Documentation/hwmon/ahc1ec0-hwmon.rst +F: drivers/hwmon/ahc1ec0-hwmon.c +F: drivers/mfd/ahc1ec0.c +F: drivers/platform/x86/ahc1ec0-core.c +F: drivers/watchdog/ahc1ec0-wdt.c +F: include/dt-bindings/mfd/ahc1ec0-dt.h +F: include/linux/platform_data/ahc1ec0.h + ADVANTECH SWBTN DRIVER M: Andrea Ho <Andrea.Ho@advantech.com.tw> L: platform-driver-x86@vger.kernel.org
Changed in V7: 1. According to the reviewer's comment, add two files: Documentation/hwmon/ahc1ec0-hwmon.rst and drivers/platform/x86/ahc1ec0-core.c Signed-off-by: Campion Kang <campion.kang@advantech.com.tw> --- MAINTAINERS | 13 +++++++++++++ 1 file changed, 13 insertions(+)