Date:   Sat, 18 Mar 2017 14:18:02 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>,
        Icenowy Zheng <icenowy@...c.xyz>,
        "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "maxime.ripard@...e-electrons.com" <maxime.ripard@...e-electrons.com>,
        "wens@...e.org" <wens@...e.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "knaack.h@....de" <knaack.h@....de>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "stefan.mavrodiev@...il.com" <stefan.mavrodiev@...il.com>
Cc:     "thomas.petazzoni@...e-electrons.com" 
        <thomas.petazzoni@...e-electrons.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-sunxi@...glegroups.com" <linux-sunxi@...glegroups.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 08/11] iio: adc: sun4i-gpadc-iio: add support for A33
 thermal sensor

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@...nel.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@...e-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@...ts.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Powered by blists - more mailing lists