On Tue, Feb 25, 2020 at 11:22:33AM +0100, Wolfram Sang wrote: > 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. Motivation is simple: - Standardize the (small) set of mostly used bus frequences - Get rid of repetition of (subset) of above in many drivers - Reduce amount of potential typos Let's discuss it here. I don't think new version of this would be good to have without initial settlement. > > --- 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. I aware about that, but I would like to avoid I²C subsystem storming for another change like this. So, let's consider this as a trampoline when in the future we will switch entire subsystem to Linux wide header at once. We have already same/similar definitions in the other drivers and I really would like to avoid cross subsystem collisions. > > +/* 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 ;) No problem. I'm fine with either. > 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. Works for me. Btw, what about Vladimir's comment WRT STANDARD -> STD? My personal opinion that STD is a bit too short. > And finally, I'd think all driver patches should be squashed into one, > and all core ones into one etc. Or? I'm fine with either. For reviewers it would be better I think to see only their portion. Since I got a lot of tags already I consider I may squash it together. So, what do you prefer? -- With Best Regards, Andy Shevchenko
