Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190604144714.2009-1-glaroque@baylibre.com> <20190604144714.2009-2-glaroque@baylibre.com>
In-Reply-To: <20190604144714.2009-2-glaroque@baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Thu, 6 Jun 2019 21:16:37 +0200
Message-ID: <CAFBinCBN4QC2tPDEQmTW_c+PP5yu2qoK5M1eSye=SmvpieKWQg@mail.gmail.com>
Subject: Re: [PATCH 1/3] Documentation: dt-bindings: add the Amlogic Meson
 Temperature Sensor
To:     Guillaume La Roque <glaroque@baylibre.com>
Cc:     jic23@kernel.org, khilman@baylibre.com, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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