Hi Daniel, Andy, On Sat, Dec 19, 2020 at 11:48:51PM +0000, Daniel Scally wrote: > On 19/12/2020 18:52, Andy Shevchenko wrote: > > On Sat, Dec 19, 2020 at 2:25 AM Daniel Scally <djrscally@xxxxxxxxx> wrote: > >> On 18/12/2020 21:17, Andy Shevchenko wrote: > >>> On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote: > > > > ... > > > >>>> + sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4); > >>> > >>> Does 4 has any meaning that can be described by #define ? > >> > >> It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: > >> > >> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36 > >> > >> That enum's not in an accessible header, but I can define it in this > >> module's header > > > > Maybe you can do a preparatory patch to make it visible to v4l2 > > drivers? (Like moving to one of v4l2 headers) > > Sure ok, guess media/v4l2-fwnode.h makes the most sense. Yes, please. > > > ... > > > >>>> + if (bridge->n_sensors >= CIO2_NUM_PORTS) { > >>>> + dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n"); > >>> > >>>> + /* overflow i so outer loop ceases */ > >>>> + i = ARRAY_SIZE(cio2_supported_sensors); > >>>> + break; > >>> > >>> Why not to create a new label below and assign ret here with probably comment > >>> why it's not an error? > >> > >> Sure, I can do that, but since it wouldn't need any cleanup I could also > >> just return 0 here as Laurent suggest (but with a comment explaining why > >> that's ok as you say) - do you have a preference? > > > > While it's a good suggestion it will bring a bit of inconsistency into > > approach. Everywhere else in the function you are using the goto > > approach. > > So yes, I have a preference. > > No problem Laurent also commented on the return code. I might just handle this as an error. The earlier ports are fine, but there's also a problem with the data here. It'd be easier to spot that this way, and we can change this in the future if need be. -- Kind regards, Sakari Ailus
