On 17.02.20 10:14:52, Luca Ceresoli wrote: > Hi, > > On 17/02/20 09:17, Wolfram Sang wrote: > > > >>> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > >> > >>> -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter, > >>> +struct i2c_client *i2c_install_smbus_alert(struct i2c_adapter *adapter, > >>> struct i2c_smbus_alert_setup *setup); > >> > >> This function naming is a bit odd. It creates a struct i2c_client. > >> Then, there is also i2c_new_client_device() and i2c_new_device(). For > >> i2c_new_client_device() there are no users at all outside of > >> i2c-core-base.c (except for Falcon NIC), it is only a wrapper. > > > > i2c_new_device (and friends) returned NULL on error. I am currently > > converting all i2c_new_* functions to return an ERRPTR. So, > > i2c_new_client_device is the new function, i2c_new_device is deprecated. > > If you check v5.6-rc1, you will find many more users. Similarily, > > i2c_new_dummy is deprecated (and removed already), i2c_new_dummy_device > > is the new thing. > > > >> So how about reducing the interface to those both only to:? > >> > >> i2c_new_device() > >> i2c_new_device_smbus() > > > > Given the above, it would be: > > > > i2c_new_client_device() > > i2c_new_smbus_device() > > > > Yet, I think this is too vague. Maybe > > > > i2c_new_smbus_alert_device() > > I always found the function naming a bit messy in the linux i2c > implementation. Among the names proposed in this thread, > i2c_new_smbus_alert_device() is the only one that makes sense to me for > the new function: it is not vague, and it is coherent with other names > of recently introduced functions (i2c_new_*_device()). Of course the > "alert device" is not a real device, but it looks like it is as it has > its own "slave" address. So I vote for this name... Yes, better fix the naming now that the i/f is new. As all these function create a i2c_client struct, the whole set of functions could be also named i2c_client_create*() with specialized functions such as i2c_client_create_smbus() or so. It will be clear it is a subset. Anyway, it's just a function name, but while reading the code it was not obvious to me that i2c_install_smbus_alert() is actually a subset of i2c_new_client_device(). That said, I like the i2c_client_create*() variants. -Robert
