[v1,01/40] i2c: qup: Move bus frequency definitions to i2c.h
diff mbox series

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
Related show

Commit Message

andriy.shevchenko@linux.intel.com Feb. 24, 2020, 3:14 p.m. UTC
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(-)

Comments

Wolfram Sang Feb. 25, 2020, 10:22 a.m. UTC | #1
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
andriy.shevchenko@linux.intel.com Feb. 25, 2020, 10:47 a.m. UTC | #2
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?
Wolfram Sang Feb. 25, 2020, 11:44 a.m. UTC | #3
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
andriy.shevchenko@linux.intel.com Feb. 25, 2020, 11:56 a.m. UTC | #4
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!

Patch
diff mbox series

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;