diff mbox

[v2,08/11] iio: adc: sun4i-gpadc-iio: add support for A33 thermal sensor

Message ID 20170310103921.19469-9-quentin.schulz@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quentin Schulz March 10, 2017, 10:39 a.m. UTC
This adds support for the Allwinner A33 thermal sensor.

Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
which is dedicated to the thermal sensor. Moreover, its thermal sensor
does not generate interruptions, thus we only need to directly read the
register storing the temperature value.

The MFD used by the A10, A13 and A31, was created to avoid breaking the
DT binding, but since the nodes for the ADC weren't there for the A33,
it is not needed.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v2:
  - removed added comments in Kconfig,
  - simplified Kconfig depends on condition,
  - removed THERMAL_OF requirement for sun8i,
  - renamed sun8i_gpadc_channels to sun8i_a33_gpadc_channels,
  - renamed use_dt boolean in no_irq as it reflects better why we need it,
  - removed spurious/unneeded modifications done in v1,

 drivers/iio/adc/Kconfig           |   2 +-
 drivers/iio/adc/sun4i-gpadc-iio.c | 100 ++++++++++++++++++++++++++++++++++++--
 include/linux/mfd/sun4i-gpadc.h   |   4 ++
 3 files changed, 102 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron March 13, 2017, 9:06 p.m. UTC | #1
On 10/03/17 10:39, Quentin Schulz wrote:
> This adds support for the Allwinner A33 thermal sensor.
> 
> Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
> which is dedicated to the thermal sensor. Moreover, its thermal sensor
> does not generate interruptions, thus we only need to directly read the
> register storing the temperature value.
> 
> The MFD used by the A10, A13 and A31, was created to avoid breaking the
> DT binding, but since the nodes for the ADC weren't there for the A33,
> it is not needed.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Talk me through why it makes sense to do this rather than simply spin out
a really simple thermal driver for the A33?

I'm not against what you have here, but don't feel it has been fully argued.

