lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 29 Mar 2017 08:54:05 +0200
From:   Quentin Schulz <quentin.schulz@...e-electrons.com>
To:     Icenowy Zheng <icenowy@...c.io>, Lee Jones <lee.jones@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3
 thermal sensor

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@...c.io>
> ---
>  drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------
>  include/linux/mfd/sun4i-gpadc.h   |  33 +++++++++-
>  2 files changed, 141 insertions(+), 22 deletions(-)
> 
> 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.

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
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Powered by blists - more mailing lists