[PATCH 0/3] Add support of New Amlogic temperature sensor for G12A SoCs

[PATCH 0/3] Add support of New Amlogic temperature sensor for G12A SoCs

[Guillaume La Roque]

This patchs series add support of New Amlogic temperature sensor.
This driver is based on IIO frameworks.
formulas and calibration values come from amlogic.

Dependencies :
- patch 2: temperature sensor clock are needed [1]

[1] https://lkml.kernel.org/r/20190412100221.26740-1-glaroque@baylibre.com

Guillaume La Roque (3):
  Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor
  arm64: dts: meson: g12a: add temperature sensor node
  iio: temperature: add a driver for the temperature sensor found in
    Amlogic Meson G12 SoCs

 .../iio/temperature/amlogic,meson-tsensor.txt |  31 ++
 arch/arm64/boot/dts/amlogic/meson-g12a.dtsi   |  22 +
 drivers/iio/temperature/Kconfig               |  11 +
 drivers/iio/temperature/Makefile              |   1 +
 drivers/iio/temperature/meson_tsensor.c       | 416 ++++++++++++++++++
 5 files changed, 481 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
 create mode 100644 drivers/iio/temperature/meson_tsensor.c

-- 
2.17.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor

[Guillaume La Roque]

This adds the devicetree binding documentation for the Temperature
Sensor found in the Amlogic Meson G12 SoCs.
Currently only the G12A SoCs are supported.

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt

diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
new file mode 100644
index 000000000000..d064db0e9cac
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
@@ -0,0 +1,31 @@
+* Amlogic Meson Temperature Sensor
+
+Required properties:
+- compatible:	depending on the SoC and the position of the sensor,
+		this should be one of:
+		- "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor
+		- "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor
+		followed by the common :
+		- "amlogic,meson-g12a-tsensor" for G12A SoC family
+- reg:		the physical base address and length of the registers
+- interrupts:	the interrupt indicating end of sampling
+- clocks:	phandle identifier for the reference clock of temperature sensor
+- #io-channel-cells: must be 1, see ../iio-bindings.txt
+- amlogic,ao-secure: phandle to the ao-secure syscon
+
+Optional properties:
+- amlogic,critical-temperature: temperature value in milli degrees Celsius
+	to set automatic reboot on too high temperature
+
+Example:
+	cpu_temp: temperature-sensor@ff634800 {
+		compatible = "amlogic,meson-g12a-cpu-tsensor",
+			     "amlogic,meson-g12a-tsensor";
+		reg = <0x0 0xff634800 0x0 0x50>;
+		interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&clkc CLKID_TS>;
+		status = "okay";
+		#io-channel-cells = <1>;
+		amlogic,meson-ao-secure = <&sec_AO>;
+		amlogic,critical-temperature = <115000>;
+	};
-- 
2.17.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor

[Martin Blumenstingl]

Hi Guillaume,

thank you for working on this!

On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote:
>
> This adds the devicetree binding documentation for the Temperature
> Sensor found in the Amlogic Meson G12 SoCs.
> Currently only the G12A SoCs are supported.
so G12B is not supported (yet)?

> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> new file mode 100644
> index 000000000000..d064db0e9cac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> @@ -0,0 +1,31 @@
> +* Amlogic Meson Temperature Sensor
> +
> +Required properties:
> +- compatible:  depending on the SoC and the position of the sensor,
> +               this should be one of:
> +               - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor
> +               - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor
> +               followed by the common :
> +               - "amlogic,meson-g12a-tsensor" for G12A SoC family
> +- reg:         the physical base address and length of the registers
> +- interrupts:  the interrupt indicating end of sampling
> +- clocks:      phandle identifier for the reference clock of temperature sensor
> +- #io-channel-cells: must be 1, see ../iio-bindings.txt
have you considered using the thermal framework [0] instead of the iio
framework (see below)?

> +- amlogic,ao-secure: phandle to the ao-secure syscon
the driver has some "u_efuse_off" access. do we need to get some
calibration values from the AO syscon or can we also fetch it from the
eFuse? you can look at arch/arm/boot/dts/meson8.dtsi where I'm passing
the temperature sensor calibration data to the SAR ADC (there's no
dedicated temperature sensor IP block prior to G12A) while reading the
data from the eFuse

> +Optional properties:
> +- amlogic,critical-temperature: temperature value in milli degrees Celsius
> +       to set automatic reboot on too high temperature
as far as I can tell the thermal framework supports multiple trip
points. I'm seeing this as a benefit because the hardware can raise
interrupts at four different temperatures (defined by the driver)

> +Example:
> +       cpu_temp: temperature-sensor@ff634800 {
> +               compatible = "amlogic,meson-g12a-cpu-tsensor",
> +                            "amlogic,meson-g12a-tsensor";
> +               reg = <0x0 0xff634800 0x0 0x50>;
> +               interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
> +               clocks = <&clkc CLKID_TS>;
> +               status = "okay";
as far as I know the dt-bindings should not have a status property in
the examples


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor

[Jonathan Cameron]

On Thu, 6 Jun 2019 21:16:37 +0200
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Guillaume,
> 
> thank you for working on this!
On comment from me inline.

Thanks,

Jonathan

> 
> On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote:
> >
> > This adds the devicetree binding documentation for the Temperature
> > Sensor found in the Amlogic Meson G12 SoCs.
> > Currently only the G12A SoCs are supported.  
> so G12B is not supported (yet)?
> 
> > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> > ---
> >  .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> >
> > diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> > new file mode 100644
> > index 000000000000..d064db0e9cac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> > @@ -0,0 +1,31 @@
> > +* Amlogic Meson Temperature Sensor
> > +
> > +Required properties:
> > +- compatible:  depending on the SoC and the position of the sensor,
> > +               this should be one of:
> > +               - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor
> > +               - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor
> > +               followed by the common :
> > +               - "amlogic,meson-g12a-tsensor" for G12A SoC family
> > +- reg:         the physical base address and length of the registers
> > +- interrupts:  the interrupt indicating end of sampling
> > +- clocks:      phandle identifier for the reference clock of temperature sensor
> > +- #io-channel-cells: must be 1, see ../iio-bindings.txt  
> have you considered using the thermal framework [0] instead of the iio
> framework (see below)?
I had the same thought.  Right now, this doesn't look generic enough
for IIO to make that much sense.

I'll review anyway as may give some useful pointers even if you do move
it over to thermal.

> 
> > +- amlogic,ao-secure: phandle to the ao-secure syscon  
> the driver has some "u_efuse_off" access. do we need to get some
> calibration values from the AO syscon or can we also fetch it from the
> eFuse? you can look at arch/arm/boot/dts/meson8.dtsi where I'm passing
> the temperature sensor calibration data to the SAR ADC (there's no
> dedicated temperature sensor IP block prior to G12A) while reading the
> data from the eFuse
> 
> > +Optional properties:
> > +- amlogic,critical-temperature: temperature value in milli degrees Celsius
> > +       to set automatic reboot on too high temperature  
> as far as I can tell the thermal framework supports multiple trip
> points. I'm seeing this as a benefit because the hardware can raise
> interrupts at four different temperatures (defined by the driver)
> 
> > +Example:
> > +       cpu_temp: temperature-sensor@ff634800 {
> > +               compatible = "amlogic,meson-g12a-cpu-tsensor",
> > +                            "amlogic,meson-g12a-tsensor";
> > +               reg = <0x0 0xff634800 0x0 0x50>;
> > +               interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
> > +               clocks = <&clkc CLKID_TS>;
> > +               status = "okay";  
> as far as I know the dt-bindings should not have a status property in
> the examples
> 
> 
> Martin


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor

[Neil Armstrong]

On 06/06/2019 21:16, Martin Blumenstingl wrote:
> Hi Guillaume,
> 
> thank you for working on this!
> 
> On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote:
>>
>> This adds the devicetree binding documentation for the Temperature
>> Sensor found in the Amlogic Meson G12 SoCs.
>> Currently only the G12A SoCs are supported.
> so G12B is not supported (yet)?

G12B is 95% similar as G12A, it will certainly use slighly different values.

> 
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>>  .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
>> new file mode 100644
>> index 000000000000..d064db0e9cac
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
>> @@ -0,0 +1,31 @@
>> +* Amlogic Meson Temperature Sensor
>> +
>> +Required properties:
>> +- compatible:  depending on the SoC and the position of the sensor,
>> +               this should be one of:
>> +               - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor
>> +               - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor
>> +               followed by the common :
>> +               - "amlogic,meson-g12a-tsensor" for G12A SoC family
>> +- reg:         the physical base address and length of the registers
>> +- interrupts:  the interrupt indicating end of sampling
>> +- clocks:      phandle identifier for the reference clock of temperature sensor
>> +- #io-channel-cells: must be 1, see ../iio-bindings.txt
> have you considered using the thermal framework [0] instead of the iio
> framework (see below)?

Question: why thermal, and not hwmon ? what's the main difference ?

> 
>> +- amlogic,ao-secure: phandle to the ao-secure syscon
> the driver has some "u_efuse_off" access. do we need to get some
> calibration values from the AO syscon or can we also fetch it from the
> eFuse? you can look at arch/arm/boot/dts/meson8.dtsi where I'm passing
> the temperature sensor calibration data to the SAR ADC (there's no
> dedicated temperature sensor IP block prior to G12A) while reading the
> data from the eFuse
> 
>> +Optional properties:
>> +- amlogic,critical-temperature: temperature value in milli degrees Celsius
>> +       to set automatic reboot on too high temperature
> as far as I can tell the thermal framework supports multiple trip
> points. I'm seeing this as a benefit because the hardware can raise
> interrupts at four different temperatures (defined by the driver)

Theoretically, but the implementation code differs a lot from the datasheet.

> 
>> +Example:
>> +       cpu_temp: temperature-sensor@ff634800 {
>> +               compatible = "amlogic,meson-g12a-cpu-tsensor",
>> +                            "amlogic,meson-g12a-tsensor";
>> +               reg = <0x0 0xff634800 0x0 0x50>;
>> +               interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
>> +               clocks = <&clkc CLKID_TS>;
>> +               status = "okay";
> as far as I know the dt-bindings should not have a status property in
> the examples
> 
> 
> Martin
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor

[Martin Blumenstingl]

Hi Neil,

On Tue, Jun 11, 2019 at 1:01 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 06/06/2019 21:16, Martin Blumenstingl wrote:
> > Hi Guillaume,
> >
> > thank you for working on this!
> >
> > On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote:
> >>
> >> This adds the devicetree binding documentation for the Temperature
> >> Sensor found in the Amlogic Meson G12 SoCs.
> >> Currently only the G12A SoCs are supported.
> > so G12B is not supported (yet)?
>
> G12B is 95% similar as G12A, it will certainly use slighly different values.
OK, thank you for clarifying this
as far as I can tell Guillaume's code is already prepared for that (as
there's a per-instance specific struct with settings for the specific
instance) which is good to know

> >
> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> >> ---
> >>  .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++
> >>  1 file changed, 31 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> >> new file mode 100644
> >> index 000000000000..d064db0e9cac
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> >> @@ -0,0 +1,31 @@
> >> +* Amlogic Meson Temperature Sensor
> >> +
> >> +Required properties:
> >> +- compatible:  depending on the SoC and the position of the sensor,
> >> +               this should be one of:
> >> +               - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor
> >> +               - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor
> >> +               followed by the common :
> >> +               - "amlogic,meson-g12a-tsensor" for G12A SoC family
> >> +- reg:         the physical base address and length of the registers
> >> +- interrupts:  the interrupt indicating end of sampling
> >> +- clocks:      phandle identifier for the reference clock of temperature sensor
> >> +- #io-channel-cells: must be 1, see ../iio-bindings.txt
> > have you considered using the thermal framework [0] instead of the iio
> > framework (see below)?
>
> Question: why thermal, and not hwmon ? what's the main difference ?
this is what came to my mind why the thermal framework fits best (at
least based on my current knowledge):
a) the thermal-zones (see meson-gxm-khadas-vim2.dts for example) a
"thermal-sensors" property. so for active (with a fan) or passive (by
limiting the maximum frequency and thus the supply voltage) cooling we
need a thermal device anyways
b) the thermal bindings support multiple trip points. we can map them
to one of the four interrupts which the IP block can generate
c) defining a temperature where the chip will power off sounds like a
use-case which may be implemented by other thermal IP blocks (in other
words: maybe the thermal frameworks provides some generic property to
replace the "amlogic,critical-temperature" property)
d) as far as I know you can tell the thermal framework to create a
hwmon device with only a couple (5?) lines of code

as Guillaume has already shown we can implement c) with a custom
property, but that's not limited to the underlying framework (IIO,
hwmon, thermal, ...)

use-case d) is not a strong one because I'm using iio-hwmon to create
a hwmon device on the 32-bit SoCs.
however, together with case a) using an IIO driver is going to be more
difficult because currently there's "only" a "generic-adc-thermal"
binding (but not a "generic-iio-temperature-thermal" binding)

the initial driver version doesn't have to support everything I listed above.
however, I believe with the thermal framework we don't limit ourselves
to one use-case and can extend the driver in the future


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson Temperature Sensor

[Jonathan Cameron]

On Tue,  4 Jun 2019 16:47:12 +0200
Guillaume La Roque <glaroque@baylibre.com> wrote:

> This adds the devicetree binding documentation for the Temperature
> Sensor found in the Amlogic Meson G12 SoCs.
> Currently only the G12A SoCs are supported.
> 
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>

Hi Guillaume,

I'm afraid we decided a month or so back that all new dt bindings proposed
for IIO drivers should be in yaml format.

Please reformat this appropriately for v2 and make sure to run
make dt_bindings_check.

There are a few examples in tree already, but we are deliberately
not converting existing bindings too quickly to avoid overloading
reviewers.

Thanks,

Jonathan

