Hi Krzysztof / Guenter / Sam, On Wed, 22 Nov 2023 at 07:53, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 21/11/2023 19:10, Guenter Roeck wrote: > > >>> static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > >>> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > >>> .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, > >>> .cnt_en_bit = 7, > >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > >>> - QUIRK_HAS_WTCLRINT_REG, > >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > >>> }; > >>> > >> > >> This patch states it's adding the feature, but in fact it's also > >> enabling this feature for gs101. Suggest moving this patch before the > >> one enabling gs101 wdt. This way, one patch will only add the feature, > >> and another patch will enable gs101 entirely (with this feature used). > >> At least it seems like more atomic approach to me. > >> > > > > Both approaches have their merits and their downsides. I for my part am not > > even sure if DBGACK_MASK should be enabled unconditionally if supported. > > With your approach, it would be impossible (or at least more difficult) to > > revert if it turns out to be broken and/or have unexpected side effects. > > That seems less desirable to me than the current approach. > > Reversing the patches does not change this. It is enabled > unconditionally in current order as well. > > Sam's idea is correct here - first you add support for new quirk, then > you add new SoC which will use this quirk. Doing the other way - first > SoC and then new quirk - looks like SoC was added incomplete. Sure I can swap the order around if that's what you prefer. I ordered it this way so it was clear who the user of the new debug feature was. Peter