| Message ID | 20200224151530.31713-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,01/40] i2c: qup: Move bus frequency definitions to i2c.h | expand |
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@kernel.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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
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@kernel.org> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > 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?
Hi Andy, > 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. Well, for me, this works. I agree to the typo thing and having less magic values. It's all OK; I think it is just nice to have some things in a coverletter. > 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. I can agree to that. > > 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. Thanks, that's the most important point to me. > Btw, what about Vladimir's comment WRT STANDARD -> STD? My personal opinion > that STD is a bit too short. No real opinion here. I think STD is understandable enough and I encounter it regularly. However, I also don't think the saving is huge enough to matter. Your call here. > 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? Sounds good to me. Keep collecting acks and squash all patches and tags in v2. Thanks, Wolfram
On Tue, Feb 25, 2020 at 12:44:01PM +0100, Wolfram Sang wrote: > > 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. > > Well, for me, this works. I agree to the typo thing and having less > magic values. It's all OK; I think it is just nice to have some things > in a coverletter. Since it looks like we will have only few patches at the end, I think the commit message for the first one would be good enough to describe this all. > > 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. > > I can agree to that. > > > > 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. > > Thanks, that's the most important point to me. > > > Btw, what about Vladimir's comment WRT STANDARD -> STD? My personal opinion > > that STD is a bit too short. > > No real opinion here. I think STD is understandable enough and I > encounter it regularly. However, I also don't think the saving is huge > enough to matter. Your call here. I prefer STANDARD over STD due to consistency (we don't have good abbreviations for Fast, Fast+, High Speed, etc, anyway). > > 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? > > Sounds good to me. Keep collecting acks and squash all patches and tags > in v2. Good, thanks for review!
diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c index 2d7dabe12723..b9d48ace9ff2 100644 --- a/drivers/i2c/busses/i2c-qup.c +++ b/drivers/i2c/busses/i2c-qup.c @@ -136,13 +136,8 @@ */ #define TOUT_MIN 2 -/* I2C Frequency Modes */ -#define I2C_STANDARD_FREQ 100000 -#define I2C_FAST_MODE_FREQ 400000 -#define I2C_FAST_MODE_PLUS_FREQ 1000000 - /* Default values. Use these if FW query fails */ -#define DEFAULT_CLK_FREQ I2C_STANDARD_FREQ +#define DEFAULT_CLK_FREQ I2C_STANDARD_MODE_FREQ #define DEFAULT_SRC_CLK 20000000 /* @@ -1861,7 +1856,7 @@ static int qup_i2c_probe(struct platform_device *pdev) qup->in_fifo_sz = qup->in_blk_sz * (2 << size); hs_div = 3; - if (clk_freq <= I2C_STANDARD_FREQ) { + if (clk_freq <= I2C_STANDARD_MODE_FREQ) { fs_div = ((src_clk_freq / clk_freq) / 2) - 3; qup->clk_ctl = (hs_div << 8) | (fs_div & 0xff); } else { diff --git a/include/linux/i2c.h b/include/linux/i2c.h index f834687989f7..708ac1262a0c 100644 --- 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 + +/* 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) + struct module; struct property_entry;
Move bus frequency definitions to i2c.h for wider use. Cc: Andy Gross <agross@kernel.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/busses/i2c-qup.c | 9 ++------- include/linux/i2c.h | 7 +++++++ 2 files changed, 9 insertions(+), 7 deletions(-)