On 04/11/2022 14:38, Andy Shevchenko wrote:
On Fri, Nov 04, 2022 at 01:59:06PM +0200, Tomi Valkeinen wrote:
On 01/11/2022 16:30, Andy Shevchenko wrote:
On Tue, Nov 01, 2022 at 03:20:26PM +0200, Tomi Valkeinen wrote:
...
+ ret = atr->ops->attach_client(atr, chan->chan_id, info, client,
+ &alias_id);
On one line looks better.
I agree, but it doesn't fit into 80 characters. I personally think that's a
too narrow a limit, but some maintainers absolutely require max 80 chars, so
I try to limit the lines to 80 unless it looks really ugly.
OK.
...
+ WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"),
+ "can't create symlink to atr device\n");
+ WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name),
+ "can't create symlink for channel %u\n", chan_id);
Why WARNs? sysfs has already some in their implementation.
True, and I can drop these if required. But afaics, sysfs_create_link only
warns if there's a duplicate entry, not for other errors.
The problem with WARN that it can be easily converted to real Oops. Do you
consider other errors are so fatal that machine would need a reboot?
Yes, WARNs are bad, especially as the error here is not critical. I'll
change these to dev_warn(). (also, I didn't know WARN could be made to
oops).
...
+ atr_size = struct_size(atr, adapter, max_adapters);
+ if (atr_size == SIZE_MAX)
+ return ERR_PTR(-EOVERFLOW);
Dunno if you really need this to be separated from devm_kzalloc(), either way
you will get an error, but in embedded case it will be -ENOMEM.
Yep. Well... I kind of like it to be explicit. Calling alloc(SIZE_MAX)
doesn't feel nice.
Yeah, but that is exactly the point of returning SIZE_MAX by the helpers from
overflow.h. And many of them are called inside a few k*alloc*() APIs.
So, I don't think it's ugly or not nice from that perspective.
Ok, sounds fine to me. I'll drop the check.
+ atr = devm_kzalloc(dev, atr_size, GFP_KERNEL);
+ if (!atr)
+ return ERR_PTR(-ENOMEM);
...
+EXPORT_SYMBOL_GPL(i2c_atr_delete);
I would put these to their own namespace from day 1.
What would be the namespace? Isn't this something that should be
subsystem-wide decision? I have to admit I have never used symbol
namespaces, and don't know much about them.
Yes, subsystem is I2C, but you introducing a kinda subsubsystem. Wouldn't be
better to provide all symbols in the I2C_ATR namespace from now on?
It really helps not polluting global namespace and also helps to identify
users in the source tree.
Alright, I'll look into this.
...
+struct i2c_atr {
+ /* private: internal use only */
+
+ struct i2c_adapter *parent;
+ struct device *dev;
+ const struct i2c_atr_ops *ops;
+
+ void *priv;
+
+ struct i2c_algorithm algo;
+ struct mutex lock;
+ int max_adapters;
+
+ struct i2c_adapter *adapter[0];
No VLAs.
Ok.
I'm not arguing against any of the comments you've made, I think they are
all valid, but I want to point out that many of them are in a code copied
from i2c-mux.
Whether there's any value in keeping i2c-mux and i2c-atr similar in
design/style... Maybe not.
You can address my comment by simply dropping 0 in the respective member.
Oh, I thought you meant no "extensible" structs. I'll drop the 0.
Tomi
