devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Icenowy Zheng <icenowy@aosc.io>
To: linux-arm-kernel@lists.infradead.org,
	Philipp Rossak <embed3d@gmail.com>,
	Jonathan Cameron <jic23@kernel.org>
Cc: mark.rutland@arm.com, sean@mess.org, linux-iio@vger.kernel.org,
	linux-sunxi@googlegroups.com, clabbe.montjoie@gmail.com,
	pmeerw@pmeerw.net, lee.jones@linaro.org, lars@metafoo.de,
	edu.molinas@gmail.com, linux@armlinux.org.uk, krzk@kernel.org,
	wens@csie.org, hans.verkuil@cisco.com, rask@formelder.dk,
	devicetree@vger.kernel.org, mchehab@kernel.org,
	robh+dt@kernel.org, singhalsimran0@gmail.com,
	linux-kernel@vger.kernel.org, quentin.schulz@free-electrons.com,
	knaack.h@gmx.de, maxime.ripard@free-electrons.com,
	davem@davemloft.net
Subject: Re: [PATCH 04/16] iio: adc: sun4i-gpadc-iio: rework: sampling start/end code readout reg
Date: Sun, 28 Jan 2018 21:37:59 +0800	[thread overview]
Message-ID: <2CC367BA-766A-4384-82EE-9A38BBFB08C2@aosc.io> (raw)
In-Reply-To: <ee2bd3b5-e0c7-66b2-d05f-1523d9d894d7@gmail.com>



于 2018年1月28日 GMT+08:00 下午9:34:17, Philipp Rossak <embed3d@gmail.com> 写到:
>
>
>On 28.01.2018 09:43, Jonathan Cameron wrote:
>> On Fri, 26 Jan 2018 16:19:29 +0100
>> Philipp Rossak <embed3d@gmail.com> wrote:
>> 
>>> For adding newer sensor some basic rework of the code is necessary.
>>>
>>> This commit reworks the code and allows the sampling start/end code
>and
>>> the position of value readout register to be altered. Later the
>start/end
>>> functions will be used to configure the ths and start/stop the
>>> sampling.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> Signed-off-by: Philipp Rossak <embed3d@gmail.com>
>> Hmm. It almost always turns out to be a bad idea to assume a
>particular
>> set of registers will be consistent across a hardware family.  Usual
>convention
>> is just to prefix them with the first supported device and use them
>across
>> the variants where they are correct.  I.e. don't use wild cards or
>generic
>> names as they often end up implying a wider range of applicability
>than
>> we want.
>>
>
>The new THS sensors seems to use the same THS ip core, only configured 
>with different amount of sensors. So for H3, A83T, H5, A80 and A64 the 
>registers are all the same.
>
>I think the big question is, do we want to have a few very big patches.
>
>Or more small patches with code that is used in later patches.

Allwinner H6 has changed the THS core again.

Please consider this :-)