Jonathan
> ---
> 
> v2:
>   - removed added comments in Kconfig,
>   - simplified Kconfig depends on condition,
>   - removed THERMAL_OF requirement for sun8i,
>   - renamed sun8i_gpadc_channels to sun8i_a33_gpadc_channels,
>   - renamed use_dt boolean in no_irq as it reflects better why we need it,
>   - removed spurious/unneeded modifications done in v1,
> 
>  drivers/iio/adc/Kconfig           |   2 +-
>  drivers/iio/adc/sun4i-gpadc-iio.c | 100 ++++++++++++++++++++++++++++++++++++--
>  include/linux/mfd/sun4i-gpadc.h   |   4 ++
>  3 files changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9f8b4b1..8c8ead6 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -562,7 +562,7 @@ config STX104
>  config SUN4I_GPADC
>  	tristate "Support for the Allwinner SoCs GPADC"
>  	depends on IIO
> -	depends on MFD_SUN4I_GPADC
> +	depends on MFD_SUN4I_GPADC || MACH_SUN8I
>  	help
>  	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>  	  GPADC. This ADC provides 4 channels which can be used as an ADC or as
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index 7cb997a..70684cd 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
>  	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>  };
>  
> +static const struct gpadc_data sun8i_a33_gpadc_data = {
> +	.temp_offset = -1662,
> +	.temp_scale = 162,
> +	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
> +};
> +
>  struct sun4i_gpadc_iio {
>  	struct iio_dev			*indio_dev;
>  	struct completion		completion;
> @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
>  	unsigned int			temp_data_irq;
>  	atomic_t			ignore_temp_data_irq;
>  	const struct gpadc_data		*data;
> +	bool				no_irq;
>  	/* prevents concurrent reads of temperature and ADC */
>  	struct mutex			mutex;
>  };
> @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
>  	SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>  };
>  
> +static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.datasheet_name = "temp_adc",
> +	},
> +};
> +
> +static const struct regmap_config sun4i_gpadc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
>  static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
>  				 unsigned int irq)
>  {
> @@ -247,6 +271,17 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>  {
>  	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>  
> +	if (info->no_irq) {
> +		pm_runtime_get_sync(indio_dev->dev.parent);
> +
> +		regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
> +
> +		pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +		pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> +		return 0;
> +	}
> +
>  	return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
>  }
>  
> @@ -454,6 +489,58 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>  	return 0;
>  }
>  
> +static const struct of_device_id sun4i_gpadc_of_id[] = {
> +	{
> +		.compatible = "allwinner,sun8i-a33-gpadc-iio",
> +		.data = &sun8i_a33_gpadc_data,
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
> +				struct iio_dev *indio_dev)
> +{
> +	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
> +	const struct of_device_id *of_dev;
> +	struct thermal_zone_device *tzd;
> +	struct resource *mem;
> +	void __iomem *base;
> +	int ret;
> +
> +	of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
> +	if (!of_dev)
> +		return -ENODEV;
> +
> +	info->no_irq = true;
> +	info->data = (struct gpadc_data *)of_dev->data;
> +	indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
> +	indio_dev->channels = sun8i_a33_gpadc_channels;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					     &sun4i_gpadc_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		ret = PTR_ERR(info->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!IS_ENABLED(THERMAL_OF))
> +		return 0;
> +
> +	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
> +						   &sun4i_ts_tz_ops);
> +	if (IS_ERR(tzd))
> +		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
> +			PTR_ERR(tzd));
> +
> +	return PTR_ERR_OR_ZERO(tzd);
> +}
> +
>  static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>  				 struct iio_dev *indio_dev)
>  {
> @@ -462,6 +549,7 @@ static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>  		dev_get_drvdata(pdev->dev.parent);
>  	int ret;
>  
> +	info->no_irq = false;
>  	info->regmap = sun4i_gpadc_dev->regmap;
>  
>  	indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
> @@ -561,7 +649,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  	indio_dev->info = &sun4i_gpadc_iio_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
> +	if (pdev->dev.of_node)
> +		ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
> +	else
> +		ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -580,7 +672,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_map:
> -	if (IS_ENABLED(CONFIG_THERMAL_OF))
> +	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);
>  
>  	pm_runtime_put(&pdev->dev);
> @@ -592,10 +684,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  static int sun4i_gpadc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>  
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> -	if (IS_ENABLED(CONFIG_THERMAL_OF))
> +	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);
>  
>  	return 0;
> @@ -611,6 +704,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
>  static struct platform_driver sun4i_gpadc_driver = {
>  	.driver = {
>  		.name = "sun4i-gpadc-iio",
> +		.of_match_table = sun4i_gpadc_of_id,
>  		.pm = &sun4i_gpadc_pm_ops,
>  	},
>  	.id_table = sun4i_gpadc_id,
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 509e736..139872c 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
> @@ -38,6 +38,10 @@
>  #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)
> +
>  #define SUN4I_GPADC_CTRL2				0x08
>  
>  #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)
>
Icenowy Zheng March 14, 2017, 5:18 a.m. UTC | #2
14.03.2017, 05:08, "Jonathan Cameron" <jic23@kernel.org>:
> On 10/03/17 10:39, Quentin Schulz wrote:
>>  This adds support for the Allwinner A33 thermal sensor.
>>
>>  Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
>>  which is dedicated to the thermal sensor. Moreover, its thermal sensor
>>  does not generate interruptions, thus we only need to directly read the
>>  register storing the temperature value.
>>
>>  The MFD used by the A10, A13 and A31, was created to avoid breaking the
>>  DT binding, but since the nodes for the ADC weren't there for the A33,
>>  it is not needed.
>>
>>  Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>
> Talk me through why it makes sense to do this rather than simply spin out
> a really simple thermal driver for the A33?

According to him the A33 thermal sensor is a simplified version of the GPADC.

I have already did a simple thermal driver ~8 months ago, but is rejected for
this reason.

>
> I'm not against what you have here, but don't feel it has been fully argued.
>
> Jonathan
>>  ---
>>
>>  v2:
>>    - removed added comments in Kconfig,
>>    - simplified Kconfig depends on condition,
>>    - removed THERMAL_OF requirement for sun8i,
>>    - renamed sun8i_gpadc_channels to sun8i_a33_gpadc_channels,
>>    - renamed use_dt boolean in no_irq as it reflects better why we need it,
>>    - removed spurious/unneeded modifications done in v1,
>>
>>   drivers/iio/adc/Kconfig | 2 +-
>>   drivers/iio/adc/sun4i-gpadc-iio.c | 100 ++++++++++++++++++++++++++++++++++++--
>>   include/linux/mfd/sun4i-gpadc.h | 4 ++
>>   3 files changed, 102 insertions(+), 4 deletions(-)
>>
>>  diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>  index 9f8b4b1..8c8ead6 100644
>>  --- a/drivers/iio/adc/Kconfig
>>  +++ b/drivers/iio/adc/Kconfig
>>  @@ -562,7 +562,7 @@ config STX104
>>   config SUN4I_GPADC
>>           tristate "Support for the Allwinner SoCs GPADC"
>>           depends on IIO
>>  - depends on MFD_SUN4I_GPADC
>>  + depends on MFD_SUN4I_GPADC || MACH_SUN8I
>>           help
>>             Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>>             GPADC. This ADC provides 4 channels which can be used as an ADC or as
>>  diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
>>  index 7cb997a..70684cd 100644
>>  --- a/drivers/iio/adc/sun4i-gpadc-iio.c
>>  +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>>  @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
>>           .adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>>   };
>>
>>  +static const struct gpadc_data sun8i_a33_gpadc_data = {
>>  + .temp_offset = -1662,
>>  + .temp_scale = 162,
>>  + .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
>>  +};
>>  +
>>   struct sun4i_gpadc_iio {
>>           struct iio_dev *indio_dev;
>>           struct completion completion;
>>  @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
>>           unsigned int temp_data_irq;
>>           atomic_t ignore_temp_data_irq;
>>           const struct gpadc_data *data;
>>  + bool no_irq;
>>           /* prevents concurrent reads of temperature and ADC */
>>           struct mutex mutex;
>>   };
>>  @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
>>           SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>>   };
>>
>>  +static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = {
>>  + {
>>  + .type = IIO_TEMP,
>>  + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  + BIT(IIO_CHAN_INFO_SCALE) |
>>  + BIT(IIO_CHAN_INFO_OFFSET),
>>  + .datasheet_name = "temp_adc",
>>  + },
>>  +};
>>  +
>>  +static const struct regmap_config sun4i_gpadc_regmap_config = {
>>  + .reg_bits = 32,
>>  + .val_bits = 32,
>>  + .reg_stride = 4,
>>  + .fast_io = true,
>>  +};
>>  +
>>   static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
>>                                    unsigned int irq)
>>   {
>>  @@ -247,6 +271,17 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>>   {
>>           struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>
>>  + if (info->no_irq) {
>>  + pm_runtime_get_sync(indio_dev->dev.parent);
>>  +
>>  + regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>>  +
>>  + pm_runtime_mark_last_busy(indio_dev->dev.parent);
>>  + pm_runtime_put_autosuspend(indio_dev->dev.parent);
>>  +
>>  + return 0;
>>  + }
>>  +
>>           return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
>>   }
>>
>>  @@ -454,6 +489,58 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>>           return 0;
>>   }
>>
>>  +static const struct of_device_id sun4i_gpadc_of_id[] = {
>>  + {
>>  + .compatible = "allwinner,sun8i-a33-gpadc-iio",
>>  + .data = &sun8i_a33_gpadc_data,
>>  + },
>>  + { /* sentinel */ }
>>  +};
>>  +
>>  +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>>  + struct iio_dev *indio_dev)
>>  +{
>>  + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>  + const struct of_device_id *of_dev;
>>  + struct thermal_zone_device *tzd;
>>  + struct resource *mem;
>>  + void __iomem *base;
>>  + int ret;
>>  +
>>  + of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
>>  + if (!of_dev)
>>  + return -ENODEV;
>>  +
>>  + info->no_irq = true;
>>  + info->data = (struct gpadc_data *)of_dev->data;
>>  + indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
>>  + indio_dev->channels = sun8i_a33_gpadc_channels;
>>  +
>>  + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  + base = devm_ioremap_resource(&pdev->dev, mem);
>>  + if (IS_ERR(base))
>>  + return PTR_ERR(base);
>>  +
>>  + info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>>  + &sun4i_gpadc_regmap_config);
>>  + if (IS_ERR(info->regmap)) {
>>  + ret = PTR_ERR(info->regmap);
>>  + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>>  + return ret;
>>  + }
>>  +
>>  + if (!IS_ENABLED(THERMAL_OF))
>>  + return 0;
>>  +
>>  + tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
>>  + &sun4i_ts_tz_ops);
>>  + if (IS_ERR(tzd))
>>  + dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
>>  + PTR_ERR(tzd));
>>  +
>>  + return PTR_ERR_OR_ZERO(tzd);
>>  +}
>>  +
>>   static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>>                                    struct iio_dev *indio_dev)
>>   {
>>  @@ -462,6 +549,7 @@ static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>>                   dev_get_drvdata(pdev->dev.parent);
>>           int ret;
>>
>>  + info->no_irq = false;
>>           info->regmap = sun4i_gpadc_dev->regmap;
>>
>>           indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
>>  @@ -561,7 +649,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>           indio_dev->info = &sun4i_gpadc_iio_info;
>>           indio_dev->modes = INDIO_DIRECT_MODE;
>>
>>  - ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>>  + if (pdev->dev.of_node)
>>  + ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
>>  + else
>>  + ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>>  +
>>           if (ret)
>>                   return ret;
>>
>>  @@ -580,7 +672,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>           return 0;
>>
>>   err_map:
>>  - if (IS_ENABLED(CONFIG_THERMAL_OF))
>>  + if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>                   iio_map_array_unregister(indio_dev);
>>
>>           pm_runtime_put(&pdev->dev);
>>  @@ -592,10 +684,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>   static int sun4i_gpadc_remove(struct platform_device *pdev)
>>   {
>>           struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>  + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>
>>           pm_runtime_put(&pdev->dev);
>>           pm_runtime_disable(&pdev->dev);
>>  - if (IS_ENABLED(CONFIG_THERMAL_OF))
>>  + if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>                   iio_map_array_unregister(indio_dev);
>>
>>           return 0;
>>  @@ -611,6 +704,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
>>   static struct platform_driver sun4i_gpadc_driver = {
>>           .driver = {
>>                   .name = "sun4i-gpadc-iio",
>>  + .of_match_table = sun4i_gpadc_of_id,
>>                   .pm = &sun4i_gpadc_pm_ops,
>>           },
>>           .id_table = sun4i_gpadc_id,
>>  diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
>>  index 509e736..139872c 100644
>>  --- a/include/linux/mfd/sun4i-gpadc.h
>>  +++ b/include/linux/mfd/sun4i-gpadc.h
>>  @@ -38,6 +38,10 @@
>>   #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)
>>  +
>>   #define SUN4I_GPADC_CTRL2 0x08
>>
>>   #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quentin Schulz March 14, 2017, 7:15 a.m. UTC | #3
Hi Jonathan,

On 14/03/2017 06:18, Icenowy Zheng wrote:
> 
> 
> 14.03.2017, 05:08, "Jonathan Cameron" <jic23@kernel.org>:
>> On 10/03/17 10:39, Quentin Schulz wrote:
>>>  This adds support for the Allwinner A33 thermal sensor.
>>>
>>>  Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
>>>  which is dedicated to the thermal sensor. Moreover, its thermal sensor
>>>  does not generate interruptions, thus we only need to directly read the
>>>  register storing the temperature value.
>>>
>>>  The MFD used by the A10, A13 and A31, was created to avoid breaking the
>>>  DT binding, but since the nodes for the ADC weren't there for the A33,
>>>  it is not needed.
>>>
>>>  Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>
>> Talk me through why it makes sense to do this rather than simply spin out
>> a really simple thermal driver for the A33?
> 
> According to him the A33 thermal sensor is a simplified version of the GPADC.
> 

Same registers with almost same bits for the same purpose (temperature).
Some SoCs have an ADC and one or more thermal sensors (A10, A13, A31,
H3) while others have only one thermal sensor (A23, A33). Same IP with
or without ADC/Touchscreen so same driver, that was my mindset.

The thermal part behaves the same except for the presence of interrupts
or not. (A33 has bits for interrupts in the datasheet but that just does
not work).

Thanks,
Quentin

> I have already did a simple thermal driver ~8 months ago, but is rejected for
> this reason.
> 
>>
>> I'm not against what you have here, but don't feel it has been fully argued.
>>
>> Jonathan
>>>  ---
>>>
>>>  v2:
>>>    - removed added comments in Kconfig,
>>>    - simplified Kconfig depends on condition,
>>>    - removed THERMAL_OF requirement for sun8i,
>>>    - renamed sun8i_gpadc_channels to sun8i_a33_gpadc_channels,
>>>    - renamed use_dt boolean in no_irq as it reflects better why we need it,
>>>    - removed spurious/unneeded modifications done in v1,
>>>
>>>   drivers/iio/adc/Kconfig | 2 +-
>>>   drivers/iio/adc/sun4i-gpadc-iio.c | 100 ++++++++++++++++++++++++++++++++++++--
>>>   include/linux/mfd/sun4i-gpadc.h | 4 ++
>>>   3 files changed, 102 insertions(+), 4 deletions(-)
>>>
>>>  diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>  index 9f8b4b1..8c8ead6 100644
>>>  --- a/drivers/iio/adc/Kconfig
>>>  +++ b/drivers/iio/adc/Kconfig
>>>  @@ -562,7 +562,7 @@ config STX104
>>>   config SUN4I_GPADC
>>>           tristate "Support for the Allwinner SoCs GPADC"
>>>           depends on IIO
>>>  - depends on MFD_SUN4I_GPADC
>>>  + depends on MFD_SUN4I_GPADC || MACH_SUN8I
>>>           help
>>>             Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>>>             GPADC. This ADC provides 4 channels which can be used as an ADC or as
>>>  diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
>>>  index 7cb997a..70684cd 100644
>>>  --- a/drivers/iio/adc/sun4i-gpadc-iio.c
>>>  +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>>>  @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
>>>           .adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>>>   };
>>>
>>>  +static const struct gpadc_data sun8i_a33_gpadc_data = {
>>>  + .temp_offset = -1662,
>>>  + .temp_scale = 162,
>>>  + .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
>>>  +};
>>>  +
>>>   struct sun4i_gpadc_iio {
>>>           struct iio_dev *indio_dev;
>>>           struct completion completion;
>>>  @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
>>>           unsigned int temp_data_irq;
>>>           atomic_t ignore_temp_data_irq;
>>>           const struct gpadc_data *data;
>>>  + bool no_irq;
>>>           /* prevents concurrent reads of temperature and ADC */
>>>           struct mutex mutex;
>>>   };
>>>  @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
>>>           SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>>>   };
>>>
>>>  +static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = {
>>>  + {
>>>  + .type = IIO_TEMP,
>>>  + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>  + BIT(IIO_CHAN_INFO_SCALE) |
>>>  + BIT(IIO_CHAN_INFO_OFFSET),
>>>  + .datasheet_name = "temp_adc",
>>>  + },
>>>  +};
>>>  +
>>>  +static const struct regmap_config sun4i_gpadc_regmap_config = {
>>>  + .reg_bits = 32,
>>>  + .val_bits = 32,
>>>  + .reg_stride = 4,
>>>  + .fast_io = true,
>>>  +};
>>>  +
>>>   static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
>>>                                    unsigned int irq)
>>>   {
>>>  @@ -247,6 +271,17 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>>>   {
>>>           struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>>
>>>  + if (info->no_irq) {
>>>  + pm_runtime_get_sync(indio_dev->dev.parent);
>>>  +
>>>  + regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>>>  +
>>>  + pm_runtime_mark_last_busy(indio_dev->dev.parent);
>>>  + pm_runtime_put_autosuspend(indio_dev->dev.parent);
>>>  +
>>>  + return 0;
>>>  + }
>>>  +
>>>           return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
>>>   }
>>>
>>>  @@ -454,6 +489,58 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>>>           return 0;
>>>   }
>>>
>>>  +static const struct of_device_id sun4i_gpadc_of_id[] = {
>>>  + {
>>>  + .compatible = "allwinner,sun8i-a33-gpadc-iio",
>>>  + .data = &sun8i_a33_gpadc_data,
>>>  + },
>>>  + { /* sentinel */ }
>>>  +};
>>>  +
>>>  +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>>>  + struct iio_dev *indio_dev)
>>>  +{
>>>  + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>>  + const struct of_device_id *of_dev;
>>>  + struct thermal_zone_device *tzd;
>>>  + struct resource *mem;
>>>  + void __iomem *base;
>>>  + int ret;
>>>  +
>>>  + of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
>>>  + if (!of_dev)
>>>  + return -ENODEV;
>>>  +
>>>  + info->no_irq = true;
>>>  + info->data = (struct gpadc_data *)of_dev->data;
>>>  + indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
>>>  + indio_dev->channels = sun8i_a33_gpadc_channels;
>>>  +
>>>  + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  + base = devm_ioremap_resource(&pdev->dev, mem);
>>>  + if (IS_ERR(base))
>>>  + return PTR_ERR(base);
>>>  +
>>>  + info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>>>  + &sun4i_gpadc_regmap_config);
>>>  + if (IS_ERR(info->regmap)) {
>>>  + ret = PTR_ERR(info->regmap);
>>>  + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>>>  + return ret;
>>>  + }
>>>  +
>>>  + if (!IS_ENABLED(THERMAL_OF))
>>>  + return 0;
>>>  +
>>>  + tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
>>>  + &sun4i_ts_tz_ops);
>>>  + if (IS_ERR(tzd))
>>>  + dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
>>>  + PTR_ERR(tzd));
>>>  +
>>>  + return PTR_ERR_OR_ZERO(tzd);
>>>  +}
>>>  +
>>>   static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>>>                                    struct iio_dev *indio_dev)
>>>   {
>>>  @@ -462,6 +549,7 @@ static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>>>                   dev_get_drvdata(pdev->dev.parent);
>>>           int ret;
>>>
>>>  + info->no_irq = false;
>>>           info->regmap = sun4i_gpadc_dev->regmap;
>>>
>>>           indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
>>>  @@ -561,7 +649,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>>           indio_dev->info = &sun4i_gpadc_iio_info;
>>>           indio_dev->modes = INDIO_DIRECT_MODE;
>>>
>>>  - ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>>>  + if (pdev->dev.of_node)
>>>  + ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
>>>  + else
>>>  + ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>>>  +
>>>           if (ret)
>>>                   return ret;
>>>
>>>  @@ -580,7 +672,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>>           return 0;
>>>
>>>   err_map:
>>>  - if (IS_ENABLED(CONFIG_THERMAL_OF))
>>>  + if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>>                   iio_map_array_unregister(indio_dev);
>>>
>>>           pm_runtime_put(&pdev->dev);
>>>  @@ -592,10 +684,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>>   static int sun4i_gpadc_remove(struct platform_device *pdev)
>>>   {
>>>           struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>  + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>>
>>>           pm_runtime_put(&pdev->dev);
>>>           pm_runtime_disable(&pdev->dev);
>>>  - if (IS_ENABLED(CONFIG_THERMAL_OF))
>>>  + if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>>                   iio_map_array_unregister(indio_dev);
>>>
>>>           return 0;
>>>  @@ -611,6 +704,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
>>>   static struct platform_driver sun4i_gpadc_driver = {
>>>           .driver = {
>>>                   .name = "sun4i-gpadc-iio",
>>>  + .of_match_table = sun4i_gpadc_of_id,
>>>                   .pm = &sun4i_gpadc_pm_ops,
>>>           },
>>>           .id_table = sun4i_gpadc_id,
>>>  diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
>>>  index 509e736..139872c 100644
>>>  --- a/include/linux/mfd/sun4i-gpadc.h
>>>  +++ b/include/linux/mfd/sun4i-gpadc.h
>>>  @@ -38,6 +38,10 @@
>>>   #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)
>>>  +
>>>   #define SUN4I_GPADC_CTRL2 0x08
>>>
>>>   #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Lee Jones March 15, 2017, 10:38 a.m. UTC | #4
On Fri, 10 Mar 2017, Quentin Schulz wrote:

> This adds support for the Allwinner A33 thermal sensor.
> 
> Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
> which is dedicated to the thermal sensor. Moreover, its thermal sensor
> does not generate interruptions, thus we only need to directly read the
> register storing the temperature value.
> 
> The MFD used by the A10, A13 and A31, was created to avoid breaking the
> DT binding, but since the nodes for the ADC weren't there for the A33,
> it is not needed.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> v2:
>   - removed added comments in Kconfig,
>   - simplified Kconfig depends on condition,
>   - removed THERMAL_OF requirement for sun8i,
>   - renamed sun8i_gpadc_channels to sun8i_a33_gpadc_channels,
>   - renamed use_dt boolean in no_irq as it reflects better why we need it,
>   - removed spurious/unneeded modifications done in v1,
> 
>  drivers/iio/adc/Kconfig           |   2 +-
>  drivers/iio/adc/sun4i-gpadc-iio.c | 100 ++++++++++++++++++++++++++++++++++++--

>  include/linux/mfd/sun4i-gpadc.h   |   4 ++

For the MFD parts only:
  Acked-by: Lee Jones <lee.jones@linaro.org>

>  3 files changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9f8b4b1..8c8ead6 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -562,7 +562,7 @@ config STX104
>  config SUN4I_GPADC
>  	tristate "Support for the Allwinner SoCs GPADC"
>  	depends on IIO
> -	depends on MFD_SUN4I_GPADC
> +	depends on MFD_SUN4I_GPADC || MACH_SUN8I
>  	help
>  	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>  	  GPADC. This ADC provides 4 channels which can be used as an ADC or as
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index 7cb997a..70684cd 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
>  	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>  };
>  
> +static const struct gpadc_data sun8i_a33_gpadc_data = {
> +	.temp_offset = -1662,
> +	.temp_scale = 162,
> +	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
> +};
> +
>  struct sun4i_gpadc_iio {
>  	struct iio_dev			*indio_dev;
>  	struct completion		completion;
> @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
>  	unsigned int			temp_data_irq;
>  	atomic_t			ignore_temp_data_irq;
>  	const struct gpadc_data		*data;
> +	bool				no_irq;
>  	/* prevents concurrent reads of temperature and ADC */
>  	struct mutex			mutex;
>  };
> @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
>  	SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>  };
>  
> +static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.datasheet_name = "temp_adc",
> +	},
> +};
> +
> +static const struct regmap_config sun4i_gpadc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
>  static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
>  				 unsigned int irq)
>  {
> @@ -247,6 +271,17 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>  {
>  	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>  
> +	if (info->no_irq) {
> +		pm_runtime_get_sync(indio_dev->dev.parent);
> +
> +		regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
> +
> +		pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +		pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> +		return 0;
> +	}
> +
>  	return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
>  }
>  
> @@ -454,6 +489,58 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>  	return 0;
>  }
>  
> +static const struct of_device_id sun4i_gpadc_of_id[] = {
> +	{
> +		.compatible = "allwinner,sun8i-a33-gpadc-iio",
> +		.data = &sun8i_a33_gpadc_data,
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
> +				struct iio_dev *indio_dev)
> +{
> +	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
> +	const struct of_device_id *of_dev;
> +	struct thermal_zone_device *tzd;
> +	struct resource *mem;
> +	void __iomem *base;
> +	int ret;
> +
> +	of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
> +	if (!of_dev)
> +		return -ENODEV;
> +
> +	info->no_irq = true;
> +	info->data = (struct gpadc_data *)of_dev->data;
> +	indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
> +	indio_dev->channels = sun8i_a33_gpadc_channels;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					     &sun4i_gpadc_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		ret = PTR_ERR(info->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!IS_ENABLED(THERMAL_OF))
> +		return 0;
> +
> +	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
> +						   &sun4i_ts_tz_ops);
> +	if (IS_ERR(tzd))
> +		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
> +			PTR_ERR(tzd));
> +
> +	return PTR_ERR_OR_ZERO(tzd);
> +}
> +
>  static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>  				 struct iio_dev *indio_dev)
>  {
> @@ -462,6 +549,7 @@ static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>  		dev_get_drvdata(pdev->dev.parent);
>  	int ret;
>  
> +	info->no_irq = false;
>  	info->regmap = sun4i_gpadc_dev->regmap;
>  
>  	indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
> @@ -561,7 +649,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  	indio_dev->info = &sun4i_gpadc_iio_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
> +	if (pdev->dev.of_node)
> +		ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
> +	else
> +		ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -580,7 +672,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_map:
> -	if (IS_ENABLED(CONFIG_THERMAL_OF))
> +	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);
>  
>  	pm_runtime_put(&pdev->dev);
> @@ -592,10 +684,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  static int sun4i_gpadc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>  
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> -	if (IS_ENABLED(CONFIG_THERMAL_OF))
> +	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);
>  
>  	return 0;
> @@ -611,6 +704,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
>  static struct platform_driver sun4i_gpadc_driver = {
>  	.driver = {
>  		.name = "sun4i-gpadc-iio",
> +		.of_match_table = sun4i_gpadc_of_id,
>  		.pm = &sun4i_gpadc_pm_ops,
>  	},
>  	.id_table = sun4i_gpadc_id,
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 509e736..139872c 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
> @@ -38,6 +38,10 @@
>  #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)
> +
>  #define SUN4I_GPADC_CTRL2				0x08
>  
>  #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)
Jonathan Cameron March 18, 2017, 2:18 p.m. UTC | #5
On 14/03/17 07:15, Quentin Schulz wrote:
> Hi Jonathan,
> 
> On 14/03/2017 06:18, Icenowy Zheng wrote:
>>
>>
>> 14.03.2017, 05:08, "Jonathan Cameron" <jic23@kernel.org>:
>>> On 10/03/17 10:39, Quentin Schulz wrote:
>>>>  This adds support for the Allwinner A33 thermal sensor.
>>>>
>>>>  Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
>>>>  which is dedicated to the thermal sensor. Moreover, its thermal sensor
>>>>  does not generate interruptions, thus we only need to directly read the
>>>>  register storing the temperature value.
>>>>
>>>>  The MFD used by the A10, A13 and A31, was created to avoid breaking the
>>>>  DT binding, but since the nodes for the ADC weren't there for the A33,
>>>>  it is not needed.
>>>>
>>>>  Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>
>>> Talk me through why it makes sense to do this rather than simply spin out
>>> a really simple thermal driver for the A33?
>>
>> According to him the A33 thermal sensor is a simplified version of the GPADC.
>>
> 
> Same registers with almost same bits for the same purpose (temperature).
> Some SoCs have an ADC and one or more thermal sensors (A10, A13, A31,
> H3) while others have only one thermal sensor (A23, A33). Same IP with
> or without ADC/Touchscreen so same driver, that was my mindset.
> 
> The thermal part behaves the same except for the presence of interrupts
> or not. (A33 has bits for interrupts in the datasheet but that just does
> not work).
Hmm. I'm just about convinced by this argument, please make it in the patch
description going forward.

Jonathan
> 
> Thanks,
> Quentin
> 
>> I have already did a simple thermal driver ~8 months ago, but is rejected for
>> this reason.
>>
>>>
>>> I'm not against what you have here, but don't feel it has been fully argued.
>>>
>>> Jonathan
>>>>  ---
>>>>
>>>>  v2:
>>>>    - removed added comments in Kconfig,
>>>>    - simplified Kconfig depends on condition,
>>>>    - removed THERMAL_OF requirement for sun8i,
>>>>    - renamed sun8i_gpadc_channels to sun8i_a33_gpadc_channels,
>>>>    - renamed use_dt boolean in no_irq as it reflects better why we need it,
>>>>    - removed spurious/unneeded modifications done in v1,
>>>>
>>>>   drivers/iio/adc/Kconfig | 2 +-
>>>>   drivers/iio/adc/sun4i-gpadc-iio.c | 100 ++++++++++++++++++++++++++++++++++++--
>>>>   include/linux/mfd/sun4i-gpadc.h | 4 ++
>>>>   3 files changed, 102 insertions(+), 4 deletions(-)
>>>>
>>>>  diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>>  index 9f8b4b1..8c8ead6 100644
>>>>  --- a/drivers/iio/adc/Kconfig
>>>>  +++ b/drivers/iio/adc/Kconfig
>>>>  @@ -562,7 +562,7 @@ config STX104
>>>>   config SUN4I_GPADC
>>>>           tristate "Support for the Allwinner SoCs GPADC"
>>>>           depends on IIO
>>>>  - depends on MFD_SUN4I_GPADC
>>>>  + depends on MFD_SUN4I_GPADC || MACH_SUN8I
>>>>           help
>>>>             Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>>>>             GPADC. This ADC provides 4 channels which can be used as an ADC or as
>>>>  diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
>>>>  index 7cb997a..70684cd 100644
>>>>  --- a/drivers/iio/adc/sun4i-gpadc-iio.c
>>>>  +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>>>>  @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
>>>>           .adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>>>>   };
>>>>
>>>>  +static const struct gpadc_data sun8i_a33_gpadc_data = {
>>>>  + .temp_offset = -1662,
>>>>  + .temp_scale = 162,
>>>>  + .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
>>>>  +};
>>>>  +
>>>>   struct sun4i_gpadc_iio {
>>>>           struct iio_dev *indio_dev;
>>>>           struct completion completion;
>>>>  @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
>>>>           unsigned int temp_data_irq;
>>>>           atomic_t ignore_temp_data_irq;
>>>>           const struct gpadc_data *data;
>>>>  + bool no_irq;
>>>>           /* prevents concurrent reads of temperature and ADC */
>>>>           struct mutex mutex;
>>>>   };
>>>>  @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
>>>>           SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>>>>   };
>>>>
>>>>  +static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = {
>>>>  + {
>>>>  + .type = IIO_TEMP,
>>>>  + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>  + BIT(IIO_CHAN_INFO_SCALE) |
>>>>  + BIT(IIO_CHAN_INFO_OFFSET),
>>>>  + .datasheet_name = "temp_adc",
>>>>  + },
>>>>  +};
>>>>  +
>>>>  +static const struct regmap_config sun4i_gpadc_regmap_config = {
>>>>  + .reg_bits = 32,
>>>>  + .val_bits = 32,
>>>>  + .reg_stride = 4,
>>>>  + .fast_io = true,
>>>>  +};
>>>>  +
>>>>   static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
>>>>                                    unsigned int irq)
>>>>   {
>>>>  @@ -247,6 +271,17 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>>>>   {
>>>>           struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>>>
>>>>  + if (info->no_irq) {
>>>>  + pm_runtime_get_sync(indio_dev->dev.parent);
>>>>  +
>>>>  + regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>>>>  +
>>>>  + pm_runtime_mark_last_busy(indio_dev->dev.parent);
>>>>  + pm_runtime_put_autosuspend(indio_dev->dev.parent);
>>>>  +
>>>>  + return 0;
>>>>  + }
>>>>  +
>>>>           return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
>>>>   }
>>>>
>>>>  @@ -454,6 +489,58 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>>>>           return 0;
>>>>   }
>>>>
>>>>  +static const struct of_device_id sun4i_gpadc_of_id[] = {
>>>>  + {
>>>>  + .compatible = "allwinner,sun8i-a33-gpadc-iio",
>>>>  + .data = &sun8i_a33_gpadc_data,
>>>>  + },
>>>>  + { /* sentinel */ }
>>>>  +};
>>>>  +
>>>>  +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>>>>  + struct iio_dev *indio_dev)
>>>>  +{
>>>>  + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>>>  + const struct of_device_id *of_dev;
>>>>  + struct thermal_zone_device *tzd;
>>>>  + struct resource *mem;
>>>>  + void __iomem *base;
>>>>  + int ret;
>>>>  +
>>>>  + of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
>>>>  + if (!of_dev)
>>>>  + return -ENODEV;
>>>>  +
>>>>  + info->no_irq = true;
>>>>  + info->data = (struct gpadc_data *)of_dev->data;
>>>>  + indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
>>>>  + indio_dev->channels = sun8i_a33_gpadc_channels;
>>>>  +
>>>>  + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>  + base = devm_ioremap_resource(&pdev->dev, mem);
>>>>  + if (IS_ERR(base))
>>>>  + return PTR_ERR(base);
>>>>  +
>>>>  + info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>>>>  + &sun4i_gpadc_regmap_config);
>>>>  + if (IS_ERR(info->regmap)) {
>>>>  + ret = PTR_ERR(info->regmap);
>>>>  + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>>>>  + return ret;
>>>>  + }
>>>>  +
>>>>  + if (!IS_ENABLED(THERMAL_OF))
>>>>  + return 0;
>>>>  +
>>>>  + tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
>>>>  + &sun4i_ts_tz_ops);
>>>>  + if (IS_ERR(tzd))
>>>>  + dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
>>>>  + PTR_ERR(tzd));
>>>>  +
>>>>  + return PTR_ERR_OR_ZERO(tzd);
>>>>  +}
>>>>  +
>>>>   static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>>>>                                    struct iio_dev *indio_dev)
>>>>   {
>>>>  @@ -462,6 +549,7 @@ static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>>>>                   dev_get_drvdata(pdev->dev.parent);
>>>>           int ret;
>>>>
>>>>  + info->no_irq = false;
>>>>           info->regmap = sun4i_gpadc_dev->regmap;
>>>>
>>>>           indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
>>>>  @@ -561,7 +649,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>>>           indio_dev->info = &sun4i_gpadc_iio_info;
>>>>           indio_dev->modes = INDIO_DIRECT_MODE;
>>>>
>>>>  - ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>>>>  + if (pdev->dev.of_node)
>>>>  + ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
>>>>  + else
>>>>  + ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>>>>  +
>>>>           if (ret)
>>>>                   return ret;
>>>>
>>>>  @@ -580,7 +672,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>>>           return 0;
>>>>
>>>>   err_map:
>>>>  - if (IS_ENABLED(CONFIG_THERMAL_OF))
>>>>  + if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>>>                   iio_map_array_unregister(indio_dev);
>>>>
>>>>           pm_runtime_put(&pdev->dev);
>>>>  @@ -592,10 +684,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>>>>   static int sun4i_gpadc_remove(struct platform_device *pdev)
>>>>   {
>>>>           struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>  + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>>>
>>>>           pm_runtime_put(&pdev->dev);
>>>>           pm_runtime_disable(&pdev->dev);
>>>>  - if (IS_ENABLED(CONFIG_THERMAL_OF))
>>>>  + if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>>>                   iio_map_array_unregister(indio_dev);
>>>>
>>>>           return 0;
>>>>  @@ -611,6 +704,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
>>>>   static struct platform_driver sun4i_gpadc_driver = {
>>>>           .driver = {
>>>>                   .name = "sun4i-gpadc-iio",
>>>>  + .of_match_table = sun4i_gpadc_of_id,
>>>>                   .pm = &sun4i_gpadc_pm_ops,
>>>>           },
>>>>           .id_table = sun4i_gpadc_id,
>>>>  diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
>>>>  index 509e736..139872c 100644
>>>>  --- a/include/linux/mfd/sun4i-gpadc.h
>>>>  +++ b/include/linux/mfd/sun4i-gpadc.h
>>>>  @@ -38,6 +38,10 @@
>>>>   #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)
>>>>  +
>>>>   #define SUN4I_GPADC_CTRL2 0x08
>>>>
>>>>   #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
diff mbox

Patch

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 9f8b4b1..8c8ead6 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -562,7 +562,7 @@  config STX104
 config SUN4I_GPADC
 	tristate "Support for the Allwinner SoCs GPADC"
 	depends on IIO
-	depends on MFD_SUN4I_GPADC
+	depends on MFD_SUN4I_GPADC || MACH_SUN8I
 	help
 	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
 	  GPADC. This ADC provides 4 channels which can be used as an ADC or as
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 7cb997a..70684cd 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -85,6 +85,12 @@  static const struct gpadc_data sun6i_gpadc_data = {
 	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
 };
 
+static const struct gpadc_data sun8i_a33_gpadc_data = {
+	.temp_offset = -1662,
+	.temp_scale = 162,
+	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
+};
+
 struct sun4i_gpadc_iio {
 	struct iio_dev			*indio_dev;
 	struct completion		completion;
@@ -96,6 +102,7 @@  struct sun4i_gpadc_iio {
 	unsigned int			temp_data_irq;
 	atomic_t			ignore_temp_data_irq;
 	const struct gpadc_data		*data;
+	bool				no_irq;
 	/* prevents concurrent reads of temperature and ADC */
 	struct mutex			mutex;
 };
@@ -138,6 +145,23 @@  static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
 	SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
 };
 
+static const struct iio_chan_spec sun8i_a33_gpadc_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.datasheet_name = "temp_adc",
+	},
+};
+
+static const struct regmap_config sun4i_gpadc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
 static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
 				 unsigned int irq)
 {
@@ -247,6 +271,17 @@  static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
 
+	if (info->no_irq) {
+		pm_runtime_get_sync(indio_dev->dev.parent);
+
+		regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+
+		pm_runtime_mark_last_busy(indio_dev->dev.parent);
+		pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+		return 0;
+	}
+
 	return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
 }
 
