linux-kernel - Re: [PATCH 2/2] drivers: power: Add support for BATTERY

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] [day] [month] [year] [list]

Message-ID: <20130809221931.GH12638@lizard.sbx05730.santaca.wayport.net>
Date:	Fri, 9 Aug 2013 15:19:31 -0700
From:	Anton Vorontsov <anton@...msg.org>
To:	Balint Czobor <czoborbalint@...il.com>
Cc:	David Woodhouse <dwmw2@...radead.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drivers: power: Add support for BATTERY_ARIESVE

On Wed, Jul 24, 2013 at 04:27:10PM +0200, Balint Czobor wrote:
> Add initial support for the battery in Samsung ARIESVE.
> 
> Signed-off-by: Balint Czobor <czoborbalint@...il.com>
> ---
>  arch/arm/mach-msm/include/mach/msm_battery.h   |   31 +
>  arch/arm/mach-msm/include/mach/msm_rpcrouter.h |  376 ++++
>  drivers/power/Kconfig                          |   14 +
>  drivers/power/Makefile                         |    1 +
>  drivers/power/ariesve_battery.c                | 2710 ++++++++++++++++++++++++
>  drivers/power/fuelgauge_max17043.c             |  434 ++++
>  drivers/power/power_supply_sysfs.c             |   19 +-
>  include/linux/earlysuspend.h                   |   56 +
>  include/linux/leds-pmic8058.h                  |   40 +
>  include/linux/mfd/pm8xxx/batt-alarm.h          |  201 ++
>  include/linux/mfd/pm8xxx/gpio.h                |  162 ++
>  include/linux/mfd/pm8xxx/misc.h                |  284 +++
>  include/linux/mfd/pm8xxx/mpp.h                 |  263 +++
>  include/linux/mfd/pm8xxx/nfc.h                 |   79 +
>  include/linux/mfd/pm8xxx/tm.h                  |   42 +
>  include/linux/mfd/pm8xxx/upl.h                 |   65 +
>  include/linux/mfd/pm8xxx/vibrator.h            |   39 +
>  include/linux/mfd/pmic8058.h                   |  137 ++
>  include/linux/pmic8058-othc.h                  |  146 ++
>  include/linux/pmic8058-pwm.h                   |  148 ++
>  include/linux/pmic8058-xoadc.h                 |  121 ++
>  include/linux/power_supply.h                   |   13 +-
>  include/linux/regulator/pm8058-xo.h            |   31 +
>  include/linux/regulator/pmic8058-regulator.h   |   89 +
>  include/linux/wakelock.h                       |   90 +

Thanks for the driver! There are a lot of issues with the patch, though.

Most of the header files are not needed for this driver. The patch should
also be split into several parts (i.e. power_supply_sysfs.c changes should
be aside). In the patch there is a lot of dead (commented-out) code and
code that completely violates Linux coding standards.

I would say that for now, drivers/staging/ is more appropriate place for
the driver. Meanwhile, be sure to read Documentation/CodingStyle and
Documentation/SubmittingPatches. And a special url about #ifdefs in .c
files, typedefs, etc.:

http://kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/

Thanks,

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