[PATCH 0/4] iio: magnetometer: ak8974: support AMI306

[PATCH 0/4] iio: magnetometer: ak8974: support AMI306

[Michał Mirosław]

This series adds support for AMI306 magnetometer. The device is very
similar to already supported AMI305. Calibration data is not used
yet as I still need to figure out what it means.

Michał Mirosław (4):
  iio: magnetometer: ak8974: support AMI306 variant
  iio: magnetometer: ak8974: add_device_randomness(fw + serial)
  iio: magnetometer: ak8974: mark INT_CLEAR as precious
  iio: magnetometer: ak8974: debug AMI306 calibration data

 drivers/iio/magnetometer/Kconfig  |   4 +-
 drivers/iio/magnetometer/ak8974.c | 134 +++++++++++++++++++++++++++++++++-----
 2 files changed, 119 insertions(+), 19 deletions(-)

-- 
2.11.0

[PATCH 1/4] iio: magnetometer: ak8974: support AMI306 variant

[Michał Mirosław]

Add support for AMI306 magnetometer - very similar to AMI305.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/iio/magnetometer/Kconfig  |  4 +-
 drivers/iio/magnetometer/ak8974.c | 90 ++++++++++++++++++++++++++++++---------
 2 files changed, 72 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 421ad90a5fbe..ed9d776d01af 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -13,8 +13,8 @@ config AK8974
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
-	  Say yes here to build support for Asahi Kasei AK8974 or
-	  AMI305 I2C-based 3-axis magnetometer chips.
+	  Say yes here to build support for Asahi Kasei AK8974, AMI305 or
+	  AMI306 I2C-based 3-axis magnetometer chips.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called ak8974.
diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index e13370dc9b1c..76091da20a0c 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -36,7 +36,7 @@
  * and MSB is at the next higher address.
  */
 
-/* These registers are common for AK8974 and AMI305 */
+/* These registers are common for AK8974 and AMI30x */
 #define AK8974_SELFTEST		0x0C
 #define AK8974_SELFTEST_IDLE	0x55
 #define AK8974_SELFTEST_OK	0xAA
@@ -44,6 +44,7 @@
 #define AK8974_INFO		0x0D
 
 #define AK8974_WHOAMI		0x0F
+#define AK8974_WHOAMI_VALUE_AMI306 0x46
 #define AK8974_WHOAMI_VALUE_AMI305 0x47
 #define AK8974_WHOAMI_VALUE_AK8974 0x48
 
@@ -73,6 +74,35 @@
 #define AK8974_TEMP		0x31
 #define AMI305_TEMP		0x60
 
+/* AMI306-specific control register */
+#define AMI306_CTRL4		0x5C
+
+/* AMI306 factory calibration data */
+
+/* fine axis sensitivity */
+#define AMI306_FINEOUTPUT_X	0x90
+#define AMI306_FINEOUTPUT_Y	0x92
+#define AMI306_FINEOUTPUT_Z	0x94
+
+/* axis sensitivity */
+#define AMI306_SENS_X		0x96
+#define AMI306_SENS_Y		0x98
+#define AMI306_SENS_Z		0x9A
+
+/* axis cross-interference */
+#define AMI306_GAIN_PARA_XZ	0x9C
+#define AMI306_GAIN_PARA_XY	0x9D
+#define AMI306_GAIN_PARA_YZ	0x9E
+#define AMI306_GAIN_PARA_YX	0x9F
+#define AMI306_GAIN_PARA_ZY	0xA0
+#define AMI306_GAIN_PARA_ZX	0xA1
+
+/* offset at ZERO magnetic field */
+#define AMI306_OFFZERO_X	0xF8
+#define AMI306_OFFZERO_Y	0xFA
+#define AMI306_OFFZERO_Z	0xFC
+
+
 #define AK8974_INT_X_HIGH	BIT(7) /* Axis over +threshold  */
 #define AK8974_INT_Y_HIGH	BIT(6)
 #define AK8974_INT_Z_HIGH	BIT(5)
