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:

quoted

Hi Phil

Am Freitag, den 06.05.2016, 15:27 +0800 schrieb Phil Reid:

quoted

G'day Karl
On 29/04/2016 03:15, Karl-Heinz Schneider wrote:

quoted

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.

quoted

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...

quoted

quoted

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