| 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
| ||
|
Date: Sun, 28 Jan 2018 15:39:15 +0100
From: Philipp Rossak <embed3d@...il.com>
To: Icenowy Zheng <icenowy@...c.io>,
linux-arm-kernel@...ts.infradead.org,
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
On 28.01.2018 14:52, Icenowy Zheng wrote:
>
>
> 于 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.
>
Yes that's true, but I haven't seen a sid/nvmem driver for the a33. If
that is available, we can change this for true (same on a83t).
>>>> };
>>>>
>>>> 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