Message-ID: <2283497.cC5M10DNsE@pali-elitebook>
Date:	Tue, 06 Dec 2011 16:19:16 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Felipe Contreras <felipe.contreras@...il.com>
Cc:	Felipe Contreras <felipe.contreras@...ia.com>,
	linux-main <linux-kernel@...r.kernel.org>,
	linux-omap <linux-omap@...r.kernel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Aliaksei Katovich <aliaksei.katovich@...ia.com>,
	Vladimir Zapolskiy <vz@...ia.com>
Subject: Re: [PATCH] mfd: add bq2415x charger driver

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@...il.com> 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@...il.com
Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists