Minor changes

1 files changed, 121 insertions, 91 deletions

diff --git a/ina260.c b/ina260.c
index 946a69c..97a7821 100644
--- a/ina260.c
+++ b/ina260.c
@@ -1,11 +1,12 @@
-#include "linux/module.h" // to access basic module functions (module_init etc..)
-#include "linux/uaccess.h" // copy_from_user() etc...
+#include "linux/module.h"
+#include "linux/uaccess.h"
 #include "linux/i2c.h"
 #include "linux/kobject.h"
-#include "linux/slab.h" // kzalloc()
-#include "linux/kernel.h" // Kernel utility e.g kstrtol
+#include "linux/slab.h"
+#include "linux/kernel.h"
 #include <linux/sysfs.h>
 
+// ina260 chip registers
 #define INA260_REG_CONFIGURATION 0x00
 #define INA260_REG_CURRENT 0x01
 #define INA260_REG_VOLTAGE 0x02
@@ -17,10 +18,12 @@
 
 #define INA260_IS_ATTR(_name) (strcmp(attr->attr.name, #_name) == 0)
 
+// ina260 average modes list
 static int ina260_avgs[8]={
     1,4,16,64,128,256,512,1024
 };
 
+// ina260 operating modes list
 static char ina260_modes[8][42]={
     "Power-Down (or Shutdown)","Shunt Current, Triggered",
     "Bus Voltage, Triggered","Shunt Current and Bus Voltage, Triggered", 
@@ -28,32 +31,41 @@ static char ina260_modes[8][42]={
     "Shunt Current and Bus Voltage, Continuous"
 };
 
+// ina260 conversion times list
 static char ina260_ishcts_vbusct[8][9]={
     "140 µs", "204 µs","332 µs", "588 µs","1.1 ms", "2.116 ms","4.156 ms","8.244 ms"
 };
 
-
+// Driver root
 static struct kobject* ina260_kobj;
 
+// Data attached to i2c clients
 struct client_data {
     struct kobject kobj;
     struct i2c_client *client;
-    unsigned char reg; // Client selected register
+    unsigned char reg; // Slave selected register
 };
+
 static const struct i2c_device_id ina260_ids[] = {
     { "ina260", 0 },
     { }
 };
-MODULE_DEVICE_TABLE(i2c,ina260_ids); // Allow i2c subsystem to probe
-
+MODULE_DEVICE_TABLE(i2c,ina260_ids);
+
+/**
+ * @brief Read from ina260 registers
+ * 
+ * @param cdata client data to use to communicate
+ * @param reg register to read
+ * @param value register content output
+ * @return int 0 on success, 1 on communication errors
+ */
 static int ina260_read_register(struct client_data* cdata, unsigned char reg, int *value){
     unsigned char bytes[2];
     if(cdata->reg == reg){
-        //printk("Cache\n");
         if(i2c_master_recv(cdata->client,bytes,2)<0)
             return 1;
     } else {
-        //printk("No cache\n");
         if(i2c_master_send(cdata->client,&reg,1)<0)
             return 1;
         cdata->reg = reg;
@@ -64,6 +76,14 @@ static int ina260_read_register(struct client_data* cdata, unsigned char reg, in
     return 0;
 }
 
+/**
+ * @brief Write to ina260 registers
+ * 
+ * @param cdata client data to use to communicate
+ * @param reg register to write to
+ * @param value value to write in @a reg
+ * @return int 0 on success, 1 on communication errors
+ */
 static int ina260_write_register(struct client_data* cdata, unsigned char reg, int value){
     unsigned char data[3];
     data[0]=reg;
@@ -75,11 +95,11 @@ static int ina260_write_register(struct client_data* cdata, unsigned char reg, i
 }
 
 static ssize_t attr_show(struct kobject *_kobj, 
-                      struct kobj_attribute *attr,
-                     char *buf)
+                    struct kobj_attribute *attr,
+                    char *buf)
 {
-    int value;
-    struct client_data *data=container_of(_kobj,struct client_data,kobj);
+    int rvalue;
+    struct client_data *cdata=container_of(_kobj,struct client_data,kobj);
     unsigned char reg=INA260_REG_POWER;
 
     if(!INA260_IS_ATTR(power)){
@@ -100,10 +120,10 @@ static ssize_t attr_show(struct kobject *_kobj,
         }
     }
 
-    if(ina260_read_register(data,reg,&value))
+    if(ina260_read_register(cdata,reg,&rvalue))
         return -1;
 
-    return sprintf(buf, "%x\n", value);
+    return sprintf(buf, "%x\n", rvalue);
 }
  
  
@@ -111,10 +131,11 @@ static ssize_t attr_store(struct kobject *_kobj,
                        struct kobj_attribute *attr,
                const char *buf, size_t count)
 {
+    int uvalue;
     struct client_data *cdata=container_of(_kobj,struct client_data,kobj);
-    int data;
     unsigned char reg=INA260_REG_CONFIGURATION;
-    if(kstrtoint(buf, 0,&data))
+    
+    if(kstrtoint(buf, 0,&uvalue))
         return -EINVAL;
 
     if (INA260_IS_ATTR(mask_enable)){
@@ -123,7 +144,7 @@ static ssize_t attr_store(struct kobject *_kobj,
         reg=INA260_REG_ALERTLIMIT;
     }
 
-    if(ina260_write_register(cdata, reg, data))
+    if(ina260_write_register(cdata, reg, uvalue))
         return -1;
 
     return count;
@@ -133,112 +154,110 @@ static ssize_t attr_field_show(struct kobject *_kobj,
                       struct kobj_attribute *attr,
                      char *buf)
 {
+    int rvalue;
     struct client_data *cdata=container_of(_kobj,struct client_data,kobj);
-    int value;
     if(INA260_IS_ATTR(avg)){
-        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&value))
+        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", ina260_avgs[(value>>9)&0x7]);
+        return sprintf(buf, "%d\n", ina260_avgs[(rvalue>>9)&0x7]);
     } else if (INA260_IS_ATTR(mode)){
-        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&value))
+        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&rvalue))
             return -1;
-        return sprintf(buf, "%s\n", ina260_modes[value&0x7]);
+        return sprintf(buf, "%s\n", ina260_modes[rvalue&0x7]);
     } else if (INA260_IS_ATTR(ishct)){
-        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&value))
+        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&rvalue))
             return -1;
