Hello all
Previous version:
https://lore.kernel.org/linux-media/[email protected]/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67
The RFC version before that:
https://lore.kernel.org/linux-media/[email protected]/
This series is to start adding support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. This problem has two main parts; the
first part, which is handled in this series, is extending the ipu3-cio2
driver to allow for patching the firmware via software_nodes if endpoints
aren't defined by ACPI. The second is adding a new driver to handle power,
clocks and GPIO pins defined in DSDT tables in an awkward way. I decided to
split that second part out from this series, and instead give it its own
series (a v2 of which should land "soon"). The reasons for that are:
1. It's a logically separate change anyway
2. The recipients list was getting really long and
3. That probably meant that handling merge for all of this in one go was
going to be impractically awkward.
Given how few comments the remaining patches of this series received in the
last posting, I'm hopeful that most or all of it could get picked up for 5.12.
We touch a few different areas:
lib (with an ack already)
lib/test_printf.c: Use helper function to unwind array of
software_nodes
drivers/base
software_node: Fix refcounts in software_node_get_next_child()
property: Return true in fwnode_device_is_available for NULL ops
property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
software_node: Enforce parent before child ordering of nodes arrays
software_node: unregister software_nodes in reverse order
drivers/acpi
acpi: Add acpi_dev_get_next_match_dev() and helper macro
drivers/media
media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
match_fwnode()
ipu3-cio2: Add T: entry to MAINTAINERS
ipu3-cio2: Rename ipu3-cio2.c
ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Given that, it feels sensible to me to try and merge them all through a single
tree; I was hoping the other maintainers would be amenable to having everything
merged through the media tree. Mauro; if that plan is ok (and of course assuming
that the rest of the patches are acked by their respective maintainers too),
could we get a dedicated feature branch just in case the following series ends
up being ready in time too?
Series-level changelog:
- Squashed the patches enforcing ordering in register/unregister_nodes()
More details of changes on each patch.
Comments as always very welcome - and thanks to everyone for all your help on
this so far, hope I've addressed everything from last time.
Dan
Daniel Scally (11):
software_node: Fix refcounts in software_node_get_next_child()
property: Return true in fwnode_device_is_available for NULL ops
property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
software_node: Enforce parent before child ordering of nodes arrays
software_node: unregister software_nodes in reverse order
lib/test_printf.c: Use helper function to unwind array of
software_nodes
ipu3-cio2: Add T: entry to MAINTAINERS
ipu3-cio2: Rename ipu3-cio2.c
media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
match_fwnode()
acpi: Add acpi_dev_get_next_match_dev() and helper macro
ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Heikki Krogerus (1):
software_node: Add support for fwnode_graph*() family of functions
MAINTAINERS | 2 +
drivers/acpi/utils.c | 30 +-
drivers/base/property.c | 15 +-
drivers/base/swnode.c | 173 +++++++++--
drivers/media/pci/intel/ipu3/Kconfig | 18 ++
drivers/media/pci/intel/ipu3/Makefile | 3 +
drivers/media/pci/intel/ipu3/cio2-bridge.c | 274 ++++++++++++++++++
drivers/media/pci/intel/ipu3/cio2-bridge.h | 122 ++++++++
.../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 34 +++
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 +
drivers/media/v4l2-core/v4l2-async.c | 8 +
include/acpi/acpi_bus.h | 7 +
lib/test_printf.c | 4 +-
13 files changed, 669 insertions(+), 27 deletions(-)
create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)
--
2.25.1
To ensure we handle situations in which multiple sensors of the same
model (and therefore _HID) are present in a system, we need to be able
to iterate over devices matching a known _HID but unknown _UID and _HRV
- add acpi_dev_get_next_match_dev() to accommodate that possibility and
change acpi_dev_get_first_match_dev() to simply call the new function
with a NULL starting point. Add an iterator macro for convenience.
Reviewed-by: Andy Shevchenko <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v2:
- None
drivers/acpi/utils.c | 30 ++++++++++++++++++++++++++----
include/acpi/acpi_bus.h | 7 +++++++
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index d5411a166685..c177165c8db2 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -843,12 +843,13 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
EXPORT_SYMBOL(acpi_dev_present);
/**
- * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * acpi_dev_get_next_match_dev - Return the next match of ACPI device
+ * @adev: Pointer to the previous acpi_device matching this hid, uid and hrv
* @hid: Hardware ID of the device.
* @uid: Unique ID of the device, pass NULL to not check _UID
* @hrv: Hardware Revision of the device, pass -1 to not check _HRV
*
- * Return the first match of ACPI device if a matching device was present
+ * Return the next match of ACPI device if another matching device was present
* at the moment of invocation, or NULL otherwise.
*
* The caller is responsible to call put_device() on the returned device.
@@ -856,8 +857,9 @@ EXPORT_SYMBOL(acpi_dev_present);
* See additional information in acpi_dev_present() as well.
*/
struct acpi_device *
-acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv)
{
+ struct device *start = adev ? &adev->dev : NULL;
struct acpi_dev_match_info match = {};
struct device *dev;
@@ -865,9 +867,29 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
match.uid = uid;
match.hrv = hrv;
- dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
+ dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
return dev ? to_acpi_device(dev) : NULL;
}
+EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
+
+/**
+ * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return the first match of ACPI device if a matching device was present
+ * at the moment of invocation, or NULL otherwise.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ *
+ * See additional information in acpi_dev_present() as well.
+ */
+struct acpi_device *
+acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+{
+ return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
+}
EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
/*
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a3abcc4b7d9f..0a028ba967d3 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -688,9 +688,16 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
+struct acpi_device *
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
struct acpi_device *
acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
+#define for_each_acpi_dev_match(adev, hid, uid, hrv) \
+ for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv); \
+ adev; \
+ adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
+
static inline void acpi_dev_put(struct acpi_device *adev)
{
put_device(&adev->dev);
--
2.25.1
Where the fwnode graph is comprised of software_nodes, these will be
assigned as the secondary to dev->fwnode. Check the v4l2_subdev's fwnode
for a secondary and attempt to match against it during match_fwnode() to
accommodate that possibility.
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v2:
- None
drivers/media/v4l2-core/v4l2-async.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..9dd896d085ec 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -87,6 +87,14 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
if (sd->fwnode == asd->match.fwnode)
return true;
+ /*
+ * Check the same situation for any possible secondary assigned to the
+ * subdev's fwnode
+ */
+ if (!IS_ERR_OR_NULL(sd->fwnode->secondary) &&
+ sd->fwnode->secondary == asd->match.fwnode)
+ return true;
+
/*
* Otherwise, check if the sd fwnode and the asd fwnode refer to an
* endpoint or a device. If they're of the same type, there's no match.
--
2.25.1
ipu3-cio2 driver needs extending with multiple files; rename the main
source file and specify the renamed file in Makefile to accommodate that.
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v2:
- None
drivers/media/pci/intel/ipu3/Makefile | 2 ++
drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0
2 files changed, 2 insertions(+)
rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%)
diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
index 98ddd5beafe0..429d516452e4 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -1,2 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
+
+ipu3-cio2-y += ipu3-cio2-main.o
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
similarity index 100%
rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c
rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
--
2.25.1
Development for the ipu3-cio2 driver is taking place in media_tree, but
there's no T: entry in MAINTAINERS to denote that - rectify that oversight
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v2:
- None
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 80881fb36404..16b544624577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8946,6 +8946,7 @@ M: Bingbu Cao <[email protected]>
R: Tianshu Qiu <[email protected]>
L: [email protected]
S: Maintained
+T: git git://linuxtv.org/media_tree.git
F: Documentation/userspace-api/media/v4l/pixfmt-srggb10-ipu3.rst
F: drivers/media/pci/intel/ipu3/
--
2.25.1
To maintain consistency with software_node_unregister_nodes(), reverse
the order in which the software_node_unregister_node_group() function
unregisters nodes.
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v2:
- Initialised i properly
drivers/base/swnode.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index cfd1faea48a7..2b90d380039b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -778,16 +778,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
* software_node_unregister_node_group - Unregister a group of software nodes
* @node_group: NULL terminated array of software node pointers to be unregistered
*
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. The array will be unwound in
+ * reverse order (I.E. last entry first) and thus if any member of the array
+ * has its .parent member set then they should appear later in the array such
+ * that they are unregistered first.
*/
void software_node_unregister_node_group(const struct software_node **node_group)
{
- unsigned int i;
+ unsigned int i = 0;
if (!node_group)
return;
- for (i = 0; node_group[i]; i++)
+ while (node_group[i]->name)
+ i++;
+
+ while (i--)
software_node_unregister(node_group[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
2.25.1
On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
...
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (I.E. last entry first) and thus if any member of the array
A nit: I.E. -> i.e.
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 17, 2020 at 11:43:36PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
> - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.
...
> - * acpi_dev_get_first_match_dev - Return the first match of ACPI device
> + * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> + * @adev: Pointer to the previous acpi_device matching this hid, uid and hrv
A nit: @hid, @uid and @hrv
> * @hid: Hardware ID of the device.
> * @uid: Unique ID of the device, pass NULL to not check _UID
> * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
--
With Best Regards,
Andy Shevchenko
Hi Daniel,
On Thu, Dec 17, 2020 at 11:43:36PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
> - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
--
Sakari Ailus
Hi Daniel,
On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v2:
>
> - Initialised i properly
>
> drivers/base/swnode.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index cfd1faea48a7..2b90d380039b 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -778,16 +778,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
> * software_node_unregister_node_group - Unregister a group of software nodes
> * @node_group: NULL terminated array of software node pointers to be unregistered
> *
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (I.E. last entry first) and thus if any member of the array
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.
> */
> void software_node_unregister_node_group(const struct software_node **node_group)
> {
> - unsigned int i;
> + unsigned int i = 0;
>
> if (!node_group)
> return;
>
> - for (i = 0; node_group[i]; i++)
> + while (node_group[i]->name)
Why is this change made? node_group is a NULL-terminated array, and the
above accesses the name pointer on each entry before checking the entry is
non-NULL. Or do I miss something here?
> + i++;
> +
> + while (i--)
> software_node_unregister(node_group[i]);
> }
> EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
Regards,
Sakari Ailus
On Mon, Dec 21, 2020 at 11:21:16AM +0200, Sakari Ailus wrote:
> On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
> > To maintain consistency with software_node_unregister_nodes(), reverse
> > the order in which the software_node_unregister_node_group() function
> > unregisters nodes.
...
> > void software_node_unregister_node_group(const struct software_node **node_group)
> > {
> > - unsigned int i;
> > + unsigned int i = 0;
> >
> > if (!node_group)
> > return;
> >
> > - for (i = 0; node_group[i]; i++)
> > + while (node_group[i]->name)
>
> Why is this change made? node_group is a NULL-terminated array, and the
> above accesses the name pointer on each entry before checking the entry is
> non-NULL. Or do I miss something here?
I believe it's a copy'n'paste typo.
> > + i++;
> > +
> > + while (i--)
> > software_node_unregister(node_group[i]);
> > }
--
With Best Regards,
Andy Shevchenko
On 21/12/2020 11:26, Andy Shevchenko wrote:
> On Mon, Dec 21, 2020 at 11:21:16AM +0200, Sakari Ailus wrote:
>> On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
>>> To maintain consistency with software_node_unregister_nodes(), reverse
>>> the order in which the software_node_unregister_node_group() function
>>> unregisters nodes.
>
> ...
>
>>> void software_node_unregister_node_group(const struct software_node **node_group)
>>> {
>>> - unsigned int i;
>>> + unsigned int i = 0;
>>>
>>> if (!node_group)
>>> return;
>>>
>>> - for (i = 0; node_group[i]; i++)
>>> + while (node_group[i]->name)
>>
>> Why is this change made? node_group is a NULL-terminated array, and the
>> above accesses the name pointer on each entry before checking the entry is
>> non-NULL. Or do I miss something here?
>
> I believe it's a copy'n'paste typo.
Careless copy and paste yeah, my bad. I was doing it for consistency but
really should've just changed the ordering; I'll just drop that part.