| 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
| ||
|
Date: Thu, 8 Jun 2017 12:11:35 -0500
From: Grygorii Strashko <grygorii.strashko@...com>
To: Enric Balletbo Serra <eballetbo@...il.com>,
Mark Brown <broonie@...nel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Lee Jones <lee.jones@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Tony Lindgren <tony@...mide.com>
CC: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
<linux-leds@...r.kernel.org>, <linux-input@...r.kernel.org>,
Daniel Thompson <daniel.thompson@...aro.org>,
Jingoo Han <jingoohan1@...il.com>,
Richard Purdie <rpurdie@...ys.net>,
Jacek Anaszewski <jacek.anaszewski@...il.com>,
Pavel Machek <pavel@....cz>,
Mark Rutland <mark.rutland@....com>,
Russell King <linux@...linux.org.uk>,
Javier Martinez Canillas <javier@...hile0.org>
Subject: Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device
tree
On 06/08/2017 08:16 AM, Enric Balletbo Serra wrote:
> 2017-06-07 18:05 GMT+02:00 Grygorii Strashko <grygorii.strashko@...com>:
>>
>>
>> On 06/07/2017 05:32 AM, Enric Balletbo i Serra wrote:
>>> The driver boots only via device tree but currently all the MFD sub-devices
>>> are instatiated independently of the device tree configuration, i.e
>>> although tps65217-charger is disabled by default it's instantiated by the
>>> MFD driver.
>>>
>>> Instead of register all sub-devices, if the TPS65217 device tree node has a
>>> sub-node enabled, try to instatiate them as MFD sub-devices but not
>>> instantiate sub-nodes that are not enabled.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
>>> ---
>>> drivers/mfd/tps65217.c | 56 +++++++-------------------------------------
>>> include/linux/mfd/tps65217.h | 6 -----
>>> 2 files changed, 9 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
>>> index f769c7d..9effdda 100644
>>> --- a/drivers/mfd/tps65217.c
>>> +++ b/drivers/mfd/tps65217.c
>>> @@ -33,14 +33,7 @@
>>> #include <linux/mfd/core.h>
>>> #include <linux/mfd/tps65217.h>
>>>
>>> -static struct resource charger_resources[] = {
>>> - DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
>>> - DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
>>> -};
>>> -
>>> -static struct resource pb_resources[] = {
>>> - DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
>>> -};
>>
>> May be I messed smth, but how about interrupts for charger and pwrbutton?
>>
>
> I might be wrong but as this driver is DT-only these resources came
> from the DT. The driver calls platform_get_irq_byname and then
> of_irq_get_byname.
>
> i.e arch/arm/boot/dts/am335x-bone-common.dtsi
>
> &tps {
> charger {
> interrupts = <0>, <1>;
> interrupt-names = "USB", "AC";
> status = "okay";
> };
>
> pwrbutton {
> interrupts = <2>;
> status = "okay";
> };
> };
Sry, but this make no sense - those IRQ configuration is static,
so it should be defined in arch/arm/boot/dts/tps65217.dtsi at least.
But, again, it also not required, because it is strictly static for
this particular device and so defined in code.
And platform_get_irq_byname() should perfectly work with mfd_add_devices().
Also not that tps65217.dtsi included in below files, but
./arch/arm/boot/dts/am335x-bone-common.dtsi:/include/ "tps65217.dtsi"
./arch/arm/boot/dts/am335x-chilisom.dtsi:/include/ "tps65217.dtsi"
./arch/arm/boot/dts/am335x-nano.dts:#include "tps65217.dtsi"
./arch/arm/boot/dts/am335x-pepper.dts:/include/ "tps65217.dtsi"
./arch/arm/boot/dts/am335x-sl50.dts:#include "tps65217.dtsi"
but IRQs defined only in
./arch/arm/boot/dts/am335x-bone-common.dtsi: interrupt-names = "USB", "AC";
./arch/arm/boot/dts/am335x-chiliboard.dts: interrupt-names = "USB", "AC";
So how are other boards working?
>
>>> +#define TPS65217_NUM_IRQ 3
>>>
>>> static void tps65217_irq_lock(struct irq_data *data)
>>> {
>>> @@ -86,29 +79,6 @@ static struct irq_chip tps65217_irq_chip = {
>>> .irq_disable = tps65217_irq_disable,
>>> };
>>>
>>> -static struct mfd_cell tps65217s[] = {
>>> - {
>>> - .name = "tps65217-pmic",
>>> - .of_compatible = "ti,tps65217-pmic",
>>> - },
>>> - {
>>> - .name = "tps65217-bl",
>>> - .of_compatible = "ti,tps65217-bl",
>>> - },
>>> - {
>>> - .name = "tps65217-charger",
>>> - .num_resources = ARRAY_SIZE(charger_resources),
>>> - .resources = charger_resources,
>>> - .of_compatible = "ti,tps65217-charger",
>>> - },
>>> - {
>>> - .name = "tps65217-pwrbutton",
>>> - .num_resources = ARRAY_SIZE(pb_resources),
>>> - .resources = pb_resources,
>>> - .of_compatible = "ti,tps65217-pwrbutton",
>>> - },
>>> -};
>>> -
>>> static irqreturn_t tps65217_irq_thread(int irq, void *data)
>>> {
>>> struct tps65217 *tps = data;
>>> @@ -359,23 +329,8 @@ static int tps65217_probe(struct i2c_client *client,
>>> return ret;
>>> }
>>>
>>> - if (client->irq) {
>>> + if (client->irq)
>>> tps65217_irq_init(tps, client->irq);
>>> - } else {
>>> - int i;
>>> -
>>> - /* Don't tell children about IRQ resources which won't fire */
>>> - for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
>>> - tps65217s[i].num_resources = 0;
>>> - }
>>> -
>>> - ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
>>> - ARRAY_SIZE(tps65217s), NULL, 0,
>>> - tps->irq_domain);
>>> - if (ret < 0) {
>>> - dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
>>> - return ret;
>>> - }
>>
>> And as I remember there was a request to use mfd_add_devices()
>> and not of_platform_populate() for mfd sub-devices instantiation.
>>
>
> From what I know or you add the sub-devices via mfd_add_devices or
> using the DT with of_platform_populate, and the first is probably the
> preferred, but in this specific case this driver is DT-only so IMHO
> makes more sense adding via the DT, there are already some bindings
> i.e:
> arch/arm/boot/dts/tps65217.dtsi
> arch/arm/boot/dts/am335x-bone-common.dtsi
>
> Let me explain a bit more, assume that you have another AM335x with
> the TPS65217 PMIC but you don't want the charger because is not wired
> in your board. Your board DT will include the tps65217.dtsi, in this
> file you can see:
>
> &tps {
> ...
> charger {
> compatible = "ti,tps65217-charger";
> status = "disabled";
> };
> ...
> };
>
> Seems that's fine but actually not works as expected, the TPS65217 MFD
> registers all the sub-devices so the charger is enabled and running
> even you have status = "disabled" in your DT. This looks incoherent to
> me, hence I replaced the devm_mfd_add_devices() for the
> of_platform_populate which takes care of the status propriety and only
> calls the probe of the sub-devices that match and are enabled (status
> = "okay").
I can't find links on corresponding discussions, but mfd_add_devices() is
preferred for MDF devices. Below is one commit i've found. Also you can compare number of
drivers using mfd_add_devices() and of_platform_populate().
regarding your problem - I think, It might be reasonable from your side to propose change to
mfd_add_device() which will take into account 'status = "disabled";' DT property.
---
commit 4531156db726d27e593d35800d43c74be4e393b9
Author: Keerthy <j-keerthy@...com>
Date: Mon Sep 19 13:09:05 2016 +0530
mfd: tps65218: Use mfd_add_devices instead of of_platform_populate
mfd_add_devices enables parsing device tree nodes without compatibles
for regulators and gpio modules. Replace of_platform_populate with
mfd_add_devices. mfd_cell currently is populated with regulators,
gpio and powerbutton.
Signed-off-by: Keerthy <j-keerthy@...com>
Signed-off-by: Lee Jones <lee.jones@...aro.org>
--
regards,
-grygorii
Powered by blists - more mailing lists