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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
