| 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
| ||
|
Message-ID: <20150203192656.GB3486@developer.amazonguestwifi.org>
Date: Tue, 3 Feb 2015 15:26:57 -0400
From: Eduardo Valentin <edubezval@...il.com>
To: "Ivan T. Ivanov" <iivanov@...sol.com>
Cc: Zhang Rui <rui.zhang@...el.com>, Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org,
David Collins <collinsd@...eaurora.org>
Subject: Re: [PATCH v4] thermal: Add QPNP PMIC temperature alarm driver
On Tue, Feb 03, 2015 at 11:24:45AM +0200, Ivan T. Ivanov wrote:
>
> Hi Eduardo,
>
> On Mon, 2015-02-02 at 14:14 -0400, Eduardo Valentin wrote:
> > Ivan,
> >
> > On Mon, Feb 02, 2015 at 05:19:30PM +0200, Ivan T. Ivanov wrote:
> > > Add support for the temperature alarm peripheral found inside
> > > Qualcomm plug-and-play (QPNP) PMIC chips. The temperature alarm
> > > peripheral outputs a pulse on an interrupt line whenever the
> > > thermal over temperature stage value changes.
> > >
> > > Register a thermal sensor. The temperature reported by this thermal
> > > sensor device should reflect the actual PMIC die temperature if an
> > > ADC is present on the given PMIC. If no ADC is present, then the
> > > reported temperature should be estimated from the over temperature
> > > stage value.
> > >
> > > Cc: David Collins <collinsd@...eaurora.org>
> > > Signed-off-by: Ivan T. Ivanov <iivanov@...sol.com>
> > > ---
> > >
> > > Changes since v3:
> > >
> > > - Driver register thermal sensor instead thermal zone. Device thermal zone
> > > should be properly described in DT files according thermal.txt document.
> >
> > Thanks a lot for keeping this up. I believe the driver looks smaller and
> > cleaner now, don't you agree?
>
> Yep.
>
> >
> > > - Dropped support for software override PMIC shutdown sequence and related
> > > bit definitions. If software did not take action for clean device shutdown,
> > > until critical temperature is reached, PMIC chip will execute internal
> > > pre-programed shutdown sequence.
> > >
> >
> > OK. Let me know if this functionality is crucial and needs further
> > discussion.
>
> Unless I miss something, I think that this functionality is
> used only for the purpose of debugging.
OK Great!
>
> >
> >
> > I have two very minor comments as follows.
> >
> >
> > > v3: http://comments.gmane.org/gmane.linux.ports.arm.msm/10071
> > >
> > > .../bindings/thermal/qcom-spmi-temp-alarm.txt | 57 ++++
> > > drivers/thermal/Kconfig | 11 +
> > > drivers/thermal/Makefile | 1 +
> > > drivers/thermal/qcom-spmi-temp-alarm.c | 311 +++++++++++++++++++++
> > > 4 files changed, 380 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > > create mode 100644 drivers/thermal/qcom-spmi-temp-alarm.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > > b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > > new file mode 100644
> > > index 0000000..290ec06
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > > @@ -0,0 +1,57 @@
> > > +Qualcomm QPNP PMIC Temperature Alarm
> > > +
> > > +QPNP temperature alarm peripherals are found inside of Qualcomm PMIC chips
> > > +that utilize the Qualcomm SPMI implementation. These peripherals provide an
> > > +interrupt signal and status register to identify high PMIC die temperature.
> > > +
> > > +Required properties:
> > > +- compatible: Should contain "qcom,spmi-temp-alarm".
> > > +- reg: Specifies the SPMI address and length of the controller's
> > > + registers.
> > > +- interrupts: PMIC temperature alarm interrupt.
> > > +- #thermal-sensor-cells: Should be 0. See thermal.txt for a description.
> > > +
> > > +Optional properties:
> > > +- io-channels: Should contain IIO channel specifier for the ADC channel,
> > > + which report chip die temperature.
> > > +- io-channel-names: Should contain "thermal".
> > > +
> > > +Example:
> > > +
> > > + pm8941_temp: thermal-alarm@...0 {
> > > + compatible = "qcom,spmi-temp-alarm";
> > > + reg = <0x2400 0x100>;
> > > + interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
> > > + #thermal-sensor-cells = <0>;
> > > +
> > > + io-channels = <&pm8941_vadc VADC_DIE_TEMP>;
> > > + io-channel-names = "thermal";
> > > + };
> > > +
> > > + thermal-zones {
> > > + pm8941 {
> > > + polling-delay-passive = <250>;
> > > + polling-delay = <1000>;
> > > +
> > > + thermal-sensors = <&pm8941_temp>;
> > > +
> > > + trips {
> > > + passive {
> > > + temperature = <1050000>;
> > > + hysteresis = <2000>;
> > > + type = "passive";
> > > + };
> > > + alert {
> > > + temperature = <125000>;
> > > + hysteresis = <2000>;
> > > + type = "hot";
> > > + };
> > > + crit {
> > > + temperature = <145000>;
> > > + hysteresis = <2000>;
> > > + type = "critical";
> > > + };
> > > + };
> > > + };
> > > + };
> >
> > Do you have the appropriate DT changes under architecture code too?
> >
> > I mean, I am fine picking these changes, but should this series include
> > also a minor inclusion under arch/arm/boot/dts too, given that you
> > already did the hard part?
>
> We don't have any DT files for these PMIC's upstream. Probably is time
> to start adding them. I have waiting to have some drivers accepted
> before pushing such changes. But not we have 2 types of ADC, RTC, GPIO's,
> MPP's and hopefully Thermal :-). I would like this to be a separate effort.
I am fine if you take the approach of doing it in two steps. It is just
that your series can be better reviewed if you include the DT changes.
>
>
> > > + * This function updates the internal temp value based on the
> > > + * current thermal stage and threshold as well as the previous stage
> > > + */
> > > +static int qpnp_tm_update_temp_no_adc(struct qpnp_tm_chip *chip)
> > > +{
> > > + unsigned int stage;
> > > + int ret;
> > > + u8 reg = 0;
> > > +
> > > + ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, ®);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + stage = reg & STATUS_STAGE_MASK;
> > > +
> > > + if (stage > chip->stage) {
> > > + /* increasing stage, use lower bound */
> > > + chip->temp = (stage - 1) * TEMP_STAGE_STEP +
> > > + chip->thresh * TEMP_THRESH_STEP
> > > +
> > > + TEMP_STAGE_HYSTERESIS +
> > > TEMP_THRESH_MIN;
> > > + } else if (stage < chip->stage) {
> > > + /* decreasing stage, use upper bound */
> > > + chip->temp = stage * TEMP_STAGE_STEP +
> > > + chip->thresh * TEMP_THRESH_STEP
> > > -
> > > + TEMP_STAGE_HYSTERESIS +
> > > TEMP_THRESH_MIN;
> > > + }
> >
> > For my own edification, no change in state means no change in
> > temperature too, right?
>
> More or less, just no way to estimate exact temperature
> between stages.
OK.
>
> >
> > > +
> > > + chip->stage = stage;
> > > +
> > > + return 0;
> > > +}
> > > +
>
> <snip>
>
> > > +static int qpnp_tm_probe(struct platform_device *pdev)
> > > +{
> > > + struct qpnp_tm_chip *chip;
> > > + struct device_node *node;
> > > + u8 type, subtype;
> > > + u32 res[2];
> > > + int ret, irq;
> > > +
> > > + node = pdev->dev.of_node;
> > > +
> > > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > + if (!chip)
> > > + return -ENOMEM;
> > > +
> > > + dev_set_drvdata(&pdev->dev, chip);
> > > +
> > > + chip->map = dev_get_regmap(pdev->dev.parent, NULL);
> > > + if (!chip->map)
> > > + return -ENXIO;
> > > +
> > > + ret = of_property_read_u32_array(node, "reg", res, 2);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* ADC based measurements are optional */
> > > + chip->adc = iio_channel_get(&pdev->dev, "thermal");
> > > + if (PTR_ERR(chip->adc) == -EPROBE_DEFER)
> > > + return PTR_ERR(chip->adc);
> > > +
> > > + irq = platform_get_irq(pdev, 0);
> > > + if (irq < 0)
> > > + return irq;
> > > +
> > > + chip->base = res[0];
> > > +
> > > + ret = qpnp_tm_read(chip, QPNP_TM_REG_TYPE, &type);
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "could not read type\n");
> > > + goto fail;
> > > + }
> > > +
> > > + ret = qpnp_tm_read(chip, QPNP_TM_REG_SUBTYPE, &subtype);
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "could not read subtype\n");
> > > + goto fail;
> > > + }
> > > +
> > > + if (type != QPNP_TM_TYPE || subtype != QPNP_TM_SUBTYPE) {
> > > + dev_err(&pdev->dev, "invalid type 0x%02x or subtype 0x%02x\n",
> > > + type, subtype);
> > > + ret = -ENODEV;
> > > + goto fail;
> > > + }
> > > +
> > > + ret = qpnp_tm_init(chip);
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "init failed\n");
> > > + goto fail;
> > > + }
> > > +
> > > + chip->tz_dev = thermal_zone_of_sensor_register(&pdev->dev, 0, chip,
> > > + &qpnp_tm_sensor_ops);
> > > + if (IS_ERR(chip->tz_dev)) {
> > > + dev_err(&pdev->dev, "failed to register sensor\n");
> > > + ret = PTR_ERR(chip->tz_dev);
> > > + goto fail;
> > > + }
> > > +
> > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, qpnp_tm_isr,
> > > + IRQF_ONESHOT, node->name, chip);
> >
> > What if we request this IRQ before registering the of thermal zone
> > sensor? I believe it makes more sense conceptually, as you mean, you
> > register into upper layers once your driver is fully ready to do so.
> >
> > Any objections changing the order?
>
> Sure.
Good!
>
> >
> > > + if (ret < 0)
> > > + goto unreg;
> > > +
> > > + return 0;
> > > +
> > > +unreg:
> > > + thermal_zone_of_sensor_unregister(&pdev->dev, chip->tz_dev);
> >
> > As mentioned by Stanimir Varban, you may also remove the above, since
> > the irq is devm.
>
> Sure. s/Varban/Varbanov/ ;-)
Ohh.. my bad. Sorry Varbanov. Fixing next time! :-)
>
> Thank you.
> Ivan
BR,
Eduardo Valentin
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)
Powered by blists - more mailing lists