linux-kernel - Re: [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160628113931.GD5550@lukather>
Date:	Tue, 28 Jun 2016 13:39:31 +0200
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Ondřej Jirman <megous@...ous.com>
Cc:	dev@...ux-sunxi.org, linux-arm-kernel@...ts.infradead.org,
	Zhang Rui <rui.zhang@...el.com>,
	Eduardo Valentin <edubezval@...il.com>,
	Chen-Yu Tsai <wens@...e.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:THERMAL" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal
 sensor on Allwinner H3

On Sat, Jun 25, 2016 at 05:12:41PM +0200, Ondřej Jirman wrote:
> >> +	data->calreg = devm_ioremap_resource(&pdev->dev, res);
> >> +	if (IS_ERR(data->calreg)) {
> >> +		ret = PTR_ERR(data->calreg);
> >> +		dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret);
> >> +		return ret;
> >> +	}
> > 
> > Why did you remove the SID use through the nvmem framework ?!
> 
> Because it's overkill for reading a single word from memeory, the sunxi
> nvmem driver doesn't support H3, the sid is not documented in the
> datasheet, aside from some registger name/offset dump on the mailing
> list some time ago.
> 
> Aside from that, I've yet to see H3 soc that has anything else than 0 in
> the calibration data memory location. So this is basically nop.
> 
> Proposed solution seems simpler with no drawbacks that I can see,
> without resorting to dropping the thing entirely from this driver. Which
> I would be fine with too. Calibration is optional feature in the BSP
> kernel, so I assume dropping it may not do too much harm either.
> 
> If anyone wants to implement sid in the future, it will be easy enough
> to do with backwards compatibility. The second reg will become optional,
> and the driver can check for nvmem.

A lot of things in drivers boil down to "reading a single word from
memory". However, abstractions are here for a reason, and there's none
to not use it.

If that's not something we can use today, remove it entirely. And if
that becomes necessary, we will add an optional nvmem property.

> >> +
> >> +	irq = platform_get_irq(pdev, 0);
> >> +	if (irq < 0) {
> >> +		dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq);
> >> +		return irq;
> >> +	}
> >> +
> >> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> >> +					sun8i_ths_irq_thread, IRQF_ONESHOT,
> >> +					dev_name(&pdev->dev), data);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	data->busclk = devm_clk_get(&pdev->dev, "ahb");
> >> +	if (IS_ERR(data->busclk)) {
> >> +		ret = PTR_ERR(data->busclk);
> >> +		dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	data->clk = devm_clk_get(&pdev->dev, "ths");
> >> +	if (IS_ERR(data->clk)) {
> >> +		ret = PTR_ERR(data->clk);
> >> +		dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	data->reset = devm_reset_control_get(&pdev->dev, "ahb");
> >> +	if (IS_ERR(data->reset)) {
> >> +		ret = PTR_ERR(data->reset);
> >> +		dev_err(&pdev->dev, "failed to get reset: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = clk_prepare_enable(data->busclk);
> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = clk_prepare_enable(data->clk);
> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret);
> >> +		goto err_disable_bus;
> >> +	}
> >> +
> >> +	ret = clk_set_rate(data->clk, THS_H3_CLK_IN);
> >> +	if (ret)
> >> +		goto err_disable_ths;
> >> +
> >> +	ret = reset_control_deassert(data->reset);
> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "reset deassert failed: %d\n", ret);
> >> +		goto err_disable_ths;
> >> +	}
> > 
> > Having runtime_pm support would be great.
> 
> Suspend/resume handling? I would have no way of testing it, other than
> blindly impelementing what BSP kernel does. Other than that, I can add
> it quite easily. It should be rather simple.

No, I mean runtime_pm, with runtime_suspend and runtime_resume, to
allow only powering up the device when it's used, and shut it down
when not used.

> >> +MODULE_AUTHOR("Ondřej Jirman <megous@...ous.com>");
> >> +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner H3 SoC");
> >> +MODULE_LICENSE("GPL v2");
> > 
> > Looks quite good otherwise. It looks very similar to the older
> > touchscreen driver (without the touchscreen part).
> > 
> > Have you tried to merge the two?
> 
> What driver?

drivers/input/touchscreen/sun4i-ts.c

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)