| Message ID | 20170913092430.cbdgerkhiuxakbxv@valkosipuli.retiisi.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> >>> --- >>> 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'. > + * Right now, both -ENODATA and -ENOENT signal the end of > + * references where only a single error code should be used > + * for the purpose. > */ > if (ret != -ENOENT && ret != -ENODATA) > return ret; > Regards, Hans
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@linux.intel.com> > >>> --- > >>> 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. > > > + * Right now, both -ENODATA and -ENOENT signal the end of > > + * references where only a single error code should be used > > + * for the purpose. > > */ > > if (ret != -ENOENT && ret != -ENODATA) > > return ret; > >
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@linux.intel.com> >>>>> --- >>>>> 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." >>> */ >>> if (ret != -ENOENT && ret != -ENODATA) >>> return ret; >>> > Regards, Hans
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. + * Right now, both -ENODATA and -ENOENT signal the end of + * references where only a single error code should be used + * for the purpose. */ if (ret != -ENOENT && ret != -ENODATA) return ret;