-        return sprintf(buf, "%s\n", ina260_ishcts_vbusct[(value>>3)&0x7]);
+        return sprintf(buf, "%s\n", ina260_ishcts_vbusct[(rvalue>>3)&0x7]);
     } else if (INA260_IS_ATTR(vbusct)){
-        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&value))
+        if(ina260_read_register(cdata, INA260_REG_CONFIGURATION,&rvalue))
             return -1;
-        return sprintf(buf, "%s\n", ina260_ishcts_vbusct[(value>>6)&0x7]);
+        return sprintf(buf, "%s\n", ina260_ishcts_vbusct[(rvalue>>6)&0x7]);
     } else if (INA260_IS_ATTR(aff)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>4)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>4)&0x1);
     } else if (INA260_IS_ATTR(cvrf)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>3)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>3)&0x1);
     } else if (INA260_IS_ATTR(ocl)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>15)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>15)&0x1);
     }else if (INA260_IS_ATTR(ucl)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>14)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>14)&0x1);
     }else if (INA260_IS_ATTR(bol)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>13)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>13)&0x1);
     }else if (INA260_IS_ATTR(bul)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>12)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>12)&0x1);
     }else if (INA260_IS_ATTR(pol)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>11)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>11)&0x1);
     }else if (INA260_IS_ATTR(cnvr)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>10)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>10)&0x1);
     }else if (INA260_IS_ATTR(ovf)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>2)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>2)&0x1);
     }else if (INA260_IS_ATTR(apol)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", (value>>1)&0x1);
+        return sprintf(buf, "%d\n", (rvalue>>1)&0x1);
     }else if (INA260_IS_ATTR(len)){
-        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&value))
+        if(ina260_read_register(cdata, INA260_REG_MASKENABLE,&rvalue))
             return -1;
-        return sprintf(buf, "%d\n", value&0x1);
+        return sprintf(buf, "%d\n", rvalue&0x1);
     }else if (INA260_IS_ATTR(did)){
-        if(ina260_read_register(cdata, INA260_REG_DIE,&value))
+        if(ina260_read_register(cdata, INA260_REG_DIE,&rvalue))
             return -1;
-        return sprintf(buf, "0x%x\n", (value>>4));
+        return sprintf(buf, "0x%x\n", (rvalue>>4));
     }else if (INA260_IS_ATTR(rid)){
-        if(ina260_read_register(cdata, INA260_REG_DIE,&value))
+        if(ina260_read_register(cdata, INA260_REG_DIE,&rvalue))
             return -1;
-        return sprintf(buf, "0x%x\n", value&0x7);
+        return sprintf(buf, "0x%x\n", rvalue&0x7);
     }else if (INA260_IS_ATTR(reset)){
         return sprintf(buf, "0\n");
     }
     return 0;
 }
 
-static int ina260_write_field3(struct client_data *cdata, unsigned char reg, unsigned char n, int value3bits){
-    int mask, value;
-    if(!(value3bits>=0 && value3bits <8))
+static int ina260_write_field3(struct client_data *cdata, unsigned char reg, unsigned char n, int uvalue){
+    int rvalue;
+    if(!(uvalue>=0 && uvalue <8))
         return -EINVAL;
     // Fetch register value:
-    if(ina260_read_register(cdata,reg,&value))
+    if(ina260_read_register(cdata,reg,&rvalue))
         return 1;
     // Write bits:
-    mask=~(0x7 << n);
-    value &= mask; // clear bits
-    value |= (value3bits << n);
+    rvalue &= ~(0x7 << n); // clear bits
+    rvalue |= (uvalue << n);
     // Write register value:
-    if(ina260_write_register(cdata,reg,value))
+    if(ina260_write_register(cdata,reg,rvalue))
         return 1;
     return 0;
 }
 
-static int ina260_write_field1(struct client_data *cdata, unsigned char reg, unsigned char n, int bit){
-    int mask, value;
-    if(!(bit==0 || bit == 1))
+static int ina260_write_field1(struct client_data *cdata, unsigned char reg, unsigned char n, int uvalue){
+    int rvalue;
+    if(!(uvalue==0 || uvalue == 1))
         return -EINVAL;
     // Fetch register value:
-    if(ina260_read_register(cdata,reg,&value))
+    if(ina260_read_register(cdata,reg,&rvalue))
         return 1;
     // Set bit:
-    mask=~(1<< n);
-    value &= mask; // clear bit
-    value |= (bit << n);
+    rvalue &= ~(1<< n); // clear bit
+    rvalue |= (uvalue << n);
     // Write register value
-    if(ina260_write_register(cdata,reg,value))
+    if(ina260_write_register(cdata,reg,rvalue))
         return 1;
     return 0;
 }
@@ -247,38 +266,38 @@ static ssize_t attr_field_store(struct kobject *_kobj,
                        struct kobj_attribute *attr,
                const char __user *buf, size_t count)
 {
+    int uvalue, ret=0;
     struct client_data *cdata=container_of(_kobj,struct client_data,kobj);
-    int data=0, ret=count;
-    // Extract user supplied value
-    if(kstrtoint(buf, 0,&data))
+    // Extract user supplied value:
+    if(kstrtoint(buf, 0,&uvalue))
         return -EINVAL;
     // Store:
-    if(INA260_IS_ATTR(reset) && data!=0){
+    if(INA260_IS_ATTR(reset) && uvalue!=0){
         ret=ina260_write_register(cdata, INA260_REG_CONFIGURATION, 0xFFFF); // Much quicker this way
     } else if(INA260_IS_ATTR(avg)){
-      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,9,data);
+      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,9,uvalue);
     } else if(INA260_IS_ATTR(mode)){
-      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,0,data);
+      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,0,uvalue);
     } else if(INA260_IS_ATTR(ishct)){
-      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,3,data);
+      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,3,uvalue);
     } else if(INA260_IS_ATTR(vbusct)){
-      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,6,data);
+      ret=ina260_write_field3(cdata,INA260_REG_CONFIGURATION,6,uvalue);
     } else if(INA260_IS_ATTR(ocl)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,15,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,15,uvalue);
     } else if(INA260_IS_ATTR(ucl)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,14,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,14,uvalue);
     } else if(INA260_IS_ATTR(bol)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,13,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,13,uvalue);
     } else if(INA260_IS_ATTR(bul)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,12,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,12,uvalue);
     } else if(INA260_IS_ATTR(pol)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,11,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,11,uvalue);
     } else if(INA260_IS_ATTR(cnvr)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,10,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,10,uvalue);
     } else if(INA260_IS_ATTR(apol)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,1,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,1,uvalue);
     } else if(INA260_IS_ATTR(len)){
-      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,0,data);
+      ret=ina260_write_field1(cdata,INA260_REG_MASKENABLE,0,uvalue);
     }
     return ret ? ret: count;
 }
@@ -320,7 +339,6 @@ static const struct attribute_group registers_group = {
 	.attrs = registers_attrs,
     .name = "registers"
 };
-
 // ----- Fields -----
 static struct kobj_attribute reset_field_attribute =
     __ATTR(reset, 0664, attr_field_show, attr_field_store);
@@ -388,6 +406,14 @@ static struct kobj_type ina260_ktype = {
 	.sysfs_ops	= &kobj_sysfs_ops,
 };
 
+/**
+ * @brief Compare on ina260 register to a supplied value
+ * 
+ * @param client i2c client to use for communications
+ * @param reg register to use for comparison
+ * @param value register expected value
+ * @return int 0 if register contains value and 1 otherwise
+ */
 static int ina260_probe_register(struct i2c_client *client, unsigned char reg, int value){
     unsigned char bytes[2];
     if(i2c_master_send(client,&reg,1)<0)
@@ -399,22 +425,26 @@ static int ina260_probe_register(struct i2c_client *client, unsigned char reg, i
 
 static int ina260_probe_new(struct i2c_client *client){
     struct client_data *p;
+    // Attempt device discovery:
     if(ina260_probe_register(client,INA260_REG_MANUFACTURER,0x5449) ||
        ina260_probe_register(client,INA260_REG_DIE,0x2270)){
-        printk("INA260 chip probe failed on i2c bus %d\n",client->adapter->nr);
+        printk("ina260 probe fails bus=%d address=0x%02x\n",client->adapter->nr,client->addr);
         return 1;
     }
-    printk("New INA260 chip addr=0x%x detected on i2c bus %d\n",client->addr,client->adapter->nr);
+    // Initialize client data:
+    printk("ina260 detected bus=%d address=0x%02x\n",client->adapter->nr,client->addr);
     p=kzalloc(sizeof(struct client_data),GFP_KERNEL);
     kobject_init(&p->kobj,&ina260_ktype);
+    // Setup sysfs root kobject:
     if(kobject_add(&p->kobj,ina260_kobj,client->dev.kobj.name)){
         kobject_put(&p->kobj);
         kfree(p);
         return 1;
     }
     p->client=client;
-    p->reg=INA260_REG_DIE; // Last reg to be used
+    p->reg=INA260_REG_DIE; // Maintain cache coherence
     i2c_set_clientdata(client,p);
+    // Setup sysfs groups:
     if(sysfs_create_group(&p->kobj,&registers_group)||
        sysfs_create_group(&p->kobj,&fields_group)){
         kobject_put(&p->kobj);
@@ -428,7 +458,7 @@ static int ina260_remove(struct i2c_client *client){
     struct client_data *p=i2c_get_clientdata(client);
     kobject_put(&p->kobj);
     kfree(p);
-    printk("INA260 addr=0x%x remove from i2c bus %d\n",client->addr,client->adapter->nr);
+    printk("ina260 removed bus=%d address=0x%02x\n",client->adapter->nr,client->addr);
     return 0;
 }