Re: [PATCH] mfd: add bq2415x charger driver

[Pali Rohár]

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.