Date:	Tue, 06 Dec 2011 14:27:52 +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 12:58:06 you wrote:
> On Tue, Dec 6, 2011 at 9:25 AM, Pali Rohár <pali.rohar@...il.com> wrote:
> > On Tuesday 06 December 2011 02:12:47 you wrote:
> >> On Tue, Dec 6, 2011 at 1:05 AM, Pali Rohár <pali.rohar@...il.com> wrote:
> >> > I started writing other implementaion of bq2415x charger driver,
> >> > which
> >> > should support also setting usb host mode. Code is still
> >> > unfinished,
> >> > but now is devided into 2 parts: one power_supply driver and one
> >> > driver which cover all bq registers. See:
> >> > 
> >> > http://atrey.karlin.mff.cuni.cz/~pali/bq2415x/
> >> > 
> >> > Felipe Contreras, I think that my implementation is better - it
> >> > will
> >> > export all bq registers (which is needed for hostmode boost) and
> >> > will
> >> > also register regulator interface.
> >> 
> >> I took a look at your driver, and there's definitely good stuff in it.
> >> However, I think there's a lot of unnecessary stuff, like the
> >> miscdevice stuff (which was frowned upon for bq27x00), and a lot of
> >> user-space interface. Moreover, it doesn't seem to do anything on its
> >> own (it needs interaction from user-space).
> > 
> > Ignore miscdevice, I will remove them from driver. I will add some
> > debugfs interface for getting registers output (needed for debugging)
> 
> 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.

> 
> >> IMO the first step should be to have a minimal driver that just works,
> >> even if it doesn't achieve the absolute best charging performance.
> >> More features could be added later on.
> >> 
> >> Also, I'm not familiar with the regulator interface, but it seems to
> >> be meant for real regulators, which have consumers, and based on those
> >> consumers's needs the real voltage changes. This battery charger on
> >> the other hand doesn't have anything like that. There will be no
> >> consumer, and some stuff like the weak battery voltage is not even
> >> related to a voltage supply, but rather a threshold that can be
> >> configured to change some behavior of the charger, but there's no
> >> point in changing it dynamically (or maybe at all).
> > 
> > If regulator interface is not good, I can change it to some sysfs
> > interface. But bq2415x chip driver is not only rx51 specified, so it
> > should handle all chip capabilities.
> 
> 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.

> 
> >> I guess the important one is the charge voltage, which is linked to a
> >> real voltage, but what consumers would it have? I don't think there's
> >> any.
> >> 
> >> Finally, I don't think user-space interaction should be needed at all.
> >> The driver should start charging immediately when there power supply
> >> available, and stop when there isn't any. Maybe at some point a
> >> user-space interface will be useful later on (I don't see why), but I
> >> don't think it should be *necessary*.
> > 
> > Userspace interfaction is needed. We need to tell driver to boost - for
> > usb host mode. But of course, battery charging should be automatic
> > without userspace interfaction.
> 
> 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.

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

In my opition bq2415x module should only export chip register access (to 
userspace via debugfs... and to kernel via some interface or symbols...)

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

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

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.

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.

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

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?

> 
> Cheers.

-- 
Pali Rohár
pali.rohar@...il.com
Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists