[PATCH v6 0/5] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

Add extended calibration calculations for the new subversion of DSP5.

V6 review comments from Andy Shevchenko 
V5:
 - Add style changes patch along with current series.

V4 review comments from Andy Shevchenko :
 - Move the function creation for Ta4 to first patch
 - Add kernel doc patch for documenting internal struct
 - Add patch to convert while loops to do-while loops for
   polling

V3 review comments from Andy Shevchenko :
 - Change commit message text to more proper English as per suggestions
 - Drop unneeded brackets and parentheses
 - Use defines from limits.h
 - Remove userspace typedefs as leftovers from porting
 - Testing of timeout loops with iopoll.h was no successful,
   because delay between measurements is 10ms, but we need to
   fill at least 3 channels, so final timeout should be 40ms
   which is out of scope of usleep function
 - Fixing some typos in comments

V2 review comments from Andy Shevchenko :
 - Convert divison back to shifts to make it more readable

Crt Mori (5):
  iio:temperature:mlx90632: Reduce number of equal calulcations
  iio:temperature:mlx90632: Add kerneldoc to the internal struct
  iio:temperature:mlx90632: Convert polling while loop to regmap
  iio:temperature:mlx90632: Adding extended calibration option
  iio:temperature:mlx90632: Some stylefixing leftovers

 drivers/iio/temperature/mlx90632.c | 274 ++---
 1 file changed, 247 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v6 2/5] iio:temperature:mlx90632: Add kerneldoc to the internal struct

[Crt Mori]

Document internal/private struct for mlx90632 device.

Signed-off-by: Crt Mori 
Reviewed-by: Andy Shevchenko 
---
 drivers/iio/temperature/mlx90632.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index c3de10ba5b1e..ce75f5a3486b 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -89,9 +89,16 @@
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
 
+/**
+ * struct mlx90632_data - private data for the MLX90632 device
+ * @client: I2C client of the device
+ * @lock: Internal mutex for multiple reads for single measurement
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ */
 struct mlx90632_data {
struct i2c_client *client;
-   struct mutex lock; /* Multiple reads for single measurement */
+   struct mutex lock;
struct regmap *regmap;
u16 emissivity;
 };
-- 
2.25.1

[PATCH v6 1/5] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori 
Reviewed-by: Andy Shevchenko 
---
 drivers/iio/temperature/mlx90632.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..c3de10ba5b1e 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
-   s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+   s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
@@ -393,30 +393,35 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
 }
 
+static s64 mlx90632_calc_ta4(s64 TAdut, s64 scale)
+{
+   return (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315);
+}
+
 static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 u16 tmp_emi)
 {
-   s64 kTA, kTA0, TAdut;
+   s64 kTA, kTA0, TAdut, TAdut4;
s64 temp = 25000;
s8 i;
 
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = mlx90632_calc_ta4(TAdut, 1LL);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

[PATCH v6 3/5] iio:temperature:mlx90632: Convert polling while loop to regmap

[Crt Mori]

Reduce number of lines and improve readability to convert polling while
loops to regmap_read_poll_timeout.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index ce75f5a3486b..d782634c107f 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -180,25 +180,19 @@ static s32 mlx90632_pwr_continuous(struct regmap *regmap)
  */
 static int mlx90632_perform_measurement(struct mlx90632_data *data)
 {
-   int ret, tries = 100;
unsigned int reg_status;
+   int ret;
 
ret = regmap_update_bits(data->regmap, MLX90632_REG_STATUS,
 MLX90632_STAT_DATA_RDY, 0);
if (ret < 0)
return ret;
 
-   while (tries-- > 0) {
-   ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
- ®_status);
-   if (ret < 0)
-   return ret;
-   if (reg_status & MLX90632_STAT_DATA_RDY)
-   break;
-   usleep_range(1, 11000);
-   }
+   ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS, 
reg_status,
+  !(reg_status & MLX90632_STAT_DATA_RDY), 
1,
+  100 * 1);
 
-   if (tries < 0) {
+   if (ret < 0) {
dev_err(&data->client->dev, "data not ready");
return -ETIMEDOUT;
}
-- 
2.25.1

[PATCH v6 4/5] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

For some time the market wants medical grade accuracy in medical range,
while still retaining the declared accuracy outside of the medical range
within the same sensor. That is why we created extended calibration
which is automatically switched to when object temperature is too high.

This patch also introduces the object_ambient_temperature variable which
is needed for more accurate calculation of the object infra-red
footprint as sensor's ambient temperature might be totally different
than what the ambient temperature is at object and that is why we can
have some more errors which can be eliminated. Currently this temperature
is fixed at 25, but the interface to adjust it by user (with external
sensor or just IR measurement of the other object which acts as ambient),
will be introduced in another commit.

Signed-off-by: Crt Mori 
Reviewed-by: Andy Shevchenko 
---
 drivers/iio/temperature/mlx90632.c | 218 -
 1 file changed, 216 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index d782634c107f..94bca2b2866a 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -10,7 +10,9 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +60,8 @@
 /* Control register address - volatile */
 #define MLX90632_REG_CONTROL   0x3001 /* Control Register address */
 #define   MLX90632_CFG_PWR_MASKGENMASK(2, 1) /* PowerMode Mask 
*/
+#define   MLX90632_CFG_MTYP_MASK   GENMASK(8, 4) /* Meas select 
Mask */
+
 /* PowerModes statuses */
 #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
 #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
@@ -65,6 +69,18 @@
 #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
 
+/* Measurement types */
+#define MLX90632_MTYP_MEDICAL 0
+#define MLX90632_MTYP_EXTENDED 17
+
+/* Measurement type select*/
+#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4)
+#define MLX90632_MTYP_STATUS_MEDICAL 
MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL)
+#define MLX90632_MTYP_STATUS_EXTENDED 
MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED)
+
+/* I2C command register - volatile */
+#define MLX90632_REG_I2C_CMD0x3005 /* I2C command Register address */
+
 /* Device status register - volatile */
 #define MLX90632_REG_STATUS0x3fff /* Device status register */
 #define   MLX90632_STAT_BUSY   BIT(10) /* Device busy indicator */
@@ -78,9 +94,21 @@
 #define MLX90632_RAM_2(meas_num)   (MLX90632_ADDR_RAM + 3 * meas_num + 1)
 #define MLX90632_RAM_3(meas_num)   (MLX90632_ADDR_RAM + 3 * meas_num + 2)
 
+/* Name important RAM_MEAS channels */
+#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_1 MLX90632_RAM_3(17)
+#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2 MLX90632_RAM_3(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_1 MLX90632_RAM_1(17)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_2 MLX90632_RAM_2(17)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_3 MLX90632_RAM_1(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_4 MLX90632_RAM_2(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_5 MLX90632_RAM_1(19)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_6 MLX90632_RAM_2(19)
+
 /* Magic constants */
 #define MLX90632_ID_MEDICAL0x0105 /* EEPROM DSPv5 Medical device id */
 #define MLX90632_ID_CONSUMER   0x0205 /* EEPROM DSPv5 Consumer device id */
+#define MLX90632_ID_EXTENDED   0x0505 /* EEPROM DSPv5 Extended range device id 
*/
+#define MLX90632_ID_MASK   GENMASK(14, 0) /* DSP version and device ID in 
EE_VERSION */
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
@@ -88,6 +116,7 @@
 #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 /**
  * struct mlx90632_data - private data for the MLX90632 device
@@ -95,16 +124,23 @@
  * @lock: Internal mutex for multiple reads for single measurement
  * @regmap: Regmap of the device
  * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @mtyp: Measurement type physical sensor configuration for extended range
+ *calculations
+ * @object_ambient_temperature: Ambient temperature at object (might differ of
+ *  the ambient temperature of sensor.
  */
 struct mlx90632_data {
struct i2c_client *client;
struct mutex lock;
struct regmap *regmap;
u16 emissivity;
+   u8 mtyp;
+   u32 object_ambient_temperature;
 };
 
 static const struct regmap_range m

[PATCH v6 5/5] iio:temperature:mlx90632: Some stylefixing leftovers

[Crt Mori]

There is some inconsistency and whitespace cleanup performed in this
patch. It was done on top of my other patches, but I can rebase to head
of the togreg branch if it would go in sooner.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 94bca2b2866a..472a7fb20615 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -112,10 +112,10 @@
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
-#define MLX90632_REF_1212LL /**< ResCtrlRef value of Ch 1 or 
Ch 2 */
-#define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
-#define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
-#define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_REF_1212LL /* ResCtrlRef value of Ch 1 or Ch 2 */
+#define MLX90632_REF_3 12LL /* ResCtrlRef value of Channel 3 */
+#define MLX90632_MAX_MEAS_NUM  31 /* Maximum measurements in list */
+#define MLX90632_SLEEP_DELAY_MS 3000 /* Autosleep delay */
 #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 /**
@@ -889,7 +889,7 @@ static int mlx90632_probe(struct i2c_client *client,
mlx90632->mtyp = MLX90632_MTYP_EXTENDED;
} else if ((read & MLX90632_DSP_MASK) == MLX90632_DSP_VERSION) {
dev_dbg(&client->dev,
-   "Detected Unknown EEPROM calibration %x\n", read);  
+   "Detected Unknown EEPROM calibration %x\n", read);
} else {
dev_err(&client->dev,
"Wrong DSP version %x (expected %x)\n",
-- 
2.25.1

Re: [PATCH v5 3/5] iio:temperature:mlx90632: Convert polling while loop to do-while

[Crt Mori]

On Fri, 14 Aug 2020 at 11:32, Andy Shevchenko  wrote:
>
> On Fri, Aug 14, 2020 at 10:33 AM Crt Mori  wrote:
> > On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko  
> > wrote:
> > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori  wrote:
> > > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko 
> > > >  wrote:
> > > > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori  wrote:
> > > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko 
> > > > > >  wrote:
> > > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori  
> > > > > > > wrote:
> > >
> > > ...
> > >
> > > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > > > > > under the hood in the same way you did here, but open coded.
> > > > > > >
> > > > > >
> > > > > > One loop is indeed 10ms and that is not the problem, the problem is
> > > > > > that timeout is at least 3 calls of this data ready (3 channels), so
> > > > > > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > > > > > case scenario and that is outside of the range for usleep to 
> > > > > > measure.
> > > > > > So in case of the other loop, where we wait 200ms for channel 
> > > > > > refresh
> > > > > > it is also out of scope. Timeout should be in number of tries or in
> > > > > > msleep range if you ask me.
> > > > >
> > > > > I still didn't buy it. You have in both cases usleep_range(). Why in
> > > > > your case it's okay and in regmap_read_poll_timeout() is not?
> > > > >
> > > >
> > > > I tried and it did not work, so then I read the manual. Looking into
> > > >
> > > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a
> > > > timeout occurs
> > >
> > > Why _atomic?!
> >
> > I just pasted something, it is the same as for non _atomic
>
> OK.
>
> ...
>
> > > >  * @delay_us: Time to udelay between reads in us (0 tight-loops).
> > > >  *Should be less than ~10us since udelay is used
> > > >  *(see Documentation/timers/timers-howto.rst).
> > > >  * @timeout_us: Timeout in us, 0 means never timeout
>
> ...
>
> > > > > > > > usleep_range(1, 11000);
> > >
> > > You use here usleep_range(). The same is used for
> > > regmap_read_poll_timeout(). What's the difference?
> > >
> > > Since it uses 1/4 of the range you probably need to update tries and
> > > timeout_us to make it work.
> > >
> >
> > Timeout_us here needs to be in one case 100 * 10ms (maybe not
> > realistic as we could live with number of around 40 * 10ms), but this
> > is a lot more than proposed range of usleep which Is up to 20ms. Even
> > in best case this timeout should be 40 ms to give enough time to
> > measure 2 channels for sure. So with the current timeout_us
> > requirement we are outside of the range of the udelay timer and that
> > is why I would need a macro with number of tries, not with the timeout
> > value (or timeout value of ms).
>
> I do not understand. The regmap_read_poll_timeout() is a macro which
> unrolls in the very similar loop you have now in the code.
> What prevents it from using it?
>
> I think there is a big misunderstanding about the parameters of that macro.
> delay_us (must be small enough), timeout_us can be any long.
>
I tested on Beaglebone with the 100 * 1 as timeout_us and I always
got the -ETIMEDOUT error. I also tested in the other case where
delay_us is 25 and then timeout_us would be 4*25 and I have
also received -ETIMEDOUT as a response.

I can prepare a patch with the iopoll.h API and maybe you will spot
the mistake, as after rechecking timeout_us is indeed 64bit and is
only used in the time comparison operations and not with timers.

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/5] iio:temperature:mlx90632: Convert polling while loop to do-while

[Crt Mori]

On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko  wrote:
>
> On Thu, Aug 13, 2020 at 4:04 PM Crt Mori  wrote:
> > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko  
> > wrote:
> > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori  wrote:
> > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko 
> > > >  wrote:
> > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori  wrote:
>
> ...
>
> > > > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > > > under the hood in the same way you did here, but open coded.
> > > > >
> > > >
> > > > One loop is indeed 10ms and that is not the problem, the problem is
> > > > that timeout is at least 3 calls of this data ready (3 channels), so
> > > > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > > > case scenario and that is outside of the range for usleep to measure.
> > > > So in case of the other loop, where we wait 200ms for channel refresh
> > > > it is also out of scope. Timeout should be in number of tries or in
> > > > msleep range if you ask me.
> > >
> > > I still didn't buy it. You have in both cases usleep_range(). Why in
> > > your case it's okay and in regmap_read_poll_timeout() is not?
> > >
> >
> > I tried and it did not work, so then I read the manual. Looking into
> >
> > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a
> > timeout occurs
>
> Why _atomic?!

I just pasted something, it is the same as for non _atomic
>
> > ...
> >  * @delay_us: Time to udelay between reads in us (0 tight-loops).
> >  *Should be less than ~10us since udelay is used
> >  *(see Documentation/timers/timers-howto.rst).
> >  * @timeout_us: Timeout in us, 0 means never timeout
> >
> >
> > So I went to read Documentation/timers/timers-howto.rst
> >
> > SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> > * Use usleep_range
> >
> > - Why not msleep for (1ms - 20ms)?
> > Explained originally here:
> > http://lkml.org/lkml/2007/8/3/250
> >
> > msleep(1~20) may not do what the caller intends, and
> > will often sleep longer (~20 ms actual sleep for any
> > value given in the 1~20ms range). In many cases this
> > is not the desired behavior.
> >
> > Since I am above the 20ms range, it is too much for usleep_range and
> > that proved to be a case as I got -ETIMEOUT and the desired channels
> > were not read.
> > > > > ...
> > > > >
> > > > > > -   while (tries-- > 0) {
> > > > > > +   do {
> > > > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > > > > >   ®_status);
> > > > > > if (ret < 0)
> > > > > > return ret;
> > > > > > -   if (reg_status & MLX90632_STAT_DATA_RDY)
> > > > > > -   break;
> > > > > > usleep_range(1, 11000);
>
> You use here usleep_range(). The same is used for
> regmap_read_poll_timeout(). What's the difference?
>
> Since it uses 1/4 of the range you probably need to update tries and
> timeout_us to make it work.
>

Timeout_us here needs to be in one case 100 * 10ms (maybe not
realistic as we could live with number of around 40 * 10ms), but this
is a lot more than proposed range of usleep which Is up to 20ms. Even
in best case this timeout should be 40 ms to give enough time to
measure 2 channels for sure. So with the current timeout_us
requirement we are outside of the range of the udelay timer and that
is why I would need a macro with number of tries, not with the timeout
value (or timeout value of ms).


> > > > > > -   }
> > > > > > +   } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
> > > > > >
> > > > > > if (tries < 0) {
> > > > > > dev_err(&data->client->dev, "data not ready");
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 5/5] iio:temperature:mlx90632: Some stylefixing leftovers

[Crt Mori]

On Thu, 13 Aug 2020 at 13:01, Andy Shevchenko  wrote:
>
> On Thu, Aug 13, 2020 at 10:53 AM Crt Mori  wrote:
> >
> > There is some inconsistency and whitespace cleanup performed in this
> > patch. It was done on top of my other patches, but I can rebase to head
> > of the togreg branch if it would go in sooner.
>
> ...
>
> > -#define MLX90632_REF_1212LL /**< ResCtrlRef value of Ch 1 
> > or Ch 2 */
> > -#define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
> > -#define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
> > -#define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
>
>
> > +#define MLX90632_REF_1212LL /* ResCtrlRef value of Ch 1 or Ch 2 */
> > +#define MLX90632_REF_3 12LL /* ResCtrlRef value of Channel 3 */
> > +#define MLX90632_MAX_MEAS_NUM  31 /* Maximum measurements in list */
> > +#define MLX90632_SLEEP_DELAY_MS 3000 /* Autosleep delay */
> >  #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
>
> This was actually in doxy (perhaps kernel doc also understands this)
> format of description.
> Can you double check that the kernel doc is okay / not okay with it?
>
> If it is okay, perhaps it's better to convert others to that format
> rather than dropping it.
>
It is indeed from doxygen and looking at other drivers it should not
be OK. I checked the docs and it does not say in fact. Maybe Jonathan
knows, but he was already OK with these changes in v1.
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/5] iio:temperature:mlx90632: Convert polling while loop to do-while

[Crt Mori]

On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko  wrote:
>
> On Thu, Aug 13, 2020 at 2:14 PM Crt Mori  wrote:
> >
> > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko  
> > wrote:
> > >
> > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori  wrote:
> > > >
> > > > Reduce number of lines and improve readability to convert polling while
> > > > loops to do-while. The iopoll.h interface was not used, because we
> > > > require more than 20ms timeout, because time for sensor to perform a
> > > > measurement is around 10ms and it needs to perform measurements for each
> > > > channel (which currently is 3).
> > >
> > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > under the hood in the same way you did here, but open coded.
> > >
> >
> > One loop is indeed 10ms and that is not the problem, the problem is
> > that timeout is at least 3 calls of this data ready (3 channels), so
> > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > case scenario and that is outside of the range for usleep to measure.
> > So in case of the other loop, where we wait 200ms for channel refresh
> > it is also out of scope. Timeout should be in number of tries or in
> > msleep range if you ask me.
>
> I still didn't buy it. You have in both cases usleep_range(). Why in
> your case it's okay and in regmap_read_poll_timeout() is not?
>

I tried and it did not work, so then I read the manual. Looking into

* regmap_read_poll_timeout_atomic - Poll until a condition is met or a
timeout occurs
...
 * @delay_us: Time to udelay between reads in us (0 tight-loops).
 *Should be less than ~10us since udelay is used
 *(see Documentation/timers/timers-howto.rst).
 * @timeout_us: Timeout in us, 0 means never timeout


So I went to read Documentation/timers/timers-howto.rst

SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
* Use usleep_range

- Why not msleep for (1ms - 20ms)?
Explained originally here:
http://lkml.org/lkml/2007/8/3/250

msleep(1~20) may not do what the caller intends, and
will often sleep longer (~20 ms actual sleep for any
value given in the 1~20ms range). In many cases this
is not the desired behavior.

Since I am above the 20ms range, it is too much for usleep_range and
that proved to be a case as I got -ETIMEOUT and the desired channels
were not read.
> > > ...
> > >
> > > > -   while (tries-- > 0) {
> > > > +   do {
> > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > > >   ®_status);
> > > > if (ret < 0)
> > > > return ret;
> > > > -   if (reg_status & MLX90632_STAT_DATA_RDY)
> > > > -   break;
> > > > usleep_range(1, 11000);
> > > > -   }
> > > > +   } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
> > > >
> > > > if (tries < 0) {
> > > > dev_err(&data->client->dev, "data not ready");
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/5] iio:temperature:mlx90632: Convert polling while loop to do-while

[Crt Mori]

On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko  wrote:
>
> On Thu, Aug 13, 2020 at 10:53 AM Crt Mori  wrote:
> >
> > Reduce number of lines and improve readability to convert polling while
> > loops to do-while. The iopoll.h interface was not used, because we
> > require more than 20ms timeout, because time for sensor to perform a
> > measurement is around 10ms and it needs to perform measurements for each
> > channel (which currently is 3).
>
> I don't see how it prevents using iopoll.h. It uses usleep_range()
> under the hood in the same way you did here, but open coded.
>

One loop is indeed 10ms and that is not the problem, the problem is
that timeout is at least 3 calls of this data ready (3 channels), so
that is at minimum 30ms of timeout, or it could even be 4 in worse
case scenario and that is outside of the range for usleep to measure.
So in case of the other loop, where we wait 200ms for channel refresh
it is also out of scope. Timeout should be in number of tries or in
msleep range if you ask me.

> ...
>
> > -   while (tries-- > 0) {
> > +   do {
> > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> >   ®_status);
> > if (ret < 0)
> > return ret;
> > -   if (reg_status & MLX90632_STAT_DATA_RDY)
> > -   break;
> > usleep_range(1, 11000);
> > -   }
> > +   } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
> >
> > if (tries < 0) {
> > dev_err(&data->client->dev, "data not ready");
>
> --
> With Best Regards,
> Andy Shevchenko

[PATCH v5 0/5] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

Add extended calibration calculations for the new subversion of DSP5.

V5 review comments from Andy Shevchenko 
V5:
 - Add style changes patch along with current series.

V4 review comments from Andy Shevchenko :
 - Move the function creation for Ta4 to first patch
 - Add kernel doc patch for documenting internal struct
 - Add patch to convert while loops to do-while loops for
   polling

V3 review comments from Andy Shevchenko :
 - Change commit message text to more proper English as per suggestions
 - Drop unneeded brackets and parentheses
 - Use defines from limits.h
 - Remove userspace typedefs as leftovers from porting
 - Testing of timeout loops with iopoll.h was no successful,
   because delay between measurements is 10ms, but we need to
   fill at least 3 channels, so final timeout should be 40ms
   which is out of scope of usleep function
 - Fixing some typos in comments

V2 review comments from Andy Shevchenko :
 - Convert divison back to shifts to make it more readable

Crt Mori (5):
  iio:temperature:mlx90632: Reduce number of equal calulcations
  iio:temperature:mlx90632: Add kerneldoc to the internal struct
  iio:temperature:mlx90632: Convert polling while loop to do-while
  iio:temperature:mlx90632: Adding extended calibration option
  iio:temperature:mlx90632: Some stylefixing leftovers

 drivers/iio/temperature/mlx90632.c | 301 +
 1 file changed, 267 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH v5 2/5] iio:temperature:mlx90632: Add kerneldoc to the internal struct

[Crt Mori]

Document internal/private struct for mlx90632 device.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index c3de10ba5b1e..ce75f5a3486b 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -89,9 +89,16 @@
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
 
+/**
+ * struct mlx90632_data - private data for the MLX90632 device
+ * @client: I2C client of the device
+ * @lock: Internal mutex for multiple reads for single measurement
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ */
 struct mlx90632_data {
struct i2c_client *client;
-   struct mutex lock; /* Multiple reads for single measurement */
+   struct mutex lock;
struct regmap *regmap;
u16 emissivity;
 };
-- 
2.25.1

[PATCH v5 3/5] iio:temperature:mlx90632: Convert polling while loop to do-while

[Crt Mori]

Reduce number of lines and improve readability to convert polling while
loops to do-while. The iopoll.h interface was not used, because we
require more than 20ms timeout, because time for sensor to perform a
measurement is around 10ms and it needs to perform measurements for each
channel (which currently is 3).

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index ce75f5a3486b..d87c792b7e72 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -188,15 +188,13 @@ static int mlx90632_perform_measurement(struct 
mlx90632_data *data)
if (ret < 0)
return ret;
 
-   while (tries-- > 0) {
+   do {
ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
  ®_status);
if (ret < 0)
return ret;
-   if (reg_status & MLX90632_STAT_DATA_RDY)
-   break;
usleep_range(1, 11000);
-   }
+   } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
 
if (tries < 0) {
dev_err(&data->client->dev, "data not ready");
-- 
2.25.1

[PATCH v5 1/5] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori 
Reviewed-by: Andy Shevchenko 
---
 drivers/iio/temperature/mlx90632.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..c3de10ba5b1e 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
-   s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+   s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
@@ -393,30 +393,35 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
 }
 
+static s64 mlx90632_calc_ta4(s64 TAdut, s64 scale)
+{
+   return (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315);
+}
+
 static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 u16 tmp_emi)
 {
-   s64 kTA, kTA0, TAdut;
+   s64 kTA, kTA0, TAdut, TAdut4;
s64 temp = 25000;
s8 i;
 
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = mlx90632_calc_ta4(TAdut, 1LL);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

[PATCH v5 5/5] iio:temperature:mlx90632: Some stylefixing leftovers

[Crt Mori]

There is some inconsistency and whitespace cleanup performed in this
patch. It was done on top of my other patches, but I can rebase to head
of the togreg branch if it would go in sooner.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 41 --
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 9fa8c078c037..4ef13509fb0f 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -94,6 +94,9 @@
 #define MLX90632_RAM_3(meas_num)   (MLX90632_ADDR_RAM + 3 * meas_num + 2)
 
 /* Name important RAM_MEAS channels */
+#define MLX90632_RAM_DSP5_AMBIENT_1 MLX90632_RAM_3(1)
+#define MLX90632_RAM_DSP5_AMBIENT_2 MLX90632_RAM_3(2)
+
 #define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_1 MLX90632_RAM_3(17)
 #define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2 MLX90632_RAM_3(18)
 #define MLX90632_RAM_DSP5_EXTENDED_OBJECT_1 MLX90632_RAM_1(17)
@@ -111,10 +114,10 @@
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
-#define MLX90632_REF_1212LL /**< ResCtrlRef value of Ch 1 or 
Ch 2 */
-#define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
-#define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
-#define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_REF_1212LL /* ResCtrlRef value of Ch 1 or Ch 2 */
+#define MLX90632_REF_3 12LL /* ResCtrlRef value of Channel 3 */
+#define MLX90632_MAX_MEAS_NUM  31 /* Maximum measurements in list */
+#define MLX90632_SLEEP_DELAY_MS 3000 /* Autosleep delay */
 #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 /**
@@ -285,12 +288,12 @@ static int mlx90632_read_ambient_raw(struct regmap 
*regmap,
int ret;
unsigned int read_tmp;
 
-   ret = regmap_read(regmap, MLX90632_RAM_3(1), &read_tmp);
+   ret = regmap_read(regmap, MLX90632_RAM_DSP5_AMBIENT_1, &read_tmp);
if (ret < 0)
return ret;
*ambient_new_raw = (s16)read_tmp;
 
-   ret = regmap_read(regmap, MLX90632_RAM_3(2), &read_tmp);
+   ret = regmap_read(regmap, MLX90632_RAM_DSP5_AMBIENT_2, &read_tmp);
if (ret < 0)
return ret;
*ambient_old_raw = (s16)read_tmp;
@@ -488,11 +491,11 @@ static s64 mlx90632_preprocess_temp_amb(s16 
ambient_new_raw,
 
kGb = ((s64)Gb * 1000LL) >> 10ULL;
VR_Ta = (s64)ambient_old_raw * 100LL +
-   kGb * div64_s64(((s64)ambient_new_raw * 1000LL),
-   (MLX90632_REF_3));
+   kGb * div64_s64((s64)ambient_new_raw * 1000LL,
+   MLX90632_REF_3);
tmp = div64_s64(
-div64_s64(((s64)ambient_new_raw * 1LL),
-  (MLX90632_REF_3)), VR_Ta);
+div64_s64((s64)ambient_new_raw * 1LL,
+  MLX90632_REF_3), VR_Ta);
return div64_s64(tmp << 19ULL, 1000LL);
 }
 
@@ -504,13 +507,13 @@ static s64 mlx90632_preprocess_temp_obj(s16 
object_new_raw, s16 object_old_raw,
 
kKa = ((s64)Ka * 1000LL) >> 10ULL;
VR_IR = (s64)ambient_old_raw * 100LL +
-   kKa * div64_s64(((s64)ambient_new_raw * 1000LL),
-   (MLX90632_REF_3));
+   kKa * div64_s64((s64)ambient_new_raw * 1000LL,
+   MLX90632_REF_3);
tmp = div64_s64(
-   div64_s64(((s64)((object_new_raw + object_old_raw) / 2)
-  * 1LL), (MLX90632_REF_12)),
+   div64_s64((s64)((object_new_raw + object_old_raw) / 2)
+  * 1LL, MLX90632_REF_12),
VR_IR);
-   return div64_s64((tmp << 19ULL), 1000LL);
+   return div64_s64(tmp << 19ULL, 1000LL);
 }
 
 static s64 mlx90632_preprocess_temp_obj_extended(s16 object_new_raw, s16 
ambient_new_raw,
@@ -560,8 +563,8 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
 * 1000LL)) >> 36LL;
calcedKsTA = ((s64)(Fb * (TAdut - 25 * 100LL))) >> 36LL;
-   Alpha_corr = div64_s64s64)(Fa * 100LL) >> 46LL)
-   * Ha_customer), 1000LL);
+   Alpha_corr = div64_s64(((s64)(Fa * 100LL) >> 46LL)
+   * Ha_customer, 1000LL);
Alpha_corr *= ((s64)(1 * 100LL + calcedKsTO + calcedKsTA));
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha

[PATCH v5 4/5] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

For some time the market wants medical grade accuracy in medical range,
while still retaining the declared accuracy outside of the medical range
within the same sensor. That is why we created extended calibration
which is automatically switched to when object temperature is too high.

This patch also introduces the object_ambient_temperature variable which
is needed for more accurate calculation of the object infra-red
footprint as sensor's ambient temperature might be totally different
than what the ambient temperature is at object and that is why we can
have some more errors which can be eliminated. Currently this temperature
is fixed at 25, but the interface to adjust it by user (with external
sensor or just IR measurement of the other object which acts as ambient),
will be introduced in another commit.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 224 -
 1 file changed, 222 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index d87c792b7e72..9fa8c078c037 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +59,8 @@
 /* Control register address - volatile */
 #define MLX90632_REG_CONTROL   0x3001 /* Control Register address */
 #define   MLX90632_CFG_PWR_MASKGENMASK(2, 1) /* PowerMode Mask 
*/
+#define   MLX90632_CFG_MTYP_MASK   GENMASK(8, 4) /* Meas select 
Mask */
+
 /* PowerModes statuses */
 #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
 #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
@@ -65,6 +68,18 @@
 #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
 
+/* Measurement types */
+#define MLX90632_MTYP_MEDICAL 0
+#define MLX90632_MTYP_EXTENDED 17
+
+/* Measurement type select*/
+#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4)
+#define MLX90632_MTYP_STATUS_MEDICAL 
MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL)
+#define MLX90632_MTYP_STATUS_EXTENDED 
MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED)
+
+/* I2C command register - volatile */
+#define MLX90632_REG_I2C_CMD0x3005 /* I2C command Register address */
+
 /* Device status register - volatile */
 #define MLX90632_REG_STATUS0x3fff /* Device status register */
 #define   MLX90632_STAT_BUSY   BIT(10) /* Device busy indicator */
@@ -78,9 +93,21 @@
 #define MLX90632_RAM_2(meas_num)   (MLX90632_ADDR_RAM + 3 * meas_num + 1)
 #define MLX90632_RAM_3(meas_num)   (MLX90632_ADDR_RAM + 3 * meas_num + 2)
 
+/* Name important RAM_MEAS channels */
+#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_1 MLX90632_RAM_3(17)
+#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2 MLX90632_RAM_3(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_1 MLX90632_RAM_1(17)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_2 MLX90632_RAM_2(17)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_3 MLX90632_RAM_1(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_4 MLX90632_RAM_2(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_5 MLX90632_RAM_1(19)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_6 MLX90632_RAM_2(19)
+
 /* Magic constants */
 #define MLX90632_ID_MEDICAL0x0105 /* EEPROM DSPv5 Medical device id */
 #define MLX90632_ID_CONSUMER   0x0205 /* EEPROM DSPv5 Consumer device id */
+#define MLX90632_ID_EXTENDED   0x0505 /* EEPROM DSPv5 Extended range device id 
*/
+#define MLX90632_ID_MASK   GENMASK(14, 0) /* DSP version and device ID in 
EE_VERSION */
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
@@ -88,6 +115,7 @@
 #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 /**
  * struct mlx90632_data - private data for the MLX90632 device
@@ -95,16 +123,23 @@
  * @lock: Internal mutex for multiple reads for single measurement
  * @regmap: Regmap of the device
  * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @mtyp: Measurement type physical sensor configuration for extended range
+ *calculations
+ * @object_ambient_temperature: Ambient temperature at object (might differ of
+ *  the ambient temperature of sensor.
  */
 struct mlx90632_data {
struct i2c_client *client;
struct mutex lock;
struct regmap *regmap;
u16 emissivity;
+   u8 mtyp;
+   u32 object_ambient_temperature;
 };
 
 static const struct regmap_range mlx90632_volatile_reg_range[] = {
regmap_reg_r

Re: [PATCH v4 2/4] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

On Sun, 9 Aug 2020 at 23:05, Crt Mori  wrote:
>
> On Sun, 9 Aug 2020 at 15:32, Jonathan Cameron  wrote:
> >
> > On Sat, 8 Aug 2020 23:57:59 +0200
> > Crt Mori  wrote:
> >
> > > Hi,
> > > I am very sorry you missed them, I thought you saw it (reply on v3 of
> > > the patch). Maybe something happened to that mail, as it contained
> > > link to datasheet, so I will omit it now.
> > >
> > > Except for the order, only the remarks below are still open (did you
> > > get the polling trail I did?)
> > >
> > > On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko  
> > > wrote:
> > > >
> > > > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori  wrote:
> > > > >
> > > > > For some time the market wants medical grade accuracy in medical 
> > > > > range,
> > > > > while still retaining the declared accuracy outside of the medical 
> > > > > range
> > > > > within the same sensor. That is why we created extended calibration
> > > > > which is automatically switched to when object temperature is too 
> > > > > high.
> > > > >
> > > > > This patch also introduces the object_ambient_temperature variable 
> > > > > which
> > > > > is needed for more accurate calculation of the object infra-red
> > > > > footprint as sensor's ambient temperature might be totally different
> > > > > than what the ambient temperature is at object and that is why we can
> > > > > have some more errors which can be eliminated. Currently this 
> > > > > temperature
> > > > > is fixed at 25, but the interface to adjust it by user (with external
> > > > > sensor or just IR measurement of the other object which acts as 
> > > > > ambient),
> > > > > will be introduced in another commit.
> > > >
> > > > The kernel doc patch should go before this patch.
> > > >
> > > > ...
> > > >
> > > > > +   *ambient_new_raw = (s16)read_tmp;
> > > >
> > > > > +   *ambient_old_raw = (s16)read_tmp;
> > > >
> > > > Sorry, did I miss your answer about these castings all over the patch?
> > > >
> > >
> > > These castings are in fact needed. You read unsigned integer, but the
> > > return value is signed integer. Without the cast it did not extend the
> > > signed bit, but just wrote the value to signed. Also I find it more
> > > obvious with casts, that I did not "accidentally" convert to signed.
> >
> > Should we perhaps be making this explicit for the cases where we
> > are sign extending?  That doesn't include these two as the lvalue
> > is s16, but does include some of the others.
> >
> > sign_extend32(read_tmp, 15)
> >
>
> So for you lines like
> s32 read;
> read = (read + (s16)read_tmp) / 2;
>
> would actually be better as:
> read = (read + sign_extend32(read_tmp, 15)) / 2;
>
> Hm, strange. I would read that more align the read_tmp to 32 bit than
> the value you have in read_tmp is actually a signed 16 bit integer...
>

OK, I did some trails without the casts and had deja-vu from the first
series of patches I submitted.  I noticed that without a cast the
value that ends up in variable is not extended to signed, but it is
unsigned value truncated. This same finding leads to have these casts
already in current ambient and object raw read functions.

So now only debate is if sign_extend32 is useful in this case, as read
in the current case is 32 bit (before it was also 16 bit).

My preference is to leave unified across the driver.

> > >
> > > > ...
> > > >
> > > > > +   ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > > > > +   ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > > > > +   ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > > > > +   ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > > > > +   ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > > > > +   ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
> > > >
> > > > What so special about these magic 17, 18, 19? Can you provide 
> > > > definitions?
> > > >
> > > When we started 0 to 19 were all open for access, from userspace, then
> > > only 1 and 2 were used with calculations, and now w

Re: [PATCH v4 2/4] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

On Sun, 9 Aug 2020 at 15:32, Jonathan Cameron  wrote:
>
> On Sat, 8 Aug 2020 23:57:59 +0200
> Crt Mori  wrote:
>
> > Hi,
> > I am very sorry you missed them, I thought you saw it (reply on v3 of
> > the patch). Maybe something happened to that mail, as it contained
> > link to datasheet, so I will omit it now.
> >
> > Except for the order, only the remarks below are still open (did you
> > get the polling trail I did?)
> >
> > On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko  
> > wrote:
> > >
> > > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori  wrote:
> > > >
> > > > For some time the market wants medical grade accuracy in medical range,
> > > > while still retaining the declared accuracy outside of the medical range
> > > > within the same sensor. That is why we created extended calibration
> > > > which is automatically switched to when object temperature is too high.
> > > >
> > > > This patch also introduces the object_ambient_temperature variable which
> > > > is needed for more accurate calculation of the object infra-red
> > > > footprint as sensor's ambient temperature might be totally different
> > > > than what the ambient temperature is at object and that is why we can
> > > > have some more errors which can be eliminated. Currently this 
> > > > temperature
> > > > is fixed at 25, but the interface to adjust it by user (with external
> > > > sensor or just IR measurement of the other object which acts as 
> > > > ambient),
> > > > will be introduced in another commit.
> > >
> > > The kernel doc patch should go before this patch.
> > >
> > > ...
> > >
> > > > +   *ambient_new_raw = (s16)read_tmp;
> > >
> > > > +   *ambient_old_raw = (s16)read_tmp;
> > >
> > > Sorry, did I miss your answer about these castings all over the patch?
> > >
> >
> > These castings are in fact needed. You read unsigned integer, but the
> > return value is signed integer. Without the cast it did not extend the
> > signed bit, but just wrote the value to signed. Also I find it more
> > obvious with casts, that I did not "accidentally" convert to signed.
>
> Should we perhaps be making this explicit for the cases where we
> are sign extending?  That doesn't include these two as the lvalue
> is s16, but does include some of the others.
>
> sign_extend32(read_tmp, 15)
>

So for you lines like
s32 read;
read = (read + (s16)read_tmp) / 2;

would actually be better as:
read = (read + sign_extend32(read_tmp, 15)) / 2;

Hm, strange. I would read that more align the read_tmp to 32 bit than
the value you have in read_tmp is actually a signed 16 bit integer...

> >
> > > ...
> > >
> > > > +   ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > > > +   ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > > > +   ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > > > +   ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > > > +   ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > > > +   ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
> > >
> > > What so special about these magic 17, 18, 19? Can you provide definitions?
> > >
> > When we started 0 to 19 were all open for access, from userspace, then
> > only 1 and 2 were used with calculations, and now we use 17, 18 and
> > 19. Matter of fact is, I can't provide a descriptive name as it
> > depends on DSP version and as you can see now within the same DSP
> > version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
> > named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
> > that might not be true in the next configuration, so I rather keep the
> > naming as in the datasheet.
> Normal solution for that is to version the defines as well.
>
> MLX90632_FW3_RAM_1_AMBIENT etc
> When a new version changes this, then you introduced new defines to
> support that firmware.
>

OK will add those, but it is ending up as:
MLX90632_RAM_DSP5_AMBIENT
MLX90632_RAM_DSP5_EXTENDED_AMBIENT
MLX90632_RAM_DSP5_OBJECT_1
MLX90632_RAM_DSP5_EXTENDED_OBJECT_1
MLX90632_RAM_DSP5_OBJECT_2
MLX90632_RAM_DSP5_EXTENDED_OBJECT_2

ok?
> >
> > > ...
> > >
> > > > +   int tries = 4;
> > >
> > > > +   while (tries-- > 0) {
> > > > +  

Re: [PATCH v4 2/4] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

Hi,
I am very sorry you missed them, I thought you saw it (reply on v3 of
the patch). Maybe something happened to that mail, as it contained
link to datasheet, so I will omit it now.

Except for the order, only the remarks below are still open (did you
get the polling trail I did?)

On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko  wrote:
>
> On Sat, Aug 8, 2020 at 3:11 PM Crt Mori  wrote:
> >
> > For some time the market wants medical grade accuracy in medical range,
> > while still retaining the declared accuracy outside of the medical range
> > within the same sensor. That is why we created extended calibration
> > which is automatically switched to when object temperature is too high.
> >
> > This patch also introduces the object_ambient_temperature variable which
> > is needed for more accurate calculation of the object infra-red
> > footprint as sensor's ambient temperature might be totally different
> > than what the ambient temperature is at object and that is why we can
> > have some more errors which can be eliminated. Currently this temperature
> > is fixed at 25, but the interface to adjust it by user (with external
> > sensor or just IR measurement of the other object which acts as ambient),
> > will be introduced in another commit.
>
> The kernel doc patch should go before this patch.
>
> ...
>
> > +   *ambient_new_raw = (s16)read_tmp;
>
> > +   *ambient_old_raw = (s16)read_tmp;
>
> Sorry, did I miss your answer about these castings all over the patch?
>

These castings are in fact needed. You read unsigned integer, but the
return value is signed integer. Without the cast it did not extend the
signed bit, but just wrote the value to signed. Also I find it more
obvious with casts, that I did not "accidentally" convert to signed.

> ...
>
> > +   ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > +   ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > +   ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > +   ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > +   ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > +   ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
>
> What so special about these magic 17, 18, 19? Can you provide definitions?
>
When we started 0 to 19 were all open for access, from userspace, then
only 1 and 2 were used with calculations, and now we use 17, 18 and
19. Matter of fact is, I can't provide a descriptive name as it
depends on DSP version and as you can see now within the same DSP
version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
that might not be true in the next configuration, so I rather keep the
naming as in the datasheet.

> ...
>
> > +   int tries = 4;
>
> > +   while (tries-- > 0) {
> > +   ret = mlx90632_perform_measurement(data);
> > +   if (ret < 0)
> > +   goto read_unlock;
> > +
> > +   if (ret == 19)
> > +   break;
> > +   }
> > +   if (tries < 0) {
> > +   ret = -ETIMEDOUT;
> > +   goto read_unlock;
> > +   }
>
> Please avoid ping-pong type of changes in the same series (similar way
> as for kernel doc), which means don't introduce something you are
> going to change later on. Patch to move to do {} while () should go
> before this one.

OK, will fix that ordering in v5, but will wait till we solve also
above discussions to avoid adding new versions.

>
> --
> With Best Regards,
> Andy Shevchenko

And about that voodoo stuff with numbers:

Honestly, the equation is in the datasheet[1] and this is just making
floating point to fixed point with proper intermediate scaling
(initially I had defines of TENTOX, but that was not desired). There
is no better explanation of this voodoo.

[PATCH v4 2/4] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

For some time the market wants medical grade accuracy in medical range,
while still retaining the declared accuracy outside of the medical range
within the same sensor. That is why we created extended calibration
which is automatically switched to when object temperature is too high.

This patch also introduces the object_ambient_temperature variable which
is needed for more accurate calculation of the object infra-red
footprint as sensor's ambient temperature might be totally different
than what the ambient temperature is at object and that is why we can
have some more errors which can be eliminated. Currently this temperature
is fixed at 25, but the interface to adjust it by user (with external
sensor or just IR measurement of the other object which acts as ambient),
will be introduced in another commit.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 212 -
 1 file changed, 210 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index c3de10ba5b1e..049c1217a166 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +59,8 @@
 /* Control register address - volatile */
 #define MLX90632_REG_CONTROL   0x3001 /* Control Register address */
 #define   MLX90632_CFG_PWR_MASKGENMASK(2, 1) /* PowerMode Mask 
*/
+#define   MLX90632_CFG_MTYP_MASK   GENMASK(8, 4) /* Meas select 
Mask */
+
 /* PowerModes statuses */
 #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
 #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
@@ -65,6 +68,18 @@
 #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
 
+/* Measurement types */
+#define MLX90632_MTYP_MEDICAL 0
+#define MLX90632_MTYP_EXTENDED 17
+
+/* Measurement type select*/
+#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4)
+#define MLX90632_MTYP_STATUS_MEDICAL 
MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL)
+#define MLX90632_MTYP_STATUS_EXTENDED 
MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED)
+
+/* I2C command register - volatile */
+#define MLX90632_REG_I2C_CMD0x3005 /* I2C command Register address */
+
 /* Device status register - volatile */
 #define MLX90632_REG_STATUS0x3fff /* Device status register */
 #define   MLX90632_STAT_BUSY   BIT(10) /* Device busy indicator */
@@ -81,6 +96,8 @@
 /* Magic constants */
 #define MLX90632_ID_MEDICAL0x0105 /* EEPROM DSPv5 Medical device id */
 #define MLX90632_ID_CONSUMER   0x0205 /* EEPROM DSPv5 Consumer device id */
+#define MLX90632_ID_EXTENDED   0x0505 /* EEPROM DSPv5 Extended range device id 
*/
+#define MLX90632_ID_MASK   GENMASK(14, 0) /* DSP version and device ID in 
EE_VERSION */
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
@@ -88,16 +105,20 @@
 #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 struct mlx90632_data {
struct i2c_client *client;
struct mutex lock; /* Multiple reads for single measurement */
struct regmap *regmap;
u16 emissivity;
+   u8 mtyp; /* measurement type - to enable extended range calculations */
+   u32 object_ambient_temperature;
 };
 
 static const struct regmap_range mlx90632_volatile_reg_range[] = {
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -113,6 +134,7 @@ static const struct regmap_range mlx90632_read_reg_range[] 
= {
regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR),
regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb),
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -199,6 +221,26 @@ static int mlx90632_perform_measurement(struct 
mlx90632_data *data)
return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
 }
 
+static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
+{
+   int ret;
+
+   if ((type != MLX90632_MTYP_MEDICAL

[PATCH v4 4/4] iio:temperature:mlx90632: Convert polling while loops to do-while

[Crt Mori]

Reduce number of lines and improve readability to convert polling while
loops to do-while. The iopoll.h interface was not used, because we
require more than 20ms timeout, because time for sensor to perform a
measurement is around 10ms and it needs to perform measurements for each
channel (which currently is 3).

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 4e0131705c11..24ffcce9ad74 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -214,15 +214,13 @@ static int mlx90632_perform_measurement(struct 
mlx90632_data *data)
if (ret < 0)
return ret;
 
-   while (tries-- > 0) {
+   do {
ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
  ®_status);
if (ret < 0)
return ret;
-   if (reg_status & MLX90632_STAT_DATA_RDY)
-   break;
usleep_range(1, 11000);
-   }
+   } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
 
if (tries < 0) {
dev_err(&data->client->dev, "data not ready");
@@ -419,7 +417,7 @@ static int mlx90632_read_object_raw_extended(struct regmap 
*regmap, s16 *object_
 static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 
*object_new_raw,
  s16 *ambient_new_raw, s16 
*ambient_old_raw)
 {
-   int tries = 4;
+   int tries = 5;
int ret;
 
mutex_lock(&data->lock);
@@ -427,14 +425,13 @@ static int mlx90632_read_all_channel_extended(struct 
mlx90632_data *data, s16 *o
if (ret < 0)
goto read_unlock;
 
-   while (tries-- > 0) {
+   do {
ret = mlx90632_perform_measurement(data);
if (ret < 0)
goto read_unlock;
 
-   if (ret == 19)
-   break;
-   }
+   } while ((ret != 19) && tries--);
+
if (tries < 0) {
ret = -ETIMEDOUT;
goto read_unlock;
-- 
2.25.1

[PATCH v4 3/4] iio:temperature:mlx90632: Add kerneldoc to the internal struct

[Crt Mori]

Document internal/private struct for mlx90632 device.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 049c1217a166..4e0131705c11 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -107,12 +107,23 @@
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
 #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
+/**
+ * struct mlx90632_data - private data for the MLX90632 device
+ * @client: I2C client of the device
+ * @lock: Internal mutex for multiple reads for single measurement
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @mtyp: Measurement type physical sensor configuration for extended range
+ *calculations
+ * @object_ambient_temperature: Ambient temperature at object (might differ of
+ *  the ambient temperature of sensor.
+ */
 struct mlx90632_data {
struct i2c_client *client;
-   struct mutex lock; /* Multiple reads for single measurement */
+   struct mutex lock;
struct regmap *regmap;
u16 emissivity;
-   u8 mtyp; /* measurement type - to enable extended range calculations */
+   u8 mtyp;
u32 object_ambient_temperature;
 };
 
-- 
2.25.1

[PATCH v4 1/4] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..c3de10ba5b1e 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
-   s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+   s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
@@ -393,30 +393,35 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
 }
 
+static s64 mlx90632_calc_ta4(s64 TAdut, s64 scale)
+{
+   return (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315) *
+   (div64_s64(TAdut, scale) + 27315);
+}
+
 static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 u16 tmp_emi)
 {
-   s64 kTA, kTA0, TAdut;
+   s64 kTA, kTA0, TAdut, TAdut4;
s64 temp = 25000;
s8 i;
 
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = mlx90632_calc_ta4(TAdut, 1LL);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

[PATCH v4 0/4] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

Since the second patch is dependent on the first and was still not
merged, I have decided to send them together. First patch just makes
second one more readable as it splits out the repeated calculation and
that enables the second patch to tweak the variable to the new
condition.

V4 review comments from Andy Shevchenko :
 - Move the function creation for Ta4 to first patch
 - Add kernel doc patch for documenting internal struct
 - Add patch to convert while loops to do-while loops for
   polling

V3 review comments from Andy Shevchenko :
 - Change commit message text to more proper English as per suggestions
 - Drop unneeded brackets and parentheses
 - Use defines from limits.h
 - Remove userspace typedefs as leftovers from porting
 - Testing of timeout loops with iopoll.h was no successful,
   because delay between measurements is 10ms, but we need to
   fill at least 3 channels, so final timeout should be 40ms
   which is out of scope of usleep function
 - Fixing some typos in comments

V2 review comments from Andy Shevchenko :
 - Convert divison back to shifts to make it more readable

Crt Mori (4):
  iio:temperature:mlx90632: Reduce number of equal calulcations
  iio:temperature:mlx90632: Adding extended calibration option
  iio:temperature:mlx90632: Add kerneldoc to the internal struct
  iio:temperature:mlx90632: Convert polling while loops to do-while

 drivers/iio/temperature/mlx90632.c | 251 +++--
 1 file changed, 236 insertions(+), 15 deletions(-)

-- 
2.25.1

Re: [PATCH v3 2/2] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

On Sat, 8 Aug 2020 at 12:44, Andy Shevchenko  wrote:
>
> On Sat, Aug 8, 2020 at 2:24 AM Crt Mori  wrote:
> >
> > For some time the market wants medical grade accuracy in medical range,
> > while still retaining the declared accuracy outside of the medical range
> > within the same sensor. That is why we created extended calibration
> > which is automatically switched to when object temperature is too high.
> >
> > This patch also introduces the object_ambient_temperature variable which
> > is needed for more accurate calculation of the object infra-red
> > footprint as sensor's ambient temperature might be totally different
> > than what the ambient temperature is at object and that is why we can
> > have some more errors which can be eliminated. Currently this temperature
> > is fixed at 25, but the interface to adjust it by user (with external
> > sensor or just IR measurement of the other object which acts as ambient),
> > will be introduced in another commit.
>
> Thank you for an update!
> My comments below.
>

Will provide next version with changelog as well.

> ...
>
> >  struct mlx90632_data {
> > struct i2c_client *client;
> > struct mutex lock; /* Multiple reads for single measurement */
> > struct regmap *regmap;
> > u16 emissivity;
> > +   u8 mtyp; /* measurement type - to enable extended range 
> > calculations */
> > +   u32 object_ambient_temperature;
> >  };
>
> As a separate patch to have a kernel doc for this, there are plenty of
> examples in the kernel, but I will show below an example
>
> /**
>  * struct foo - private data for the MLX90632 device
>  * @client: I2C client of the device
>  * @bar: ...
>  ...
>  */
> struct foo {
>   struct i2c_client *client;
>   ... bar;
>   ...
> };
>
> ...
>
> > +   if ((type != MLX90632_MTYP_MEDICAL) && (type != 
> > MLX90632_MTYP_EXTENDED))
> > +   return -EINVAL;
>
> So, just to point out what the difference between & and &&  (even for
> boolean): the former one forces both sides of operand to be executed,
> while && checks for only first in case of false.
>

OK, I am OK with evaluating first only. I mostly thought it is boolean
vs bitwise, but I keep on learning.

> ...
>
> > +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap,
> > + s16 *ambient_new_raw, s16 
> > *ambient_old_raw)
> > +{
> > +   unsigned int read_tmp;
> > +   int ret;
> > +
> > +   ret = regmap_read(regmap, MLX90632_RAM_3(17), &read_tmp);
> > +   if (ret < 0)
> > +   return ret;
> > +   *ambient_new_raw = (s16)read_tmp;
>
> Again the same question, do you need all these castings?
>

These castings are in fact needed. You read unsigned integer, but the
return value is signed integer. Without the cast it did not extend the
signed bit, but just wrote the value to signed. Also I find it more
obvious with casts, that I did not "accidentally" convert to signed.

> > +   ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp);
>
> These 17, 18 and 19 should be defined with descriptive names.
>

When we started 0 to 19 were all open for access, from userspace, then
only 1 and 2 were used with calculations, and now we use 17, 18 and
19. Matter of fact is, I can't provide a descriptive name as it
depends on DSP version and as you can see now within the same DSP
version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
that might not be true in the next configuration, so I rather keep the
naming as in the datasheet.

> > +   if (ret < 0)
> > +   return ret;
> > +   *ambient_old_raw = (s16)read_tmp;
> > +
> > +   return 0;
> > +}
>
> ...
>
> > +   int tries = 4;
>
> > +   while (tries-- > 0) {
> > +   ret = mlx90632_perform_measurement(data);
> > +   if (ret < 0)
> > +   goto read_unlock;
> > +
> > +   if (ret == 19)
> > +   break;
> > +   }
> > +   if (tries < 0) {
> > +   ret = -ETIMEDOUT;
> > +   goto read_unlock;
> > +   }
>
> Again, please use
>
> unsigned int tries = 4; // or should be 5?
>
> do {
>   ...
> } while (ret != ..._ LAST_CHANNEL && --tries);
> if (!tries)
>   ...
>
> It will really make the code easier to read.
>

I thought 

[PATCH v3 1/2] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..763a148a0095 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
-   s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+   s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
@@ -393,10 +393,6 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
@@ -406,17 +402,21 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 
ambient, s32 Ea, s32 Eb,
 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 u16 tmp_emi)
 {
-   s64 kTA, kTA0, TAdut;
+   s64 kTA, kTA0, TAdut, TAdut4;
s64 temp = 25000;
s8 i;
 
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL)  + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

[PATCH v3 0/2] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

Since the second patch is dependent on the first and was still not
merged, I have decided to send them together. First patch just makes
second one more readable as it splits out the repeated calculation and
that enables the second patch to tweak the variable to the new
condition.

Crt Mori (2):
  iio:temperature:mlx90632: Reduce number of equal calulcations
  iio:temperature:mlx90632: Adding extended calibration option

 drivers/iio/temperature/mlx90632.c | 233 +++--
 1 file changed, 223 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v3 2/2] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

For some time the market wants medical grade accuracy in medical range,
while still retaining the declared accuracy outside of the medical range
within the same sensor. That is why we created extended calibration
which is automatically switched to when object temperature is too high.

This patch also introduces the object_ambient_temperature variable which
is needed for more accurate calculation of the object infra-red
footprint as sensor's ambient temperature might be totally different
than what the ambient temperature is at object and that is why we can
have some more errors which can be eliminated. Currently this temperature
is fixed at 25, but the interface to adjust it by user (with external
sensor or just IR measurement of the other object which acts as ambient),
will be introduced in another commit.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 225 -
 1 file changed, 219 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 763a148a0095..049c1217a166 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +59,8 @@
 /* Control register address - volatile */
 #define MLX90632_REG_CONTROL   0x3001 /* Control Register address */
 #define   MLX90632_CFG_PWR_MASKGENMASK(2, 1) /* PowerMode Mask 
*/
+#define   MLX90632_CFG_MTYP_MASK   GENMASK(8, 4) /* Meas select 
Mask */
+
 /* PowerModes statuses */
 #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
 #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
@@ -65,6 +68,18 @@
 #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
 
+/* Measurement types */
+#define MLX90632_MTYP_MEDICAL 0
+#define MLX90632_MTYP_EXTENDED 17
+
+/* Measurement type select*/
+#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4)
+#define MLX90632_MTYP_STATUS_MEDICAL 
MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL)
+#define MLX90632_MTYP_STATUS_EXTENDED 
MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED)
+
+/* I2C command register - volatile */
+#define MLX90632_REG_I2C_CMD0x3005 /* I2C command Register address */
+
 /* Device status register - volatile */
 #define MLX90632_REG_STATUS0x3fff /* Device status register */
 #define   MLX90632_STAT_BUSY   BIT(10) /* Device busy indicator */
@@ -81,6 +96,8 @@
 /* Magic constants */
 #define MLX90632_ID_MEDICAL0x0105 /* EEPROM DSPv5 Medical device id */
 #define MLX90632_ID_CONSUMER   0x0205 /* EEPROM DSPv5 Consumer device id */
+#define MLX90632_ID_EXTENDED   0x0505 /* EEPROM DSPv5 Extended range device id 
*/
+#define MLX90632_ID_MASK   GENMASK(14, 0) /* DSP version and device ID in 
EE_VERSION */
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
@@ -88,16 +105,20 @@
 #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 struct mlx90632_data {
struct i2c_client *client;
struct mutex lock; /* Multiple reads for single measurement */
struct regmap *regmap;
u16 emissivity;
+   u8 mtyp; /* measurement type - to enable extended range calculations */
+   u32 object_ambient_temperature;
 };
 
 static const struct regmap_range mlx90632_volatile_reg_range[] = {
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -113,6 +134,7 @@ static const struct regmap_range mlx90632_read_reg_range[] 
= {
regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR),
regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb),
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -199,6 +221,26 @@ static int mlx90632_perform_measurement(struct 
mlx90632_data *data)
return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
 }
 
+static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
+{
+   int ret;
+
+   if ((type != MLX90632_MTYP_MEDICAL

Re: [PATCH 2/2 resend] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

On Fri, 7 Aug 2020 at 13:13, Crt Mori  wrote:
>
> On Fri, 7 Aug 2020 at 12:29, Andy Shevchenko  
> wrote:
> >
> > On Fri, Aug 7, 2020 at 12:21 PM Crt Mori  wrote:
> >
> > Oh yeah, you are right, there will be some comments :-)
> >
>
> Told ya. No matter how many times I go through it, I always find
> something. I will prepare v3 with fixes, except for some additional
> questions below.
>
I tried some suggestions and it was just not working. See the
explanation below. I am resending v3 without those.

> > > For some time market wants medical grade accuracy in medical range,
> >
> > the market
> >
> > > while still retaining the declared accuracy outside of the medical range
> > > within the same sensor. That is why we created extended calibration
> > > which is automatically switched to when object temperature is too high.
> > >
> > > This patch also introduces the object_ambient_temperature variable which
> > > is needed for more accurate calculation of the object infra-red
> > > footprint as sensor's ambient temperature might be totally different
> > > than what the ambient temperature is at object and that is why we can
> > > have some more error which can be eliminated. Currently this temperature
> >
> > errors
> >
> > > is fixed at 25, but interface to adjust it by user (with external sensor
> >
> > the interface
> >
> > > or just IR measurement of the another object which acts as ambient),
> >
> > 'of another' or 'the other' if we know what it is exactly.
> >
> > > will be introduced in another commit.
> >
> > ...
> >
> > >  struct mlx90632_data {
> > > struct i2c_client *client;
> > > struct mutex lock; /* Multiple reads for single measurement */
> > > struct regmap *regmap;
> > > u16 emissivity;
> >
> > > +   u8 mtyp; /* measurement type - to enable extended range 
> > > calculations */
> >
> > Perhaps better to switch this struct to follow kernel doc in one of
> > preparatory patches and add the description of this field accordingly.
> >
>
> Can you explain a bit more? I was looking in kernel doc, but could not
> find much about how to comment these members.
>
> > > +   u32 object_ambient_temperature;
> > >  };
> >
> > ...
> >
> > > +static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
> > > +{
> > > +   int ret;
> > > +
> > > +   if ((type != MLX90632_MTYP_MEDICAL) & (type != 
> > > MLX90632_MTYP_EXTENDED))
> > > +   return -EINVAL;
> >
> > Not sure I understand the point of & vs. && here.
> >
>
> Should indeed be &&, if it is needed at all. Both are boolean types.
>
> > > +   ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, 
> > > MLX90632_RESET_CMD);
> > > +   if (ret < 0)
> > > +   return ret;
> > > +
> > > +   ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > > +(MLX90632_CFG_MTYP_MASK | 
> > > MLX90632_CFG_PWR_MASK),
> > > +(MLX90632_MTYP_STATUS(type) | 
> > > MLX90632_PWR_STATUS_HALT));
> > > +   if (ret < 0)
> > > +   return ret;
> > > +
> > > +   mlx90632_pwr_continuous(regmap);
> >
> > > +
> > > +   return ret;
> >
> > Since you are using ' < 0' above and below (and I think it doesn't
> > worth it, i.o.w. you may drop them) here is something interesting
> > might be returned (actually not, see first part of this sentence).
> > Should be
> >
> > return 0;
> >
> > > +}
> >
> > ...
> >
> > > +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap,
> > > + s16 *ambient_new_raw, s16 
> > > *ambient_old_raw)
> > > +{
> >
> > > +   int ret;
> > > +   unsigned int read_tmp;
> >
> > Please keep them in reversed xmas tree format.
> >
> > > +
> > > +   ret = regmap_read(regmap, MLX90632_RAM_3(17), &read_tmp);
> > > +   if (ret < 0)
> > > +   return ret;
> > > +   *ambient_new_raw = (s16)read_tmp;
> > > +
> > > +   ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp);
> > > +  

Re: [PATCH 2/2 resend] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

On Fri, 7 Aug 2020 at 12:29, Andy Shevchenko  wrote:
>
> On Fri, Aug 7, 2020 at 12:21 PM Crt Mori  wrote:
>
> Oh yeah, you are right, there will be some comments :-)
>

Told ya. No matter how many times I go through it, I always find
something. I will prepare v3 with fixes, except for some additional
questions below.

> > For some time market wants medical grade accuracy in medical range,
>
> the market
>
> > while still retaining the declared accuracy outside of the medical range
> > within the same sensor. That is why we created extended calibration
> > which is automatically switched to when object temperature is too high.
> >
> > This patch also introduces the object_ambient_temperature variable which
> > is needed for more accurate calculation of the object infra-red
> > footprint as sensor's ambient temperature might be totally different
> > than what the ambient temperature is at object and that is why we can
> > have some more error which can be eliminated. Currently this temperature
>
> errors
>
> > is fixed at 25, but interface to adjust it by user (with external sensor
>
> the interface
>
> > or just IR measurement of the another object which acts as ambient),
>
> 'of another' or 'the other' if we know what it is exactly.
>
> > will be introduced in another commit.
>
> ...
>
> >  struct mlx90632_data {
> > struct i2c_client *client;
> > struct mutex lock; /* Multiple reads for single measurement */
> > struct regmap *regmap;
> > u16 emissivity;
>
> > +   u8 mtyp; /* measurement type - to enable extended range 
> > calculations */
>
> Perhaps better to switch this struct to follow kernel doc in one of
> preparatory patches and add the description of this field accordingly.
>

Can you explain a bit more? I was looking in kernel doc, but could not
find much about how to comment these members.

> > +   u32 object_ambient_temperature;
> >  };
>
> ...
>
> > +static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
> > +{
> > +   int ret;
> > +
> > +   if ((type != MLX90632_MTYP_MEDICAL) & (type != 
> > MLX90632_MTYP_EXTENDED))
> > +   return -EINVAL;
>
> Not sure I understand the point of & vs. && here.
>

Should indeed be &&, if it is needed at all. Both are boolean types.

> > +   ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, 
> > MLX90632_RESET_CMD);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> > +(MLX90632_CFG_MTYP_MASK | 
> > MLX90632_CFG_PWR_MASK),
> > +(MLX90632_MTYP_STATUS(type) | 
> > MLX90632_PWR_STATUS_HALT));
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   mlx90632_pwr_continuous(regmap);
>
> > +
> > +   return ret;
>
> Since you are using ' < 0' above and below (and I think it doesn't
> worth it, i.o.w. you may drop them) here is something interesting
> might be returned (actually not, see first part of this sentence).
> Should be
>
> return 0;
>
> > +}
>
> ...
>
> > +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap,
> > + s16 *ambient_new_raw, s16 
> > *ambient_old_raw)
> > +{
>
> > +   int ret;
> > +   unsigned int read_tmp;
>
> Please keep them in reversed xmas tree format.
>
> > +
> > +   ret = regmap_read(regmap, MLX90632_RAM_3(17), &read_tmp);
> > +   if (ret < 0)
> > +   return ret;
> > +   *ambient_new_raw = (s16)read_tmp;
> > +
> > +   ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp);
> > +   if (ret < 0)
> > +   return ret;
> > +   *ambient_old_raw = (s16)read_tmp;
>
> > +   return ret;
>
> Same comments as per previous function.
>
> > +}
>
> > +static int mlx90632_read_object_raw_extended(struct regmap *regmap, s16 
> > *object_new_raw)
> > +{
> > +   int ret;
> > +   unsigned int read_tmp;
> > +   s32 read;
>
> Besides all above comments being applicable here...
>
> > +   ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > +   if (ret < 0)
> > +   return ret;
> > +   read = (s16)read_tmp;
> > +
> > +   ret = regmap_read(regmap, MLX90632_

Re: [PATCH 0/2] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

On Fri, 7 Aug 2020 at 11:29, Andy Shevchenko  wrote:
>
> On Fri, Aug 7, 2020 at 12:22 PM Crt Mori  wrote:
> >
> > Since the second patch is dependent on the first and was still not
> > merged, I have decided to send them together. First patch just makes
> > second one more readable as it splits out the repeated calculation and
> > that enables the second patch to tweak the variable to the new
> > condition.
> >
>
> When at it, bump the version relative to the maximum of all patches
> involved, like v2 should be for all patches in this series.
> So, there is -v option to git format-patch to help with this.

Thanks for the heads-up. I was doing this by hand so far :/

I assume there will be some comments, so I will follow this in future
submissions.

>
>
> --
> With Best Regards,
> Andy Shevchenko

[PATCH 2/2 resend] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

For some time market wants medical grade accuracy in medical range,
while still retaining the declared accuracy outside of the medical range
within the same sensor. That is why we created extended calibration
which is automatically switched to when object temperature is too high.

This patch also introduces the object_ambient_temperature variable which
is needed for more accurate calculation of the object infra-red
footprint as sensor's ambient temperature might be totally different
than what the ambient temperature is at object and that is why we can
have some more error which can be eliminated. Currently this temperature
is fixed at 25, but interface to adjust it by user (with external sensor
or just IR measurement of the another object which acts as ambient),
will be introduced in another commit.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 219 -
 1 file changed, 217 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 763a148a0095..bb35a65bb9f0 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -58,6 +58,8 @@
 /* Control register address - volatile */
 #define MLX90632_REG_CONTROL   0x3001 /* Control Register address */
 #define   MLX90632_CFG_PWR_MASKGENMASK(2, 1) /* PowerMode Mask 
*/
+#define   MLX90632_CFG_MTYP_MASK   GENMASK(8, 4) /* Meas select 
Mask */
+
 /* PowerModes statuses */
 #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
 #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
@@ -65,6 +67,18 @@
 #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
 
+/* Measurement types */
+#define MLX90632_MTYP_MEDICAL 0
+#define MLX90632_MTYP_EXTENDED 17
+
+/* Measurement type select*/
+#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4)
+#define MLX90632_MTYP_STATUS_MEDICAL 
MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL)
+#define MLX90632_MTYP_STATUS_EXTENDED 
MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED)
+
+/* I2C command register - volatile */
+#define MLX90632_REG_I2C_CMD0x3005 /* I2C command Register address */
+
 /* Device status register - volatile */
 #define MLX90632_REG_STATUS0x3fff /* Device status register */
 #define   MLX90632_STAT_BUSY   BIT(10) /* Device busy indicator */
@@ -81,6 +95,8 @@
 /* Magic constants */
 #define MLX90632_ID_MEDICAL0x0105 /* EEPROM DSPv5 Medical device id */
 #define MLX90632_ID_CONSUMER   0x0205 /* EEPROM DSPv5 Consumer device id */
+#define MLX90632_ID_EXTENDED   0x0505 /* EEPROM DSPv5 Extended range device id 
*/
+#define MLX90632_ID_MASK   GENMASK(14, 0) /* DSP version and device ID in 
EE_VERSION */
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
@@ -88,16 +104,20 @@
 #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 struct mlx90632_data {
struct i2c_client *client;
struct mutex lock; /* Multiple reads for single measurement */
struct regmap *regmap;
u16 emissivity;
+   u8 mtyp; /* measurement type - to enable extended range calculations */
+   u32 object_ambient_temperature;
 };
 
 static const struct regmap_range mlx90632_volatile_reg_range[] = {
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -113,6 +133,7 @@ static const struct regmap_range mlx90632_read_reg_range[] 
= {
regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR),
regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb),
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -199,6 +220,28 @@ static int mlx90632_perform_measurement(struct 
mlx90632_data *data)
return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
 }
 
+static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
+{
+   int ret;
+
+   if ((type != MLX90632_MTYP_MEDICAL) & (type != MLX90632_MTYP_EXTENDED))
+   return -EINVAL;
+
+   ret = regmap_write(regmap, 

[PATCH 1/2 v2 resend] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..763a148a0095 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
-   s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+   s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
@@ -393,10 +393,6 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
@@ -406,17 +402,21 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 
ambient, s32 Ea, s32 Eb,
 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 u16 tmp_emi)
 {
-   s64 kTA, kTA0, TAdut;
+   s64 kTA, kTA0, TAdut, TAdut4;
s64 temp = 25000;
s8 i;
 
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL)  + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

[PATCH 0/2] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

Since the second patch is dependent on the first and was still not
merged, I have decided to send them together. First patch just makes
second one more readable as it splits out the repeated calculation and
that enables the second patch to tweak the variable to the new
condition.

Crt Mori (2):
  iio:temperature:mlx90632: Reduce number of equal calulcations
  iio:temperature:mlx90632: Adding extended calibration option

 drivers/iio/temperature/mlx90632.c | 235 +++--
 1 file changed, 225 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH] iio:temperature:mlx90632: Some stylefixing leftovers

[Crt Mori]

There is some inconsistency and whitespace cleanup performed in this
patch. It was done on top of my other patches, but I can rebase to head
of the togreg branch if it would go in sooner.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index bb35a65bb9f0..d966e5387c48 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -100,10 +100,10 @@
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
-#define MLX90632_REF_1212LL /**< ResCtrlRef value of Ch 1 or 
Ch 2 */
-#define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
-#define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
-#define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_REF_1212LL /* ResCtrlRef value of Ch 1 or Ch 2 */
+#define MLX90632_REF_3 12LL /* ResCtrlRef value of Channel 3 */
+#define MLX90632_MAX_MEAS_NUM  31 /* Maximum measurements in list */
+#define MLX90632_SLEEP_DELAY_MS 3000 /* Autosleep delay */
 #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 struct mlx90632_data {
@@ -884,7 +884,7 @@ static int mlx90632_probe(struct i2c_client *client,
mlx90632->mtyp = MLX90632_MTYP_EXTENDED;
} else if ((read & MLX90632_DSP_MASK) == MLX90632_DSP_VERSION) {
dev_dbg(&client->dev,
-   "Detected Unknown EEPROM calibration %x\n", read);  
+   "Detected Unknown EEPROM calibration %x\n", read);
} else {
dev_err(&client->dev,
"Wrong DSP version %x (expected %x)\n",
-- 
2.25.1

[PATCH] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

For some time market wants medical grade accuracy in medical range,
while still retaining the declared accuracy outside of the medical range
within the same sensor. That is why we created extended calibration
which is automatically switched to when object temperature is too high.

This patch also introduces the object_ambient_temperature variable which
is needed for more accurate calculation of the object infra-red
footprint as sensor's ambient temperature might be totally different
than what the ambient temperature is at object and that is why we can
have some more error which can be eliminated. Currently this temperature
is fixed at 25, but interface to adjust it by user (with external sensor
or just IR measurement of the another object which acts as ambient),
will be introduced in another commit.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 219 -
 1 file changed, 217 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 763a148a0095..bb35a65bb9f0 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -58,6 +58,8 @@
 /* Control register address - volatile */
 #define MLX90632_REG_CONTROL   0x3001 /* Control Register address */
 #define   MLX90632_CFG_PWR_MASKGENMASK(2, 1) /* PowerMode Mask 
*/
+#define   MLX90632_CFG_MTYP_MASK   GENMASK(8, 4) /* Meas select 
Mask */
+
 /* PowerModes statuses */
 #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1)
 #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */
@@ -65,6 +67,18 @@
 #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */
 #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
 
+/* Measurement types */
+#define MLX90632_MTYP_MEDICAL 0
+#define MLX90632_MTYP_EXTENDED 17
+
+/* Measurement type select*/
+#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4)
+#define MLX90632_MTYP_STATUS_MEDICAL 
MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL)
+#define MLX90632_MTYP_STATUS_EXTENDED 
MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED)
+
+/* I2C command register - volatile */
+#define MLX90632_REG_I2C_CMD0x3005 /* I2C command Register address */
+
 /* Device status register - volatile */
 #define MLX90632_REG_STATUS0x3fff /* Device status register */
 #define   MLX90632_STAT_BUSY   BIT(10) /* Device busy indicator */
@@ -81,6 +95,8 @@
 /* Magic constants */
 #define MLX90632_ID_MEDICAL0x0105 /* EEPROM DSPv5 Medical device id */
 #define MLX90632_ID_CONSUMER   0x0205 /* EEPROM DSPv5 Consumer device id */
+#define MLX90632_ID_EXTENDED   0x0505 /* EEPROM DSPv5 Extended range device id 
*/
+#define MLX90632_ID_MASK   GENMASK(14, 0) /* DSP version and device ID in 
EE_VERSION */
 #define MLX90632_DSP_VERSION   5 /* DSP version */
 #define MLX90632_DSP_MASK  GENMASK(7, 0) /* DSP version in EE_VERSION */
 #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */
@@ -88,16 +104,20 @@
 #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */
 #define MLX90632_MAX_MEAS_NUM  31 /**< Maximum measurements in list */
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
 struct mlx90632_data {
struct i2c_client *client;
struct mutex lock; /* Multiple reads for single measurement */
struct regmap *regmap;
u16 emissivity;
+   u8 mtyp; /* measurement type - to enable extended range calculations */
+   u32 object_ambient_temperature;
 };
 
 static const struct regmap_range mlx90632_volatile_reg_range[] = {
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -113,6 +133,7 @@ static const struct regmap_range mlx90632_read_reg_range[] 
= {
regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR),
regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb),
regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL),
+   regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD),
regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS),
regmap_reg_range(MLX90632_RAM_1(0),
 MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)),
@@ -199,6 +220,28 @@ static int mlx90632_perform_measurement(struct 
mlx90632_data *data)
return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2;
 }
 
+static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
+{
+   int ret;
+
+   if ((type != MLX90632_MTYP_MEDICAL) & (type != MLX90632_MTYP_EXTENDED))
+   return -EINVAL;
+
+   ret = regmap_write(regmap, 

[PATCH v2] iio:temperature:mlx90632: Reduce number of equal calculations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..763a148a0095 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
-   s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+   s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
@@ -393,10 +393,6 @@ static s32 mlx90632_calc_temp_object_iteration(s32 
prev_object_temp, s64 object,
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
@@ -406,17 +402,21 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 
ambient, s32 Ea, s32 Eb,
 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 u16 tmp_emi)
 {
-   s64 kTA, kTA0, TAdut;
+   s64 kTA, kTA0, TAdut, TAdut4;
s64 temp = 25000;
s8 i;
 
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL)  + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

Hi Andy,
Thanks for the comments. This is indeed a cut-out section of what I
wanted to submit next.

On Mon, 3 Aug 2020 at 18:35, Andy Shevchenko  wrote:
>
> On Mon, Aug 3, 2020 at 6:17 PM Crt Mori  wrote:
> >
> > TAdut4 was calculated each iteration although it did not change. In light
> > of near future additions of the Extended range DSP calculations, this
> > function refactoring will help reduce unrelated changes in that series as
> > well as reduce the number of new functions needed.
>
> Okay!
>
> > Also converted shifts in this function of signed integers to divisions as
> > that is less implementation-defined behavior.
>
> This is what I'm wondering about. Why?
>
> ...

The reason for this is that whenever something is wrong with the
calculation I am looking into the shifts which are
implementation-defined and might not keep the signed bit. Division
however would.

>
> > -   Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
> > -   Hb_customer = ((s64)Hb * 100) >> 10ULL;
> > +   Ha_customer = div64_s64((s64)Ha * 100LL, 16384);
> > +   Hb_customer = div64_s64((s64)Hb * 100, 1024);
>
> Have you checked the code on 32-bit machines?
> As far as I can see the div64_*64() do not have power of two divisor
> optimizations. I bet it will generate a bulk of unneeded code.
>
> ...
>
> > -   calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
> > -* 1000LL)) >> 36LL;
> > -   calcedKsTA = ((s64)(Fb * (TAdut - 25 * 100LL))) >> 36LL;
> > -   Alpha_corr = div64_s64s64)(Fa * 100LL) >> 46LL)
> > -   * Ha_customer), 1000LL);
>
> > +   calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 
> > 1000LL)
> > +* 1000LL), 68719476736);
> > +   calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 100LL)), 
> > 68719476736);
> > +   Alpha_corr = div64_s64(div64_s64((s64)(Fa * 100LL), 
> > 70368744177664)
> > +  * Ha_customer, 1000LL);
>
> This is less readable and full of magic numbers in comparison to the
> above (however, also full of magics, but at least gives better hint).
>
> ...

These are coefficients so there is not much to unmagic. I can keep the
shifts, if you think that is more readable or add comments after lines
with 2^46 or something?
>
> > +   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
> > +   (div64_s64(TAdut, 1LL) + 27315) *
> > +   (div64_s64(TAdut, 1LL)  + 27315) *
> > +   (div64_s64(TAdut, 1LL) + 27315);
>
> Shouldn't you switch to definitions from units.h? (perhaps as a separate 
> change)
>
> --
> With Best Regards,
> Andy Shevchenko

[PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Also converted shifts in this function of signed integers to divisions as
that is less implementation-defined behavior.

Signed-off-by: Crt Mori 
---
 drivers/iio/temperature/mlx90632.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c 
b/drivers/iio/temperature/mlx90632.c
index eaca6ba06864..d7ba0b2fe3c0 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,29 +374,25 @@ static s32 mlx90632_calc_temp_ambient(s16 
ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 
object,
-  s64 TAdut, s32 Fa, s32 Fb,
+  s64 TAdut, s64 TAdut4, s32 Fa, 
s32 Fb,
   s32 Ga, s16 Ha, s16 Hb,
   u16 emissivity)
 {
s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
s64 Ha_customer, Hb_customer;
 
-   Ha_customer = ((s64)Ha * 100LL) >> 14ULL;
-   Hb_customer = ((s64)Hb * 100) >> 10ULL;
+   Ha_customer = div64_s64((s64)Ha * 100LL, 16384);
+   Hb_customer = div64_s64((s64)Hb * 100, 1024);
 
-   calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
-* 1000LL)) >> 36LL;
-   calcedKsTA = ((s64)(Fb * (TAdut - 25 * 100LL))) >> 36LL;
-   Alpha_corr = div64_s64s64)(Fa * 100LL) >> 46LL)
-   * Ha_customer), 1000LL);
+   calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
+* 1000LL), 68719476736);
+   calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 100LL)), 
68719476736);
+   Alpha_corr = div64_s64(div64_s64((s64)(Fa * 100LL), 
70368744177664)
+  * Ha_customer, 1000LL);
Alpha_corr *= ((s64)(1 * 100LL + calcedKsTO + calcedKsTA));
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
-   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315) *
-   (div64_s64(TAdut, 1LL)  + 27315) *
-   (div64_s64(TAdut, 1LL) + 27315);
 
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + TAdut4))
- 27315 - Hb_customer) * 10;
@@ -413,10 +409,14 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 
ambient, s32 Ea, s32 Eb,
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
TAdut = div64_s64(((ambient - kTA0) * 100LL), kTA) + 25 * 100LL;
+   TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315) *
+   (div64_s64(TAdut, 1LL)  + 27315) *
+   (div64_s64(TAdut, 1LL) + 27315);
 
/* Iterations of calculation as described in datasheet */
for (i = 0; i < 5; ++i) {
-   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+   temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, 
TAdut4,
   Fa, Fb, Ga, Ha, Hb,
   tmp_emi);
}
-- 
2.25.1

Re: [PATCH 09/30] iio: temperature: mlx90632: Function parameter descriptions must match exactly

[Crt Mori]

Acked-by: Crt Mori 

On Fri, 17 Jul 2020 at 18:56, Lee Jones  wrote:
>
> '*'s are not welcome in kerneldoc parameter names.
>
> Fixes the following W=1 kernel build warning(s):
>
>  drivers/iio/temperature/mlx90632.c:175: warning: Function parameter or 
> member 'data' not described in 'mlx90632_perform_measurement'
>
> Cc: Crt Mori 
> Signed-off-by: Lee Jones 
> ---
>  drivers/iio/temperature/mlx90632.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/temperature/mlx90632.c 
> b/drivers/iio/temperature/mlx90632.c
> index eaca6ba068646..b9a8089be3f63 100644
> --- a/drivers/iio/temperature/mlx90632.c
> +++ b/drivers/iio/temperature/mlx90632.c
> @@ -164,8 +164,8 @@ static s32 mlx90632_pwr_continuous(struct regmap *regmap)
>  }
>
>  /**
> - * mlx90632_perform_measurement - Trigger and retrieve current measurement 
> cycle
> - * @*data: pointer to mlx90632_data object containing regmap information
> + * mlx90632_perform_measurement() - Trigger and retrieve current measurement 
> cycle
> + * @data: pointer to mlx90632_data object containing regmap information
>   *
>   * Perform a measurement and return latest measurement cycle position 
> reported
>   * by sensor. This is a blocking function for 500ms, as that is default 
> sensor
> --
> 2.25.1
>

Re: fix int_sqrt() for very large numbers

[Crt Mori]

On Mon, 21 Jan 2019 at 01:20, Linus Torvalds
 wrote:
>
> On Sun, Jan 20, 2019 at 4:15 AM Florian La Roche
>  wrote:
> >
> > @@ -52,7 +52,7 @@ u32 int_sqrt64(u64 x)
> > if (x <= ULONG_MAX)
> > return int_sqrt((unsigned long) x);
> >
> > -   m = 1ULL << (fls64(x) & ~1ULL);
> > +   m = 1ULL << ((fls64(x) - 1) & ~1ULL);
>
> I've applied this part of the patch as commit fbfaf851902c ("fix
> int_sqrt64() for very large numbers") with slightly edited commit
> log.
>

Thanks for the patch - I its indeed my copy-paste error, because
__fls64 does not exist on 32bit CPU, but the fls64 is not "equal"
replacement. I am very sorry for the bug.

> I still think there are some oddities in here in the types. I
> mentioned the caller that unnecessarily does the int_sqrt64() twice,
> even though the outer one doesn't actually take a 64-bit value.
>

True. This is oddity is originating from time where mlx90632 used its
own function for int_sqrt64 on 32bit.

> But in the very line above, there's another type oddity: the "& ~1ULL"
> is entirely the wrong type. The shift *count* shouldn't be an unsigned
> long long, so that type doesn't make much sense. It should be just a
> ~1, or even just "62".
>

This was also inline with above copy-paste and variable expansion to
force the 64bit everywhere. I will prepare a patch to clean this line
to ~1, question is why does the int_sqrt is having UL if this should
just be "62". I was thinking because we want to cast to the type
before we shift.

> But I didn't actually start micro-editing the patch, and just did that
> one-liner off-by-one fix.
>
>  Linus

Re: fix int_sqrt() for very large numbers

[Crt Mori]

On Sun, 20 Jan 2019 at 09:31, Crt Mori  wrote:
>
> On Sun, 20 Jan 2019 at 04:49, Linus Torvalds
>  wrote:
> >
> > On Sun, Jan 20, 2019 at 12:01 PM Will Deacon  wrote:
> > >
> > > > @@ -52,7 +52,7 @@ u32 int_sqrt64(u64 x)
> > > >   if (x <= ULONG_MAX)
> > > >   return int_sqrt((unsigned long) x);
> > > >
> > > > - m = 1ULL << (fls64(x) & ~1ULL);
> > > > + m = 1ULL << ((fls64(x) - 1) & ~1ULL);
> > >
> > > This just looks like a copy-paste error because there isn't an __fls64().
> > > But I think your suggestion here is ok, given the previous check against
> > > ULONG_MAX.
> >
> > Hmm. We probably *should* add a __fls64().
> >
> > There looks to be only one user of int_sqrt64(), and that one is
> > confused. It does int_sqrt64() twice, but since the inner one will
> > reduce the range to 32 bits, the outer one is just silly.
>
> II have a usecase (mlx90632) where this calculation worked on arm64
> (nexus), but not in normal 32-bit arm (beaglebone). I have tried going
> with full u64 to u64, but I was persuaded that it is not necessary and
> testing on black body (sensor range from 0 - 80 degrees) confirmed
> that for my calculations u32/u64 is enough. Because of the testing

I have just re-read the patch submit discussion and a sqrt of 64bit
number can never be more than 32bit. That is why u32 return value is
enough. But I do not clearly remember if I tested with outside
int_sqrt (insted int_sqrt64). We still have everything, so I could
test if that is the suggestion.

> range (and keep in mind it is casted to signed after two sqrts) the
> high bit might never affect my end result, but I needed precision, not
> the range. Inside the function the b was 32bit on 32bit core, but I
> needed it to be 64bit. To keep it similar to existing int_sqrt, I have
> decided to just type all variables there to 64bit.
>
> We have implementation of this with doubles (see datasheet) and I
> ported it to integer on arm64. The end result was fairly similar
> calculation (for within object tempearture range from 0-80), between
> both.
>
> > That one user also had better not be overflowing into the high bit -
> > it uses "s64" as a type and does seem to use signed operatons, so high
> > bit set really means negative. sqrt() returning something odd for a
> > negative number wouldn't be all that odd in that context.
> >
> > But yes, our current int_sqrt64() does seem buggy as-is, because it's
> > *supposed* to work on u64's, even if I don't think we really have any
> > users that care.
>
> I introduced strong types for existing int_sqrt implementation to keep
> it aligned between 64bit and 32bit.
>
> Best regards,
> Crt
>
> > And as Will mentioned, the regular int_sqrt() looks perfectly fine,
> > and subtracting 1 from the __fls() return value would actually
> > _introduce_ a bug.
> >
> > Linus

Re: fix int_sqrt() for very large numbers

[Crt Mori]

On Sun, 20 Jan 2019 at 04:49, Linus Torvalds
 wrote:
>
> On Sun, Jan 20, 2019 at 12:01 PM Will Deacon  wrote:
> >
> > > @@ -52,7 +52,7 @@ u32 int_sqrt64(u64 x)
> > >   if (x <= ULONG_MAX)
> > >   return int_sqrt((unsigned long) x);
> > >
> > > - m = 1ULL << (fls64(x) & ~1ULL);
> > > + m = 1ULL << ((fls64(x) - 1) & ~1ULL);
> >
> > This just looks like a copy-paste error because there isn't an __fls64().
> > But I think your suggestion here is ok, given the previous check against
> > ULONG_MAX.
>
> Hmm. We probably *should* add a __fls64().
>
> There looks to be only one user of int_sqrt64(), and that one is
> confused. It does int_sqrt64() twice, but since the inner one will
> reduce the range to 32 bits, the outer one is just silly.

II have a usecase (mlx90632) where this calculation worked on arm64
(nexus), but not in normal 32-bit arm (beaglebone). I have tried going
with full u64 to u64, but I was persuaded that it is not necessary and
testing on black body (sensor range from 0 - 80 degrees) confirmed
that for my calculations u32/u64 is enough. Because of the testing
range (and keep in mind it is casted to signed after two sqrts) the
high bit might never affect my end result, but I needed precision, not
the range. Inside the function the b was 32bit on 32bit core, but I
needed it to be 64bit. To keep it similar to existing int_sqrt, I have
decided to just type all variables there to 64bit.

We have implementation of this with doubles (see datasheet) and I
ported it to integer on arm64. The end result was fairly similar
calculation (for within object tempearture range from 0-80), between
both.

> That one user also had better not be overflowing into the high bit -
> it uses "s64" as a type and does seem to use signed operatons, so high
> bit set really means negative. sqrt() returning something odd for a
> negative number wouldn't be all that odd in that context.
>
> But yes, our current int_sqrt64() does seem buggy as-is, because it's
> *supposed* to work on u64's, even if I don't think we really have any
> users that care.

I introduced strong types for existing int_sqrt implementation to keep
it aligned between 64bit and 32bit.

Best regards,
Crt

> And as Will mentioned, the regular int_sqrt() looks perfectly fine,
> and subtracting 1 from the __fls() return value would actually
> _introduce_ a bug.
>
> Linus

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 12 January 2018 at 10:41, David Laight  wrote:
> From: Crt Mori [mailto:c...@melexis.com]
>> Sent: 09 January 2018 15:18
>>
>> It has been some time now since this moved. I have decided not to use
>> David's implementation because I want to maintain also range above
>> 2^62
>
> The last version I did supported the full range.

Nothing changed below that comment, so I was assuming it is not
supported (or did I miss a mail?).

Also fls discussion had opposite opinions or it just seems inconclusive to me?

>
> David
>

So you all agree David Laight version is much better and should be
used instead of currently proposed version?

[PATCH v13 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Using same algorithm as int_sqrt()
with strong typing provides enough precision also on 32bit platforms,
but it sacrifices some performance. In case values are smaller than
ULONG_MAX the standard int_sqrt is used for calculation to maximize the
performance due to more native calculations.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  9 +
 lib/int_sqrt.c | 31 +++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..e4a3dc64e650 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -460,6 +460,15 @@ extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
 
+#if BITS_PER_LONG < 64
+u32 int_sqrt64(u64 x);
+#else
+static inline u32 int_sqrt64(u64 x)
+{
+   return (u32)int_sqrt(x);
+}
+#endif
+
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
 extern int panic_timeout;
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..77386bc611d0 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -7,6 +7,7 @@
 
 #include 
 #include 
+#include 
 
 /**
  * int_sqrt - rough approximation to sqrt
@@ -36,3 +37,33 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG < 64
+/**
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= ULONG_MAX)
+   return int_sqrt((unsigned long) x);
+
+   m = 1ULL << (fls64(x) & ~1ULL);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
+#endif
-- 
2.15.0

[PATCH v13 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

v8 Small style fixup in the comment

v9 Usage of fls64 as per Joe Perches suggestion with adjustment
to int_sqrt changes from Peter Zijlstra for int_sqrt64 function.
Also replaced the macro magic with a simple ifdef in source file.

v10 Put defintion for int_sqrt64 for 64 bit machines into the
header file as per suggestion of Joe Perches

v11 Commit rewording as per David Laight suggestion, because we
changed the function to newer int_sqrt in past versions.

v12 Simplification of the mlx90632 PM flow

v13 Use int_sqrt when values are smaller than ULONG_MAX as per
Joe Perches suggestion. Adjustment of PM flow with adding a
pm_runtime_disable in probe.

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 750 +
 include/linux/kernel.h |   9 +
 lib/int_sqrt.c |  31 +
 7 files changed, 838 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

Re: [PATCH v12 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 10 January 2018 at 09:33, Crt Mori  wrote:
> On 10 January 2018 at 09:15, Crt Mori  wrote:
>> On 9 January 2018 at 20:23, Joe Perches  wrote:
>>> On Tue, 2018-01-09 at 16:18 +0100, Crt Mori wrote:
>>>> There is no option to perform 64bit integer sqrt on 32bit platform.
>>>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>>>> be performed on 32bit platforms. Using same algorithm as int_sqrt()
>>>> with strong typing provides enough precision also on 32bit platforms,
>>>> but it sacrifices some performance.
>>> []
>>>> diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
>>> []
>>>> @@ -36,3 +37,34 @@ unsigned long int_sqrt(unsigned long x)
>>>>   return y;
>>>>  }
>>>>  EXPORT_SYMBOL(int_sqrt);
>>>> +
>>>> +#if BITS_PER_LONG < 64
>>>> +/**
>>>> + * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
>>>> + * is expected.
>>>> + * @x: 64bit integer of which to calculate the sqrt
>>>> + */
>>>> +u32 int_sqrt64(u64 x)
>>>> +{
>>>> + u64 b, m;
>>>> + u32 y = 0;
>>>> +
>>>> + if (x <= 1)
>>>> + return x;
>>>
>>> I think this should instead be:
>>>
>>> if (x <= INT_MAX)
>>> return int_sqrt((int)x);
>>>
>>> to reduce the loop cost below when the
>>> value is small enough.
>>>
>>
>> In existing int_sqrt its only 1 and I assume that is more to protect
>> from loop execution with 0 or 1. Since there is no difference (except
>> fls64) with int_sqrt I assume there is no need to call it to avoid
>> loop?
>>
>
> Nevermind, I see what you mean (should have thought longer before I
> written). The cost of below loop is because of 64bit calculation is
> not native on 32bit and we could just use 32bit calculation in that
> loop. Will send v13 with a fix for this.
>
Shouldn't I rather make it

 if (x <= ULONG_MAX)
 return int_sqrt((unsigned long) x);


>>>> +
>>>> + m = 1ULL << (fls64(x) & ~1ULL);
>>>> + while (m != 0) {
>>>> + b = y + m;
>>>> + y >>= 1;
>>>> +
>>>> + if (x >= b) {
>>>> + x -= b;
>>>> + y += m;
>>>> + }
>>>> + m >>= 2;
>>>> + }
>>>> +
>>>> + return y;
>>>> +}
>>>> +EXPORT_SYMBOL(int_sqrt64);
>>>> +#endif

Re: [PATCH v12 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 10 January 2018 at 09:15, Crt Mori  wrote:
> On 9 January 2018 at 20:23, Joe Perches  wrote:
>> On Tue, 2018-01-09 at 16:18 +0100, Crt Mori wrote:
>>> There is no option to perform 64bit integer sqrt on 32bit platform.
>>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>>> be performed on 32bit platforms. Using same algorithm as int_sqrt()
>>> with strong typing provides enough precision also on 32bit platforms,
>>> but it sacrifices some performance.
>> []
>>> diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
>> []
>>> @@ -36,3 +37,34 @@ unsigned long int_sqrt(unsigned long x)
>>>   return y;
>>>  }
>>>  EXPORT_SYMBOL(int_sqrt);
>>> +
>>> +#if BITS_PER_LONG < 64
>>> +/**
>>> + * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
>>> + * is expected.
>>> + * @x: 64bit integer of which to calculate the sqrt
>>> + */
>>> +u32 int_sqrt64(u64 x)
>>> +{
>>> + u64 b, m;
>>> + u32 y = 0;
>>> +
>>> + if (x <= 1)
>>> + return x;
>>
>> I think this should instead be:
>>
>> if (x <= INT_MAX)
>> return int_sqrt((int)x);
>>
>> to reduce the loop cost below when the
>> value is small enough.
>>
>
> In existing int_sqrt its only 1 and I assume that is more to protect
> from loop execution with 0 or 1. Since there is no difference (except
> fls64) with int_sqrt I assume there is no need to call it to avoid
> loop?
>

Nevermind, I see what you mean (should have thought longer before I
written). The cost of below loop is because of 64bit calculation is
not native on 32bit and we could just use 32bit calculation in that
loop. Will send v13 with a fix for this.

>>> +
>>> + m = 1ULL << (fls64(x) & ~1ULL);
>>> + while (m != 0) {
>>> + b = y + m;
>>> + y >>= 1;
>>> +
>>> + if (x >= b) {
>>> + x -= b;
>>> + y += m;
>>> + }
>>> + m >>= 2;
>>> + }
>>> +
>>> + return y;
>>> +}
>>> +EXPORT_SYMBOL(int_sqrt64);
>>> +#endif

Re: [PATCH v12 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 9 January 2018 at 20:23, Joe Perches  wrote:
> On Tue, 2018-01-09 at 16:18 +0100, Crt Mori wrote:
>> There is no option to perform 64bit integer sqrt on 32bit platform.
>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>> be performed on 32bit platforms. Using same algorithm as int_sqrt()
>> with strong typing provides enough precision also on 32bit platforms,
>> but it sacrifices some performance.
> []
>> diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
> []
>> @@ -36,3 +37,34 @@ unsigned long int_sqrt(unsigned long x)
>>   return y;
>>  }
>>  EXPORT_SYMBOL(int_sqrt);
>> +
>> +#if BITS_PER_LONG < 64
>> +/**
>> + * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
>> + * is expected.
>> + * @x: 64bit integer of which to calculate the sqrt
>> + */
>> +u32 int_sqrt64(u64 x)
>> +{
>> + u64 b, m;
>> + u32 y = 0;
>> +
>> + if (x <= 1)
>> + return x;
>
> I think this should instead be:
>
> if (x <= INT_MAX)
> return int_sqrt((int)x);
>
> to reduce the loop cost below when the
> value is small enough.
>

In existing int_sqrt its only 1 and I assume that is more to protect
from loop execution with 0 or 1. Since there is no difference (except
fls64) with int_sqrt I assume there is no need to call it to avoid
loop?

>> +
>> + m = 1ULL << (fls64(x) & ~1ULL);
>> + while (m != 0) {
>> + b = y + m;
>> + y >>= 1;
>> +
>> + if (x >= b) {
>> + x -= b;
>> + y += m;
>> + }
>> + m >>= 2;
>> + }
>> +
>> + return y;
>> +}
>> +EXPORT_SYMBOL(int_sqrt64);
>> +#endif

[PATCH v12 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Using same algorithm as int_sqrt()
with strong typing provides enough precision also on 32bit platforms,
but it sacrifices some performance.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  9 +
 lib/int_sqrt.c | 32 
 2 files changed, 41 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..e4a3dc64e650 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -460,6 +460,15 @@ extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
 
+#if BITS_PER_LONG < 64
+u32 int_sqrt64(u64 x);
+#else
+static inline u32 int_sqrt64(u64 x)
+{
+   return (u32)int_sqrt(x);
+}
+#endif
+
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
 extern int panic_timeout;
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..737fca54bb02 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -7,6 +7,7 @@
 
 #include 
 #include 
+#include 
 
 /**
  * int_sqrt - rough approximation to sqrt
@@ -36,3 +37,34 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG < 64
+/**
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 int_sqrt64(u64 x)
+{
+   u64 b, m;
+   u32 y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (fls64(x) & ~1ULL);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
+#endif
-- 
2.15.0

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

It has been some time now since this moved. I have decided not to use
David's implementation because I want to maintain also range above
2^62

Are there any additional objections for this not to go in as it is?

On 22 December 2017 at 14:44, Crt Mori  wrote:
>
> From simple strong typing of existing int_sqrt we came to something a
> bit more complex or better. Can we decide now which we want in, or I
> submit v12 and we decide then (although it is not a v12, but whole new
> thing)?
>
> On 21 December 2017 at 15:48, David Laight  wrote:
> > From: Peter Zijlstra
> >> Sent: 21 December 2017 14:12
> > ...
> >> > > This part above looks like FLS
> >> > It also does the rest of the required shifts.
> >>
> >> Still, fls() + shift is way faster on hardware that has an fls
> >> instruction.
> >>
> >> Writing out that binary search doesn't make sense.
> >
> > If the hardware doesn't have an appropriate fls instruction
> > the soft fls()will be worse.
> >
> > If you used fls() you'd still need quite a bit of code
> > to generate the correct shift and loop count adjustment.
> > Given the cost of the loop iterations the 3 tests are noise.
> > The open coded version is obviously correct...
> >
> > I didn't add the 4th one because the code always does 2 iterations.
> >
> > If you were really worried about performance there are faster
> > algorithms (even doing 2 or 4 bits a time is faster).
> >
> > David
> >

[PATCH v12 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

v8 Small style fixup in the comment

v9 Usage of fls64 as per Joe Perches suggestion with adjustment
to int_sqrt changes from Peter Zijlstra for int_sqrt64 function.
Also replaced the macro magic with a simple ifdef in source file.

v10 Put defintion for int_sqrt64 for 64 bit machines into the
header file as per suggestion of Joe Perches

v11 Commit rewording as per David Laight suggestion, because we
changed the function to newer int_sqrt in past versions.

v12 Simplification of the mlx90632 PM flow

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 754 +
 include/linux/kernel.h |   9 +
 lib/int_sqrt.c |  32 +
 7 files changed, 843 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

>From simple strong typing of existing int_sqrt we came to something a
bit more complex or better. Can we decide now which we want in, or I
submit v12 and we decide then (although it is not a v12, but whole new
thing)?

On 21 December 2017 at 15:48, David Laight  wrote:
> From: Peter Zijlstra
>> Sent: 21 December 2017 14:12
> ...
>> > > This part above looks like FLS
>> > It also does the rest of the required shifts.
>>
>> Still, fls() + shift is way faster on hardware that has an fls
>> instruction.
>>
>> Writing out that binary search doesn't make sense.
>
> If the hardware doesn't have an appropriate fls instruction
> the soft fls()will be worse.
>
> If you used fls() you'd still need quite a bit of code
> to generate the correct shift and loop count adjustment.
> Given the cost of the loop iterations the 3 tests are noise.
> The open coded version is obviously correct...
>
> I didn't add the 4th one because the code always does 2 iterations.
>
> If you were really worried about performance there are faster
> algorithms (even doing 2 or 4 bits a time is faster).
>
> David
>

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 21 December 2017 at 12:43, David Laight  wrote:
> From: Crt Mori
>> Sent: 20 December 2017 17:30
>> I did a quick run through unit tests for the sensor and the results
>> are way off
>> ...
>
> Try this version instead:
> unsigned int sqrt64(unsigned long long x_in)
> {
> unsigned int x = x_in >> 32;
>
> unsigned int b = 0;
> unsigned int y = 0;
> unsigned int i;

i can be u8. And I will still use explicit typing.

>
> i = 31;
> if (!x) {
> x = x_in;
> i = 15;
> }
> if (!(x & 0x)) {
> x <<= 16;
> i -= 8;
> }
> if (!(x & 0xff00)) {
> x <<= 8;
> i -= 4;
> }
> if (!(x & 0xf000)) {
> x <<= 4;
> i -= 2;
> }
>

This part above looks like FLS

> do {
> b <<= 2;
> b |= x >> 30;
> x <<= 2;
> if (i == 16)
> x = x_in;
> y <<= 1;
> if (b > y) {
> b -= ++y;
> y++;
> }
> } while (--i);
>
> /* 'b' becomes 33 bits if the input is greater than 2^62 */
> b <<= 1;
> b |= x >> 31;
> if (b > y || (b == y && x & (1u << 30)))
> y |= 1;
>
> return y;
> }
>
> I've tested that one with more values.
>
> David
>

This one indeed works. I did some more testing this morning and I am
fine with either.

So question is: Do I make change as per David's suggestion with his
sign-off, or leave the version it was before the change?

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 20 December 2017 at 17:46, David Laight  wrote:
> From: Crt Mori
>> Sent: 20 December 2017 16:17
>>
>> On 20 December 2017 at 17:00, Peter Zijlstra  wrote:
>> > On Wed, Dec 20, 2017 at 02:39:26PM +, David Laight wrote:
>> >
>> >> With minor changes it ought to be possible to remove most of the
>> >> 64bit arithmetic and shifts.
>> >>
>> >> If you care about performance then using 32 bit maths will be much faster.
>> >
>> > Some, u64 add/sub/shift isn't exactly expensive, but yes, I also
>> > indicated that improvement is possible. At the very least y can be made
>> > a u32 I suppose.
>>
>> OK, is there any more easy optimizations you see?
>
> I think this version works.
> It doesn't have the optimisation for small values.
>
> unsigned int sqrt64(unsigned long long x)
> {
> unsigned int x_hi = x >> 32;
>
> unsigned int b = 0;
> unsigned int y = 0;
> unsigned int i;
>
> for (i = 0; i < 32; i++) {
> b <<= 2;
> b |= x_hi >> 30;
> x_hi <<= 2;
> if (i == 15)
> x_hi = x;
> y <<= 1;
> if (b > y)
> b -= ++y;
> }
> return y;
> }
>
> Put it through cc -O3 -m32 -c -o sqrt64.o sqrt64.c and then objdump sqrt64.o
> and compare to that of your version.
>
> David
>

Wow, thanks. This seems like a major change.

I did a quick run through unit tests for the sensor and the results
are way off. On the sensor I had to convert double calculations to
integer calculations and target was to get end result within 0.02 degC
(with previous approximate sqrt implementation) in sensor working
range. This now gets into 3 degC delta at least and some are way off.
It might be off because of some scaling on the other hand during the
equation (not exactly comparing sqrt implementations here).

I will need a bit more testing of this to see if it is replaceable.

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 20 December 2017 at 17:00, Peter Zijlstra  wrote:
> On Wed, Dec 20, 2017 at 02:39:26PM +, David Laight wrote:
>
>> With minor changes it ought to be possible to remove most of the
>> 64bit arithmetic and shifts.
>>
>> If you care about performance then using 32 bit maths will be much faster.
>
> Some, u64 add/sub/shift isn't exactly expensive, but yes, I also
> indicated that improvement is possible. At the very least y can be made
> a u32 I suppose.

OK, is there any more easy optimizations you see?

