Message-ID: <8650361.EbH1hYJT76@pali-elitebook>
Date:	Tue, 06 Dec 2011 08:25:49 +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

Hi,

On Tuesday 06 December 2011 02:12:47 you wrote:
> Hi,
> 
> 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)

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

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

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)

My code has also prepaired boost support - for usb host mode, which must be 
done in driver.

If you do not agree with other parts, tell me about it - we can fix it.

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