Hi, On Thu, Mar 01, 2018 at 05:26:58PM +0100, Niklas Söderlund wrote: > I did not do a full review on this series, I only browsed it to check > how you handled some CSI-2 related problems. While doing so I noticed a > few small issues. Thanks for your review :) The comment I stripped out will be addressed. > > + /* > > + * Create a static mapping between the CSI virtual channels > > + * and the input streams. > > + * > > + * This should be enhanced, but v4l2 lacks the support for > > + * changing that mapping dynamically at the moment. > > Sakaris work will help with this in the kernel and I have some RFC > patches for v4l-utils which can be used to configure it from user-space. > > https://www.spinics.net/lists/linux-media/msg126128.html So my vision for this was this would come as a second step. I'd really like to get the base driver merged, and then build on top of that to get extra features. The mapping is definitely on my radar, and like I was saying in my other mail, I even started to work on it. > > +static int csi2tx_probe(struct platform_device *pdev) > > +{ > > + struct csi2tx_priv *csi2tx; > > + unsigned int i; > > + int ret; > > + > > + csi2tx = kzalloc(sizeof(*csi2tx), GFP_KERNEL); > > No real issue I'm just curious, why not use devm_kzalloc() here? You > free csi2tx in the remove() callback so not using devm_ will not help > with (the potential) v4l2 related lifetime management issues. Laurent asked me to do this in an earlier iteration: https://patchwork.linuxtv.org/patch/42683/ I'm fine with both, but he seemed to say that it would be later for a later cleanup to not use the devm variant here. > > +MODULE_DEVICE_TABLE(of, csi2tx_of_table); > > + > > +static struct platform_driver csi2tx_driver = { > > + .probe = csi2tx_probe, > > + .remove = csi2tx_remove, > > + > > + .driver = { > > + .name = "cdns-csi2tx", > > + .of_match_table = csi2tx_of_table, > > + }, > > +}; > > +module_platform_driver(csi2tx_driver); > > Are MODULE_LICENSE() not needed anymore with the introduction of the > SPDX headers? This is definitely missing, I'll add it. Thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
