Re: [RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

From: Lee Jones
Date: Wed Mar 29 2017 - 03:54:23 EST

Next message: Yisheng Xie: "Re: [RFC]mm/zsmalloc,: trigger BUG_ON in function zs_map_object."
Previous message: Mars Cheng: "Re: [PATCH v3 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement"
Next in thread: Maxime Ripard: "Re: [RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 29 Mar 2017, Icenowy Zheng wrote:

>
> 2017å3æ29æ 14:54ä Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx>åéï
> >
> > Hi,
> >
> > On 28/03/2017 19:30, Icenowy Zheng wrote:
> > > This adds support for the Allwinner H3 thermal sensor.
> > >
> > > Allwinner H3 has a thermal sensor like the one in A33, but have its
> > > registers nearly all re-arranged, sample clock moved to CCU and a pair
> > > of bus clock and reset added. It's also the base of newer SoCs' thermal
> > > sensors.
> > >
> > > An option is added to gpadc_data struct, to indicate whether this device
> > > is a new-generation Allwinner thermal sensor.
> > >
> > > The thermal sensors on A64 and H5 is like the one on H3, but with of
> > > course different formula factors.
> > >
> > > Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
> > > ---
> > >Â drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------
> > >Â include/linux/mfd/sun4i-gpadc.hÂÂ |Â 33 +++++++++-
> > >Â 2 files changed, 141 insertions(+), 22 deletions(-)

How have you managed to reply un-threaded?

Please ensure you mailer attaches your reply to the rest of the
thread, or things will become very confusing, very quickly.

> > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> > > index 74705aa37982..7512b1cae877 100644
> > > --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> > > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> > > @@ -22,6 +22,7 @@
> > >ÂÂ * shutdown for not being used.
> > >ÂÂ */
> > >Â
> > > +#include <linux/clk.h>
> > >Â #include <linux/completion.h>
> > >Â #include <linux/interrupt.h>
> > >Â #include <linux/io.h>
> > > @@ -31,6 +32,7 @@
> > >Â #include <linux/platform_device.h>
> > >Â #include <linux/pm_runtime.h>
> > >Â #include <linux/regmap.h>
> > > +#include <linux/reset.h>
> > >Â #include <linux/thermal.h>
> > >Â #include <linux/delay.h>
> > >Â
> > > @@ -56,6 +58,7 @@ struct gpadc_data {
> > >Â unsigned int tp_adc_select;
> > >Â unsigned int (*adc_chan_select)(unsigned int chan);
> > >Â unsigned int adc_chan_mask;
> > > + bool gen2_ths;
> > >Â };
> > >Â
> >
> > Instead of a boolean, give the TEMP_DATA register address.
> >
> > >Â static const struct gpadc_data sun4i_gpadc_data = {
> > > @@ -88,7 +91,20 @@ static const struct gpadc_data sun6i_gpadc_data = {
> > >Â static const struct gpadc_data sun8i_a33_gpadc_data = {
> > >Â .temp_offset = -1662,
> > >Â .temp_scale = 162,
> > > - .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
> > > + .tp_mode_en = SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN,
> > > +};
> >
> > Separate patch for this?
> >
> > > +
> > > +static const struct gpadc_data sun8i_h3_gpadc_data = {
> > > + /*
> > > + * The original formula on the datasheet seems to be wrong.
> > > + * These factors are calculated based on the formula in the BSP
> > > + * kernel, which is originally Tem = 217 - (T / 8.253), in which Tem
> > > + * is the temperature in Celsius degree and T is the raw value
> > > + * from the sensor.
> > > + */
> > > + .temp_offset = -1791,
> > > + .temp_scale = -121,
> > > + .gen2_ths = true,
> > >Â };
> > >Â
> > >Â struct sun4i_gpadc_iio {
> > > @@ -103,6 +119,9 @@ struct sun4i_gpadc_iio {
> > >Â atomic_t ignore_temp_data_irq;
> > >Â const struct gpadc_data *data;
> > >Â bool no_irq;
> > > + struct clk *ths_bus_clk;
> > > + struct clk *ths_clk;
> > > + struct reset_control *reset;
> > >Â /* prevents concurrent reads of temperature and ADC */
> > >Â struct mutex mutex;
> > >Â };
> > > @@ -274,7 +293,11 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
> > >Â if (info->no_irq) {
> > >Â pm_runtime_get_sync(indio_dev->dev.parent);
> > >Â
> > > - regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
> > > + if (info->data->gen2_ths)
> > > + regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA,
> > > + ÂÂÂ val);
> > > + else
> > > + regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
> > >Â
> >
> > Instead of gen2_ths, use the TEMP_DATA register address.
> >
> > >Â pm_runtime_mark_last_busy(indio_dev->dev.parent);
> > >Â pm_runtime_put_autosuspend(indio_dev->dev.parent);
> > > @@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
> > >Â {
> > >Â struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
> > >Â
> > > - /* Disable the ADC on IP */
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
> > > - /* Disable temperature sensor on IP */
> > > - regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
> > > + if (info->data->gen2_ths) {
> > > + /* Disable temperature sensor */
> > > + regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
> > > + } else {
> > > + /* Disable the ADC on IP */
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
> > > + /* Disable temperature sensor on IP */
> > > + regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
> > > + }
> > >Â
> >
> > Either use another register address or add a suspend function to struct
> > gpadc_data which will be different for each version of the IP.
> >
> > >Â return 0;
> > >Â }
> > > @@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
> > >Â {
> > >Â struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
> > >Â
> > > - /* clkin = 6MHz */
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
> > > - ÂÂÂÂ SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
> > > - ÂÂÂÂ SUN4I_GPADC_CTRL0_FS_DIV(7) |
> > > - ÂÂÂÂ SUN4I_GPADC_CTRL0_T_ACQ(63));
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en);
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
> > > - ÂÂÂÂ SUN4I_GPADC_CTRL3_FILTER_EN |
> > > - ÂÂÂÂ SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
> > > - /* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */
> > > - regmap_write(info->regmap, SUN4I_GPADC_TPR,
> > > - ÂÂÂÂ SUN4I_GPADC_TPR_TEMP_ENABLE |
> > > - ÂÂÂÂ SUN4I_GPADC_TPR_TEMP_PERIOD(800));
> > > + if (info->data->gen2_ths) {
> > > + regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2,
> > > + ÂÂÂÂ SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN |
> > > + ÂÂÂÂ SUN8I_H3_GPADC_CTRL2_T_ACQ1(31));
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL0_T_ACQ(31));
> > > + regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL3,
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL3_FILTER_EN |
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
> > > + regmap_write(info->regmap, SUN8I_H3_GPADC_INTC,
> > > + ÂÂÂÂ SUN8I_H3_GPADC_INTC_TEMP_PERIOD(800));
> > > + } else {
> > > + /* clkin = 6MHz */
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL0_FS_DIV(7) |
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL0_T_ACQ(63));
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL1,
> > > + ÂÂÂÂ info->data->tp_mode_en);
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL3_FILTER_EN |
> > > + ÂÂÂÂ SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
> > > + /*
> > > + * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin;
> > > + * ~0.6s
> > > + */
> > > + regmap_write(info->regmap, SUN4I_GPADC_TPR,
> > > + ÂÂÂÂ SUN4I_GPADC_TPR_TEMP_ENABLE |
> > > + ÂÂÂÂ SUN4I_GPADC_TPR_TEMP_PERIOD(800));
> > > + }
> > >Â
> >
> > Same here as suspend function?
> >
> > >Â return 0;
> > >Â }
> > > @@ -494,6 +539,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_gpadc_data,
> > > + },
> > >Â { /* sentinel */ }
> > >Â };
> > >Â
> > > @@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
> > >Â return ret;
> > >Â }
> > >Â
> > > + if (info->data->gen2_ths) {
> > > + info->reset = devm_reset_control_get(&pdev->dev, NULL);
> > > + if (IS_ERR(info->reset)) {
> > > + ret = PTR_ERR(info->reset);
> > > + return ret;
> > > + }
> > > +
> > > + ret = reset_control_deassert(info->reset);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus");
> > > + if (IS_ERR(info->ths_bus_clk)) {
> > > + ret = PTR_ERR(info->ths_bus_clk);
> > > + return ret;
> > > + }
> > > +
> > > + ret = clk_prepare_enable(info->ths_bus_clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + info->ths_clk = devm_clk_get(&pdev->dev, "ths");
> > > + if (IS_ERR(info->ths_clk)) {
> > > + ret = PTR_ERR(info->ths_clk);
> > > + return ret;
> > > + }
> > > +
> > > + /* Running at 6MHz */
> > > + ret = clk_set_rate(info->ths_clk, 6000000);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = clk_prepare_enable(info->ths_clk);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > >Â if (!IS_ENABLED(CONFIG_THERMAL_OF))
> > >Â return 0;
> > >Â
> > > @@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
> > >Â if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
> > >Â iio_map_array_unregister(indio_dev);
> > >Â
> > > + if (info->data->gen2_ths) {
> > > + clk_disable_unprepare(info->ths_clk);
> > > + clk_disable_unprepare(info->ths_bus_clk);
> > > + reset_control_deassert(info->reset);
> > > + }
> > > +
> >
> > I'm not really fond of using this boolean as I don't see it being
> > possibly reused for any other SoCs that has a GPADC or THS.
>
> Because you didn't care new SoCs :-)
>
> All SoCs after H3 (A64, H5, R40) uses the same THS architecture with H3.
>
> >
> > Here, you could make use of a list/array of clk which then can be reused
> > for other SoCs just by changing the list. Add a default rate to the
> > gpadc_data structure and you're go to go.
> >
> > >Â return 0;
> > >Â }
> > >Â
> > > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> > > index 139872c2e0fe..f794a2988a93 100644
> > > --- a/include/linux/mfd/sun4i-gpadc.h
> > > +++ b/include/linux/mfd/sun4i-gpadc.h
> > > @@ -38,9 +38,12 @@
> > >Â #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x))
> > >Â #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK GENMASK(3, 0)
> > >Â
> > > -/* TP_CTRL1 bits for sun8i SoCs */
> > > -#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
> > > -#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
> > > +/* TP_CTRL1 bits for sun8i A23/A33 SoCs */
> > > +#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
> > > +#define SUN8I_A23_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
> > > +
> >
> > Different patch for these?
> >
> > Thanks,
> > Quentin
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Next message: Yisheng Xie: "Re: [RFC]mm/zsmalloc,: trigger BUG_ON in function zs_map_object."
Previous message: Mars Cheng: "Re: [PATCH v3 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement"
Next in thread: Maxime Ripard: "Re: [RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]