From: swarren@wwwdotorg•org (Stephen Warren)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v2 1/5] of: Add descriptions of thermtrip properties to Tegra PMC bindings
Date: Fri, 05 Sep 2014 12:48:51 -0600 [thread overview]
Message-ID: <540A0593.7000108@wwwdotorg.org> (raw)
In-Reply-To: <5409875E.2080607@kapsi.fi>
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 at vger.kernel.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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
next prev parent reply other threads:[~2014-09-05 18:48 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-13 12:41 [PATCH v2 0/5] Thermal reset support in PMC Mikko Perttunen
2014-08-13 12:41 ` [PATCH v2 1/5] of: Add descriptions of thermtrip properties to Tegra PMC bindings Mikko Perttunen
2014-08-20 20:16 ` Stephen Warren
2014-08-21 6:58 ` Thierry Reding
2014-08-21 15:38 ` Stephen Warren
2014-08-21 16:53 ` Thierry Reding
2014-08-21 17:54 ` Stephen Warren
2014-09-05 9:50 ` Mikko Perttunen
2014-09-05 18:48 ` Stephen Warren [this message]
2014-08-20 20:22 ` Stephen Warren
2014-08-13 12:41 ` [PATCH v2 2/5] of: Add nvidia, controller-id property to Tegra I2C bindings Mikko Perttunen
2014-08-20 20:19 ` [PATCH v2 2/5] of: Add nvidia,controller-id " Stephen Warren
2014-08-21 7:05 ` Thierry Reding
2014-08-21 15:41 ` Stephen Warren
2014-08-13 12:41 ` [PATCH v2 3/5] ARM: tegra124: Add I2C controller ids to device tree Mikko Perttunen
2014-08-13 12:41 ` [PATCH v2 4/5] ARM: tegra: Add PMC thermtrip programming to Jetson TK1 " Mikko Perttunen
2014-08-13 12:41 ` [PATCH v2 5/5] ARM: tegra: Add thermal reset (thermtrip) support to PMC Mikko Perttunen
2014-08-20 20:25 ` Stephen Warren
2014-08-21 13:11 ` Mikko Perttunen
2014-08-21 15:40 ` Stephen Warren
2014-08-22 12:55 ` Mikko Perttunen
2014-08-14 11:26 ` [PATCH v2 0/5] Thermal reset support in PMC Wei Ni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=540A0593.7000108@wwwdotorg.org \
--to=swarren@wwwdotorg$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox