On 8/18/2018 4:06 AM, Himanshu Jha wrote:
> On Fri, Aug 17, 2018 at 12:03:14PM -0700, David Frey wrote:
>> Signed-off-by: David Frey <dpfrey@xxxxxxxxx>
>
> One minor comment below.
<snip>
>> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
>> index 35cbcb16c9f9..cde08d57e7d5 100644
>> --- a/drivers/iio/chemical/bme680_core.c
>> +++ b/drivers/iio/chemical/bme680_core.c
<snip>
>> @@ -208,30 +200,26 @@ static int bme680_read_calib(struct bme680_data *data,
>> dev_err(dev, "failed to read BME680_H1_MSB_REG\n");
>> return ret;
>> }
>> -
>> ret = regmap_read(data->regmap, BME680_H1_LSB_REG, &tmp_lsb);
>> if (ret < 0) {
>> dev_err(dev, "failed to read BME680_H1_LSB_REG\n");
>> return ret;
>> }
>> -
>> calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
>> - (tmp_lsb & BME680_BIT_H1_DATA_MSK);
>> + (tmp_lsb & BME680_BIT_H1_DATA_MSK);
>
> Prefer tabs instead of spaces!
> Please run checkpatch on the patch to get those warnings.
Thanks for telling me about the existence of checkpatch.pl. The
official docs
(https://www.kernel.org/doc/html/v4.18/process/coding-style.html) are
pretty light on information about indentation/alignment, but there is
this relevant quote: "Outside of comments, documentation and except in
Kconfig, spaces are never used for indentation, and the above example is
deliberately broken."
I thought that the way I indented it was optimal because I used spaces a
tab for indentation and then spaces for alignment, but checkpatch.pl
seems to disagree. Based on a quick read of the code, it seems that it
will complain about more than 7 spaces in a row.
I guess you could achieve visually identical alignment (where "------->"
is tab and "_" is space) and this would still pass checkpatch.pl. Note
that I changed the identifier name slightly to demonstrate a mix of tabs
and spaces.
------->calib->par_h123 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->------->__(tmp_lsb & BME680_BIT_H1_DATA_MSK);
What I don't understand is the logic used to justify the original
indentation:
------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);
Why not?
------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);
Or?
------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);
I'm not trying to turn this into a crazy bike shedding thread. I just
want to understand the rules a bit better.
Thanks,
David
