First project idea: AD7292 and two’s component
With my development enviroment working, I now had to choose an individual project to contribute to. We’d been given four suggested issues to work on, each accompanied by a list of drivers that needed a patch for that issue. The issues had been tagged with their estimated complexity, with the first suggestion being just fixing code style errors (easy), the second and fourth being replacing a small chunk of code with an equivalent scoped macro to facilitate error handling (easy to moderate) and the third one being fixing read outputs to correctly return negative values when a device used two’s complement encoding, usually in differential mode ADCs (moderate). Since I’m a fan of challenges and in possession of questionable self-preservation instincts when it comes to academics, I chose the most complex category right away and picked the device ad7292, a 10-bit Analog-to-Digital Converter (ADC) that also had temperature measuring and digital-to-analog conversion capabilities and, most importantly, a toggle for a differential mode, in which the device would output the difference in voltage between the inputs in the first two input channels, Vin0 and Vin1. In an usual non-differential scenario, the measured voltages were always positive values, as the inputs were measured relative to ground. In differential mode, however, the voltage differential between Vin0 and Vin1 could be negative or positive depending on which input was higher, which meant that the range of possible output values was twice that of the non-differential case, going from -Vmax to Vmax instead of from 0 to Vmax. My task was then to add a condition on the drivers read_raw function so that the driver would correctly interpret the negative values returned when the device was in differential mode.
Binary encoding 101 (= 5 or = -3 or = 1?)
Binary as a representation of positive numbers is pretty straight forward: in n bits, we can store 2n possible values, which in positive straight binary range from 0 to 2n-1. With four bits, for example, we can store all integers from 0000 -> 0 to 1111 -> 15 (24-1). If we need those four bits to also encode negative numbers, however, we have a few options on how to encode our value. We know that our four bits can only hold 2n possible values, so if we want to give the negatives an equal share of the range our binary code represents, we can, for example, shift our range from 0000 -> 0 | 1111 -> 15 to 0000 -> -8 | 1111 -> 7. We are still using our 16 “slots” of values, but now we’ve offset the starting value by half our range, letting us encode negative numbers. This encoding is known as offset binary since it works by, well, offsetting the value with a subtraction.
VERY IMPORTANT: Encoding is a matter of interpretation. The number 0001 does not “contain” an encoding or a “true value”. It can be equal to 1 or -7 or any other value depending on how we choose to interpret it. This is why this patch is relevant: our driver needs to know how it should interpret the 10 bits of output binary it receives from the device. Depending on the situation, the same 10 bit code can mean radically different values and it is up to the driver to translate it correctly based on its knowledge of the encoding used for that context.
Another option for encoding is sign-magnitude encoding, in which we designate the most significant bit of our binary code as the sign and the rest as the magnitude. This is equivalent to writing + or - before the value represented by the magnitude, which would give us values like 0000 -> +0, 1000 -> -0, 0111 -> +7 and 0111 -> -7. This encoding is problematic because it sacrifices a slot to represent both +0 and -0, which are equivalent, and because it doesn’t naturally support arithmetic operations such as sum, since 0001 (+1) + 1001 (-1) = 1010 (-2), which is incorrect. To address this issue, a third possible encoding, two’s complement, represents signed values by inverting the sign of the power of two represented by the most significant bit: instead of reading 1001 as 1001 = 8*1 + 4*0 + 2*0 + 1*1 = 9, we interpret 1001 as 1001 = -8*1 + 4*0 + 2*0 + 1*1 = -7. Since nothing changes when the most significant bit is set to zero, positive numbers don’t change in two’s complement encoding. We also only have one representation of zero and arithmetic operations work without needing to be sign-specific: 0001 (+1) + 1001 (-7) = 1010 (-6). Curiously, two’s complement and offset binary differ only in the value of the most significant bit, so a number written in offset binary can be converted to two’s complement simply by flipping its most significant bit (and vice versa).
Back to ad7292
The device was listed in the suggested patches file as a candidate for patch #3 since it seemed like it used two’s complement encoding to output negative values in the differential case, but after investigating the device’s datasheet for a while I found out that it instead used offset straight binary. Although this seems obvious now (it’s pretty much written verbatim on the datasheet) it took me quite a while to figure it out, since I was completely unfamiliar with both ADCs and drivers in general. After a few days of studying this and other devices’s datasheets and familiarizing myself with the way drivers were implemented and how they functioned, I confirmed that the current implementation of the ad7292 driver didn’t process the output correctly in differential mode, but I also realized that the fix would be more complicated than expected for this first project. Due to that, I decided to stash this project for later.
rtq6056 speedrun any%
With just a few days until the deadline for this project, I had to pretty much start from scratch. Due to that, I decided to pivot to a less complicated project: rewriting a chunk of rtq6056 to use a scoped macro to simplify error handling. The patch I wrote did just that, replacing
static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int val,
int val2, long mask)
{
struct rtq6056_priv *priv = iio_priv(indio_dev);
const struct richtek_dev_data *devdata = priv->devdata;
int ret;
ret = iio_device_claim_direct_mode(indio_dev);
if (ret)
return ret;
switch (mask) {
case IIO_CHAN_INFO_SAMP_FREQ:
if (devdata->fixed_samp_freq) {
ret = -EINVAL;
break;
}
ret = rtq6056_adc_set_samp_freq(priv, chan, val);
break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
ret = devdata->set_average(priv, val);
break;
default:
ret = -EINVAL;
break;
}
iio_device_release_direct_mode(indio_dev);
return ret;
}
with
static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int val,
int val2, long mask)
{
struct rtq6056_priv *priv = iio_priv(indio_dev);
const struct richtek_dev_data *devdata = priv->devdata;
iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
switch (mask) {
case IIO_CHAN_INFO_SAMP_FREQ:
if (devdata->fixed_samp_freq)
return -EINVAL;
return rtq6056_adc_set_samp_freq(priv, chan, val);
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
return devdata->set_average(priv, val);
default:
return -EINVAL;
}
}
unreachable();
}
The motivation behind this change is to avoid using the confusing syntax of breaks leading to a single return that may or may not be an error. Using the iio_device_claim_direct_scoped handler, I was able to replace the ret assignments with direct returns that made understanding and following the flow of the code much easier. This project was much more straightforward than the other one, especially since it was mostly just refactoring code instead of having to learn how the device worked and what it would take to add new functionality. I managed to get it done it a day (although I was already much more familiar with the kernel due to the previous weeks of studying for ad7292) and submitted my patch, which was a smooth (if nerve-wracking) process (I did almost send an incomplete test patch to everyone by accident, but luckily I had –dry-run on). The patch was approved with no further changes.