On 31.01.2018 20:23, Quentin Schulz wrote:
Hi Philipp,

On Mon, Jan 29, 2018 at 12:29:12AM +0100, Philipp Rossak wrote:
This patch adds support for the H3 ths sensor.

The H3 supports interrupts. The interrupt is configured to update the
the sensor values every second. The calibration data is writen at the
begin of the init process.

Signed-off-by: Philipp Rossak <embe...@gmail.com>
---
  drivers/iio/adc/sun4i-gpadc-iio.c | 86 +++++++++++++++++++++++++++++++++++++++
  include/linux/mfd/sun4i-gpadc.h   | 22 ++++++++++
  2 files changed, 108 insertions(+)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
b/drivers/iio/adc/sun4i-gpadc-iio.c
index b7b5451226b0..8196203d65fe 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -61,6 +61,9 @@ struct sun4i_gpadc_iio;
  static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info);
  static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info);
+static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info);
+static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info);
+

We try to avoid using the generic sunxi prefix.

  struct gpadc_data {
        int             temp_offset;
        int             temp_scale;
@@ -71,6 +74,10 @@ struct gpadc_data {
        unsigned int    temp_data[MAX_SENSOR_COUNT];
        int             (*sample_start)(struct sun4i_gpadc_iio *info);
        int             (*sample_end)(struct sun4i_gpadc_iio *info);
+       u32             ctrl0_map;
+       u32             ctrl2_map;
+       u32             sensor_en_map;
+       u32             filter_map;
        u32             irq_clear_map;
        u32             irq_control_map;
        bool            has_bus_clk;
@@ -138,6 +145,31 @@ static const struct gpadc_data sun8i_a33_gpadc_data = {
        .support_irq = false,
  };
+static const struct gpadc_data sun8i_h3_ths_data = {
+       .temp_offset = -1791,
+       .temp_scale = -121,
+       .temp_data = {SUN8I_H3_THS_TDATA0, 0, 0, 0},
+       .sample_start = sunxi_ths_sample_start,
+       .sample_end = sunxi_ths_sample_end,
+       .has_bus_clk = true,
+       .has_bus_rst = true,
+       .has_mod_clk = true,
+       .sensor_count = 1,
+       .supports_nvmem = true,
+       .support_irq = true,
+       .ctrl0_map = SUN4I_GPADC_CTRL0_T_ACQ(0xff),
+       .ctrl2_map = SUN8I_H3_THS_ACQ1(0x3f),
+       .sensor_en_map = SUN8I_H3_THS_TEMP_SENSE_EN0,
+       .filter_map = SUN4I_GPADC_CTRL3_FILTER_EN |
+               SUN4I_GPADC_CTRL3_FILTER_TYPE(0x2),
+       .irq_clear_map = SUN8I_H3_THS_INTS_ALARM_INT_0 |
+                       SUN8I_H3_THS_INTS_SHUT_INT_0   |
+                       SUN8I_H3_THS_INTS_TDATA_IRQ_0  |
+                       SUN8I_H3_THS_INTS_ALARM_OFF_0,
+       .irq_control_map = SUN8I_H3_THS_INTC_TDATA_IRQ_EN0 |
+               SUN8I_H3_THS_TEMP_PERIOD(0x7),

 From what I've understood, ACQ regs are basically clock dividers. We
should make a better job at explaining it :)


I agree, I will add this in the next version in the commit message.

+};
+
  struct sun4i_gpadc_iio {
        struct iio_dev                  *indio_dev;
        struct completion               completion;
@@ -462,6 +494,16 @@ static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio 
*info)
        return 0;
  }
+static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info)
+{
+       /* Disable ths interrupt */
+       regmap_write(info->regmap, SUN8I_H3_THS_INTC, 0x0);
+       /* Disable temperature sensor */
+       regmap_write(info->regmap, SUN8I_H3_THS_CTRL2, 0x0);
+
+       return 0;
+}
+
  static int sun4i_gpadc_runtime_suspend(struct device *dev)
  {
        struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
@@ -473,6 +515,17 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
        return info->data->sample_end(info);
  }
+static void sunxi_calibrate(struct sun4i_gpadc_iio *info)
+{
+       if (info->has_calibration_data[0])
+               regmap_write(info->regmap, SUNXI_THS_CDATA_0_1,
+                       info->calibration_data[0]);
+
+       if (info->has_calibration_data[1])
+               regmap_write(info->regmap, SUNXI_THS_CDATA_2_3,
+                       info->calibration_data[1]);
+}
+
  static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info)
  {
        /* clkin = 6MHz */
@@ -492,6 +545,35 @@ static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio 
*info)
        return 0;
  }
