| Message ID | cover.1696495372.git.advantech.susiteam@gmail.com |
|---|---|
| Headers | show |
| Series | watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver | expand |
On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote: > From: Wenkai <advantech.susiteam@gmail.com> > > This patch series aims to add support for the Advantech EIO-IS200 > Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200 > is a widely used embedded controller, and this series introduces a > native driver for its watchdog timer functionality within the Linux > ecosystem. > I am not going to review this patch series. This is just one watchdog driver. One patch is sufficient. Guenter
Guenter Roeck 於 10/6/2023 11:02 AM 寫道: > On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote: >> From: Wenkai <advantech.susiteam@gmail.com> >> >> This patch series aims to add support for the Advantech EIO-IS200 >> Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200 >> is a widely used embedded controller, and this series introduces a >> native driver for its watchdog timer functionality within the Linux >> ecosystem. >> > I am not going to review this patch series. This is just ne watchdog driver. > One patch is sufficient. > > Guenter Hi Guenter, Advantech's EIO-IS200 watchdog supports 5 output pins: RESET, Power Button, SCI, IRQ, and GPIO. The most traditional scenario is that the Pretimeout triggers IRQ, and the timeout triggers RESET. However, unfortunately, for industrial usages, there are various use cases, which require certain mechanisms and logic to manage which signal is output when Pretimeout and timeout expire. I am concerned that consolidating all these features into a single patch for upstream may lead to confusion and make the source code less readable and understandable. Therefore, I have divided the implementation into 5 separate patches, aiming to make the code more comprehensible and acceptable. If it's acceptable to you, I am more than willing to provide a single patch as per your preference. I would also like to note that this watchdog driver is part of the EIO-IS200 MFD (Multi-Function Device) driver family. It serves as one of the child-drivers of the drivers/mfd/eiois200_core core driver. It's important to mention that without the presence of drivers/mfd/eiois200_core, this child-driver eiois200_wdt cannot be compiled or used. Should we wait until the core driver (drivers/mfd/eiois200_core) is successfully incorporated before upstreaming the watchdog child-driver, or would you prefer to proceed with these patches independently? Best regards, Wenkai
On Fri, Oct 06, 2023 at 05:27:48PM +0800, Wenkai wrote: > > > Guenter Roeck 於 10/6/2023 11:02 AM 寫道: > > On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote: > > > From: Wenkai <advantech.susiteam@gmail.com> > > > > > > This patch series aims to add support for the Advantech EIO-IS200 > > > Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200 > > > is a widely used embedded controller, and this series introduces a > > > native driver for its watchdog timer functionality within the Linux > > > ecosystem. > > > > > I am not going to review this patch series. This is just ne watchdog driver. > > One patch is sufficient. > > > > Guenter > Hi Guenter, > > Advantech's EIO-IS200 watchdog supports 5 output pins: RESET, Power > Button, SCI, IRQ, and GPIO. The most traditional scenario is that the > Pretimeout triggers IRQ, and the timeout triggers RESET. > > However, unfortunately, for industrial usages, there are various use > cases, which require certain mechanisms and logic to manage which signal > is output when Pretimeout and timeout expire. I am concerned that > consolidating all these features into a single patch for upstream may > lead to confusion and make the source code less readable and > understandable. > The 1st patch in your series doesn't even compile. I don't call that understandable. Oh, it fails to compile because you include a non-existing file from ../mfd directly and because you select a non-existing configuration option instead of depending on it. None of those is even remotely acceptable. Are you seriously sending me a series of patches that don't even build to review ? > Therefore, I have divided the implementation into 5 separate patches, > aiming to make the code more comprehensible and acceptable. If it's > acceptable to you, I am more than willing to provide a single patch as > per your preference. > Frankly, your series is one more nail in the coffin. I am now seriously considering to resign as co-maintainer of the watchdog subsystem. Guenter
Guenter Roeck 於 10/6/2023 10:16 PM 寫道: > On Fri, Oct 06, 2023 at 05:27:48PM +0800, Wenkai wrote: >> >> Guenter Roeck 於 10/6/2023 11:02 AM 寫道: >>> On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote: >>>> From: Wenkai <advantech.susiteam@gmail.com> >>>> >>>> This patch series aims to add support for the Advantech EIO-IS200 >>>> Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200 >>>> is a widely used embedded controller, and this series introduces a >>>> native driver for its watchdog timer functionality within the Linux >>>> ecosystem. >>>> >>> I am not going to review this patch series. This is just ne watchdog driver. >>> One patch is sufficient. >>> >>> Guenter >> Hi Guenter, >> >> Advantech's EIO-IS200 watchdog supports 5 output pins: RESET, Power >> Button, SCI, IRQ, and GPIO. The most traditional scenario is that the >> Pretimeout triggers IRQ, and the timeout triggers RESET. >> >> However, unfortunately, for industrial usages, there are various use >> cases, which require certain mechanisms and logic to manage which signal >> is output when Pretimeout and timeout expire. I am concerned that >> consolidating all these features into a single patch for upstream may >> lead to confusion and make the source code less readable and >> understandable. >> > The 1st patch in your series doesn't even compile. I don't call that > understandable. > > Oh, it fails to compile because you include a non-existing file from > ../mfd directly and because you select a non-existing configuration option > instead of depending on it. > > None of those is even remotely acceptable. Are you seriously sending me > a series of patches that don't even build to review ? I understand that the patches don't meet the expected quality standards. The compile issue is due to my MFD core driver, which is currently under review and has not been merged yet. I would also like to seek your advice on how to best proceed with the sub-drivers like the watchdog driver. Should I wait for my core MFD driver to be successfully merged before submitting the sub-drivers, or let Jones Lee review my core MFD driver and all its sub-drivers, or is there another approach that you recommend? >> Therefore, I have divided the implementation into 5 separate patches, >> aiming to make the code more comprehensible and acceptable. If it's >> acceptable to you, I am more than willing to provide a single patch as >> per your preference. >> > Frankly, your series is one more nail in the coffin. I am now seriously > considering to resign as co-maintainer of the watchdog subsystem. > > Guenter I also appreciate your patience, dedication, and valuable contributions to the Linux community. Your longstanding efforts and expertise are commendable and have been instrumental in advancing the Linux ecosystem. I understand that upstream review can be a meticulous and vital, albeit thankless, task. I don't want my actions to cause any inconvenience or distress, especially to someone as esteemed as you are in the Linux community. Your insights and guidance are incredibly valuable to all of us. Once again, thank you for your understanding, and I am committed to delivering high-quality code for your review. Best regards, Wenkai
On Wed, Oct 11, 2023 at 12:08:57PM +0800, Wenkai wrote: > > I understand that the patches don't meet the expected quality standards. > The compile issue is due to my MFD core driver, which is currently under > review and has not been merged yet. > > I would also like to seek your advice on how to best proceed with the > sub-drivers like the watchdog driver. Should I wait for my core MFD > driver to be successfully merged before submitting the sub-drivers, or > let Jones Lee review my core MFD driver and all its sub-drivers, or is > there another approach that you recommend? If the sub-drivers depend on the mfd driver, at least provide a reference to the patch or patch series introducing that driver. Either case, a direct include from "../mfd" is simply unacceptable. include/linux/mfd/ does exist for a reason, after all. I don't know the best solution for reviewing all the drivers. I didn't (and do not plan to) look into the driver-driver API. If the interface is regmap, reviewing sub-drivers on their own should be straightforward. If the API is with function calls, things get more complicated because the API needs the sub-drivers to be tested and everything needs to be reviewed together. Guenter
Guenter Roeck 於 10/11/2023 11:05 PM 寫道: > On Wed, Oct 11, 2023 at 12:08:57PM +0800, Wenkai wrote: >> I understand that the patches don't meet the expected quality standards. >> The compile issue is due to my MFD core driver, which is currently under >> review and has not been merged yet. >> >> I would also like to seek your advice on how to best proceed with the >> sub-drivers like the watchdog driver. Should I wait for my core MFD >> driver to be successfully merged before submitting the sub-drivers, or >> let Jones Lee review my core MFD driver and all its sub-drivers, or is >> there another approach that you recommend? > If the sub-drivers depend on the mfd driver, at least provide a reference > to the patch or patch series introducing that driver. Either case, a direct > include from "../mfd" is simply unacceptable. include/linux/mfd/ does exist > for a reason, after all. The LKML link is https://lkml.org/lkml/2023/9/6/1245. Is this link to the patch sufficient? And, I'll move the "eiois200.h" to "include/linux/mfd/". > I don't know the best solution for reviewing all the drivers. I didn't > (and do not plan to) look into the driver-driver API. If the interface > is regmap, reviewing sub-drivers on their own should be straightforward. > If the API is with function calls, things get more complicated because > the API needs the sub-drivers to be tested and everything needs to be > reviewed together. > > Guenter Unfortunately, all sub-drivers mostly communicate with the EC firmware through the MFD core driver's driver-driver API, only a few uses regmap. So should the MFD core driver and its sub-drivers be reviewed by the same maintainers? Best regards, Wenkai
From: Wenkai <advantech.susiteam@gmail.com> This patch series aims to add support for the Advantech EIO-IS200 Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200 is a widely used embedded controller, and this series introduces a native driver for its watchdog timer functionality within the Linux ecosystem. Driver Features: - Complete support for the Advantech EIO-IS200 Embedded Controller's hardware watchdog timer. - Seamless integration with the Linux Watchdog framework, enabling standard watchdog functionality. - Flexible configuration options for watchdog timeout and event types. - Module parameters for setting default timeout and event type. - The EIO-IS200 can select special event output pin such as PWRBTN (Power button),SCI (ACPI System Control Interrupt), IRQ, and GPIO Driver Dependencies: - The driver relies on the Linux Multi-Function Device (MFD) framework and related dependencies. - Assumption of the presence of the Advantech eiois200_core MFD core driver. Wenkai (5): watchdog: eiois200_wdt: Constructing Advantech EIO-IS200 watchdog driver watchdog: eiois200_wdt: Add PMC support with eiois200_core. watchdog: eiois200_wdt: Implement basic watchdog functionalities watchdog: eiois200_wdt: Enhanced watchdog functionality and pretimeout watchdog: eiois200_wdt: Enhanced IRQ trigger behavior drivers/watchdog/Kconfig | 14 + drivers/watchdog/Makefile | 1 + drivers/watchdog/eiois200_wdt.c | 658 ++++++++++++++++++++++++++++++++ 3 files changed, 673 insertions(+) create mode 100644 drivers/watchdog/eiois200_wdt.c base-commit: 9aab92bc3a8922d4b2e24d10271dfe3034cbf5c2