Messages in this thread | | | Date | Wed, 8 Jul 2020 15:33:05 +0200 | From | Ondřej Jirman <> | Subject | Re: [PATCH] thermal: sun8i: Be loud when probe fails |
| |
On Wed, Jul 08, 2020 at 07:55:40PM +0800, Frank Lee wrote: > HI Ondrej, > On Wed, Jul 8, 2020 at 6:55 PM Ondrej Jirman <megous@megous.com> wrote: > > > > I noticed several mobile Linux distributions failing to enable the > > thermal regulation correctly, because the kernel is silent > > when thermal driver fails to probe. Add enough error reporting > > to debug issues and warn users in case thermal sensor is failing > > to probe. > > > > Failing to notify users means, that SoC can easily overheat under > > load. > > > > Signed-off-by: Ondrej Jirman <megous@megous.com> > > --- > > drivers/thermal/sun8i_thermal.c | 55 ++++++++++++++++++++++++++------- > > 1 file changed, 43 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c > > index 74d73be16496..9065e79ae743 100644 > > --- a/drivers/thermal/sun8i_thermal.c > > +++ b/drivers/thermal/sun8i_thermal.c > > @@ -287,8 +287,12 @@ static int sun8i_ths_calibrate(struct ths_device *tmdev) > > > > calcell = devm_nvmem_cell_get(dev, "calibration"); > > if (IS_ERR(calcell)) { > > + dev_err(dev, "Failed to get calibration nvmem cell (%ld)\n", > > + PTR_ERR(calcell)); > > + > > if (PTR_ERR(calcell) == -EPROBE_DEFER) > > return -EPROBE_DEFER; > > + > > /* > > * Even if the external calibration data stored in sid is > > * not accessible, the THS hardware can still work, although > > @@ -308,6 +312,8 @@ static int sun8i_ths_calibrate(struct ths_device *tmdev) > > caldata = nvmem_cell_read(calcell, &callen); > > if (IS_ERR(caldata)) { > > ret = PTR_ERR(caldata); > > + dev_err(dev, "Failed to read calibration data (%d)\n", > > + ret); > > goto out; > > } > > > > @@ -330,23 +336,35 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev) > > return PTR_ERR(base); > > > > tmdev->regmap = devm_regmap_init_mmio(dev, base, &config); > > - if (IS_ERR(tmdev->regmap)) > > + if (IS_ERR(tmdev->regmap)) { > > + dev_err(dev, "Failed to init regmap (%ld)\n", > > + PTR_ERR(tmdev->regmap)); > > return PTR_ERR(tmdev->regmap); > > + } > > > > if (tmdev->chip->has_bus_clk_reset) { > > tmdev->reset = devm_reset_control_get(dev, NULL); > > - if (IS_ERR(tmdev->reset)) > > + if (IS_ERR(tmdev->reset)) { > > + dev_err(dev, "Failed to get reset (%ld)\n", > > + PTR_ERR(tmdev->reset)); > > return PTR_ERR(tmdev->reset); > > + } > > > > tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus"); > > - if (IS_ERR(tmdev->bus_clk)) > > + if (IS_ERR(tmdev->bus_clk)) { > > + dev_err(dev, "Failed to get bus clock (%ld)\n", > > + PTR_ERR(tmdev->bus_clk)); > > return PTR_ERR(tmdev->bus_clk); > > + } > > } > > > > if (tmdev->chip->has_mod_clk) { > > tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod"); > > - if (IS_ERR(tmdev->mod_clk)) > > + if (IS_ERR(tmdev->mod_clk)) { > > + dev_err(dev, "Failed to get mod clock (%ld)\n", > > + PTR_ERR(tmdev->mod_clk)); > > return PTR_ERR(tmdev->mod_clk); > > + } > > } > > > > ret = reset_control_deassert(tmdev->reset); > > @@ -471,8 +489,12 @@ static int sun8i_ths_register(struct ths_device *tmdev) > > i, > > &tmdev->sensor[i], > > &ths_ops); > > - if (IS_ERR(tmdev->sensor[i].tzd)) > > + if (IS_ERR(tmdev->sensor[i].tzd)) { > > + dev_err(tmdev->dev, > > + "Failed to register sensor %d (%ld)\n", > > + i, PTR_ERR(tmdev->sensor[i].tzd)); > > return PTR_ERR(tmdev->sensor[i].tzd); > > + } > > > > if (devm_thermal_add_hwmon_sysfs(tmdev->sensor[i].tzd)) > > dev_warn(tmdev->dev, > > @@ -501,19 +523,21 @@ static int sun8i_ths_probe(struct platform_device *pdev) > > > > ret = sun8i_ths_resource_init(tmdev); > > if (ret) > > - return ret; > > + goto err_out; > > > > irq = platform_get_irq(pdev, 0); > > - if (irq < 0) > > - return irq; > > + if (irq < 0) { > > + ret = irq; > > + goto err_out; > > + } > > > > ret = tmdev->chip->init(tmdev); > > if (ret) > > - return ret; > > + goto err_out; > > > > ret = sun8i_ths_register(tmdev); > > if (ret) > > - return ret; > > + goto err_out; > > > > /* > > * Avoid entering the interrupt handler, the thermal device is not > > @@ -523,10 +547,17 @@ static int sun8i_ths_probe(struct platform_device *pdev) > > ret = devm_request_threaded_irq(dev, irq, NULL, > > sun8i_irq_thread, > > IRQF_ONESHOT, "ths", tmdev); > > - if (ret) > > - return ret; > > + if (ret) { > > + dev_err(dev, "Failed to request irq (%d)\n", ret); > > + goto err_out; > > + } > > > > + dev_info(dev, "Thermal sensor ready!\n"); > > return 0; > > + > > +err_out: > > + dev_err(dev, "Failed to probe thermal sensor (%d)\n", ret); > > When the driver fails, there will be this print. Isn't it superfluous > for you to add these? > > sun8i-thermal: probe of 5070400.thermal-sensor failed with error
Thinking more about it. You're right. This is probably only shown for non-EPROBE_DEFER errors. So this printk is superfluous, since EPROBE_DEFER from nvmem/sid is already shown elsewhere in this patch.
thanks, o.
> > Yangtao
| |