Re: [RFC v1 3/6] power: supply: bq27xxx: Enable data memory update for certain chips

[Sebastian Reichel <sebastian.reichel@collabora.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi,

On Sun, Jul 09, 2017 at 05:12:34PM +0200, Pali Rohár wrote:
> On Sunday 09 July 2017 16:13:59 Liam Breck wrote:
> > On Sun, Jul 9, 2017 at 2:07 AM, Pali Rohár <pali.rohar@gmail.com>
> > wrote:
> > > On Sunday 09 July 2017 04:16:57 Liam Breck wrote:
> > >> From: Liam Breck <kernel@networkimprov.net>
> > >> 
> > >> Support data memory update on BQ27500, 545, 425, 421, 441, 621.
> > >> With exception of BQ27425, these are only enabled #ifdef DEBUG,
> > >> as they are not tested.
> > >> 
> > >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > >> ---
> > >> 
> > >>  drivers/power/supply/bq27xxx_battery.c | 164
> > > 
> > > Why under #ifdef DEBUG? Seems that this is misusing DEBUG which is
> > > used for other things.
> > 
> > This is a temporary measure, as those definitions are not tested on
> > their chips. Nothing new happens in debug mode unless
> > monitored-battery is set in driver's DT config, and it's either a
> > ram-only chip or the dt_updates_nvm config option is set.
> > 
> > If DEBUG isn't ok in this case, alternative suggestions?
> > 
> > Thanks for your input!
> 
> DEBUG can be enabled by other kernel option and should provide just more 
> debugging functionality. But you are adding non-debug functionality 
> under #ifdef DEBUG, which basically is "new experimental untested 
> feature".
> 
> Better way for "experimental feature" would be introduction of other 
> kconfig switch or probably runtime module option...

As far as I understand it, there is a risk to brick hardware
when internal data is modified. So if these chips are untested,
let's just leave them out.

Testing should be done by a developer anyways and then he can
just add the required values.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]