[PATCH v11 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Using same algorithm as int_sqrt()
with strong typing provides enough precision also on 32bit platforms,
but it sacrifices some performance.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  9 +
 lib/int_sqrt.c | 31 +++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..e4a3dc64e650 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -460,6 +460,15 @@ extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
 
+#if BITS_PER_LONG < 64
+u32 int_sqrt64(u64 x);
+#else
+static inline u32 int_sqrt64(u64 x)
+{
+   return (u32)int_sqrt(x);
+}
+#endif
+
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
 extern int panic_timeout;
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..184da54677ec 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -7,6 +7,7 @@
 
 #include 
 #include 
+#include 
 
 /**
  * int_sqrt - rough approximation to sqrt
@@ -36,3 +37,33 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG < 64
+/**
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (fls64(x) & ~1ULL);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
+#endif
-- 
2.15.0

[PATCH v11 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

v8 Small style fixup in the comment

v9 Usage of fls64 as per Joe Perches suggestion with adjustment
to int_sqrt changes from Peter Zijlstra for int_sqrt64 function.
Also replaced the macro magic with a simple ifdef in source file.

v10 Put defintion for int_sqrt64 for 64 bit machines into the
header file as per suggestion of Joe Perches

v11 Commit rewording as per David Laight suggestion, because we
changed the function to newer int_sqrt in past versions.

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 759 +
 include/linux/kernel.h |   9 +
 lib/int_sqrt.c |  31 +
 7 files changed, 847 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

Re: [PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 20 December 2017 at 15:39, David Laight  wrote:
> From: Crt Mori
>> Sent: 20 December 2017 14:20
>>
>> There is no option to perform 64bit integer sqrt on 32bit platform.
>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>> be performed on 32bit platforms. Although int_sqrt() is a rough
>> approximation, the same algorithm is used in int_sqrt64() as good
>> enough on 32bit platform.
>
> The algorithm used gives an exact (rounded down) square root,
> not an approximation.
>

OK, noted. This is from new changed function indeed - will change in v11.

> With minor changes it ought to be possible to remove most of the
> 64bit arithmetic and shifts.
>
> If you care about performance then using 32 bit maths will be much faster.
>

I need precision not the performance. That is why I would like to
leave int_sqrt as one for good performance while my will be used when
you require a precision.

> David
>

Re: [PATCH v9 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 20 December 2017 at 10:56, Joe Perches  wrote:
> On Wed, 2017-12-20 at 10:33 +0100, Crt Mori wrote:
>> There is no option to perform 64bit integer sqrt on 32bit platform.
>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>> be performed on 32bit platforms. Although int_sqrt() is a rough
>> approximation, the same algorithm is used in int_sqrt64() as good
>> enough on 32bit platform.
> []
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> []
>> @@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
>>  extern int func_ptr_is_kernel_text(void *ptr);
>>
>>  unsigned long int_sqrt(unsigned long);
>> +u32 int_sqrt64(u64 x);
>
> #ifdef BITS_PER_LONG == 64
> static inline u32 int_sqrt64(u64 x)
> {
> re
> turn (u32)int_sqrt(x);
> }
> #else
> u32 int_sqrt64(u64 x);
> #endif
>
> []
>

OK, fixing it in v10.

>> +#if BITS_PER_LONG < 64
>> +/**
>> + * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
>> + * is expected.
>> + * @x: 64bit integer of which to calculate the sqrt
>> + */
>> +u32 int_sqrt64(u64 x)
>> +{
>> + u64 b, m, y = 0;
>> +
>> + if (x <= 1)
>> + return x;
>> +
>> + m = 1ULL << (fls64(x) & ~1ULL);
>> + while (m != 0) {
>> + b = y + m;
>> + y >>= 1;
>> +
>> + if (x >= b) {
>> + x -= b;
>> + y += m;
>> + }
>> + m >>= 2;
>> + }
>> +
>> + return y;
>> +}
>> +EXPORT_SYMBOL(int_sqrt64);
>> +#else
>> +u32 int_sqrt64(u64 x)
>> +{
>> + return int_sqrt(x);
>> +}
>> +EXPORT_SYMBOL(int_sqrt64);
>> +#endif
>
> This doesn't need to be EXPORT_SYMBOL for the
> BITS_PER_LONG == 64 case
>

[PATCH v10 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

v8 Small style fixup in the comment

v9 Usage of fls64 as per Joe Perches suggestion with adjustment
to int_sqrt changes from Peter Zijlstra for int_sqrt64 function.
Also replaced the macro magic with a simple ifdef in source file.

v10 Put defintion for int_sqrt64 for 64 bit machines into the
header file as per suggestion of Joe Perches

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 759 +
 include/linux/kernel.h |   9 +
 lib/int_sqrt.c |  31 +
 7 files changed, 847 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v10 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  9 +
 lib/int_sqrt.c | 31 +++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..e4a3dc64e650 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -460,6 +460,15 @@ extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
 
+#if BITS_PER_LONG < 64
+u32 int_sqrt64(u64 x);
+#else
+static inline u32 int_sqrt64(u64 x)
+{
+   return (u32)int_sqrt(x);
+}
+#endif
+
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
 extern int panic_timeout;
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..184da54677ec 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -7,6 +7,7 @@
 
 #include 
 #include 
+#include 
 
 /**
  * int_sqrt - rough approximation to sqrt
@@ -36,3 +37,33 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG < 64
+/**
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (fls64(x) & ~1ULL);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
+#endif
-- 
2.15.0

Re: [PATCH v7 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

Thanks for suggestions and comments. Respinned v9 with them.

On 19 December 2017 at 17:14, Joe Perches  wrote:
> On Tue, 2017-12-19 at 17:12 +0100, Peter Zijlstra wrote:
>> On Tue, Dec 19, 2017 at 08:09:53AM -0800, Joe Perches wrote:
>> >
>> > I suspect you need to define this as:
>> >
>> > #define int_sqrt64(x) ((u32)int_sqrt(x))
>>
>> What's wrong with using C ?
>
> Duh.  Nothing at all.
>
>> static inline u32 int_sqrt64(u64 x)
>> {
>>   return int_sqrt(x);
>> }

[PATCH v9 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

v8 Small style fixup in the comment

v9 Usage of fls64 as per Joe Perches suggestion with adjustment
to int_sqrt changes from Peter Zijlstra for int_sqrt64 function.
Also replaced the macro magic with a simple ifdef in source file.

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 759 +
 include/linux/kernel.h |   1 +
 lib/int_sqrt.c |  37 +
 7 files changed, 845 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v9 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  1 +
 lib/int_sqrt.c | 37 +
 2 files changed, 38 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..5c6949e9e309 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+u32 int_sqrt64(u64 x);
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..30acab761dc1 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -7,6 +7,7 @@
 
 #include 
 #include 
+#include 
 
 /**
  * int_sqrt - rough approximation to sqrt
@@ -36,3 +37,39 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG < 64
+/**
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (fls64(x) & ~1ULL);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
+#else
+u32 int_sqrt64(u64 x)
+{
+   return int_sqrt(x);
+}
+EXPORT_SYMBOL(int_sqrt64);
+#endif
-- 
2.15.0

[PATCH v8 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  6 ++
 lib/int_sqrt.c | 31 +++
 2 files changed, 37 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..f3bebd3a9213 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,12 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+#if BITS_PER_LONG != 64
+#define int_sqrt64 _int_sqrt64
+u32 _int_sqrt64(u64 x);
+#else
+#define int_sqrt64 int_sqrt
+#endif
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..41338f1194b2 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,34 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG != 64
+/**
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 _int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(_int_sqrt64);
+#endif
+
-- 
2.15.0

[PATCH v8 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

v8 Small style fixup in the comment

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 759 +
 include/linux/kernel.h |   6 +
 lib/int_sqrt.c |  31 +
 7 files changed, 844 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v7 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  6 ++
 lib/int_sqrt.c | 32 
 2 files changed, 38 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..f3bebd3a9213 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,12 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+#if BITS_PER_LONG != 64
+#define int_sqrt64 _int_sqrt64
+u32 _int_sqrt64(u64 x);
+#else
+#define int_sqrt64 int_sqrt
+#endif
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..84c77c934ec3 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,35 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+#if BITS_PER_LONG != 64
+/**
+ *
+ * int_sqrt64 - strongly typed int_sqrt function when minimum 64 bit input
+ * is expected.
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u32 _int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+
+   return y;
+}
+EXPORT_SYMBOL(_int_sqrt64);
+#endif
+
-- 
2.15.0

[PATCH v7 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

v7 Rework usage of int_sqrt according to Joe Perches and fixing
return size to u32 as per Peter Zijlstra suggestion, as sqrt of
64bit number cannot be have more than 32bits.

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 759 +
 include/linux/kernel.h |   6 +
 lib/int_sqrt.c |  32 +
 7 files changed, 845 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

Re: [PATCH v6 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 19 December 2017 at 14:25, Peter Zijlstra  wrote:
> On Tue, Dec 19, 2017 at 10:31:18AM +0100, Crt Mori wrote:
>> IIO kernel does not have the recent version in, so thanks for heads
>
> IIO?
>

Industrial Input / Output Subsytem tree

>> up. It does not change much for my function.
>
> The comment says:
>
>  * Computes: floor(sqrt(x))
>
> floor(sqrt(2^64-1)) == 2^32-1
>

OK, so return value should be u32.

Re: [PATCH v6 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 18 December 2017 at 17:24, Joe Perches  wrote:
> On Mon, 2017-12-18 at 16:05 +0100, Crt Mori wrote:
>> There is no option to perform 64bit integer sqrt on 32bit platform.
>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>> be performed on 32bit platforms. Although int_sqrt() is a rough
>> approximation, the same algorithm is used in int_sqrt64() as good
>> enough on 32bit platform.
> []
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> []
>> @@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
>>  extern int func_ptr_is_kernel_text(void *ptr);
>>
>>  unsigned long int_sqrt(unsigned long);
>> +u64 int_sqrt64(u64 x);
>>
>>  extern void bust_spinlocks(int yes);
>>  extern int oops_in_progress; /* If set, an oops, panic(), BUG() or 
>> die() is in progress */
>> diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
> []
>> @@ -36,3 +36,29 @@ unsigned long int_sqrt(unsigned long x)
>>   return y;
>>  }
>>  EXPORT_SYMBOL(int_sqrt);
>> +
>> +/**
>> + * int_sqrt64 - strongly typed int_sqrt function
>> + * @x: 64bit integer of which to calculate the sqrt
>> + */
>> +u64 int_sqrt64(u64 x)
>> +{
>> + u64 b, m, y = 0;
>> +
>> + if (x <= 1)
>> + return x;
>> +
>> + m = 1ULL << (64 - 2);
>> + while (m != 0) {
>> + b = y + m;
>> + y >>= 1;
>> +
>> + if (x >= b) {
>> + x -= b;
>> + y += m;
>> + }
>> + m >>= 2;
>> + }
>> + return y;
>> +}
>> +EXPORT_SYMBOL(int_sqrt64);
>
> int_sqrt is now optimized with __fls
>
> Because u64 is unsigned long on 64 bit arches, this
> could probably be optimized for 64 bit distributions
> with something like:
>
> #if BITS_PER_LONG != 64
> #define int_sqrt64 _int_sqrt64
> u64 _int_sqrt64(u64 x);
> #else
> #define int_sqrt64(x) int_sqrt
> #endif
>
> ...
>
> #if BITS_PER_LONG != 64
> u64 _int_sqrt64(u64 x)
> { etc.. }
> #endif
>

Thanks for the tip. I will respin with this change so that 64 bit
arches use the optimized int_sqrt from above.

Re: [PATCH v6 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

On 18 December 2017 at 17:44, Peter Zijlstra  wrote:
> On Mon, Dec 18, 2017 at 04:05:44PM +0100, Crt Mori wrote:
>> There is no option to perform 64bit integer sqrt on 32bit platform.
>> Added stronger typed int_sqrt64 enables the 64bit calculations to
>> be performed on 32bit platforms. Although int_sqrt() is a rough
>> approximation, the same algorithm is used in int_sqrt64() as good
>> enough on 32bit platform.
>
> You clearly haven't read a recent version of the file you're patching.
> Please take a moment to do so now.
>

IIO kernel does not have the recent version in, so thanks for heads
up. It does not change much for my function.

>> +/**
>> + * int_sqrt64 - strongly typed int_sqrt function
>> + * @x: 64bit integer of which to calculate the sqrt
>> + */
>> +u64 int_sqrt64(u64 x)
>
> Please explain how the result of sqrt(u64) can be larger than u32.
>

My hand calculator tells me it could be.

× = FFFE0001 which still has some margin which
will end up above the 32 bit number. Further more the __fls
optimization automatically casts the inputs to unsigned long (32 bit
on 32 bit machines), so that also makes it out of option.

> Also, I expect that this fact could be exploited to optimize this for
> 32bit archs if one were so inclined.
>
>> +{
>> + u64 b, m, y = 0;
>> +
>> + if (x <= 1)
>> + return x;
>> +
>> + m = 1ULL << (64 - 2);
>> + while (m != 0) {
>> + b = y + m;
>> + y >>= 1;
>> +
>> + if (x >= b) {
>> + x -= b;
>> + y += m;
>> + }
>> + m >>= 2;
>> + }
>> + return y;
>> +}
>
> so yeah, no, please try again after reading the current file.

[PATCH v6 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  1 +
 lib/int_sqrt.c | 26 ++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..09f53063f2c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+u64 int_sqrt64(u64 x);
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..de5836448ea3 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,29 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+/**
+ * int_sqrt64 - strongly typed int_sqrt function
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u64 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
-- 
2.15.0

[PATCH v6 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

v6 Adding usage of UNIVERSAL_DEV_PM_OPS macro for PM function
mapping.

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 759 +
 include/linux/kernel.h |   1 +
 lib/int_sqrt.c |  26 +
 7 files changed, 834 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v5 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  1 +
 lib/int_sqrt.c | 26 ++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..09f53063f2c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+u64 int_sqrt64(u64 x);
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..de5836448ea3 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,29 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+/**
+ * int_sqrt64 - strongly typed int_sqrt function
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u64 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
-- 
2.15.0

[PATCH v5 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).

v5 Re-ordered patches so that dt-bindings is before driver patch
as per Andreas Farber remark. Using same function for Power
Management during runtime and suspend, as the runtime state
already puts device to lowest possible state (without power off)
and that is why special state for suspend is not needed at the
current moment. Added few more checks of current PM state and
move marking regmap dirty to suspend instead of resume.

Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  dt-bindings: iio: temperature: add MLX90632 device bindings
  iio: temperature: Adding support for MLX90632

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 761 +
 include/linux/kernel.h |   1 +
 lib/int_sqrt.c |  26 +
 7 files changed, 836 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v4 1/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  1 +
 lib/int_sqrt.c | 26 ++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..09f53063f2c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+u64 int_sqrt64(u64 x);
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..de5836448ea3 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,29 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+/**
+ * int_sqrt64 - strongly typed int_sqrt function
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u64 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
-- 
2.15.0

[PATCH v4 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

v4 CONFIG_PM replaced by __maybe_unused, some cleanup in suspend,
EEPROM_VERSION identifier is grouped together in one single number
instead of assembled on fly. Replaced license on top with SPDX.
Most importantly - reordered patches so that lib is added before
the driver itself (which uses it).


Crt Mori (3):
  lib: Add strongly typed 64bit int_sqrt
  iio: temperature: Adding support for MLX90632
  dt-bindings: iio: temperature: add MLX90632 device bindings

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 774 +
 include/linux/kernel.h |   1 +
 lib/int_sqrt.c |  26 +
 7 files changed, 849 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v3 3/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  1 +
 lib/int_sqrt.c | 26 ++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..09f53063f2c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+u64 int_sqrt64(u64 x);
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..de5836448ea3 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,29 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+/**
+ * int_sqrt64 - strongly typed int_sqrt function
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u64 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
-- 
2.15.0

[PATCH v3 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

v3 comments were added to some parts of mlx90632.c and TENTO defines
replaced by values. Fixes were done to emissivity read/write and
changed to INT_PLUS_MICRO instead. Empty line in int_sqrt.c was
removed as it generated a warning when applying.

Crt Mori (3):
  iio: temperature: Adding support for MLX90632
  dt-bindings: iio: temperature: add MLX90632 device bindings
  lib: Add strongly typed 64bit int_sqrt

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 787 +
 include/linux/kernel.h |   1 +
 lib/int_sqrt.c |  26 +
 7 files changed, 862 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

[PATCH v2 3/3] lib: Add strongly typed 64bit int_sqrt

[Crt Mori]

There is no option to perform 64bit integer sqrt on 32bit platform.
Added stronger typed int_sqrt64 enables the 64bit calculations to
be performed on 32bit platforms. Although int_sqrt() is a rough
approximation, the same algorithm is used in int_sqrt64() as good
enough on 32bit platform.

Signed-off-by: Crt Mori 
---
 include/linux/kernel.h |  1 +
 lib/int_sqrt.c | 27 +++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..09f53063f2c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -459,6 +459,7 @@ extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
 unsigned long int_sqrt(unsigned long);
+u64 int_sqrt64(u64 x);
 
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;   /* If set, an oops, panic(), BUG() or 
die() is in progress */
diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
index 1ef4cc344977..3a106ffb1069 100644
--- a/lib/int_sqrt.c
+++ b/lib/int_sqrt.c
@@ -36,3 +36,30 @@ unsigned long int_sqrt(unsigned long x)
return y;
 }
 EXPORT_SYMBOL(int_sqrt);
+
+/**
+ * int_sqrt64 - strongly typed int_sqrt function
+ * @x: 64bit integer of which to calculate the sqrt
+ */
+u64 int_sqrt64(u64 x)
+{
+   u64 b, m, y = 0;
+
+   if (x <= 1)
+   return x;
+
+   m = 1ULL << (64 - 2);
+   while (m != 0) {
+   b = y + m;
+   y >>= 1;
+
+   if (x >= b) {
+   x -= b;
+   y += m;
+   }
+   m >>= 2;
+   }
+   return y;
+}
+EXPORT_SYMBOL(int_sqrt64);
+
-- 
2.15.0

[PATCH v2 0/3] iio: temperataure: MLX90632

[Crt Mori]

Hi everybody,

With the sensor now available to broad public, it is time to also
share the driver with the community. MLX90632 is just 3x3mm in size,
but with factory calibration offers instant usage in every project.

Driver currently provides basic functionality, but I might add
some more fancy features after a while.

v2 includes comments from Rob Herring (dt-bindings) and Jonathan
Cameron (iio), as well as proposal to split out int_sqrt64 function
into the linux/kernel.h

Crt Mori (3):
  iio: temperature: Adding support for MLX90632
  dt-bindings: iio: temperature: add MLX90632 device bindings
  lib: Add strongly typed 64bit int_sqrt

 .../bindings/iio/temperature/mlx90632.txt  |  28 +
 MAINTAINERS|   7 +
 drivers/iio/temperature/Kconfig|  12 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/mlx90632.c | 793 +
 include/linux/kernel.h |   1 +
 lib/int_sqrt.c |  27 +
 7 files changed, 869 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/temperature/mlx90632.txt
 create mode 100644 drivers/iio/temperature/mlx90632.c

-- 
2.15.0

Re: [PATCH v2 02/17] iio: mlx96014: Add OF device ID table

[Crt Mori]

Hi,
Thanks for the patch. If the assumption will not be there than this
fix is needed. I tested it on BeagleBoneBlack and it does not effect
anything.

Tested-by: Crt Mori 
Acked-by: Crt Mori 

Best regards,
Crt


On 15 March 2017 at 05:44, Javier Martinez Canillas
 wrote:
> The driver doesn't have a struct of_device_id table but supported devices
> are registered via Device Trees. This is working on the assumption that a
> I2C device registered via OF will always match a legacy I2C device ID and
> that the MODALIAS reported will always be of the form i2c:.
>
> But this could change in the future so the correct approach is to have an
> OF device ID table if the devices are registered via OF.
>
> Signed-off-by: Javier Martinez Canillas 
> ---
>
>  drivers/iio/temperature/mlx90614.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/iio/temperature/mlx90614.c 
> b/drivers/iio/temperature/mlx90614.c
> index 4b645fc672aa..2077eef4095c 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -585,6 +585,12 @@ static const struct i2c_device_id mlx90614_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, mlx90614_id);
>
> +static const struct of_device_id mlx90614_of_match[] = {
> +   { .compatible = "melexis,mlx90614" },
> +   { }
> +};
> +MODULE_DEVICE_TABLE(of, mlx90614_of_match);
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int mlx90614_pm_suspend(struct device *dev)
>  {
> @@ -644,6 +650,7 @@ static const struct dev_pm_ops mlx90614_pm_ops = {
>  static struct i2c_driver mlx90614_driver = {
> .driver = {
> .name   = "mlx90614",
> +   .of_match_table = mlx90614_of_match,
> .pm = &mlx90614_pm_ops,
> },
> .probe = mlx90614_probe,
> --
> 2.9.3
>

Re: [RFC v2] regmap: Add regmap_pipe_read API

[Crt Mori]

Adding linux-iio list to this - especially since there are IIO
drivers which work because of some special case...

Otherwise nice series.
Crt
On 20 July 2016 at 17:08, Crestez Dan Leonard  wrote:
> The regmap API usually assumes that bulk read operations will read a
> range of registers but some I2C/SPI devices have certain registers for
> which a such a read operation will return data from an internal FIFO or
> some other kind of buffer. Add an explicit API to support bulk read with
> "output pipe" rather than range semantics.
>
> Some linux drivers use regmap_bulk_read or regmap_raw_read for such
> registers, for example mpu6050 or bmi150 from IIO. This only happens to
> work because when caching is disabled a single short regmap read op will
> map to a single bus read op and behave as intended. This breaks if
> caching is enabled and reg+1 happens to be a cacheable register.
>
> Without regmap support refactoring a driver to enable regmap caching
> requires separate i2c and spi paths. This is exactly what regmap is
> supposed to help avoid.
>
> Suggested-by: Jonathan Cameron 
> Signed-off-by: Crestez Dan Leonard 
>
> ---
> Changes since V1:
> * Improve comments. I think "pipe" is the correct word here.
> * Rely on void* pointer arithmetic instead of casting to u8*
>
> The SPMI implementations of regmap_bus have read functions which
> auto-increment internally and this will not work correctly with
> regmap_pipe_read. I think the correct fix would be for regmap-spmi to
> only implement reg_read. This can be considered an acceptable limitation
> because in order to run into this somebody would have to write new code
> to call this new API with an SPMI device.
>
> I attempted to also support this when the underlying bus only supports
> reg_read (for example an adapter with only I2C_FUNC_SMBUS_BYTE_DATA) but
> it's not clear how to do this on top of _regmap_read. Apparently this
> returns the value as an "unsigned int" which needs to be formatted
> according to val_bytes. Unfortunately map->format.format_val is not
> always available. Presumably the code dealing with this from regmap_bulk_read
> should be refactored into a separate function?
>
> It's not clear why regmap doesn't just always initialize format_val.
>
>  drivers/base/regmap/regmap.c | 64 
> 
>  include/linux/regmap.h   |  9 +++
>  2 files changed, 73 insertions(+)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index df2d2ef..55c3090 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -2408,6 +2408,70 @@ int regmap_raw_read(struct regmap *map, unsigned int 
> reg, void *val,
>  EXPORT_SYMBOL_GPL(regmap_raw_read);
>
>  /**
> + * regmap_pipe_read(): Read data from a register with pipe semantics
> + *
> + * @map: Register map to read from
> + * @reg: Register to read from
> + * @val: Pointer to data buffer
> + * @val_len: Length of output buffer in bytes.
> + *
> + * The regmap API usually assumes that bulk bus read operations will read a
> + * range of registers. Some devices have certain registers for which a read
> + * operation will read a block of data from an internal buffer.
> + *
> + * The target register must be volatile but registers after it can be
> + * completely unrelated cacheable registers.
> + *
> + * This will attempt multiple reads as required to read val_len bytes.
> + *
> + * This function is not supported over SPMI.
> + *
> + * A value of zero will be returned on success, a negative errno will be
> + * returned in error cases.
> + */
> +int regmap_pipe_read(struct regmap *map, unsigned int reg,
> +void *val, size_t val_len)
> +{
> +   size_t read_len;
> +   int ret;
> +
> +   if (!map->bus)
> +   return -EINVAL;
> +   if (!map->bus->read)
> +   return -ENOTSUPP;
> +   if (val_len % map->format.val_bytes)
> +   return -EINVAL;
> +   if (!IS_ALIGNED(reg, map->reg_stride))
> +   return -EINVAL;
> +   if (val_len == 0)
> +   return -EINVAL;
> +
> +   map->lock(map->lock_arg);
> +
> +   if (!regmap_volatile(map, reg)) {
> +   ret = -EINVAL;
> +   goto out_unlock;
> +   }
> +
> +   while (val_len) {
> +   if (map->max_raw_read && map->max_raw_read < val_len)
> +   read_len = map->max_raw_read;
> +   else
> +   read_len = val_len;
> +   ret = _regmap_raw_read(map, reg, val, read_len);
> +   if (ret)
> +   goto out_unlock;
> +   val += read_len;
> +   val_len -= read_len;
> +   }
> +
> +out_unlock:
> +   map->unlock(map->lock_arg);
> +   return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_pipe_read);
> +
> +/**
>   * regmap_field_read(): Read a value to a single register field
>   *
>   * @field: Register field to read from
> diff --git a/

Re: [PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Crt Mori]

On 20 July 2016 at 14:37, Quentin Schulz
 wrote:
> On 18/07/2016 15:18, Jonathan Cameron wrote:
> [...]
>>> +
>>> +if (!wait_for_completion_timeout(&info->completion,
>>> + msecs_to_jiffies(100))) {
>>> +ret = -ETIMEDOUT;
>>> +goto out;
>>> +}
>>> +
>>> +if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>>> +*val = info->temp_data * 133 - 257000;
>> Why report as processed?  I'd just report them as raw with the scale
>> and offset provided.  It's not a big thing, but if we can leave it so
>> that the conversion only occurs when desired, why not?
>>
>> For in kernel users, this all happen 'automagically' anyway ;)
>>
>
> Mmmmh, in the code above we apply the scale on the raw value and then
> the offset. While in iio_convert_raw_to_processed
> (http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the
> offset is applied before the scale.
>
> The way would be to factorize the computation by scale:
> Now: *val = raw * scale + offset
> Then: *val = (raw + offset/scale) * scale
>
> But the offset is an integer and offset/scale is therefore rounded.
> Currently, we have the following values:
> sun4i: -257000/133 = -1932.3308270676691
> sun5i: -144700/100 = -1447
> sun6i: -271000/167 = -1622.754491017964
>
> Do we accept such rounding?
How much does this rounding bring as mistake on end result and is this
acceptable for you? You can always multiply it by 1000 or more to get
the precision you want out.
>
> If not, we either stay with the processed value in read_raw or patch
> inkern to add an offset to apply after having applied the scale to the
> raw value (val2 from iio_channel_read is yet unused with
> IIO_CHAN_INFO_OFFSET for example, we could use that to specify an
> offset2 to apply after the switch(scale_type)-case).
>
> [...]
> Quentin
>

Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

[Crt Mori]

On 31 March 2016 at 19:20, Crestez Dan Leonard
 wrote:
> Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.
Then we are missing DEPENDS in Kconfig...

>
> The device can be configured to do internal periodic sampling but does
> not appear to offer some sort of interrupt on data ready. It only offers
> interrupts on values out of a specific range.
>
> Signed-off-by: Crestez Dan Leonard 
> ---
>  drivers/iio/adc/ti-adc081c.c | 99 
> +---
>  1 file changed, 83 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> index 9b2f26f..040e2aa 100644
> --- a/drivers/iio/adc/ti-adc081c.c
> +++ b/drivers/iio/adc/ti-adc081c.c
> @@ -24,6 +24,9 @@
>  #include 
>
>  #include 
> +#include 
> +#include 
> +#include 
>  #include 
>
>  struct adc081c {
> @@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
> return -EINVAL;
>  }
>
> -static const struct iio_chan_spec adc081c_channel = {
> -   .type = IIO_VOLTAGE,
> -   .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> -   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -};
> +static irqreturn_t adc081c_trigger_handler(int irq, void *p)
> +{
> +   struct iio_poll_func *pf = p;
> +   struct iio_dev *indio_dev = pf->indio_dev;
> +   struct adc081c *data = iio_priv(indio_dev);
> +   s64 ts;
> +   u16 buf[8];
> +   int ret;
> +
> +   /* Otherwise iio_push_to_buffers will corrupt the stack. */
> +   if (indio_dev->scan_bytes > sizeof(buf)) {
> +   dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
> +   indio_dev->scan_bytes, (int)sizeof(buf));
> +   goto out;
> +   }
> +
> +   ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
> +   ts = iio_get_time_ns();
> +   if (ret < 0)
> +   goto out;
> +   buf[0] = ret;
> +   iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
> +out:
> +   iio_trigger_notify_done(indio_dev->trig);
> +   return IRQ_HANDLED;
> +}
>
>  static const struct iio_info adc081c_info = {
> .read_raw = adc081c_read_raw,
> .driver_module = THIS_MODULE,
>  };
>
> +struct adcxx1c_model {
> +   int bits;
> +   const struct iio_chan_spec* channels;
> +};
> +
> +#define ADCxx1C_CHAN(_bits) {  \
> +   .type = IIO_VOLTAGE,\
> +   .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
> +   .scan_type = {  \
> +   .sign = 'u',\
> +   .realbits = (_bits),\
> +   .storagebits = 16,  \
> +   .shift = 12 - (_bits),  \
> +   .endianness = IIO_CPU,  \
> +   },  \
> +}
> +
> +#define DEFINE_ADCxx1C_MODEL(_name, _bits) \
> +   static const struct iio_chan_spec _name ## _channels[] = {  \
> +   ADCxx1C_CHAN((_bits)),  \
> +   IIO_CHAN_SOFT_TIMESTAMP(1), \
> +   };  \
> +   static const struct adcxx1c_model _name ## _model = {   \
> +   .bits = (_bits),\
> +   .channels = _name ## _channels, \
> +   }
> +
> +DEFINE_ADCxx1C_MODEL(adc081c,  8);
> +DEFINE_ADCxx1C_MODEL(adc101c, 10);
> +DEFINE_ADCxx1C_MODEL(adc121c, 12);
> +
> +struct adcxx1c_info {
> +   int bits;
> +   const struct adc081c_channels* channels;
> +};
> +
>  static int adc081c_probe(struct i2c_client *client,
>  const struct i2c_device_id *id)
>  {
> struct iio_dev *iio;
> struct adc081c *adc;
> +   struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
> int err;
>
> -   if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data 
> != 12)
> -   return -EINVAL;
> -
> if (!i2c_check_functionality(client->adapter, 
> I2C_FUNC_SMBUS_WORD_DATA))
> return -EOPNOTSUPP;
>
> @@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
>
> adc = iio_priv(iio);
> adc->i2c = client;
> -   adc->bits = id->driver_data;
> +   adc->bits = model->bits;
>
> adc->ref = devm_regulator_get(&client->dev, "vref");
> if (IS_ERR(adc->ref))
> @@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
> iio->modes = INDIO_DIRECT_MODE;
> iio->info = &adc081c_info;
>
> -   iio->channels = &adc081c

Re: extending /sys/.../iio:deviceX/in_accelX_power_mode

[Crt Mori]

On 1 March 2016 at 10:47, Martin Kepplinger  wrote:
> Am 2016-03-01 um 10:38 schrieb Daniel Baluta:
>> On Tue, Mar 1, 2016 at 10:59 AM, Martin Kepplinger  wrote:
>>> Would it be ok, if adding in_accelX_power_mode to a driver, to extend it
>>> so that in_accel_power_mode_available offers:
>>>
>>> low_noise low_power low_power_low_noise normal
>>>
>>> if there's a default "normal" mode, plus options to increase or decrease
>>> oversampling / power consumption for my device?
>>>
>>> Specifically I'm unsure about "low_power_low_noise" being enough
>>> user-friendly. The chip I work with just happens to offer these 4 modes.
>>> Would you leave out "low_power_low_noise" and go with
>>>
>>> low_noise low_power normal
>>>
>>> or is it not even desired to add "normal" to the list?
>>>
>>> Although strictly not necessary, I would add any new addition to the
>>> Documentation as well.
>>
>> The problem with this is that is not uniform across sensors. What
>> chip are you looking at?
>>
>> For example INV6500 has:
>> * sleep mode
>> * standby mode
>> * etc.
>>
>> Daniel.
>>
>
> I suspect these modes are something else. I'm looking at the mma8452
> driver, and it also has "active" "standby" and "sleep" modes, but I'm
> talking about different *power* (oversampling) configurations in
> "active" mode, which is what said sysfs file is about.
>
> But yes, it should be potenially uniform across sensors, which is why I
> would probably only add "normal" to the list. At least I can imagine
> that many devices have an oversampling mode called "normal".

If that is oversampling option then why don't you just use that as a
setup? Power mode does not sound like oversampling to me... Maybe you
should use a sampling_frequency parameter instead?

>
> A simple user interface is important so right now I think the best is to
> leave it as it is, and not to add complexity and every possible option
> for the user.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/9] iio: imu: inv_mpu6050: Fix Yoda conditions

[Crt Mori]

This is quite trivial.

Acked-by: Crt Mori 

