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

[Philipp Rossak]

/* prevents concurrent reads of temperature and ADC */
struct mutexmutex;
struct thermal_zone_device  *tzd;
@@ -561,6 +569,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(>dev);
if (!info->data)
@@ -575,6 +586,39 @@ 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 = nvmem_cell_get(>dev, "calibration");
+   if (IS_ERR(cell)) {
+   if (PTR_ERR(cell) == -EPROBE_DEFER)
+   return PTR_ERR(cell);
+   goto no_nvmem;


goto considered evil ? :)


this was a suggestion from Jonatan in version one, to make the code better
readable.


Isn't

if (info->data->supports_nvmem && IS_ERR(cell = nvmem_cell_get()))

pretty much the same thing?

Maxime


I would say :

if (info->data->supports_nvmem && !IS_ERR(cell = nvmem_cell_get())) is

the same.
This would require an else if statement like this:

else if (info->data->supports_nvmem && PTR_ERR(cell) == -EPROBE_DEFER)
return PTR_ERR(cell);

to avoid errors if the thermal sensor is probed before the sid driver.

Philipp

--
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

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

[kbuild test robot]

Hi Philipp,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.15 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Philipp-Rossak/IIO-based-thermal-sensor-driver-for-Allwinner-H3-and-A83T-SoC/20180201-043415
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/iio/adc/sun4i-gpadc-iio.c:608:53: sparse: cast to restricted __be32
>> drivers/iio/adc/sun4i-gpadc-iio.c:608:53: sparse: cast to restricted __be32
>> drivers/iio/adc/sun4i-gpadc-iio.c:608:53: sparse: cast to restricted __be32
>> drivers/iio/adc/sun4i-gpadc-iio.c:608:53: sparse: cast to restricted __be32
>> drivers/iio/adc/sun4i-gpadc-iio.c:608:53: sparse: cast to restricted __be32
>> drivers/iio/adc/sun4i-gpadc-iio.c:608:53: sparse: cast to restricted __be32
   drivers/iio/adc/sun4i-gpadc-iio.c:613:53: sparse: cast to restricted __be32
   drivers/iio/adc/sun4i-gpadc-iio.c:613:53: sparse: cast to restricted __be32
   drivers/iio/adc/sun4i-gpadc-iio.c:613:53: sparse: cast to restricted __be32
   drivers/iio/adc/sun4i-gpadc-iio.c:613:53: sparse: cast to restricted __be32
   drivers/iio/adc/sun4i-gpadc-iio.c:613:53: sparse: cast to restricted __be32
   drivers/iio/adc/sun4i-gpadc-iio.c:613:53: sparse: cast to restricted __be32

