| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1927F24D-D3ED-49CF-9B7C-A7C8C873F0CF@aosc.io>
Date: Sun, 28 Jan 2018 21:52:36 +0800
From: Icenowy Zheng <icenowy@...c.io>
To: linux-arm-kernel@...ts.infradead.org,
Philipp Rossak <embed3d@...il.com>,
Jonathan Cameron <jic23@...nel.org>
CC: mark.rutland@....com, sean@...s.org, linux-iio@...r.kernel.org,
linux-sunxi@...glegroups.com, clabbe.montjoie@...il.com,
pmeerw@...erw.net, lee.jones@...aro.org, lars@...afoo.de,
edu.molinas@...il.com, linux@...linux.org.uk, krzk@...nel.org,
wens@...e.org, hans.verkuil@...co.com, rask@...melder.dk,
devicetree@...r.kernel.org, mchehab@...nel.org, robh+dt@...nel.org,
singhalsimran0@...il.com, linux-kernel@...r.kernel.org,
quentin.schulz@...e-electrons.com, knaack.h@....de,
maxime.ripard@...e-electrons.com, davem@...emloft.net
Subject: Re: [PATCH 07/16] iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data
于 2018年1月28日 GMT+08:00 下午9:46:18, Philipp Rossak <embed3d@...il.com> 写到:
>
>
>On 28.01.2018 10:02, Jonathan Cameron wrote:
>> On Fri, 26 Jan 2018 16:19:32 +0100
>> Philipp Rossak <embed3d@...il.com> 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 <embed3d@...il.com>
>>
>> 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 <linux/interrupt.h>
>>> #include <linux/io.h>
>>> #include <linux/module.h>
>>> +#include <linux/nvmem-consumer.h>
>>> #include <linux/of.h>
>>> #include <linux/of_device.h>
>>> #include <linux/platform_device.h>
>>> @@ -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@...ts.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Powered by blists - more mailing lists