All platform drivers have been converted to use
v4l2_async_notifier_add_subdev(), in place of adding
asd's to the notifier subdevs array. So the subdevs
array can now be removed from struct v4l2_async_notifier,
and remove the backward compatibility support for that
array in v4l2-async.c.
Signed-off-by: Steve Longerbeam <[email protected]>
---
Changes since v5:
- also remove notifier num_subdevs and V4L2_MAX_SUBDEVS macro.
Suggested by Sakari Ailus.
---
drivers/media/v4l2-core/v4l2-async.c | 114 ++++++++---------------------------
include/media/v4l2-async.h | 21 ++-----
include/media/v4l2-fwnode.h | 12 ----
3 files changed, 30 insertions(+), 117 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 78cf1a9..9d8343e 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -358,31 +358,23 @@ static bool __v4l2_async_notifier_has_async_subdev(
/*
* Find out whether an async sub-device was set up already or
* whether it exists in a given notifier before @this_index.
+ * If @this_index < 0, search the notifier's entire @asd_list.
*/
static bool v4l2_async_notifier_has_async_subdev(
struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
- unsigned int this_index)
+ int this_index)
{
struct v4l2_async_subdev *asd_y;
- unsigned int j;
+ int j = 0;
lockdep_assert_held(&list_lock);
/* Check that an asd is not being added more than once. */
- if (notifier->subdevs) {
- for (j = 0; j < this_index; j++) {
- asd_y = notifier->subdevs[j];
- if (asd_equal(asd, asd_y))
- return true;
- }
- } else {
- j = 0;
- list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) {
- if (j++ >= this_index)
- break;
- if (asd_equal(asd, asd_y))
- return true;
- }
+ list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) {
+ if (this_index >= 0 && j++ >= this_index)
+ break;
+ if (asd_equal(asd, asd_y))
+ return true;
}
/* Check that an asd does not exist in other notifiers. */
@@ -395,7 +387,7 @@ static bool v4l2_async_notifier_has_async_subdev(
static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
struct v4l2_async_subdev *asd,
- unsigned int this_index)
+ int this_index)
{
struct device *dev =
notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
@@ -434,36 +426,19 @@ EXPORT_SYMBOL(v4l2_async_notifier_init);
static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
{
struct v4l2_async_subdev *asd;
- int ret;
- int i;
-
- if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
- return -EINVAL;
+ int ret, i = 0;
INIT_LIST_HEAD(¬ifier->waiting);
INIT_LIST_HEAD(¬ifier->done);
mutex_lock(&list_lock);
- if (notifier->subdevs) {
- for (i = 0; i < notifier->num_subdevs; i++) {
- asd = notifier->subdevs[i];
-
- ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
- if (ret)
- goto err_unlock;
+ list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
+ ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
+ if (ret)
+ goto err_unlock;
- list_add_tail(&asd->list, ¬ifier->waiting);
- }
- } else {
- i = 0;
- list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
- ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
- if (ret)
- goto err_unlock;
-
- list_add_tail(&asd->list, ¬ifier->waiting);
- }
+ list_add_tail(&asd->list, ¬ifier->waiting);
}
ret = v4l2_async_notifier_try_all_subdevs(notifier);
@@ -556,45 +531,22 @@ EXPORT_SYMBOL(v4l2_async_notifier_unregister);
static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
{
struct v4l2_async_subdev *asd, *tmp;
- unsigned int i;
if (!notifier)
return;
- if (notifier->subdevs) {
- for (i = 0; i < notifier->num_subdevs; i++) {
- asd = notifier->subdevs[i];
-
- switch (asd->match_type) {
- case V4L2_ASYNC_MATCH_FWNODE:
- fwnode_handle_put(asd->match.fwnode);
- break;
- default:
- break;
- }
-
- kfree(asd);
+ list_for_each_entry_safe(asd, tmp, ¬ifier->asd_list, asd_list) {
+ switch (asd->match_type) {
+ case V4L2_ASYNC_MATCH_FWNODE:
+ fwnode_handle_put(asd->match.fwnode);
+ break;
+ default:
+ break;
}
- kvfree(notifier->subdevs);
- notifier->subdevs = NULL;
- } else {
- list_for_each_entry_safe(asd, tmp,
- ¬ifier->asd_list, asd_list) {
- switch (asd->match_type) {
- case V4L2_ASYNC_MATCH_FWNODE:
- fwnode_handle_put(asd->match.fwnode);
- break;
- default:
- break;
- }
-
- list_del(&asd->asd_list);
- kfree(asd);
- }
+ list_del(&asd->asd_list);
+ kfree(asd);
}
-
- notifier->num_subdevs = 0;
}
void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
@@ -614,27 +566,11 @@ int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
mutex_lock(&list_lock);
- if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
- ret = -EINVAL;
- goto unlock;
- }
-
- /*
- * If caller uses this function, it cannot also allocate and
- * place asd's in the notifier->subdevs array.
- */
- if (WARN_ON(notifier->subdevs)) {
- ret = -EINVAL;
- goto unlock;
- }
-
- ret = v4l2_async_notifier_asd_valid(notifier, asd,
- notifier->num_subdevs);
+ ret = v4l2_async_notifier_asd_valid(notifier, asd, -1);
if (ret)
goto unlock;
list_add_tail(&asd->asd_list, ¬ifier->asd_list);
- notifier->num_subdevs++;
unlock:
mutex_unlock(&list_lock);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 16b1e2b..89b152f 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -20,9 +20,6 @@ struct v4l2_device;
struct v4l2_subdev;
struct v4l2_async_notifier;
-/* A random max subdevice number, used to allocate an array on stack */
-#define V4L2_MAX_SUBDEVS 128U
-
/**
* enum v4l2_async_match_type - type of asynchronous subdevice logic to be used
* in order to identify a match
@@ -124,20 +121,16 @@ struct v4l2_async_notifier_operations {
* struct v4l2_async_notifier - v4l2_device notifier data
*
* @ops: notifier operations
- * @num_subdevs: number of subdevices used in the subdevs array
- * @subdevs: array of pointers to subdevice descriptors
* @v4l2_dev: v4l2_device of the root notifier, NULL otherwise
* @sd: sub-device that registered the notifier, NULL otherwise
* @parent: parent notifier
- * @asd_list: master list of struct v4l2_async_subdev, replaces @subdevs
+ * @asd_list: master list of struct v4l2_async_subdev
* @waiting: list of struct v4l2_async_subdev, waiting for their drivers
* @done: list of struct v4l2_subdev, already probed
* @list: member in a global list of notifiers
*/
struct v4l2_async_notifier {
const struct v4l2_async_notifier_operations *ops;
- unsigned int num_subdevs;
- struct v4l2_async_subdev **subdevs;
struct v4l2_device *v4l2_dev;
struct v4l2_subdev *sd;
struct v4l2_async_notifier *parent;
@@ -164,10 +157,8 @@ void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
* @notifier: pointer to &struct v4l2_async_notifier
* @asd: pointer to &struct v4l2_async_subdev
*
- * This can be used before registering a notifier to add an
- * asd to the notifiers @asd_list. If the caller uses this
- * method to compose an asd list, it must never allocate
- * or place asd's in the @subdevs array.
+ * Call this function before registering a notifier to link the
+ * provided asd to the notifiers master @asd_list.
*/
int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
struct v4l2_async_subdev *asd);
@@ -184,10 +175,8 @@ int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
* the driver's async sub-device struct, i.e. both
* begin at the same memory address.
*
- * This can be used before registering a notifier to add a
- * fwnode-matched asd to the notifiers master asd_list. If the caller
- * uses this method to compose an asd list, it must never allocate
- * or place asd's in the @subdevs array.
+ * Allocate a fwnode-matched asd of size asd_struct_size, and add it
+ * to the notifiers @asd_list.
*/
struct v4l2_async_subdev *
v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 031ebb0..8b4873c 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -259,12 +259,6 @@ typedef int (*parse_endpoint_func)(struct device *dev,
* This function may not be called on a registered notifier and may be called on
* a notifier only once.
*
- * Do not allocate the notifier's subdevs array, or change the notifier's
- * num_subdevs field. This is because this function uses
- * @v4l2_async_notifier_add_subdev to populate the notifier's asd_list,
- * which is in-place-of the subdevs array which must remain unallocated
- * and unused.
- *
* The &struct v4l2_fwnode_endpoint passed to the callback function
* @parse_endpoint is released once the function is finished. If there is a need
* to retain that configuration, the user needs to allocate memory for it.
@@ -316,12 +310,6 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
* This function may not be called on a registered notifier and may be called on
* a notifier only once per port.
*
- * Do not allocate the notifier's subdevs array, or change the notifier's
- * num_subdevs field. This is because this function uses
- * @v4l2_async_notifier_add_subdev to populate the notifier's asd_list,
- * which is in-place-of the subdevs array which must remain unallocated
- * and unused.
- *
* The &struct v4l2_fwnode_endpoint passed to the callback function
* @parse_endpoint is released once the function is finished. If there is a need
* to retain that configuration, the user needs to allocate memory for it.
--
2.7.4
Hi Steve,
Thanks for the update.
On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
> All platform drivers have been converted to use
> v4l2_async_notifier_add_subdev(), in place of adding
> asd's to the notifier subdevs array. So the subdevs
> array can now be removed from struct v4l2_async_notifier,
> and remove the backward compatibility support for that
> array in v4l2-async.c.
>
> Signed-off-by: Steve Longerbeam <[email protected]>
This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
discussed previously) but it doesn't include the corresponding
driver changes. Is there a patch missing from the set?
--
Kind regards,
Sakari Ailus
e-mail: [email protected]
On 07/23/2018 05:35 AM, Sakari Ailus wrote:
> Hi Steve,
>
> Thanks for the update.
>
> On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
>> All platform drivers have been converted to use
>> v4l2_async_notifier_add_subdev(), in place of adding
>> asd's to the notifier subdevs array. So the subdevs
>> array can now be removed from struct v4l2_async_notifier,
>> and remove the backward compatibility support for that
>> array in v4l2-async.c.
>>
>> Signed-off-by: Steve Longerbeam <[email protected]>
> This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
> discussed previously) but it doesn't include the corresponding
> driver changes. Is there a patch missing from the set?
Hi Sakari, yes somehow patch 15/17 (the large patch to all drivers)
got dropped by the ML, maybe because the cc-list was too big?
I will resend with only linux-media and cc: you.
Steve
On Mon, Jul 23, 2018 at 09:44:57AM -0700, Steve Longerbeam wrote:
>
>
> On 07/23/2018 05:35 AM, Sakari Ailus wrote:
> > Hi Steve,
> >
> > Thanks for the update.
> >
> > On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
> > > All platform drivers have been converted to use
> > > v4l2_async_notifier_add_subdev(), in place of adding
> > > asd's to the notifier subdevs array. So the subdevs
> > > array can now be removed from struct v4l2_async_notifier,
> > > and remove the backward compatibility support for that
> > > array in v4l2-async.c.
> > >
> > > Signed-off-by: Steve Longerbeam <[email protected]>
> > This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
> > discussed previously) but it doesn't include the corresponding
> > driver changes. Is there a patch missing from the set?
>
> Hi Sakari, yes somehow patch 15/17 (the large patch to all drivers)
> got dropped by the ML, maybe because the cc-list was too big?
>
> I will resend with only linux-media and cc: you.
I noticed the patch in my inbox and I re-pushed my v4l2-fwnode branch,
including that patch. Please still resend to the list. Thanks.
--
Sakari Ailus
[email protected]
Hi Steve,
On Mon, Jul 23, 2018 at 09:44:57AM -0700, Steve Longerbeam wrote:
>
>
> On 07/23/2018 05:35 AM, Sakari Ailus wrote:
> >Hi Steve,
> >
> >Thanks for the update.
> >
> >On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
> >>All platform drivers have been converted to use
> >>v4l2_async_notifier_add_subdev(), in place of adding
> >>asd's to the notifier subdevs array. So the subdevs
> >>array can now be removed from struct v4l2_async_notifier,
> >>and remove the backward compatibility support for that
> >>array in v4l2-async.c.
> >>
> >>Signed-off-by: Steve Longerbeam <[email protected]>
> >This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
> >discussed previously) but it doesn't include the corresponding
> >driver changes. Is there a patch missing from the set?
>
> Hi Sakari, yes somehow patch 15/17 (the large patch to all drivers)
> got dropped by the ML, maybe because the cc-list was too big?
>
> I will resend with only linux-media and cc: you.
For the Renesas CEU and Renesas R-Car VIN you can add my:
Tested-by: Jacopo Mondi <[email protected]>
I would have a very small comment on the renesas-ceu.c patch. I'm copying
the hunk here below as the patch didn't reach the mailing list
>@@ -1562,40 +1557,46 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> dev_err(ceudev->dev,
> "No subdevice connected on endpoint %u.\n", i);
> ret = -ENODEV;
>- goto error_put_node;
>+ goto error_cleanup;
> }
>
> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &fw_ep);
> if (ret) {
> if (ret) {
> dev_err(ceudev->dev,
> "Unable to parse endpoint #%u.\n", i);
>- goto error_put_node;
>+ goto error_cleanup;
> }
>
> if (fw_ep.bus_type != V4L2_MBUS_PARALLEL) {
> dev_err(ceudev->dev,
> "Only parallel input supported.\n");
> ret = -EINVAL;
>- goto error_put_node;
>+ goto error_cleanup;
> }
>
> /* Setup the ceu subdevice and the async subdevice. */
> ceu_sd = &ceudev->subdevs[i];
> INIT_LIST_HEAD(&ceu_sd->asd.list);
>
>+ remote = of_graph_get_remote_port_parent(ep);
> ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>- ceu_sd->asd.match.fwnode =
>- fwnode_graph_get_remote_port_parent(
>- of_fwnode_handle(ep));
>+ ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
>+
>+ ret = v4l2_async_notifier_add_subdev(&ceudev->notifier,
>+ &ceu_sd->asd);
>+ if (ret) {
>+ of_node_put(remote);
^^^ The 'remote' device node is only put in
the error path
>
>+ goto error_cleanup;
>+ }
>
>- ceudev->asds[i] = &ceu_sd->asd;
> of_node_put(ep);
> }
>
> return num_ep;
>
>-error_put_node:
>+error_cleanup:
>+ v4l2_async_notifier_cleanup(&ceudev->notifier);
> of_node_put(ep);
> return ret;
> }
Thanks
j
Hi Jacopo,
On 08/04/2018 03:33 AM, jacopo mondi wrote:
> Hi Steve,
>
> On Mon, Jul 23, 2018 at 09:44:57AM -0700, Steve Longerbeam wrote:
>>
>> On 07/23/2018 05:35 AM, Sakari Ailus wrote:
>>> Hi Steve,
>>>
>>> Thanks for the update.
>>>
>>> On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
>>>> All platform drivers have been converted to use
>>>> v4l2_async_notifier_add_subdev(), in place of adding
>>>> asd's to the notifier subdevs array. So the subdevs
>>>> array can now be removed from struct v4l2_async_notifier,
>>>> and remove the backward compatibility support for that
>>>> array in v4l2-async.c.
>>>>
>>>> Signed-off-by: Steve Longerbeam <[email protected]>
>>> This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
>>> discussed previously) but it doesn't include the corresponding
>>> driver changes. Is there a patch missing from the set?
>> Hi Sakari, yes somehow patch 15/17 (the large patch to all drivers)
>> got dropped by the ML, maybe because the cc-list was too big?
>>
>> I will resend with only linux-media and cc: you.
> For the Renesas CEU and Renesas R-Car VIN you can add my:
>
> Tested-by: Jacopo Mondi <[email protected]>
Thanks for testing!
>
> I would have a very small comment on the renesas-ceu.c patch. I'm copying
> the hunk here below as the patch didn't reach the mailing list
>
>> @@ -1562,40 +1557,46 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
>> dev_err(ceudev->dev,
>> "No subdevice connected on endpoint %u.\n", i);
>> ret = -ENODEV;
>> - goto error_put_node;
>> + goto error_cleanup;
>> }
>>
>> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &fw_ep);
>> if (ret) {
>> if (ret) {
>> dev_err(ceudev->dev,
>> "Unable to parse endpoint #%u.\n", i);
>> - goto error_put_node;
>> + goto error_cleanup;
>> }
>>
>> if (fw_ep.bus_type != V4L2_MBUS_PARALLEL) {
>> dev_err(ceudev->dev,
>> "Only parallel input supported.\n");
>> ret = -EINVAL;
>> - goto error_put_node;
>> + goto error_cleanup;
>> }
>>
>> /* Setup the ceu subdevice and the async subdevice. */
>> ceu_sd = &ceudev->subdevs[i];
>> INIT_LIST_HEAD(&ceu_sd->asd.list);
>>
>> + remote = of_graph_get_remote_port_parent(ep);
>> ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
>> ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>> - ceu_sd->asd.match.fwnode =
>> - fwnode_graph_get_remote_port_parent(
>> - of_fwnode_handle(ep));
>> + ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
>> +
>> + ret = v4l2_async_notifier_add_subdev(&ceudev->notifier,
>> + &ceu_sd->asd);
>> + if (ret) {
>> + of_node_put(remote);
> ^^^ The 'remote' device node is only put in
> the error path
If v4l2_async_notifier_add_subdev() succeeds, then the reference
is kept on the remote node until the asd is freed in
v4l2_async_notifier_cleanup(). Otherwise if
v4l2_async_notifier_add_subdev() fails, the asd is not
added to the notifier asd_list so the caller is responsible
for putting the remote node. So the only case where
the remote needs to be put is in the line you marked above.
Otherwise in the other error-out paths, the remote nodes
for all asd's that have been added will be put below in the
error_cleanup path.
Steve
>> + goto error_cleanup;
>> + }
>>
>> - ceudev->asds[i] = &ceu_sd->asd;
>> of_node_put(ep);
>> }
>>
>> return num_ep;
>>
>> -error_put_node:
>> +error_cleanup:
>> + v4l2_async_notifier_cleanup(&ceudev->notifier);
>> of_node_put(ep);
>> return ret;
>> }
> Thanks
> j