linux-kernel - Re: [PATCH v2] max77693: added device tree support

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: <52136400.7040406@samsung.com>
Date:	Tue, 20 Aug 2013 14:41:36 +0200
From:	Andrzej Hajda <a.hajda@...sung.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Lee Jones <lee.jones@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ian.campbell@...rix.com>,
	Rob Landley <rob@...dley.net>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Grant Likely <grant.likely@...aro.org>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Jonghwa Lee <jonghwa3.lee@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] max77693: added device tree support

Hi,

Thanks for the review.

On 08/19/2013 11:18 PM, Stephen Warren wrote:
> On 08/19/2013 05:40 AM, Andrzej Hajda wrote:
>> max77693 mfd main device uses only wakeup field
>> from max77693_platform_data. This field is mapped
>> to wakeup-source common property in device tree.
>> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
>>  Optional properties:
>>  - regulators : The regulators of max77693 have to be instantiated under subnod
>>    named "regulators" using the following format.
>> +- wakeup-source : Indicates if the device can wakeup the system from the sleep
>> +  state.
> Does the property mean "can" or "should"?
>
> "Can" implies that the property means something about the HW. What
> exactly does it mean; perhaps that some specific output pin of the chip
> has been wired to an input IRQ/GPIO of the SoC or PMIC that (can) wake
> up the system? If so, which pin, signal, ...? Also, doesn't this also
> depend on the SoC itself supporting its input IRQ/GPIO as a wakeup
> source, so isn't some co-ordination required between the SoC and chip,
> such that this property doesn't mean "can wakeup the system", but simply
> "a signal is routed to the SoC, so perhaps it can wakeup the system".
>
> "Should" implies policy, which probably shouldn't be represented in
> device tree, since DT should describe the HW and not how it should be used.
After short digging I have realized that "wakeup-source" property is
already parsed by 
of_i2c_register_devices core function and it should not be parsed again
by the driver itself.
So I suppose the description of it can be removed from max77693 binding.
Do you agree?

Anyway I will prepare separate patch removing wakeup related code from
probe and
related field from max77693 platform_data - i2c_board_info::flag field
should be used instead.
>
> Finally, if there was already a binding for max77693.txt, I don't think
> the patch subject "added device tree support" is entirely accurate; this
> change to the binding document seems to be more about adding a new
> feature than adding DT support to the driver...
>
This patch really adds DT support to max77693 mfd driver :)

The binding file which is already in kernel currently has no related
code in the max77693 driver.
I will add clarification in the commit message.

Regards
Andrzej


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