Re: [PATCH 07/16] iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data

[Philipp Rossak <embed3d@gmail.com>]

On 28.01.2018 10:02, Jonathan Cameron wrote:
> On Fri, 26 Jan 2018 16:19:32 +0100
> Philipp Rossak <embed3d@gmail.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@gmail.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