linux-kernel - Re: [PATCH] mfd: Implement devicetree support for AB8500 Btemp

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 10 Jul 2012 14:12:01 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	linux-arm-kernel@...ts.infradead.org
Cc:	"Rajanikanth H.V" <rajanikanth.hv@...ricsson.com>,
	linux-kernel@...r.kernel.org, linaro-dev@...ts.linaro.org,
	lee.jones@...aro.org, patches@...aro.org
Subject: Re: [PATCH] mfd: Implement devicetree support for AB8500 Btemp

On Tuesday 10 July 2012, Rajanikanth H.V wrote:

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt
> @@ -0,0 +1,54 @@
> +AB8500 Battery Termperature Monitor Driver
> +
> +AB8500 is a mixed signal multimedia and power management
> +device comprising: power and energy management module,
> +WalliCharger and USB charger interface, audio, general
> +purpose ADC TVOut, clock management and SIM card Interface.
> +
> +Battery temperature monitoring support is part of 'energy
> +management module', the other components of this module
> +are: 'main and USB Combo charger' and fuel guage.
> +
> +The properties below describes the node for battery
> +temperature monitor driver.
> +
> +Required Properties:
> +- compatible = "stericsson,ab8500-btemp"
> +
> +interrupts:
> +	Four battery temperature ranges are be defined
> +	which results in interrupt events as:
> +	- Btemp
> +	- BtempLow
> +	- BtempMedium
> +	- BtempHigh
> +

These names do not match the five interrupts in the example or in the
code. When you provide an "interrupt-names" property you have to define
the exact strings that are permissible for them in the binding.

> +Supplied-to:
> +	This shall be power supply class dependency where in the runtime battery
> +	properties will be shared across fuel guage and charging algorithm driver.

I probably don't understand enough of this, but shouldn't the other devices
that are supplied by this have a reference to this node rather than doing
it this way around? Why use strings here instead of phandles?

You are also not listing some of the properties that are in the device
tree here, like the "interrupts" property itself.

> diff --git a/arch/arm/mach-ux500/board-mop500-bm.c b/arch/arm/mach-ux500/board-mop500-bm.c
> new file mode 100644
> index 0000000..3349ceb
> --- /dev/null
> +++ b/arch/arm/mach-ux500/board-mop500-bm.c

Didn't we conclude that this file has no board-specific information in it?
Either explain why it's still here, or move it into the driver itself.

> +/*
> + * Note that the batres_vs_temp table must be strictly sorted by falling
> + * temperature values to work.
> + */
> +#ifdef CONFIG_AB8500_9100_LI_ION_BATTERY
> +#define BATRES	180
> +#else
> +#define BATRES	300
> +#endif

I think I mentioned before that you need to get rid of the
CONFIG_AB8500_9100_LI_ION_BATTERY symbol. If you have exclusive
compile-time options, it is impossible to build a kernel that
runs on all system, so this has to be a run-time option.

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