Hi Andy, On Mon, Feb 24, 2020 at 05:14:51PM +0200, Andy Shevchenko wrote: > Move bus frequency definitions to i2c.h for wider use. > > Cc: Andy Gross <agross@xxxxxxxxxx> > Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> A cover letter would have been nice so we could discuss the general appraoch there. And to read more about the motivation. > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -39,6 +39,13 @@ enum i2c_slave_event; > typedef int (*i2c_slave_cb_t)(struct i2c_client *client, > enum i2c_slave_event event, u8 *val); > > +#define HZ_PER_KHZ 1000 Unlike Jarkko, I think such macros help readability when calculating frequencies within drivers. However, they shouldn't be local to I2C if we agree on them. They should be available Linux-wide. There are some other (few) local implementations already. > + > +/* I2C Frequency Modes */ > +#define I2C_STANDARD_MODE_FREQ (100 * HZ_PER_KHZ) > +#define I2C_FAST_MODE_FREQ (400 * HZ_PER_KHZ) > +#define I2C_FAST_MODE_PLUS_FREQ (1000 * HZ_PER_KHZ) For such a header, I'd prefer the plain number, though. There will be enough review to make sure we get it right ;) Furthermore, I'd prefer to have 'MAX' in there, e.g. I2C_MAX_STANDARD_MODE_FREQ etc. Just to make clear that I2C can have other bus speeds as well. And finally, I'd think all driver patches should be squashed into one, and all core ones into one etc. Or? Thanks, Wolfram
Attachment:
signature.asc
Description: PGP signature
