I2C driver for LTC1760 dual smart battery system manager

I2C driver for LTC1760 dual smart battery system manager

[Karl-Heinz Schneider]

Hello,

I have written an Kernel driver for the LTC1760 which is basically an
charger which can handle 2 batteries. Datasheet can be found at
http://www.linear.com/product/LTC1760

However, the device has one speciality: Hence it handles two smart
batteries, which are expected to sit on I2C address 0x0b, it implements
an i2c mux. As the device does so, my driver does also (using
i2c_add_mux_adapter() call).
Further more, Linux already ships with an driver capable to talk to
these smart battery chips, namely "sbs-battery".

I currently using device tree to bind the LTC1760 to the smbus it sits
on and further to define the i2c-lines it implements as well as the
batteries sitting on the two muxed lines.

Would you say this approach is technically right? The LTC expects SBS
compliant batteries connected to it, which implies a standard minimal
interface. But binding the batteries via device tree gives the user the
freedom to specify a more specialized driver.
On the other hand one could argue that if the LTC is present, also
batteries are (potentially) present and the LTC driver is responsible to
read the related registers and provide proper PM attributes. Personally
I don't like to rewrite or copy code wich works just fine...

Greetings
Karl-Heinz Schneider

Re: I2C driver for LTC1760 dual smart battery system manager

[Phil Reid]

G'day Karl
On 29/04/2016 03:15, Karl-Heinz Schneider wrote:
>
> I have written an Kernel driver for the LTC1760 which is basically an
> charger which can handle 2 batteries. Datasheet can be found at
> http://www.linear.com/product/LTC1760
They're nice chips.
>
> However, the device has one speciality: Hence it handles two smart
> batteries, which are expected to sit on I2C address 0x0b, it implements
> an i2c mux. As the device does so, my driver does also (using
> i2c_add_mux_adapter() call).
> Further more, Linux already ships with an driver capable to talk to
> these smart battery chips, namely "sbs-battery".
>
> I currently using device tree to bind the LTC1760 to the smbus it sits
> on and further to define the i2c-lines it implements as well as the
> batteries sitting on the two muxed lines.
>
> Would you say this approach is technically right? The LTC expects SBS
> compliant batteries connected to it, which implies a standard minimal
> interface. But binding the batteries via device tree gives the user the
> freedom to specify a more specialized driver.
> On the other hand one could argue that if the LTC is present, also
> batteries are (potentially) present and the LTC driver is responsible to
> read the related registers and provide proper PM attributes. Personally
> I don't like to rewrite or copy code wich works just fine...
>

I've been writing a driver for the same chip :).
My system has 2 ltc1760 for a total of 4 batteries.
Haven't completed it as yet so hadn't posted, but got it talking to the batteries.
I implemented an I2c mux in the driver and just attached two sbs-battery's to it in the device tree.
I think the mux is the way to go, simple and reuses existing code.

The sbs-battery driver needs a couple of gpio pins to indicate battery presence,
so I was thinking of implementing these in the ltc1760 as gpio pins.
Not sure what the bext approach is here.

FYI, if you didn't find it there is an acpi only driver for the ltc1760 in the kernel.
But I could see a way to make it work with device trees. It enumerates it's own
batteries.

I have found the ltc1760 doesn't work to well with some i2c masters.
Currently using the bit bang i2c bus driver as that was the most reliable.
The designware controller kept locking up.



-- 
Regards
Phil Reid

Re: I2C driver for LTC1760 dual smart battery system manager

[Karl-Heinz Schneider]

Hi Phil

Am Freitag, den 06.05.2016, 15:27 +0800 schrieb Phil Reid:
> G'day Karl
> On 29/04/2016 03:15, Karl-Heinz Schneider wrote:
> >
> > I have written an Kernel driver for the LTC1760 which is basically an
> > charger which can handle 2 batteries. Datasheet can be found at
> > http://www.linear.com/product/LTC1760
> They're nice chips.
> >
> > However, the device has one speciality: Hence it handles two smart
> > batteries, which are expected to sit on I2C address 0x0b, it implements
> > an i2c mux. As the device does so, my driver does also (using
> > i2c_add_mux_adapter() call).
> > Further more, Linux already ships with an driver capable to talk to
> > these smart battery chips, namely "sbs-battery".
> >
> > I currently using device tree to bind the LTC1760 to the smbus it sits
> > on and further to define the i2c-lines it implements as well as the
> > batteries sitting on the two muxed lines.
> >
> > Would you say this approach is technically right? The LTC expects SBS
> > compliant batteries connected to it, which implies a standard minimal
> > interface. But binding the batteries via device tree gives the user the
> > freedom to specify a more specialized driver.
> > On the other hand one could argue that if the LTC is present, also
> > batteries are (potentially) present and the LTC driver is responsible to
> > read the related registers and provide proper PM attributes. Personally
> > I don't like to rewrite or copy code wich works just fine...
> >
> 
> I've been writing a driver for the same chip :).
> My system has 2 ltc1760 for a total of 4 batteries.
> Haven't completed it as yet so hadn't posted, but got it talking to the batteries.
> I implemented an I2c mux in the driver and just attached two sbs-battery's to it in the device tree.
> I think the mux is the way to go, simple and reuses existing code.
> 

Think so too.
My version doesn't do very much. It presents the "online" property and
charging state, which is also changeable to fast mode. It has some more
interesting information in it's registers, but don't know how to put
them into standard power_supply properties.

> The sbs-battery driver needs a couple of gpio pins to indicate battery presence,
> so I was thinking of implementing these in the ltc1760 as gpio pins.
> Not sure what the bext approach is here.

Would say that's not necessary. Using the GPIO for the sbs-battery
driver to detect the presence of the battery is purely optional. Indeed
it requests the GPIO as input and tries to make it an interrupt line. If
either step fails, handling the GPIO and interrupt is skipped.

> 
> FYI, if you didn't find it there is an acpi only driver for the ltc1760 in the kernel.
> But I could see a way to make it work with device trees. It enumerates it's own
> batteries.

Indeed I failed to find it. Looked through drivers to find something
similar. How is it named?

> 
> I have found the ltc1760 doesn't work to well with some i2c masters.
> Currently using the bit bang i2c bus driver as that was the most reliable.
> The designware controller kept locking up.

Did guess that could happen. The ltc1760 acts by itself as an I2C master
on it's muxed lines, and if it communicates to the currently selected
battery the masters clock line is held low during this time, preventing
the master from communicating at all. This may lead to time outs on
masters side.

I'm using an IMX6 on an SECO Q7 board, where the ltc is soldered on it's
smbus line. Did you try to reduce I2C speed? Mine is running fine at
100kHz. I don't know if there is a way to adjust time out values of I2C
masters?

Re: I2C driver for LTC1760 dual smart battery system manager

[Phil Reid]

G'day Karl,
On 7/05/2016 03:23, Karl-Heinz Schneider wrote:
> Hi Phil
>
> Am Freitag, den 06.05.2016, 15:27 +0800 schrieb Phil Reid:
>> G'day Karl
>> On 29/04/2016 03:15, Karl-Heinz Schneider wrote:
>>>
>>> I have written an Kernel driver for the LTC1760 which is basically an
>>> charger which can handle 2 batteries. Datasheet can be found at
>>> http://www.linear.com/product/LTC1760
>> They're nice chips.
>>>
>>> However, the device has one speciality: Hence it handles two smart
>>> batteries, which are expected to sit on I2C address 0x0b, it implements
>>> an i2c mux. As the device does so, my driver does also (using
>>> i2c_add_mux_adapter() call).
>>> Further more, Linux already ships with an driver capable to talk to
>>> these smart battery chips, namely "sbs-battery".
>>>
>>> I currently using device tree to bind the LTC1760 to the smbus it sits
>>> on and further to define the i2c-lines it implements as well as the
>>> batteries sitting on the two muxed lines.
>>>
>>> Would you say this approach is technically right? The LTC expects SBS
>>> compliant batteries connected to it, which implies a standard minimal
>>> interface. But binding the batteries via device tree gives the user the
>>> freedom to specify a more specialized driver.
>>> On the other hand one could argue that if the LTC is present, also
>>> batteries are (potentially) present and the LTC driver is responsible to
>>> read the related registers and provide proper PM attributes. Personally
>>> I don't like to rewrite or copy code wich works just fine...
>>>
>>
>> I've been writing a driver for the same chip :).
>> My system has 2 ltc1760 for a total of 4 batteries.
>> Haven't completed it as yet so hadn't posted, but got it talking to the batteries.
>> I implemented an I2c mux in the driver and just attached two sbs-battery's to it in the device tree.
>> I think the mux is the way to go, simple and reuses existing code.
>>
>
> Think so too.
> My version doesn't do very much. It presents the "online" property and
> charging state, which is also changeable to fast mode. It has some more
> interesting information in it's registers, but don't know how to put
> them into standard power_supply properties.
That's more than I did so far.

>
>> The sbs-battery driver needs a couple of gpio pins to indicate battery presence,
>> so I was thinking of implementing these in the ltc1760 as gpio pins.
>> Not sure what the bext approach is here.
>
> Would say that's not necessary. Using the GPIO for the sbs-battery
> driver to detect the presence of the battery is purely optional. Indeed
> it requests the GPIO as input and tries to make it an interrupt line. If
> either step fails, handling the GPIO and interrupt is skipped.
Yes, my initial pass is to ignore the battery detect. I've been looking at
how to implement the GPIO and interrupt stuff. But I've got distracted with
other priorities at the moment.

