Re: [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 06/04/2018 06:38 PM, Benjamin Herrenschmidt wrote:
On Mon, 2018-06-04 at 22:21 +0300, Andy Shevchenko wrote:
+#define I2C_INT_ENABLE         0x0000ff80
+#define I2C_INT_ERR            0x0000fcc0
Now it looks like a flags combinations.
For me as for reader would be better to see quickly a decoded line.

My proposal is to introduce something like following

_INT_ALL  GENMASK()
_INT_ENABLE (_INT_ALL & ~(_FOO | _BAR))
_INT_ERR ... similar way as above ...

What do you think?
I don't think this absolutely needs to change but yes, open coding is
error prone. However I would think it more readable to use positive
logic and just list all the bits that are *set* even if it's a bit more
text:

#define I2C_INT_ERR	(I2C_INT_INV_CMD	|\                			
  I2C_INT_PARITY         |\
			 I2C_INT_BE_OVERRUN	|\
			.../...)

#define I2C_INT_ENABLE	(I2C_INT_ERR		|\
			 I2C_INT_DAT_REQ	|\
			 I2C_INT_CMD_COMP)

Note: Eddie, I notice I2C_INT_BUSY is in "ERR" but not in "ENABLE", any
reason for that ?

Yes, we don't want to enable an interrupt if I2C gets into the busy state, as that happens during every transfer.  However it would likely be an error condition if we get that when the transfer is supposed to be complete. These were from the legacy driver... I just realized that neither are actually being used in this driver, so I will drop them.

Thanks,
Eddie


Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux