Hi Peter,
thank you for taking the time to review my patch!
On Sun, Oct 28, 2018 at 5:19 PM Peter Meerwald-Stadler
<pmeerw@xxxxxxxxxx> wrote:
>
> On Sun, 28 Oct 2018, Martin Blumenstingl wrote:
>
> comments below
>
> > Channel 6 of the SAR ADC can be switched between two inputs:
> > SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> > temperature sensor inside the SoC.
> >
> > To get usable results from the temperature sensor we need to read the
> > corresponding calibration data from the eFuse and pass it to the SAR ADC
> > (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> > HHI region.
> > If the temperature sensor is not calibrated (the eFuse data contains a
> > bit for this) then the driver will return -ENOTSUPP when trying to read
> > the temperature.
> >
> > This only enables the temperature sensor for the Meson8, Meson8b and
> > Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> > temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> > can simply use the SCPI hwmon driver to get the chip temperature).
> >
> > To keep the devicetree interface backwards compatible we simply skip the
> > temperature sensor initialization if no eFuse nvmem cell is passed via
> > devicetree.
> >
> > The public documentation for the SAR ADC IP block does not explain how
> > to use the registers to read the temperature. The logic from this patch
> > is based on reading and understanding Amlogic's GPL kernel sources.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> > ---
> > drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
> > 1 file changed, 248 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > index 028ccd218f82..df1e45805c23 100644
> > --- a/drivers/iio/adc/meson_saradc.c
> > +++ b/drivers/iio/adc/meson_saradc.c
> > @@ -18,6 +18,7 @@
> > #include <linux/io.h>
> > #include <linux/iio/iio.h>
> > #include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > #include <linux/interrupt.h>
> > #include <linux/of.h>
> > #include <linux/of_irq.h>
> > @@ -25,6 +26,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/mfd/syscon.h>
> >
> > #define MESON_SAR_ADC_REG0 0x00
> > #define MESON_SAR_ADC_REG0_PANEL_DETECT BIT(31)
> > @@ -165,6 +167,17 @@
> >
> > #define MESON_SAR_ADC_MAX_FIFO_SIZE 32
> > #define MESON_SAR_ADC_TIMEOUT 100 /* ms */
> > +#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL 6
> > +#define MESON_SAR_ADC_TEMP_OFFSET 27
> > +
> > +/* temperature sensor calibration information in eFuse */
> > +#define MESON_SAR_ADC_EFUSE_BYTES 4
> > +#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL GENMASK(6, 0)
> > +#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED BIT(7)
>
> MESON_SAR_ADC_ prefix for consistency?
I missed that, thanks
> > +
> > +#define MESON_HHI_DPLL_TOP_0 0x318
> > +#define MESON_HHI_DPLL_TOP_0_TSC_BIT4 BIT(9)
>
> I guess these do not belog to the SAR ADC at all?
yes and no
these register are part of the HHI register region which is shared the:
- clock controller
- HDMI controller
- IIRC the HDMI PHY as well
- one TSC bit for the SAR ADC
- ...possibly more
currently there's no header file for all the HHI includes because (as
far as I'm aware) each register is only used by one IP block
> > +
> > /* for use with IIO_VAL_INT_PLUS_MICRO */
> > #define MILLION 1000000
> >
> > @@ -175,16 +188,27 @@
> > .address = _chan, \
> > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > BIT(IIO_CHAN_INFO_AVERAGE_RAW), \
> > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> > - BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> > BIT(IIO_CHAN_INFO_CALIBSCALE), \
> > .datasheet_name = "SAR_ADC_CH"#_chan, \
> > }
> >
> > -/*
> > - * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
> > - * currently not supported by this driver.
> > - */
> > +#define MESON_SAR_ADC_TEMP_CHAN(_chan) { \
> > + .type = IIO_TEMP, \
> > + .indexed = 0, \
>
> .indexed = 0 not needed
OK, I will drop this
> > + .channel = _chan, \
> > + .address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_AVERAGE_RAW), \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_ENABLE), \
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> > + BIT(IIO_CHAN_INFO_CALIBSCALE), \
> > + .datasheet_name = "TEMP_SENSOR", \
> > +}
> > +
> > static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> > MESON_SAR_ADC_CHAN(0),
> > MESON_SAR_ADC_CHAN(1),
> > @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> > IIO_CHAN_SOFT_TIMESTAMP(8),
> > };
> >
> > +static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
> > + MESON_SAR_ADC_CHAN(0),
> > + MESON_SAR_ADC_CHAN(1),
> > + MESON_SAR_ADC_CHAN(2),
> > + MESON_SAR_ADC_CHAN(3),
> > + MESON_SAR_ADC_CHAN(4),
> > + MESON_SAR_ADC_CHAN(5),
> > + MESON_SAR_ADC_CHAN(6),
> > + MESON_SAR_ADC_CHAN(7),
> > + MESON_SAR_ADC_TEMP_CHAN(8),
> > + IIO_CHAN_SOFT_TIMESTAMP(9),
> > +};
> > +
> > enum meson_sar_adc_avg_mode {
> > NO_AVERAGING = 0x0,
> > MEAN_AVERAGING = 0x1,
> > @@ -225,6 +262,11 @@ struct meson_sar_adc_param {
> > u32 bandgap_reg;
> > unsigned int resolution;
> > const struct regmap_config *regmap_config;
> > + u8 temperature_trimming_bits;
> > + unsigned int temperature_multiplier;
> > + unsigned int temperature_divider;
> > + const struct iio_chan_spec *channels;
> > + unsigned int num_channels;
> > };
> >
> > struct meson_sar_adc_data {
> > @@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
> > struct completion done;
> > int calibbias;
> > int calibscale;
> > + struct regmap *tsc_regmap;
> > + bool temperature_sensor_calibrated;
> > + u8 temperature_sensor_coefficient;
> > + u16 temperature_sensor_adc_val;
> > };
> >
> > static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
> > @@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
> > MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
> > regval);
> >
> > - if (chan->address == 6)
> > - regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> > - MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> > + if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {
>
> quite repetitive, maybe (just a suggestion)
> u8 val = chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;
> regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> MESON_SAR_ADC_DELTA_10_TEMP_SEL, val);
thanks for the suggestion!
I already have a "regval" variable which we can use here
I'll simplify this in the next version - I'll keep the basic if/else
though instead of using a ternary operator (because "regval =
chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;" would
exceed the line length limit of 80 chars)
> > + if (chan->type == IIO_TEMP)
> > + regmap_update_bits(priv->regmap,
> > + MESON_SAR_ADC_DELTA_10,
> > + MESON_SAR_ADC_DELTA_10_TEMP_SEL,
> > + MESON_SAR_ADC_DELTA_10_TEMP_SEL);
> > + else
> > + regmap_update_bits(priv->regmap,
> > + MESON_SAR_ADC_DELTA_10,
> > + MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> > + }
> > }
[snip]
Regards
Martin
