Re: [PATCH v2] rtc: add Abracon ABx80x driver

[Philippe De Muyter <phdm@macqel.be>]

Hi Alexandre,

On Wed, Mar 04, 2015 at 10:06:01AM +0100, Alexandre Belloni wrote:
> On 04/03/2015 at 09:52:42 +0100, Philippe De Muyter wrote :
> > > > And is the naming in Philippe's driver appropriate?  If it supports the
> > > > AB1801 (for example) then why is it described as an "abx805" driver?
> > > 
> > > The real naming is in the form ABx8yz. With:
> > > 
> > > x: 0 or 1, indicate the presence of the reset management
> > > y: 0 or 1: 0 is i2c, 1 is spi
> > > z: [1-5]: different amount of on chip SRAM. From what I understand, only
> > > ABx8y5 are actually recommended for new designs.
> > 
> > My driver is based on the datashet called 'AB18X5 Real-Time Clock with
> > Power Management Family', which calls the parts 'AB18X5 family' with X
> > being '0' for I2c parts and '1' for SPI parts.
> > 
> > As the part I have and the driver I wrote are I2C, not SPI, I fixed the
> > 'X' as 0 in the name of the driver.
> > 
> > The datasheet lists only one part as being 'software and pin compatible'
> > with the 'AB1805': the 'AB0805', hence the 'x' in the name I choose : abx805.
> > 
> 
> The "AB08XX Real-Time Clock Family" document states that they are all
> software and pin compatible (including the AB18xx).

Which part(s) do you really have ?  Mine is a 1805.  Do all the parts have
the trickle charger ?  As I don't know, I prefer not to pretend that the
driver supports those chips.

> 
> > Actually, my driver is used in production and works fine, because the
> > default(reset) value of the 12/24 mode bit is '24 hour mode' and the
> > default value of the 'write RTC bit' enables writing.  There is
> > no real need to play with them.
> > 
> 
> I know this is unlikely to happen but what if someone messes with the
> RTC in the bootloader?

Yes, you may unconditionnaly rewrite this register if you want.

> 
> > > 
> > > What I like in Philippe's driver is the info printed at probe time and
> > > the support for trickle charging. However, I wouldn't enable it
> > > unconditionally.
> > 
> > My hardware colleagues told me that the only way to enable the 'ultra low-power'
> > functionality is enabling the trickle charger.  And the 'ultra low-power'
> > functionality is the reason we choose that chip, so I would at least
> > keep that as the default behaviour.
> > 
> 
> My concern is that you have a static configuration. I would expose a
> sysfs interface to configure the diode, resistor and enable/disable the
> trickle charger. Would that work for you?

I think a 'of_xxx' dts/dtb description is the way to go, but I did not want to
introduce unnecessaty complexity without knowing other usages.  My hardware
colleagues followed the recommended design from Abracon.

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles