Updates on Kernel patch submission

On the last post, I have covered the Kernel contribution chosen by me and my partner, and explained a little about the return obtained by the Kernel mantainer. Going back on it, I feel like it would be nice to have a separate post to cover that communication process, because a lot can get lost on other posts.

The patch answer

Our initial patch was proposing the creation of a new function in order to avoid code duplication and promote code readability. In a nutshell, our code change was actually proposing a function call that would just call 3 operations: the i2c xfer process, disabling i2c slave and checking i2c status, in order to return a positive value or raise an error.

What we failed to see in that patch is that those operations were always being called sequentially, and only on the file we have modified, so we did not need to create a new function to do so. Instead, we could only call the i2c slave disabling and status check inside the function that performs xfer. The mantainer said exactly that:

Jean Baptiste’s answer to our patch

The mantainer correctly identified our mistake and also gave us a suggestion on how to procceed with this patch. We also failed to understand another comment, that another Kernel mantainer is going to identify on a further response.

Our last modifications

The following code is huge, but it is pretty much what we have just explained. We attributed i2c slave disabling and status check on the master xfer function, in order to get a better readability on the code. The master xfer function follows:

static int inv_mpu_i2c_master_xfer(const struct inv_mpu6050_state *st)
{
    /* use 50hz frequency for xfer */
	const unsigned int freq = 50;
	const unsigned int period_ms = 1000 / freq;
	uint8_t d;
	unsigned int user_ctrl;
	int ret;
	unsigned int status;

	/* set sample rate */
	d = INV_MPU6050_FIFO_RATE_TO_DIVIDER(freq);
	ret = regmap_write(st->map, st->reg->sample_rate_div, d);
	if (ret)
		return ret;

	/* start i2c master */
	user_ctrl = st->chip_config.user_ctrl | INV_MPU6050_BIT_I2C_MST_EN;
	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
	if (ret)
		goto error_restore_rate;

	/* wait for xfer: 1 period + half-period margin */
	msleep(period_ms + period_ms / 2);

	/* stop i2c master */
	user_ctrl = st->chip_config.user_ctrl;
	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
	if (ret)
		goto error_stop_i2c;

	/* restore sample rate */
	d = st->chip_config.divider;
	ret = regmap_write(st->map, st->reg->sample_rate_div, d);
	if (ret)
		goto error_restore_rate;

	/* disable i2c slave */
	ret = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_CTRL(0), 0);
	if (ret)
		goto error_disable_i2c;

	/* check i2c status */
	ret = regmap_read(st->map, INV_MPU6050_REG_I2C_MST_STATUS, &status);
	if (ret)
		return ret;

	if (status & INV_MPU6050_BIT_I2C_SLV0_NACK)
		return -EIO;

	return 0;

error_stop_i2c:
	regmap_write(st->map, st->reg->user_ctrl, st->chip_config.user_ctrl);
error_restore_rate:
	regmap_write(st->map, st->reg->sample_rate_div, st->chip_config.divider);
error_disable_i2c:
	regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_CTRL(0), 0);
	return ret;
}

Response and communication

With that new patch sent and the error appointed corrected, it is time to see the new responses. We actually waited a little before sending again this new version of the contribution because of holidays. We got a message stating that Jean Baptiste’s office would not be working for a couple of days and during the weekend, so we postponed the e-mail. As a matter of fact, we also tried to confirm with the mantainer if we understood the patch, but got no response.

While time was running by, we decided to try and apply his suggestions on our own, taking a risk and running it by the TA’s, who also thought we were right on procceding the way we did. So we sent the new patch (V3, since we messed up on the V2 e-mail), and after some days, we got a response from Marcelo Schmitt, a Kernel mantainer who does not work exactly on this IIO driver, but is an USP alumnus, who we were sending copies from our patches for review. He is not really able to approve our patch, but sent this message:

Marcelo Schmitt’s answer to our patch

Based on his answer, we saw that we could have added a slave slot parameter on the function call, even though it would not make a huge difference. Also, another suggestion he made was a code styling one, on the variable declaration, but he just left it up for Baptiste to decide if another version was necessary.

After that, we spent some more days waiting for Baptiste’s answer on this patch, to see if we were able to continue, but got no response whatsoever. A weird fact is that another mantainer tagged our e-mail, to see if he would answer:

Jonathan Cameron’s tagging our patch

Now, there is nothing really we can do. We are just waiting for Baptiste’s new response for this contribution, but hopefully he is going to take a look at it soon and this patch will be reviewed.

Final updates

After all those responses and another e-mail from Jonathan Cameron asking for an approval on the patch, Jean-Baptiste finally came through and approved our patch!

Finally, Jonathan sent another e-mail, saying he added our patch to the testing branch and will be merged on the next update, so we achieved our initial goal of having a Kernel patch accepted :)

Patch merge e-mail

It was a really fun experience dealing with the Kernel contribution, although a hard one, but hopefully this won’t be our only contribution to this really important project, even though we are now focusing on other projects to set our efforts into. The whole involvement on this environment was really useful and helped us learn a lot during the process.