Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1163236AbbKTQ7I (ORCPT ); Fri, 20 Nov 2015 11:59:08 -0500 Received: from down.free-electrons.com ([37.187.137.238]:46253 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1759507AbbKTQ7E (ORCPT ); Fri, 20 Nov 2015 11:59:04 -0500 Date: Fri, 20 Nov 2015 17:59:00 +0100 From: Maxime Ripard To: Josef Gajdusek Cc: linux-sunxi@googlegroups.com, linux-clk@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, gpatchesrdh@mveas.com, mturquette@linaro.org, hdegoede@redhat.com, sboyd@codeaurora.org, mturquette@baylibre.com, emilio@elopez.com.ar, linux@arm.linux.org.uk, edubezval@gmail.com, rui.zhang@intel.com, wens@csie.org, galak@codeaurora.org, ijc+devicetree@hellion.org.uk, mark.rutland@arm.com, pawel.moll@arm.com, robh+dt@kernel.org Subject: Re: [PATCH] thermal: Add support for Sunxi THS on the Allwinner H3 Message-ID: <20151120165900.GK32142@lukather> References: <20151118205147.GA19779@maud.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="T5F8Mpu1Sg8jtYtl" Content-Disposition: inline In-Reply-To: <20151118205147.GA19779@maud.lan> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13396 Lines: 440 --T5F8Mpu1Sg8jtYtl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi! Thanks for your patch. On Wed, Nov 18, 2015 at 09:51:48PM +0100, Josef Gajdusek wrote: > This patch adds support for the Sunxi thermal sensor on the Allwinner H3. > Also adds declaration of the H3 THS clock to clk-sunxi.c ignoring the > dividers as they are not continuous (clk-divider.c cannot be used as it > does not support setting an enable bit). > Should be easily extendable for the A33/A83T/... as they have similar but > not completely identical sensors. >=20 > Signed-off-by: Josef Gajdusek > --- > Documentation/devicetree/bindings/clock/sunxi.txt | 1 + > .../devicetree/bindings/thermal/sunxi-ths.txt | 24 ++ > arch/arm/boot/dts/sun8i-h3.dtsi | 27 +++ > drivers/clk/sunxi/clk-sunxi.c | 16 ++ > drivers/thermal/Kconfig | 7 + > drivers/thermal/Makefile | 1 + > drivers/thermal/sunxi_ths.c | 263 +++++++++++++++= ++++++ > 7 files changed, 339 insertions(+) > create mode 100644 Documentation/devicetree/bindings/thermal/sunxi-ths.t= xt > create mode 100644 drivers/thermal/sunxi_ths.c Like other have pointed out, this should be split in several patches. > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > index 6293c65..637401a 100644 > --- a/drivers/clk/sunxi/clk-sunxi.c > +++ b/drivers/clk/sunxi/clk-sunxi.c > @@ -604,6 +604,13 @@ static void sun7i_a20_get_out_factors(u32 *freq, u32= parent_rate, > *p =3D calcp; > } > =20 > +static void sun8i_h3_get_ths_factors(u32 *freq, u32 parent_rate, > + u8 *n, u8 *k, u8 *m, u8 *p) > +{ > + /* Ignore the dividers as they are not continuous */ You'd rather use a divider table. > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index c463c89..0111d4d 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -365,6 +365,13 @@ config INTEL_PCH_THERMAL > Thermal reporting device will provide temperature reading, > programmable trip points and other information. > =20 > +config SUNXI_THS > + tristate "Sunxi THS driver" Allwinner H3 Thermal Sensor > + depends on ARCH_SUNXI ARCH_SUN8I maybe ? > + depends on OF > + help > + Enable this to support thermal reporting on some newer Allwinner SoC. > + > menu "Texas Instruments thermal drivers" > depends on ARCH_HAS_BANDGAP || COMPILE_TEST > source "drivers/thermal/ti-soc-thermal/Kconfig" > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile > index cfae6a6..3a25e3c 100644 > --- a/drivers/thermal/Makefile > +++ b/drivers/thermal/Makefile > @@ -48,3 +48,4 @@ obj-$(CONFIG_INTEL_PCH_THERMAL) +=3D intel_pch_thermal.o > obj-$(CONFIG_ST_THERMAL) +=3D st/ > obj-$(CONFIG_TEGRA_SOCTHERM) +=3D tegra_soctherm.o > obj-$(CONFIG_HISI_THERMAL) +=3D hisi_thermal.o > +obj-$(CONFIG_SUNXI_THS) +=3D sunxi_ths.o And call it sun8i_ths. > diff --git a/drivers/thermal/sunxi_ths.c b/drivers/thermal/sunxi_ths.c > new file mode 100644 > index 0000000..650cd39 > --- /dev/null > +++ b/drivers/thermal/sunxi_ths.c > @@ -0,0 +1,263 @@ > +/* > + * Sunxi THS driver > + * > + * Copyright (C) 2015 Josef Gajdusek > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include You don't need this header, it's meant for clock providers, as its name suggest. You're only using the consumer API. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define THS_H3_CTRL0 0x00 > +#define THS_H3_CTRL1 0x04 > +#define THS_H3_CDAT 0x14 > +#define THS_H3_CTRL2 0x40 > +#define THS_H3_INT_CTRL 0x44 > +#define THS_H3_STAT 0x48 > +#define THS_H3_ALARM_CTRL 0x50 > +#define THS_H3_SHUTDOWN_CTRL 0x60 > +#define THS_H3_FILTER 0x70 > +#define THS_H3_CDATA 0x74 > +#define THS_H3_DATA 0x80 > + > +#define THS_H3_CTRL0_SENSOR_ACQ0 0 > + > +#define THS_H3_CTRL1_ADC_CALI_EN 17 > +#define THS_H3_CTRL1_OP_BIAS 20 > + > +#define THS_H3_CTRL2_SENSE_EN 0 > +#define THS_H3_CTRL2_SENSOR_ACQ1 16 > + > +#define THS_H3_INT_CTRL_ALARM_INT_EN 0 > +#define THS_H3_INT_CTRL_SHUT_INT_EN 4 > +#define THS_H3_INT_CTRL_DATA_IRQ_EN 8 > +#define THS_H3_INT_CTRL_THERMAL_PER 12 > + > +#define THS_H3_STAT_ALARM_INT_STS 0 > +#define THS_H3_STAT_SHUT_INT_STS 4 > +#define THS_H3_STAT_DATA_IRQ_STS 8 > +#define THS_H3_STAT_ALARM_OFF_STS 12 > + > +#define THS_H3_ALARM_CTRL_ALARM0_T_HYST 0 > +#define THS_H3_ALARM_CTRL_ALARM0_T_HOT 16 > + > +#define THS_H3_SHUTDOWN_CTRL_SHUT0_T_HOT 16 Usually, the masks are defined here, not the offsets, so that we can use them directly in the code later, without risking to miss a BIT() call. > + > +#define THS_H3_FILTER_TYPE 0 > +#define THS_H3_FILTER_EN 2 > + > +struct sunxi_ths_data { > + struct sunxi_ths_type *type; > + struct reset_control *reset; > + struct clk *clk; > + struct clk *busclk; > + void __iomem *regs; > + void __iomem *calreg; > + struct platform_device *pdev; > + struct thermal_zone_device *tzd; > + int irq; > +}; > + > +struct sunxi_ths_type { > + int (*init)(struct sunxi_ths_data *); > + int (*get_temp)(struct sunxi_ths_data *, int *out); > +}; What do you need that structure for? Can't you just call the functions directly? > + > +static int sunxi_ths_reg_to_temperature(int32_t reg, int divisor, int mi= nus) > +{ > + return minus - (reg * 1000000) / divisor; > +} > + > +static int sunxi_ths_get_temp(void *_data, int *out) > +{ > + struct sunxi_ths_data *data =3D _data; > + > + return data->type->get_temp(data, out); > +} > + > +static int sunxi_ths_h3_init(struct sunxi_ths_data *data) > +{ > + if (data->calreg) > + writel(readl(data->calreg) & 0xfff, data->regs + THS_H3_CDATA); As pointed out by Chen-Yu, you should be using nvmem here. > + /* Magical constants mostly taken from Allwinner 3.4 kernel. > + * Seem to work fine, though this could be configurable in DT/sysft > + */ > + writel(0xff << THS_H3_CTRL0_SENSOR_ACQ0, > + data->regs + THS_H3_CTRL0); > + writel((0x3f << THS_H3_CTRL2_SENSOR_ACQ1) | BIT(THS_H3_CTRL2_SENSE_EN), > + data->regs + THS_H3_CTRL2); > + writel((0x390 << THS_H3_INT_CTRL_THERMAL_PER) | BIT(THS_H3_INT_CTRL_DAT= A_IRQ_EN), > + data->regs + THS_H3_INT_CTRL); > + writel(BIT(THS_H3_FILTER_EN) | (0x2 << THS_H3_FILTER_TYPE), > + data->regs + THS_H3_FILTER); Please define those values, even if they are not documented. > + return 0; > +} > + > +static int sunxi_ths_h3_get_temp(struct sunxi_ths_data *data, int *out) > +{ > + *out =3D sunxi_ths_reg_to_temperature(readl(data->regs + THS_H3_DATA), > + 8253, 217000); > + return 0; > +} > + > +static const struct thermal_zone_of_device_ops sunxi_ths_thermal_ops =3D= { > + .get_temp =3D sunxi_ths_get_temp, > +}; > + > +static const struct sunxi_ths_type sunxi_ths_device_h3 =3D { > + .init =3D sunxi_ths_h3_init, > + .get_temp =3D sunxi_ths_h3_get_temp, > +}; > + > +static const struct of_device_id sunxi_ths_id_table[] =3D { > + { > + .compatible =3D "allwinner,sun8i-h3-ths", > + .data =3D &sunxi_ths_device_h3, > + }, > + { > + /* sentinel */ > + }, > +}; > +MODULE_DEVICE_TABLE(of, sunxi_ths_id_table); > + > +static int sunxi_ths_probe(struct platform_device *pdev) > +{ > + struct device_node *np =3D pdev->dev.of_node; > + const struct of_device_id *match; > + struct sunxi_ths_data *data; > + struct resource *res; > + int ret; > + > + match =3D of_match_node(sunxi_ths_id_table, np); > + if (!match) > + return -ENXIO; It cannot fail, otherwise you wouldn't have probed in the first place. > + > + data =3D > + devm_kzalloc(&pdev->dev, sizeof(struct sunxi_ths_data), GFP_KERNEL); Why did you split this on an new line? > + if (!data) > + return -ENOMEM; > + > + data->type =3D (struct sunxi_ths_type *) match->data; > + data->pdev =3D pdev; > + > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->regs =3D devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->regs)) { > + ret =3D PTR_ERR(data->regs); > + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); > + return ret; > + } > + > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 1); > + data->calreg =3D devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->calreg)) > + data->calreg =3D NULL; /* No calibration register */ Please use the nvmem framework here. > + data->busclk =3D devm_clk_get(&pdev->dev, "ahb"); > + if (IS_ERR(data->busclk)) { > + ret =3D PTR_ERR(data->busclk); > + dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret); > + return ret; > + } > + > + data->clk =3D devm_clk_get(&pdev->dev, "ths"); > + if (IS_ERR(data->clk)) { > + ret =3D PTR_ERR(data->clk); > + dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret); > + return ret; > + } > + > + data->reset =3D devm_reset_control_get_optional(&pdev->dev, "ahb"); Are you sure it's optional? In which cases? > + if (IS_ERR(data->reset)) { > + ret =3D PTR_ERR(data->reset); > + dev_err(&pdev->dev, "failed to get reset: %d\n", ret); > + return ret; > + } > + > + ret =3D clk_prepare_enable(data->busclk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret); > + return ret; > + } > + > + ret =3D clk_prepare_enable(data->clk); You probably don't need to enable the module clock all the time, but only when reading values. Is there a callback in the thermal subsystem that gets called when something start reading values? or can you sleep in get_temp? > + if (ret) { > + dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret); > + goto err_disable_bus; > + } > + > + ret =3D reset_control_deassert(data->reset); If you're using an optional reset control, it will probably fail. > + if (ret) { > + dev_err(&pdev->dev, "reset deassert failed: %d\n", ret); > + goto err_disable_ths; > + } > + > + ret =3D data->type->init(data); > + if (ret) > + goto err_reset_assert; > + > + data->tzd =3D thermal_zone_of_sensor_register(&pdev->dev, 0, data, > + &sunxi_ths_thermal_ops); Your wrapping is weird here too, it should be aligned on the opening parenthesis. > + if (IS_ERR(data->tzd)) { > + ret =3D PTR_ERR(data->tzd); > + dev_err(&pdev->dev, "failed to register thermal zone: %d\n", > + ret); > + goto err_reset_assert; > + } > + > + platform_set_drvdata(pdev, data); > + return 0; > + > +err_reset_assert: > + reset_control_assert(data->reset); > +err_disable_ths: > + clk_disable_unprepare(data->clk); > +err_disable_bus: > + clk_disable_unprepare(data->busclk); > + return ret; > +} > + > +static int sunxi_ths_remove(struct platform_device *pdev) > +{ > + struct sunxi_ths_data *data =3D platform_get_drvdata(pdev); > + > + thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd); > + reset_control_assert(data->reset); > + clk_disable_unprepare(data->clk); > + clk_disable_unprepare(data->busclk); > + return 0; > +} > + > +static struct platform_driver sunxi_ths_driver =3D { > + .probe =3D sunxi_ths_probe, > + .remove =3D sunxi_ths_remove, > + .driver =3D { > + .name =3D "sunxi_ths", > + .of_match_table =3D sunxi_ths_id_table, > + }, > +}; > + > +module_platform_driver(sunxi_ths_driver); > + > +MODULE_AUTHOR("Josef Gajdusek "); > +MODULE_DESCRIPTION("Sunxi THS driver"); > +MODULE_LICENSE("GPL v2"); It looks fine otherwise, thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --T5F8Mpu1Sg8jtYtl Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWT1FUAAoJEBx+YmzsjxAgLWsP/j89WCPxLz2sJu+WsuYpiH7F cIrHQuk1O2SeDMoaizy6ySd01SR93dqvFwoDwt7NCbPS4cJtbVT2EIR/vu4wRAAt glrX3EOh/62Mb1Tbi5LMGl+wuchFdz9HhR6CyqB29ZrZMusxMHMvnIAvbzAPJL+D CeFCF7AvD3sQmXF0Vlai8iRnqjFxH/4ecPy0aqCQm7eDzJE/mSfTZkknvsQgd0Ps rICNmgD3ApKrVktQdClvck2QwLbQ+O86e5SlmRUX/4+5IPPyh0RTrN5FdVY+zY/h xQFCICKpC7alY5v5NYpN6fiW8TNTZFZmswR0AnwTdbxFbyAfFYm5dLMY27AhURm1 yFMZdSI5QKXKjkvi5CWJoT50gGmHzqN8cob+N9i8j6ohcD/nv+XNJuAbElDUHLYd vjwhkt3hAdO7SqswyE7jqH23D6qowO6ajrb4dbwLDkNQb6pRW+SrMNZxum9Qg2FU rgMb+TQ1OcMFjpr+vE811ZvcgmhS1g0JgE7pi5nKoxA6VrZ5uxGwWatPOFxRZk3s MMaICezjF9zrYQo65+68sqolO+JMTh1bGouOh8ks6rO5fcX32JAo+ZLXpKwvDlDo NnrZgpMd300bMd7wYcpLzn8LXZQAIKXk7QXv1ubZB1UJpyNPbeLgz5PIjb18GxWf bM0znqCQ1kkny1fNNtJe =edtV -----END PGP SIGNATURE----- --T5F8Mpu1Sg8jtYtl-- -- 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/