diff mbox series

[v6,2/5] i2c: core: add api to provide frequency mode strings

Message ID 1617197790-30627-3-git-send-email-yangyicong@hisilicon.com
State Superseded
Headers show
Series Add support for HiSilicon I2C controller | expand

Commit Message

Yicong Yang March 31, 2021, 1:36 p.m. UTC 
Some I2C drivers like Designware and HiSilicon will print the
bus frequency mode information, so add a public one that everyone
can make use of.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 include/linux/i2c.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Wolfram Sang April 6, 2021, 7:54 p.m. UTC | #1 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 10bd0b0..7268180 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>  #define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
>  #define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
>  
> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
> +{
> +	switch (bus_freq_hz) {
> +	case I2C_MAX_STANDARD_MODE_FREQ:
> +		return "Standard Mode (100 kHz)";
> +	case I2C_MAX_FAST_MODE_FREQ:
> +		return "Fast Mode (400 kHz)";
> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> +		return "Fast Mode Plus (1.0 MHz)";
> +	case I2C_MAX_TURBO_MODE_FREQ:
> +		return "Turbo Mode (1.4 MHz)";
> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
> +		return "High Speed Mode (3.4 MHz)";
> +	case I2C_MAX_ULTRA_FAST_MODE_FREQ:
> +		return "Ultra Fast Mode (5.0 MHz)";
> +	default:
> +		return "Unknown Mode";
> +	}
> +}

Any reason ehy this is an inline function? My gut feeling says it would
be better added to the core?
Yicong Yang April 7, 2021, 8:29 a.m. UTC | #2 
On 2021/4/7 3:54, Wolfram Sang wrote:
> 
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 10bd0b0..7268180 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>>  #define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
>>  #define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
>>  
>> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
>> +{
>> +	switch (bus_freq_hz) {
>> +	case I2C_MAX_STANDARD_MODE_FREQ:
>> +		return "Standard Mode (100 kHz)";
>> +	case I2C_MAX_FAST_MODE_FREQ:
>> +		return "Fast Mode (400 kHz)";
>> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
>> +		return "Fast Mode Plus (1.0 MHz)";
>> +	case I2C_MAX_TURBO_MODE_FREQ:
>> +		return "Turbo Mode (1.4 MHz)";
>> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
>> +		return "High Speed Mode (3.4 MHz)";
>> +	case I2C_MAX_ULTRA_FAST_MODE_FREQ:
>> +		return "Ultra Fast Mode (5.0 MHz)";
>> +	default:
>> +		return "Unknown Mode";
>> +	}
>> +}
> 
> Any reason ehy this is an inline function? My gut feeling says it would
> be better added to the core?
> 

it's not a complicated function so i didn't think it'll make much difference,
so i just put it in the header along with the coresponding macro definitions.
do you want me to move it to the core?

Thanks
Andy Shevchenko April 7, 2021, 10:08 a.m. UTC | #3 
On Wed, Apr 07, 2021 at 04:29:29PM +0800, Yicong Yang wrote:
> On 2021/4/7 3:54, Wolfram Sang wrote:
> > 
> >> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> >> index 10bd0b0..7268180 100644
> >> --- a/include/linux/i2c.h
> >> +++ b/include/linux/i2c.h
> >> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
> >>  #define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
> >>  #define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
> >>  
> >> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
> >> +{
> >> +	switch (bus_freq_hz) {
> >> +	case I2C_MAX_STANDARD_MODE_FREQ:
> >> +		return "Standard Mode (100 kHz)";
> >> +	case I2C_MAX_FAST_MODE_FREQ:
> >> +		return "Fast Mode (400 kHz)";
> >> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> >> +		return "Fast Mode Plus (1.0 MHz)";
> >> +	case I2C_MAX_TURBO_MODE_FREQ:
> >> +		return "Turbo Mode (1.4 MHz)";
> >> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
> >> +		return "High Speed Mode (3.4 MHz)";
> >> +	case I2C_MAX_ULTRA_FAST_MODE_FREQ:
> >> +		return "Ultra Fast Mode (5.0 MHz)";
> >> +	default:
> >> +		return "Unknown Mode";
> >> +	}
> >> +}
> > 
> > Any reason ehy this is an inline function? My gut feeling says it would
> > be better added to the core?
> > 
> 
> it's not a complicated function so i didn't think it'll make much difference,
> so i just put it in the header along with the coresponding macro definitions.
> do you want me to move it to the core?