vim +608 drivers/iio/adc/sun4i-gpadc-iio.c

   564  
   565  static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
   566  struct iio_dev *indio_dev)
   567  {
   568  struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
   569  struct resource *mem;
   570  void __iomem *base;
   571  int ret;
   572  struct nvmem_cell *cell;
   573  ssize_t cell_size;
   574  u64 *cell_data;
   575  
   576  info->data = of_device_get_match_data(>dev);
   577  if (!info->data)
   578  return -ENODEV;
   579  
   580  info->no_irq = true;
   581  indio_dev->num_channels = ARRAY_SIZE(sun8i_a33_gpadc_channels);
   582  indio_dev->channels = sun8i_a33_gpadc_channels;
   583  
   584  mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   585  base = devm_ioremap_resource(>dev, mem);
   586  if (IS_ERR(base))
   587  return PTR_ERR(base);
   588  
   589  info->has_calibration_data[0] = false;
   590  info->has_calibration_data[1] = false;
   591  
   592  if (!info->data->supports_nvmem)
   593  goto no_nvmem;
   594  
   595  cell = nvmem_cell_get(>dev, "calibration");
   596  if (IS_ERR(cell)) {
   597  if (PTR_ERR(cell) == -EPROBE_DEFER)
   598  return PTR_ERR(cell);
   599  goto no_nvmem;
   600  }
   601  
   602  cell_data = (u64 *)nvmem_cell_read(cell, _size);
   603  nvmem_cell_put(cell);
   604  switch (cell_size) {
   605  case 8:
   606  case 6:
   607  info->has_calibration_data[1] = true;
 > 608  info->calibration_data[1] = be32_to_cpu(
   609  upper_32_bits(cell_data[0]));
   610  case 4:
   611  case 2:
   612  info->has_calibration_data[0] = true;
   613  info->calibration_data[0] = be32_to_cpu(
   614  lower_32_bits(cell_data[0]));
   615  break;
   616  default:
   617  break;
   618  }
   619  
   620  no_nvmem:
   621  
   622  info->regmap = devm_regmap_init_mmio(>dev, base,
   623   
_gpadc_regmap_config);
   624  if (IS_ERR(info->regmap)) {
   625  ret = PTR_ERR(info->regmap);
   626  dev_err(>dev, "failed to init regmap: %d\n", ret);
   627  return ret;
   628  }
   629  
   630  if (info->data->has_bus_rst) {
   631  info->reset = devm_reset_control_get(>dev, NULL);
   632  if (IS_ERR(info->reset)) {
   633  ret = PTR_ERR(info->reset);
   634  return ret;
   635  }
   636  
   637  ret = reset_control_deassert(info->reset);
   638  if (ret)
   639  return ret;
   640  }
   641  
   642  if (info->data->has_bus_clk) {
   643  info->bus_clk = devm_clk_get(>dev, "bus");
   644  

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

[Philipp Rossak]

On 29.01.2018 10:40, Maxime Ripard wrote:

On Mon, Jan 29, 2018 at 12:29:10AM +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.

Signed-off-by: Philipp Rossak 
---
  drivers/iio/adc/sun4i-gpadc-iio.c | 44 +++
  1 file changed, 44 insertions(+)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
b/drivers/iio/adc/sun4i-gpadc-iio.c
index ac9ad2f8232f..74eeb5cd5218 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 
@@ -74,6 +75,7 @@ struct gpadc_data {
boolhas_bus_rst;
boolhas_mod_clk;
int sensor_count;
+   boolsupports_nvmem;


I think you should add some documentation along with all the fields
you're adding.


ok I will add more informations in the next version into the commit message.




  };
  
  static const struct gpadc_data sun4i_gpadc_data = {

@@ -87,6 +89,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,


That's already its value if you leave it out.


  };
  
  static const struct gpadc_data sun5i_gpadc_data = {

@@ -100,6 +103,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 = {

@@ -113,6 +117,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 = {

@@ -123,6 +128,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 {

@@ -141,6 +147,8 @@ struct sun4i_gpadc_iio {
struct clk  *mod_clk;
struct reset_control*reset;
int sensor_id;
+   u32 calibration_data[2];
+   boolhas_calibration_data[2];


Why do you have two different values here?



I think my idea was too complex! I thought it would be better to check 
if calibration data was read, and is able to be written to hardware. 
those information were split per register.


I think a u64 should be fine for calibration_data. When I write the 
calibration data I can check on the sensor count and write only the 
lower 32 bits if there are less than 3 sensors.


Is this ok for you?



/* prevents concurrent reads of temperature and ADC */
struct mutexmutex;
struct thermal_zone_device  *tzd;
@@ -561,6 +569,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(>dev);

if (!info->data)
@@ -575,6 +586,39 @@ 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 = nvmem_cell_get(>dev, "calibration");
+   if (IS_ERR(cell)) {
+   if (PTR_ERR(cell) == -EPROBE_DEFER)
+   return PTR_ERR(cell);
+   goto no_nvmem;


goto considered evil ? :)



this was a suggestion from Jonatan in version one, to make the code 
better readable.

.

+   }
+
+   cell_data = (u64 *)nvmem_cell_read(cell, _size);
+   nvmem_cell_put(cell);
+   switch (cell_size) {
+   case 8:
+   case 6:
+   info->has_calibration_data[1] = true;
+   info->calibration_data[1] = be32_to_cpu(
+   upper_32_bits(cell_data[0]));
+   case 4:
+   case 2:
+   info->has_calibration_data[0] = true;
+   info->calibration_data[0] = be32_to_cpu(
+   lower_32_bits(cell_data[0]));


Why do you need that switch?


You are right! The calibration reg seems to be always 64 bit wide. [1]
So I will just check for the length of 

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

[Maxime Ripard]

On Mon, Jan 29, 2018 at 12:29:10AM +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.
> 
> Signed-off-by: Philipp Rossak 
> ---
>  drivers/iio/adc/sun4i-gpadc-iio.c | 44 
> +++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
> b/drivers/iio/adc/sun4i-gpadc-iio.c
> index ac9ad2f8232f..74eeb5cd5218 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 
> @@ -74,6 +75,7 @@ struct gpadc_data {
>   boolhas_bus_rst;
>   boolhas_mod_clk;
>   int sensor_count;
> + boolsupports_nvmem;

I think you should add some documentation along with all the fields
you're adding.

>  };
>  
>  static const struct gpadc_data sun4i_gpadc_data = {
> @@ -87,6 +89,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,

That's already its value if you leave it out.

>  };
>  
>  static const struct gpadc_data sun5i_gpadc_data = {
> @@ -100,6 +103,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 = {
> @@ -113,6 +117,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 = {
> @@ -123,6 +128,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 {
> @@ -141,6 +147,8 @@ struct sun4i_gpadc_iio {
>   struct clk  *mod_clk;
>   struct reset_control*reset;
>   int sensor_id;
> + u32 calibration_data[2];
> + boolhas_calibration_data[2];

Why do you have two different values here?

>   /* prevents concurrent reads of temperature and ADC */
>   struct mutexmutex;
>   struct thermal_zone_device  *tzd;
> @@ -561,6 +569,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(>dev);
>   if (!info->data)
> @@ -575,6 +586,39 @@ 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 = nvmem_cell_get(>dev, "calibration");
> + if (IS_ERR(cell)) {
> + if (PTR_ERR(cell) == -EPROBE_DEFER)
> + return PTR_ERR(cell);
> + goto no_nvmem;

goto considered evil ? :)

> + }
> +
> + cell_data = (u64 *)nvmem_cell_read(cell, _size);
> + nvmem_cell_put(cell);
> + switch (cell_size) {
> + case 8:
> + case 6:
> + info->has_calibration_data[1] = true;
> + info->calibration_data[1] = be32_to_cpu(
> + upper_32_bits(cell_data[0]));
> + case 4:
> + case 2:
> + info->has_calibration_data[0] = true;
> + info->calibration_data[0] = be32_to_cpu(
> + lower_32_bits(cell_data[0]));

Why do you need that switch?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature