Re: [PATCH v2] max77693: added device tree support

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/20/2013 06:41 AM, Andrzej Hajda wrote:
> 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?

Well, that in turn means that the core I2C binding should describe this
property, and it doesn't. So, that just means you need to apply this
part of the patch to a different document instead, but doesn't remove
the need to document exactly what it means...

But, it's reasonable to go ahead and add DT support to the driver based
on the current binding in this patch, and add the documentation for this
property to the I2C core binding document in some other patch.