Re: [PATCH v3 7/8] watchdog: max77714: add driver for the watchdog in the MAX77714 PMIC

[Guenter Roeck]

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.

Thanks,
Guenter