| Message ID | 20221101132032.1542416-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| Headers | show |
| Series | i2c-atr and FPDLink | expand |
On Tue, Nov 01, 2022 at 03:20:28PM +0200, Tomi Valkeinen wrote: > Add DT bindings for TI DS90UB913 FPDLink-3 Serializer. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../bindings/media/i2c/ti,ds90ub913.yaml | 127 ++++++++++++++++++ > 1 file changed, 127 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml Similar comments for this one. Rob
Hi Andy, On 01/11/2022 16:30, Andy Shevchenko wrote: > On Tue, Nov 01, 2022 at 03:20:26PM +0200, Tomi Valkeinen wrote: >> From: Luca Ceresoli <luca@lucaceresoli.net> >> >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. > > ... > >> i2c-topology >> muxes/i2c-mux-gpio >> i2c-sysfs >> + muxes/i2c-atr > > Doesn't make sense to group muxes/*, that they are following each other? Ok. > ... > >> +I2C ADDRESS TRANSLATOR (ATR) >> +M: Luca Ceresoli <luca@lucaceresoli.net> > > Hmm... Are you going to maintain this? Or Review? Why not? We haven't discussed with Luca if he wants to maintain this (this is mostly his code). But, indeed, I should add my name there. >> +L: linux-i2c@vger.kernel.org >> +S: Maintained >> +F: drivers/i2c/i2c-atr.c >> +F: include/linux/i2c-atr.h > > ... > >> + void *new_buf = kmalloc_array(num, sizeof(chan->orig_addrs[0]), >> + GFP_KERNEL); >> + if (new_buf == NULL) >> + return -ENOMEM; > > Isn't it better to write this as > > void *new_buf; > > new_buf = kmalloc_array(num, sizeof(chan->orig_addrs[0]), GFP_KERNEL); > if (!new_buf) > return -ENOMEM; Ok. > Remarks: > - note the style of the conditional > - why is it void? No idea. I'll change it. > > Also, does it make sense to use krealloc_array() or is it complete replacement > of the data? The whole array will be rewritten, so we don't need to preserve the current data. The buffer allocated here (i.e. orig_addrs) is only used for the duration of the i2c_atr_master_xfer(). So, we could allocate a new buffer for every xfer call, but to avoid that, we retain the old buffer. Any old data in the buffer can be discarded. >> + kfree(chan->orig_addrs); >> + chan->orig_addrs = new_buf; >> + chan->orig_addrs_size = num; > > ... > >> +static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, struct i2c_msg msgs[], >> + int num) > > [] in the function parameter is longer than * and actually doesn't make > difference in C. Ditto for the rest of similar cases. Ok. I missed a few, it seems. > ... > >> +static int i2c_atr_smbus_xfer(struct i2c_adapter *adap, u16 addr, >> + unsigned short flags, char read_write, u8 command, >> + int size, union i2c_smbus_data *data) > > Can flags be fixed size (yes I understand that in our case short would probably > never be different to u16, but for the sake of clearness)? The parameters and their types come from the ops in struct i2c_algorithm. > ... > >> +static int i2c_atr_attach_client(struct i2c_adapter *adapter, >> + const struct i2c_board_info *info, >> + const struct i2c_client *client) >> +{ >> + struct i2c_atr_chan *chan = adapter->algo_data; >> + struct i2c_atr *atr = chan->atr; >> + struct i2c_atr_cli2alias_pair *c2a; > >> + u16 alias_id = 0; > > Can we split assignment from the definition and locate it closer to the first > use? Actually, I don't think we need to initialize it at all. If attach_client() fails, we don't care about alias_id. If attach_client() succeeds, it _must_ return alias_id. >> + int ret = 0; > > Useless assignment. Yep. >> + >> + c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); >> + if (!c2a) >> + return -ENOMEM; >> + >> + 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. >> + if (ret) >> + goto err_free; >> + if (alias_id == 0) { >> + ret = -EINVAL; >> + goto err_free; >> + } >> + >> + c2a->client = client; >> + c2a->alias = alias_id; >> + list_add(&c2a->node, &chan->alias_list); >> + >> + return 0; >> + >> +err_free: >> + kfree(c2a); >> + return ret; >> +} > > ... > >> +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id, >> + struct fwnode_handle *bus_handle) >> +{ >> + struct i2c_adapter *parent = atr->parent; >> + struct device *dev = atr->dev; >> + struct i2c_atr_chan *chan; >> + char *symlink_name; >> + int ret; >> + >> + if (chan_id >= atr->max_adapters) { >> + dev_err(dev, "No room for more i2c-atr adapters\n"); >> + return -EINVAL; >> + } >> + >> + if (atr->adapter[chan_id]) { >> + dev_err(dev, "Adapter %d already present\n", chan_id); >> + return -EEXIST; >> + } >> + >> + chan = kzalloc(sizeof(*chan), GFP_KERNEL); >> + if (!chan) >> + return -ENOMEM; >> + >> + chan->atr = atr; >> + chan->chan_id = chan_id; >> + INIT_LIST_HEAD(&chan->alias_list); >> + mutex_init(&chan->orig_addrs_lock); >> + >> + snprintf(chan->adap.name, sizeof(chan->adap.name), "i2c-%d-atr-%d", >> + i2c_adapter_id(parent), chan_id); >> + chan->adap.owner = THIS_MODULE; >> + chan->adap.algo = &atr->algo; >> + chan->adap.algo_data = chan; >> + chan->adap.dev.parent = dev; >> + chan->adap.retries = parent->retries; >> + chan->adap.timeout = parent->timeout; >> + chan->adap.quirks = parent->quirks; >> + chan->adap.lock_ops = &i2c_atr_lock_ops; >> + chan->adap.attach_ops = &i2c_atr_attach_ops; >> + >> + if (bus_handle) { >> + device_set_node(&chan->adap.dev, fwnode_handle_get(bus_handle)); >> + } else { >> + struct fwnode_handle *atr_node; >> + struct fwnode_handle *child; >> + u32 reg; >> + >> + atr_node = device_get_named_child_node(dev, "i2c-atr"); >> + >> + fwnode_for_each_child_node(atr_node, child) { >> + ret = fwnode_property_read_u32(child, "reg", ®); >> + if (ret) >> + continue; >> + if (chan_id == reg) >> + break; >> + } >> + >> + device_set_node(&chan->adap.dev, child); >> + fwnode_handle_put(atr_node); >> + } > > It seems you have OF independent code, but by some reason you included of.h > instead of property.h. Am I right? Just an leftover from the conversion from of to fwnode. >> + ret = i2c_add_adapter(&chan->adap); >> + if (ret) { >> + dev_err(dev, "failed to add atr-adapter %u (error=%d)\n", >> + chan_id, ret); >> + goto err_add_adapter; >> + } >> + >> + symlink_name = kasprintf(GFP_KERNEL, "channel-%u", chan_id); > > No NULL check? Right, missed that. >> + 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. >> + >> + kfree(symlink_name); >> + >> + dev_dbg(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap)); >> + >> + atr->adapter[chan_id] = &chan->adap; >> + return 0; >> + >> +err_add_adapter: >> + mutex_destroy(&chan->orig_addrs_lock); >> + kfree(chan); >> + return ret; >> +} > > ... > >> + struct fwnode_handle *fwnode = adap->dev.fwnode; > > Please don't dereference fwnode like this, we have dev_fwnode() for that. Ok. > ... > >> + if (atr->adapter[chan_id] == NULL) { > > ! Yep. >> + dev_err(dev, "Adapter %d does not exist\n", chan_id); >> + return; >> + } > > ... > >> + snprintf(symlink_name, sizeof(symlink_name), >> + "channel-%u", chan->chan_id); > > Once line? 80 char limit here too. But I see that this is (kind of) broken. In the i2c_atr_add_adapter() I used dynamic alloc for the symlink_name, but here we still have the fixed size buffer. > > ... > >> + 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. >> + 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. > > ... > >> +/** >> + * Helper to add I2C ATR features to a device driver. >> + */ > > ??? Copy'n'paste typo? No idea where that is from... I'll fix it. >> +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. >> +}; > > ... > >> +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id, >> + struct fwnode_handle *bus_np); > > Missing > > struct fwnode_handle; > > at the top of the file? Ok. Tomi
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? ... > > > + 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. > > > + 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. ... > > > +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. > > > +};
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
On Fri, Nov 04, 2022 at 05:26:24PM +0200, Tomi Valkeinen wrote: > 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: ... > > > > > + 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). panic_on_warn
On Fri, 4 Nov 2022 13:59:06 +0200 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > Hi Andy, > > On 01/11/2022 16:30, Andy Shevchenko wrote: > > On Tue, Nov 01, 2022 at 03:20:26PM +0200, Tomi Valkeinen wrote: > >> From: Luca Ceresoli <luca@lucaceresoli.net> > >> > >> An ATR is a device that looks similar to an i2c-mux: it has an I2C > >> slave "upstream" port and N master "downstream" ports, and forwards > >> transactions from upstream to the appropriate downstream port. But is > >> is different in that the forwarded transaction has a different slave > >> address. The address used on the upstream bus is called the "alias" > >> and is (potentially) different from the physical slave address of the > >> downstream chip. > >> > >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow > >> implementing ATR features in a device driver. The helper takes care or > >> adapter creation/destruction and translates addresses at each transaction. First of all, thank you for bringing this work on! > > ... > > > >> +I2C ADDRESS TRANSLATOR (ATR) > >> +M: Luca Ceresoli <luca@lucaceresoli.net> > > > > Hmm... Are you going to maintain this? Or Review? Why not? > > We haven't discussed with Luca if he wants to maintain this (this is > mostly his code). But, indeed, I should add my name there. I think at this point you are probably in a better position to be the maintainer, but I'm OK with being listed here as reviewer (R:). Ah, would you please use my bootlin dot com address here? > 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. Tomi is right, when I wrote this initially I tried to keep it as similar as possible to i2c-mux.c. No problem in deviating from that wherever it makes sense.
Hi Tomi, All, On Tue, 1 Nov 2022 15:20:24 +0200 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > Hi, > > Intro > ----- > > This is, kind of, v4 of Luca's i2c-atr and FPDLink series, v3 of which > you can find from: > > https://lore.kernel.org/all/20220206115939.3091265-1-luca@lucaceresoli.net/ > > I say "kind of", as the FPDLink drivers have diverged from Luca's > version quite a bit and the drivers support different HW versions. A > Big thanks for Luca for working on the drivers! > > I'd really like to send and review i2c-atr and FPDLink drivers > separately, but as the concepts are new and those drivers are linked > together, in the end I decided to keep them in one series. Even so, I > think these patches divide quite clearly into to areas: > > 1) i2c-atr and the related code in the FPDLink drivers > 2) Everything else about FPDLink > > The FPDLink drivers support multiple streams and for that reason the > series is based on V4L2 streams series v15: > > https://lore.kernel.org/all/20221003121852.616745-1-tomi.valkeinen@ideasonboard.com/ > > HW overview > ----------- > > TI's DS90UB9xx IC (UB9xx for short) family is a set of deserializer and > serializer ICs for video, both display and capture. These ICs support > FPDLink 3 and some also support FPDLink 4. From the user's point of view > there's really not much difference between FPDLink 3 and 4. > > An example HW setup with two cameras could be like this: > > +-- [Ser 1] <-- [Camera 1] > [CSI-2 RX] <-- [Deser] <--| > +-- [Ser 2] <-- [Camera 2] > > The cameras send video streams over CSI-2 to the serializers. The > serializers encode the received data to FPDLink and send it to the > deserializer. The deserializer decodes the data and sends it forward > over CSI-2 bus. > > The FPDLink bus is a bi-directional bus, with a fast forward-channel and > a slow back-channel. In addition to the video, the devices support > forwarding GPIOs (both ways) and forwarding i2c transactions with > address translation. > > This series is only about the capture ICs. The HW versions supported > by these drivers are: > > - UB960, FPDLink 3 deserializer > - UB9702, FPDLink 3/4 deserializer > - UB953, FPDLink 3 CSI-2 serializer > - UB913, FPDLink 3 parallel video serializer > - UB971, FPDLink 3/4 CSI-2 serializer > > Note that this series does not support UB954 deserializer, which Luca > has. I don't have it and decided early to drop it for simpler > development. As UB954 is a subset of UB960, adding support for it should > be trivial. > > FPDLink Deser and Ser Devices & Drivers > --------------------------------------- > > The serializer devices are, in a way, child devices of the deserializer. > It is the deserializer driver which creates and removes the serializer > devices, although the serializer device itself is not a child of the > deserializer, but rather the serializer is created as a device on the > i2c bus. > > There are perhaps other methods to add the serializer devices. I think a > real bus is an overkill, as FPDLink is a point-to-point link. But if > there's no FPDLink bus, what are the options? Platform device? That's > not good either. So, for the time being, the serializers are i2c clients > on the main i2c bus, even if they're really behind the FPDLink. > > Also, the deserializer driver needs to share some information with the > serializers, and it is done with platform data set to the serializer > device. If we had a bus, the bus could probably be used to convey this > information. > > I2C & I2C ATR > ------------- > > We have three different i2c client types: > > 1) The deserializer is a normal i2c slave, and there's nothing special > about accessing it. The deserializer is connected to the "main" i2c bus. > > 2) The serializers are also accessible with i2c via FPDLink: when a > transaction to a specific (programmable) address happens on the main > i2c-bus the deserializer takes the transaction and forwards it via > FPDLink to the serializer. This is implemented in the deserializer > driver by just creating the serializer i2c device on the main i2c-bus. > This is not 100% correct, as the serializers are not directly connected > to an i2c bus as i2c slaves. > > 3) The serializer is an i2c master, and the camera and possibly other > i2c peripherals can be connected to that "remote" i2c bus. The > deserializer hardware has an i2c-alias table which describes which > i2c-alias address the deserializer will react to, what is the real > i2c-address and to which serializer the transactions will be forwarded > to. This is called address translation (ATR). An excellent description so far! > I2C ATR Thoughts > ---------------- > > I have addressed most of the comments Luca received for the i2c-atr > driver, but other than that the i2c-atr driver is mostly the same. > However, there's one big difference: the i2c bus can be given by the > drivers. > > So, looking at the DT, in Luca's version the deserializer node looked > like this for serializers and i2c-atr: > > deser { > remote-chips { > remote-chip@0 { > // The serializer > }; > }; > > i2c-atr { > i2c@0 { > // This is the bus behind the serializer > }; > }; > }; > > I have: > > deser { > links { > link@0 { > serializer { > // The serializer > i2c { > // This is the bus > }; > } > }; > }; > }; > > I think this reflects the hardware much better. But this means that the > i2c-atr "device", which in Luca's version is only the deserializer, is > in my version split between the deser and serializer. However, the only > thing I changed in the i2c-atr driver is that the i2c_atr_add_adapter() > function takes an fwnode handle which tells where the i2c bus is found > (but if it's NULL, it looks for i2c-atr just like in Luca's version). > > Perhaps the biggest difference is that in Luca's version the i2c-atr was > private to the deserializer, and the deser driver called > i2c_atr_add_adapter(). In my version the deser shares the i2c-atr with > the serializers, and the serializers call i2c_atr_add_adapter(). I think > this is much better, as the i2c-bus is behind the serializer, and the > serializer's registers affect the bus, and thus the bus should really be > create/destroyed by the serializer driver as it controls the bus' > i2c-master hardware. > > Now, i2c-mux and i2c-atr are quite similar as has been discussed in the > earlier reviews. And while the FPDLink ICs support ATR, we can easily > imagine a simpler deserializer which only supports mux-style forwarding. > For these reasons I believe we have two topics: 1) i2c-atr without > FPDLink, and 2) i2c-atr (and i2c-mux) with FPDlink. > > 1) > I am not aware of a stand-alone IC that performs address translation. > If there is, I think i2c-atr as it is in this series is a good solution > (but the bus fwnode feature mentioned above can be dropped). > > 2) > If I suggested adding an i2c-bus fwnode parameter to > i2c_mux_add_adapter(), and the i2c-bus might be under some other device, > I think the reception could be quite negative (and I would agree). For > this reason I'm not very happy with the i2c-atr and using it with > FPDLink. > > In fact, I'm thinking that it might be better to just drop the i2c-atr > driver, and add the support directly to the FPDlink drivers. But that > could mean possibly duplicating the same code for other deser/ser > architectures, so I have kept the i2c-atr driver for now. Indeed I think the ROHM serdes chips do have an address translation feature that works pretty much like the TI ones, and the ATR should be cleanly reusable across the two brands. The ATR code might be simplified to just provide helpers for common code maybe, but I'd rather avoid code duplication.
On 11/7/22 13:48, Luca Ceresoli wrote: > Hi Tomi, All, > > On Tue, 1 Nov 2022 15:20:24 +0200 > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: >> >> 2) >> If I suggested adding an i2c-bus fwnode parameter to >> i2c_mux_add_adapter(), and the i2c-bus might be under some other device, >> I think the reception could be quite negative (and I would agree). For >> this reason I'm not very happy with the i2c-atr and using it with >> FPDLink. >> >> In fact, I'm thinking that it might be better to just drop the i2c-atr >> driver, and add the support directly to the FPDlink drivers. But that >> could mean possibly duplicating the same code for other deser/ser >> architectures, so I have kept the i2c-atr driver for now. > > Indeed I think the ROHM serdes chips do have an address translation > feature that works pretty much like the TI ones, and the ATR should be > cleanly reusable across the two brands. The ATR code might be > simplified to just provide helpers for common code maybe, but I'd > rather avoid code duplication. I tend to agree with Luca on keeping the I2C-atr a re-usable generic code. The reason why I did not (yet) bring this (or other concerns) up is that currently I don't have a permission to upstream my work for ROHM SerDes Linux drivers :( I still do discuss the decision but to be honest, I believe the chances to get my drivers upstream are getting slim. Hence I did not see it reasonable to try pushing Tomi's / Luca's work to any direction. Yet, after that being said - these SerDes devices which can combine I2C buses from multiple remote devices to one local I2C controller do bring a real need for a solution like the address translation. Thus, I would not be surprized if we saw new devices with this kind of feature. Potentially also from other vendors. So, regardless what happens with ROHM SERDESes - I (too) think we should keep the ATR as independently reusable component. Yours, -- Matti Vaittinen
On 07/11/2022 13:48, Luca Ceresoli wrote: >> In fact, I'm thinking that it might be better to just drop the i2c-atr >> driver, and add the support directly to the FPDlink drivers. But that >> could mean possibly duplicating the same code for other deser/ser >> architectures, so I have kept the i2c-atr driver for now. > > Indeed I think the ROHM serdes chips do have an address translation > feature that works pretty much like the TI ones, and the ATR should be > cleanly reusable across the two brands. The ATR code might be > simplified to just provide helpers for common code maybe, but I'd > rather avoid code duplication. I agree. The reason I'm wondering about this is the fact that the i2c slave side (deser) is in a different IC and driver than the i2c master side and driver (ser). That makes it quite different from the i2c-mux (at least how the i2c-mux exists now). Tomi
On 11/1/22 15:20, Tomi Valkeinen wrote: > Hi, > > Intro > ----- > > This is, kind of, v4 of Luca's i2c-atr and FPDLink series, v3 of which > you can find from: > > https://lore.kernel.org/all/20220206115939.3091265-1-luca@lucaceresoli.net/ > //snip First of all, I like this series. There is just one thing that slightly bothers me here. It is putting everything the deserializer has inside one driver. I think I mentioned this already earlier - but to me these devices seem like MFD ICs. Hence, my attempt to support one of SerDes devices resembling the TI SerDes devices had following split. MFD I2C drivers for SER and DES. Own platform drivers for DES ATR, V4L2, pinctrl/GPIO. Also own platform drivers for SER V4L2, pinctrl/GPIO. Below you can see the relevant pieces of simplified (pseudo-)code explaining the (optional?) design which attempts creating somewhat smaller and re-usable individual drivers for DES/SER blocks. Please, do not treat this as a requirement - please treat it as a food for thoughts ;) DES MFD-core: as I2C - device. Registers regmap and IRQ-chip for sub-devices. Creates the sub-devices for DES. Parses the required SerDes topology from device-tree and makes the minimum required configuration to get the actual link (FPDLink in TI's case) run in a level that I2C connection is possible when ATR is configured. (Actually, I noticed this could be put in ATR driver, but in my opinion it's better to parse the overall DT in MFD and put it in the MFD driver-data so all sub-devices can get the parsed bits of DT data from parent w/o duplicating the parsing code). static struct mfd_cell bu18rm84_mfd_cells[] = { { .name = "bu18rm84-i2c", }, { .name = "bu18rm84-video", }, { .name = "bu18rm84-pinctrl", }, }; ... static int link_init(struct device *dev, struct regmap *regmap, struct bu18rm84_core_data *data, struct bu18rm84_ser_conf *ser, int ser_id) { ... } static int prepare_hw_for_subdevs(struct device *dev, struct regmap *regmap, struct bu18rm84_core_data *data) { int ret, i; ret = bu18rm84_common_init_pre(dev, regmap, data); if (ret) return ret; for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) { if (data->ser[i].ser_ep) { /* * We initialize link between DES => SERsin MFD so * the sub-devices have working link to SER devices. */ ret = link_init(dev, regmap, data, &data->ser[i], i); if (ret) return ret; } } ... return ret; } static int connected_devices(struct device *dev, struct bu18rm84_core_data *data) { int i; for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) { /* * TODO: See where/when/if we should drop the reference to * the node */ data->ser[i].ser_ep = fwnode_graph_get_endpoint_by_id(data->fw, i, 0, 0); if (data->ser[i].ser_ep) { dev_dbg(dev, "Found node for SER %d\n", i); data->num_ser++; } } for (; i < BU18RM84_MAX_NUM_SER_NODES + BU18RM84_MAX_NUM_CSI_NODES; i++) { int csi = i - BU18RM84_MAX_NUM_SER_NODES; data->csi_ep[csi] = fwnode_graph_get_endpoint_by_id(data->fw, i, 0, 0); if (data->csi_ep[csi]) data->num_csi++; } dev_dbg(dev, "found %d ser, %d csi\n", data->num_ser, data->num_csi); return (!data->num_ser) ? -ENODEV : 0; } static int bu18rm84_i2c_probe(struct i2c_client *i2c) { data = devm_kzalloc(&i2c->dev, sizeof(*data), GFP_KERNEL); regmap = devm_regmap_init_i2c(i2c, &bu18rm84_regmap); ret = connected_devices(&i2c->dev, data); /* * We need to set-up the link between DES and SER(s) before * the I2C to SER(s) is operational. Sub-devices like the SER GPIO, pinmux * or media do need this I2C so we'd better to prepare things up-to-the * point where I2C works prior registering the subdev cells. * * Unfortunately, this means the MFD needs to go and read & understand * the DT for SER topology - and do some magic settings to get things * up & running. */ ret = prepare_hw_for_subdevs(&i2c->dev, regmap, data); ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, i2c->irq, IRQF_SHARED, 0, &bu18rm84_irq_chip, &irq_data); ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bu18rm84_mfd_cells, ARRAY_SIZE(bu18rm84_mfd_cells), NULL, 0, regmap_irq_get_domain(irq_data)); } =============== DES ATR platform driver: Kicked by DES MFD. Uses I2C-atr to configure address translation. Creates SER devices and creates the I2C adapter for the remote SER buses. This in-turn launches SER MFD driver + remote I2C slave devices. And yes. I think the remote I2C slave devices must not be created before SER side link configurations are done by SER MFD driver. Thanks Tomi - I guess I've been lucky this far (or there is something I didn't spot quite yet ;] ) static int bu18rm84_atr_attach_client(struct i2c_atr *atr, u32 chan_id, const struct i2c_board_info *info, const struct i2c_client *client, u16 *alias_id) { /* Configure the ATR for slave to DES registers */ } static void bu18rm84_atr_detach_client(struct i2c_atr *atr, u32 chan_id, const struct i2c_client *client) { /* Clear the configuration for the ATR for slave at DES registers */ } static const struct i2c_atr_ops bu18rm84_atr_ops = { .attach_client = bu18rm84_atr_attach_client, .detach_client = bu18rm84_atr_detach_client, }; static int bu18rm84_populate_ser(struct bu18rm84_i2c_atr *data, u32 *addrs, int num_addrs, struct fwnode_handle *des_node, int ser_id) { /* * Configure the I2C alias for SER devices in DES registers based on * reg-names in the DT */ idx = fwnode_property_match_string(des_node, "reg-names", ser_names[ser_id]); ... } static int setup_ser_i2c(struct bu18rm84_i2c_atr *data, int ser_id, struct fwnode_handle *ser_ep) { int ret; const char *name; struct i2c_client **new = &data->ser_client[ser_id]; struct i2c_board_info ser_info = { 0 }; ser_info.fwnode = fwnode_get_named_child_node(ser_ep, "remote-chip"); name = fwnode_get_name(ser_info.fwnode); ser_info.addr = data->ser_alias[ser_id]; *new = i2c_new_client_device(data->parent_i2c->adapter, &ser_info); /* Add adapter to correctly enable i2c to devices behind SER */ /* We need to have ATR done before this */ ret = i2c_atr_add_adapter(data->atr, ser_id); ... return ret; } static int bu18rm84_setup_ser_i2cs(const struct bu18rm84_core_data *core, struct bu18rm84_i2c_atr *data) { for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) { if (core->ser[i].ser_ep) { ret = setup_ser_i2c(data, i, core->ser[i].ser_ep); ... } } } static int bu18rm84_atr_probe(struct platform_device *pdev) { parent = dev->parent; core = dev_get_drvdata(parent); data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); data->parent_i2c = to_i2c_client(parent); data->regmap = dev_get_regmap(parent, NULL); dev_set_drvdata(&pdev->dev, data); /* Go read the I2C addresses from DT */ ret = bu18rm84_i2c_parse_dt(data, core); /* * Create device for SER. We won't add SER to ATR in order to avoid * messing with the SER specific aliases in ATR. * * Also, add adapter. */ for (i = 0; i < BU18RM84_MAX_NUM_SER_NODES; i++) { if (core->ser[i].ser_ep) { ret = bu18rm84_populate_ser(data, &addrs[0], num_addrs, core->fw, i); } } data->atr = i2c_atr_new(data->parent_i2c->adapter, parent, &bu18rm84_atr_ops, 4); i2c_atr_set_clientdata(data->atr, data); return bu18rm84_setup_ser_i2cs(core, data); ============================= Ser MFD (i2c)driver. Kicked by DES ATR platform device when creating the I2C devices with the SER MFD fwnode. Finalizes the SER <=> DES I2C connection and kicks up the SER slave devices for DES pinctrl / V4L2 or others. static struct mfd_cell bu18tm41_mfd_cells[] = { { .name = "bu18tm41-video", }, /* Video forwarding */ { .name = "bu18tm41-pinctrl", }, /* pinmux, pinconf and gpio */ }; static int bu18tm41_conf_ser(struct device *dev, struct regmap *regmap, struct bu18tm41_core *core) { fw = dev_fwnode(dev); ret = fwnode_property_read_u64(fw, "compulsory stuff to get link up", &foo); ret = bu18tm41_enable_link( ... ); ... return 0; } static int bu18tm41_i2c_probe(struct i2c_client *i2c) { regmap = devm_regmap_init_i2c(i2c, &bu18tm41_regmap); core = devm_kzalloc(&i2c->dev, sizeof(*core), GFP_KERNEL); dev_set_drvdata(&i2c->dev, core); /* * Do the minimum required link config on SER config to get I2C * between DES <=> SER up * * Unfortunately, this means the MFD needs to do some magic settings * to get things up & running. */ ret = bu18tm41_conf_ser(&i2c->dev, regmap, core); ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bu18tm41_mfd_cells, ARRAY_SIZE(bu18tm41_mfd_cells), NULL, 0, /*regmap_irq_get_domain(irq_data) */ NULL); return ret; } =========================== Des media platform driver. V4L2/media stuff. ... static int bu18rm84_media_probe(struct platform_device *pdev) { core = dev_get_drvdata(dev->parent); data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); data->dev = dev; data->parent_i2c = to_i2c_client(dev->parent); data->regmap = dev_get_regmap(dev->parent, NULL); platform_set_drvdata(pdev, data); ret = bu18rm84_parse_csi_dt(core, data); ret = bu18rm84_config_csi2(data); ret = bu18rm84_add_ser_data(core, data); ... /* Create the V4L2 subdevices */ ret = bu18rm84_create_subdev(core, data); if (ret) goto err_subdev; ... } SER video, Des/Ser pinctrl/GPIO platform drivers: ... (Like usual) I think it was Tomi who asked me the benefit of using MFD. In some cases the digital interface towards pinctrl/GPIO or other functional blocks in SER/DES is re-used from other products - or the blocks are re-used on other products. Separating them in own platform-drivers is a nice way to re-use drivers and avoid code duplication. Whether it is worth or correct can be evaluated by brighter minds :) As I said, I am in a no way demanding a change if you see the current proposal being better! I am just presenting an optional way to feed your thoughts ;) Ps. Sorry for _very_ stripped down code in this explanation. I am currently not allowed to publish any details of the ICs these drivers are written for. Yours -- Matti
Hi Matti, On 07/11/2022 16:37, Vaittinen, Matti wrote: I only had time to have a brief look at your code, but I have a few quick questions. > I think it was Tomi who asked me the benefit of using MFD. In some cases > the digital interface towards pinctrl/GPIO or other functional blocks in > SER/DES is re-used from other products - or the blocks are re-used on > other products. Separating them in own platform-drivers is a nice way to > re-use drivers and avoid code duplication. Is there anything that prevents us (or makes it difficult) from refactoring a "monolithic" driver into an MFD later? If we see such IP re-use, can we then move to an MFD? I admit I have never written an MFD driver (but I have hacked with a few years back). As I see it, the "subcomponents" in FPDLink ICs are more or less tied to the FPDLink. It's not like they're independent. Compare to, for example, a PMIC which provides regulators and GPIOs, and possibly the only shared part between those two features are the pins. So, I think I'm still wondering about the benefit... In the current version I have the deser driver supporting UB960 and UB9702. I guess I could split those into separate drivers, and using MFD I could share lot of code between them. But I still can't see why that's better than having both UB960 and UB9702 in the same driver (and, if the amount of supported devices increases, perhaps dividing some parts to separate files and using function points for a few things). The benefit would be more obvious if there was some other type of IC that uses the same IP subcomponents. Maybe the display side FPDLink devices are such, but I have never done a deep dive in their documentation. And, even then, I think I still have the question: can we just move to an MFD later when the need comes? Also, isn't the use or non-use of MFD strictly a driver private thing, it should not affect any shared parts or shared designs? In other words, if you have your ROHM hat on, why do you care how the UB9xx driver is structured internally? ;) Tomi
Evening Tomi, On 11/7/22 17:37, Tomi Valkeinen wrote: > Hi Matti, > > On 07/11/2022 16:37, Vaittinen, Matti wrote: > > I only had time to have a brief look at your code, but I have a few > quick questions. > >> I think it was Tomi who asked me the benefit of using MFD. In some cases >> the digital interface towards pinctrl/GPIO or other functional blocks in >> SER/DES is re-used from other products - or the blocks are re-used on >> other products. Separating them in own platform-drivers is a nice way to >> re-use drivers and avoid code duplication. > > Is there anything that prevents us (or makes it difficult) from > refactoring a "monolithic" driver into an MFD later? If we see such IP > re-use, can we then move to an MFD? I haven't done such conversion. I think the work for doing the conversion at later phase is roughly same it would be now. However, synchronizing such change with related subsystem trees might be some extra work. > I admit I have never written an MFD driver (but I have hacked with a few > years back). As I see it, the "subcomponents" in FPDLink ICs are more or > less tied to the FPDLink. It's not like they're independent. Compare to, > for example, a PMIC which provides regulators and GPIOs, and possibly > the only shared part between those two features are the pins. I think that in SerDes driver case the benefit may come from re-use and clarity. Smaller drivers tend to be easier to comprehend, although I liked the way you had divided the drivers in sections. > So, I think I'm still wondering about the benefit... > > In the current version I have the deser driver supporting UB960 and > UB9702. I guess I could split those into separate drivers, I wouldn't break the driver per IC. If the ICs are similar enough to be nicely handled by same driver, then they probably should. > > The benefit would be more obvious if there was some other type of IC > that uses the same IP subcomponents. Yes. Same or similar subcomponents. This indeed is the benefit I see. I don't know if TI could use same - say GPIO - control logic on another type of device? Or, maybe separating the logic could guide one to use some generic stuff like regmap_gpio driver? And finally, submitting small platform drivers via respective subsystems can yield better review ;) > > Also, isn't the use or non-use of MFD strictly a driver private thing, I am tempted to say "yes", but when giving it a thought - it's really not fully that. Splitting a driver to subdrivers can allow re-use of subcomponents by unrelated ICs. OTOH, always stuffing everything in a driver "because it is driver internal decision" can lead to code duplication and bloat. > it should not affect any shared parts or shared designs? In other words, > if you have your ROHM hat on, why do you care how the UB9xx driver is > structured internally? ;) As I wrote: > > Please, do not treat this as a requirement - please treat it as a food for thoughts ;) Eg, I am not trying to tell you how to do the UB9xx drivers. I just think that considering another design _might_ result more optimal design - but I do leave the decision to you who know this area better than I do. Yours -- Matti
On 07/11/2022 13:40, Luca Ceresoli wrote: > On Fri, 4 Nov 2022 13:59:06 +0200 > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > >> Hi Andy, >> >> On 01/11/2022 16:30, Andy Shevchenko wrote: >>> On Tue, Nov 01, 2022 at 03:20:26PM +0200, Tomi Valkeinen wrote: >>>> From: Luca Ceresoli <luca@lucaceresoli.net> >>>> >>>> An ATR is a device that looks similar to an i2c-mux: it has an I2C >>>> slave "upstream" port and N master "downstream" ports, and forwards >>>> transactions from upstream to the appropriate downstream port. But is >>>> is different in that the forwarded transaction has a different slave >>>> address. The address used on the upstream bus is called the "alias" >>>> and is (potentially) different from the physical slave address of the >>>> downstream chip. >>>> >>>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >>>> implementing ATR features in a device driver. The helper takes care or >>>> adapter creation/destruction and translates addresses at each transaction. > > First of all, thank you for bringing this work on! > >>> ... >>> >>>> +I2C ADDRESS TRANSLATOR (ATR) >>>> +M: Luca Ceresoli <luca@lucaceresoli.net> >>> >>> Hmm... Are you going to maintain this? Or Review? Why not? >> >> We haven't discussed with Luca if he wants to maintain this (this is >> mostly his code). But, indeed, I should add my name there. > > I think at this point you are probably in a better position to be the > maintainer, but I'm OK with being listed here as reviewer (R:). > > Ah, would you please use my bootlin dot com address here? Ok, I've added your bootlin address as R: Tomi
Hi, Intro ----- This is, kind of, v4 of Luca's i2c-atr and FPDLink series, v3 of which you can find from: https://lore.kernel.org/all/20220206115939.3091265-1-luca@lucaceresoli.net/ I say "kind of", as the FPDLink drivers have diverged from Luca's version quite a bit and the drivers support different HW versions. A Big thanks for Luca for working on the drivers! I'd really like to send and review i2c-atr and FPDLink drivers separately, but as the concepts are new and those drivers are linked together, in the end I decided to keep them in one series. Even so, I think these patches divide quite clearly into to areas: 1) i2c-atr and the related code in the FPDLink drivers 2) Everything else about FPDLink The FPDLink drivers support multiple streams and for that reason the series is based on V4L2 streams series v15: https://lore.kernel.org/all/20221003121852.616745-1-tomi.valkeinen@ideasonboard.com/ HW overview ----------- TI's DS90UB9xx IC (UB9xx for short) family is a set of deserializer and serializer ICs for video, both display and capture. These ICs support FPDLink 3 and some also support FPDLink 4. From the user's point of view there's really not much difference between FPDLink 3 and 4. An example HW setup with two cameras could be like this: +-- [Ser 1] <-- [Camera 1] [CSI-2 RX] <-- [Deser] <--| +-- [Ser 2] <-- [Camera 2] The cameras send video streams over CSI-2 to the serializers. The serializers encode the received data to FPDLink and send it to the deserializer. The deserializer decodes the data and sends it forward over CSI-2 bus. The FPDLink bus is a bi-directional bus, with a fast forward-channel and a slow back-channel. In addition to the video, the devices support forwarding GPIOs (both ways) and forwarding i2c transactions with address translation. This series is only about the capture ICs. The HW versions supported by these drivers are: - UB960, FPDLink 3 deserializer - UB9702, FPDLink 3/4 deserializer - UB953, FPDLink 3 CSI-2 serializer - UB913, FPDLink 3 parallel video serializer - UB971, FPDLink 3/4 CSI-2 serializer Note that this series does not support UB954 deserializer, which Luca has. I don't have it and decided early to drop it for simpler development. As UB954 is a subset of UB960, adding support for it should be trivial. FPDLink Deser and Ser Devices & Drivers --------------------------------------- The serializer devices are, in a way, child devices of the deserializer. It is the deserializer driver which creates and removes the serializer devices, although the serializer device itself is not a child of the deserializer, but rather the serializer is created as a device on the i2c bus. There are perhaps other methods to add the serializer devices. I think a real bus is an overkill, as FPDLink is a point-to-point link. But if there's no FPDLink bus, what are the options? Platform device? That's not good either. So, for the time being, the serializers are i2c clients on the main i2c bus, even if they're really behind the FPDLink. Also, the deserializer driver needs to share some information with the serializers, and it is done with platform data set to the serializer device. If we had a bus, the bus could probably be used to convey this information. I2C & I2C ATR ------------- We have three different i2c client types: 1) The deserializer is a normal i2c slave, and there's nothing special about accessing it. The deserializer is connected to the "main" i2c bus. 2) The serializers are also accessible with i2c via FPDLink: when a transaction to a specific (programmable) address happens on the main i2c-bus the deserializer takes the transaction and forwards it via FPDLink to the serializer. This is implemented in the deserializer driver by just creating the serializer i2c device on the main i2c-bus. This is not 100% correct, as the serializers are not directly connected to an i2c bus as i2c slaves. 3) The serializer is an i2c master, and the camera and possibly other i2c peripherals can be connected to that "remote" i2c bus. The deserializer hardware has an i2c-alias table which describes which i2c-alias address the deserializer will react to, what is the real i2c-address and to which serializer the transactions will be forwarded to. This is called address translation (ATR). I2C ATR Thoughts ---------------- I have addressed most of the comments Luca received for the i2c-atr driver, but other than that the i2c-atr driver is mostly the same. However, there's one big difference: the i2c bus can be given by the drivers. So, looking at the DT, in Luca's version the deserializer node looked like this for serializers and i2c-atr: deser { remote-chips { remote-chip@0 { // The serializer }; }; i2c-atr { i2c@0 { // This is the bus behind the serializer }; }; }; I have: deser { links { link@0 { serializer { // The serializer i2c { // This is the bus }; } }; }; }; I think this reflects the hardware much better. But this means that the i2c-atr "device", which in Luca's version is only the deserializer, is in my version split between the deser and serializer. However, the only thing I changed in the i2c-atr driver is that the i2c_atr_add_adapter() function takes an fwnode handle which tells where the i2c bus is found (but if it's NULL, it looks for i2c-atr just like in Luca's version). Perhaps the biggest difference is that in Luca's version the i2c-atr was private to the deserializer, and the deser driver called i2c_atr_add_adapter(). In my version the deser shares the i2c-atr with the serializers, and the serializers call i2c_atr_add_adapter(). I think this is much better, as the i2c-bus is behind the serializer, and the serializer's registers affect the bus, and thus the bus should really be create/destroyed by the serializer driver as it controls the bus' i2c-master hardware. Now, i2c-mux and i2c-atr are quite similar as has been discussed in the earlier reviews. And while the FPDLink ICs support ATR, we can easily imagine a simpler deserializer which only supports mux-style forwarding. For these reasons I believe we have two topics: 1) i2c-atr without FPDLink, and 2) i2c-atr (and i2c-mux) with FPDlink. 1) I am not aware of a stand-alone IC that performs address translation. If there is, I think i2c-atr as it is in this series is a good solution (but the bus fwnode feature mentioned above can be dropped). 2) If I suggested adding an i2c-bus fwnode parameter to i2c_mux_add_adapter(), and the i2c-bus might be under some other device, I think the reception could be quite negative (and I would agree). For this reason I'm not very happy with the i2c-atr and using it with FPDLink. In fact, I'm thinking that it might be better to just drop the i2c-atr driver, and add the support directly to the FPDlink drivers. But that could mean possibly duplicating the same code for other deser/ser architectures, so I have kept the i2c-atr driver for now. In any case, I think when figuring out 2), we can forget about ATR and the added complexity the translation brings, and just think how i2c-mux could be used in a deser-ser setup. And maybe the answer is "don't use it, just write the support directly to the deser-ser drivers". Tomi Luca Ceresoli (2): i2c: core: let adapters be notified of client attach/detach i2c: add I2C Address Translator (ATR) support Tomi Valkeinen (6): dt-bindings: media: add bindings for TI DS90UB960 dt-bindings: media: add bindings for TI DS90UB913 dt-bindings: media: add bindings for TI DS90UB953 media: i2c: add DS90UB960 driver media: i2c: add DS90UB913 driver media: i2c: add DS90UB953 driver .../bindings/media/i2c/ti,ds90ub913.yaml | 127 + .../bindings/media/i2c/ti,ds90ub953.yaml | 120 + .../bindings/media/i2c/ti,ds90ub960.yaml | 392 ++ Documentation/i2c/index.rst | 1 + Documentation/i2c/muxes/i2c-atr.rst | 78 + MAINTAINERS | 7 + drivers/i2c/Kconfig | 9 + drivers/i2c/Makefile | 1 + drivers/i2c/i2c-atr.c | 497 ++ drivers/i2c/i2c-core-base.c | 18 +- drivers/media/i2c/Kconfig | 28 + drivers/media/i2c/Makefile | 3 + drivers/media/i2c/ds90ub913.c | 892 ++++ drivers/media/i2c/ds90ub953.c | 1607 +++++++ drivers/media/i2c/ds90ub960.c | 4198 +++++++++++++++++ include/linux/i2c-atr.h | 80 + include/linux/i2c.h | 16 + include/media/i2c/ds90ub9xx.h | 16 + 18 files changed, 8089 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml create mode 100644 Documentation/i2c/muxes/i2c-atr.rst create mode 100644 drivers/i2c/i2c-atr.c create mode 100644 drivers/media/i2c/ds90ub913.c create mode 100644 drivers/media/i2c/ds90ub953.c create mode 100644 drivers/media/i2c/ds90ub960.c create mode 100644 include/linux/i2c-atr.h create mode 100644 include/media/i2c/ds90ub9xx.h