Hi, On 05/09/2017 at 07:27:08 +0200, Heiner Kallweit wrote: > Most code went into a new rtc-ds1307-lib library in a generic form, the driver > contains mainly chip configuration info now plus chip-specific routines for > reading / setting alarm. > Now it would require very little effort to e.g. make ds3231 a separate driver > because the hwmon code is needed for this chip only. So I had a closer look (very late, sorry...) I think too much went in the lib and it is not generic enough. What I'd like are functions that could be used from every driver handling RTC in [centisec] sec min hour day month year format, not just ds1307. The ds1307 driver has been abused because it handles that format. Maybe, we could call that rtc_bcd_* or something similar. For example, there is nothing ds1307 specific in ds1307_regs_to_tm apart from the century handling. The same for the function reading or writing the registers. A function taking a regmap and an offset will be generic enough for most drivers. So, the first step would be to make functions to handle that generically and call them from the driver. It would be good to also have the conversion of other drivers as a PoC but I can take care of that. Then, regarding the individual bits, they would be better handled using regmap_fields instead of rolling our own implementation. Those will have to be ds1307 specific. I wouldn't bother with their polarity as if it is different, it probably means a different driver is needed instead. Have read_time/set_time functions handling the osc bit the century bit(s) will make them reusable for around 40 drivers. (BTW, it seems ds1307_set_time() never resets the osc bit). I'm not sure about the alarm ops yet but they can probably be made generic enough. Also, the buffer update optimization instead of updating bits in the registers makes the code quite complicated. I'm not sure it is worth it. Also, it doesn't work well with the regmap_field idea. rtc_ops, irq handling, nvram handling, clock handling, trickle charger handling and probe should probably stay in the driver for now and will need to be separated in their own drivers later. So, let's start small and introduce functions that will work for most current drivers, handling bcd time. regmap can be used to abstract register accesses. A struct to pass register offsets and other data (like presence of wday or number of century bits) will probably be needed. I hope this is clear enough. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