>
>>
>> FYI, if you didn't find it there is an acpi only driver for the ltc1760 in the kernel.
>> But I could see a way to make it work with device trees. It enumerates it's own
>> batteries.
>
> Indeed I failed to find it. Looked through drivers to find something
> similar. How is it named?
It wrapped up in the acpi/sbs.c driver. Does specifically mention the ltc1760.
It doesn't really do much with the charger other than use it to enumerate number of batteries
and switch the i2c mux.
Search for manager_present and you'll find the relevant code.

>
>>
>> I have found the ltc1760 doesn't work to well with some i2c masters.
>> Currently using the bit bang i2c bus driver as that was the most reliable.
>> The designware controller kept locking up.
>
> Did guess that could happen. The ltc1760 acts by itself as an I2C master
> on it's muxed lines, and if it communicates to the currently selected
> battery the masters clock line is held low during this time, preventing
> the master from communicating at all. This may lead to time outs on
> masters side.
>
> I'm using an IMX6 on an SECO Q7 board, where the ltc is soldered on it's
> smbus line. Did you try to reduce I2C speed? Mine is running fine at
> 100kHz. I don't know if there is a way to adjust time out values of I2C
> masters?
>
Haven't looked to far into at the moment. Just busy validating all the hardware for our device.
The but bang works well enough for now.
I did notice on boot that the sbs driver tries to read all the parameters from the
battery at boot. Not sure what as every time you query a sysfs param it re-reads them.
It generates alot of traffic at boot which seems unnecessary.


-- 
Regards
Phil Reid

Re: I2C driver for LTC1760 dual smart battery system manager

[Peter Rosin]

Hi!

[adding acpi people]

On 2016-05-09 03:41, Phil Reid wrote:
> G'day Karl,
> On 7/05/2016 03:23, Karl-Heinz Schneider wrote:
>> Hi Phil
>>
>> Am Freitag, den 06.05.2016, 15:27 +0800 schrieb Phil Reid:
>>> G'day Karl
>>> On 29/04/2016 03:15, Karl-Heinz Schneider wrote:
>>>>
>>>> I have written an Kernel driver for the LTC1760 which is basically an
>>>> charger which can handle 2 batteries. Datasheet can be found at
>>>> http://www.linear.com/product/LTC1760
>>> They're nice chips.
>>>>
>>>> However, the device has one speciality: Hence it handles two smart
>>>> batteries, which are expected to sit on I2C address 0x0b, it implements
>>>> an i2c mux. As the device does so, my driver does also (using
>>>> i2c_add_mux_adapter() call).
>>>> Further more, Linux already ships with an driver capable to talk to
>>>> these smart battery chips, namely "sbs-battery".
>>>>
>>>> I currently using device tree to bind the LTC1760 to the smbus it sits
>>>> on and further to define the i2c-lines it implements as well as the
>>>> batteries sitting on the two muxed lines.
>>>>
>>>> Would you say this approach is technically right? The LTC expects SBS
>>>> compliant batteries connected to it, which implies a standard minimal
>>>> interface. But binding the batteries via device tree gives the user the
>>>> freedom to specify a more specialized driver.
>>>> On the other hand one could argue that if the LTC is present, also
>>>> batteries are (potentially) present and the LTC driver is responsible to
>>>> read the related registers and provide proper PM attributes. Personally
>>>> I don't like to rewrite or copy code wich works just fine...
>>>>
>>>
>>> I've been writing a driver for the same chip :).
>>> My system has 2 ltc1760 for a total of 4 batteries.
>>> Haven't completed it as yet so hadn't posted, but got it talking to the batteries.
>>> I implemented an I2c mux in the driver and just attached two sbs-battery's to it in the device tree.
>>> I think the mux is the way to go, simple and reuses existing code.
>>>
>>
>> Think so too.

I also think the muxing in ltc1760 appears to fit nicely with the
i2c-mux framework. But I only had a cursory look...

>>> FYI, if you didn't find it there is an acpi only driver for the ltc1760 in the kernel.
>>> But I could see a way to make it work with device trees. It enumerates it's own
>>> batteries.
>>
>> Indeed I failed to find it. Looked through drivers to find something
>> similar. How is it named?
> It wrapped up in the acpi/sbs.c driver. Does specifically mention the ltc1760.
> It doesn't really do much with the charger other than use it to enumerate number of batteries
> and switch the i2c mux.
> Search for manager_present and you'll find the relevant code.

I had a look at the acpi code you point to, and it appears to be
racy.

It registers an attribute (alarm_attr) for every batteriy with the
"store" function set to acpi_battery_alarm_store, which calls
acpi_battery_set_alarm, which -- without any locking -- checks if
the mux is set correctly, updates the mux if not, and then writes
to the battery.