> ---
>  .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> new file mode 100644
> index 000000000000..d064db0e9cac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt
> @@ -0,0 +1,31 @@
> +* Amlogic Meson Temperature Sensor
> +
> +Required properties:
> +- compatible:	depending on the SoC and the position of the sensor,
> +		this should be one of:
> +		- "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor
> +		- "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor
> +		followed by the common :
> +		- "amlogic,meson-g12a-tsensor" for G12A SoC family
> +- reg:		the physical base address and length of the registers
> +- interrupts:	the interrupt indicating end of sampling
> +- clocks:	phandle identifier for the reference clock of temperature sensor
> +- #io-channel-cells: must be 1, see ../iio-bindings.txt
> +- amlogic,ao-secure: phandle to the ao-secure syscon
> +
> +Optional properties:
> +- amlogic,critical-temperature: temperature value in milli degrees Celsius
> +	to set automatic reboot on too high temperature
> +
> +Example:
> +	cpu_temp: temperature-sensor@ff634800 {
> +		compatible = "amlogic,meson-g12a-cpu-tsensor",
> +			     "amlogic,meson-g12a-tsensor";
> +		reg = <0x0 0xff634800 0x0 0x50>;
> +		interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&clkc CLKID_TS>;
> +		status = "okay";
> +		#io-channel-cells = <1>;
> +		amlogic,meson-ao-secure = <&sec_AO>;
> +		amlogic,critical-temperature = <115000>;
> +	};


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 2/3] arm64: dts: meson: g12a: add temperature sensor node

[Guillaume La Roque]

Add two temperature sensor
the first near CPU and GPU, second near DDR

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 22 +++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
index 840dab606110..37f17087bdb1 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
@@ -1360,6 +1360,28 @@
 				};
 			};
 
+			cpu_temp: temperature-sensor@34800 {
+				compatible = "amlogic,meson-g12a-cpu-tsensor",
+					     "amlogic,meson-g12a-tsensor";
+				reg = <0x0 0x34800 0x0 0x50>;
+				interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
+				clocks = <&clkc CLKID_TS>;
+				status = "okay";
+				#io-channel-cells = <1>;
+				amlogic,ao-secure = <&sec_AO>;
+			};
+
+			ddr_temp: temperature-sensor@34c00 {
+				compatible = "amlogic,meson-g12a-ddr-tsensor",
+					     "amlogic,meson-g12a-tsensor";
+				reg = <0x0 0x34c00 0x0 0x50>;
+				interrupts = <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>;
+				clocks = <&clkc CLKID_TS>;
+				status = "okay";
+				#io-channel-cells = <1>;
+				amlogic,ao-secure = <&sec_AO>;
+			};
+
 			usb2_phy0: phy@36000 {
 				compatible = "amlogic,g12a-usb2-phy";
 				reg = <0x0 0x36000 0x0 0x2000>;
-- 
2.17.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 3/3] iio: temperature: add a driver for the temperature sensor found in Amlogic Meson G12 SoCs

[Guillaume La Roque]

The code is based on Amlogic source code. No public datasheet for this.
Currently the G12A SoCs are supported.

Supported features:
- possibility to set an automatic reboot temperature

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 drivers/iio/temperature/Kconfig         |  11 +
 drivers/iio/temperature/Makefile        |   1 +
 drivers/iio/temperature/meson_tsensor.c | 416 ++++++++++++++++++++++++
 3 files changed, 428 insertions(+)
 create mode 100644 drivers/iio/temperature/meson_tsensor.c

diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 737faa0901fe..712a0062790d 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -34,6 +34,17 @@ config HID_SENSOR_TEMP
 	  To compile this driver as a module, choose M here: the module
 	  will be called hid-sensor-temperature.
 
+config MESON_TSENSOR
+	tristate "Amlogic Meson temperature sensor Support"
+	default ARCH_MESON
+	depends on OF && ARCH_MESON
+	help
+	  If you say yess here you get support for Meson Temperature sensor
+	  for G12 SoC Family.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called meson_tsensor.
+
 config MLX90614
 	tristate "MLX90614 contact-less infrared sensor"
 	depends on I2C
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index baca4776ca0d..466d8c1c91d6 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
 obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
 obj-$(CONFIG_MAX31856) += max31856.o
+obj-$(CONFIG_MESON_TSENSOR) += meson_tsensor.o
 obj-$(CONFIG_MLX90614) += mlx90614.o
 obj-$(CONFIG_MLX90632) += mlx90632.o
 obj-$(CONFIG_TMP006) += tmp006.o