On 18 February 2016 at 16:53, Daniel Baluta  wrote:
> This fixes the following checkpatch warning:
> * WARNING: Comparisons should place the constant
>   on the right side of the test
>
> Signed-off-by: Daniel Baluta 
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 6 +++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 84e014c..c550ebb 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -84,7 +84,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, 
> bool en, u32 mask)
>  * clock source be switched to gyro. Otherwise, it must be set to
>  * internal clock
>  */
> -   if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
> +   if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
> result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
> if (result)
> return result;
> @@ -92,7 +92,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, 
> bool en, u32 mask)
> mgmt_1 &= ~INV_MPU6050_BIT_CLK_MASK;
> }
>
> -   if ((INV_MPU6050_BIT_PWR_GYRO_STBY == mask) && (!en)) {
> +   if ((mask == INV_MPU6050_BIT_PWR_GYRO_STBY) && (!en)) {
> /*
>  * turning off gyro requires switch to internal clock first.
>  * Then turn off gyro engine
> @@ -117,7 +117,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state 
> *st, bool en, u32 mask)
> if (en) {
> /* Wait for output stabilize */
> msleep(INV_MPU6050_TEMP_UP_TIME);
> -   if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
> +   if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
> /* switch internal clock to PLL */
> mgmt_1 |= INV_CLK_PLL;
> result = regmap_write(st->map,
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 1fc5fd9..441080b 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -168,7 +168,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>
> result = kfifo_out(&st->timestamps, ×tamp, 1);
> /* when there is no timestamp, put timestamp as 0 */
> -   if (0 == result)
> +   if (result == 0)
> timestamp = 0;
>
> result = iio_push_to_buffers_with_timestamp(indio_dev, data,
> --
> 2.5.0
>

Re: [PATCH v2 0/8] i2c mux cleanup and locking update

[Crt Mori]

Hi Wolfram and Peter,
I will give my opinion about the path chosen although it should be
taken lightly.

I can see that hardware guys missed the software guys again on the
development path, but since this happens more often than not, I would
say it seems OK to have support for this as long as it does not make
more complex (longer) standard i2c transfers. I would support to have
additional mutex before mux as that will make less chance that someone
forgets to lock mutex before mux and proposed solution seems valid.

Regards,
Crt

On 5 January 2016 at 19:48, Wolfram Sang  wrote:
> Peter,
>
>> PS. needs a bunch of testing, I do not have access to all the involved hw
>
> First of all, thanks for diving into this topic and the huge effort you
> apparently have put into it.
>
> It is obviously a quite intrusive series, so it needs careful review.
> TBH, I can't really tell when I have the bandwidth to do that, so I hope
> other people will step up. And yes, it needs serious testing.
>
> To all: Although I appreciate any review support, I'd think the first
> thing to be done should be a very high level review - is this series
> worth the huge update? Is the path chosen proper? Stuff like this. I'd
> appreciate Acks or Revs for that. Stuff like fixing checkpatch warnings
> and other minor stuff should come later.
>
> Thanks,
>
>Wolfram
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Issues with Lenovo Yoga 900 IIO devices (accelerometer, etc.)

[Crt Mori]

On 17 December 2015 at 19:10, Nish Aravamudan  wrote:
> On Thu, Dec 17, 2015 at 9:32 AM, Nish Aravamudan
>  wrote:
>> On Thu, Dec 17, 2015 at 12:41 AM, Crt Mori  wrote:
>>> On 17 December 2015 at 06:27, Nish Aravamudan  
>>> wrote:
>>>> On Wed, Dec 16, 2015 at 3:05 PM, Nish Aravamudan
>>>>  wrote:
>>>>> On Wed, Dec 16, 2015 at 2:55 PM, Crt Mori  wrote:
>>>>>>
>>>>>> On Dec 16, 2015 11:37 PM, "Nish Aravamudan" 
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Wed, Dec 16, 2015 at 2:22 PM, Crt Mori  wrote:
>>>>>>> > On 16 December 2015 at 22:41, Nish Aravamudan
>>>>>>> >  wrote:
>>>>>>> >> Hi Daniel,
>>>>>>> >>
>>>>>>> >> On Wed, Dec 16, 2015 at 1:43 AM, Daniel Baluta
>>>>>>> >>  wrote:
>>>>>>> >>> On Tue, Dec 15, 2015 at 9:19 PM, Nish Aravamudan
>>>>>>> >>>  wrote:
>>>>>>> >>>> So, I apologize in advance for this relatively vague report, but 
>>>>>>> >>>> I'm
>>>>>>> >>>> fairly sure
>>>>>>> >>>> the Yoga 900 has an accelerometer amongst other sensors (ambient
>>>>>>> >>>> light?)
>>>>>>> >>>> exported over IIO.
>>>>>>> >>>>
>>>>>>> >>>> But, these sensors seem to not be updating at all with a 4.4-rc5+
>>>>>>> >>>> kernel (a
>>>>>>> >>>> set of patches from https://lkml.org/lkml/2015/11/30/441 applied to
>>>>>>> >>>> Linus'
>>>>>>> >>>> tree).
>>>>>>> >>>>
>>>>>>> >>>> The odd part is at some point in messing with this, I'm fairly sure
>>>>>>> >>>> it did work!
>>>>>>> >>>> That is,
>>>>>>> >>>>
>>>>>>> >>>> `watch -n 0.1 cat '/sys/bus/iio/devices/iio:device'*/*raw*`
>>>>>>> >>>
>>>>>>> >>> Can you send us a sample of the output? Also, would be
>>>>>>> >>> good to identify the exact driver for accel.
>>>>>>> >>
>>>>>>> >> cat /sys/bus/iio/devices/iio:device*/*raw*
>>>>>>> >> 65478
>>>>>>> >> 7
>>>>>>> >> 1023
>>>>>>> >> 0
>>>>>>> >> 0
>>>>>>> >> 0
>>>>>>> >> 100
>>>>>>> >> -539062
>>>>>>> >> -742187
>>>>>>> >> 1292968
>>>>>>> >> 1592
>>>>>>> >> 64932
>>>>>>> >> 2
>>>>>>> >> 275
>>>>>>> >> 0 0 0 0
>>>>>>> >>
>>>>>>> >> Now, I should say that I distinctly remember at some point waving my
>>>>>>> >> laptop around and seeing these values change ... but now they seem to
>>>>>>> >> be "stuck". Maybe it's a hardware issue or something special that
>>>>>>> >> WIndows does to leverage the IIO sensors?
>>>>>>> >>
>>>>>>> >>> Perhaps: cat /sys/bus/iio/devices/iio:device'*/name
>>>>>>> >>
>>>>>>> >> $ cat /sys/bus/iio/devices/iio:device*/name
>>>>>>> >> accel_3d
>>>>>>> >
>>>>>>> > Can you list the directory of iio:device with this name (it is:
>>>>>>> > drivers/iio/accel/hid-sensor-accel-3d.c).
>>>>>>> > This is something you will be looking at for accel debugging, but it
>>>>>>> > seems more like
>>>>>>> > standard
>>>>>>>
>>>>>>> /sys/bus/iio/devices/iio:device0/name
>>>>>>> gyro_3d
>>>>>>> /sys/bus/iio/devices/iio:device1/name
>>>>>>> dev_rotation
>>>>>>> /sys/bus/iio/devices/iio:de

Re: Issues with Lenovo Yoga 900 IIO devices (accelerometer, etc.)

[Crt Mori]

On 17 December 2015 at 06:27, Nish Aravamudan  wrote:
> On Wed, Dec 16, 2015 at 3:05 PM, Nish Aravamudan
>  wrote:
>> On Wed, Dec 16, 2015 at 2:55 PM, Crt Mori  wrote:
>>>
>>> On Dec 16, 2015 11:37 PM, "Nish Aravamudan" 
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Dec 16, 2015 at 2:22 PM, Crt Mori  wrote:
>>>> > On 16 December 2015 at 22:41, Nish Aravamudan
>>>> >  wrote:
>>>> >> Hi Daniel,
>>>> >>
>>>> >> On Wed, Dec 16, 2015 at 1:43 AM, Daniel Baluta
>>>> >>  wrote:
>>>> >>> On Tue, Dec 15, 2015 at 9:19 PM, Nish Aravamudan
>>>> >>>  wrote:
>>>> >>>> So, I apologize in advance for this relatively vague report, but I'm
>>>> >>>> fairly sure
>>>> >>>> the Yoga 900 has an accelerometer amongst other sensors (ambient
>>>> >>>> light?)
>>>> >>>> exported over IIO.
>>>> >>>>
>>>> >>>> But, these sensors seem to not be updating at all with a 4.4-rc5+
>>>> >>>> kernel (a
>>>> >>>> set of patches from https://lkml.org/lkml/2015/11/30/441 applied to
>>>> >>>> Linus'
>>>> >>>> tree).
>>>> >>>>
>>>> >>>> The odd part is at some point in messing with this, I'm fairly sure
>>>> >>>> it did work!
>>>> >>>> That is,
>>>> >>>>
>>>> >>>> `watch -n 0.1 cat '/sys/bus/iio/devices/iio:device'*/*raw*`
>>>> >>>
>>>> >>> Can you send us a sample of the output? Also, would be
>>>> >>> good to identify the exact driver for accel.
>>>> >>
>>>> >> cat /sys/bus/iio/devices/iio:device*/*raw*
>>>> >> 65478
>>>> >> 7
>>>> >> 1023
>>>> >> 0
>>>> >> 0
>>>> >> 0
>>>> >> 100
>>>> >> -539062
>>>> >> -742187
>>>> >> 1292968
>>>> >> 1592
>>>> >> 64932
>>>> >> 2
>>>> >> 275
>>>> >> 0 0 0 0
>>>> >>
>>>> >> Now, I should say that I distinctly remember at some point waving my
>>>> >> laptop around and seeing these values change ... but now they seem to
>>>> >> be "stuck". Maybe it's a hardware issue or something special that
>>>> >> WIndows does to leverage the IIO sensors?
>>>> >>
>>>> >>> Perhaps: cat /sys/bus/iio/devices/iio:device'*/name
>>>> >>
>>>> >> $ cat /sys/bus/iio/devices/iio:device*/name
>>>> >> accel_3d
>>>> >
>>>> > Can you list the directory of iio:device with this name (it is:
>>>> > drivers/iio/accel/hid-sensor-accel-3d.c).
>>>> > This is something you will be looking at for accel debugging, but it
>>>> > seems more like
>>>> > standard
>>>>
>>>> /sys/bus/iio/devices/iio:device0/name
>>>> gyro_3d
>>>> /sys/bus/iio/devices/iio:device1/name
>>>> dev_rotation
>>>> /sys/bus/iio/devices/iio:device2/name
>>>> als
>>>> /sys/bus/iio/devices/iio:device3/name
>>>> magn_3d
>>>> /sys/bus/iio/devices/iio:device4/name
>>>> accel_3d
>>>> /sys/bus/iio/devices/iio:device5/name
>>>> incli_3d
>>>>
>>>> are all the IIO sensors, sorry about that!
>>>>
>>>
>>> I was more thinking what else is in iio:device4 directory, so that we can
>>> see if it was initialized OK. Can you also grep the dmesg for any errors?
>>
>> Well, I just noticed the device #s are not consistent boot-to-boot, so
>> this time it was device0. Trimming out the directories:
>>

This is correct/intended. You need to confirm "deviceX/name" each time.

>> /sys/bus/iio/devices/iio:device0/buffer
>> cat: /sys/bus/iio/devices/iio:device0/buffer: Is a directory
>> /sys/bus/iio/devices/iio:device0/dev
>> 250:0
>> /sys/bus/iio/devices/iio:device0/in_accel_hysteresis
>> cat: /sys/bus/iio/devices/iio:device0/in_accel_hysteresis: Invalid argument
>> /sys/bus/iio/devices/iio:device0/in_accel_off

Re: Issues with Lenovo Yoga 900 IIO devices (accelerometer, etc.)

[Crt Mori]

On 16 December 2015 at 22:41, Nish Aravamudan  wrote:
> Hi Daniel,
>
> On Wed, Dec 16, 2015 at 1:43 AM, Daniel Baluta  
> wrote:
>> On Tue, Dec 15, 2015 at 9:19 PM, Nish Aravamudan
>>  wrote:
>>> So, I apologize in advance for this relatively vague report, but I'm fairly 
>>> sure
>>> the Yoga 900 has an accelerometer amongst other sensors (ambient light?)
>>> exported over IIO.
>>>
>>> But, these sensors seem to not be updating at all with a 4.4-rc5+ kernel (a
>>> set of patches from https://lkml.org/lkml/2015/11/30/441 applied to Linus'
>>> tree).
>>>
>>> The odd part is at some point in messing with this, I'm fairly sure it did 
>>> work!
>>> That is,
>>>
>>> `watch -n 0.1 cat '/sys/bus/iio/devices/iio:device'*/*raw*`
>>
>> Can you send us a sample of the output? Also, would be
>> good to identify the exact driver for accel.
>
> cat /sys/bus/iio/devices/iio:device*/*raw*
> 65478
> 7
> 1023
> 0
> 0
> 0
> 100
> -539062
> -742187
> 1292968
> 1592
> 64932
> 2
> 275
> 0 0 0 0
>
> Now, I should say that I distinctly remember at some point waving my
> laptop around and seeing these values change ... but now they seem to
> be "stuck". Maybe it's a hardware issue or something special that
> WIndows does to leverage the IIO sensors?
>
>> Perhaps: cat /sys/bus/iio/devices/iio:device'*/name
>
> $ cat /sys/bus/iio/devices/iio:device*/name
> accel_3d

Can you list the directory of iio:device with this name (it is:
drivers/iio/accel/hid-sensor-accel-3d.c).
This is something you will be looking at for accel debugging, but it
seems more like
standard

> gyro_3d
> als
> magn_3d
> incli_3d
> dev_rotation
>
>
>>>
>>> showed updating values as I moved the laptop around.
>>>
>>> I've not done any accelerometer debugging before, so any suggestion on
>>> where to start would be greatly appreciated!
>>
>> Did you applied some patches and recompiled the kernel? Or when it did 
>> stopped
>> working?
>
> As far as I can tell, it only worked that one one time and hasn't
> since. Although your question does make me wonder *which* kernel I was
> on that I experienced the values changing. Let me go back to a stock
> 4.4-rc5 and see.

Did you compile the stock kernel? It might be that .dts file you are
using (or defconfig)
is not correct.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Issues with Lenovo Yoga 900 IIO devices (accelerometer, etc.)

[Crt Mori]

Hi,
So you were using 4.4-rc5 before and it worked, but after applying a
patch it stopped?
The patch you pasted seems to relate to dts files and declarations in
it - did anything there change?

Best regards,
Crt


On 15 December 2015 at 20:19, Nish Aravamudan  wrote:
> So, I apologize in advance for this relatively vague report, but I'm fairly 
> sure
> the Yoga 900 has an accelerometer amongst other sensors (ambient light?)
> exported over IIO.
>
> But, these sensors seem to not be updating at all with a 4.4-rc5+ kernel (a
> set of patches from https://lkml.org/lkml/2015/11/30/441 applied to Linus'
> tree).
>
> The odd part is at some point in messing with this, I'm fairly sure it did 
> work!
> That is,
>
> `watch -n 0.1 cat '/sys/bus/iio/devices/iio:device'*/*raw*`
>
> showed updating values as I moved the laptop around.
>
> I've not done any accelerometer debugging before, so any suggestion on
> where to start would be greatly appreciated!
>
> -Nish
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 5/9] iio: Documentation: Add IIO configfs documentation

[Crt Mori]

This has my acked-by, yet it is missing a commit message? Can we get that
and the removed From: as I think this is a new patch and not resend of Daniel's?
Also it would need your signed-off-by in that case

On 18 November 2015 at 15:38, Marc Titinger  wrote:
> From: Daniel Baluta 
>
> Signed-off-by: Daniel Baluta 
> Acked-by: Crt Mori 
> ---
>  Documentation/ABI/testing/configfs-iio | 21 
>  Documentation/iio/iio_configfs.txt | 93 
> ++
>  2 files changed, 114 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-iio
>  create mode 100644 Documentation/iio/iio_configfs.txt
>
> diff --git a/Documentation/ABI/testing/configfs-iio 
> b/Documentation/ABI/testing/configfs-iio
> new file mode 100644
> index 000..2483756
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-iio
> @@ -0,0 +1,21 @@
> +What:  /config/iio
> +Date:  October 2015
> +KernelVersion: 4.4
> +Contact:   linux-...@vger.kernel.org
> +Description:
> +   This represents Industrial IO configuration entry point
> +   directory. It contains sub-groups corresponding to IIO
> +   objects.
> +
> +What:  /config/iio/triggers
> +Date:  October 2015
> +KernelVersion: 4.4
> +Description:
> +   Industrial IO software triggers directory.
> +
> +What:  /config/iio/triggers/hrtimers
> +Date:  October 2015
> +KernelVersion: 4.4
> +Description:
> +   High resolution timers directory. Creating a directory here
> +   will result in creating a hrtimer trigger in the IIO 
> subsystem.
> diff --git a/Documentation/iio/iio_configfs.txt 
> b/Documentation/iio/iio_configfs.txt
> new file mode 100644
> index 000..f0add35
> --- /dev/null
> +++ b/Documentation/iio/iio_configfs.txt
> @@ -0,0 +1,93 @@
> +Industrial IIO configfs support
> +
> +1. Overview
> +
> +Configfs is a filesystem-based manager of kernel objects. IIO uses some
> +objects that could be easily configured using configfs (e.g.: devices,
> +triggers).
> +
> +See Documentation/filesystems/configfs/configfs.txt for more information
> +about how configfs works.
> +
> +2. Usage
> +
> +In order to use configfs support in IIO we need to select it at compile
> +time via CONFIG_IIO_CONFIGFS config option.
> +
> +Then, mount the configfs filesystem (usually under /config directory):
> +
> +$ mkdir /config
> +$ mount -t configfs none /config
> +
> +At this point, all default IIO groups will be created and can be accessed
> +under /config/iio. Next chapters will describe available IIO configuration
> +objects.
> +
> +3. Software triggers
> +
> +One of the IIO default configfs groups is the "triggers" group. It is
> +automagically accessible when the configfs is mounted and can be found
> +under /config/iio/triggers.
> +
> +IIO software triggers implementation offers support for creating multiple
> +trigger types. A new trigger type is usually implemented as a separate
> +kernel module following the interface in include/linux/iio/sw_trigger.h:
> +
> +/*
> + * drivers/iio/trigger/iio-trig-sample.c
> + * sample kernel module implementing a new trigger type
> + */
> +#include 
> +
> +
> +static struct iio_sw_trigger *iio_trig_sample_probe(const char *name)
> +{
> +   /*
> +* This allocates and registers an IIO trigger plus other
> +* trigger type specific initialization.
> +*/
> +}
> +
> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
> +{
> +   /*
> +* This undoes the actions in iio_trig_sample_probe
> +*/
> +}
> +
> +static const struct iio_sw_trigger_ops iio_trig_sample_ops = {
> +   .probe  = iio_trig_sample_probe,
> +   .remove = iio_trig_sample_remove,
> +};
> +
> +static struct iio_sw_trigger_type iio_trig_sample = {
> +   .name = "trig-sample",
> +   .owner = THIS_MODULE,
> +   .ops = &iio_trig_sample_ops,
> +};
> +
> +module_iio_sw_trigger_driver(iio_trig_sample);
> +
> +Each trigger type has its own directory under /config/iio/triggers. Loading
> +iio-trig-sample module will create 'trig-sample' trigger type directory
> +/config/iio/triggers/trig-sample.
> +
> +We support the following interrupt sources (trigger types):
> +   * hrtimer, uses high resolution timers as interrupt source
> +
> +3.1 Hrtimer triggers creation and destruction
> +
> +Loading iio-trig-hrtimer module will register hrtimer trigger types allowing
> +users to create hrtimer triggers under /config/iio/triggers/hrtimer.
&

Re: [PATCH v10 5/5] iio: Documentation: Add IIO configfs documentation

[Crt Mori]

(as my last comment was sufficiently explained and kinda forgot to add it)
Acked-by: Crt Mori 

On 23 October 2015 at 17:33, Daniel Baluta  wrote:
> Signed-off-by: Daniel Baluta 
> ---
>  Documentation/ABI/testing/configfs-iio | 21 
>  Documentation/iio/iio_configfs.txt | 93 
> ++
>  2 files changed, 114 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-iio
>  create mode 100644 Documentation/iio/iio_configfs.txt
>
> diff --git a/Documentation/ABI/testing/configfs-iio 
> b/Documentation/ABI/testing/configfs-iio
> new file mode 100644
> index 000..2483756
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-iio
> @@ -0,0 +1,21 @@
> +What:  /config/iio
> +Date:  October 2015
> +KernelVersion: 4.4
> +Contact:   linux-...@vger.kernel.org
> +Description:
> +   This represents Industrial IO configuration entry point
> +   directory. It contains sub-groups corresponding to IIO
> +   objects.
> +
> +What:  /config/iio/triggers
> +Date:  October 2015
> +KernelVersion: 4.4
> +Description:
> +   Industrial IO software triggers directory.
> +
> +What:  /config/iio/triggers/hrtimers
> +Date:  October 2015
> +KernelVersion: 4.4
> +Description:
> +   High resolution timers directory. Creating a directory here
> +   will result in creating a hrtimer trigger in the IIO 
> subsystem.
> diff --git a/Documentation/iio/iio_configfs.txt 
> b/Documentation/iio/iio_configfs.txt
> new file mode 100644
> index 000..f0add35
> --- /dev/null
> +++ b/Documentation/iio/iio_configfs.txt
> @@ -0,0 +1,93 @@
> +Industrial IIO configfs support
> +
> +1. Overview
> +
> +Configfs is a filesystem-based manager of kernel objects. IIO uses some
> +objects that could be easily configured using configfs (e.g.: devices,
> +triggers).
> +
> +See Documentation/filesystems/configfs/configfs.txt for more information
> +about how configfs works.
> +
> +2. Usage
> +
> +In order to use configfs support in IIO we need to select it at compile
> +time via CONFIG_IIO_CONFIGFS config option.
> +
> +Then, mount the configfs filesystem (usually under /config directory):
> +
> +$ mkdir /config
> +$ mount -t configfs none /config
> +
> +At this point, all default IIO groups will be created and can be accessed
> +under /config/iio. Next chapters will describe available IIO configuration
> +objects.
> +
> +3. Software triggers
> +
> +One of the IIO default configfs groups is the "triggers" group. It is
> +automagically accessible when the configfs is mounted and can be found
> +under /config/iio/triggers.
> +
> +IIO software triggers implementation offers support for creating multiple
> +trigger types. A new trigger type is usually implemented as a separate
> +kernel module following the interface in include/linux/iio/sw_trigger.h:
> +
> +/*
> + * drivers/iio/trigger/iio-trig-sample.c
> + * sample kernel module implementing a new trigger type
> + */
> +#include 
> +
> +
> +static struct iio_sw_trigger *iio_trig_sample_probe(const char *name)
> +{
> +   /*
> +* This allocates and registers an IIO trigger plus other
> +* trigger type specific initialization.
> +*/
> +}
> +
> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
> +{
> +   /*
> +* This undoes the actions in iio_trig_sample_probe
> +*/
> +}
> +
> +static const struct iio_sw_trigger_ops iio_trig_sample_ops = {
> +   .probe  = iio_trig_sample_probe,
> +   .remove = iio_trig_sample_remove,
> +};
> +
> +static struct iio_sw_trigger_type iio_trig_sample = {
> +   .name = "trig-sample",
> +   .owner = THIS_MODULE,
> +   .ops = &iio_trig_sample_ops,
> +};
> +
> +module_iio_sw_trigger_driver(iio_trig_sample);
> +
> +Each trigger type has its own directory under /config/iio/triggers. Loading
> +iio-trig-sample module will create 'trig-sample' trigger type directory
> +/config/iio/triggers/trig-sample.
> +
> +We support the following interrupt sources (trigger types):
> +   * hrtimer, uses high resolution timers as interrupt source
> +
> +3.1 Hrtimer triggers creation and destruction
> +
> +Loading iio-trig-hrtimer module will register hrtimer trigger types allowing
> +users to create hrtimer triggers under /config/iio/triggers/hrtimer.
> +
> +e.g:
> +
> +$ mkdir /config/triggers/hrtimer/instance1
> +$ rmdir /config/triggers/hrtimer/instance1
> +
> +Each trigger can have one or mor

Re: [PATCH v8 5/5] iio: Documentation: Add IIO configfs documentation

[Crt Mori]

On 20 October 2015 at 12:53, Daniel Baluta  wrote:
> Signed-off-by: Daniel Baluta 
> ---
>  Documentation/ABI/testing/configfs-iio | 21 
>  Documentation/iio/iio_configfs.txt | 93 
> ++
>  2 files changed, 114 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-iio
>  create mode 100644 Documentation/iio/iio_configfs.txt
>
> diff --git a/Documentation/ABI/testing/configfs-iio 
> b/Documentation/ABI/testing/configfs-iio
> new file mode 100644
> index 000..2483756
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-iio
> @@ -0,0 +1,21 @@
> +What:  /config/iio
> +Date:  October 2015
> +KernelVersion: 4.4
> +Contact:   linux-...@vger.kernel.org
> +Description:
> +   This represents Industrial IO configuration entry point
> +   directory. It contains sub-groups corresponding to IIO
> +   objects.
> +
> +What:  /config/iio/triggers
> +Date:  October 2015
> +KernelVersion: 4.4
> +Description:
> +   Industrial IO software triggers directory.
> +
> +What:  /config/iio/triggers/hrtimers
> +Date:  October 2015
> +KernelVersion: 4.4
> +Description:
> +   High resolution timers directory. Creating a directory here
> +   will result in creating a hrtimer trigger in the IIO 
> subsystem.
> diff --git a/Documentation/iio/iio_configfs.txt 
> b/Documentation/iio/iio_configfs.txt
> new file mode 100644
> index 000..f0add35
> --- /dev/null
> +++ b/Documentation/iio/iio_configfs.txt
> @@ -0,0 +1,93 @@
> +Industrial IIO configfs support
> +
> +1. Overview
> +
> +Configfs is a filesystem-based manager of kernel objects. IIO uses some
> +objects that could be easily configured using configfs (e.g.: devices,
> +triggers).
> +
> +See Documentation/filesystems/configfs/configfs.txt for more information
> +about how configfs works.
> +
> +2. Usage
> +
> +In order to use configfs support in IIO we need to select it at compile
> +time via CONFIG_IIO_CONFIGFS config option.
> +
> +Then, mount the configfs filesystem (usually under /config directory):
> +
> +$ mkdir /config
> +$ mount -t configfs none /config
> +
> +At this point, all default IIO groups will be created and can be accessed
> +under /config/iio. Next chapters will describe available IIO configuration
> +objects.
> +
> +3. Software triggers
> +
> +One of the IIO default configfs groups is the "triggers" group. It is
> +automagically accessible when the configfs is mounted and can be found
automagically is probably automatically?
> +under /config/iio/triggers.
> +
> +IIO software triggers implementation offers support for creating multiple
> +trigger types. A new trigger type is usually implemented as a separate
> +kernel module following the interface in include/linux/iio/sw_trigger.h:
> +
> +/*
> + * drivers/iio/trigger/iio-trig-sample.c
> + * sample kernel module implementing a new trigger type
> + */
> +#include 
> +
> +
> +static struct iio_sw_trigger *iio_trig_sample_probe(const char *name)
> +{
> +   /*
> +* This allocates and registers an IIO trigger plus other
> +* trigger type specific initialization.
> +*/
> +}
> +
> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
> +{
> +   /*
> +* This undoes the actions in iio_trig_sample_probe
> +*/
> +}
> +
> +static const struct iio_sw_trigger_ops iio_trig_sample_ops = {
> +   .probe  = iio_trig_sample_probe,
> +   .remove = iio_trig_sample_remove,
> +};
> +
> +static struct iio_sw_trigger_type iio_trig_sample = {
> +   .name = "trig-sample",
> +   .owner = THIS_MODULE,
> +   .ops = &iio_trig_sample_ops,
> +};
> +
> +module_iio_sw_trigger_driver(iio_trig_sample);
> +
> +Each trigger type has its own directory under /config/iio/triggers. Loading
> +iio-trig-sample module will create 'trig-sample' trigger type directory
> +/config/iio/triggers/trig-sample.
> +
> +We support the following interrupt sources (trigger types):
> +   * hrtimer, uses high resolution timers as interrupt source
> +
> +3.1 Hrtimer triggers creation and destruction
> +
> +Loading iio-trig-hrtimer module will register hrtimer trigger types allowing
> +users to create hrtimer triggers under /config/iio/triggers/hrtimer.
> +
> +e.g:
> +
> +$ mkdir /config/triggers/hrtimer/instance1
> +$ rmdir /config/triggers/hrtimer/instance1
> +
> +Each trigger can have one or more attributes specific to the trigger type.
> +
> +3.2 "hrtimer" trigger types attributes
> +
> +"hrtimer" trigger type doesn't have any configurable attribute from /config 
> dir.
> +It does introduce the sampling_frequency attribute to trigger directory.
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/l

Re: [PATCH v2] iio: mcp4xxx_dpot: Driver for Microchip digital potentiometers

[Crt Mori]

Hi,
Just follow advice and it will get better. Maybe a bit of naming
advice since that needs to be fixed all over.
Usual naming convention is chipname_function_name not
chipname_part_function_name as you have
now. There was a newly added  IIO_RESISTANCE channel in case you would
think of the change (more
depends on what you would rather expect as input/output of your driver
to user-space).

Few more comments inline.

On 21 September 2015 at 23:18, Peter Rosin  wrote:
>
> From: Peter Rosin 
>
> Add support for Microchip digital potentiometers and rheostats
> MCP4531, MCP4532, MCP4551, MCP4552,
> MCP4631, MCP4632, MCP4651, MCP4652
>
> These are either single (45xx) or dual (46xx) wipers with either
> 129 (4x3x) or 257 (4x5x) steps, and configured either as
> potentiometers (4xx1) or rheostats (4xx2).
>
> Signed-off-by: Peter Rosin 
> ---
>  MAINTAINERS|5 +
>  drivers/iio/Kconfig|1 +
>  drivers/iio/Makefile   |1 +
>  drivers/iio/pot/Kconfig|   20 
>  drivers/iio/pot/Makefile   |6 ++
>  drivers/iio/pot/mcp4xxx_dpot.c |  218 
> 
>  6 files changed, 251 insertions(+)
>  create mode 100644 drivers/iio/pot/Kconfig
>  create mode 100644 drivers/iio/pot/Makefile
>  create mode 100644 drivers/iio/pot/mcp4xxx_dpot.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b60e2b2369d2..2a34b75b3034 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6600,6 +6600,11 @@ W:   http://linuxtv.org
>  S: Maintained
>  F: drivers/media/radio/radio-maxiradio*
>
> +MCP4XXX MICROCHIP DIGITAL POTENTIOMETER DRIVER
> +M: Peter Rosin 
You are missing +L: linux-...@vger.kernel.org
> +S: Maintained
> +F: drivers/iio/pot/mcp4xxx_dpot.*
I would put a .c here since you do not have a .h anymore?
> +
>  MEDIA DRIVERS FOR RENESAS - VSP1
>  M: Laurent Pinchart 
>  L: linux-me...@vger.kernel.org
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 4011effe4c05..837aabad4b9a 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -73,6 +73,7 @@ source "drivers/iio/orientation/Kconfig"
>  if IIO_TRIGGER
> source "drivers/iio/trigger/Kconfig"
>  endif #IIO_TRIGGER
> +source "drivers/iio/pot/Kconfig"
>  source "drivers/iio/pressure/Kconfig"
>  source "drivers/iio/proximity/Kconfig"
>  source "drivers/iio/temperature/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 698afc2d17ce..df026289309f 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -23,6 +23,7 @@ obj-y += imu/
>  obj-y += light/
>  obj-y += magnetometer/
>  obj-y += orientation/
> +obj-y += pot/
>  obj-y += pressure/
>  obj-y += proximity/
>  obj-y += temperature/
> diff --git a/drivers/iio/pot/Kconfig b/drivers/iio/pot/Kconfig
> new file mode 100644
> index ..48ebc7ef6b6b
> --- /dev/null
> +++ b/drivers/iio/pot/Kconfig
> @@ -0,0 +1,20 @@
> +#
> +# Potentiometer drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Digital potentiometers"
> +
> +config MCP4XXX_DPOT
> +   tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
> +   depends on I2C
> +   help
> + Say yes here to build support for the Microship
> + MCP4531, MCP4532, MCP4551, MCP4552,
> + MCP4631, MCP4632, MCP4651, MCP4652
> + digital potentiomenter chips.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called mcp4xxx_dpot.
> +
> +endmenu
> diff --git a/drivers/iio/pot/Makefile b/drivers/iio/pot/Makefile
> new file mode 100644
> index ..763628ed649a
> --- /dev/null
> +++ b/drivers/iio/pot/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for industrial I/O potentiometer drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_MCP4XXX_DPOT) += mcp4xxx_dpot.o
> diff --git a/drivers/iio/pot/mcp4xxx_dpot.c b/drivers/iio/pot/mcp4xxx_dpot.c
> new file mode 100644
> index ..30a1fb8fa46d
> --- /dev/null
> +++ b/drivers/iio/pot/mcp4xxx_dpot.c
> @@ -0,0 +1,218 @@
> +/*
> + * Industrial I/O driver for Microchip digital potentiometers
> + * Copyright (c) 2015  Axentia Technologies AB
> + * Author: Peter Rosin 
> + *
> + * DEVID   #Wipers #Positions  Resistor Options 
> (kOhm)
> + * mcp4531 1   129 5, 10, 50, 100
> + * mcp4532 1   129 5, 10, 50, 100
> + * mcp4551 1   257 5, 10, 50, 100
> + * mcp4552 1   257 5, 10, 50, 100
> + * mcp4631 2   129 5, 10, 50, 100
> + * mcp4632 2   129 5, 10, 50, 100
> + * mcp4651 2   257 5, 10, 50, 100
> + * mcp4652 2   257 5, 10, 50, 100
> + *
> + * This program is fr

Re: [PATCH] Staging: iio: Move evgen interrupt generation to irq_work

[Crt Mori]

Hi, few small comments inline, although I have problems applying the patch
Crt

On 18 August 2015 at 14:55, Cristina Opriceana
 wrote:
> A more abstract way of handling interrupt generation in the dummy driver
> is represented by irq_work which substitutes the irq_chip, used to
> provide an IRQ line number. irq_work runs tasks from hardirq context,
> but in a NMI-safe environment and deals better with synchronization
> problems. An interrupt can be triggered by calling irq_work_queue()
> and the work will be performed by a pre-registered handler, which
> acts as a callback.
>
> Cc: Daniel Baluta 
> Suggested-by: Jonathan Cameron 
> Signed-off-by: Cristina Opriceana 
> ---
> This is the first draft for the irq_work replacement.
> As we want to move the iio dummy driver out of staging,
> we started to implement Jonathan's suggestions from here:
> 
>
>  drivers/staging/iio/iio_dummy_evgen.c | 171 
> +-
>  drivers/staging/iio/iio_dummy_evgen.h |  15 ++-
>  drivers/staging/iio/iio_simple_dummy.c|   6 +-
>  drivers/staging/iio/iio_simple_dummy.h|   8 +-
>  drivers/staging/iio/iio_simple_dummy_events.c |  71 ---
>  5 files changed, 105 insertions(+), 166 deletions(-)
>
> diff --git a/drivers/staging/iio/iio_dummy_evgen.c 
> b/drivers/staging/iio/iio_dummy_evgen.c
> index 6d38854..974ef8a 100644
> --- a/drivers/staging/iio/iio_dummy_evgen.c
> +++ b/drivers/staging/iio/iio_dummy_evgen.c
> @@ -25,132 +25,87 @@
>  #include 
>  #include 
>
> -/* Fiddly bit of faking and irq without hardware */
> -#define IIO_EVENTGEN_NO 10
> -/**
> - * struct iio_dummy_evgen - evgen state
> - * @chip: irq chip we are faking
> - * @base: base of irq range
> - * @enabled: mask of which irqs are enabled
> - * @inuse: mask of which irqs are connected
> - * @regs: irq regs we are faking
> - * @lock: protect the evgen state
> - */
> -struct iio_dummy_eventgen {
> -   struct irq_chip chip;
> -   int base;
> -   bool enabled[IIO_EVENTGEN_NO];
> -   bool inuse[IIO_EVENTGEN_NO];
> -   struct iio_dummy_regs regs[IIO_EVENTGEN_NO];
> -   struct mutex lock;
> -};
> +static LIST_HEAD(iio_dummy_event_list);
> +static DEFINE_MUTEX(iio_dummy_event_list_mutex);
>
> -/* We can only ever have one instance of this 'device' */
> -static struct iio_dummy_eventgen *iio_evgen;
> -static const char *iio_evgen_name = "iio_dummy_evgen";
> -
> -static void iio_dummy_event_irqmask(struct irq_data *d)
> +struct iio_dummy_event *get_event(int index)
>  {
> -   struct irq_chip *chip = irq_data_get_irq_chip(d);
> -   struct iio_dummy_eventgen *evgen =
> -   container_of(chip, struct iio_dummy_eventgen, chip);
> +   struct list_head *ptr, *tmp;
> +   struct iio_dummy_event *iio_event = NULL;
>
> -   evgen->enabled[d->irq - evgen->base] = false;
> -}
> -
> -static void iio_dummy_event_irqunmask(struct irq_data *d)
> -{
> -   struct irq_chip *chip = irq_data_get_irq_chip(d);
> -   struct iio_dummy_eventgen *evgen =
> -   container_of(chip, struct iio_dummy_eventgen, chip);
> -
> -   evgen->enabled[d->irq - evgen->base] = true;
> +   mutex_lock(&iio_dummy_event_list_mutex);
> +   list_for_each_safe(ptr, tmp, &iio_dummy_event_list) {
> +   iio_event = list_entry(ptr, struct iio_dummy_event, l);
> +   if (iio_event->regs.reg_id == index)
> +   break;
> +   }
> +   mutex_unlock(&iio_dummy_event_list_mutex);
> +   return iio_event;
>  }
>
> -static int iio_dummy_evgen_create(void)
> +int iio_dummy_evgen_create(struct iio_dev *indio_dev, int index)
>  {
> -   int ret, i;
> +   struct iio_dummy_event *iio_event;
>
> -   iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL);
> -   if (!iio_evgen)
> +   iio_event = kzalloc(sizeof(*iio_event), GFP_KERNEL);
> +   if (!iio_event)
> return -ENOMEM;
>
> -   iio_evgen->base = irq_alloc_descs(-1, 0, IIO_EVENTGEN_NO, 0);
> -   if (iio_evgen->base < 0) {
> -   ret = iio_evgen->base;
> -   kfree(iio_evgen);
> -   return ret;
> -   }
> -   iio_evgen->chip.name = iio_evgen_name;
> -   iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask;
> -   iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask;
> -   for (i = 0; i < IIO_EVENTGEN_NO; i++) {
> -   irq_set_chip(iio_evgen->base + i, &iio_evgen->chip);
> -   irq_set_handler(iio_evgen->base + i, &handle_simple_irq);
> -   irq_modify_status(iio_evgen->base + i,
> - IRQ_NOREQUEST | IRQ_NOAUTOEN,
> - IRQ_NOPROBE);
> -   }
> -   mutex_init(&iio_evgen->lock);
> +   iio_event->dev = indio_dev;
> +   iio_event->regs.reg_id = index;
> +   mutex_lock(&iio_dummy_event_list_mutex);
> +   list_add(&iio_event->l, &iio_dummy_event

Re: [PATCH V2] iio: declare struct to fix warning

[Crt Mori]

On 4 August 2015 at 09:56, Pengyu Ma  wrote:
>
>
> On 08/04/2015 03:54 PM, Lars-Peter Clausen wrote:
>>
>> On 08/04/2015 09:52 AM, Pengyu Ma wrote:
>>>
>>> If this patch is fine, could somebody help me to merge into upstream
>>> kernel?
>>
>> It will be merged, don't worry. But things typically take a bit longer
>> than
>> just a single day.
>
> So it's will be merge by patch, no need a pull-request, right?
Correct.
>
> Thanks,
> Pengyu
>
>>> Thanks,
>>> Pengyu
>>>
>>> On 08/03/2015 10:39 AM, Pengyu Ma wrote:

 When compile iio related driver the following warning shown:

 include/linux/iio/trigger.h:35:34: warning: 'struct iio_trigger'
 declared inside parameter list
 int (*set_trigger_state)(struct iio_trigger *trig, bool state);

 include/linux/iio/trigger.h:38:18: warning: 'struct iio_dev'
 declared inside parameter list
  struct iio_dev *indio_dev);

 'struct iio_dev' and 'struct iio_trigger' was used before declaration,
 forward declaration for these structs to fix warning.

 Signed-off-by: Pengyu Ma 
 ---
include/linux/iio/trigger.h | 4 
1 file changed, 4 insertions(+)

 diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
 index fa76c79..974cf73 100644
 --- a/include/linux/iio/trigger.h
 +++ b/include/linux/iio/trigger.h
 @@ -18,6 +18,10 @@ struct iio_subirq {
bool enabled;
};
+/* forward declaration */
 +struct iio_dev;
 +struct iio_trigger;
 +
/**
 * struct iio_trigger_ops - operations structure for an iio_trigger.
 * @owner:used to monitor usage count of the trigger.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/