+static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info)
+{
+       u32 value;
+       sunxi_calibrate(info);
+
+       if (info->data->ctrl0_map)
+               regmap_write(info->regmap, SUN8I_H3_THS_CTRL0,
+                       info->data->ctrl0_map);
+
+       regmap_write(info->regmap, SUN8I_H3_THS_CTRL2,
+               info->data->ctrl2_map);
+
+       regmap_write(info->regmap, SUN8I_H3_THS_STAT,
+                       info->data->irq_clear_map);
+
+       regmap_write(info->regmap, SUN8I_H3_THS_FILTER,
+               info->data->filter_map);
+
+       regmap_write(info->regmap, SUN8I_H3_THS_INTC,
+               info->data->irq_control_map);
+
+       regmap_read(info->regmap, SUN8I_H3_THS_CTRL2, &value);
+
+       regmap_write(info->regmap, SUN8I_H3_THS_CTRL2,
+               info->data->sensor_en_map | value);
+
+       return 0;
+}
+

I'm a bit worried. Will all the sensors have the same sample start? Or
will we need at some point also entries in info->data for register
addresses, if they have ctrl2 or filter, etc...
 > Maybe we could define a sample_start for the h3 only and reuse-it if
possible and if not declare another sample start? All depends if the
sample start is most likely to change in the near future or not. If it
is, then we should avoid adding a lot of variables in info->data and
just go for a single sample_start per SoC.


for H3, A83T, H5, A64 we can use the same sample start. So I would use here a H3 sample start function and reuse it.

A80 is special since it has no crtl0 register, thus it would get also its own function.

H6 will need also an own sample start function (looking in the near future).

  static int sun4i_gpadc_runtime_resume(struct device *dev)
  {
        struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
@@ -582,6 +664,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = {
                .compatible = "allwinner,sun8i-a33-ths",
                .data = &sun8i_a33_gpadc_data,
        },
+       {
+               .compatible = "allwinner,sun8i-h3-ths",
+               .data = &sun8i_h3_ths_data,
+       },
        { /* sentinel */ }
  };
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 458f2159a95a..80b79c31cea3 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -91,7 +91,29 @@
  #define SUN4I_GPADC_AUTOSUSPEND_DELAY                 10000
/* SUNXI_THS COMMON REGISTERS + DEFINES */
+#define SUN8I_H3_THS_CTRL0                             0x00
+#define SUN8I_H3_THS_CTRL2                             0x40
  #define SUN8I_H3_THS_INTC                             0x44
+#define SUN8I_H3_THS_STAT                              0x48
+#define SUN8I_H3_THS_FILTER                            0x70
+#define SUNXI_THS_CDATA_0_1                            0x74
+#define SUNXI_THS_CDATA_2_3                            0x78
+#define SUN8I_H3_THS_TDATA0                            0x80
+
+#define SUN8I_H3_THS_ACQ1(x)                           (GENMASK(31, 16) & ((x) 
<< 16))
+
+#define SUN8I_H3_THS_TEMP_SENSE_EN0                    BIT(0)
+
+#define SUN8I_H3_THS_TEMP_PERIOD(x)                    (GENMASK(31, 12) & ((x) 
<< 12))
+
+#define SUN8I_H3_THS_INTS_ALARM_INT_0                  BIT(0)
+#define SUN8I_H3_THS_INTS_SHUT_INT_0                   BIT(4)
+#define SUN8I_H3_THS_INTS_TDATA_IRQ_0                  BIT(8)
+#define SUN8I_H3_THS_INTS_ALARM_OFF_0                  BIT(12)
+
+#define SUN8I_H3_THS_INTC_ALARM_INT_EN0                        BIT(0)
+#define SUN8I_H3_THS_INTC_SHUT_INT_EN0                 BIT(4)
+#define SUN8I_H3_THS_INTC_TDATA_IRQ_EN0                        BIT(8)

Personal taste here but I prefer the register bits to be defined just
below the register address define.

i.e.:

#define SUN8I_H3_THS_CTRL2                           0x40
#define SUN8I_H3_THS_ACQ1(x)                         (GENMASK(31, 16) & ((x) << 
16))
#define SUN8I_H3_THS_TEMP_SENSE_EN0                  BIT(0)

that way we know for which register we should use which constants.

Quentin


I agree, this the better way to do it.


Thanks,
Philipp

Reply via email to