The Patch
In this post, we will describe the end-to-end process of contributing to the Linux kernel, specifically focusing on the IIO (Industrial I/O) subsystem.
1. Cloning the Correct Tree
Since we were already working on tutorials using the IIO tree, it was not necessary to clone it again. However, as my previous work was done on a testing branch, the first step was to ensure a clean workspace.
I moved my previous experiments to a dedicated backup branch (tutoriais-backup) to preserve them, and then performed a checkout to the development branch: togreg. Authoritative information about kernel trees and branches are available at MAINTAINERS file.
2. Deduplicating Code
New branch: iio-deduplicate-code
The code duplications targeted for refactoring were identified across several drivers within the IIO HID subsystem. Specifically, the following six functions—each consisting of 9 lines of identical logic were selected for deduplication:
Function find: accel/hid-sensor-accel-3d.c::accel_3d_adjust_channel_bit_mask, TOTAL NUMBER LINES IN FUNCTION: 9 Function find: pressure/hid-sensor-press.c::press_adjust_channel_bit_mask, TOTAL NUMBER LINES IN FUNCTION: 9 Function find: magnetometer/hid-sensor-magn-3d.c::magn_3d_adjust_channel_bit_mask, TOTAL NUMBER LINES IN FUNCTION: 9 Function find: light/hid-sensor-prox.c::prox_adjust_channel_bit_mask, TOTAL NUMBER LINES IN FUNCTION: 9 Function find: light/hid-sensor-als.c::als_adjust_channel_bit_mask, TOTAL NUMBER LINES IN FUNCTION: 9 Function find: gyro/hid-sensor-gyro-3d.c::gyro_3d_adjust_channel_bit_mask, TOTAL NUMBER LINES IN FUNCTION: 9
To begin the refactoring process, we checked the files:
- drivers/iio/accel/hid-sensor-accel-3d.c
- drivers/iio/pressure/hid-sensor-press.c
- drivers/iio/magnetometer/hid-sensor-magn-3d.c
- drivers/iio/light/hid-sensor-prox.c
- drivers/iio/light/hid-sensor-als.c
- drivers/iio/gyro/hid-sensor-gyro-3d.c
Aside from their names, the functions are identical:
static void magn_3d_adjust_channel_bit_mask(struct iio_chan_spec *channels,
int channel, int size)
{
channels[channel].scan_type.sign = 's';
/* Real storage bits will change based on the report desc. */
channels[channel].scan_type.realbits = size * 8;
/* Maximum size of a sample to capture is u32 */
channels[channel].scan_type.storagebits = sizeof(u32) * 8;
}
We decided to create a helper function to replace the local functions in each file.
static void hid_sensor_adjust_channel_bit_mask(struct iio_chan_spec *channels,
int channel, int size)
{
channels[channel].scan_type.sign = 's';
/* Real storage bits will change based on the report desc. */
channels[channel].scan_type.realbits = size * 8;
/* Maximum size of a sample to capture is u32 */
channels[channel].scan_type.storagebits = sizeof(u32) * 8;
}
Since the code was identical across multiple drivers, we had to move it to a shared location. In the IIO HID subsystem, the most suitable place was drivers/iio/common/hid-sensors/hid-sensor-attributes.c, as it already manages shared attributes and formatting logic for these sensors.
We also exported the symbol using EXPORT_SYMBOL_NS to ensure that drivers compiled as loadable modules can access this function at runtime.
void hid_sensor_adjust_channel_bit_mask(struct iio_chan_spec *channels,
int channel, int size)
{
channels[channel].scan_type.sign = 's';
/* Real storage bits will change based on the report desc. */
channels[channel].scan_type.realbits = size * 8;
/* Maximum size of a sample to capture is u32 */
channels[channel].scan_type.storagebits = sizeof(u32) * 8;
}
EXPORT_SYMBOL_NS(hid_sensor_adjust_channel_bit_mask, "IIO_HID");
In addition to modifying this file, we also updated a header file: include/linux/hid-sensor-hub.h
void hid_sensor_adjust_channel_bit_mask(struct iio_chan_spec *channels,
int channel, int size);
We removed the local functions from all six files and replaced their calls with our new generic function.
3. Checks before submiting the patch
An important step before submitting a patch is to verify that it complies with the style guidelines. We check this using the following command:
./scripts/checkpatch.pl --terse --file \
drivers/iio/accel/hid-sensor-accel-3d.c \
drivers/iio/common/hid-sensors/hid-sensor-attributes.c \
drivers/iio/gyro/hid-sensor-gyro-3d.c \
drivers/iio/light/hid-sensor-als.c \
drivers/iio/light/hid-sensor-prox.c \
drivers/iio/magnetometer/hid-sensor-magn-3d.c \
drivers/iio/pressure/hid-sensor-press.c \
include/linux/hid-sensor-hub.h
Since we had already set the kw configurations, they remain valid and no changes were necessary.
cd "$IIO_TREE"
cat .kw/build.config
We compile our code to ensure everything is working correctly, following the process of the tutorial number 4.
We looked in the same directory of the files modified for a Kconfig, searched a config name and description that relates to where we were working. Then, opened the .config file for editing with kw build –menu. We also checked the Depends on attribute to check the dependencies that needed to be enabled before the module.
We enabled: HID_SENSOR_GYRO_3D, HID_SENSOR_ALS, HID_SENSOR_PROX, HID_SENSOR_MAGNETOMETER_3D, HID_SENSOR_PRESS, HID_SENSOR_ACCEL_3D, HID_SENSOR_HUB
After testing that our changes compile, we committed our work:
git commit --signoff \
--trailer='Co-developed-by: Pietro Di Consolo Gregorio <pietro.gregorio@usp.br>' \
--trailer='Signed-off-by: Pietro Di Consolo Gregorio <pietro.gregorio@usp.br>' \
-m "iio: HID: Add helper method hid_sensor_adjust_channel_bit_mask()" \
-m "Add helper method to deduplicate code in HID sensors."
At total we made 7 atomic commits.
The last check was sending the patch to the CI. To send patch using the usp email we also did the steps in tutorial 6.
kw send-patch --send -7 --private --to=kernel@lists.ime.usp.br --cc=natalia.andre@ime.usp.br,pietro.gregorio@usp.br
4. Sending the patch
Since no errors were found, we sent the patch to the correct list of mantainers
kw send-patch --send -7
We edited the email subject and blurb before sending too.
5. The first review
We received the first review two days later.
During the review, David Lechner provided specific feedback to improve code consistency and modern standards. He requested adjustments to the indentation logic—specifically ensuring alignment with opening parentheses or using an extra tab for long lines—and recommended replacing hardcoded values with the BITS_PER_BYTE macro. Additionally, he suggested updating the sign field to the newly adopted format name, advising that a precursor patch be created to rename existing instances within the file to maintain uniformity.
The new version of the code he talked about was actually in testing branch (instead of togreg), so we did a rebase to have the correct work in our branch.
Other maintainers also reviewed the patch series but offered no additional suggestions.
We addressed the requested fixes and used git commit --amend to update our previous work without introducing new commits.
git rebase -i HEAD~7
git commit --amend
git rebase --continue
git format-patch -7 --stdout | ./scripts/checkpatch.pl --
Then, we made the checks again and since no errors were found we submitted the patch, but with version 2 flag:
kw send-patch --send -7 -v2
6. The second review
We received a second round of reviews from multiple maintainers. They highlighted that some lines in the commit messages exceeded the 72-character limit. One of the maintainers pointed out that creating a new “helper” function might not be the most efficient approach for this specific case. Instead, he suggested using a compound literal struct initializer to make the code more explicit and follow modern C standards (C99) used in the kernel.
Looking at this again: This feels like a helper that doesn’t necessarily add much value.
channels[CHANNEL_SCAN_INDEX_X + i].scan_type = (struct iio_scan_type) { .format = 's', .real_bits = BYTES_TO_BITS(st->accel[CHANNEL_SCAN_INDEX_X + i].size), .storage_bits = BITS_PER_TYPE(u32), };
Maybe with local variables to help a touch. such as
unsigned int ch = CHANNEL_SCAN_INDEX_X + i]; channels[ch].scan_type = (struct iio_scan_type) { .format = 's', .real_bits = BYTES_TO_BITS(st->accel[ch].size), .storage_bits = BITS_PER_TYPE(u32), };
We created a backup branch to ensure we could revert to our previous work if needed and replaced the external function calls with a direct struct iio_scan_type initialization.
The manteiner also suggested a loop optimization in accel-3d.
Hmm. That’s also an odd loop as the use assumes CHANNEL_SCAN_INDEX_X = 0 (which is true) but then uses an offset that implies it isn’t. Clearer to just loop over the actual enum values. So probably wants to be something like. ```c for (unsigned int ch = CHANNEL_SCAN_INDEX_X; i <= CHANNEL_SCAN_INDEX_Z; ch++) { //maybe on a long single line. ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT, usage_id, ch, &st->accel[ch]); if (ret < 0) break;
channels[ch].scan_type = (struct iio_scan_type) {
.format = 's',
.real_bits = BYTES_TO_BITS(st->accel[ch].size),
.storage_bits = BITS_PER_TYPE(u32),
}; ```
We simplified the loop logic by using the channel index as the primary iterator, reducing redundant calculations.
Header Updates: To support the new logic, we had to include <linux/bitops.h> and <linux/bits.h>. These headers provide essential macros like BYTES_TO_BITS and BITS_PER_TYPE.
To fix indentation problems it was necessary to configurate vscode to use tabs identation (Ctrl+Shift+P + “Convert Indentation to Tabs”), with tab size 8.
https://docs.kernel.org/process/coding-style.html
It is important to note that at first the maintainers had accepted the idea of deduplication, until the most senior maintainer spoke out against it and suggested other changes.
7. The third review
In the third review manteiners noticed the there was another loop optimization opportunity in gyro-3d. They also pointed the lack of a changelog in the patch. Their AI bot reviewer was used to send more feedback and one manteiner tested the changes. The AI and the tests found the same problem: Incorrect HID attribute usage ID passed to sensor_hub_input_get_attribute_info(), breaking device initialization.
8. The fourth review
The bug was fixed and patch v4 was sent with a change log explaning the changes in each version. The patch was accepted after and merged in a testing branch.