[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c0f0a5a3-c4bd-97e2-2047-da33bd896310@ideasonboard.com>
Date: Fri, 4 Nov 2022 17:26:24 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: devicetree@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Jacopo Mondi <jacopo@...ndi.org>,
Kieran Bingham <kieran.bingham@...asonboard.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Luca Ceresoli <luca@...aceresoli.net>,
Mark Rutland <mark.rutland@....com>,
Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Peter Rosin <peda@...ntia.se>,
Rob Herring <robh+dt@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Vladimir Zapolskiy <vz@...ia.com>,
Wolfram Sang <wsa@...-dreams.de>,
satish.nagireddy@...cruise.com
Subject: Re: [PATCH v4 2/8] i2c: add I2C Address Translator (ATR) support
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
Powered by blists - more mailing lists