Re: [PATCH v1 01/40] i2c: qup: Move bus frequency definitions to i2c.h

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]