Re: [PATCH 1/2] i2c: smbus: pass write disabling bit to spd instantiating function

[Andi Shyti]

Hi,

> @@ -44,9 +44,9 @@ static inline void i2c_free_slave_host_notify_device(struct i2c_client *client)
>  #endif
>  
>  #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
> -void i2c_register_spd(struct i2c_adapter *adap);
> +void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled);
>  #else
> -static inline void i2c_register_spd(struct i2c_adapter *adap) { }
> +static inline void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled) { }
>  #endif

Please don't add random bool flags to function parameters,
especially in library functions. They add confusion and make the
code harder to understand. I even dislike them when they're used
inside drivers.

I'd much prefer something like this:

   static void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
   {
   	...
   }
   
   void i2c_register_spd_write_disable(struct i2c_adapter *adap)
   {
   	i2c_register_spd(adap, true);
   }
   
   void i2c_register_spd_write_enable(struct i2c_adapter *adap)
   {
   	i2c_register_spd(adap, false);
   }

This way, we gain readability at the cost of a bit of redundancy,
a trade-off I'm more than happy with.

Of course, this is just my preference (even if I'm pretty strong
about it, especially when it comes to libraries). Wolfram is free
to override me on this.

Andi