diff --git a/drivers/iio/temperature/meson_tsensor.c b/drivers/iio/temperature/meson_tsensor.c
new file mode 100644
index 000000000000..be0a8d073ba3
--- /dev/null
+++ b/drivers/iio/temperature/meson_tsensor.c
@@ -0,0 +1,416 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson Temperature Sensor
+ *
+ * Copyright (C) 2017 Huan Biao <huan.biao@amlogic.com>
+ * Copyright (C) 2019 Guillaume La Roque <glaroque@baylibre.com>
+ *
+ * Register value to celsius temperature formulas:
+ *	Read_Val	    m * U
+ * U = ---------, Uptat = ---------
+ *	2^16		  1 + n * U
+ *
+ * Temperature = A * ( Uptat + u_efuse / 2^16 )- B
+ *
+ *  A B m n : calibration parameters
+ *  u_efuse : fused calibration value, it's a signed 16 bits value
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/iio/iio.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define TSENSOR_CFG_REG1			0x4
+	#define TSENSOR_CFG_REG1_RSET_VBG	BIT(12)
+	#define TSENSOR_CFG_REG1_RSET_ADC	BIT(11)
+	#define TSENSOR_CFG_REG1_VCM_EN		BIT(10)
+	#define TSENSOR_CFG_REG1_VBG_EN		BIT(9)
+	#define TSENSOR_CFG_REG1_OUT_CTL	BIT(6)
+	#define TSENSOR_CFG_REG1_FILTER_EN	BIT(5)
+	#define TSENSOR_CFG_REG1_DEM_EN		BIT(3)
+	#define TSENSOR_CFG_REG1_CH_SEL		GENMASK(1, 0)
+	#define TSENSOR_CFG_REG1_ENABLE		\
+		(TSENSOR_CFG_REG1_FILTER_EN |	\
+		 TSENSOR_CFG_REG1_VCM_EN |	\
+		 TSENSOR_CFG_REG1_VBG_EN |	\
+		 TSENSOR_CFG_REG1_DEM_EN |	\
+		 TSENSOR_CFG_REG1_CH_SEL)
+
+#define TSENSOR_CFG_REG2				0x8
+	#define TSENSOR_CFG_REG2_HITEMP_EN		BIT(31)
+	#define TSENSOR_CFG_REG2_REBOOT_ALL_EN		BIT(30)
+	#define TSENSOR_CFG_REG2_REBOOT_TIME		GENMASK(25, 16)
+	#define TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE	\
+		(TSENSOR_CFG_REG2_HITEMP_EN |		\
+		 TSENSOR_CFG_REG2_REBOOT_ALL_EN |	\
+		 TSENSOR_CFG_REG2_REBOOT_TIME)
+	#define TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE_MASK		\
+		(GENMASK(31, 30) | GENMASK(25, 4))
+	#define TSENSOR_CFG_REG2_HITEMP_REBOOT_REG_MASK			\
+		GENMASK(15, 4)
+	#define TSENSOR_CFG_REG2_HITEMP_REG_VAL(_reg_val)		\
+		(FIELD_PREP(TSENSOR_CFG_REG2_HITEMP_REBOOT_REG_MASK,	\
+			    _reg_val) |					\
+		 TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE)
+
+#define TSENSOR_CFG_REG3		0xC
+#define TSENSOR_CFG_REG4		0x10
+#define TSENSOR_CFG_REG5		0x14
+#define TSENSOR_CFG_REG6		0x18
+#define TSENSOR_CFG_REG7		0x1C
+#define TSENSOR_CFG_REG8		0x20
+
+#define TSENSOR_STAT0			0x40
+
+#define TSENSOR_STAT9			0x64
+
+#define TSENSOR_READ_TEMP_MASK		GENMASK(15, 0)
+#define TSENSOR_TEMP_MASK		GENMASK(11, 0)
+
+#define TSENSOR_TRIM_SIGN_MASK		BIT(15)
+#define TSENSOR_TRIM_TEMP_MASK		GENMASK(14, 0)
+#define TSENSOR_TRIM_VERSION_MASK	GENMASK(31, 24)
+
+#define TSENSOR_TRIM_VERSION(_version) 	\
+	FIELD_GET(TSENSOR_TRIM_VERSION_MASK, _version)
+
+#define TSENSOR_TRIM_CALIB_VALID_MASK	(GENMASK(3, 2) | BIT(7))
+
+#define TSENSOR_CALIB_OFFSET	1
+#define TSENSOR_CALIB_SHIFT	4
+
+static const struct iio_chan_spec temperature_channel[] = {
+	{
+		.type = IIO_TEMP,
+		.channel = 0,
+		.address = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+	},
+};
+
+/**
+ * struct meson_tsensor_soc_data
+ * @A, B, m, n: calibration parameters
+ * This structure is required for configuration of meson tsensor driver.
+ */
+struct meson_tsensor_soc_data {
+	int A;
+	int B;
+	int m;
+	int n;
+};
+
+/**
+ * struct meson_tsensor_data
+ * @u_efuse_off: register offset to read fused calibration value
+ * @soc: calibration parameters structure pointer
+ * @regmap_config: regmap config for the device
+ * This structure is required for configuration of meson tsensor driver.
+ */
+struct meson_tsensor_data {
+	int u_efuse_off;
+	const struct meson_tsensor_soc_data *soc;
+	const struct regmap_config *regmap_config;
+};
+
+struct meson_tsensor {
+	int id;
+	const struct meson_tsensor_data *data;
+	struct regmap *regmap;
+	struct regmap *sec_ao_map;
+	struct clk *clk;
+	u32 trim_info;
+	void __iomem *base;
+	int reboot_temp;
+};
+
+/*
+ * tsensor treats temperature as a mapped temperature code.
+ * The temperature is converted differently depending on the calibration type.
+ */
+static u32 temp_to_code(struct meson_tsensor *priv, int temp)
+{
+	const struct meson_tsensor_soc_data *param = priv->data->soc;
+	s64 divisor, factor, uefuse;
+	u32 reg_code;
+
+	uefuse = priv->trim_info & TSENSOR_TRIM_SIGN_MASK ?
+			 ~(priv->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
+			 (priv->trim_info & TSENSOR_TRIM_TEMP_MASK);
+
+	factor = BIT(16) * (temp * 10 + param->B);
+	factor = div_s64(factor, param->A);
+	factor = factor + uefuse;
+
+	factor = factor * 100;
+
+	divisor = param->n * factor;
+	divisor = div_s64(divisor, BIT(16));
+	divisor = param->m - divisor;
+
+	reg_code = div_s64(factor, divisor);
+	reg_code = ((reg_code >> TSENSOR_CALIB_SHIFT) & TSENSOR_TEMP_MASK) +
+		   TSENSOR_CALIB_OFFSET;
+
+	return reg_code;
+}
+
+/*
+ * Calculate a temperature value from a temperature code.
+ * The unit of the temperature is degree Celsius.
+ */
+static int code_to_temp(struct meson_tsensor *priv, int temp_code)
+{
+	const struct meson_tsensor_soc_data *param = priv->data->soc;
+	int temp;
+	s64 factor, Uptat, uefuse;
+
+	uefuse = priv->trim_info & TSENSOR_TRIM_SIGN_MASK ?
+			     ~(priv->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
+			     (priv->trim_info & TSENSOR_TRIM_TEMP_MASK);
+
+	factor = param->n * temp_code;
+	factor = div_s64(factor, 100);
+
+	Uptat = temp_code * param->m;
+	Uptat = div_s64(Uptat, 100);
+	Uptat = Uptat * BIT(16);
+	Uptat = div_s64(Uptat, BIT(16) + factor);
+
+	temp = (Uptat + uefuse) * param->A;
+	temp = div_s64(temp, BIT(16));
+	temp = (temp - param->B) * 100;
+
+	return temp;
+}
+
+static int meson_tsensor_initialize(struct iio_dev *indio_dev)
+{
+	struct meson_tsensor *priv = iio_priv(indio_dev);
+	u32 reg_val;
+	int ret = 0;
+	int ver;
+
+	regmap_read(priv->sec_ao_map, priv->data->u_efuse_off,
+		    &priv->trim_info);
+
+	ver = TSENSOR_TRIM_VERSION(priv->trim_info);
+
+	if ((ver & TSENSOR_TRIM_CALIB_VALID_MASK) == 0) {
+		ret = -EINVAL;
+		dev_err(&indio_dev->dev,
+			"tsensor thermal calibration not supported: 0x%x!\n",
+			ver);
+		goto out;
+	}
+
+	/* init the ts reboot soc function */
+	if (priv->reboot_temp) {
+		/* register need value in celsius */
+		reg_val = temp_to_code(priv, priv->reboot_temp / 1000);
+		regmap_update_bits(priv->regmap, TSENSOR_CFG_REG2,
+				   TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE_MASK,
+				   TSENSOR_CFG_REG2_HITEMP_REG_VAL(reg_val));
+	}
+
+out:
+	return ret;
+}
+
+static int meson_tsensor_enable(struct iio_dev *indio_dev)
+{
+	struct meson_tsensor *priv = iio_priv(indio_dev);
+
+	clk_prepare_enable(priv->clk);
+	regmap_update_bits(priv->regmap, TSENSOR_CFG_REG1,
+			   TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
+
+	return 0;
+}
+
+static int meson_tsensor_disable(struct iio_dev *indio_dev)
+{
+	struct meson_tsensor *priv = iio_priv(indio_dev);
+
+	regmap_update_bits(priv->regmap, TSENSOR_CFG_REG1,
+			   TSENSOR_CFG_REG1_ENABLE, 0);
+	clk_disable(priv->clk);
+
+	return 0;
+}
+
+static int meson_tsensor_read(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int *val,
+			      int *val2, long mask)
+{
+	unsigned int tvalue;
+	struct meson_tsensor *priv = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		regmap_read(priv->regmap, TSENSOR_STAT0, &tvalue);
+		*val = code_to_temp(priv,
+				    tvalue & TSENSOR_READ_TEMP_MASK);
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info meson_tsensor_iio_info = {
+	.read_raw = &meson_tsensor_read,
+};
+
+static const struct regmap_config meson_tsensor_regmap_config_g12a = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = TSENSOR_STAT9,
+};
+
+static const struct meson_tsensor_soc_data meson_tsensor_g12a = {
+	.A = 9411,
+	.B = 3159,
+	.m = 424,
+	.n = 324,
+};
+
+static const struct meson_tsensor_data meson_tsensor_g12a_cpu_param = {
+	.u_efuse_off = 0x128,
+	.soc = &meson_tsensor_g12a,
+	.regmap_config = &meson_tsensor_regmap_config_g12a,
+};
+
+static const struct meson_tsensor_data meson_tsensor_g12a_ddr_param = {
+	.u_efuse_off = 0xF0,
+	.soc = &meson_tsensor_g12a,
+	.regmap_config = &meson_tsensor_regmap_config_g12a,
+};
+
+static const struct of_device_id meson_tsensor_of_match[] = {
+	{
+		.compatible = "amlogic,meson-g12a-ddr-tsensor",
+		.data = &meson_tsensor_g12a_ddr_param,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-cpu-tsensor",
+		.data = &meson_tsensor_g12a_cpu_param,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, meson_tsensor_of_match);
+
+static int meson_tsensor_probe(struct platform_device *pdev)
+{
+	struct meson_tsensor *priv;
+	struct iio_dev *indio_dev;
+	struct resource *res;
+
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
+	if (!indio_dev) {
+		dev_err(&pdev->dev, "failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	priv = iio_priv(indio_dev);
+	priv->data = of_device_get_match_data(&pdev->dev);
+	if (!priv->data) {
+		dev_err(&pdev->dev, "failed to get match data\n");
+		return -ENODEV;
+	}
+
+	indio_dev->channels = temperature_channel;
+	indio_dev->num_channels = ARRAY_SIZE(temperature_channel);
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &meson_tsensor_iio_info;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->regmap = devm_regmap_init_mmio(&pdev->dev, priv->base,
+					     priv->data->regmap_config);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		if (PTR_ERR(priv->clk) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to get clock\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node,
+				 "amlogic,critical-temperature",
+				 &priv->reboot_temp)) {
+		priv->reboot_temp = 0;
+	}
+
+	priv->sec_ao_map = syscon_regmap_lookup_by_phandle
+		(pdev->dev.of_node, "amlogic,ao-secure");
+	if (IS_ERR(priv->sec_ao_map)) {
+		dev_err(&pdev->dev, "syscon regmap lookup failed.\n");
+		return PTR_ERR(priv->sec_ao_map);
+	}
+
+	ret = meson_tsensor_initialize(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = meson_tsensor_enable(indio_dev);
+	if (ret)
+		goto err;
+
+	platform_set_drvdata(pdev, indio_dev);
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err_hw;
+
+	return 0;
+
+err_hw:
+	meson_tsensor_disable(indio_dev);
+err:
+	clk_unprepare(priv->clk);
+
+	return ret;
+}
+
+static int meson_tsensor_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	iio_device_unregister(indio_dev);
+
+	return meson_tsensor_disable(indio_dev);
+}
+
+static struct platform_driver meson_tsensor_driver = {
+	.probe = meson_tsensor_probe,
+	.remove = meson_tsensor_remove,
+	.driver = {
+			.name = "meson-tsensor",
+			.of_match_table = meson_tsensor_of_match,
+		},
+};
+
+module_platform_driver(meson_tsensor_driver);
+
+MODULE_AUTHOR("Guillaume La Roque <glaroque@baylibre.com>");
+MODULE_DESCRIPTION("Amlogic Meson Temperature Sensor Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/3] iio: temperature: add a driver for the temperature sensor found in Amlogic Meson G12 SoCs

[Martin Blumenstingl]

Hi Guillaume,

below are my initial impressions. I will have a closer look once
there's a decision whether this belongs to the IIO or thermal
framework.

On Tue, Jun 4, 2019 at 4:48 PM Guillaume La Roque <glaroque@baylibre.com> wrote:
>
> The code is based on Amlogic source code. No public datasheet for this.
the public S922X datasheet from Hardkernel [0] has some documentation
(starting at page 1106).

[...]
> +config MESON_TSENSOR
> +       tristate "Amlogic Meson temperature sensor Support"
> +       default ARCH_MESON
> +       depends on OF && ARCH_MESON
depends on OF && (ARCH_MESON || COMPILE_TEST)
so all the nice auto-builders / testing tools will speak up if someone
tries to break your driver

> +       help
> +         If you say yess here you get support for Meson Temperature sensor
s/yess/yes/

> +         for G12 SoC Family.
G12 (which I assume includes G12A and G12B) or G12A?

[...]
> diff --git a/drivers/iio/temperature/meson_tsensor.c b/drivers/iio/temperature/meson_tsensor.c
> new file mode 100644
> index 000000000000..be0a8d073ba3
> --- /dev/null
> +++ b/drivers/iio/temperature/meson_tsensor.c
> @@ -0,0 +1,416 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson Temperature Sensor
> + *
> + * Copyright (C) 2017 Huan Biao <huan.biao@amlogic.com>
> + * Copyright (C) 2019 Guillaume La Roque <glaroque@baylibre.com>
> + *
> + * Register value to celsius temperature formulas:
> + *     Read_Val            m * U
> + * U = ---------, Uptat = ---------
> + *     2^16              1 + n * U
> + *
> + * Temperature = A * ( Uptat + u_efuse / 2^16 )- B
> + *
> + *  A B m n : calibration parameters
> + *  u_efuse : fused calibration value, it's a signed 16 bits value
it's great to have this explained in the docs (instead of having to
look it up in some out of tree driver, as it's not part of the
datasheet) - thank you for this!

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/iio/iio.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define TSENSOR_CFG_REG1                       0x4
> +       #define TSENSOR_CFG_REG1_RSET_VBG       BIT(12)
> +       #define TSENSOR_CFG_REG1_RSET_ADC       BIT(11)
> +       #define TSENSOR_CFG_REG1_VCM_EN         BIT(10)
> +       #define TSENSOR_CFG_REG1_VBG_EN         BIT(9)
> +       #define TSENSOR_CFG_REG1_OUT_CTL        BIT(6)
> +       #define TSENSOR_CFG_REG1_FILTER_EN      BIT(5)
> +       #define TSENSOR_CFG_REG1_DEM_EN         BIT(3)
> +       #define TSENSOR_CFG_REG1_CH_SEL         GENMASK(1, 0)
> +       #define TSENSOR_CFG_REG1_ENABLE         \
> +               (TSENSOR_CFG_REG1_FILTER_EN |   \
> +                TSENSOR_CFG_REG1_VCM_EN |      \
> +                TSENSOR_CFG_REG1_VBG_EN |      \
> +                TSENSOR_CFG_REG1_DEM_EN |      \
> +                TSENSOR_CFG_REG1_CH_SEL)
are all of these needed to enabled *and* disable the temperature
sensor? TSENSOR_CFG_REG1_CH_SEL doesn't seem related when disabling
the sensor reading (but I don't know since there's no documentation)

> +#define TSENSOR_CFG_REG2                               0x8
> +       #define TSENSOR_CFG_REG2_HITEMP_EN              BIT(31)
> +       #define TSENSOR_CFG_REG2_REBOOT_ALL_EN          BIT(30)
> +       #define TSENSOR_CFG_REG2_REBOOT_TIME            GENMASK(25, 16)
> +       #define TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE   \
> +               (TSENSOR_CFG_REG2_HITEMP_EN |           \
> +                TSENSOR_CFG_REG2_REBOOT_ALL_EN |       \
> +                TSENSOR_CFG_REG2_REBOOT_TIME)
the name mix between TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE and
TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE_MASK confused me.
personally I would drop the macros which just bit-wise or multiple
other macros together

> +       #define TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE_MASK              \
> +               (GENMASK(31, 30) | GENMASK(25, 4))
> +       #define TSENSOR_CFG_REG2_HITEMP_REBOOT_REG_MASK                 \
> +               GENMASK(15, 4)
> +       #define TSENSOR_CFG_REG2_HITEMP_REG_VAL(_reg_val)               \
> +               (FIELD_PREP(TSENSOR_CFG_REG2_HITEMP_REBOOT_REG_MASK,    \
> +                           _reg_val) |                                 \
> +                TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE)
> +
> +#define TSENSOR_CFG_REG3               0xC
I like to use lower-case hex letters
and I also pad them -> 0x0c in this case because we have for example 0x10 below)

> +#define TSENSOR_CFG_REG4               0x10
> +#define TSENSOR_CFG_REG5               0x14
> +#define TSENSOR_CFG_REG6               0x18
> +#define TSENSOR_CFG_REG7               0x1C
> +#define TSENSOR_CFG_REG8               0x20
> +
> +#define TSENSOR_STAT0                  0x40
> +
> +#define TSENSOR_STAT9                  0x64
> +
> +#define TSENSOR_READ_TEMP_MASK         GENMASK(15, 0)
TSENSOR_STAT0_FILTER_OUT would match the naming from the datasheet

> +#define TSENSOR_TEMP_MASK              GENMASK(11, 0)
>
> +#define TSENSOR_TRIM_SIGN_MASK         BIT(15)
> +#define TSENSOR_TRIM_TEMP_MASK         GENMASK(14, 0)
> +#define TSENSOR_TRIM_VERSION_MASK      GENMASK(31, 24)
> +
> +#define TSENSOR_TRIM_VERSION(_version)         \
> +       FIELD_GET(TSENSOR_TRIM_VERSION_MASK, _version)
I would drop this and use the FIELD_GET directly where needed (it's
only one occurrence anyways)

[...]
> +static int meson_tsensor_enable(struct iio_dev *indio_dev)
> +{
> +       struct meson_tsensor *priv = iio_priv(indio_dev);
> +
> +       clk_prepare_enable(priv->clk);
may return an error which you're discarding here

> +       regmap_update_bits(priv->regmap, TSENSOR_CFG_REG1,
> +                          TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
> +
> +       return 0;
> +}
> +
> +static int meson_tsensor_disable(struct iio_dev *indio_dev)
> +{
> +       struct meson_tsensor *priv = iio_priv(indio_dev);
> +
> +       regmap_update_bits(priv->regmap, TSENSOR_CFG_REG1,
> +                          TSENSOR_CFG_REG1_ENABLE, 0);
> +       clk_disable(priv->clk);
clk_disable_unprepare as you're calling clk_prepare_enable above?

> +
> +       return 0;
make it a void function instead?

> +static const struct regmap_config meson_tsensor_regmap_config_g12a = {
> +       .reg_bits = 8,
> +       .val_bits = 32,
> +       .reg_stride = 4,
> +       .max_register = TSENSOR_STAT9,
.fast_io = true
if you ever need to ACK interrupts from the IRQ handler
(IIRC fast_io will use a spinlock instead of mutex)

[...]
> +static const struct of_device_id meson_tsensor_of_match[] = {
> +       {
> +               .compatible = "amlogic,meson-g12a-ddr-tsensor",
> +               .data = &meson_tsensor_g12a_ddr_param,
> +       },
> +       {
> +               .compatible = "amlogic,meson-g12a-cpu-tsensor",
> +               .data = &meson_tsensor_g12a_cpu_param,
> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, meson_tsensor_of_match);
I would move the of_device_id table above the platform_driver definition below
of_device_get_match_data doesn't need the of_device_id as parameter
(compared to it's predecessor)

> +static int meson_tsensor_probe(struct platform_device *pdev)
> +{
> +       struct meson_tsensor *priv;
> +       struct iio_dev *indio_dev;
> +       struct resource *res;
> +
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> +       if (!indio_dev) {
> +               dev_err(&pdev->dev, "failed allocating iio device\n");
> +               return -ENOMEM;
> +       }
> +
> +       priv = iio_priv(indio_dev);
> +       priv->data = of_device_get_match_data(&pdev->dev);
> +       if (!priv->data) {
> +               dev_err(&pdev->dev, "failed to get match data\n");
> +               return -ENODEV;
> +       }
> +
> +       indio_dev->channels = temperature_channel;
> +       indio_dev->num_channels = ARRAY_SIZE(temperature_channel);
> +       indio_dev->name = dev_name(&pdev->dev);
> +       indio_dev->dev.parent = &pdev->dev;
> +       indio_dev->dev.of_node = pdev->dev.of_node;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->info = &meson_tsensor_iio_info;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->base = devm_ioremap_resource(&pdev->dev, res);
you're only using priv->base in this function. consider dropping it
from struct meson_tsensor


[0] https://dn.odroid.com/S922X/ODROID-N2/Datasheet/S922X_Public_Datasheet_V0.2.pdf

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/3] iio: temperature: add a driver for the temperature sensor found in Amlogic Meson G12 SoCs

[Jonathan Cameron]

On Tue,  4 Jun 2019 16:47:14 +0200
Guillaume La Roque <glaroque@baylibre.com> wrote:

> The code is based on Amlogic source code. No public datasheet for this.
> Currently the G12A SoCs are supported.
> 
> Supported features:
> - possibility to set an automatic reboot temperature
> 
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
A few comments inline.

However, I'm still of the view this is probably better as a thermal
driver than an IIO one.

Jonathan
> ---
>  drivers/iio/temperature/Kconfig         |  11 +
>  drivers/iio/temperature/Makefile        |   1 +
>  drivers/iio/temperature/meson_tsensor.c | 416 ++++++++++++++++++++++++
>  3 files changed, 428 insertions(+)
>  create mode 100644 drivers/iio/temperature/meson_tsensor.c
> 
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 737faa0901fe..712a0062790d 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -34,6 +34,17 @@ config HID_SENSOR_TEMP
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called hid-sensor-temperature.
>  
> +config MESON_TSENSOR
> +	tristate "Amlogic Meson temperature sensor Support"
> +	default ARCH_MESON
> +	depends on OF && ARCH_MESON
> +	help
> +	  If you say yess here you get support for Meson Temperature sensor
> +	  for G12 SoC Family.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called meson_tsensor.
> +
>  config MLX90614
>  	tristate "MLX90614 contact-less infrared sensor"
>  	depends on I2C
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index baca4776ca0d..466d8c1c91d6 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -6,6 +6,7 @@
>  obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
>  obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
>  obj-$(CONFIG_MAX31856) += max31856.o
> +obj-$(CONFIG_MESON_TSENSOR) += meson_tsensor.o
>  obj-$(CONFIG_MLX90614) += mlx90614.o
>  obj-$(CONFIG_MLX90632) += mlx90632.o
>  obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/meson_tsensor.c b/drivers/iio/temperature/meson_tsensor.c
> new file mode 100644
> index 000000000000..be0a8d073ba3
> --- /dev/null
> +++ b/drivers/iio/temperature/meson_tsensor.c
> @@ -0,0 +1,416 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson Temperature Sensor
> + *
> + * Copyright (C) 2017 Huan Biao <huan.biao@amlogic.com>
> + * Copyright (C) 2019 Guillaume La Roque <glaroque@baylibre.com>
> + *
> + * Register value to celsius temperature formulas:
> + *	Read_Val	    m * U
> + * U = ---------, Uptat = ---------
> + *	2^16		  1 + n * U
> + *
> + * Temperature = A * ( Uptat + u_efuse / 2^16 )- B
> + *
> + *  A B m n : calibration parameters
> + *  u_efuse : fused calibration value, it's a signed 16 bits value
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/iio/iio.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define TSENSOR_CFG_REG1			0x4
> +	#define TSENSOR_CFG_REG1_RSET_VBG	BIT(12)
> +	#define TSENSOR_CFG_REG1_RSET_ADC	BIT(11)
> +	#define TSENSOR_CFG_REG1_VCM_EN		BIT(10)
> +	#define TSENSOR_CFG_REG1_VBG_EN		BIT(9)
> +	#define TSENSOR_CFG_REG1_OUT_CTL	BIT(6)
> +	#define TSENSOR_CFG_REG1_FILTER_EN	BIT(5)
> +	#define TSENSOR_CFG_REG1_DEM_EN		BIT(3)
> +	#define TSENSOR_CFG_REG1_CH_SEL		GENMASK(1, 0)
> +	#define TSENSOR_CFG_REG1_ENABLE		\
> +		(TSENSOR_CFG_REG1_FILTER_EN |	\
> +		 TSENSOR_CFG_REG1_VCM_EN |	\
> +		 TSENSOR_CFG_REG1_VBG_EN |	\
> +		 TSENSOR_CFG_REG1_DEM_EN |	\
> +		 TSENSOR_CFG_REG1_CH_SEL)
> +
> +#define TSENSOR_CFG_REG2				0x8
> +	#define TSENSOR_CFG_REG2_HITEMP_EN		BIT(31)
> +	#define TSENSOR_CFG_REG2_REBOOT_ALL_EN		BIT(30)
> +	#define TSENSOR_CFG_REG2_REBOOT_TIME		GENMASK(25, 16)
> +	#define TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE	\
> +		(TSENSOR_CFG_REG2_HITEMP_EN |		\
> +		 TSENSOR_CFG_REG2_REBOOT_ALL_EN |	\
> +		 TSENSOR_CFG_REG2_REBOOT_TIME)
> +	#define TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE_MASK		\
> +		(GENMASK(31, 30) | GENMASK(25, 4))
> +	#define TSENSOR_CFG_REG2_HITEMP_REBOOT_REG_MASK			\
> +		GENMASK(15, 4)
> +	#define TSENSOR_CFG_REG2_HITEMP_REG_VAL(_reg_val)		\
> +		(FIELD_PREP(TSENSOR_CFG_REG2_HITEMP_REBOOT_REG_MASK,	\
> +			    _reg_val) |					\
> +		 TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE)
> +
> +#define TSENSOR_CFG_REG3		0xC
> +#define TSENSOR_CFG_REG4		0x10
> +#define TSENSOR_CFG_REG5		0x14
> +#define TSENSOR_CFG_REG6		0x18
> +#define TSENSOR_CFG_REG7		0x1C
> +#define TSENSOR_CFG_REG8		0x20
> +
> +#define TSENSOR_STAT0			0x40
> +
> +#define TSENSOR_STAT9			0x64
> +
> +#define TSENSOR_READ_TEMP_MASK		GENMASK(15, 0)
> +#define TSENSOR_TEMP_MASK		GENMASK(11, 0)
> +
> +#define TSENSOR_TRIM_SIGN_MASK		BIT(15)
> +#define TSENSOR_TRIM_TEMP_MASK		GENMASK(14, 0)
> +#define TSENSOR_TRIM_VERSION_MASK	GENMASK(31, 24)
> +
> +#define TSENSOR_TRIM_VERSION(_version) 	\
> +	FIELD_GET(TSENSOR_TRIM_VERSION_MASK, _version)

I'd just use the FIELD_GET directly inline.  What it is doing
is kind of obvious from the parameter.

> +
> +#define TSENSOR_TRIM_CALIB_VALID_MASK	(GENMASK(3, 2) | BIT(7))
> +
> +#define TSENSOR_CALIB_OFFSET	1
> +#define TSENSOR_CALIB_SHIFT	4
> +
> +static const struct iio_chan_spec temperature_channel[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.channel = 0,
.channel doesn't do anything unless you set indexed.
> +		.address = 0,
.address not used...

> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +	},
> +};
> +
> +/**
> + * struct meson_tsensor_soc_data
> + * @A, B, m, n: calibration parameters
> + * This structure is required for configuration of meson tsensor driver.
> + */
> +struct meson_tsensor_soc_data {
> +	int A;
> +	int B;
> +	int m;
> +	int n;
> +};
> +
> +/**
> + * struct meson_tsensor_data
> + * @u_efuse_off: register offset to read fused calibration value
> + * @soc: calibration parameters structure pointer
> + * @regmap_config: regmap config for the device
> + * This structure is required for configuration of meson tsensor driver.
> + */
> +struct meson_tsensor_data {
> +	int u_efuse_off;
> +	const struct meson_tsensor_soc_data *soc;
> +	const struct regmap_config *regmap_config;
> +};
> +
> +struct meson_tsensor {
> +	int id;
> +	const struct meson_tsensor_data *data;
> +	struct regmap *regmap;
> +	struct regmap *sec_ao_map;
> +	struct clk *clk;
> +	u32 trim_info;
> +	void __iomem *base;
> +	int reboot_temp;
> +};
> +
> +/*
> + * tsensor treats temperature as a mapped temperature code.
> + * The temperature is converted differently depending on the calibration type.
> + */
> +static u32 temp_to_code(struct meson_tsensor *priv, int temp)
> +{
> +	const struct meson_tsensor_soc_data *param = priv->data->soc;
> +	s64 divisor, factor, uefuse;
> +	u32 reg_code;
> +
> +	uefuse = priv->trim_info & TSENSOR_TRIM_SIGN_MASK ?
> +			 ~(priv->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
> +			 (priv->trim_info & TSENSOR_TRIM_TEMP_MASK);
> +
> +	factor = BIT(16) * (temp * 10 + param->B);
> +	factor = div_s64(factor, param->A);
> +	factor = factor + uefuse;
> +
> +	factor = factor * 100;
> +
> +	divisor = param->n * factor;
> +	divisor = div_s64(divisor, BIT(16));
> +	divisor = param->m - divisor;
> +
> +	reg_code = div_s64(factor, divisor);
> +	reg_code = ((reg_code >> TSENSOR_CALIB_SHIFT) & TSENSOR_TEMP_MASK) +
> +		   TSENSOR_CALIB_OFFSET;
> +
> +	return reg_code;
> +}
> +
> +/*
> + * Calculate a temperature value from a temperature code.
> + * The unit of the temperature is degree Celsius.
> + */
> +static int code_to_temp(struct meson_tsensor *priv, int temp_code)
> +{
> +	const struct meson_tsensor_soc_data *param = priv->data->soc;
> +	int temp;
> +	s64 factor, Uptat, uefuse;
> +
> +	uefuse = priv->trim_info & TSENSOR_TRIM_SIGN_MASK ?
> +			     ~(priv->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
> +			     (priv->trim_info & TSENSOR_TRIM_TEMP_MASK);
Hmm. 2's complement from unsigned + a sign bit.

uefuse = priv->trim_info & TSENSOR_TRIM_TEMP_MASK;
if (priv->trim_info & TSENSOR_TRIM_SIGN_MASK)
	uefuse *= -1;

Is probably going to do the same thing whilst being more readable.

> +
> +	factor = param->n * temp_code;
> +	factor = div_s64(factor, 100);
> +
> +	Uptat = temp_code * param->m;
Why the capital U?  Not really kernel style.
> +	Uptat = div_s64(Uptat, 100);
> +	Uptat = Uptat * BIT(16);
> +	Uptat = div_s64(Uptat, BIT(16) + factor);
> +
> +	temp = (Uptat + uefuse) * param->A;
> +	temp = div_s64(temp, BIT(16));
> +	temp = (temp - param->B) * 100;
> +
> +	return temp;
> +}
> +
> +static int meson_tsensor_initialize(struct iio_dev *indio_dev)
> +{
> +	struct meson_tsensor *priv = iio_priv(indio_dev);
> +	u32 reg_val;
> +	int ret = 0;
> +	int ver;
> +
> +	regmap_read(priv->sec_ao_map, priv->data->u_efuse_off,
> +		    &priv->trim_info);
> +
> +	ver = TSENSOR_TRIM_VERSION(priv->trim_info);
> +
> +	if ((ver & TSENSOR_TRIM_CALIB_VALID_MASK) == 0) {
> +		ret = -EINVAL;
> +		dev_err(&indio_dev->dev,
> +			"tsensor thermal calibration not supported: 0x%x!\n",
> +			ver);
> +		goto out;
return -EINVAL;
> +	}
> +
> +	/* init the ts reboot soc function */
> +	if (priv->reboot_temp) {
> +		/* register need value in celsius */
> +		reg_val = temp_to_code(priv, priv->reboot_temp / 1000);
> +		regmap_update_bits(priv->regmap, TSENSOR_CFG_REG2,
> +				   TSENSOR_CFG_REG2_HITEMP_REBOOT_ENABLE_MASK,
> +				   TSENSOR_CFG_REG2_HITEMP_REG_VAL(reg_val));
> +	}
> +
return 0;
> +out:
> +	return ret;
> +}
> +
> +static int meson_tsensor_enable(struct iio_dev *indio_dev)
> +{
> +	struct meson_tsensor *priv = iio_priv(indio_dev);
> +
> +	clk_prepare_enable(priv->clk);
> +	regmap_update_bits(priv->regmap, TSENSOR_CFG_REG1,
> +			   TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
> +
> +	return 0;
Given neither this nor the follow up have any error paths,
stop them having a return value and you can drop some error handling..

> +}
> +
> +static int meson_tsensor_disable(struct iio_dev *indio_dev)
> +{
> +	struct meson_tsensor *priv = iio_priv(indio_dev);
> +
> +	regmap_update_bits(priv->regmap, TSENSOR_CFG_REG1,
> +			   TSENSOR_CFG_REG1_ENABLE, 0);
> +	clk_disable(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int meson_tsensor_read(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)
> +{
> +	unsigned int tvalue;
> +	struct meson_tsensor *priv = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		regmap_read(priv->regmap, TSENSOR_STAT0, &tvalue);
> +		*val = code_to_temp(priv,
> +				    tvalue & TSENSOR_READ_TEMP_MASK);
Looks to me like this will fit on one line under 80 chars.

> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info meson_tsensor_iio_info = {
> +	.read_raw = &meson_tsensor_read,
> +};
> +
> +static const struct regmap_config meson_tsensor_regmap_config_g12a = {
> +	.reg_bits = 8,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = TSENSOR_STAT9,
> +};
> +
> +static const struct meson_tsensor_soc_data meson_tsensor_g12a = {
> +	.A = 9411,
> +	.B = 3159,
> +	.m = 424,
> +	.n = 324,
> +};
> +
> +static const struct meson_tsensor_data meson_tsensor_g12a_cpu_param = {
> +	.u_efuse_off = 0x128,
> +	.soc = &meson_tsensor_g12a,
> +	.regmap_config = &meson_tsensor_regmap_config_g12a,
> +};
> +
> +static const struct meson_tsensor_data meson_tsensor_g12a_ddr_param = {
> +	.u_efuse_off = 0xF0,
> +	.soc = &meson_tsensor_g12a,
> +	.regmap_config = &meson_tsensor_regmap_config_g12a,
I'm going to guess there is support for other devices on it's way where
the regmap_config and soc are different?

If it is going to be a while I'd prefer you moved them into this param
structure only when you need to.

> +};
> +
> +static const struct of_device_id meson_tsensor_of_match[] = {
> +	{
> +		.compatible = "amlogic,meson-g12a-ddr-tsensor",

Given we don't do a match on the compatible without the type specified
g12a-tsensor, is there any reason to have that in the binding?

> +		.data = &meson_tsensor_g12a_ddr_param,
> +	},
> +	{
> +		.compatible = "amlogic,meson-g12a-cpu-tsensor",
> +		.data = &meson_tsensor_g12a_cpu_param,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, meson_tsensor_of_match);
> +
> +static int meson_tsensor_probe(struct platform_device *pdev)
> +{
> +	struct meson_tsensor *priv;
> +	struct iio_dev *indio_dev;
> +	struct resource *res;
> +
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv = iio_priv(indio_dev);
> +	priv->data = of_device_get_match_data(&pdev->dev);
> +	if (!priv->data) {
> +		dev_err(&pdev->dev, "failed to get match data\n");
> +		return -ENODEV;
> +	}
> +
> +	indio_dev->channels = temperature_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(temperature_channel);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &meson_tsensor_iio_info;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->regmap = devm_regmap_init_mmio(&pdev->dev, priv->base,
> +					     priv->data->regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		if (PTR_ERR(priv->clk) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to get clock\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	if (of_property_read_u32(pdev->dev.of_node,
> +				 "amlogic,critical-temperature",
> +				 &priv->reboot_temp)) {
> +		priv->reboot_temp = 0;
> +	}
> +
> +	priv->sec_ao_map = syscon_regmap_lookup_by_phandle
> +		(pdev->dev.of_node, "amlogic,ao-secure");
> +	if (IS_ERR(priv->sec_ao_map)) {
> +		dev_err(&pdev->dev, "syscon regmap lookup failed.\n");
> +		return PTR_ERR(priv->sec_ao_map);
> +	}
> +
> +	ret = meson_tsensor_initialize(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = meson_tsensor_enable(indio_dev);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err_hw;
> +
> +	return 0;
> +
> +err_hw:
> +	meson_tsensor_disable(indio_dev);
> +err:
> +	clk_unprepare(priv->clk);
> +
> +	return ret;
> +}
> +
> +static int meson_tsensor_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return meson_tsensor_disable(indio_dev);

Is there a missing clk_unprepare in here?
One option to think about is whether to use devm_add_action_or_reset
to move all cleanup into the device managed queue.  That way you can
drop the error handling in probe and get rid of remove entirely.

> +}
> +
> +static struct platform_driver meson_tsensor_driver = {
> +	.probe = meson_tsensor_probe,
> +	.remove = meson_tsensor_remove,
> +	.driver = {
> +			.name = "meson-tsensor",
> +			.of_match_table = meson_tsensor_of_match,
> +		},
> +};
> +
> +module_platform_driver(meson_tsensor_driver);
> +
> +MODULE_AUTHOR("Guillaume La Roque <glaroque@baylibre.com>");
> +MODULE_DESCRIPTION("Amlogic Meson Temperature Sensor Driver");
> +MODULE_LICENSE("GPL v2");


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic