Re: [PATCH v2 1/5] of: Add descriptions of thermtrip properties to Tegra PMC bindings

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 09/05/2014 03:50 AM, Mikko Perttunen wrote:
> Hi again!
>
> I have fixed all other issues mentioned apart from this one. I can see
> three ways ahead:
>
> 1) Keep things as is. There is a slight possibility that in the future
> we will have a hardware configuration with multiple bus addresses and
> this will cause annoyance.
> 2) Keep things otherwise as is, but read the bus address from the
> thermtrip node. While at it, just have a reference to the the I2C bus
> rather than the PMIC in the bindings.
> 3) Create a new poweroff driver class with two operations: poweroff and
> get_i2c_command. The AS3722 poweroff "driver" is already separated from
> the MFD and would be relatively straightforward to port to a new driver
> subsystem. However, other PMICs we have, such as Palmas, don't do this
> and would require larger refactoring. TBH, this smells of
> overengineering to me.
> 4) Other ideas?

I think the two possible solutions are:

1) Put the following in the thermal node:

* phandle to I2C bus
* I2C address to write to (together with 7- or 10-bit indicator, if the 
HW supports that)
* Device register address to write to (together with byte count 
indicator if the HW supports that)
* Device register value (together with byte count indicator if the HW 
supports that)

This seems simplest; it's a very direct representation of the HW, and 
requires not extra infra-structure to be written or maintained. The only 
issue is that it encodes register values in the DT which has previously 
been frowned upon, and the values duplicate a tiny tiny part of the PMIC 
driver. Personally, I don't think that's a big deal though.

or:

2) Put the following in the thermal node:

* phandle to I2C device of PMIC

... and have the thermal node's driver call function(s) in the PMIC 
driver to retrieve all the information I mentioned above.

Any other option has too many holes or special cases that aren't covered.

>
> What is your opinion?
>
> Cheers,
> Mikko
>
> On 08/21/2014 08:54 PM, Stephen Warren wrote:
>> On 08/21/2014 10:53 AM, Thierry Reding wrote:
>>> On Thu, Aug 21, 2014 at 09:38:29AM -0600, Stephen Warren wrote:
>>>> On 08/21/2014 12:58 AM, Thierry Reding wrote:
>>>>> On Wed, Aug 20, 2014 at 02:16:49PM -0600, Stephen Warren wrote:
>>>>>> On 08/13/2014 06:41 AM, Mikko Perttunen wrote:
>>>>>>> Hardware-triggered thermal reset requires configuring the I2C
>>>>>>> reset procedure. This configuration is read from the device tree,
>>>>>>> so document the relevant properties in the binding documentation.
>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>
>>>>>>> +- nvidia,pmu : Phandle to power management unit / PMIC handling
>>>>>>> poweroff
>>>>>>> +- nvidia,reg-addr : I2C register address to write poweroff
>>>>>>> command to
>>>>>>> +- nvidia,reg-data : Poweroff command to write to PMU
>>>>>>
>>>>>> Why are both the PMU/PMIC phandle and the register address/data
>>>>>> required? I
>>>>>> thought the purpose of having the phandle was to allow the register
>>>>>> address
>>>>>> and data to be queried from the PMU/PMIC driver.
>>>>>>
>>>>>> To me, it seems much simpler to get rid of the phandle and just
>>>>>> hard-code
>>>>>> the I2C bus number, address, and data into this node, rather than
>>>>>> having to
>>>>>> go query it from the PMU/PMIC driver, then find the I2C controller,
>>>>>> then
>>>>>> query it for its ID (and hope that all HW modules that talk to I2C
>>>>>> controllers directly use the same numbering scheme...)
>>>>>
>>>>> I originally requested this to be changed. It seems wrong to duplicate
>>>>> information about the PMIC in both the PMIC device tree node and the
>>>>> i2c-thermtrip node if we can get the same information from the driver
>>>>> directly (via the phandle). It certainly requires a little more code,
>>>>> but at the advantage of not having to figure out the I2C controller
>>>>> hardware number and I2C slave addresses when writing the i2c-thermtrip
>>>>> node.
>>>>
>>>> I cant see that argument, but surely the PMIC driver can also supply
>>>> the
>>
>> Oops, that was meant to be can not cant.
>>
>>>> "reg-addr" and "reg-data" values too, if it's already being queried
>>>> for the
>>>> I2C device address and bus number? The binding above appears to
>>>> duplicate
>>>> part of the information, while requiring querying of the other part.
>>>
>>> I suppose that could be done. It would take a new function to do that,
>>> though, since I'm not aware of a generic mechanism to query this kind of
>>> information from a PMIC (there's no generic PMIC API, either, so perhaps
>>> it would be a good time to create one?). The I2C controller and I2C
>>> slave are generic I2C properties, whereas the register and data to power
>>> off the device are very device specific.
>>
>> I don't think it's possible to generically query an I2C device for its
>> address from the struct i2c_device object; the code still needs to call
>> a function in the PMIC driver to obtain this.
>>
>> That's because some I2C devices respond to multiple I2C addresses, and
>> there's no guarantee that the one I2C address in the DT (and hence the
>> struct i2c_device) is the address upon which the regulator function
>> exists.
>>
>> grep for i2c_new_dummy in drivers/mfd and you'll find quite a few
>> examples. The Atmel MXT touchpad/screen is another example.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>