@@ -454,6 +489,58 @@  static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 	return 0;
 }
 
+static const struct of_device_id sun4i_gpadc_of_id[] = {
+	{
+		.compatible = "allwinner,sun8i-a33-gpadc-iio",
+		.data = &sun8i_a33_gpadc_data,
+	},
+	{ /* sentinel */ }
+};
+
+static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
+				struct iio_dev *indio_dev)
+{
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	const struct of_device_id *of_dev;
+	struct thermal_zone_device *tzd;
+	struct resource *mem;
+	void __iomem *base;
+	int ret;
+
+	of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
+	if (!of_dev)
+		return -ENODEV;
+
+	info->no_irq = true;
+	info->data = (struct gpadc_data *)of_dev->data;
+	indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
+	indio_dev->channels = sun8i_a33_gpadc_channels;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					     &sun4i_gpadc_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		ret = PTR_ERR(info->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
+
+	if (!IS_ENABLED(THERMAL_OF))
+		return 0;
+
+	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
+						   &sun4i_ts_tz_ops);
+	if (IS_ERR(tzd))
+		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
+			PTR_ERR(tzd));
+
+	return PTR_ERR_OR_ZERO(tzd);
+}
+
 static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
 				 struct iio_dev *indio_dev)
 {
@@ -462,6 +549,7 @@  static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
 		dev_get_drvdata(pdev->dev.parent);
 	int ret;
 
+	info->no_irq = false;
 	info->regmap = sun4i_gpadc_dev->regmap;
 
 	indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
@@ -561,7 +649,11 @@  static int sun4i_gpadc_probe(struct platform_device *pdev)
 	indio_dev->info = &sun4i_gpadc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
+	if (pdev->dev.of_node)
+		ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
+	else
+		ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
+
 	if (ret)
 		return ret;
 
@@ -580,7 +672,7 @@  static int sun4i_gpadc_probe(struct platform_device *pdev)
 	return 0;
 
 err_map:
-	if (IS_ENABLED(CONFIG_THERMAL_OF))
+	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
 		iio_map_array_unregister(indio_dev);
 
 	pm_runtime_put(&pdev->dev);
@@ -592,10 +684,11 @@  static int sun4i_gpadc_probe(struct platform_device *pdev)
 static int sun4i_gpadc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
 
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	if (IS_ENABLED(CONFIG_THERMAL_OF))
+	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
 		iio_map_array_unregister(indio_dev);
 
 	return 0;
@@ -611,6 +704,7 @@  static const struct platform_device_id sun4i_gpadc_id[] = {
 static struct platform_driver sun4i_gpadc_driver = {
 	.driver = {
 		.name = "sun4i-gpadc-iio",
+		.of_match_table = sun4i_gpadc_of_id,
 		.pm = &sun4i_gpadc_pm_ops,
 	},
 	.id_table = sun4i_gpadc_id,
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 509e736..139872c 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -38,6 +38,10 @@ 
 #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)
+
 #define SUN4I_GPADC_CTRL2				0x08
 
 #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)