On Thu, Jan 19, 2023 at 5:18 PM Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> wrote: > I need to find time to think about it because while setting the analog > trimming statically from the device tree solves your immediate problem, > it will also remove the possibility to handle it from userspace later > on. I would really prefer this uses the offset interface or a better > interface that unfortunately doesn't exist yet. Thanks for letting me know what you're thinking about this. I think I see what you're getting at. However, I think this is more complex than either of us had considered. The M41T82 has two different calibration capabilities: 1. Digital calibration. This looks to me like it behaves similarly to the digital calibration feature of the M41T00, which ds1307.c exposes through the offset interface. The M41T8x driver doesn't currently expose the digital calibration register at all, but if it did I would agree that the offset interface looks appropriate. 2. Analog calibration -- that's what the datasheet calls it, but the range on it is very big -- 3.5 pF all the way up to 17.4 pF -- and their reference design uses it as the only xtal load capacitance in the circuit. Most of the values you could set for this would be wildly inappropriate for any given design's choice of xtal oscillator. Between these, I don't know if you'd want to expose just one, the other, or some synthesis of both via the offset interface or some new interface. I'd make the case that the xtal's required load capacitance is a hardware requirement that's appropriate to configure via the Device Tree. Even if you did want to allow some amount of runtime fine-tuning of this register, you'd still want to document a rational starting value chosen based on the hardware. I agree with you, though, that if a runtime fine-tuning feature were added, we'd have to find a way to choose whether to initialize the register on boot or not, so that we didn't overwrite the fine-tuning. Just to demonstrate something that could work, and would be backward-compatible with this patchset, here's a hypothetical design: * dt-bindings: add quartz-load-femtofarad-tuning-min and quartz-load-femtofarad-tuning-max * Limit run-time tuning adjustments to be within that range * Only overwrite the analog calibration register on start-up if its value is outside that range After thinking through all this, I'd still advocate for merging this patchset in some form and leaving integration with runtime APIs as a potential future enhancement. I look forward to hearing your thoughts about it. -- Dennis Lambe (He/Him) Lead Firmware Engineer sparkcharge.io
