Re: [PATCH v4 15/19] watchdog: s3c2410_wdt: Add support for WTCON register DBGACK_MASK bit

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

Best regards,
Krzysztof