I see other code-paths that also appear to touch the mux in
similar ways (i.e. without locking) but I didn't really look any
further than the above, which seems to be enough of a real
problem if separate users write to the alarm attr of batteries
connected to the same manager.

The suggested fix is to register the ltc1760 mux as a real
i2c-mux, which would probably be easiest when the i2c-mux locking
update scheduled for 4.7 has landed (presently available in the
i2c for-next branch and in linux-next).

Cheers,
Peter

Re: I2C driver for LTC1760 dual smart battery system manager

[Karl-Heinz Schneider]

Greetings,

Am Montag, den 09.05.2016, 10:00 +0200 schrieb Peter Rosin:
> Hi!
> 
> [adding acpi people]
> 
> On 2016-05-09 03:41, Phil Reid wrote:
> > G'day Karl,
> > On 7/05/2016 03:23, Karl-Heinz Schneider wrote:
> >> Hi Phil
> >>
> >> Am Freitag, den 06.05.2016, 15:27 +0800 schrieb Phil Reid:
> >>> G'day Karl
> >>> On 29/04/2016 03:15, Karl-Heinz Schneider wrote:
> >>>>
> >>>> I have written an Kernel driver for the LTC1760 which is basically an
> >>>> charger which can handle 2 batteries. Datasheet can be found at
> >>>> http://www.linear.com/product/LTC1760
> >>> They're nice chips.
> >>>>
> >>>> However, the device has one speciality: Hence it handles two smart
> >>>> batteries, which are expected to sit on I2C address 0x0b, it implements
> >>>> an i2c mux. As the device does so, my driver does also (using
> >>>> i2c_add_mux_adapter() call).
> >>>> Further more, Linux already ships with an driver capable to talk to
> >>>> these smart battery chips, namely "sbs-battery".
> >>>>
> >>>> I currently using device tree to bind the LTC1760 to the smbus it sits
> >>>> on and further to define the i2c-lines it implements as well as the
> >>>> batteries sitting on the two muxed lines.
> >>>>
> >>>> Would you say this approach is technically right? The LTC expects SBS
> >>>> compliant batteries connected to it, which implies a standard minimal
> >>>> interface. But binding the batteries via device tree gives the user the
> >>>> freedom to specify a more specialized driver.
> >>>> On the other hand one could argue that if the LTC is present, also
> >>>> batteries are (potentially) present and the LTC driver is responsible to
> >>>> read the related registers and provide proper PM attributes. Personally
> >>>> I don't like to rewrite or copy code wich works just fine...
> >>>>
> >>>
> >>> I've been writing a driver for the same chip :).
> >>> My system has 2 ltc1760 for a total of 4 batteries.
> >>> Haven't completed it as yet so hadn't posted, but got it talking to the batteries.
> >>> I implemented an I2c mux in the driver and just attached two sbs-battery's to it in the device tree.
> >>> I think the mux is the way to go, simple and reuses existing code.
> >>>
> >>
> >> Think so too.
> 
> I also think the muxing in ltc1760 appears to fit nicely with the
> i2c-mux framework. But I only had a cursory look...
> 
> >>> FYI, if you didn't find it there is an acpi only driver for the ltc1760 in the kernel.
> >>> But I could see a way to make it work with device trees. It enumerates it's own
> >>> batteries.
> >>
> >> Indeed I failed to find it. Looked through drivers to find something
> >> similar. How is it named?
> > It wrapped up in the acpi/sbs.c driver. Does specifically mention the ltc1760.
> > It doesn't really do much with the charger other than use it to enumerate number of batteries
> > and switch the i2c mux.
> > Search for manager_present and you'll find the relevant code.
> 
> I had a look at the acpi code you point to, and it appears to be
> racy.
> 
> It registers an attribute (alarm_attr) for every batteriy with the
> "store" function set to acpi_battery_alarm_store, which calls
> acpi_battery_set_alarm, which -- without any locking -- checks if
> the mux is set correctly, updates the mux if not, and then writes
> to the battery.
> 
> I see other code-paths that also appear to touch the mux in
> similar ways (i.e. without locking) but I didn't really look any
> further than the above, which seems to be enough of a real
> problem if separate users write to the alarm attr of batteries
> connected to the same manager.
> 
> The suggested fix is to register the ltc1760 mux as a real
> i2c-mux, which would probably be easiest when the i2c-mux locking
> update scheduled for 4.7 has landed (presently available in the
> i2c for-next branch and in linux-next).

Then I will take a look at this new stuff. For the device I target I'm
forced to use an old 3.14 Kernel :-(.

At the moment I'm doing some further tests, especially the thing with
read/write failures which do happen from time to time does bother me
(ltc1760 does stretch i2c clock). Once I'm happy with it I will post
code.
> 
> Cheers,
> Peter

Greetings
Karl-Heinz