>
>> A few other comments inline.  Reworking code to make it ready for
>later
>> patches is fine, but this also adds a lot of new code which isn't
>used until
>> much later in the series.  Move it there...
>
>please see comments below.
>
>>> ---
>>>   drivers/iio/adc/sun4i-gpadc-iio.c | 87
>+++++++++++++++++++++++++++++++++++----
>>>   include/linux/mfd/sun4i-gpadc.h   | 19 +++++++--
>>>   2 files changed, 94 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c
>b/drivers/iio/adc/sun4i-gpadc-iio.c
>>> index 03804ff9c006..363936b37c5a 100644
>>> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
>>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>>> @@ -49,6 +49,18 @@ static unsigned int
>sun6i_gpadc_chan_select(unsigned int chan)
>>>   	return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>>>   }
>>>   
>>> +struct sun4i_gpadc_iio;
>>> +
>>> +/*
>>> + * Prototypes for these functions, which enable these functions to
>be
>>> + * referenced in gpadc_data structures.
>>> + */
>>> +static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info);
>>> +static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info);
>>> +
>>> +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info);
>> 
>> Superficially this appears to be introduced but not used... Patch 9
>> is first to use it I think?  Introduce it then rather than here.
>> 
>
>You are right, this function is used first in Patch 9. In an very early
>
>version, I had this together with Patch 9. But then I had the feeling 
>that everything got too messy... Right now this is a very big patch 
>series and I wanted to have a clear structure in it and easy to review.
>
>If I would rework this it will end up in squashing, Patch 4, Parts of 
>Patch 7, Patch 8 and Patch 9.
>
>>> +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info);
>>> +
>>>   struct gpadc_data {
>>>   	int		temp_offset;
>>>   	int		temp_scale;
>>> @@ -56,6 +68,13 @@ struct gpadc_data {
>>>   	unsigned int	tp_adc_select;
>>>   	unsigned int	(*adc_chan_select)(unsigned int chan);
>>>   	unsigned int	adc_chan_mask;
>>> +	unsigned int	temp_data;
>>> +	int		(*sample_start)(struct sun4i_gpadc_iio *info);
>>> +	int		(*sample_end)(struct sun4i_gpadc_iio *info);
>>> +	u32		ctrl0_map;
>>> +	u32		ctrl2_map;
>>> +	u32		sensor_en_map;
>>> +	u32		filter_map;
>>>   };
>>>   
>>>   static const struct gpadc_data sun4i_gpadc_data = {
>>> @@ -65,6 +84,9 @@ static const struct gpadc_data sun4i_gpadc_data =
>{
>>>   	.tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT,
>>>   	.adc_chan_select = &sun4i_gpadc_chan_select,
>>>   	.adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK,
>>> +	.temp_data = SUN4I_GPADC_TEMP_DATA,
>>> +	.sample_start = sun4i_gpadc_sample_start,
>>> +	.sample_end = sun4i_gpadc_sample_end,
>>>   };
>>>   
>>>   static const struct gpadc_data sun5i_gpadc_data = {
>>> @@ -74,6 +96,9 @@ static const struct gpadc_data sun5i_gpadc_data =
>{
>>>   	.tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT,
>>>   	.adc_chan_select = &sun4i_gpadc_chan_select,
>>>   	.adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK,
>>> +	.temp_data = SUN4I_GPADC_TEMP_DATA,
>>> +	.sample_start = sun4i_gpadc_sample_start,
>>> +	.sample_end = sun4i_gpadc_sample_end,
>>>   };
>>>   
>>>   static const struct gpadc_data sun6i_gpadc_data = {
>>> @@ -83,12 +108,18 @@ static const struct gpadc_data sun6i_gpadc_data
>= {
>>>   	.tp_adc_select = SUN6I_GPADC_CTRL1_TP_ADC_SELECT,
>>>   	.adc_chan_select = &sun6i_gpadc_chan_select,
>>>   	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>>> +	.temp_data = SUN4I_GPADC_TEMP_DATA,
>>> +	.sample_start = sun4i_gpadc_sample_start,
>>> +	.sample_end = sun4i_gpadc_sample_end,
>>>   };
>>>   
>>>   static const struct gpadc_data sun8i_a33_gpadc_data = {
>>>   	.temp_offset = -1662,
>>>   	.temp_scale = 162,
>>>   	.tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN,
>>> +	.temp_data = SUN4I_GPADC_TEMP_DATA,
>>> +	.sample_start = sun4i_gpadc_sample_start,
>>> +	.sample_end = sun4i_gpadc_sample_end,
>>>   };
>>>   
>>>   struct sun4i_gpadc_iio {
>>> @@ -277,7 +308,7 @@ 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);
>>> +		regmap_read(info->regmap, info->data->temp_data, val);
>>>   
>>>   		pm_runtime_mark_last_busy(indio_dev->dev.parent);
>>>   		pm_runtime_put_autosuspend(indio_dev->dev.parent);
>>> @@ -382,10 +413,8 @@ static irqreturn_t
>sun4i_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
>>>   	return IRQ_HANDLED;
>>>   }
>>>   
>>> -static int sun4i_gpadc_runtime_suspend(struct device *dev)
>>> +static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info)
>>>   {
>>> -	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 */
>>> @@ -394,19 +423,32 @@ static int sun4i_gpadc_runtime_suspend(struct
>device *dev)
>>>   	return 0;
>>>   }
>>>   
>>> -static int sun4i_gpadc_runtime_resume(struct device *dev)
>>> +static int sunxi_ths_sample_end(struct sun4i_gpadc_iio *info)
>>> +{
>>> +	/* Disable temperature sensor */
>>> +	regmap_write(info->regmap, SUNXI_THS_CTRL2, 0x0);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun4i_gpadc_runtime_suspend(struct device *dev)
>>>   {
>>>   	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
>>>   
>>> +	return info->data->sample_end(info);
>>> +}
>>> +
>>> +static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info)
>>> +{
>>>   	/* 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));
>>> +		     SUNXI_THS_ACQ0(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));
>>> +		     SUNXI_THS_FILTER_EN |
>>> +		     SUNXI_THS_FILTER_TYPE(1));
>> 
>> Why if these are temperature sensor features are we setting them in
>the
>> general purpose ADC start function?
>
>This is part of the olds sensors (before Allwinner reworked their 
>temperature sensor (only ths) ). Those "old" sensors integrate an ADC 
>and a Thermal sensor in one ip block/register space. I don't want to 
>break those sensors/adc, since I'm not able to test them!
>
>>>   	/* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s
>*/
>>>   	regmap_write(info->regmap, SUN4I_GPADC_TPR,
>>>   		     SUN4I_GPADC_TPR_TEMP_ENABLE |
>>> @@ -415,6 +457,35 @@ static int sun4i_gpadc_runtime_resume(struct
>device *dev)
>>>   	return 0;
>>>   }
>>>   
>>> +static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info)
>>> +{
>>> +	u32 value;
>>> +
>>> +	if (info->data->ctrl0_map)
>>> +		regmap_write(info->regmap, SUNXI_THS_CTRL0,
>>> +			info->data->ctrl0_map);
>>> +
>>> +	regmap_write(info->regmap, SUNXI_THS_CTRL2,
>>> +		info->data->ctrl2_map);
>>> +
>>> +	regmap_write(info->regmap, SUNXI_THS_FILTER,
>>> +		info->data->filter_map);
>>> +
>>> +	regmap_read(info->regmap, SUNXI_THS_CTRL2, &value);
>>> +
>>> +	regmap_write(info->regmap, SUNXI_THS_CTRL2,
>>> +		info->data->sensor_en_map | value);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun4i_gpadc_runtime_resume(struct device *dev)
>>> +{
>>> +	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
>>> +
>>> +	return info->data->sample_start(info);
>>> +}
>>> +
>>>   static int sun4i_gpadc_get_temp(void *data, int *temp)
>>>   {
>>>   	struct sun4i_gpadc_iio *info = data;
>>> diff --git a/include/linux/mfd/sun4i-gpadc.h
>b/include/linux/mfd/sun4i-gpadc.h
>>> index 78d31984a222..39e096c3ddac 100644
>>> --- a/include/linux/mfd/sun4i-gpadc.h
>>> +++ b/include/linux/mfd/sun4i-gpadc.h
>>> @@ -17,7 +17,6 @@
>>>   #define SUN4I_GPADC_CTRL0_ADC_CLK_SELECT		BIT(22)
>>>   #define SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(x)		((GENMASK(1, 0) &
>(x)) << 20)
>>>   #define SUN4I_GPADC_CTRL0_FS_DIV(x)			((GENMASK(3, 0) & (x)) <<
>16)
>>> -#define SUN4I_GPADC_CTRL0_T_ACQ(x)			(GENMASK(15, 0) & (x))
>>>   
>>>   #define SUN4I_GPADC_CTRL1				0x04
>>>   
>>> @@ -51,9 +50,6 @@
>>>   
>>>   #define SUN4I_GPADC_CTRL3				0x0c
>>>   
>>> -#define SUN4I_GPADC_CTRL3_FILTER_EN			BIT(2)
>>> -#define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
>>> -
>>>   #define SUN4I_GPADC_TPR					0x18
>>>   
>>>   #define SUN4I_GPADC_TPR_TEMP_ENABLE			BIT(16)
>>> @@ -90,6 +86,21 @@
>>>   /* 10s delay before suspending the IP */
>>>   #define SUN4I_GPADC_AUTOSUSPEND_DELAY			10000
>>>   
>>> +/* SUNXI_THS COMMON REGISTERS + DEFINES */
>>> +#define SUNXI_THS_CTRL0					0x00
>>> +#define SUNXI_THS_CTRL2					0x40
>>> +#define SUNXI_THS_FILTER				0x70
>>> +
>>> +#define SUNXI_THS_FILTER_EN				BIT(2)
>>> +#define SUNXI_THS_FILTER_TYPE(x)			(GENMASK(1, 0) & (x))
>>> +#define SUNXI_THS_ACQ0(x)				(GENMASK(15, 0) & (x))
>>> +#define SUNXI_THS_ACQ1(x)				(GENMASK(31, 16) & ((x) << 16))
>>> +
>>> +#define SUNXI_THS_TEMP_SENSE_EN0			BIT(0)
>>> +#define SUNXI_THS_TEMP_SENSE_EN1			BIT(1)
>>> +#define SUNXI_THS_TEMP_SENSE_EN2			BIT(2)
>>> +#define SUNXI_THS_TEMP_SENSE_EN3			BIT(3)
>>> +
>>>   struct sun4i_gpadc_dev {
>>>   	struct device			*dev;
>>>   	struct regmap			*regmap;
>> 
>
>In general I'm ok with changing those things, but I would like to wait 
>for some more feedback/thoughts about this.
>
>Thanks,
>Philipp
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2018-01-28 13:37 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-26 15:19 [PATCH 00/16] IIO-based thermal sensor driver for Allwinner H3 and A83T SoC Philipp Rossak
     [not found] ` <20180126151941.12183-1-embed3d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-26 15:19   ` [PATCH 01/16] dt-bindings: update the Allwinner GPADC device tree binding for H3 & A83T Philipp Rossak
2018-01-26 15:19   ` [PATCH 02/16] arm: config: sunxi_defconfig: enable SUN4I_GPADC Philipp Rossak
2018-01-26 15:19   ` [PATCH 03/16] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33 Philipp Rossak
     [not found]     ` <20180126151941.12183-4-embed3d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-28  8:34       ` Jonathan Cameron
2018-01-26 15:19   ` [PATCH 04/16] iio: adc: sun4i-gpadc-iio: rework: sampling start/end code readout reg Philipp Rossak
2018-01-28  8:43     ` Jonathan Cameron
2018-01-28 13:34       ` Philipp Rossak
2018-01-28 13:37         ` Icenowy Zheng [this message]
     [not found]           ` <2CC367BA-766A-4384-82EE-9A38BBFB08C2-h8G6r0blFSE@public.gmane.org>
2018-01-28 14:32             ` Philipp Rossak
2018-01-26 15:19   ` [PATCH 05/16] iio: adc: sun4i-gpadc-iio: rework: support clocks and reset Philipp Rossak
2018-01-28  8:50     ` Jonathan Cameron
2018-01-28 11:34       ` Philipp Rossak
2018-01-26 15:19   ` [PATCH 06/16] iio: adc: sun4i-gpadc-iio: rework: support multible sensors Philipp Rossak
2018-01-28  9:08     ` Jonathan Cameron
2018-01-28 14:10       ` Philipp Rossak
2018-01-26 15:19   ` [PATCH 07/16] iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data Philipp Rossak
2018-01-28  9:02     ` Jonathan Cameron
2018-01-28 13:46       ` Philipp Rossak
     [not found]         ` <6ebbb5e0-eb5f-afe4-50e8-eb3cee8ec7fa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-28 13:52           ` Icenowy Zheng
     [not found]             ` <1927F24D-D3ED-49CF-9B7C-A7C8C873F0CF-h8G6r0blFSE@public.gmane.org>
2018-01-28 14:39               ` Philipp Rossak
2018-01-26 15:19   ` [PATCH 08/16] iio: adc: sun4i-gpadc-iio: rework: add interrupt support Philipp Rossak
     [not found]     ` <20180126151941.12183-9-embed3d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-28  9:06       ` Jonathan Cameron
2018-01-28 13:48         ` Philipp Rossak
2018-01-26 15:19   ` [PATCH 09/16] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor Philipp Rossak
2018-01-26 15:19   ` [PATCH 10/16] iio: adc: sun4i-gpadc-iio: add support for A83T " Philipp Rossak
     [not found]     ` <20180126151941.12183-11-embed3d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-26 17:46       ` [linux-sunxi] " Ondřej Jirman
     [not found]         ` <20180126174602.qolcswhqblgmlad7-9v8tmBix7cb9zxVx7UNMDg@public.gmane.org>
2018-01-27  4:30           ` Philipp Rossak
2018-01-26 15:19   ` [PATCH 11/16] arm: dts: sunxi-h3-h5: add support for the thermal sensor in H3 and H5 Philipp Rossak
2018-01-26 15:19   ` [PATCH 12/16] arm: dts: sun8i: h3: add support for the thermal sensor in H3 Philipp Rossak
2018-01-26 15:19   ` [PATCH 13/16] arm: dts: sun8i: h3: add thermal zone to H3 Philipp Rossak
     [not found]     ` <20180126151941.12183-14-embed3d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-26 16:26       ` Samuel Holland
     [not found]         ` <4cfb1faf-a712-c882-1ef1-55845c1bb00e-RkNLwX/CsU9g9hUCZPvPmw@public.gmane.org>
2018-01-26 17:48           ` [linux-sunxi] " Philipp Rossak
2018-01-26 15:19   ` [PATCH 14/16] arm: dts: sun8i: h3: enable H3 sid controller Philipp Rossak
2018-01-26 15:19   ` [PATCH 15/16] arm: dts: sun8i: a83t: add support for the thermal sensor in A83T Philipp Rossak
2018-01-26 15:19   ` [PATCH 16/16] arm: dts: sun8i: a83t: add thermal zone to A83T Philipp Rossak
     [not found]     ` <20180126151941.12183-17-embed3d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-01-26 16:25       ` Samuel Holland
2018-01-26 17:35         ` [linux-sunxi] " Philipp Rossak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2CC367BA-766A-4384-82EE-9A38BBFB08C2@aosc.io \
    --to=icenowy@aosc.io \
    --cc=clabbe.montjoie@gmail.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=edu.molinas@gmail.com \
    --cc=embed3d@gmail.com \
    --cc=hans.verkuil@cisco.com \
    --cc=jic23@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=krzk@kernel.org \
    --cc=lars@metafoo.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=linux@armlinux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=maxime.ripard@free-electrons.com \
    --cc=mchehab@kernel.org \
    --cc=pmeerw@pmeerw.net \
    --cc=quentin.schulz@free-electrons.com \
    --cc=rask@formelder.dk \
    --cc=robh+dt@kernel.org \
    --cc=sean@mess.org \
    --cc=singhalsimran0@gmail.com \
    --cc=wens@csie.org \
    --subject='Re: [PATCH 04/16] iio: adc: sun4i-gpadc-iio: rework: sampling start/end code readout reg' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).