Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300784 > unrolled thread

Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer <s.hauer@pengutronix.de> - 2016-01-04 15:40 +0100
    Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support Daniel Kurtz <djkurtz@chromium.org> - 2016-01-04 16:50 +0100

#1300784 — Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support

On Mon, Dec 21, 2015 at 12:07:58PM +0800, Daniel Kurtz wrote:
> Hi Sascha,
> 
> One nit below that can be fixed up later, or now if you don't plan to
> spin this driver to
> address Eduardo's feedback...
> 
> On Mon, Nov 30, 2015 at 7:42 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > This adds support for the Mediatek thermal controller found on MT8173
> > and likely other SoCs.
> > The controller is a bit special. It does not have its own ADC, instead
> > it controls the on-SoC AUXADC via AHB bus accesses. For this reason
> > we need the physical address of the AUXADC. Also it controls a mux
> > using AHB bus accesses, so we need the APMIXEDSYS physical address aswell.
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> [snip]
> 
> > +static int mtk_thermal_get_calibration_data(struct device *dev, struct mtk_thermal *mt)
> > +{
> > +       struct nvmem_cell *cell;
> > +       u32 *buf;
> > +       size_t len;
> > +       int i, ret = 0;
> > +
> > +       /* Start with default values */
> > +       mt->adc_ge = 512;
> > +       for (i = 0; i < MT8173_NUM_SENSORS; i++)
> > +               mt->vts[i] = 260;
> > +       mt->degc_cali = 40;
> > +       mt->o_slope = 0;
> > +
> > +       cell = nvmem_cell_get(dev, "calibration-data");
> > +       if (IS_ERR(cell)) {
> > +               if (PTR_ERR(cell) == -EPROBE_DEFER)
> 
> It is useful to know why the thermal driver is being probe defered, so
> I suggest here:
> dev_warn(dev, "Waiting for calibration data.\n");

The problem with that is that this message is not shown once but
possibly many times and may not even show a problem because in the end
the device may be probed successfully. In this case the last thing you
see from the device is "Waiting for calibration data." and get annoyed
by all this useless noise from the driver.

Of course I agree that this information may be useful in the case you
wonder why your device doesn't show up...

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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300834

On Mon, Jan 4, 2016 at 10:31 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Dec 21, 2015 at 12:07:58PM +0800, Daniel Kurtz wrote:
>> Hi Sascha,
>>
>> One nit below that can be fixed up later, or now if you don't plan to
>> spin this driver to
>> address Eduardo's feedback...
>>
>> On Mon, Nov 30, 2015 at 7:42 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > This adds support for the Mediatek thermal controller found on MT8173
>> > and likely other SoCs.
>> > The controller is a bit special. It does not have its own ADC, instead
>> > it controls the on-SoC AUXADC via AHB bus accesses. For this reason
>> > we need the physical address of the AUXADC. Also it controls a mux
>> > using AHB bus accesses, so we need the APMIXEDSYS physical address aswell.
>> >
>> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>
>> [snip]
>>
>> > +static int mtk_thermal_get_calibration_data(struct device *dev, struct mtk_thermal *mt)
>> > +{
>> > +       struct nvmem_cell *cell;
>> > +       u32 *buf;
>> > +       size_t len;
>> > +       int i, ret = 0;
>> > +
>> > +       /* Start with default values */
>> > +       mt->adc_ge = 512;
>> > +       for (i = 0; i < MT8173_NUM_SENSORS; i++)
>> > +               mt->vts[i] = 260;
>> > +       mt->degc_cali = 40;
>> > +       mt->o_slope = 0;
>> > +
>> > +       cell = nvmem_cell_get(dev, "calibration-data");
>> > +       if (IS_ERR(cell)) {
>> > +               if (PTR_ERR(cell) == -EPROBE_DEFER)
>>
>> It is useful to know why the thermal driver is being probe defered, so
>> I suggest here:
>> dev_warn(dev, "Waiting for calibration data.\n");
>
> The problem with that is that this message is not shown once but
> possibly many times and may not even show a problem because in the end
> the device may be probed successfully. In this case the last thing you
> see from the device is "Waiting for calibration data." and get annoyed
> by all this useless noise from the driver.
>
> Of course I agree that this information may be useful in the case you
> wonder why your device doesn't show up...

The solution for this, then, is:
 dev_dbg()
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web