Hello, On Tuesday 06 December 2011 16:11:33 Felipe Contreras wrote: > On Tue, Dec 6, 2011 at 3:27 PM, Pali Rohár <pali.rohar@xxxxxxxxx> wrote: > > On Tuesday 06 December 2011 12:58:06 you wrote: > >> Ok. A device with such debugfs would be nice, but I would start > >> without one, just something that works. > > > > I think we do not need in mainline kernel driver which "only works" > > without any debug or additional support. > > I think that's a good start, and that's what I will focus on. > > >> I don't know if the regulator interface makes sense, but I think not. > >> Anyway, I don't see how my code is specific to rx51, it should work > >> with all bq2415x models. > > > > voltage and current values could be different for other boards. So each > > board (with bq2415x chip) should have defined default charge properties > > (in platform data structure or something else...). your interface does > > not support such as other changes. > > No, they wouldn't, that depends on many things, like the type of > charger. As Sebastian pointed out, the current *sense* voltage, is > board specific, but that's about it. > > >> Why do we need user-space for the boost mode? > > > > Because on n900 we *want* USB host mode. Without boost mode support (in > > kernel driver) again will need to rmmod driver (now we stopping BME) > > and start handling it in userspace. > > But we can have boost mode *without* user-space. There's no reason why > it can't be handled by the kernel. I thought that: we need from userspace signal "now enable host mode" and "now disable host mode". So kernel driver should has sysfs for enable/disable host mode - only this, all other will be of course handled in kernel. > > >> >> I'm not familiar with any of this stuff, so don't take my > >> >> opinions too seriously :) > >> > > >> > Consider my code. We do not need two (or more) implementation of > >> > same > >> > driver in kernel. And also we do not need only rx51 specified > >> > code. > >> > >> Of course, that's why I am discussing this :) > >> > >> > I separated bq2415x register access into one module (bq2415x.c - > >> > without any logic, only cover chip options) and real battery > >> > charging should be done in power_supply interface > >> > (bq2415x_charger.c) > >> > >> I don't see the point of having two drivers. > > > > Because proper charging on n900 needs interact with isp1704 driver. But > > this is specified for n900, not for all boards. bq2415x module should > > be general for all boards - so it should cover *only* bq2415x chip - > > nothing other. > Yes, and that be done with hooks. The bq2415x driver will have hooks, > and the rx51 board configuration will connect isp1704 to bq2415x, and > that's it. No need for yet another driver. Ok, this sounds good. > > >> > My code has also prepaired boost support - for usb host mode, > >> > which must be done in driver. > >> > >> Well, yeah, in my driver it can be added as well, however, I don't > >> think it's _needed_ right now. > > > > Of course, but I (and maybe some other people) do not need uncompleted > > chip driver. > > Anything is better than what we have now, which is nothing. Look for > example to how the bq27x00 battery driver evolved; it started very > simple. I know, I sent more bq27x00_battery patches :-) > > >> First, I would like something that works by itself (without > >> user-space), which I already have. Next, I would like it to plug into > >> isp1704 to detect when a charger is connected, and select the correct > >> limits accordingly. I guess this hooks should be connected on the > >> board code. Once having that, I think the driver should be ready for > >> merging, the rest of the features can come later. > > > > Working without userspace is my primary goal. But also for debugging > > (and > > status apps/scripts) is needed direct register access. isp1704 > > interaction should not be in bq2415x chip driver, but in some rx51 > > specified code. > Yes. > > > Charging should be done in power_supply interface. > > > > See also api specification by Joerg Reisenweber (one of n900 usb > > hostmode > > support) on http://maemo.cloud-7.de/bq24150-sysnode.spec.txt > > Similar interface is needed for proper usb host mode. > > That is very interesting... Is there yet another module for this? > Again, I don't understand why interaction with user-space is *needed* > for host mode. No other module exists - my is not finished :-) Host mode only needs to kernel enable or disable it. > > > Also your driver does not handle errors, when charging and watchdog > > should be stopped. Charging is *very* crytical parts and it really > > should detect errors. > Indeed, that's why this is RFC only. > > > When in future I (or someone else) will want to add all missing features > > into bq2415x chip driver, it will be needed to rewrite it... (e.g. > > handling errors in boost mode)... > > Perhaps, but I don't think so. Anyway, again, see the evolution of > bq27x00, or basically anything in the kernel. If something needs to be > refactored for new features, so be it. > > But I think there is some consensus; the drivers should be in > drivers/power, and have a power supply interface, rx51 board info > should configure some sense voltage, and hook it up with > isp1704_charger somehow. Once this is done and driver is merged, I > don't expect that to change. Ok, I agree with this. > > > Why to very very quicky merge uncompleted (but working) driver to > > upstream? I think we should finish bq2415x chip driver and if all will > > be implemented, then to merge it. What other developers think about > > that? > > This version of the driver is not the one I am proposing to merge. The > one I'm proposed to merge should be a good basis for future work (or > what you are doing right now). > > Cheers. I updated my last code at http://atrey.karlin.mff.cuni.cz/~pali/bq2415x/ Now my question is: Should I stop working on my implementation and wait until you finish yours? Or start merging code? -- Pali Rohár pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: This is a digitally signed message part.
