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 <meg...@megous.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
signature.asc
Description: PGP signature