@@ -158,6 +188,26 @@ struct ak8974 {
 static const char ak8974_reg_avdd[] = "avdd";
 static const char ak8974_reg_dvdd[] = "dvdd";
 
+static int ak8974_get_u16_val(struct ak8974 *ak8974, u8 reg, u16 *val)
+{
+	int ret;
+	__le16 bulk;
+
+	ret = regmap_bulk_read(ak8974->map, reg, &bulk, 2);
+	if (ret)
+		return ret;
+	*val = le16_to_cpu(bulk);
+
+	return 0;
+}
+
+static int ak8974_set_u16_val(struct ak8974 *ak8974, u8 reg, u16 val)
+{
+	__le16 bulk = cpu_to_le16(val);
+
+	return regmap_bulk_write(ak8974->map, reg, &bulk, 2);
+}
+
 static int ak8974_set_power(struct ak8974 *ak8974, bool mode)
 {
 	int ret;
@@ -209,6 +259,12 @@ static int ak8974_configure(struct ak8974 *ak8974)
 	ret = regmap_write(ak8974->map, AK8974_CTRL3, 0);
 	if (ret)
 		return ret;
+	if (ak8974->variant == AK8974_WHOAMI_VALUE_AMI306) {
+		/* magic from datasheet: set high-speed measurement mode */
+		ret = ak8974_set_u16_val(ak8974, AMI306_CTRL4, 0xA07E);
+		if (ret)
+			return ret;
+	}
 	ret = regmap_write(ak8974->map, AK8974_INT_CTRL, AK8974_INT_CTRL_POL);
 	if (ret)
 		return ret;
@@ -388,19 +444,6 @@ static int ak8974_selftest(struct ak8974 *ak8974)
 	return 0;
 }
 
-static int ak8974_get_u16_val(struct ak8974 *ak8974, u8 reg, u16 *val)
-{
-	int ret;
-	__le16 bulk;
-
-	ret = regmap_bulk_read(ak8974->map, reg, &bulk, 2);
-	if (ret)
-		return ret;
-	*val = le16_to_cpu(bulk);
-
-	return 0;
-}
-
 static int ak8974_detect(struct ak8974 *ak8974)
 {
 	unsigned int whoami;
@@ -413,9 +456,13 @@ static int ak8974_detect(struct ak8974 *ak8974)
 	if (ret)
 		return ret;
 
+	name = "ami305";
+
 	switch (whoami) {
+	case AK8974_WHOAMI_VALUE_AMI306:
+		name = "ami306";
+		/* fall-through */
 	case AK8974_WHOAMI_VALUE_AMI305:
-		name = "ami305";
 		ret = regmap_read(ak8974->map, AMI305_VER, &fw);
 		if (ret)
 			return ret;
@@ -602,9 +649,11 @@ static bool ak8974_writeable_reg(struct device *dev, unsigned int reg)
 	case AMI305_OFFSET_Y + 1:
 	case AMI305_OFFSET_Z:
 	case AMI305_OFFSET_Z + 1:
-		if (ak8974->variant == AK8974_WHOAMI_VALUE_AMI305)
-			return true;
-		return false;
+		return ak8974->variant == AK8974_WHOAMI_VALUE_AMI305 ||
+		       ak8974->variant == AK8974_WHOAMI_VALUE_AMI306;
+	case AMI306_CTRL4:
+	case AMI306_CTRL4 + 1:
+		return ak8974->variant == AK8974_WHOAMI_VALUE_AMI306;
 	default:
 		return false;
 	}
@@ -678,7 +727,7 @@ static int ak8974_probe(struct i2c_client *i2c,
 
 	ret = ak8974_detect(ak8974);
 	if (ret) {
-		dev_err(&i2c->dev, "neither AK8974 nor AMI305 found\n");
+		dev_err(&i2c->dev, "neither AK8974 nor AMI30x found\n");
 		goto power_off;
 	}
 
@@ -827,6 +876,7 @@ static const struct dev_pm_ops ak8974_dev_pm_ops = {
 
 static const struct i2c_device_id ak8974_id[] = {
 	{"ami305", 0 },
+	{"ami306", 0 },
 	{"ak8974", 0 },
 	{}
 };
@@ -850,7 +900,7 @@ static struct i2c_driver ak8974_driver = {
 };
 module_i2c_driver(ak8974_driver);
 
-MODULE_DESCRIPTION("AK8974 and AMI305 3-axis magnetometer driver");
+MODULE_DESCRIPTION("AK8974 and AMI30x 3-axis magnetometer driver");
 MODULE_AUTHOR("Samu Onkalo");
 MODULE_AUTHOR("Linus Walleij");
 MODULE_LICENSE("GPL v2");
-- 
2.11.0

[PATCH 2/4] iio: magnetometer: ak8974: add_device_randomness(fw + serial)

[Michał Mirosław]

Mix device-specific data into randomness pool.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/iio/magnetometer/ak8974.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index 76091da20a0c..ab204f8560e3 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -20,6 +20,7 @@
 #include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/bitops.h>
+#include <linux/random.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
@@ -466,10 +467,12 @@ static int ak8974_detect(struct ak8974 *ak8974)
 		ret = regmap_read(ak8974->map, AMI305_VER, &fw);
 		if (ret)
 			return ret;
+		add_device_randomness(&fw, sizeof(fw));
 		fw &= 0x7f; /* only bits 0 thru 6 valid */
 		ret = ak8974_get_u16_val(ak8974, AMI305_SN, &sn);
 		if (ret)
 			return ret;
+		add_device_randomness(&sn, sizeof(sn));
 		dev_info(&ak8974->i2c->dev,
 			 "detected %s, FW ver %02x, S/N: %04x\n",
 			 name, fw, sn);
-- 
2.11.0

[PATCH 3/4] iio: magnetometer: ak8974: mark INT_CLEAR as precious

[Michał Mirosław]

Reading INT_CLEAR has side effects - disallow reading it via debugfs.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/iio/magnetometer/ak8974.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index ab204f8560e3..d8c45206ba00 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -662,11 +662,17 @@ static bool ak8974_writeable_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static bool ak8974_precious_reg(struct device *dev, unsigned int reg)
+{
+	return reg == AK8974_INT_CLEAR;
+}
+
 static const struct regmap_config ak8974_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.max_register = 0xff,
 	.writeable_reg = ak8974_writeable_reg,
+	.precious_reg = ak8974_precious_reg,
 };
 
 static int ak8974_probe(struct i2c_client *i2c,
-- 
2.11.0

[PATCH 4/4] iio: magnetometer: ak8974: debug AMI306 calibration data

[Michał Mirosław]

Use AMI306 calibration data for randomness and debugging.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/iio/magnetometer/ak8974.c | 41 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index d8c45206ba00..b3221b34606b 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -445,6 +445,20 @@ static int ak8974_selftest(struct ak8974 *ak8974)
 	return 0;
 }
 
+static void ak8974_read_calib_data(struct ak8974 *ak8974, unsigned int reg,
+				   __le16 *tab, size_t tab_size)
+{
+	int ret = regmap_bulk_read(ak8974->map, reg, tab, tab_size);
+	if (ret) {
+		memset(tab, 0xFF, tab_size);
+		dev_warn(&ak8974->i2c->dev,
+			 "can't read calibration data (regs %u..%u): %d\n",
+			 reg, reg + tab_size - 1, ret);
+	} else {
+		add_device_randomness(tab, tab_size);
+	}
+}
+
 static int ak8974_detect(struct ak8974 *ak8974)
 {
 	unsigned int whoami;
@@ -490,6 +504,33 @@ static int ak8974_detect(struct ak8974 *ak8974)
 	ak8974->name = name;
 	ak8974->variant = whoami;
 
+	if (whoami == AK8974_WHOAMI_VALUE_AMI306) {
+		__le16 fab_data1[9], fab_data2[3];
+		int i;
+
+		ak8974_read_calib_data(ak8974, AMI306_FINEOUTPUT_X,
+				       fab_data1, sizeof(fab_data1));
+		ak8974_read_calib_data(ak8974, AMI306_OFFZERO_X,
+				       fab_data2, sizeof(fab_data2));
+
+		for (i = 0; i < 3; ++i) {
+			static const char axis[3] = "XYZ";
+			static const char pgaxis[6] = "ZYZXYX";
+			unsigned offz = le16_to_cpu(fab_data2[i]) & 0x7F;
+			unsigned fine = le16_to_cpu(fab_data1[i]);
+			unsigned sens = le16_to_cpu(fab_data1[i + 3]);
+			unsigned pgain1 = le16_to_cpu(fab_data1[i + 6]);
+			unsigned pgain2 = pgain1 >> 8;
+
+			pgain1 &= 0xFF;
+
+			dev_info(&ak8974->i2c->dev,
+				 "factory calibration for axis %c: offz=%u sens=%u fine=%u pga%c=%u pga%c=%u\n",
+				 axis[i], offz, sens, fine, pgaxis[i * 2],
+				 pgain1, pgaxis[i * 2 + 1], pgain2);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.11.0

Re: [PATCH 0/4] iio: magnetometer: ak8974: support AMI306

[Linus Walleij]

On Thu, Aug 17, 2017 at 3:56 PM, Micha=C5=82 Miros=C5=82aw
<mirq-linux@rere.qmqm.pl> wrote:

> This series adds support for AMI306 magnetometer. The device is very
> similar to already supported AMI305.

All four patches:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Very clear and to the point. Nice to see some wider use of this :)

> Calibration data is not used
> yet as I still need to figure out what it means.

Does your datasheet tell you how to convert to ISO units? Right now
the sensor only suppport raw data and it would be nice to support
IIO_CHAN_INFO_SCALE like e.g. ak8975.c does.

I bet the same scaling applies as to the other variants.

Yours,
Linus Walleij

Re: [PATCH 1/4] iio: magnetometer: ak8974: support AMI306 variant

[Jonathan Cameron]

On Thu, 17 Aug 2017 15:56:10 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> Add support for AMI306 magnetometer - very similar to AMI305.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/magnetometer/Kconfig  |  4 +-
>  drivers/iio/magnetometer/ak8974.c | 90 ++++++++++++++++++++++++++++++---------
>  2 files changed, 72 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 421ad90a5fbe..ed9d776d01af 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -13,8 +13,8 @@ config AK8974
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  	help
> -	  Say yes here to build support for Asahi Kasei AK8974 or
> -	  AMI305 I2C-based 3-axis magnetometer chips.
> +	  Say yes here to build support for Asahi Kasei AK8974, AMI305 or
> +	  AMI306 I2C-based 3-axis magnetometer chips.
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called ak8974.
> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> index e13370dc9b1c..76091da20a0c 100644
> --- a/drivers/iio/magnetometer/ak8974.c
> +++ b/drivers/iio/magnetometer/ak8974.c
> @@ -36,7 +36,7 @@
>   * and MSB is at the next higher address.
>   */
>  
> -/* These registers are common for AK8974 and AMI305 */
> +/* These registers are common for AK8974 and AMI30x */
>  #define AK8974_SELFTEST		0x0C
>  #define AK8974_SELFTEST_IDLE	0x55
>  #define AK8974_SELFTEST_OK	0xAA
> @@ -44,6 +44,7 @@
>  #define AK8974_INFO		0x0D
>  
>  #define AK8974_WHOAMI		0x0F
> +#define AK8974_WHOAMI_VALUE_AMI306 0x46
>  #define AK8974_WHOAMI_VALUE_AMI305 0x47
>  #define AK8974_WHOAMI_VALUE_AK8974 0x48
>  
> @@ -73,6 +74,35 @@
>  #define AK8974_TEMP		0x31
>  #define AMI305_TEMP		0x60
>  
> +/* AMI306-specific control register */
> +#define AMI306_CTRL4		0x5C
> +
> +/* AMI306 factory calibration data */
> +
> +/* fine axis sensitivity */
> +#define AMI306_FINEOUTPUT_X	0x90
> +#define AMI306_FINEOUTPUT_Y	0x92
> +#define AMI306_FINEOUTPUT_Z	0x94
> +
> +/* axis sensitivity */
> +#define AMI306_SENS_X		0x96
> +#define AMI306_SENS_Y		0x98
> +#define AMI306_SENS_Z		0x9A
> +
> +/* axis cross-interference */
> +#define AMI306_GAIN_PARA_XZ	0x9C
> +#define AMI306_GAIN_PARA_XY	0x9D
> +#define AMI306_GAIN_PARA_YZ	0x9E
> +#define AMI306_GAIN_PARA_YX	0x9F
> +#define AMI306_GAIN_PARA_ZY	0xA0
> +#define AMI306_GAIN_PARA_ZX	0xA1
> +
> +/* offset at ZERO magnetic field */
> +#define AMI306_OFFZERO_X	0xF8
> +#define AMI306_OFFZERO_Y	0xFA
> +#define AMI306_OFFZERO_Z	0xFC
> +
> +
>  #define AK8974_INT_X_HIGH	BIT(7) /* Axis over +threshold  */
>  #define AK8974_INT_Y_HIGH	BIT(6)
>  #define AK8974_INT_Z_HIGH	BIT(5)
> @@ -158,6 +188,26 @@ struct ak8974 {
>  static const char ak8974_reg_avdd[] = "avdd";
>  static const char ak8974_reg_dvdd[] = "dvdd";
>  
> +static int ak8974_get_u16_val(struct ak8974 *ak8974, u8 reg, u16 *val)
> +{
> +	int ret;
> +	__le16 bulk;
> +
> +	ret = regmap_bulk_read(ak8974->map, reg, &bulk, 2);
> +	if (ret)
> +		return ret;
> +	*val = le16_to_cpu(bulk);
> +
> +	return 0;
> +}
> +
> +static int ak8974_set_u16_val(struct ak8974 *ak8974, u8 reg, u16 val)
> +{
> +	__le16 bulk = cpu_to_le16(val);
> +
> +	return regmap_bulk_write(ak8974->map, reg, &bulk, 2);
> +}
> +
>  static int ak8974_set_power(struct ak8974 *ak8974, bool mode)
>  {
>  	int ret;
> @@ -209,6 +259,12 @@ static int ak8974_configure(struct ak8974 *ak8974)
>  	ret = regmap_write(ak8974->map, AK8974_CTRL3, 0);
>  	if (ret)
>  		return ret;
> +	if (ak8974->variant == AK8974_WHOAMI_VALUE_AMI306) {
> +		/* magic from datasheet: set high-speed measurement mode */
> +		ret = ak8974_set_u16_val(ak8974, AMI306_CTRL4, 0xA07E);
> +		if (ret)
> +			return ret;
> +	}
>  	ret = regmap_write(ak8974->map, AK8974_INT_CTRL, AK8974_INT_CTRL_POL);
>  	if (ret)
>  		return ret;
> @@ -388,19 +444,6 @@ static int ak8974_selftest(struct ak8974 *ak8974)
>  	return 0;
>  }
>  
> -static int ak8974_get_u16_val(struct ak8974 *ak8974, u8 reg, u16 *val)
> -{
> -	int ret;
> -	__le16 bulk;
> -
> -	ret = regmap_bulk_read(ak8974->map, reg, &bulk, 2);
> -	if (ret)
> -		return ret;
> -	*val = le16_to_cpu(bulk);
> -
> -	return 0;
> -}
> -
>  static int ak8974_detect(struct ak8974 *ak8974)
>  {
>  	unsigned int whoami;
> @@ -413,9 +456,13 @@ static int ak8974_detect(struct ak8974 *ak8974)
>  	if (ret)
>  		return ret;
>  
> +	name = "ami305";
> +
>  	switch (whoami) {
> +	case AK8974_WHOAMI_VALUE_AMI306:
> +		name = "ami306";
> +		/* fall-through */
>  	case AK8974_WHOAMI_VALUE_AMI305:
> -		name = "ami305";
>  		ret = regmap_read(ak8974->map, AMI305_VER, &fw);
>  		if (ret)
>  			return ret;
> @@ -602,9 +649,11 @@ static bool ak8974_writeable_reg(struct device *dev, unsigned int reg)
>  	case AMI305_OFFSET_Y + 1:
>  	case AMI305_OFFSET_Z:
>  	case AMI305_OFFSET_Z + 1:
> -		if (ak8974->variant == AK8974_WHOAMI_VALUE_AMI305)
> -			return true;
> -		return false;
> +		return ak8974->variant == AK8974_WHOAMI_VALUE_AMI305 ||
> +		       ak8974->variant == AK8974_WHOAMI_VALUE_AMI306;
> +	case AMI306_CTRL4:
> +	case AMI306_CTRL4 + 1:
> +		return ak8974->variant == AK8974_WHOAMI_VALUE_AMI306;
>  	default:
>  		return false;
>  	}
> @@ -678,7 +727,7 @@ static int ak8974_probe(struct i2c_client *i2c,
>  
>  	ret = ak8974_detect(ak8974);
>  	if (ret) {
> -		dev_err(&i2c->dev, "neither AK8974 nor AMI305 found\n");
> +		dev_err(&i2c->dev, "neither AK8974 nor AMI30x found\n");
>  		goto power_off;
>  	}
>  
> @@ -827,6 +876,7 @@ static const struct dev_pm_ops ak8974_dev_pm_ops = {
>  
>  static const struct i2c_device_id ak8974_id[] = {
>  	{"ami305", 0 },
> +	{"ami306", 0 },
>  	{"ak8974", 0 },
>  	{}
>  };
> @@ -850,7 +900,7 @@ static struct i2c_driver ak8974_driver = {
>  };
>  module_i2c_driver(ak8974_driver);
>  
> -MODULE_DESCRIPTION("AK8974 and AMI305 3-axis magnetometer driver");
> +MODULE_DESCRIPTION("AK8974 and AMI30x 3-axis magnetometer driver");
>  MODULE_AUTHOR("Samu Onkalo");
>  MODULE_AUTHOR("Linus Walleij");
>  MODULE_LICENSE("GPL v2");

Re: [PATCH 2/4] iio: magnetometer: ak8974: add_device_randomness(fw + serial)

[Jonathan Cameron]

On Thu, 17 Aug 2017 15:56:11 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> Mix device-specific data into randomness pool.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Hmm. I wondered about the firmware version as a source of randomness, but
will leave it in here.

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
>  drivers/iio/magnetometer/ak8974.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> index 76091da20a0c..ab204f8560e3 100644
> --- a/drivers/iio/magnetometer/ak8974.c
> +++ b/drivers/iio/magnetometer/ak8974.c
> @@ -20,6 +20,7 @@
>  #include <linux/mutex.h>
>  #include <linux/delay.h>
>  #include <linux/bitops.h>
> +#include <linux/random.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
> @@ -466,10 +467,12 @@ static int ak8974_detect(struct ak8974 *ak8974)
>  		ret = regmap_read(ak8974->map, AMI305_VER, &fw);
>  		if (ret)
>  			return ret;
> +		add_device_randomness(&fw, sizeof(fw));

The firmware version seems rather to guessable to really count as randomness.
presumably it would constant for a particular batch of chips.


>  		fw &= 0x7f; /* only bits 0 thru 6 valid */
>  		ret = ak8974_get_u16_val(ak8974, AMI305_SN, &sn);
>  		if (ret)
>  			return ret;
> +		add_device_randomness(&sn, sizeof(sn));

This is better.  The serial number should be hard to guess on a particular
part.

Jonathan
>  		dev_info(&ak8974->i2c->dev,
>  			 "detected %s, FW ver %02x, S/N: %04x\n",
>  			 name, fw, sn);

Re: [PATCH 4/4] iio: magnetometer: ak8974: debug AMI306 calibration data

[Jonathan Cameron]

On Thu, 17 Aug 2017 15:56:12 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> Use AMI306 calibration data for randomness and debugging.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/magnetometer/ak8974.c | 41 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> index d8c45206ba00..b3221b34606b 100644
> --- a/drivers/iio/magnetometer/ak8974.c
> +++ b/drivers/iio/magnetometer/ak8974.c
> @@ -445,6 +445,20 @@ static int ak8974_selftest(struct ak8974 *ak8974)
>  	return 0;
>  }
>  
> +static void ak8974_read_calib_data(struct ak8974 *ak8974, unsigned int reg,
> +				   __le16 *tab, size_t tab_size)
> +{
> +	int ret = regmap_bulk_read(ak8974->map, reg, tab, tab_size);
> +	if (ret) {
> +		memset(tab, 0xFF, tab_size);
> +		dev_warn(&ak8974->i2c->dev,
> +			 "can't read calibration data (regs %u..%u): %d\n",
> +			 reg, reg + tab_size - 1, ret);
> +	} else {
> +		add_device_randomness(tab, tab_size);
> +	}
> +}
> +
>  static int ak8974_detect(struct ak8974 *ak8974)
>  {
>  	unsigned int whoami;
> @@ -490,6 +504,33 @@ static int ak8974_detect(struct ak8974 *ak8974)
>  	ak8974->name = name;
>  	ak8974->variant = whoami;
>  
> +	if (whoami == AK8974_WHOAMI_VALUE_AMI306) {
> +		__le16 fab_data1[9], fab_data2[3];
> +		int i;
> +
> +		ak8974_read_calib_data(ak8974, AMI306_FINEOUTPUT_X,
> +				       fab_data1, sizeof(fab_data1));
> +		ak8974_read_calib_data(ak8974, AMI306_OFFZERO_X,
> +				       fab_data2, sizeof(fab_data2));
> +
> +		for (i = 0; i < 3; ++i) {
> +			static const char axis[3] = "XYZ";
> +			static const char pgaxis[6] = "ZYZXYX";
> +			unsigned offz = le16_to_cpu(fab_data2[i]) & 0x7F;
> +			unsigned fine = le16_to_cpu(fab_data1[i]);
> +			unsigned sens = le16_to_cpu(fab_data1[i + 3]);
> +			unsigned pgain1 = le16_to_cpu(fab_data1[i + 6]);
> +			unsigned pgain2 = pgain1 >> 8;
> +
> +			pgain1 &= 0xFF;
> +
> +			dev_info(&ak8974->i2c->dev,
> +				 "factory calibration for axis %c: offz=%u sens=%u fine=%u pga%c=%u pga%c=%u\n",
> +				 axis[i], offz, sens, fine, pgaxis[i * 2],
> +				 pgain1, pgaxis[i * 2 + 1], pgain2);
> +		}
> +	}
> +
>  	return 0;
>  }
>  

Re: [PATCH 4/4] iio: magnetometer: ak8974: debug AMI306 calibration data

[Jonathan Cameron]

On Sun, 20 Aug 2017 11:25:52 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 17 Aug 2017 15:56:12 +0200
> Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> 
> > Use AMI306 calibration data for randomness and debugging.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>  
> Applied to the togreg branch of iio.git and pushed out as testing for the
> autobuilders to play with it.
> 
> Thanks,
> 
> Jonathan
> > ---
> >  drivers/iio/magnetometer/ak8974.c | 41 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> > index d8c45206ba00..b3221b34606b 100644
> > --- a/drivers/iio/magnetometer/ak8974.c
> > +++ b/drivers/iio/magnetometer/ak8974.c
> > @@ -445,6 +445,20 @@ static int ak8974_selftest(struct ak8974 *ak8974)
> >  	return 0;
> >  }
> >  
> > +static void ak8974_read_calib_data(struct ak8974 *ak8974, unsigned int reg,
> > +				   __le16 *tab, size_t tab_size)
> > +{
> > +	int ret = regmap_bulk_read(ak8974->map, reg, tab, tab_size);
> > +	if (ret) {
> > +		memset(tab, 0xFF, tab_size);
> > +		dev_warn(&ak8974->i2c->dev,
> > +			 "can't read calibration data (regs %u..%u): %d\n",
> > +			 reg, reg + tab_size - 1, ret);
Slight tweak made to avoid the warning:

drivers/iio/magnetometer/ak8974.c: In function ‘ak8974_read_calib_data’:
drivers/iio/magnetometer/ak8974.c:455:45: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 4 has type ‘size_t {aka long unsigned int}’ [-Wformat=]
     "can't read calibration data (regs %u..%u): %d\n",
                                            ~^
                                            %lu
     reg, reg + tab_size - 1, ret);
          ~~~~~~~~~~~~~~~~~~                  

Did the obvious ;)

Jonathan

> > +	} else {
> > +		add_device_randomness(tab, tab_size);
> > +	}
> > +}
> > +
> >  static int ak8974_detect(struct ak8974 *ak8974)
> >  {
> >  	unsigned int whoami;
> > @@ -490,6 +504,33 @@ static int ak8974_detect(struct ak8974 *ak8974)
> >  	ak8974->name = name;
> >  	ak8974->variant = whoami;
> >  
> > +	if (whoami == AK8974_WHOAMI_VALUE_AMI306) {
> > +		__le16 fab_data1[9], fab_data2[3];
> > +		int i;
> > +
> > +		ak8974_read_calib_data(ak8974, AMI306_FINEOUTPUT_X,
> > +				       fab_data1, sizeof(fab_data1));
> > +		ak8974_read_calib_data(ak8974, AMI306_OFFZERO_X,
> > +				       fab_data2, sizeof(fab_data2));
> > +
> > +		for (i = 0; i < 3; ++i) {
> > +			static const char axis[3] = "XYZ";
> > +			static const char pgaxis[6] = "ZYZXYX";
> > +			unsigned offz = le16_to_cpu(fab_data2[i]) & 0x7F;
> > +			unsigned fine = le16_to_cpu(fab_data1[i]);
> > +			unsigned sens = le16_to_cpu(fab_data1[i + 3]);
> > +			unsigned pgain1 = le16_to_cpu(fab_data1[i + 6]);
> > +			unsigned pgain2 = pgain1 >> 8;
> > +
> > +			pgain1 &= 0xFF;
> > +
> > +			dev_info(&ak8974->i2c->dev,
> > +				 "factory calibration for axis %c: offz=%u sens=%u fine=%u pga%c=%u pga%c=%u\n",
> > +				 axis[i], offz, sens, fine, pgaxis[i * 2],
> > +				 pgain1, pgaxis[i * 2 + 1], pgain2);
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >    
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] iio: magnetometer: ak8974: add_device_randomness(fw + serial)

[Linus Walleij]

On Sun, Aug 20, 2017 at 12:22 PM, Jonathan Cameron <jic23@kernel.org> wrote=
:
> On Thu, 17 Aug 2017 15:56:11 +0200
> Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl> wrote:
>> Signed-off-by: Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl>
>
> Hmm. I wondered about the firmware version as a source of randomness, but
> will leave it in here.
(...)
>> +             add_device_randomness(&fw, sizeof(fw));
>
> The firmware version seems rather to guessable to really count as randomn=
ess.
> presumably it would constant for a particular batch of chips.

That is true, this should go, not for security reasons but because it doesn=
't
add much device-unique randomness.

Nothing we add with add_device_randomness() actually affects the entropy
pool trust, it is just icing making it - maybe - even more random, so addin=
g
256 zeroes is fine for the trust, just pointless and taking time for no goo=
d.

Yours,
Linus Walleij

Re: [PATCH 2/4] iio: magnetometer: ak8974: add_device_randomness(fw + serial)

[Jonathan Cameron]

On Sun, 20 Aug 2017 13:10:58 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sun, Aug 20, 2017 at 12:22 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Thu, 17 Aug 2017 15:56:11 +0200
> > Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:  
> >> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>  
> >
> > Hmm. I wondered about the firmware version as a source of randomness, but
> > will leave it in here.  
> (...)
> >> +             add_device_randomness(&fw, sizeof(fw));  
> >
> > The firmware version seems rather to guessable to really count as randomness.
> > presumably it would constant for a particular batch of chips.  
> 
> That is true, this should go, not for security reasons but because it doesn't
> add much device-unique randomness.
Dropped the fw one...

J
> 
> Nothing we add with add_device_randomness() actually affects the entropy
> pool trust, it is just icing making it - maybe - even more random, so adding
> 256 zeroes is fine for the trust, just pointless and taking time for no good.
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] iio: magnetometer: ak8974: debug AMI306 calibration data

[Jonathan Cameron]

On Sun, 20 Aug 2017 11:28:29 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> On Sun, 20 Aug 2017 11:25:52 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Thu, 17 Aug 2017 15:56:12 +0200
> > Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> >   
> > > Use AMI306 calibration data for randomness and debugging.
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>    
> > Applied to the togreg branch of iio.git and pushed out as testing for the
> > autobuilders to play with it.
> > 
> > Thanks,
> > 
> > Jonathan  
> > > ---
> > >  drivers/iio/magnetometer/ak8974.c | 41 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> > > index d8c45206ba00..b3221b34606b 100644
> > > --- a/drivers/iio/magnetometer/ak8974.c
> > > +++ b/drivers/iio/magnetometer/ak8974.c
> > > @@ -445,6 +445,20 @@ static int ak8974_selftest(struct ak8974 *ak8974)
> > >  	return 0;
> > >  }
> > >  
> > > +static void ak8974_read_calib_data(struct ak8974 *ak8974, unsigned int reg,
> > > +				   __le16 *tab, size_t tab_size)
> > > +{
> > > +	int ret = regmap_bulk_read(ak8974->map, reg, tab, tab_size);
> > > +	if (ret) {
> > > +		memset(tab, 0xFF, tab_size);
> > > +		dev_warn(&ak8974->i2c->dev,
> > > +			 "can't read calibration data (regs %u..%u): %d\n",
> > > +			 reg, reg + tab_size - 1, ret);  
> Slight tweak made to avoid the warning:
> 
> drivers/iio/magnetometer/ak8974.c: In function ‘ak8974_read_calib_data’:
> drivers/iio/magnetometer/ak8974.c:455:45: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 4 has type ‘size_t {aka long unsigned int}’ [-Wformat=]
>      "can't read calibration data (regs %u..%u): %d\n",
>                                             ~^
>                                             %lu
>      reg, reg + tab_size - 1, ret);
>           ~~~~~~~~~~~~~~~~~~                  
> 
> Did the obvious ;)
Lets try that again.  %zu is the right option for size_t to
deal with how it changes type across architectures.

Anyhow, updated (again).

Jonathan

> 
> Jonathan
> 
> > > +	} else {
> > > +		add_device_randomness(tab, tab_size);
> > > +	}
> > > +}
> > > +
> > >  static int ak8974_detect(struct ak8974 *ak8974)
> > >  {
> > >  	unsigned int whoami;
> > > @@ -490,6 +504,33 @@ static int ak8974_detect(struct ak8974 *ak8974)
> > >  	ak8974->name = name;
> > >  	ak8974->variant = whoami;
> > >  
> > > +	if (whoami == AK8974_WHOAMI_VALUE_AMI306) {
> > > +		__le16 fab_data1[9], fab_data2[3];
> > > +		int i;
> > > +
> > > +		ak8974_read_calib_data(ak8974, AMI306_FINEOUTPUT_X,
> > > +				       fab_data1, sizeof(fab_data1));
> > > +		ak8974_read_calib_data(ak8974, AMI306_OFFZERO_X,
> > > +				       fab_data2, sizeof(fab_data2));
> > > +
> > > +		for (i = 0; i < 3; ++i) {
> > > +			static const char axis[3] = "XYZ";
> > > +			static const char pgaxis[6] = "ZYZXYX";
> > > +			unsigned offz = le16_to_cpu(fab_data2[i]) & 0x7F;
> > > +			unsigned fine = le16_to_cpu(fab_data1[i]);
> > > +			unsigned sens = le16_to_cpu(fab_data1[i + 3]);
> > > +			unsigned pgain1 = le16_to_cpu(fab_data1[i + 6]);
> > > +			unsigned pgain2 = pgain1 >> 8;
> > > +
> > > +			pgain1 &= 0xFF;
> > > +
> > > +			dev_info(&ak8974->i2c->dev,
> > > +				 "factory calibration for axis %c: offz=%u sens=%u fine=%u pga%c=%u pga%c=%u\n",
> > > +				 axis[i], offz, sens, fine, pgaxis[i * 2],
> > > +				 pgain1, pgaxis[i * 2 + 1], pgain2);
> > > +		}
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >      
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html