Re: [PATCH v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

[Oleksandr Kozaruk]

Hello Lars,

On Wed, Jul 17, 2013 at 9:04 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
>> +static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
>> +{
>> +     int chn, d1 = 0, d2 = 0, temp;
>> +     u8 trim_regs[17];
>> +     int ret;
>> +
>> +     ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
>> +                     TWL6030_GPADC_TRIM1, 16);
>> +     if (ret < 0) {
>> +             dev_err(gpadc->dev, "calibration failed\n");
>> +             return ret;
>> +     }
>> +
>> +     /*
>> +      * Loop to calculate the value needed for returning voltages from
>> +      * GPADC not values.
>> +      *
>> +      * gain is calculated to 3 decimal places fixed point.
>> +      */
>> +     for (chn = 0; chn < TWL6032_GPADC_MAX_CHANNELS; chn++) {
>> +
>> +             switch (chn) {
>> +             case 0:
>> +             case 1:
>> +             case 2:
>> +             case 3:
>> +             case 4:
>> +             case 5:
>> +             case 6:
>> +             case 11:
>> +             case 12:
>> +             case 13:
>> +             case 14:
>> +                     /* D1 */
>> +                     d1 = (trim_regs[3] & 0x1F) << 2;
>> +                     d1 |= (trim_regs[1] & 0x06) >> 1;
>> +                     if (trim_regs[1] & 0x01)
>> +                             d1 = -d1;
>> +
>> +                     /* D2 */
>> +                     d2 = (trim_regs[4] & 0x3F) << 2;
>> +                     d2 |= (trim_regs[2] & 0x06) >> 1;
>> +                     if (trim_regs[2] & 0x01)
>> +                             d2 = -d2;
>> +                     break;
>> +             case 8:
>> +                     /* D1 */
>> +                     temp = (trim_regs[3] & 0x1F) << 2;
>> +                     temp |= (trim_regs[1] & 0x06) >> 1;
>> +                     if (trim_regs[1] & 0x01)
>> +                             temp = -temp;
>> +
>> +                     d1 = (trim_regs[8] & 0x18) << 1;
>> +                     d1 |= (trim_regs[7] & 0x1E) >> 1;
>> +                     if (trim_regs[7] & 0x01)
>> +                             d1 = -d1;
>> +
>> +                     d1 += temp;
>> +
>> +                     /* D2 */
>> +                     temp = (trim_regs[4] & 0x3F) << 2;
>> +                     temp |= (trim_regs[2] & 0x06) >> 1;
>> +                     if (trim_regs[2] & 0x01)
>> +                             temp = -temp;
>> +
>> +                     d2 = (trim_regs[10] & 0x1F) << 2;
>> +                     d2 |= (trim_regs[8] & 0x06) >> 1;
>> +                     if (trim_regs[8] & 0x01)
>> +                             d2 = -d2;
>> +
>> +                     d2 += temp;
>> +                     break;
>> +             case 9:
>> +                     /* D1 */
>> +                     temp = (trim_regs[3] & 0x1F) << 2;
>> +                     temp |= (trim_regs[1] & 0x06) >> 1;
>> +                     if (trim_regs[1] & 0x01)
>> +                             temp = -temp;
>> +
>> +                     d1 = (trim_regs[14] & 0x18) << 1;
>> +                     d1 |= (trim_regs[12] & 0x1E) >> 1;
>> +                     if (trim_regs[12] & 0x01)
>> +                             d1 = -d1;
>> +
>> +                     d1 += temp;
>> +
>> +                     /* D2 */
>> +                     temp = (trim_regs[4] & 0x3F) << 2;
>> +                     temp |= (trim_regs[2] & 0x06) >> 1;
>> +                     if (trim_regs[2] & 0x01)
>> +                             temp = -temp;
>> +
>> +                     d2 = (trim_regs[16] & 0x1F) << 2;
>> +                     d2 |= (trim_regs[14] & 0x06) >> 1;
>> +                     if (trim_regs[14] & 0x01)
>> +                             d2 = -d2;
>> +
>> +                     d2 += temp;
>> +             case 10:
>> +                     /* D1 */
>> +                     d1 = (trim_regs[11] & 0x0F) << 3;
>> +                     d1 |= (trim_regs[9] & 0x0E) >> 1;
>> +                     if (trim_regs[9] & 0x01)
>> +                             d1 = -d1;
>> +
>> +                     /* D2 */
>> +                     d2 = (trim_regs[15] & 0x0F) << 3;
>> +                     d2 |= (trim_regs[13] & 0x0E) >> 1;
>> +                     if (trim_regs[13] & 0x01)
>> +                             d2 = -d2;
>> +                     break;
>> +             case 7:
>> +             case 18:
>> +                     /* D1 */
>> +                     temp = (trim_regs[3] & 0x1F) << 2;
>> +                     temp |= (trim_regs[1] & 0x06) >> 1;
>> +                     if (trim_regs[1] & 0x01)
>> +                             temp = -temp;
>> +
>> +                     d1 = (trim_regs[5] & 0x7E) >> 1;
>> +                     if (trim_regs[5] & 0x01)
>> +                             d1 = -d1;
>> +.
>> +                     d1 += temp;
>> +
>> +                     /* D2 */
>> +                     temp = (trim_regs[4] & 0x3F) << 2;
>> +                     temp |= (trim_regs[2] & 0x06) >> 1;
>> +                     if (trim_regs[2] & 0x01)
>> +                             temp = -temp;
>> +
>> +                     d2 = (trim_regs[6] & 0xFE) >> 1;
>> +                     if (trim_regs[6] & 0x01)
>> +                             d2 = -d2;
>> +
>> +                     d2 += temp;
>> +                     break;
>
>
> There is quite a bit of copy paste in here. Putting the bit swizziling into
> a helper function should also improbe legibility.

Trimming bits are scattered randomly between registers and in
different bit positions.
I don't see any commonalities, that could be generalized. as Graeme
said about it "it's a bane of my life".

Regards,
OK.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/