From: Sascha Hauer <s.hauer@pengutronix.de> To: Eduardo Valentin <edubezval@gmail.com> Cc: linux-pm@vger.kernel.org, Zhang Rui <rui.zhang@intel.com>, linux-kernel@vger.kernel.org, kernel@pengutronix.de, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, Matthias Brugger <matthias.bgg@gmail.com>, devicetree@vger.kernel.org, mark.rutland@arm.com, robh+dt@kernel.org Subject: Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support Date: Wed, 30 Sep 2015 08:13:49 +0200 [thread overview] Message-ID: <20150930061349.GV7858@pengutronix.de> (raw) In-Reply-To: <20150929230438.GB22673@localhost.localdomain> Hi Eduardo, On Tue, Sep 29, 2015 at 04:04:40PM -0700, Eduardo Valentin wrote: > On Wed, Sep 23, 2015 at 03:37:42PM +0200, Sascha Hauer wrote: > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > You dont seam to be using this header. Can you please clean up to have > only the headers you need? Ok, did that. > > + struct mtk_thermal *mt = bank->mt; > > + int temp, i, max; > > + u32 raw; > > + > > + temp = max = INT_MIN; > > + > > + for (i = 0; i < bank_data[bank->id].num_sensors; i++) { > > + raw = readl(mt->thermal_base + sensing_points[i].msr); > > + > > + temp = raw_to_mcelsius(mt, raw); > > + > > + /* > > + * The first read of a sensor often contains very high bogus > > + * temperature value. Filter these out so that the system does > > + * not immediately shut down. > > + */ > > + if (temp > 200000) > > Ok... How about after the first read? is > 200000 a valid (supported) value range? No, temperatures over 200°C are not supported for this SoC. > > + > > + mt->dev = &pdev->dev; > > + > > + auxadc = of_parse_phandle(np, "mediatek,auxadc", 0); > of_put? added > > + if (!auxadc) { > > + dev_err(&pdev->dev, "missing auxadc node\n"); > > + return -ENODEV; > > + } > > + > > + auxadc_phys_base = of_get_phys_base(auxadc); > > + if (auxadc_phys_base == OF_BAD_ADDR) { > > + dev_err(&pdev->dev, "Can't get auxadc phys address\n"); > > + return -EINVAL; > > + } > > + > > + apmixedsys = of_parse_phandle(np, "mediatek,apmixedsys", 0); > > + if (!apmixedsys) { > > + dev_err(&pdev->dev, "missing apmixedsys node\n"); > > + return -ENODEV; > > + } For this one aswell. > > + /* > > + * These calibration values should finally be provided by the > > + * firmware or fuses. For now use default values. > > + */ > > + mt->calib_slope = -123; > > + mt->calib_offset = 465124; > > I would still prefer that this driver would not have these hardcoded > values. Specially considering the fact that we could map it in DT for > instance. What is the impact of using this? Does it work across all chip > distribution? I think yes, but not that accurate. > > Should we wait until you have the code to read the fuses before merging > this? I'd say we should merge this. It makes the life easier for the guys writing the cpufreq driver. Adding a dependency on a not yet written driver IMO only adds unnecessary delays. > > > + > > + for (i = 0; i < MT8173_NUM_ZONES; i++) > > + mtk_thermal_init_bank(mt, i, apmixed_phys_base, auxadc_phys_base); > > + > > + platform_set_drvdata(pdev, mt); > > + > > + for (i = 0; i < MT8173_NUM_ZONES; i++) { > > + struct mtk_thermal_bank *bank = &mt->banks[i]; > > + > > + bank->tzd = thermal_zone_of_sensor_register(&pdev->dev, i, bank, > > + &mtk_thermal_ops); > > You need to error handle this. Errors here are pretty much expected. This is due to the behaviour of thermal_zone_of_sensor_register which errors out when no user for a sensor is found. Normally I would expect that I can register a sensor regardless if it is used or not, but that's not how thermal_zone_of_sensor_register is written. I could catch -EPROBE_DEFER here, but currently I see no case where this can actually be returned from thermal_zone_of_sensor_register. What I can and should do though is to call thermal_zone_of_sensor_unregister only for sensors that are successfully registered. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
WARNING: multiple messages have this Message-ID (diff)
From: s.hauer@pengutronix.de (Sascha Hauer) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] thermal: Add Mediatek thermal controller support Date: Wed, 30 Sep 2015 08:13:49 +0200 [thread overview] Message-ID: <20150930061349.GV7858@pengutronix.de> (raw) In-Reply-To: <20150929230438.GB22673@localhost.localdomain> Hi Eduardo, On Tue, Sep 29, 2015 at 04:04:40PM -0700, Eduardo Valentin wrote: > On Wed, Sep 23, 2015 at 03:37:42PM +0200, Sascha Hauer wrote: > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > You dont seam to be using this header. Can you please clean up to have > only the headers you need? Ok, did that. > > + struct mtk_thermal *mt = bank->mt; > > + int temp, i, max; > > + u32 raw; > > + > > + temp = max = INT_MIN; > > + > > + for (i = 0; i < bank_data[bank->id].num_sensors; i++) { > > + raw = readl(mt->thermal_base + sensing_points[i].msr); > > + > > + temp = raw_to_mcelsius(mt, raw); > > + > > + /* > > + * The first read of a sensor often contains very high bogus > > + * temperature value. Filter these out so that the system does > > + * not immediately shut down. > > + */ > > + if (temp > 200000) > > Ok... How about after the first read? is > 200000 a valid (supported) value range? No, temperatures over 200?C are not supported for this SoC. > > + > > + mt->dev = &pdev->dev; > > + > > + auxadc = of_parse_phandle(np, "mediatek,auxadc", 0); > of_put? added > > + if (!auxadc) { > > + dev_err(&pdev->dev, "missing auxadc node\n"); > > + return -ENODEV; > > + } > > + > > + auxadc_phys_base = of_get_phys_base(auxadc); > > + if (auxadc_phys_base == OF_BAD_ADDR) { > > + dev_err(&pdev->dev, "Can't get auxadc phys address\n"); > > + return -EINVAL; > > + } > > + > > + apmixedsys = of_parse_phandle(np, "mediatek,apmixedsys", 0); > > + if (!apmixedsys) { > > + dev_err(&pdev->dev, "missing apmixedsys node\n"); > > + return -ENODEV; > > + } For this one aswell. > > + /* > > + * These calibration values should finally be provided by the > > + * firmware or fuses. For now use default values. > > + */ > > + mt->calib_slope = -123; > > + mt->calib_offset = 465124; > > I would still prefer that this driver would not have these hardcoded > values. Specially considering the fact that we could map it in DT for > instance. What is the impact of using this? Does it work across all chip > distribution? I think yes, but not that accurate. > > Should we wait until you have the code to read the fuses before merging > this? I'd say we should merge this. It makes the life easier for the guys writing the cpufreq driver. Adding a dependency on a not yet written driver IMO only adds unnecessary delays. > > > + > > + for (i = 0; i < MT8173_NUM_ZONES; i++) > > + mtk_thermal_init_bank(mt, i, apmixed_phys_base, auxadc_phys_base); > > + > > + platform_set_drvdata(pdev, mt); > > + > > + for (i = 0; i < MT8173_NUM_ZONES; i++) { > > + struct mtk_thermal_bank *bank = &mt->banks[i]; > > + > > + bank->tzd = thermal_zone_of_sensor_register(&pdev->dev, i, bank, > > + &mtk_thermal_ops); > > You need to error handle this. Errors here are pretty much expected. This is due to the behaviour of thermal_zone_of_sensor_register which errors out when no user for a sensor is found. Normally I would expect that I can register a sensor regardless if it is used or not, but that's not how thermal_zone_of_sensor_register is written. I could catch -EPROBE_DEFER here, but currently I see no case where this can actually be returned from thermal_zone_of_sensor_register. What I can and should do though is to call thermal_zone_of_sensor_unregister only for sensors that are successfully registered. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
next prev parent reply other threads:[~2015-09-30 6:14 UTC|newest] Thread overview: 135+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-09-23 13:37 [PATCH v9] Add Mediatek thermal support Sascha Hauer 2015-09-23 13:37 ` Sascha Hauer 2015-09-23 13:37 ` [PATCH 1/3] dt-bindings: thermal: Add binding document for Mediatek thermal controller Sascha Hauer 2015-09-23 13:37 ` Sascha Hauer 2015-09-23 13:37 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-09-23 13:37 ` Sascha Hauer 2015-09-23 18:31 ` Vladimir Zapolskiy 2015-09-23 18:31 ` Vladimir Zapolskiy 2015-09-23 18:31 ` Vladimir Zapolskiy 2015-09-30 6:14 ` Sascha Hauer 2015-09-30 6:14 ` Sascha Hauer 2015-09-29 23:04 ` Eduardo Valentin 2015-09-29 23:04 ` Eduardo Valentin 2015-09-29 23:04 ` Eduardo Valentin 2015-09-30 6:13 ` Sascha Hauer [this message] 2015-09-30 6:13 ` Sascha Hauer 2015-09-30 9:36 ` Punit Agrawal 2015-09-30 9:36 ` Punit Agrawal 2015-09-30 9:36 ` Punit Agrawal 2015-09-30 10:37 ` Sascha Hauer 2015-09-30 10:37 ` Sascha Hauer 2015-09-30 11:07 ` Punit Agrawal 2015-09-30 11:07 ` Punit Agrawal 2015-09-23 13:37 ` [PATCH 3/3] ARM64: dts: mt8173: Add thermal/auxadc device nodes Sascha Hauer 2015-09-23 13:37 ` Sascha Hauer -- strict thread matches above, loose matches on Subject: below -- 2015-11-30 11:42 [PATCH v12] Add Mediatek thermal support Sascha Hauer 2015-11-30 11:42 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-11-30 11:42 ` Sascha Hauer 2015-12-17 19:33 ` Eduardo Valentin 2015-12-17 19:33 ` Eduardo Valentin 2016-01-04 14:19 ` Sascha Hauer 2016-01-04 14:19 ` Sascha Hauer 2016-01-19 7:29 ` Sascha Hauer 2016-01-19 7:29 ` Sascha Hauer 2016-02-01 2:54 ` Eddie Huang 2016-02-01 2:54 ` Eddie Huang 2016-02-01 2:54 ` Eddie Huang 2016-02-15 2:11 ` Daniel Kurtz 2016-02-15 2:11 ` Daniel Kurtz 2016-02-15 2:11 ` Daniel Kurtz 2016-02-15 2:14 ` Daniel Kurtz 2016-02-15 2:14 ` Daniel Kurtz 2016-02-15 2:14 ` Daniel Kurtz 2016-02-17 17:05 ` Matthias Brugger 2016-02-17 17:05 ` Matthias Brugger 2016-02-17 17:05 ` Matthias Brugger 2016-02-18 10:56 ` Sascha Hauer 2016-02-18 10:56 ` Sascha Hauer 2016-02-18 10:56 ` Sascha Hauer 2016-02-18 14:28 ` Javi Merino 2016-02-18 14:28 ` Javi Merino 2016-02-18 14:28 ` Javi Merino 2016-02-18 15:15 ` Eduardo Valentin 2016-02-18 15:15 ` Eduardo Valentin 2016-02-18 15:15 ` Eduardo Valentin 2016-02-19 7:21 ` Sascha Hauer 2016-02-19 7:21 ` Sascha Hauer 2016-02-19 7:21 ` Sascha Hauer 2015-12-21 4:07 ` Daniel Kurtz 2015-12-21 4:07 ` Daniel Kurtz 2015-12-21 4:07 ` Daniel Kurtz 2016-01-04 14:31 ` Sascha Hauer 2016-01-04 14:31 ` Sascha Hauer 2016-01-04 14:31 ` Sascha Hauer 2016-01-04 15:43 ` Daniel Kurtz 2016-01-04 15:43 ` Daniel Kurtz 2016-01-04 15:43 ` Daniel Kurtz 2015-11-18 8:24 [PATCH v11] Add Mediatek thermal support Sascha Hauer 2015-11-18 8:24 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-11-18 8:24 ` Sascha Hauer 2015-11-24 6:06 ` dawei chien 2015-11-24 6:06 ` dawei chien 2015-11-24 6:06 ` dawei chien 2015-11-24 7:53 ` Sascha Hauer 2015-11-24 7:53 ` Sascha Hauer 2015-11-09 10:13 [PATCH v10] Add Mediatek thermal support Sascha Hauer 2015-11-09 10:13 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 14:39 ` Andy Shevchenko 2015-11-09 14:39 ` Andy Shevchenko 2015-11-18 8:11 ` Sascha Hauer 2015-11-18 8:11 ` Sascha Hauer 2015-11-18 8:11 ` Sascha Hauer 2015-11-10 12:05 ` Javi Merino 2015-11-10 12:05 ` Javi Merino 2015-11-10 18:26 ` Eduardo Valentin 2015-11-10 18:26 ` Eduardo Valentin 2015-11-11 7:27 ` Sascha Hauer 2015-11-11 7:27 ` Sascha Hauer 2015-11-11 9:40 ` Javi Merino 2015-11-11 9:40 ` Javi Merino 2015-11-11 9:40 ` Javi Merino 2015-11-13 10:09 ` Sascha Hauer 2015-11-13 10:09 ` Sascha Hauer 2015-11-13 11:26 ` Javi Merino 2015-11-13 11:26 ` Javi Merino 2015-11-18 8:18 ` Sascha Hauer 2015-11-18 8:18 ` Sascha Hauer 2015-08-31 7:34 [PATCH v8] Add Mediatek thermal support Sascha Hauer 2015-08-31 7:34 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-31 7:34 ` Sascha Hauer 2015-08-31 7:34 ` Sascha Hauer 2015-09-14 7:32 ` Daniel Kurtz 2015-09-14 7:32 ` Daniel Kurtz 2015-09-14 7:32 ` Daniel Kurtz 2015-09-22 7:30 ` Daniel Kurtz 2015-09-22 7:30 ` Daniel Kurtz 2015-09-22 7:30 ` Daniel Kurtz 2015-09-22 8:30 ` Sascha Hauer 2015-09-22 8:30 ` Sascha Hauer 2015-09-22 8:30 ` Sascha Hauer 2015-08-27 6:41 [PATCH v7] Add Mediatek thermal support Sascha Hauer 2015-08-27 6:41 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-27 11:50 ` Punit Agrawal 2015-08-26 13:58 [PATCH v6] Add Mediatek thermal support Sascha Hauer 2015-08-26 13:58 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-20 8:05 [PATCH v5] Add Mediatek thermal support Sascha Hauer 2015-08-20 8:06 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-20 22:20 ` Eduardo Valentin 2015-08-20 22:29 ` Daniel Lezcano 2015-08-21 5:06 ` Sascha Hauer 2015-08-20 23:12 ` Daniel Lezcano 2015-08-26 13:54 ` Sascha Hauer 2015-08-26 14:02 ` Daniel Lezcano 2015-08-25 17:41 ` Daniel Kurtz 2015-08-07 13:55 [PATCH v4] Add Mediatek thermal support Sascha Hauer 2015-08-07 13:55 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-11 7:03 ` Daniel Kurtz 2015-08-20 7:57 ` Sascha Hauer 2015-08-05 12:25 [PATCH v3] Add Mediatek thermal support Sascha Hauer 2015-08-05 12:25 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-05 18:02 ` Daniel Kurtz 2015-08-06 8:10 ` Sascha Hauer 2015-07-21 7:59 [PATCH v2] Add Mediatek thermal support Sascha Hauer 2015-07-21 7:59 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-07-21 7:59 ` Sascha Hauer 2015-07-21 15:13 ` Daniel Kurtz 2015-07-21 15:13 ` Daniel Kurtz 2015-07-21 15:13 ` Daniel Kurtz 2015-08-05 10:20 ` Sascha Hauer 2015-08-05 10:20 ` Sascha Hauer 2015-08-05 10:20 ` Sascha Hauer 2015-07-13 10:34 [PATCH] thermal: Add Mediatek thermal support Sascha Hauer 2015-07-13 10:34 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-07-13 10:34 ` Sascha Hauer
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=20150930061349.GV7858@pengutronix.de \ --to=s.hauer@pengutronix.de \ --cc=devicetree@vger.kernel.org \ --cc=edubezval@gmail.com \ --cc=kernel@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-pm@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=matthias.bgg@gmail.com \ --cc=robh+dt@kernel.org \ --cc=rui.zhang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.