Re: [PATCH v12 18/26] v4l: fwnode: Add a helper function for parsing generic references

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Hans,

On Wed, Sep 13, 2017 at 12:32:17PM +0200, Hans Verkuil wrote:
> On 09/13/17 12:07, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Sep 13, 2017 at 11:28:44AM +0200, Hans Verkuil wrote:
> >> On 09/13/17 11:24, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> Thanks for the review!
> >>>
> >>> On Wed, Sep 13, 2017 at 09:27:34AM +0200, Hans Verkuil wrote:
> >>>> On 09/12/2017 03:41 PM, Sakari Ailus wrote:
> >>>>> Add function v4l2_fwnode_reference_count() for counting external
> >>>>
> >>>> ???? There is no function v4l2_fwnode_reference_count()?!
> >>>
> >>> It got removed during the revisions but the commit message was not changed
> >>> accordingly, I do that now.
> >>>
> >>>>
> >>>>> references and v4l2_fwnode_reference_parse() for parsing them as async
> >>>>> sub-devices.
> >>>>>
> >>>>> This can be done on e.g. flash or lens async sub-devices that are not part
> >>>>> of but are associated with a sensor.
> >>>>>
> >>>>> struct v4l2_async_notifier.max_subdevs field is added to contain the
> >>>>> maximum number of sub-devices in a notifier to reflect the memory
> >>>>> allocated for the subdevs array.
> >>>>
> >>>> You forgot to remove this outdated paragraph.
> >>>
> >>> Oops. Removed it now.
> >>>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 69 +++++++++++++++++++++++++++++++++++
> >>>>>  1 file changed, 69 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> index 44ee35f6aad5..a32473f95be1 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> @@ -498,6 +498,75 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> >>>>>  }
> >>>>>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);
> >>>>>  
> >>>>> +/*
> >>>>> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> >>>>> + * @dev: the device node the properties of which are parsed for references
> >>>>> + * @notifier: the async notifier where the async subdevs will be added
> >>>>> + * @prop: the name of the property
> >>>>> + *
> >>>>> + * Return: 0 on success
> >>>>> + *	   -ENOENT if no entries were found
> >>>>> + *	   -ENOMEM if memory allocation failed
> >>>>> + *	   -EINVAL if property parsing failed
> >>>>> + */
> >>>>> +static int v4l2_fwnode_reference_parse(
> >>>>> +	struct device *dev, struct v4l2_async_notifier *notifier,
> >>>>> +	const char *prop)
> >>>>> +{
> >>>>> +	struct fwnode_reference_args args;
> >>>>> +	unsigned int index;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	for (index = 0;
> >>>>> +	     !(ret = fwnode_property_get_reference_args(
> >>>>> +		       dev_fwnode(dev), prop, NULL, 0, index, &args));
> >>>>> +	     index++)
> >>>>> +		fwnode_handle_put(args.fwnode);
> >>>>> +
> >>>>> +	if (!index)
> >>>>> +		return -ENOENT;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * To-do: handle -ENODATA when "device property: Align return
> >>>>> +	 * codes of acpi_fwnode_get_reference_with_args" is merged.
> >>>>> +	 */
> >>>>> +	if (ret != -ENOENT && ret != -ENODATA)
> >>>>
> >>>> So while that patch referenced in the To-do above is not merged yet,
> >>>> what does fwnode_property_get_reference_args return? ENOENT or ENODATA?
> >>>> Or ENOENT now and ENODATA later? Or vice versa?
> >>>>
> >>>> I can't tell based on that information whether this code is correct or not.
> >>>>
> >>>> The comment needs to explain this a bit better.
> >>>
> >>> I'll add this:
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> index a32473f95be1..74fcc3ba9ebd 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> @@ -529,6 +529,9 @@ static int v4l2_fwnode_reference_parse(
> >>>  	/*
> >>>  	 * To-do: handle -ENODATA when "device property: Align return
> >>>  	 * codes of acpi_fwnode_get_reference_with_args" is merged.
> >>
> >> So after this patch referred to in the To-do is applied it will only
> >> return ENODATA?
> >>
> >> In that case, change 'handle' to 'handle only'.
> > 
> > That depends a bit in which form the patch will be eventually accepted. The
> > underlying issue there is that different error codes are used to signal
> > conditions for out-of-bounds access and missing entry. After aligning them
> > the code here can be updated.
> 
> Ah. In that case I'd drop the 'To-do' sentence.
> 
> > 
> >>
> >>> +	 * Right now, both -ENODATA and -ENOENT signal the end of
> >>> +	 * references where only a single error code should be used
> >>> +	 * for the purpose.
> 
> And add something like: "This might change in the future, in which
> case this code should be updated."

I'll use:

	/*
	 * Note that right now both -ENODATA and -ENOENT may signal
	 * out-of-bounds access. Return the error in cases other than that.
	 */

-- 
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux