From mboxrd@z Thu Jan 1 00:00:00 1970 From: Icenowy Zheng Subject: Re: [PATCH 07/16] iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data Date: Sun, 28 Jan 2018 21:52:36 +0800 Message-ID: <1927F24D-D3ED-49CF-9B7C-A7C8C873F0CF@aosc.io> References: <20180126151941.12183-1-embed3d@gmail.com> <20180126151941.12183-8-embed3d@gmail.com> <20180128090258.56e8ac93@archlinux> <6ebbb5e0-eb5f-afe4-50e8-eb3cee8ec7fa@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <6ebbb5e0-eb5f-afe4-50e8-eb3cee8ec7fa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Philipp Rossak , Jonathan Cameron Cc: mark.rutland-5wv7dgnIgG8@public.gmane.org, sean-hENCXIMQXOg@public.gmane.org, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org, lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org, edu.molinas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org, krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, wens-jdAy2FN1RRM@public.gmane.org, hans.verkuil-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org, rask-SivP7zSAdNDZaaYASwVUlg@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, mchehab-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, singhalsimran0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org, knaack.h-Mmb7MZpHnFY@public.gmane.org, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org List-Id: devicetree@vger.kernel.org 于 2018年1月28日 GMT+08:00 下午9:46:18, Philipp Rossak 写到: > > >On 28.01.2018 10:02, Jonathan Cameron wrote: >> On Fri, 26 Jan 2018 16:19:32 +0100 >> Philipp Rossak wrote: >> >>> This patch reworks the driver to support nvmem calibration cells. >>> The driver checks if the nvmem calibration is supported and reads >out >>> the nvmem. At the beginning of the startup process the calibration >data >>> is written to the related registers. >>> >>> Signed-off-by: Philipp Rossak >> >> A few minor suggestions inline. >> >> Jonathan >> >>> --- >>> drivers/iio/adc/sun4i-gpadc-iio.c | 52 >+++++++++++++++++++++++++++++++++++++++ >>> include/linux/mfd/sun4i-gpadc.h | 2 ++ >>> 2 files changed, 54 insertions(+) >>> >>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c >b/drivers/iio/adc/sun4i-gpadc-iio.c >>> index bff06f2798e8..7b12666cdd9e 100644 >>> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >>> @@ -27,6 +27,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -81,6 +82,7 @@ struct gpadc_data { >>> bool has_bus_rst; >>> bool has_mod_clk; >>> int sensor_count; >>> + bool supports_nvmem; >>> }; >>> >>> static const struct gpadc_data sun4i_gpadc_data = { >>> @@ -94,6 +96,7 @@ static const struct gpadc_data sun4i_gpadc_data = >{ >>> .sample_start = sun4i_gpadc_sample_start, >>> .sample_end = sun4i_gpadc_sample_end, >>> .sensor_count = 1, >>> + .supports_nvmem = false, >>> }; >>> >>> static const struct gpadc_data sun5i_gpadc_data = { >>> @@ -107,6 +110,7 @@ static const struct gpadc_data sun5i_gpadc_data >= { >>> .sample_start = sun4i_gpadc_sample_start, >>> .sample_end = sun4i_gpadc_sample_end, >>> .sensor_count = 1, >>> + .supports_nvmem = false, >>> }; >>> >>> static const struct gpadc_data sun6i_gpadc_data = { >>> @@ -120,6 +124,7 @@ static const struct gpadc_data sun6i_gpadc_data >= { >>> .sample_start = sun4i_gpadc_sample_start, >>> .sample_end = sun4i_gpadc_sample_end, >>> .sensor_count = 1, >>> + .supports_nvmem = false, >>> }; >>> >>> static const struct gpadc_data sun8i_a33_gpadc_data = { >>> @@ -130,6 +135,7 @@ static const struct gpadc_data >sun8i_a33_gpadc_data = { >>> .sample_start = sun4i_gpadc_sample_start, >>> .sample_end = sun4i_gpadc_sample_end, >>> .sensor_count = 1, >>> + .supports_nvmem = false, BTW A33 claims to support calibration data according to the manual. >>> }; >>> >>> struct sun4i_gpadc_iio { >>> @@ -148,6 +154,8 @@ struct sun4i_gpadc_iio { >>> struct clk *mod_clk; >>> struct reset_control *reset; >>> int sensor_id; >>> + u32 calibration_data[2]; >>> + bool has_calibration_data[2]; >>> /* prevents concurrent reads of temperature and ADC */ >>> struct mutex mutex; >>> struct thermal_zone_device *tzd; >>> @@ -459,6 +467,17 @@ static int sun4i_gpadc_runtime_suspend(struct >device *dev) >>> return info->data->sample_end(info); >>> } >>> >>> +static void sunxi_calibrate(struct sun4i_gpadc_iio *info) >>> +{ >>> + if (info->has_calibration_data[0]) >>> + regmap_write(info->regmap, SUNXI_THS_CDATA_0_1, >>> + info->calibration_data[0]); >>> + >>> + if (info->has_calibration_data[1]) >>> + regmap_write(info->regmap, SUNXI_THS_CDATA_2_3, >>> + info->calibration_data[1]); >>> +} >>> + >>> static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info) >>> { >>> /* clkin = 6MHz */ >>> @@ -481,6 +500,7 @@ static int sun4i_gpadc_sample_start(struct >sun4i_gpadc_iio *info) >>> static int sunxi_ths_sample_start(struct sun4i_gpadc_iio *info) >>> { >>> u32 value; >>> + sunxi_calibrate(info); >>> >>> if (info->data->ctrl0_map) >>> regmap_write(info->regmap, SUNXI_THS_CTRL0, >>> @@ -602,6 +622,9 @@ static int sun4i_gpadc_probe_dt(struct >platform_device *pdev, >>> struct resource *mem; >>> void __iomem *base; >>> int ret; >>> + struct nvmem_cell *cell; >>> + ssize_t cell_size; >>> + u64 *cell_data; >>> >>> info->data = of_device_get_match_data(&pdev->dev); >>> if (!info->data) >>> @@ -616,6 +639,35 @@ static int sun4i_gpadc_probe_dt(struct >platform_device *pdev, >>> if (IS_ERR(base)) >>> return PTR_ERR(base); >>> >>> + info->has_calibration_data[0] = false; >>> + info->has_calibration_data[1] = false; >>> + >>> + if (!info->data->supports_nvmem) >>> + goto no_nvmem; >>> + >>> + cell = devm_nvmem_cell_get(&pdev->dev, "calibration"); >>> + if (IS_ERR(cell)) { >>> + if (PTR_ERR(cell) == -EPROBE_DEFER) >>> + return PTR_ERR(cell); >> Use a goto here to no_nvmem. >> >> Then you can drop the below else to make things more readable. >>> + } else { >>> + cell_data = (u64 *)nvmem_cell_read(cell, &cell_size); >>> + devm_nvmem_cell_put(&pdev->dev, cell); >> >> I'm really not keen on use of devm for things we are intending >> to drop almost immediately. Just use the non managed versions >> and clean up properly in the error paths (if there are any >> where it is needed - which there aren't that I can see) >> > >^^ Ok, I will rework that. > >>> + if (cell_size <= 4) { >> Is it valid if it is anything other than 4? > >For sensors with only one sensor would be also a 2 valid, for those >with >2 sensors ==> 4, with 3 sensors ==> 6 and with and with 4 sensors ==> >8. > >In hardware we have in total to 32bit registers for calibration, thus I > >thought it would be a good idea to use always four bytes per register. >If two bytes are used, they should be the default value. > >But I agree, this needs a rework. > >>> + info->has_calibration_data[0] = true; >>> + info->calibration_data[0] = be32_to_cpu(cell_data[0] & >>> + GENMASK(31, 0)); >> >> Masking isn't needed, If you want to be paranoid there is the >lower_32_bits >> function.. >> >^^ Ok, I will rework that. >>> + } else if (cell_size <= 8) { >>> + info->has_calibration_data[0] = true; >>> + info->calibration_data[0] = be32_to_cpu(cell_data[0] & >>> + GENMASK(31, 0)); >> >> This first block is repeated. Easy enough to avoid I think... >> > >^^ Ok, I will rework that. > >>> + info->has_calibration_data[1] = true; >>> + info->calibration_data[1] = be32_to_cpu( >>> + (cell_data[0] >> 32) & GENMASK(31, 0)); >>> + } >>> + } >>> + >>> +no_nvmem: >>> + >>> info->regmap = devm_regmap_init_mmio(&pdev->dev, base, >>> &sun4i_gpadc_regmap_config); >>> if (IS_ERR(info->regmap)) { >>> diff --git a/include/linux/mfd/sun4i-gpadc.h >b/include/linux/mfd/sun4i-gpadc.h >>> index 40b4dd9d2405..c251002431bd 100644 >>> --- a/include/linux/mfd/sun4i-gpadc.h >>> +++ b/include/linux/mfd/sun4i-gpadc.h >>> @@ -90,6 +90,8 @@ >>> #define SUNXI_THS_CTRL0 0x00 >>> #define SUNXI_THS_CTRL2 0x40 >>> #define SUNXI_THS_FILTER 0x70 >>> +#define SUNXI_THS_CDATA_0_1 0x74 >>> +#define SUNXI_THS_CDATA_2_3 0x78 >>> #define SUNXI_THS_TDATA0 0x80 >>> #define SUNXI_THS_TDATA1 0x84 >>> #define SUNXI_THS_TDATA2 0x88 >> >Thanks, >Philipp > >_______________________________________________ >linux-arm-kernel mailing list >linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel