linux-kernel - Re: [PATCH] mfd: add bq2415x charger driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMP44s1TrRkcy-xS+fXMyv0FAL1TnsPnnTa=gHSd2=P=iFkSUg@mail.gmail.com>
Date:	Tue, 6 Dec 2011 02:12:47 +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

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

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

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

I'm not familiar with any of this stuff, so don't take my opinions too
seriously :)

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/