I guess exporting will save few dozens of bytes if the function is used more
than once. (All strings will be duplicated or multiplied in that case)
Yicong Yang April 7, 2021, 10:24 a.m. UTC | #4 
On 2021/4/7 18:08, Andy Shevchenko wrote:
> On Wed, Apr 07, 2021 at 04:29:29PM +0800, Yicong Yang wrote:
>> On 2021/4/7 3:54, Wolfram Sang wrote:
>>>
>>>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>>>> index 10bd0b0..7268180 100644
>>>> --- a/include/linux/i2c.h
>>>> +++ b/include/linux/i2c.h
>>>> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>>>>  #define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
>>>>  #define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
>>>>  
>>>> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
>>>> +{
>>>> +	switch (bus_freq_hz) {
>>>> +	case I2C_MAX_STANDARD_MODE_FREQ:
>>>> +		return "Standard Mode (100 kHz)";
>>>> +	case I2C_MAX_FAST_MODE_FREQ:
>>>> +		return "Fast Mode (400 kHz)";
>>>> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
>>>> +		return "Fast Mode Plus (1.0 MHz)";
>>>> +	case I2C_MAX_TURBO_MODE_FREQ:
>>>> +		return "Turbo Mode (1.4 MHz)";
>>>> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
>>>> +		return "High Speed Mode (3.4 MHz)";
>>>> +	case I2C_MAX_ULTRA_FAST_MODE_FREQ:
>>>> +		return "Ultra Fast Mode (5.0 MHz)";
>>>> +	default:
>>>> +		return "Unknown Mode";
>>>> +	}
>>>> +}
>>>
>>> Any reason ehy this is an inline function? My gut feeling says it would
>>> be better added to the core?
>>>
>>
>> it's not a complicated function so i didn't think it'll make much difference,
>> so i just put it in the header along with the coresponding macro definitions.
>> do you want me to move it to the core?
> 
> I guess exporting will save few dozens of bytes if the function is used more
> than once. (All strings will be duplicated or multiplied in that case)
> 

yes, that's one concern. since we don't need this to perform fast, an inline
one maybe unnecessary.
Wolfram Sang April 7, 2021, 10:56 p.m. UTC | #5 
> > I guess exporting will save few dozens of bytes if the function is used more
> > than once. (All strings will be duplicated or multiplied in that case)
> > 
> 
> yes, that's one concern. since we don't need this to perform fast, an inline
> one maybe unnecessary.

Exactly. I also don't see an advantage of the function being inline. But
potential disadvantage, even if just small memory overhead. So, I'd
still rather see it as a core function.
diff mbox series

Patch

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 10bd0b0..7268180 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -47,6 +47,26 @@  typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
 #define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
 #define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
 
+static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
+{
+	switch (bus_freq_hz) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		return "Standard Mode (100 kHz)";
+	case I2C_MAX_FAST_MODE_FREQ:
+		return "Fast Mode (400 kHz)";
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		return "Fast Mode Plus (1.0 MHz)";
+	case I2C_MAX_TURBO_MODE_FREQ:
+		return "Turbo Mode (1.4 MHz)";
+	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+		return "High Speed Mode (3.4 MHz)";
+	case I2C_MAX_ULTRA_FAST_MODE_FREQ:
+		return "Ultra Fast Mode (5.0 MHz)";
+	default:
+		return "Unknown Mode";
+	}
+}
+
 struct module;
 struct property_entry;