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. > ... > >>>> + 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 >>>> + } > > ... > >>>> + ret = cio2_bridge_read_acpi_buffer(adev, "SSDB", >>>> + &sensor->ssdb, >>>> + sizeof(sensor->ssdb)); >>>> + if (ret < 0) >>> >>> if (ret) (because positive case can be returned just by next conditional). >> >> cio2_bridge_read_acpi_buffer() returns the buffer length on success at >> the moment, but I can change it to return 0 and have this be if (ret) > > Please correct this somehow, because the next failure returns it > instead of error... Ah! Good spot - thank you. I will fix that yes. >>>> + goto err_put_adev; >>>> + >>>> + if (sensor->ssdb.lanes > 4) { >>>> + dev_err(&adev->dev, >>>> + "Number of lanes in SSDB is invalid\n"); > > ...I'm even thinking that you have to assign ret here to something meaningful. Yeah I agree, I will do this too. >>>> + goto err_put_adev; >>>> + } >
