Message-ID: <CAMP44s3hxKgP_epT48vwqwM-jme7ZsZx0U1xW4DTrn9aSpb6hg@mail.gmail.com>
Date:	Tue, 6 Dec 2011 12:58:06 +0200
From:	Felipe Contreras <felipe.contreras@...il.com>
To:	Pali Rohár <pali.rohar@...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

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.

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

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

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

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

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.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists