| 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 14:46:18 +0100
From: Philipp Rossak <embed3d@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lee.jones@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
maxime.ripard@...e-electrons.com, wens@...e.org,
linux@...linux.org.uk, knaack.h@....de, lars@...afoo.de,
pmeerw@...erw.net, davem@...emloft.net, hans.verkuil@...co.com,
mchehab@...nel.org, rask@...melder.dk, clabbe.montjoie@...il.com,
sean@...s.org, krzk@...nel.org, quentin.schulz@...e-electrons.com,
icenowy@...c.io, edu.molinas@...il.com, singhalsimran0@...il.com,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-sunxi@...glegroups.com
Subject: Re: [PATCH 07/16] iio: adc: sun4i-gpadc-iio: rework: support nvmem
calibration data
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,
>> };
>>
>> 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
Powered by blists - more mailing lists