Re: [PATCH v2 3/4] staging: iio: ad5933: add ABI documentation
On Sun, 3 Mar 2019 16:30:07 -0300 Marcelo Schmitt wrote: > On 03/03, Jonathan Cameron wrote: > > On Thu, 28 Feb 2019 23:53:14 -0300 > > Marcelo Schmitt wrote: > > > > > Add an ABI documentation for the ad5933 driver. > > > > > > Signed-off-by: Marcelo Schmitt > > Hi Marcelo, > > > > The ABI that you have defined which is actually new is mostly fine, > > however it seems that some of the existing ABI is not used correctly > > and that will need to be fixed. > > > > Jonathan > > Hi Jonathan, > > Thanks for the comments. > > > > > > --- > > > .../ABI/testing/sysfs-bus-iio-ad5933 | 91 +++ > > > 1 file changed, 91 insertions(+) > > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > > b/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > > new file mode 100644 > > > index ..81e3d3f6f724 > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > > @@ -0,0 +1,91 @@ > > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale > > > +Date:February 2019 > > > +KernelVersion: Kernel 4.19 > > > +Contact: linux-...@vger.kernel.org > > > +Description: > > > + The output peak-to-peak voltage range. Writting 0 sets range > > > + to 2.0V p-p typical, 1 sets range to 200mV p-p typical, 2 sets > > > + range to 400mV p-p typical, 3 sets range to 1.0V p-p typical. > > > + The p-p value of the ac output exitation voltage scales with > > > + supply voltage according to the following formula: > > > + Output Excitation Voltage (V p-p) = normalized_3v3 × [VDD/3.3] > > > + where normalized_3v3 is one of the four voltage range above and > > > + VDD is the supply voltage. > > > > Definitely not. The device must comply with standard ABI and this is not > > using it correctly (see below or the docs). > > > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale_available > > > +Date:February 2019 > > > +KernelVersion: Kernel 4.19 > > > +Contact: linux-...@vger.kernel.org > > > +Description: > > > + Prints available peak-to-peak voltage range to buffer. > > > > I really hope it doesn't. Scale is a mulitplier not a range. It should be > > printing out the value by which the _raw reading should be multiplied to > > get the voltage in the base units for voltage (millivolts) > > As the driver is right now I believe that calling this device attribute will > print "1980, 970, 383, 198" which are the mean values of four distributions > (called range1 to range4) of possible actual millivolt values used as the AC > output excitation. Interesting. > Certainly they don't mean "the value by which the _raw > reading should be multiplied to get the voltage in the base units for voltage" > so, as you said, this ABI is probably being misused here. Hence, I guess > I ought to change this attribute's call to something different than > out_voltage0_scale_available (as well as the out_voltage0_scale above). Maybe > out_voltage0_excit_available or out_voltage0_mean_excit_available, any other > suggestion? It's a perfectly good output alternating voltage, we have altvoltage for that where the value corresponds to peak to peak voltage. However, I've just noticed we don't actually seem to specify 'how' that is measured. Hmm. My assumption would be that existing devices are measuring peak voltage. Given voltage is measured in millivolts anyway, out_altvoltage_raw of 0,1,2,3 with scale also provided as 198 should work nicely I think? > > > > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_start > > > +Date:February 2019 > > > +KernelVersion: Kernel 4.19 > > > +Contact: linux-...@vger.kernel.org > > > +Description: > > > + The start frequency. Set this to define de frequency point at > > > + which the device should start the next frequency sweep. Default > > > + start frequency point set to 1Hz. > > > > No need to specify the default. It's trivial to read and any user of this > > chip should set it to the value they want. > > OK, then I will remove these on the next patch. > > > > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_increment > > > +Date:February 2019 > > > +KernelVersion: Kernel 4.19 > > > +Contact: linux-...@vger.kernel.org > > > +Description: > > > + The frequency sweep increment. Set this to define at which rate > > > + frequency sweep points are incremented. > > Rate is a temporal term. Perhaps "step by which the frequency is > > incremented"? > > Maybe > > "define the amount by which the frequency is incremented after each scan > point." > > would it be ok? That's good. > > > > > > After the measurement at > > > + a frequency point is completed, the next measurement will be > > > +
Re: [PATCH v2 3/4] staging: iio: ad5933: add ABI documentation
On 03/03, Jonathan Cameron wrote: > On Thu, 28 Feb 2019 23:53:14 -0300 > Marcelo Schmitt wrote: > > > Add an ABI documentation for the ad5933 driver. > > > > Signed-off-by: Marcelo Schmitt > Hi Marcelo, > > The ABI that you have defined which is actually new is mostly fine, > however it seems that some of the existing ABI is not used correctly > and that will need to be fixed. > > Jonathan Hi Jonathan, Thanks for the comments. > > > --- > > .../ABI/testing/sysfs-bus-iio-ad5933 | 91 +++ > > 1 file changed, 91 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > b/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > new file mode 100644 > > index ..81e3d3f6f724 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > @@ -0,0 +1,91 @@ > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale > > +Date: February 2019 > > +KernelVersion: Kernel 4.19 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + The output peak-to-peak voltage range. Writting 0 sets range > > + to 2.0V p-p typical, 1 sets range to 200mV p-p typical, 2 sets > > + range to 400mV p-p typical, 3 sets range to 1.0V p-p typical. > > + The p-p value of the ac output exitation voltage scales with > > + supply voltage according to the following formula: > > + Output Excitation Voltage (V p-p) = normalized_3v3 × [VDD/3.3] > > + where normalized_3v3 is one of the four voltage range above and > > + VDD is the supply voltage. > > Definitely not. The device must comply with standard ABI and this is not > using it correctly (see below or the docs). > > > + > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale_available > > +Date: February 2019 > > +KernelVersion: Kernel 4.19 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + Prints available peak-to-peak voltage range to buffer. > > I really hope it doesn't. Scale is a mulitplier not a range. It should be > printing out the value by which the _raw reading should be multiplied to > get the voltage in the base units for voltage (millivolts) As the driver is right now I believe that calling this device attribute will print "1980, 970, 383, 198" which are the mean values of four distributions (called range1 to range4) of possible actual millivolt values used as the AC output excitation. Certainly they don't mean "the value by which the _raw reading should be multiplied to get the voltage in the base units for voltage" so, as you said, this ABI is probably being misused here. Hence, I guess I ought to change this attribute's call to something different than out_voltage0_scale_available (as well as the out_voltage0_scale above). Maybe out_voltage0_excit_available or out_voltage0_mean_excit_available, any other suggestion? > > > + > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_start > > +Date: February 2019 > > +KernelVersion: Kernel 4.19 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + The start frequency. Set this to define de frequency point at > > + which the device should start the next frequency sweep. Default > > + start frequency point set to 1Hz. > > No need to specify the default. It's trivial to read and any user of this > chip should set it to the value they want. OK, then I will remove these on the next patch. > > > + > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_increment > > +Date: February 2019 > > +KernelVersion: Kernel 4.19 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + The frequency sweep increment. Set this to define at which rate > > + frequency sweep points are incremented. > Rate is a temporal term. Perhaps "step by which the frequency is > incremented"? Maybe "define the amount by which the frequency is incremented after each scan point." would it be ok? > > > After the measurement at > > + a frequency point is completed, the next measurement will be > > + made with a frequency 'frequency increment'Hz higher than the > > + previous point until the defined number of increments has been > > + made. Default frequency increment set to 200Hz. > > No need to specify the default. People can just read the file to find that > out. > > > + > > + > > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_points > > +Date: February 2019 > > +KernelVersion: Kernel 4.19 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + The number of increments. This defines the number of frequency > > + points in the frequency sweep. Device stores a 9-bit integer > > + number in binary fo
Re: [PATCH v2 3/4] staging: iio: ad5933: add ABI documentation
On Thu, 28 Feb 2019 23:53:14 -0300 Marcelo Schmitt wrote: > Add an ABI documentation for the ad5933 driver. > > Signed-off-by: Marcelo Schmitt Hi Marcelo, The ABI that you have defined which is actually new is mostly fine, however it seems that some of the existing ABI is not used correctly and that will need to be fixed. Jonathan > --- > .../ABI/testing/sysfs-bus-iio-ad5933 | 91 +++ > 1 file changed, 91 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ad5933 > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > b/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > new file mode 100644 > index ..81e3d3f6f724 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad5933 > @@ -0,0 +1,91 @@ > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale > +Date:February 2019 > +KernelVersion: Kernel 4.19 > +Contact: linux-...@vger.kernel.org > +Description: > + The output peak-to-peak voltage range. Writting 0 sets range > + to 2.0V p-p typical, 1 sets range to 200mV p-p typical, 2 sets > + range to 400mV p-p typical, 3 sets range to 1.0V p-p typical. > + The p-p value of the ac output exitation voltage scales with > + supply voltage according to the following formula: > + Output Excitation Voltage (V p-p) = normalized_3v3 × [VDD/3.3] > + where normalized_3v3 is one of the four voltage range above and > + VDD is the supply voltage. Definitely not. The device must comply with standard ABI and this is not using it correctly (see below or the docs). > + > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale_available > +Date:February 2019 > +KernelVersion: Kernel 4.19 > +Contact: linux-...@vger.kernel.org > +Description: > + Prints available peak-to-peak voltage range to buffer. I really hope it doesn't. Scale is a mulitplier not a range. It should be printing out the value by which the _raw reading should be multiplied to get the voltage in the base units for voltage (millivolts) > + > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_start > +Date:February 2019 > +KernelVersion: Kernel 4.19 > +Contact: linux-...@vger.kernel.org > +Description: > + The start frequency. Set this to define de frequency point at > + which the device should start the next frequency sweep. Default > + start frequency point set to 1Hz. No need to specify the default. It's trivial to read and any user of this chip should set it to the value they want. > + > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_increment > +Date:February 2019 > +KernelVersion: Kernel 4.19 > +Contact: linux-...@vger.kernel.org > +Description: > + The frequency sweep increment. Set this to define at which rate > + frequency sweep points are incremented. Rate is a temporal term. Perhaps "step by which the frequency is incremented"? > After the measurement at > + a frequency point is completed, the next measurement will be > + made with a frequency 'frequency increment'Hz higher than the > + previous point until the defined number of increments has been > + made. Default frequency increment set to 200Hz. No need to specify the default. People can just read the file to find that out. > + > + > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_points > +Date:February 2019 > +KernelVersion: Kernel 4.19 > +Contact: linux-...@vger.kernel.org > +Description: > + The number of increments. This defines the number of frequency > + points in the frequency sweep. Device stores a 9-bit integer > + number in binary format for this so the maximum number of > + increments that can be programmed is 511. I would relax this ABI description so that it doesn't describe the bit depth and ideally doesn't describe the range either. If we want to provide the range, then provide an available attribute to pair with this. The reason is that we should be looking to define interfaces in a way that allows them to be potentially generalised to similar devices in future. I suspect a lot of this is generic to impedance measuring ICs. > + > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_settling_cycles > +Date:February 2019 > +KernelVersion: Kernel 4.19 > +Contact: linux-...@vger.kernel.org > +Description: > + Number of settling time cycles. This attribute is a 11 bit field > + divided in two parts. The 9 least significant bit define the > + number of output excitation cycles that are passed through the > + unknown impedance, after the receipt of a start frequency sweep, > +
