Hi Guenter, Krzysztof, On 12/11/21 20:23, Guenter Roeck wrote: > On 11/12/21 8:07 AM, Krzysztof Kozlowski wrote: >> On 12/11/2021 17:02, Luca Ceresoli wrote: >>> Hi Guenter, >>> >>> On 12/11/21 15:57, Guenter Roeck wrote: >>>> On 11/11/21 2:58 PM, Luca Ceresoli wrote: >>>>> Add a simple driver to support the watchdog embedded in the Maxim >>>>> MAX77714 >>>>> PMIC. >>>>> >>>>> Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> >>>>> >>>> >>>> I just realized that this is effectively a rewrite of >>>> drivers/watchdog/max77620_wdt.c. >>>> The only difference I can see is is the register offsets (0x91 and 0x92 >>>> vs. 1 and 2) and some implementation details. Please add support for >>>> this >>>> watchdog to the other driver or provide a _really_ good reason why that >>>> is not possible. >>> >>> I initially started developing MAX77714 watchdog support as an addition >>> to max77620_wdt.c as the procedures look identical at least for the >>> basic features. >>> >>> But the register content seems completely different. Here are the notes >>> I took at that time: >>> >>> -------------------------8<------------------------- >>> >>> MAX77620 has reg ONOFFCNFG1 at 0x41, ONOFFCNFG2 at 0x42. >>> MAX77714 has reg CNFG1_ONOFF at 0x93, CNFG2_ONOFF at 0x94. >>> OK, we can handle this with a register indirection table, indexed by >>> chip model. >>> >>> MAX77620 has MAX77620_REG_FPS_CFG0 register. >>> On MAX77714 I was unable to find any such register (I haven't looked at >>> FPS in detail though). >>> OK, we can handle this with some if()s or entirely disable PM on the >>> 77714 until anybody cares. >>> >>> MAX77620 ONOFFCNFG1 has SFT_RST in bit 7. >>> MAX77714 CNFG1_ONOFF has SFT_RST is bit 6. >>> Uhm, should we have a _bit_ indirection table in addition to the >>> _register_ indirection table? >>> >>> MAX77620 ONOFFCNFG2 bit 5 is SLP_LPM_MSK, involved in FPS. >>> MAX77620 ONOFFCNFG2 bit 6 is WD_RTS_WK, configures the watchdog timer. >>> MAX77714 CNFG2_ONOFF bit 5 is WD_RTS_WK, configures the watchdog timer. >>> On MAX77714 I haven't found SLP_LPM_MSK. >>> >>> MAX77620 has 6 CID registers with "ES version" in CID5. >>> MAX77714 has 5 CID registers with "DEVICE id" in CID3. >>> CID registers would be useful to get the chip model directly from the >>> chip, if only they had the same structure. >>> >>> Almost all of the registers I have been looking into have similar >>> differences. >>> >>> -------------------------8<------------------------- >>> >>> When I started adding indirection tables the driver started growing >>> bigger and uglier, and that little simple driver started being big and >>> complex. So I opted to add a new driver. >>> >> >> The register offset differences are trivial and we do it in several >> drivers. Also in rtc-max77686 used by you here. >> Lack of features as well - just have a variant/driver data which defines >> certain features (true/false) or quirk bits (see s3c2410_wdt). >> >> The second driver - s3c2410_wdt - also customizes the bits. >> >> Therefore if the generic device operating configuration is similar (same >> generic control flow) and differences are in bits and offsets, then it >> should be one driver. >> > > Exactly. Ok, I'll do that and send v4. Now I realize I should have mentioned from the beginning the reasons for creating a new driver, so this discussion would have been cleared much earlier. Apologies for that. Patches 1-6 + 8 are not impacted and will need no change for this issue. -- Luca
