Re: [PATCH v2 2/2] media: i2c: Add support for ov5693 sensor
Hi JM! On 06/04/2021 14:54, Jean-Michel Hautbois wrote: > Hello Daniel ! > > Thanks for the patch ! > > On 06/04/2021 00:56, Daniel Scally wrote: >> The OV5693 is a 5 Mpx CMOS image sensor, connected via MIPI CSI-2. The >> chip is capable of a single lane configuration, but currently only two >> lanes are supported. >> >> Most of the sensor's features are supported, with the main exception >> being the lens correction algorithm. >> >> The driver provides all mandatory, optional and recommended V4L2 controls >> for maximum compatibility with libcamera. >> >> Signed-off-by: Daniel Scally >> --- >> Changes in v2: >> >> - Removed HDR mode from the exposure configure function, as it's not >> necessary and seemed to be causing frame sync errors. >> - Switched from a mode based driver to use the .s_selection() API. At >> the moment scaler support is lacking so .s_fmt() is limited to the crop >> rectangle or those dimensions binned (separately) by 2. >> - Switched to i2c_transfer() in ov5693_read_reg() >> - Lots of more minor cleanup. >> >> Jacopo; I thought that the changes were significant enough that I should drop >> your R-b, so I did. >> >> MAINTAINERS|7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile |1 + >> drivers/media/i2c/ov5693.c | 1557 >> 4 files changed, 1576 insertions(+) >> create mode 100644 drivers/media/i2c/ov5693.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index cf44b3e77b90..34311d55b189 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -13140,6 +13140,13 @@ S: Maintained >> T: git git://linuxtv.org/media_tree.git >> F: drivers/media/i2c/ov5675.c >> >> +OMNIVISION OV5693 SENSOR DRIVER >> +M: Daniel Scally >> +L: linux-me...@vger.kernel.org >> +S: Maintained >> +T: git git://linuxtv.org/media_tree.git >> +F: drivers/media/i2c/ov5693.c >> + >> OMNIVISION OV5695 SENSOR DRIVER >> M: Shunqian Zheng >> L: linux-me...@vger.kernel.org >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 4c1ae687ab10..4da2278ec84c 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -985,6 +985,17 @@ config VIDEO_OV5675 >>To compile this driver as a module, choose M here: the >>module will be called ov5675. >> >> +config VIDEO_OV5693 >> +tristate "OmniVision OV5693 sensor support" >> +depends on I2C && VIDEO_V4L2 >> +select V4L2_FWNODE >> +help >> + This is a Video4Linux2 sensor driver for the OmniVision >> + OV5693 camera. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called ov5693. >> + >> config VIDEO_OV5695 >> tristate "OmniVision OV5695 sensor support" >> depends on I2C && VIDEO_V4L2 >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index 65cfc94d25b6..7df680e110c9 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -75,6 +75,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o >> obj-$(CONFIG_VIDEO_OV5648) += ov5648.o >> obj-$(CONFIG_VIDEO_OV5670) += ov5670.o >> obj-$(CONFIG_VIDEO_OV5675) += ov5675.o >> +obj-$(CONFIG_VIDEO_OV5693) += ov5693.o >> obj-$(CONFIG_VIDEO_OV5695) += ov5695.o >> obj-$(CONFIG_VIDEO_OV6650) += ov6650.o >> obj-$(CONFIG_VIDEO_OV7251) += ov7251.o >> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c >> new file mode 100644 >> index ..da2ca99a7ad3 >> --- /dev/null >> +++ b/drivers/media/i2c/ov5693.c >> @@ -0,0 +1,1557 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2013 Intel Corporation. All Rights Reserved. >> + * >> + * Adapted from the atomisp-ov5693 driver, with contributions from: >> + * >> + * Daniel Scally >> + * Jean-Michel Hautbois >> + * Fabian Wuthrich >> + * Tsuchiya Yuto >> + * Jordan Hand >> + * Jake Day >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* System Control */ >> +#define OV5693_SW_RESET_REG
[PATCH v2 1/2] media: ipu3-cio2: Toggle sensor streaming in pm runtime ops
The .suspend() and .resume() runtime_pm operations for the ipu3-cio2
driver currently do not handle the sensor's stream. Setting .s_stream() on
or off for the sensor subdev means that sensors will pause and resume the
stream at the appropriate time even if their drivers don't implement those
operations.
Signed-off-by: Daniel Scally
---
Changes in v2:
Patch introduced
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index db59b19a6236..0a970ddeb281 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1981,12 +1981,19 @@ static int __maybe_unused cio2_suspend(struct device
*dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct cio2_device *cio2 = pci_get_drvdata(pci_dev);
struct cio2_queue *q = cio2->cur_queue;
+ int r;
dev_dbg(dev, "cio2 suspend\n");
if (!cio2->streaming)
return 0;
/* Stop stream */
+ r = v4l2_subdev_call(q->sensor, video, s_stream, 0);
+ if (r) {
+ dev_err(dev, "failed to stop sensor streaming\n");
+ return r;
+ }
+
cio2_hw_exit(cio2, q);
synchronize_irq(pci_dev->irq);
@@ -2021,8 +2028,14 @@ static int __maybe_unused cio2_resume(struct device *dev)
}
r = cio2_hw_init(cio2, q);
- if (r)
+ if (r) {
dev_err(dev, "fail to init cio2 hw\n");
+ return r;
+ }
+
+ r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
+ if (r)
+ dev_err(dev, "fail to start sensor streaming\n");
return r;
}
--
2.25.1
[PATCH v2 0/2] Add support for OV5693 Sensor
Hello all Previous version here: https://lore.kernel.org/linux-media/20210312103239.279523-1-djrsca...@gmail.com/ Patch #1 updates the CIO2 driver to call s_stream() for the current sensor when runtime .suspend() and .resume() ops fire, which should mean the sensor drivers can pause and restart streaming without having those ops implemented themselves. Patch #2 adds support for the OV5693 sensor found as the front camera in many Microsoft Surface devices, along with a number of similar style laptops. It is a heavily adapted derivative of the atomisp-ov5693 driver in staging, which retains most of the global register settings and some of the other functions from that driver, but otherwise uses the "normal" v4l2 infrastructure. Daniel Scally (2): media: ipu3-cio2: Toggle sensor streaming in pm runtime ops media: i2c: Add support for ov5693 sensor MAINTAINERS |7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile|1 + drivers/media/i2c/ov5693.c| 1557 + drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 15 +- 5 files changed, 1590 insertions(+), 1 deletion(-) create mode 100644 drivers/media/i2c/ov5693.c -- 2.25.1
[PATCH v2 2/2] media: i2c: Add support for ov5693 sensor
The OV5693 is a 5 Mpx CMOS image sensor, connected via MIPI CSI-2. The chip is capable of a single lane configuration, but currently only two lanes are supported. Most of the sensor's features are supported, with the main exception being the lens correction algorithm. The driver provides all mandatory, optional and recommended V4L2 controls for maximum compatibility with libcamera. Signed-off-by: Daniel Scally --- Changes in v2: - Removed HDR mode from the exposure configure function, as it's not necessary and seemed to be causing frame sync errors. - Switched from a mode based driver to use the .s_selection() API. At the moment scaler support is lacking so .s_fmt() is limited to the crop rectangle or those dimensions binned (separately) by 2. - Switched to i2c_transfer() in ov5693_read_reg() - Lots of more minor cleanup. Jacopo; I thought that the changes were significant enough that I should drop your R-b, so I did. MAINTAINERS|7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/ov5693.c | 1557 4 files changed, 1576 insertions(+) create mode 100644 drivers/media/i2c/ov5693.c diff --git a/MAINTAINERS b/MAINTAINERS index cf44b3e77b90..34311d55b189 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13140,6 +13140,13 @@ S: Maintained T: git git://linuxtv.org/media_tree.git F: drivers/media/i2c/ov5675.c +OMNIVISION OV5693 SENSOR DRIVER +M: Daniel Scally +L: linux-me...@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: drivers/media/i2c/ov5693.c + OMNIVISION OV5695 SENSOR DRIVER M: Shunqian Zheng L: linux-me...@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 4c1ae687ab10..4da2278ec84c 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -985,6 +985,17 @@ config VIDEO_OV5675 To compile this driver as a module, choose M here: the module will be called ov5675. +config VIDEO_OV5693 + tristate "OmniVision OV5693 sensor support" + depends on I2C && VIDEO_V4L2 + select V4L2_FWNODE + help + This is a Video4Linux2 sensor driver for the OmniVision + OV5693 camera. + + To compile this driver as a module, choose M here: the + module will be called ov5693. + config VIDEO_OV5695 tristate "OmniVision OV5695 sensor support" depends on I2C && VIDEO_V4L2 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 65cfc94d25b6..7df680e110c9 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -75,6 +75,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o obj-$(CONFIG_VIDEO_OV5648) += ov5648.o obj-$(CONFIG_VIDEO_OV5670) += ov5670.o obj-$(CONFIG_VIDEO_OV5675) += ov5675.o +obj-$(CONFIG_VIDEO_OV5693) += ov5693.o obj-$(CONFIG_VIDEO_OV5695) += ov5695.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o obj-$(CONFIG_VIDEO_OV7251) += ov7251.o diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c new file mode 100644 index ..da2ca99a7ad3 --- /dev/null +++ b/drivers/media/i2c/ov5693.c @@ -0,0 +1,1557 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2013 Intel Corporation. All Rights Reserved. + * + * Adapted from the atomisp-ov5693 driver, with contributions from: + * + * Daniel Scally + * Jean-Michel Hautbois + * Fabian Wuthrich + * Tsuchiya Yuto + * Jordan Hand + * Jake Day + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* System Control */ +#define OV5693_SW_RESET_REG0x0103 +#define OV5693_SW_STREAM_REG 0x0100 +#define OV5693_START_STREAMING 0x01 +#define OV5693_STOP_STREAMING 0x00 +#define OV5693_SW_RESET0x01 + +#define OV5693_REG_CHIP_ID_H 0x300a +#define OV5693_REG_CHIP_ID_L 0x300b +/* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */ +#define OV5693_CHIP_ID 0x5690 + +/* Exposure */ +#define OV5693_EXPOSURE_L_CTRL_HH_REG 0x3500 +#define OV5693_EXPOSURE_L_CTRL_H_REG 0x3501 +#define OV5693_EXPOSURE_L_CTRL_L_REG 0x3502 +#define OV5693_EXPOSURE_CTRL_HH(v) (((v) & GENMASK(14, 12)) >> 12) +#define OV5693_EXPOSURE_CTRL_H(v) (((v) & GENMASK(11, 4)) >> 4) +#define OV5693_EXPOSURE_CTRL_L(v) (((v) & GENMASK(3, 0)) << 4) +#define OV5693_INTEGRATION_TIME_MARGIN 8 +#define OV5693_EXPOSURE_MIN1 +#define OV5693_EXPOSURE_STEP 1 + +/* Analogue Gain */ +#define OV5693_GAIN_CTRL_H_REG 0x350
Re: [PATCH v1 1/1] media: ipu3-cio2: Fix referece counting when looping over ACPI devices
Hi Andy
On 04/04/2021 19:14, Andy Shevchenko wrote:
> When we continue, due to device is disabled, loop we have to drop reference
> count.
> When we have an array full of devices we have to also drop the reference
> count.
> Note, in this case the cio2_bridge_unregister_sensors() is called by the
> caller.
>
> Fixes: 803abec64ef9 ("media: ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver")
> Cc: Daniel Scally
> Cc: Sakari Ailus
> Signed-off-by: Andy Shevchenko
Ah; thanks for catching those, I'm annoyed to have missed the
acpi_dev_put() calls in particular
Reviewed-by: Daniel Scally
Re: [PATCH v2 5/6] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro
Hi Andy
On 30/03/2021 10:26, Andy Shevchenko wrote:
>>> + { .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), }, \
>> What are the .args intended to be used for? I actually had it in mind to
>> replace this with a simple pointer to a struct software_node, because I
>> can't see any users of them and the fact that it's actually storing a
>> pointer to a new variable is something that confused me for a good long
>> time when I wrote the cio2-bridge (though that's mostly due to my
>> relative inexperience of course, but still)
> It's to be in align with DT phandle references that can take arguments. While
> for now, indeed, we have no users of this, it might be changed in the future
> (I hadn't checked DesignWare DMA where I would like to transform the code to
> use device properties eventually and there it might be the case).
Ah yeah I see - haven't come across phandles before but having looked
them up now I see what this is meant to emulate. Consistency is good; in
that case, for this and 6/6:
Reviewed-by: Daniel Scally
and
Tested-by: Daniel Scally
>
Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails
Hi Andy
On 29/03/2021 16:12, Andy Shevchenko wrote:
> Currently we have a slightly twisted logic in swnode_register().
> It frees resources that it doesn't allocate on error path and
> in once case it relies on the ->release() implementation.
>
> Untwist the logic by freeing resources explicitly when swnode_register()
> fails. Currently it happens only in fwnode_create_software_node().
>
> Signed-off-by: Andy Shevchenko
Reviewed-by: Daniel Scally
and
Tested-by: Daniel Scally
> ---
> v2: no changes
> drivers/base/swnode.c | 29 +
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index fa3719ef80e4..456f5fe58b58 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -767,22 +767,19 @@ swnode_register(const struct software_node *node,
> struct swnode *parent,
> int ret;
>
> swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
> - if (!swnode) {
> - ret = -ENOMEM;
> - goto out_err;
> - }
> + if (!swnode)
> + return ERR_PTR(-ENOMEM);
>
> ret = ida_simple_get(parent ? &parent->child_ids : &swnode_root_ids,
>0, 0, GFP_KERNEL);
> if (ret < 0) {
> kfree(swnode);
> - goto out_err;
> + return ERR_PTR(ret);
> }
>
> swnode->id = ret;
> swnode->node = node;
> swnode->parent = parent;
> - swnode->allocated = allocated;
> swnode->kobj.kset = swnode_kset;
> fwnode_init(&swnode->fwnode, &software_node_ops);
>
> @@ -803,16 +800,17 @@ swnode_register(const struct software_node *node,
> struct swnode *parent,
> return ERR_PTR(ret);
> }
>
> + /*
> + * Assign the flag only in the successful case, so
> + * the above kobject_put() won't mess up with properties.
> + */
> + swnode->allocated = allocated;
> +
> if (parent)
> list_add_tail(&swnode->entry, &parent->children);
>
> kobject_uevent(&swnode->kobj, KOBJ_ADD);
> return &swnode->fwnode;
> -
> -out_err:
> - if (allocated)
> - property_entries_free(node->properties);
> - return ERR_PTR(ret);
> }
>
> /**
> @@ -963,6 +961,7 @@ struct fwnode_handle *
> fwnode_create_software_node(const struct property_entry *properties,
> const struct fwnode_handle *parent)
> {
> + struct fwnode_handle *fwnode;
> struct software_node *node;
> struct swnode *p = NULL;
> int ret;
> @@ -987,7 +986,13 @@ fwnode_create_software_node(const struct property_entry
> *properties,
>
> node->parent = p ? p->node : NULL;
>
> - return swnode_register(node, p, 1);
> + fwnode = swnode_register(node, p, 1);
> + if (IS_ERR(fwnode)) {
> + property_entries_free(node->properties);
> + kfree(node);
> + }
> +
> + return fwnode;
> }
> EXPORT_SYMBOL_GPL(fwnode_create_software_node);
>
Re: [PATCH v2 5/6] software node: Introduce SOFTWARE_NODE_REFERENCE() helper macro
Hi Andy
On 29/03/2021 16:12, Andy Shevchenko wrote:
> This is useful to assign software node reference with arguments
> in a common way. Moreover, we have already couple of users that
> may be converted. And by the fact, one of them is moved right here
> to use the helper.
>
> Signed-off-by: Andy Shevchenko
> ---
> v2: no changes
> drivers/base/test/property-entry-test.c | 11 ++-
> include/linux/property.h| 13 -
> 2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/test/property-entry-test.c
> b/drivers/base/test/property-entry-test.c
> index abe03315180f..c2e455d46ffd 100644
> --- a/drivers/base/test/property-entry-test.c
> +++ b/drivers/base/test/property-entry-test.c
> @@ -370,15 +370,8 @@ static void pe_test_reference(struct kunit *test)
> };
>
> static const struct software_node_ref_args refs[] = {
> - {
> - .node = &nodes[0],
> - .nargs = 0,
> - },
> - {
> - .node = &nodes[1],
> - .nargs = 2,
> - .args = { 3, 4 },
> - },
> + SOFTWARE_NODE_REFERENCE(&nodes[0]),
> + SOFTWARE_NODE_REFERENCE(&nodes[1], 3, 4),
> };
>
> const struct property_entry entries[] = {
> diff --git a/include/linux/property.h b/include/linux/property.h
> index dd4687b56239..0d876316e61d 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -254,6 +254,13 @@ struct software_node_ref_args {
> u64 args[NR_FWNODE_REFERENCE_ARGS];
> };
>
> +#define SOFTWARE_NODE_REFERENCE(_ref_, ...) \
> +(const struct software_node_ref_args) { \
> + .node = _ref_, \
> + .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
> + .args = { __VA_ARGS__ },\
> +}
> +
> /**
> * struct property_entry - "Built-in" device property representation.
> * @name: Name of the property.
> @@ -362,11 +369,7 @@ struct property_entry {
> .name = _name_, \
> .length = sizeof(struct software_node_ref_args),\
> .type = DEV_PROP_REF, \
> - { .pointer = &(const struct software_node_ref_args) { \
> - .node = _ref_, \
> - .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
> - .args = { __VA_ARGS__ },\
> - } },\
> + { .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), }, \
> }
What are the .args intended to be used for? I actually had it in mind to
replace this with a simple pointer to a struct software_node, because I
can't see any users of them and the fact that it's actually storing a
pointer to a new variable is something that confused me for a good long
time when I wrote the cio2-bridge (though that's mostly due to my
relative inexperience of course, but still)
>
> struct property_entry *
Re: [PATCH v2 3/6] software node: Deduplicate code in fwnode_create_software_node()
Hi Andy
On 29/03/2021 16:12, Andy Shevchenko wrote:
> Deduplicate conditional and assignment in fwnode_create_software_node(),
> i.e. parent is checked in two out of three cases and parent software node
> is assigned by to_swnode() call.
>
> Signed-off-by: Andy Shevchenko
Reviewed-by: Daniel Scally
> ---
> v2: no changes
> drivers/base/swnode.c | 17 -
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 19aa44bc2628..db982859171e 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -973,15 +973,14 @@ fwnode_create_software_node(const struct property_entry
> *properties,
> {
> struct fwnode_handle *fwnode;
> struct software_node *node;
> - struct swnode *p = NULL;
> -
> - if (parent) {
> - if (IS_ERR(parent))
> - return ERR_CAST(parent);
> - if (!is_software_node(parent))
> - return ERR_PTR(-EINVAL);
> - p = to_swnode(parent);
> - }
> + struct swnode *p;
> +
> + if (IS_ERR(parent))
> + return ERR_CAST(parent);
> +
> + p = to_swnode(parent);
> + if (parent && !p)
> + return ERR_PTR(-EINVAL);
>
> node = software_node_alloc(properties);
> if (IS_ERR(node))
Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
Hi Andy On 29/03/2021 16:03, Andy Shevchenko wrote: > On Thu, Mar 04, 2021 at 01:49:14PM +0000, Daniel Scally wrote: >> On 04/03/2021 13:37, Hans de Goede wrote: >>> On 2/22/21 2:07 PM, Daniel Scally wrote: > ... > >>>> The existing mfd/tps68470.c driver being thus superseded, it is removed. >>> Thank you for this patch series. Since there have already been a whole >>> bunch of review-comments, I've not taken a detailed look at this yet. >> No problem, I'm hoping to do a v3 over the weekend anyway. > Do you mean v4? Oops, I do indeed. > I'm just wondering if you need any help. Thanks - I don't think so; I've just not been working on it very much lately. I got sidetracked with a sensor driver [1] that was pretty fun, so I've been focused on that instead. I'm just finishing up a v2 for that, and then I'll come back to this. [1] https://lore.kernel.org/linux-media/20210312103239.279523-2-djrsca...@gmail.com/
Re: [PATCH 1/1] media: i2c: Add support for ov5693 sensor
Hi Laurent On 14/03/2021 23:29, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Fri, Mar 12, 2021 at 10:32:39AM +, Daniel Scally wrote: >> The OV5693 is a 5 Mpx CMOS image sensor, connected via MIPI CSI-2. The >> chip is capable of a single lane configuration, but currently only two >> lanes are supported. >> >> Most of the sensor's features are supported, with the main exception >> being the lens correction algorithm. >> >> The driver provides all mandatory, optional and recommended V4L2 controls >> for maximum compatibility with libcamera. >> >> Signed-off-by: Daniel Scally >> --- >> MAINTAINERS|7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile |1 + >> drivers/media/i2c/ov5693.c | 1585 >> 4 files changed, 1604 insertions(+) >> create mode 100644 drivers/media/i2c/ov5693.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index cf44b3e77b90..34311d55b189 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -13140,6 +13140,13 @@ S: Maintained >> T: git git://linuxtv.org/media_tree.git >> F: drivers/media/i2c/ov5675.c >> >> +OMNIVISION OV5693 SENSOR DRIVER >> +M: Daniel Scally >> +L: linux-me...@vger.kernel.org >> +S: Maintained >> +T: git git://linuxtv.org/media_tree.git >> +F: drivers/media/i2c/ov5693.c >> + >> OMNIVISION OV5695 SENSOR DRIVER >> M: Shunqian Zheng >> L: linux-me...@vger.kernel.org >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 4c1ae687ab10..4da2278ec84c 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -985,6 +985,17 @@ config VIDEO_OV5675 >>To compile this driver as a module, choose M here: the >>module will be called ov5675. >> >> +config VIDEO_OV5693 >> +tristate "OmniVision OV5693 sensor support" >> +depends on I2C && VIDEO_V4L2 >> +select V4L2_FWNODE >> +help >> + This is a Video4Linux2 sensor driver for the OmniVision >> + OV5693 camera. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called ov5693. >> + >> config VIDEO_OV5695 >> tristate "OmniVision OV5695 sensor support" >> depends on I2C && VIDEO_V4L2 >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index 65cfc94d25b6..7df680e110c9 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -75,6 +75,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o >> obj-$(CONFIG_VIDEO_OV5648) += ov5648.o >> obj-$(CONFIG_VIDEO_OV5670) += ov5670.o >> obj-$(CONFIG_VIDEO_OV5675) += ov5675.o >> +obj-$(CONFIG_VIDEO_OV5693) += ov5693.o >> obj-$(CONFIG_VIDEO_OV5695) += ov5695.o >> obj-$(CONFIG_VIDEO_OV6650) += ov6650.o >> obj-$(CONFIG_VIDEO_OV7251) += ov7251.o >> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c >> new file mode 100644 >> index ..0460371164ea >> --- /dev/null >> +++ b/drivers/media/i2c/ov5693.c >> @@ -0,0 +1,1585 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2013 Intel Corporation. All Rights Reserved. >> + * >> + * Adapted from the atomisp-ov5693 driver, with contributions from: >> + * >> + * Daniel Scally >> + * Jean-Michel Hautbois >> + * Fabian Wuthrich >> + * Tsuchiya Yuto >> + * Jordan Hand >> + * Jake Day >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > You've nearly mastered the alphabetical order ;-) Hah, only so long as it occurs to me to do so. > >> + >> +/* System Control */ >> +#define OV5693_SW_RESET_REG 0x0103 >> +#define OV5693_SW_STREAM_REG0x0100 >> +#define OV5693_START_STREAMING 0x01 >> +#define OV5693_STOP_STREAMING 0x00 >> +#define OV5693_SW_RESET 0x01 >> + >> +#define OV5693_REG_CHIP_ID_H0x300A >> +#define OV5693_REG_CHIP_ID_L0x300B >> +/* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 >> */ >> +#define OV5693_C
Re: [PATCH 1/1] media: i2c: Add support for ov5693 sensor
Hi Jacopo, thanks for the review
On 14/03/2021 10:40, Jacopo Mondi wrote:
>> +#define OV5693_REG_CHIP_ID_H0x300A
>> +#define OV5693_REG_CHIP_ID_L0x300B
> Please use lower case letters in hex identifiers
Will do
>> +
>> +/* Miscellaneous */
>> +#define OV5693_NUM_MBUS_FMTS1
> Nit: Only used in one place, maybe could be removed
Okedokey
>> +#define OV5693_NUM_RESOLUTIONS ARRAY_SIZE(ov5693_resolutions)
>> +struct ov5693_resolution ov5693_resolutions[] = {
>> +{
>> +.desc = "ov5693_2592x1944_30fps",
>> +.fps = 30,
>> +
>> +.crop_start_x = 16,
>> +.offset_x = 0,
>> +.output_size_x = 2592,
>> +.crop_end_x = 2608,
>> +.hts = 2688,
> Aren't the crop information here duplicated in the .crop field ? Do
> you need to have them separate ?
They don't need to separate, but I wonder if it's a bit confusing
because only crop.left and crop.top can be used. We couldn't draw the
output sizes from crop.width or crop.height because those values are
pre-scaling/binning. So it just seemed a bit clearer to treat them
separately.
>> +
>> +.crop_start_y = 6,
>> +.offset_y = 0,
>> +.output_size_y = 1944,
>> +.crop_end_y = 1950,
>> +.vts = 1984,
>> +
>> +.inc_x_odd = 1,
>> +.inc_x_even = 1,
>> +.inc_y_odd = 1,
>> +.inc_y_even = 1,
>> +
>> +.crop = {
>> +.left = 16,
>> +.top = 6,
>> +.width = 2592,
>> +.height = 1944
>> +},
>> +},
>> +{
>> +.desc = "ov5693_1920x1080_30fps",
> What's desc used for ?
Actually it's a leftover from the atomisp version that I've been using
as a handy label...it never even crossed my mind to change it. But it
should be a comment of course, I'll swap to that.
>> +static int ov5693_update_bits(struct ov5693_device *ov5693, u16 address,
>> + u16 mask, u16 bits)
>> +{
>> +u8 value = 0;
>> +int ret;
>> +
>> +ret = ov5693_read_reg(ov5693, address, &value);
>> +if (ret)
>> +return ret;
>> +
>> +value &= ~mask;
>> +value |= bits;
>> +
>> +ov5693_write_reg(ov5693, address, value, &ret);
> Shouldn't ret be initialized to 0 ?
Isn't that done by the return of ov5693_read_reg()?
>
>> +if (ret)
>> +return ret;
>> +
>> +return 0;
> You can just return ret I guess
Oops, yes. Thanks
>
>> +}
>> +
>> +/* V4L2 Controls Functions */
>> +
>> +static int ov5693_flip_vert_configure(struct ov5693_device *ov5693, bool
>> enable)
>> +{
>> +u8 bits = OV5693_FORMAT1_FLIP_VERT_ISP_EN |
>> + OV5693_FORMAT1_FLIP_VERT_SENSOR_EN;
>> +int ret;
>> +
>> +ret = ov5693_update_bits(ov5693, OV5693_FORMAT1_REG, bits,
>> + enable ? bits : 0);
>> +if (ret)
>> +return ret;
>> +
>> +return 0;
>> +}
>> +
>> +static int ov5693_flip_horz_configure(struct ov5693_device *ov5693, bool
>> enable)
>> +{
>> +u8 bits = OV5693_FORMAT2_FLIP_HORZ_ISP_EN |
>> + OV5693_FORMAT2_FLIP_HORZ_SENSOR_EN;
>> +int ret;
>> +
>> +ret = ov5693_update_bits(ov5693, OV5693_FORMAT2_REG, bits,
>> + enable ? bits : 0);
>> +if (ret)
>> +return ret;
>> +
>> +return 0;
>> +}
>> +
>> +static int ov5693_get_exposure(struct ov5693_device *ov5693, s32 *value)
>> +{
> Out of curiosity, what's the unit of the exposure for this chip ?
1/16ths of a line, but the 4 fractional bits are unsupported, so
effectively in units of lines.
>> +u8 exposure_hh = 0, exposure_h = 0, exposure_l = 0;
>> +int ret;
>> +
>> +ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_HH_REG,
>> &exposure_hh);
>> +if (ret)
>> +return ret;
>> +
>> +ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_H_REG,
>> &exposure_h);
>> +if (ret)
>> +return ret;
>> +
>> +ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_L_REG,
>> &exposure_l);
>> +if (ret)
>> +return ret;
>> +
>> +/* The lowest 4 bits are unsupported fractional bits */
>> +*value = ((exposure_hh << 16) | (exposure_h << 8) | exposure_l) >> 4;
>> +
>> +return 0;
>> +}
>> +
>> +static int ov5693_exposure_configure(struct ov5693_device *ov5693, u32
>> exposure)
>> +{
>> +int ret = 0;
> No intialization required here, it will be overwritten below.
Thanks
>> +
>> +gpiod_set_value_cansleep(ov5693->reset, 1);
>> +gpiod_set_value_cansleep(ov5693->powerdown, 1);
>> +
>> +ret = clk_prepare_enable(ov5693->clk);
>> +if (ret) {
>> +dev_err(ov5693->dev, "Failed to enable clk\n");
>> +goto fail_power;
>> +}
>> +
>> +ret = regulator_bulk_enable(OV5693_NUM_SUPPLIES, ov5693->supplies);
>> +i
[PATCH 0/1] Add support for OV5693 Sensor
Hello all This patch adds support for the OV5693 sensor found as the front camera in many Microsoft Surface devices, along with a number of similar style laptops. It is a heavily adapted derivative of the atomisp-ov5693 driver in staging, which retains most of the global register settings and some of the other functions from that driver, but otherwise uses the "normal" v4l2 infrastructure. There are 3 supported modes (down from 18 in the atomisp one!); 2592x1944, 1920x1080 and 1280x720. As we're targeting libcamera, all mandatory, recommended and optional controls for that library (at least, at time of writing) are supported. Daniel Scally (1): media: i2c: Add support for ov5693 sensor MAINTAINERS|7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/ov5693.c | 1585 4 files changed, 1604 insertions(+) create mode 100644 drivers/media/i2c/ov5693.c -- 2.25.1
[PATCH 1/1] media: i2c: Add support for ov5693 sensor
The OV5693 is a 5 Mpx CMOS image sensor, connected via MIPI CSI-2. The chip is capable of a single lane configuration, but currently only two lanes are supported. Most of the sensor's features are supported, with the main exception being the lens correction algorithm. The driver provides all mandatory, optional and recommended V4L2 controls for maximum compatibility with libcamera. Signed-off-by: Daniel Scally --- MAINTAINERS|7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/ov5693.c | 1585 4 files changed, 1604 insertions(+) create mode 100644 drivers/media/i2c/ov5693.c diff --git a/MAINTAINERS b/MAINTAINERS index cf44b3e77b90..34311d55b189 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13140,6 +13140,13 @@ S: Maintained T: git git://linuxtv.org/media_tree.git F: drivers/media/i2c/ov5675.c +OMNIVISION OV5693 SENSOR DRIVER +M: Daniel Scally +L: linux-me...@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: drivers/media/i2c/ov5693.c + OMNIVISION OV5695 SENSOR DRIVER M: Shunqian Zheng L: linux-me...@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 4c1ae687ab10..4da2278ec84c 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -985,6 +985,17 @@ config VIDEO_OV5675 To compile this driver as a module, choose M here: the module will be called ov5675. +config VIDEO_OV5693 + tristate "OmniVision OV5693 sensor support" + depends on I2C && VIDEO_V4L2 + select V4L2_FWNODE + help + This is a Video4Linux2 sensor driver for the OmniVision + OV5693 camera. + + To compile this driver as a module, choose M here: the + module will be called ov5693. + config VIDEO_OV5695 tristate "OmniVision OV5695 sensor support" depends on I2C && VIDEO_V4L2 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 65cfc94d25b6..7df680e110c9 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -75,6 +75,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o obj-$(CONFIG_VIDEO_OV5648) += ov5648.o obj-$(CONFIG_VIDEO_OV5670) += ov5670.o obj-$(CONFIG_VIDEO_OV5675) += ov5675.o +obj-$(CONFIG_VIDEO_OV5693) += ov5693.o obj-$(CONFIG_VIDEO_OV5695) += ov5695.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o obj-$(CONFIG_VIDEO_OV7251) += ov7251.o diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c new file mode 100644 index ..0460371164ea --- /dev/null +++ b/drivers/media/i2c/ov5693.c @@ -0,0 +1,1585 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2013 Intel Corporation. All Rights Reserved. + * + * Adapted from the atomisp-ov5693 driver, with contributions from: + * + * Daniel Scally + * Jean-Michel Hautbois + * Fabian Wuthrich + * Tsuchiya Yuto + * Jordan Hand + * Jake Day + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* System Control */ +#define OV5693_SW_RESET_REG0x0103 +#define OV5693_SW_STREAM_REG 0x0100 +#define OV5693_START_STREAMING 0x01 +#define OV5693_STOP_STREAMING 0x00 +#define OV5693_SW_RESET0x01 + +#define OV5693_REG_CHIP_ID_H 0x300A +#define OV5693_REG_CHIP_ID_L 0x300B +/* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */ +#define OV5693_CHIP_ID 0x5690 + +/* Exposure */ +#define OV5693_EXPOSURE_L_CTRL_HH_REG 0x3500 +#define OV5693_EXPOSURE_L_CTRL_H_REG 0x3501 +#define OV5693_EXPOSURE_L_CTRL_L_REG 0x3502 +#define OV5693_EXPOSURE_S_CTRL_HH_REG 0x3506 +#define OV5693_EXPOSURE_S_CTRL_H_REG 0x3507 +#define OV5693_EXPOSURE_S_CTRL_L_REG 0x3508 +#define OV5693_EXPOSURE_CTRL_HH(v) (((v) & GENMASK(14, 12)) >> 12) +#define OV5693_EXPOSURE_CTRL_H(v) (((v) & GENMASK(11, 4)) >> 4) +#define OV5693_EXPOSURE_CTRL_L(v) (((v) & GENMASK(3, 0)) << 4) +#define OV5693_INTEGRATION_TIME_MARGIN 8 +#define OV5693_EXPOSURE_MIN1 +#define OV5693_EXPOSURE_STEP 1 + +/* Analogue Gain */ +#define OV5693_GAIN_CTRL_H_REG 0x350A +#define OV5693_GAIN_CTRL_H(v) (((v) >> 4) & GENMASK(2, 0)) +#define OV5693_GAIN_CTRL_L_REG 0x350B +#define OV5693_GAIN_CTRL_L(v) (((v) << 4) & GENMASK(7, 4)) +#define OV5693_GAIN_MIN1 +#define OV5693_GAIN_MAX127 +#define OV5693_GAIN_DEF8 +#define OV5693
Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
Hi Rafael On 08/03/2021 17:23, Rafael J. Wysocki wrote: > On Mon, Mar 8, 2021 at 4:45 PM Rafael J. Wysocki wrote: >> On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko >> wrote: >>> On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote: >>>> On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko >>>> wrote: >>>>> On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally wrote: >>>>>> On 22/02/2021 13:34, Andy Shevchenko wrote: >>>>>>> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally >>>>>>> wrote: >>>>>>>> The acpi_walk_dep_device_list() is not as generalisable as its name >>>>>>>> implies, serving only to decrement the dependency count for each >>>>>>>> dependent device of the input. Extend the function to instead accept >>>>>>>> a callback which can be applied to all the dependencies in >>>>>>>> acpi_dep_list. >>>>>>>> Replace all existing calls to the function with calls to a wrapper, >>>>>>>> passing >>>>>>>> a callback that applies the same dependency reduction. >>>>>>> The code looks okay to me, if it was the initial idea, feel free to add >>>>>>> Reviewed-by: Andy Shevchenko >>> ... >>> >>>>>>>> +void acpi_dev_flag_dependency_met(acpi_handle handle) >>>>>>> Since it's acpi_dev_* namespace, perhaps it should take struct >>>>>>> acpi_device here? >>>>>> I can do this, but I avoided it because in most of the uses in the >>>>>> kernel currently there's no struct acpi_device, they're just passing >>>>>> ACPI_HANDLE(dev) instead, so I'd need to get the adev with >>>>>> ACPI_COMPANION() in each place. It didn't seem worth it... >>>> It may not even be possible sometimes, because that function may be >>>> called before creating all of the struct acpi_device objects (like in >>>> the case of deferred enumeration). >>>> >>>>>> but happy to >>>>>> do it if you'd prefer it that way? >>>>> I see, let Rafael decide then. I'm not pushing here. >>>> Well, it's a matter of correctness. >>> Looking at your above comment it is indeed. Thanks for clarification! >> Well, actually, the struct device for the object passed to this >> function should be there already, because otherwise it wouldn't make >> sense to update the list. So my comment above is not really >> applicable to this particular device and the function could take a >> struct acpi_device pointer argument. Sorry for the confusion. >> >>> But should we have acpi_dev_*() namespace for this function if it takes >>> handle? >> It takes a device object handle. >> >> Anyway, as per the above, it can take a struct acpi_device pointer >> argument in which case the "acpi_dev_" prefix should be fine. OK, so the conclusion there is change the argument to a struct acpi_device pointer and update all the uses. >>> For time being nothing better than following comes to my mind: >>> >>> __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met() >>> acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met() >> The above said, the name is somewhat confusing overall IMV. >> >> Something like acpi_dev_clear_dependencies() might be better. >> >> So lets make it something like >> >> void acpi_dev_clear_dependencies(struct acpi_device *supplier); > To be precise, there are two functions in the patch, > acpi_dev_flag_dependency_met() which invokes > acpi_walk_dep_device_list() and __acpi_dev_flag_dependency_met() > invoked by the latter as a callback. > > Above I was talking about the first one. > > The callback should still take a struct acpi_dep_data pointer argument > and I would call it acpi_scan_clear_dep() or similar. OK, works for me, I'll make those changes - thanks
Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
Hi Rafael
On 08/03/2021 17:46, Rafael J. Wysocki wrote:
>> +void acpi_walk_dep_device_list(acpi_handle handle,
>> + int (*callback)(struct acpi_dep_data *, void
>> *),
>> + void *data)
>> +{
>> + struct acpi_dep_data *dep, *tmp;
>> + int ret;
>> +
>> mutex_lock(&acpi_dep_list_lock);
>> list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
>> if (dep->supplier == handle) {
>> - acpi_bus_get_device(dep->consumer, &adev);
>> - if (!adev)
>> - continue;
>> -
>> - adev->dep_unmet--;
>> - if (!adev->dep_unmet)
>> - acpi_bus_attach(adev, true);
> The above code in the mainline has changed recently, so you need to
> rebase the above and adjust for the change of behavior.
Yeah, I'll rebase onto 5.12-rc2 before next submission.
>
>> -
>> - list_del(&dep->node);
>> - kfree(dep);
>> + ret = callback(dep, data);
>> + if (ret)
>> + break;
>> }
>> }
>> mutex_unlock(&acpi_dep_list_lock);
>> }
>> EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
>>
>> +/**
>> + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the
>> device
>> + * is now active
> No parens here, please, and make it fit one line.
>
> Also the description should be something like "Clear dependencies on
> the given device."
OK - no problem
Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
Hi Andy
On 22/02/2021 13:34, Andy Shevchenko wrote:
> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally wrote:
>> The acpi_walk_dep_device_list() is not as generalisable as its name
>> implies, serving only to decrement the dependency count for each
>> dependent device of the input. Extend the function to instead accept
>> a callback which can be applied to all the dependencies in acpi_dep_list.
>> Replace all existing calls to the function with calls to a wrapper, passing
>> a callback that applies the same dependency reduction.
> The code looks okay to me, if it was the initial idea, feel free to add
> Reviewed-by: Andy Shevchenko
Thank you!
>> + */
>> +void acpi_dev_flag_dependency_met(acpi_handle handle)
>> +{
> Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device
> here?
I can do this, but I avoided it because in most of the uses in the
kernel currently there's no struct acpi_device, they're just passing
ACPI_HANDLE(dev) instead, so I'd need to get the adev with
ACPI_COMPANION() in each place. It didn't seem worth it...but happy to
do it if you'd prefer it that way?
Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
Hi Hans On 04/03/2021 13:37, Hans de Goede wrote: > Hi, > > On 2/22/21 2:07 PM, Daniel Scally wrote: >> v1 for this series was originally 14-18 of this series: >> https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrsca...@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67 >> >> v2 was here: >> https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrsca...@gmail.com/ >> >> Series level changelog: >> >> - Dropped the patch moving acpi_lpss_dep() to utils since it's not used >> in acpi_dev_get_dependent_dev() anymore. >> - Replaced it with a patch extending acpi_walk_dep_device_list() to be >> able to apply a given callback against each device in acpi_dep_list >> - Dropped the patch creating acpi_i2c_dev_name() and simply open coded >> that functionality. >> >> This has been tested on a number of devices, but currently **not** on a >> device >> designed for ChromeOS, which we ideally need to do to ensure no regression >> caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any >> way >> you could help with that? Unfortunately, I don't have a device to test it on >> myself. >> >> Original cover letter: >> >> At the moment in the kernel the ACPI _HID INT3472 is taken by the tps68470 >> MFD driver, but that driver can only handle some of the cases of that _HID >> that we see. There are at least these three possibilities: >> >> 1. INT3472 devices that provide GPIOs through the usual framework and run >>power and clocks through an operation region; this is the situation that >>the current module handles and is seen on ChromeOS devices >> 2. INT3472 devices that provide GPIOs, plus clocks and regulators that are >>meant to be driven through the usual frameworks, usually seen on devices >>designed to run Windows >> 3. INT3472 devices that don't actually represent a physical tps68470, but >>are being used as a convenient way of grouping a bunch of system GPIO >>lines that are intended to enable power and clocks for sensors which >>are called out as dependent on them. Also seen on devices designed to >>run Windows. >> >> This series introduces a new module which registers: >> >> 1. An i2c driver that determines which scenario (#1 or #2) applies to the >>machine and registers platform devices to be bound to GPIO, OpRegion, >>clock and regulator drivers as appropriate. >> 2. A platform driver that binds to the dummy INT3472 devices described in >>#3 >> >> The platform driver for the dummy device registers the GPIO lines that >> enable the clocks and regulators to the sensors via those frameworks so >> that sensor drivers can consume them in the usual fashion. The existing >> GPIO and OpRegion tps68470 drivers will work with the i2c driver that's >> registered. Clock and regulator drivers are available but have not so far >> been >> tested, so aren't part of this series. >> >> The existing mfd/tps68470.c driver being thus superseded, it is removed. > Thank you for this patch series. Since there have already been a whole > bunch of review-comments, I've not taken a detailed look at this yet. No problem, I'm hoping to do a v3 over the weekend anyway. > I do wonder if you have thought about how this series should be merged? > This series is spread over quite a few subsytems and since there are > various interdependencies in the patches it is probably best if it gets > merged in its entirety through a single tree. > > I guess that merging though either Rafael's (drivers/acpi) tree or > Lee's (drivers/mfd) tree makes the most sense. > > As drivers/platform/x86 maintainer I'm happy with whatever solution > works for the other subsystem maintainers. I also think it's a good idea to go through a single tree, and my plan was to raise that probably after the next review round or so, but I hadn't gotten as far as thinking about whos tree it should be or anything yet. To be honest I'm not sure what factors dictate which choice is best in that regard; handling complex git merges is a bit outside my experience. > > Regards, > > Hans > > > > >> Thanks >> Dan >> >> Daniel Scally (6): >> ACPI: scan: Extend acpi_walk_dep_device_list() >> ACPI: scan: Add function to fetch dependent of acpi device >> i2c: core: Add a format macro for I2C device names >> gpiolib: acpi: Export acpi_get_gpiod() >> platform/x86: Add intel_skl_int3472 driver >> mfd: t
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
Hi Andy, Laurent On 24/02/2021 10:18, Andy Shevchenko wrote: > On Wed, Feb 24, 2021 at 12:16 PM Laurent Pinchart > wrote: >> On Tue, Feb 23, 2021 at 10:36:18PM +0000, Daniel Scally wrote: >>> On 23/02/2021 20:04, Laurent Pinchart wrote: > ... > >>>>> + get_device(&int3472->sensor->dev); >>>> I see no corresponding put_device(), am I missing something ? I'm also >>>> not sure why this is needed. >>> The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems >>> to be no acpi_dev_get() for some reason. We use the sensor acpi_device >>> to get the clock frequency, and to fetch the sensor module string, so I >>> thought it ought to hold a reference on those grounds. >> Shouldn't acpi_dev_get_dependent_dev() increase the reference count >> then, instead of doing it manually here ? > That's what I expected as well. > We have plenty of acpi_dev_get_*() and they do increase the reference > counter one way or the other. > Okedokey, I'll move the get() to that function and drop it from here.
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
Hi Laurent
On 23/02/2021 20:04, Laurent Pinchart wrote:
> +
> +/*
> + * Here follows platform specific mapping information that we can pass to
> + * the functions mapping resources to the sensors. Where the sensors have
> + * a power enable pin defined in DSDT we need to provide a supply name so
> + * the sensor drivers can find the regulator. The device name will be derived
> + * from the sensor's ACPI device within the code. Optionally, we can provide
> a
> + * NULL terminated array of function name mappings to deal with any platform
> + * specific deviations from the documented behaviour of GPIOs.
> + *
> + * Map a GPIO function name to NULL to prevent the driver from mapping that
> + * GPIO at all.
> + */
> +
> +static const struct int3472_gpio_function_remap
> ov2680_gpio_function_remaps[] = {
> + { "reset", NULL },
> + { "powerdown", "reset" },
> + { }
> +};
> +
> +static struct int3472_sensor_config int3472_sensor_configs[] = {
> This should be static const (and there will be some fallout due to that,
> as skl_int3472_register_regulator() modifies the supply_map, so I think
> you'll have a copy of supply_map in int3472_discrete_device).
Ack to all of the constness; you mentioned that last time too - not sure
how I missed doing those! I think I can just having a local struct
regulator_consumer_supply in skl_int3472_register_regulator and fill it
from int3472->sensor_config.supply_map
>> +static unsigned int skl_int3472_get_clk_frequency(struct
>> int3472_discrete_device *int3472)
>> +{
>> +union acpi_object *obj;
>> +unsigned int ret = 0;
>> +
>> +obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
>> +if (IS_ERR(obj))
>> +return 0; /* report rate as 0 on error */
>> +
>> +if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET +
>> sizeof(u32)) {
> Should we define an ssdb structure instead of peeking into the buffer
> with an offset ?
I thought about that, but in the end decided it didn't seem worth
defining the whole SSDB structure just to use one field. Particularly
since we use it in cio2-bridge already, so if we're going to do that it
really ought to just live in a header that's included in both - and that
seemed even less worthwhile.
I don't have a strong feeling though, so if you think it's better to
define the struct I'm happy to.
>> +static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> +struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> +struct int3472_discrete_device *int3472 = to_int3472_device(clk);
>> +
>> +return int3472->clock.frequency;
> Maybe just
>
> struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>
> return clk->frequency;
Oops, of course.
>> +static int skl_int3472_register_regulator(struct int3472_discrete_device
>> *int3472,
>> + struct acpi_resource *ares)
>> +{
>> +char *path = ares->data.gpio.resource_source.string_ptr;
>> +struct int3472_sensor_config *sensor_config;
>> +struct regulator_init_data init_data = { };
>> +struct regulator_config cfg = { };
>> +int ret;
>> +
>> +sensor_config = int3472->sensor_config;
>> +if (IS_ERR_OR_NULL(sensor_config)) {
>> +dev_err(int3472->dev, "No sensor module config\n");
>> +return PTR_ERR(sensor_config);
>> +}
>> +
>> +if (!sensor_config->supply_map.supply) {
>> +dev_err(int3472->dev, "No supply name defined\n");
>> +return -ENODEV;
>> +}
>> +
>> +init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>> +init_data.num_consumer_supplies = 1;
>> +sensor_config->supply_map.dev_name = int3472->sensor_name;
>> +init_data.consumer_supplies = &sensor_config->supply_map;
>> +
>> +snprintf(int3472->regulator.regulator_name,
>> + sizeof(int3472->regulator.regulator_name), "%s-regulator",
>> + acpi_dev_name(int3472->adev));
>> +snprintf(int3472->regulator.supply_name,
>> + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
>> +
>> +int3472->regulator.rdesc = INT3472_REGULATOR(
>> +
>> int3472->regulator.regulator_name,
>> +int3472->regulator.supply_name,
>> +&int3472_gpio_regulator_ops);
>> +
>> +int3472->regulator.gpio = acpi_get_gpiod(path,
>> + ares->data.gpio.pin_table[0],
>> + "int3472,regulator");
>> +if (IS_ERR(int3472->regulator.gpio)) {
>> +dev_err(int3472->dev, "Failed to get regulator GPIO lines\n");
> s/lines/line/ (sorry, it was a typo in my review of v2)
No problem!
>> +static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
>> +{
>> +struct list_head resource_list;
>> +in
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
On 23/02/2021 12:01, Andy Shevchenko wrote: + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) + return 1; /* Deliberately positive so parsing continues */ >>> I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e. >>> spreading it over kernel code (yes, I know about one existing TS >>> case). >>> Consider to provide a helper in analogue to acpi_gpio_get_irq_resource(). >> Sure, but I probably name it acpi_gpio_is_io_resource() - a function >> named "get" which returns a bool seems a bit funny to me. > But don't you need the resource itself? > > You may extract and check resource at the same time as > acpi_gpio_get_irq_resource() does. Oh! Reading comprehension fail; I didn't notice it was returning the pointer through agpio; you're right of course. > > ... > + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); + if (int3472->gpios.dev_id) + gpiod_remove_lookup_table(&int3472->gpios); >>> gpiod_remove_lookup_table() is now NULL-aware. >>> But in any case I guess you don't need the above check. >> Sorry; forgot to call out that I didn't follow that suggestion; >> int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios >> won't be NULL, even if I haven't filled anything in to there yet because >> it failed before it got to that point. So, not sure that it quite works >> there. > I think if you initialize the ->list member you can remove without check. I'll give that a try - thanks >
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
Hi Hans, thanks for the input On 22/02/2021 13:27, Hans de Goede wrote: > Hi, > > On 2/22/21 2:19 PM, Daniel Scally wrote: >> Hi all >> >> On 22/02/2021 13:07, Daniel Scally wrote: >>> diff --git a/drivers/platform/x86/intel-int3472/Kconfig >>> b/drivers/platform/x86/intel-int3472/Kconfig >>> new file mode 100644 >>> index ..b94622245c21 >>> --- /dev/null >>> +++ b/drivers/platform/x86/intel-int3472/Kconfig >>> @@ -0,0 +1,31 @@ >>> +config INTEL_SKL_INT3472 >>> + tristate "Intel SkyLake ACPI INT3472 Driver" >>> + depends on ACPI >>> + depends on REGULATOR >>> + depends on GPIOLIB >>> + depends on COMMON_CLK && CLKDEV_LOOKUP >>> + depends on I2C >>> + select MFD_CORE >>> + select REGMAP_I2C >>> + help >>> + This driver adds support for the INT3472 ACPI devices found on some >>> + Intel SkyLake devices. >>> + >>> + The INT3472 is an Intel camera power controller, a logical device >>> + found on some Skylake-based systems that can map to different >>> + hardware devices depending on the platform. On machines >>> + designed for Chrome OS, it maps to a TPS68470 camera PMIC. On >>> + machines designed for Windows, it maps to either a TP68470 >>> + camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs >>> + and power gates. >>> + >>> + If your device was designed for Chrome OS, this driver will provide >>> + an ACPI OpRegion, which must be available before any of the devices >>> + using it are probed. For this reason, you should select Y if your >>> + device was designed for ChromeOS. For the same reason the >>> + I2C_DESIGNWARE_PLATFORM option must be set to Y too. >>> + >>> + Say Y or M here if you have a SkyLake device designed for use >>> + with Windows or ChromeOS. Say N here if you are not sure. >>> + >>> + The module will be named "intel-skl-int3472" >> The Kconfig option for the existing tps68470 driver is a bool which >> depends on I2C_DESIGNWARE_PLATFORM=y, giving the following reason: >> >> This option is a bool as it provides an ACPI operation >> region, which must be available before any of the devices >> using this are probed. This option also configures the >> designware-i2c driver to be built-in, for the same reason. >> >> One problem I've faced is that that scenario only applies to some >> devices that this new driver can support, so hard-coding it as built in >> didn't make much sense. For that reason I opted to set it tristate, but >> of course that issue still exists for ChromeOS devices where the >> OpRegion will be registered. I opted for simply documenting that >> requirement, as is done in aaac4a2eadaa6: "mfd: axp20x-i2c: Document >> that this must be builtin on x86", but that's not entirely satisfactory. >> Possible alternatives might be setting "depends on >> I2C_DESIGNWARE_PLATFORM=y if CHROME_PLATFORMS" or something similar, >> though of course the User would still have to realise they need to >> build-in the INTEL_SKL_INT3472 Kconfig option too. >> >> Feedback around this issue would be particularly welcome, as I'm not >> sure what the best approach might be. > This is a tricky area, I actually wrote the "mfd: axp20x-i2c: Document > that this must be builtin on x86" patch you refer to. At first I tried > to express the dependency in Kconfig language but things got too complex > and Kconfig sometimes became unhappy about circular deps (or something > like that). Yes, I had a go too; with similar results > The most important thing here is to make sure that the generic configs > shipped by distros get this right; and we can hope that people creating > those configs at least read the help text... > > So all in all I believe that just documenting the requirement is fine. OK - that's what I'm hoping is the consensus, as I don't think it can be made _entirely_ seamless through dependencies or whatever anyway, in which case documenting it seems like the cleanest approach to me. > > The alternative would be to just change I2C_DESIGNWARE_PLATFORM (and the > core) to a bool, or at least make it not selectable as module when > X86 and ACPI are set... That would be a bit of a big hammer but might > not be the worst idea actually. > > Regards, > > Hans >
Re: [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
On 22/02/2021 14:12, Andy Shevchenko wrote: > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally wrote: >> This driver only covered one scenario in which ACPI devices with _HID >> INT3472 are found, and its functionality has been taken over by the >> intel-skl-int3472 module, so remove it. > As long as patch 5 accepted > Acked-by: Andy Shevchenko Great - thank you! And likewise for the R-bs on the previous patches, I'll follow all the comments for those
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
Hi Andy - thanks for comments!
On 22/02/2021 14:58, Andy Shevchenko wrote:
> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally wrote:
>> ACPI devices with _HID INT3472 are currently matched to the tps68470
>> driver, however this does not cover all situations in which that _HID
>> occurs. We've encountered three possibilities:
>>
>> 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
>> a physical TPS68470 device) that requires a GPIO and OpRegion driver
>> 2. On devices designed for Windows, an ACPI device with _HID INT3472
>> (again representing a physical TPS68470 device) which requires GPIO,
>> Clock and Regulator drivers.
>> 3. On other devices designed for Windows, an ACPI device with _HID
>> INT3472 which does **not** represent a physical TPS68470, and is instead
>> used as a dummy device to group some system GPIO lines which are meant
>> to be consumed by the sensor that is dependent on this entry.
>>
>> This commit adds a new module, registering a platform driver to deal
>> with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
>> querying the CLDB buffer found against INT3472 entries to determine
>> which is most appropriate.
> Can you split CLK parts (and maybe regulators as well) to something
> like intel_skl_int3472_clk.c?
Sure, no problem
>
>> +#include
>> +#include
>> +#include
>> +#include
>> + dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
> Perhaps acpi_handle_err() et al. instead of dev_*(&adev->dev, ...)
> where it's applicable?
Ah - yes, ok, thanks. TIL those exist
>> + if (obj->buffer.length > sizeof(*cldb)) {
>> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
>> + ret = -EINVAL;
> ENOSPC? ENOMEM?
I still think EINVAL actually, as in this case the problem isn't that
space couldn't be allocated but that the buffer in the SSDB is larger
than I expect it to be, which means the definition of it has changed /
this device isn't actually supported.
>> + ret = platform_driver_register(&int3472_discrete);
>> + if (ret)
>> + return ret;
>> +
>> + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
>> + if (ret)
>> + platform_driver_unregister(&int3472_discrete);
> Not a fan of the above, but let's see what others will say...
Yeah; happy to discuss this more if needed.
>> +#include
> This is definitely not for *.h. (Not all C files needed this)
>
>> +#include
> Ditto.
>
>> +#include
>> +#include
> Ditto.
Yep; I'll move them to *_clk.c and *_regulator.c files.
>> +static int skl_int3472_clk_prepare(struct clk_hw *hw)
>> +{
>> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> +
>> + gpiod_set_value(clk->ena_gpio, 1);
>> + if (clk->led_gpio)
> Make it optional and drop this check. Same for other places of use of this
> GPIO.
Oops, of course, thanks
>> +static int skl_int3472_clk_enable(struct clk_hw *hw)
>> +{
>> + /*
>> +* We're just turning a GPIO on to enable, which operation has the
>> +* potential to sleep. Given enable cannot sleep, but prepare can,
>> +* we toggle the GPIO in prepare instead. Thus, nothing to do here.
>> +*/
> Missed . and / or () in some words? (Describing callbacks, personally
> I use the form "->callback()" in such cases)
OK, I'll fix the comment to match that style.
>> +static unsigned int skl_int3472_get_clk_frequency(struct
>> int3472_discrete_device *int3472)
>> +{
>> + union acpi_object *obj;
>>
>> + unsigned int ret = 0;
> unsigned for ret is unusual. Looking into the code, first of all it
> doesn't need this assignment; second, it probably can gain a better
> name: "frequency"?
Yep ok, I'll rename to freq/frequency
>> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
> Hmm...
>
> Would
>
> if (IS_ERR_OR_NULL(sensor_config))
> return 0;
>
> if (!_maps)
> return 0;
>
> with respective comments working here?
No, because the absence of either sensor_config or
sensor_config->function_maps is not a failure mode. We only need to
provide sensor_configs for some platforms, and function_maps for even
fewer. So if that check is false, the rest of the function should still
execute.
>> +static int skl_int3472_register_clock(struct int3472_discrete_device
>> *int3472)
>>
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
Hi all On 22/02/2021 13:07, Daniel Scally wrote: > diff --git a/drivers/platform/x86/intel-int3472/Kconfig > b/drivers/platform/x86/intel-int3472/Kconfig > new file mode 100644 > index ..b94622245c21 > --- /dev/null > +++ b/drivers/platform/x86/intel-int3472/Kconfig > @@ -0,0 +1,31 @@ > +config INTEL_SKL_INT3472 > + tristate "Intel SkyLake ACPI INT3472 Driver" > + depends on ACPI > + depends on REGULATOR > + depends on GPIOLIB > + depends on COMMON_CLK && CLKDEV_LOOKUP > + depends on I2C > + select MFD_CORE > + select REGMAP_I2C > + help > + This driver adds support for the INT3472 ACPI devices found on some > + Intel SkyLake devices. > + > + The INT3472 is an Intel camera power controller, a logical device > + found on some Skylake-based systems that can map to different > + hardware devices depending on the platform. On machines > + designed for Chrome OS, it maps to a TPS68470 camera PMIC. On > + machines designed for Windows, it maps to either a TP68470 > + camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs > + and power gates. > + > + If your device was designed for Chrome OS, this driver will provide > + an ACPI OpRegion, which must be available before any of the devices > + using it are probed. For this reason, you should select Y if your > + device was designed for ChromeOS. For the same reason the > + I2C_DESIGNWARE_PLATFORM option must be set to Y too. > + > + Say Y or M here if you have a SkyLake device designed for use > + with Windows or ChromeOS. Say N here if you are not sure. > + > + The module will be named "intel-skl-int3472" The Kconfig option for the existing tps68470 driver is a bool which depends on I2C_DESIGNWARE_PLATFORM=y, giving the following reason: This option is a bool as it provides an ACPI operation region, which must be available before any of the devices using this are probed. This option also configures the designware-i2c driver to be built-in, for the same reason. One problem I've faced is that that scenario only applies to some devices that this new driver can support, so hard-coding it as built in didn't make much sense. For that reason I opted to set it tristate, but of course that issue still exists for ChromeOS devices where the OpRegion will be registered. I opted for simply documenting that requirement, as is done in aaac4a2eadaa6: "mfd: axp20x-i2c: Document that this must be builtin on x86", but that's not entirely satisfactory. Possible alternatives might be setting "depends on I2C_DESIGNWARE_PLATFORM=y if CHROME_PLATFORMS" or something similar, though of course the User would still have to realise they need to build-in the INTEL_SKL_INT3472 Kconfig option too. Feedback around this issue would be particularly welcome, as I'm not sure what the best approach might be.
Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
+cc Andy, who's email address I managed to screw up - sorry. On 22/02/2021 13:07, Daniel Scally wrote: > v1 for this series was originally 14-18 of this series: > https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrsca...@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67 > > v2 was here: > https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrsca...@gmail.com/ > > Series level changelog: > > - Dropped the patch moving acpi_lpss_dep() to utils since it's not used > in acpi_dev_get_dependent_dev() anymore. > - Replaced it with a patch extending acpi_walk_dep_device_list() to be > able to apply a given callback against each device in acpi_dep_list > - Dropped the patch creating acpi_i2c_dev_name() and simply open coded > that functionality. > > This has been tested on a number of devices, but currently **not** on a device > designed for ChromeOS, which we ideally need to do to ensure no regression > caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way > you could help with that? Unfortunately, I don't have a device to test it on > myself. > > Original cover letter: > > At the moment in the kernel the ACPI _HID INT3472 is taken by the tps68470 > MFD driver, but that driver can only handle some of the cases of that _HID > that we see. There are at least these three possibilities: > > 1. INT3472 devices that provide GPIOs through the usual framework and run >power and clocks through an operation region; this is the situation that >the current module handles and is seen on ChromeOS devices > 2. INT3472 devices that provide GPIOs, plus clocks and regulators that are >meant to be driven through the usual frameworks, usually seen on devices >designed to run Windows > 3. INT3472 devices that don't actually represent a physical tps68470, but >are being used as a convenient way of grouping a bunch of system GPIO >lines that are intended to enable power and clocks for sensors which >are called out as dependent on them. Also seen on devices designed to >run Windows. > > This series introduces a new module which registers: > > 1. An i2c driver that determines which scenario (#1 or #2) applies to the >machine and registers platform devices to be bound to GPIO, OpRegion, >clock and regulator drivers as appropriate. > 2. A platform driver that binds to the dummy INT3472 devices described in >#3 > > The platform driver for the dummy device registers the GPIO lines that > enable the clocks and regulators to the sensors via those frameworks so > that sensor drivers can consume them in the usual fashion. The existing > GPIO and OpRegion tps68470 drivers will work with the i2c driver that's > registered. Clock and regulator drivers are available but have not so far been > tested, so aren't part of this series. > > The existing mfd/tps68470.c driver being thus superseded, it is removed. > > Thanks > Dan > > Daniel Scally (6): > ACPI: scan: Extend acpi_walk_dep_device_list() > ACPI: scan: Add function to fetch dependent of acpi device > i2c: core: Add a format macro for I2C device names > gpiolib: acpi: Export acpi_get_gpiod() > platform/x86: Add intel_skl_int3472 driver > mfd: tps68470: Remove tps68470 MFD driver > > MAINTAINERS | 5 + > drivers/acpi/ec.c | 2 +- > drivers/acpi/pmic/Kconfig | 2 +- > drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +- > drivers/acpi/scan.c | 92 ++- > drivers/gpio/Kconfig | 2 +- > drivers/gpio/gpiolib-acpi.c | 38 +- > drivers/i2c/i2c-core-acpi.c | 2 +- > drivers/i2c/i2c-core-base.c | 4 +- > drivers/mfd/Kconfig | 18 - > drivers/mfd/Makefile | 1 - > drivers/mfd/tps68470.c| 97 --- > drivers/platform/surface/surface3_power.c | 2 +- > drivers/platform/x86/Kconfig | 2 + > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/intel-int3472/Kconfig| 31 + > drivers/platform/x86/intel-int3472/Makefile | 4 + > .../intel-int3472/intel_skl_int3472_common.c | 106 > .../intel-int3472/intel_skl_int3472_common.h | 110 > .../intel_skl_int3472_discrete.c | 592 ++ > .../intel_skl_int3472_tps68470.c | 113 > include/acpi/acpi_bus.h | 8 + > include/linux/acpi.h | 4 +- > include/linux/gpio/consumer.h
[PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
This driver only covered one scenario in which ACPI devices with _HID
INT3472 are found, and its functionality has been taken over by the
intel-skl-int3472 module, so remove it.
Reviewed-by: Laurent Pinchart
Signed-off-by: Daniel Scally
---
Changes in v3:
- Replaced Kconfig dependencies with INTEL_SKL_INT3472 for the tps68470
OpRegion and GPIO drivers.
drivers/acpi/pmic/Kconfig | 2 +-
drivers/gpio/Kconfig | 2 +-
drivers/mfd/Kconfig | 18
drivers/mfd/Makefile | 1 -
drivers/mfd/tps68470.c| 97 ---
5 files changed, 2 insertions(+), 118 deletions(-)
delete mode 100644 drivers/mfd/tps68470.c
diff --git a/drivers/acpi/pmic/Kconfig b/drivers/acpi/pmic/Kconfig
index 56bbcb2ce61b..f84b8f6038dc 100644
--- a/drivers/acpi/pmic/Kconfig
+++ b/drivers/acpi/pmic/Kconfig
@@ -52,7 +52,7 @@ endif # PMIC_OPREGION
config TPS68470_PMIC_OPREGION
bool "ACPI operation region support for TPS68470 PMIC"
- depends on MFD_TPS68470
+ depends on INTEL_SKL_INT3472
help
This config adds ACPI operation region support for TI TPS68470 PMIC.
TPS68470 device is an advanced power management unit that powers
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c70f46e80a3b..998898c72af8 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1343,7 +1343,7 @@ config GPIO_TPS65912
config GPIO_TPS68470
bool "TPS68470 GPIO"
- depends on MFD_TPS68470
+ depends on INTEL_SKL_INT3472
help
Select this option to enable GPIO driver for the TPS68470
chip family.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bdfce7b15621..9a1f648efde0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1520,24 +1520,6 @@ config MFD_TPS65217
This driver can also be built as a module. If so, the module
will be called tps65217.
-config MFD_TPS68470
- bool "TI TPS68470 Power Management / LED chips"
- depends on ACPI && PCI && I2C=y
- depends on I2C_DESIGNWARE_PLATFORM=y
- select MFD_CORE
- select REGMAP_I2C
- help
- If you say yes here you get support for the TPS68470 series of
- Power Management / LED chips.
-
- These include voltage regulators, LEDs and other features
- that are often used in portable devices.
-
- This option is a bool as it provides an ACPI operation
- region, which must be available before any of the devices
- using this are probed. This option also configures the
- designware-i2c driver to be built-in, for the same reason.
-
config MFD_TI_LP873X
tristate "TI LP873X Power Management IC"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 14fdb188af02..5994e812f479 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -105,7 +105,6 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
-obj-$(CONFIG_MFD_TPS68470) += tps68470.o
obj-$(CONFIG_MFD_TPS80031) += tps80031.o
obj-$(CONFIG_MENELAUS) += menelaus.o
diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
deleted file mode 100644
index 4a4df4ffd18c..
--- a/drivers/mfd/tps68470.c
+++ /dev/null
@@ -1,97 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * TPS68470 chip Parent driver
- *
- * Copyright (C) 2017 Intel Corporation
- *
- * Authors:
- * Rajmohan Mani
- * Tianshu Qiu
- * Jian Xu Zheng
- * Yuning Pu
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-static const struct mfd_cell tps68470s[] = {
- { .name = "tps68470-gpio" },
- { .name = "tps68470_pmic_opregion" },
-};
-
-static const struct regmap_config tps68470_regmap_config = {
- .reg_bits = 8,
- .val_bits = 8,
- .max_register = TPS68470_REG_MAX,
-};
-
-static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
-{
- unsigned int version;
- int ret;
-
- /* Force software reset */
- ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
- if (ret)
- return ret;
-
- ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
- if (ret) {
- dev_err(dev, "Failed to read revision register: %d\n", ret);
- return ret;
- }
-
- dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
-
- return 0;
-}
-
-static int tps68470_probe(struct i2c_client *client)
-{
- struct device *dev = &client->dev;
- struct regmap *regmap;
- int ret;
-
- regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
- if (IS_ERR(reg
[PATCH v3 3/6] i2c: core: Add a format macro for I2C device names
Some places in the kernel allow users to map resources to a device
using device name (for example, in the struct gpiod_lookup_table).
Currently this involves waiting for the I2C client to have been registered
so we can use dev_name(&client->dev). We want to add a function to allow
users to refer to an I2C device by name before it has been instantiated,
so create a macro for the format that's accessible outside the I2C layer
and use it in i2c_dev_set_name().
Suggested-by: Andy Shevchenko
Reviewed-by: Laurent Pinchart
Reviewed-by: Sakari Ailus
Reviewed-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
Changes in v3:
- None
drivers/i2c/i2c-core-base.c | 4 ++--
include/linux/i2c.h | 3 +++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf722a424..547b8926cac8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -811,12 +811,12 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
struct acpi_device *adev = ACPI_COMPANION(&client->dev);
if (info && info->dev_name) {
- dev_set_name(&client->dev, "i2c-%s", info->dev_name);
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name);
return;
}
if (adev) {
- dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT,
acpi_dev_name(adev));
return;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b215..4d40a4b46810 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -39,6 +39,9 @@ enum i2c_slave_event;
typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
enum i2c_slave_event event, u8 *val);
+/* I2C Device Name Format - to maintain consistency outside the i2c layer */
+#define I2C_DEV_NAME_FORMAT"i2c-%s"
+
/* I2C Frequency Modes */
#define I2C_MAX_STANDARD_MODE_FREQ 10
#define I2C_MAX_FAST_MODE_FREQ 40
--
2.25.1
[PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod()
I need to be able to translate GPIO resources in an ACPI device's _CRS
into GPIO descriptor array. Those are represented in _CRS as a pathname
to a GPIO device plus the pin's index number: this function is perfect
for that purpose.
As it's currently only used internally within the GPIO layer, provide and
export a wrapper function that additionally holds a reference to the GPIO
device.
Signed-off-by: Daniel Scally
---
Changes in v3:
- Having realised that it wasn't taking a reference to the GPIO device,
I decided the best thing to do was leave the existing function as-is
(apart from renaming) and provide a wrapper that simply passes
through to the original and takes a reference before returning the
struct gpio_desc
Because of the change to that functionality, I dropped the R-b's from
the last version.
drivers/gpio/gpiolib-acpi.c | 36 +++
include/linux/gpio/consumer.h | 7 +++
2 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e4d728fda982..0cc7cc327757 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -102,7 +102,8 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void
*data)
}
/**
- * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
GPIO API
+ * __acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
+ * GPIO API
* @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
* @pin: ACPI GPIO pin number (0-based, controller-relative)
*
@@ -111,7 +112,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void
*data)
* controller does not have GPIO chip registered at the moment. This is to
* support probe deferral.
*/
-static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+static struct gpio_desc *__acpi_get_gpiod(char *path, int pin)
{
struct gpio_chip *chip;
acpi_handle handle;
@@ -128,6 +129,33 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int
pin)
return gpiochip_get_desc(chip, pin);
}
+/**
+ * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
+ * GPIO API, and hold a refcount to the GPIO device.
+ * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
+ * @pin: ACPI GPIO pin number (0-based, controller-relative)
+ * @label: Label to pass to gpiod_request()
+ *
+ * This function is a simple pass-through to __acpi_get_gpiod(), except that as
+ * it is intended for use outside of the GPIO layer (in a similar fashion to
+ * gpiod_get_index() for example) it also holds a reference to the GPIO device.
+ */
+struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label)
+{
+ struct gpio_desc *gpio = __acpi_get_gpiod(path, pin);
+ int ret;
+
+ if (IS_ERR(gpio))
+ return gpio;
+
+ ret = gpiod_request(gpio, label);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return gpio;
+}
+EXPORT_SYMBOL_GPL(acpi_get_gpiod);
+
static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
{
struct acpi_gpio_event *event = data;
@@ -689,8 +717,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource
*ares, void *data)
if (pin_index >= agpio->pin_table_length)
return 1;
- lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
- agpio->pin_table[pin_index]);
+ lookup->desc =
__acpi_get_gpiod(agpio->resource_source.string_ptr,
+ agpio->pin_table[pin_index]);
lookup->info.pin_config = agpio->pin_config;
lookup->info.debounce = agpio->debounce_timeout;
lookup->info.gpioint = gpioint;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index ef49307611d2..6eee751f44dd 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -690,6 +690,8 @@ int devm_acpi_dev_add_driver_gpios(struct device *dev,
const struct acpi_gpio_mapping *gpios);
void devm_acpi_dev_remove_driver_gpios(struct device *dev);
+struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label);
+
#else /* CONFIG_GPIOLIB && CONFIG_ACPI */
struct acpi_device;
@@ -708,6 +710,11 @@ static inline int devm_acpi_dev_add_driver_gpios(struct
device *dev,
}
static inline void devm_acpi_dev_remove_driver_gpios(struct device *dev) {}
+struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label)
+{
+ return NULL;
+}
+
#endif /* CONFIG_GPIOLIB && CONFIG_ACPI */
--
2.25.1
[PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
ACPI devices with _HID INT3472 are currently matched to the tps68470 driver, however this does not cover all situations in which that _HID occurs. We've encountered three possibilities: 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing a physical TPS68470 device) that requires a GPIO and OpRegion driver 2. On devices designed for Windows, an ACPI device with _HID INT3472 (again representing a physical TPS68470 device) which requires GPIO, Clock and Regulator drivers. 3. On other devices designed for Windows, an ACPI device with _HID INT3472 which does **not** represent a physical TPS68470, and is instead used as a dummy device to group some system GPIO lines which are meant to be consumed by the sensor that is dependent on this entry. This commit adds a new module, registering a platform driver to deal with the 3rd scenario plus an i2c driver to deal with #1 and #2, by querying the CLDB buffer found against INT3472 entries to determine which is most appropriate. Suggested-by: Laurent Pinchart Signed-off-by: Daniel Scally --- Changes in v3: - Added clk_recalc_rate() operation to the clk that's registered, plus some associated functions / revisions. - Moved source and header files to their own folder within platform/x86 - Switched the GPIO toggling for the clock from enable/disable to prepare/unprepare to avoid any sleep problems - Switched handling of the privacy LED GPIO from map-to-sensor to being toggled along with the clk enable GPIO; register the clock in intel_skl_int3472_parse_crs() instead of during handle_gpio_resources - Better commenting in a lot of places - Used the sensor_name formed from acpi_dev_name(int3472->sensor) and the i2c name format macro as the dev_id in regulator init data rather than hardcoding an instance name. - Fetched the sensor module config a single time rather than once per GPIO - Switched int3472-tps68470 driver to use MFD framework properly rather than open coding the same functionality - A myriad of other fixes too minor to call out. Suggested changes (from Andy) that I didn't follow: - Using clk-gpio.c as a library: The requirement to add clk_recalc_rate so that clk_get_rate() would be supported, along with driving the privacy LED on clk_enable() meant this wouldn't work I think - Leave the MFD driver in its usual place, and prevent the INT3472 from probing as an i2c device from ACPI. I definitely see the argument for this, but in the end I think probably having all the code for the HID within a single place is probably a bit preferable. MAINTAINERS | 5 + drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel-int3472/Kconfig| 31 + drivers/platform/x86/intel-int3472/Makefile | 4 + .../intel-int3472/intel_skl_int3472_common.c | 106 .../intel-int3472/intel_skl_int3472_common.h | 110 .../intel_skl_int3472_discrete.c | 592 ++ .../intel_skl_int3472_tps68470.c | 113 9 files changed, 964 insertions(+) create mode 100644 drivers/platform/x86/intel-int3472/Kconfig create mode 100644 drivers/platform/x86/intel-int3472/Makefile create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c diff --git a/MAINTAINERS b/MAINTAINERS index a091b496fdd8..cf44b3e77b90 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9147,6 +9147,11 @@ S: Maintained F: arch/x86/include/asm/intel_scu_ipc.h F: drivers/platform/x86/intel_scu_* +INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER +M: Daniel Scally +S: Maintained +F: drivers/platform/x86/intel-int3472/intel_skl_int3472_* + INTEL SPEED SELECT TECHNOLOGY M: Srinivas Pandruvada L: platform-driver-...@vger.kernel.org diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 91e6176cdfbd..9739d30951b6 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -844,6 +844,8 @@ config INTEL_CHT_INT33FE device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m for Type-C device. +source "drivers/platform/x86/intel-int3472/Kconfig" + config INTEL_HID_EVENT tristate "INTEL HID Event" depends on ACPI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 581475f59819..2293b6c3d1c2 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@
[PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
The acpi_walk_dep_device_list() is not as generalisable as its name
implies, serving only to decrement the dependency count for each
dependent device of the input. Extend the function to instead accept
a callback which can be applied to all the dependencies in acpi_dep_list.
Replace all existing calls to the function with calls to a wrapper, passing
a callback that applies the same dependency reduction.
Suggested-by: Rafael J. Wysocki
Signed-off-by: Daniel Scally
---
Changes in v3:
- patch introduced
drivers/acpi/ec.c | 2 +-
drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
drivers/acpi/scan.c | 58 ---
drivers/gpio/gpiolib-acpi.c | 2 +-
drivers/i2c/i2c-core-acpi.c | 2 +-
drivers/platform/surface/surface3_power.c | 2 +-
include/acpi/acpi_bus.h | 7 +++
include/linux/acpi.h | 4 +-
8 files changed, 55 insertions(+), 24 deletions(-)
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13565629ce0a..a258db713bd2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1627,7 +1627,7 @@ static int acpi_ec_add(struct acpi_device *device)
WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
/* Reprobe devices depending on the EC */
- acpi_walk_dep_device_list(ec->handle);
+ acpi_dev_flag_dependency_met(ec->handle);
acpi_handle_debug(ec->handle, "enumerated.\n");
return 0;
diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
index a5101b07611a..59cca504325e 100644
--- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
+++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
@@ -117,7 +117,7 @@ static int chtdc_ti_pmic_opregion_probe(struct
platform_device *pdev)
return err;
/* Re-enumerate devices depending on PMIC */
- acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent));
+ acpi_dev_flag_dependency_met(ACPI_HANDLE(pdev->dev.parent));
return 0;
}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 80b668c80073..c9e4190316ef 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -49,12 +49,6 @@ static DEFINE_MUTEX(acpi_hp_context_lock);
*/
static u64 spcr_uart_addr;
-struct acpi_dep_data {
- struct list_head node;
- acpi_handle supplier;
- acpi_handle consumer;
-};
-
void acpi_scan_lock_acquire(void)
{
mutex_lock(&acpi_scan_lock);
@@ -2099,30 +2093,58 @@ static void acpi_bus_attach(struct acpi_device *device,
bool first_pass)
device->handler->hotplug.notify_online(device);
}
-void acpi_walk_dep_device_list(acpi_handle handle)
+static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
+ void *data)
{
- struct acpi_dep_data *dep, *tmp;
struct acpi_device *adev;
+ acpi_bus_get_device(dep->consumer, &adev);
+ if (!adev)
+ return 0;
+
+ adev->dep_unmet--;
+ if (!adev->dep_unmet)
+ acpi_bus_attach(adev, true);
+
+ list_del(&dep->node);
+ kfree(dep);
+ return 0;
+}
+
+void acpi_walk_dep_device_list(acpi_handle handle,
+ int (*callback)(struct acpi_dep_data *, void *),
+ void *data)
+{
+ struct acpi_dep_data *dep, *tmp;
+ int ret;
+
mutex_lock(&acpi_dep_list_lock);
list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
if (dep->supplier == handle) {
- acpi_bus_get_device(dep->consumer, &adev);
- if (!adev)
- continue;
-
- adev->dep_unmet--;
- if (!adev->dep_unmet)
- acpi_bus_attach(adev, true);
-
- list_del(&dep->node);
- kfree(dep);
+ ret = callback(dep, data);
+ if (ret)
+ break;
}
}
mutex_unlock(&acpi_dep_list_lock);
}
EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
+/**
+ * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device
+ * is now active
+ * @handle: acpi_handle for the supplier device
+ *
+ * This function walks through the dependencies list and informs each consumer
+ * of @handle that their dependency upon it is now met. Devices with no more
+ * unmet dependencies will be attached to the acpi bus.
+ */
+void acpi_dev_flag_dependency_met(acpi_handle handle)
+{
+ acpi_walk_dep_device_list(handle, __acpi_dev_flag_dependency_met, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
+
/**
* acpi_bus_scan - Add ACPI device node objects
[PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device
In some ACPI tables we encounter, devices use the _DEP method to assert
a dependence on other ACPI devices as opposed to the OpRegions that the
specification intends. We need to be able to find those devices "from"
the dependee, so add a callback and a wrapper to walk over the
acpi_dep_list and return the dependent ACPI device.
Signed-off-by: Daniel Scally
---
Changes in v3:
- Switched from a standalone function to a callback passed to
acpi_walk_dep_device_list().
drivers/acpi/scan.c | 34 ++
include/acpi/acpi_bus.h | 1 +
2 files changed, 35 insertions(+)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c9e4190316ef..55626925261c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2093,6 +2093,21 @@ static void acpi_bus_attach(struct acpi_device *device,
bool first_pass)
device->handler->hotplug.notify_online(device);
}
+static int __acpi_dev_get_dependent_dev(struct acpi_dep_data *dep, void *data)
+{
+ struct acpi_device *adev;
+ int ret;
+
+ ret = acpi_bus_get_device(dep->consumer, &adev);
+ if (ret)
+ /* If we don't find an adev then we want to continue parsing */
+ return 0;
+
+ *(struct acpi_device **)data = adev;
+
+ return 1;
+}
+
static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
void *data)
{
@@ -2145,6 +2160,25 @@ void acpi_dev_flag_dependency_met(acpi_handle handle)
}
EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
+/**
+ * acpi_dev_get_dependent_dev - Return ACPI device dependent on @adev
+ * @adev: Pointer to the dependee device
+ *
+ * Returns the first &struct acpi_device which declares itself dependent on
+ * @adev via the _DEP buffer, parsed from the acpi_dep_list.
+ */
+struct acpi_device *
+acpi_dev_get_dependent_dev(struct acpi_device *supplier)
+{
+ struct acpi_device *adev = NULL;
+
+ acpi_walk_dep_device_list(supplier->handle,
+ __acpi_dev_get_dependent_dev, &adev);
+
+ return adev;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_dependent_dev);
+
/**
* acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
* @handle: Root of the namespace scope to scan.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 91172af3a04d..5b14a9ae4ed5 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -690,6 +690,7 @@ 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);
void acpi_dev_flag_dependency_met(acpi_handle handle);
+struct acpi_device *acpi_dev_get_dependent_dev(struct acpi_device *supplier);
struct acpi_device *
acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const
char *uid, s64 hrv);
struct acpi_device *
--
2.25.1
[PATCH v3 0/6] Introduce intel_skl_int3472 module
v1 for this series was originally 14-18 of this series: https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrsca...@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67 v2 was here: https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrsca...@gmail.com/ Series level changelog: - Dropped the patch moving acpi_lpss_dep() to utils since it's not used in acpi_dev_get_dependent_dev() anymore. - Replaced it with a patch extending acpi_walk_dep_device_list() to be able to apply a given callback against each device in acpi_dep_list - Dropped the patch creating acpi_i2c_dev_name() and simply open coded that functionality. This has been tested on a number of devices, but currently **not** on a device designed for ChromeOS, which we ideally need to do to ensure no regression caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way you could help with that? Unfortunately, I don't have a device to test it on myself. Original cover letter: At the moment in the kernel the ACPI _HID INT3472 is taken by the tps68470 MFD driver, but that driver can only handle some of the cases of that _HID that we see. There are at least these three possibilities: 1. INT3472 devices that provide GPIOs through the usual framework and run power and clocks through an operation region; this is the situation that the current module handles and is seen on ChromeOS devices 2. INT3472 devices that provide GPIOs, plus clocks and regulators that are meant to be driven through the usual frameworks, usually seen on devices designed to run Windows 3. INT3472 devices that don't actually represent a physical tps68470, but are being used as a convenient way of grouping a bunch of system GPIO lines that are intended to enable power and clocks for sensors which are called out as dependent on them. Also seen on devices designed to run Windows. This series introduces a new module which registers: 1. An i2c driver that determines which scenario (#1 or #2) applies to the machine and registers platform devices to be bound to GPIO, OpRegion, clock and regulator drivers as appropriate. 2. A platform driver that binds to the dummy INT3472 devices described in #3 The platform driver for the dummy device registers the GPIO lines that enable the clocks and regulators to the sensors via those frameworks so that sensor drivers can consume them in the usual fashion. The existing GPIO and OpRegion tps68470 drivers will work with the i2c driver that's registered. Clock and regulator drivers are available but have not so far been tested, so aren't part of this series. The existing mfd/tps68470.c driver being thus superseded, it is removed. Thanks Dan Daniel Scally (6): ACPI: scan: Extend acpi_walk_dep_device_list() ACPI: scan: Add function to fetch dependent of acpi device i2c: core: Add a format macro for I2C device names gpiolib: acpi: Export acpi_get_gpiod() platform/x86: Add intel_skl_int3472 driver mfd: tps68470: Remove tps68470 MFD driver MAINTAINERS | 5 + drivers/acpi/ec.c | 2 +- drivers/acpi/pmic/Kconfig | 2 +- drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +- drivers/acpi/scan.c | 92 ++- drivers/gpio/Kconfig | 2 +- drivers/gpio/gpiolib-acpi.c | 38 +- drivers/i2c/i2c-core-acpi.c | 2 +- drivers/i2c/i2c-core-base.c | 4 +- drivers/mfd/Kconfig | 18 - drivers/mfd/Makefile | 1 - drivers/mfd/tps68470.c| 97 --- drivers/platform/surface/surface3_power.c | 2 +- drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel-int3472/Kconfig| 31 + drivers/platform/x86/intel-int3472/Makefile | 4 + .../intel-int3472/intel_skl_int3472_common.c | 106 .../intel-int3472/intel_skl_int3472_common.h | 110 .../intel_skl_int3472_discrete.c | 592 ++ .../intel_skl_int3472_tps68470.c | 113 include/acpi/acpi_bus.h | 8 + include/linux/acpi.h | 4 +- include/linux/gpio/consumer.h | 7 + include/linux/i2c.h | 3 + 25 files changed, 1100 insertions(+), 148 deletions(-) delete mode 100644 drivers/mfd/tps68470.c create mode 100644 drivers/platform/x86/intel-int3472/Kconfig create mode 100644 drivers/platform/x86/intel-int3472/Makefile create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int347
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hello Andy, Laurent
On 21/01/2021 00:18, Daniel Scally wrote:
> On 20/01/2021 12:57, Andy Shevchenko wrote:
>> On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote:
>>> On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote:
>>>> On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote:
>>>>> On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote:
>>>>>> On Tue, Jan 19, 2021 at 10:40:42AM +, Daniel Scally wrote:
>>>>>>> On 19/01/2021 09:24, Andy Shevchenko wrote:
>>>>>>>>>>>> +static struct i2c_driver int3472_tps68470 = {
>>>>>>>>>>>> + .driver = {
>>>>>>>>>>>> + .name = "int3472-tps68470",
>>>>>>>>>>>> + .acpi_match_table = int3472_device_id,
>>>>>>>>>>>> + },
>>>>>>>>>>>> + .probe_new = skl_int3472_tps68470_probe,
>>>>>>>>>>>> +};
>>>>>>>>>> I'm not sure we want to have like this. If I'm not mistaken the I²C
>>>>>>>>>> driver can
>>>>>>>>>> be separated without ACPI IDs (just having I²C IDs) and you may
>>>>>>>>>> instantiate it
>>>>>>>>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits
>>>>>>>>>> better...
>>>>>>>>> Sorry, I'm a bit confused by this. The i2c device is already
>>>>>>>>> present...we just want the driver to bind to them, so what role do
>>>>>>>>> those
>>>>>>>>> functions have there?
>>>>>>>> What I meant is something like
>>>>>>>>
>>>>>>>> *_i2c.c
>>>>>>>>real I²C driver for the TPS chip, but solely with I²C ID table,
>>>>>>>> no ACPI
>>>>>>>>involved (and it sounds like it should be mfd/tps one, in which
>>>>>>>> you
>>>>>>>>just cut out ACPI IDs and convert to pure I²C one, that what I
>>>>>>>> had
>>>>>>>>suggested in the first place)
>>>>>>> Ahh; sorry - i misunderstood what you meant there. I understand now I
>>>>>>> think, but there is one complication; the ACPI subsystem already creates
>>>>>>> a client for that i2c adapter and address; i2c_new_client_device()
>>>>>>> includes a check to see whether that adapter / address combination has
>>>>>>> an i2c device already. So we would have to have the platform driver
>>>>>>> with ACPI ID first find the existing i2c_client and unregister it before
>>>>>>> registering the new one...the existing clients have a name matching the
>>>>>>> ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an
>>>>>>> i2c_device_id of course.
>>>>>> See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600
>>>>>>
>>>>>> static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
>>>>>> {"BSG1160", },
>>>>>> {"BSG2150", },
>>>>>> {"INT33FE", },
>>>>>> {"INT3515", },
>>>>>> {}
>>>>>> };
>>>>>>
>>>>>> So, we quirklist it here and instantiate manually from platform driver
>>>>>> (new
>>>>>> coming one).
>>>>> This is documented as used for devices that have multiple I2cSerialBus
>>>>> resources. That's not the case for the INT3472 as far as I can tell. I
>>>>> don't think we should abuse this mechanism.
>>>> This is quite a similar case to that one. Let's avoid yak shaving, right?
>>> Exactly my point, that's why I think this patch is good overall, I don't
>>> think it requires a complete rewrite.
>> The approach in the series is to reinvent the MFD driver which I against of.
>> I don;t think we need to kill it there and reborn in a new form and dragging
>> code from there to here to there.
>>
>> On top of that the approach with a quirk driver in the middle seems to me
>> cleaner than using different paths how the two drivers are b
Re: [PATCH 1/1] ipu3-cio2: Build bridge only if ACPI is enabled
On 02/02/2021 20:30, Sakari Ailus wrote:
> On Tue, Feb 02, 2021 at 12:24:54PM -0800, Randy Dunlap wrote:
>> On 2/2/21 12:14 PM, Sakari Ailus wrote:
>>> ipu3-cio2-bridge uses several features of the ACPI framework that have no
>>> meaningful replacement when ACPI is disabled. Instead of adding #ifdefs to
>>> the affected places, only build the bridge code if CONFIG_ACPI is enabled.
>>>
>>> Fixes: 803abec64ef9 ("media: ipu3-cio2: Add cio2-bridge to ipu3-cio2
>>> driver")
>>> Signed-off-by: Sakari Ailus
>> Reported-by: Randy Dunlap
>> Acked-by: Randy Dunlap # build-tested
> Thanks! I'll include this in a pull request to Mauro shortly.
>
Ah - thank you both; sorry to have missed that.
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
Hi Rafael
On 21/01/2021 21:06, Daniel Scally wrote:
>
> On 21/01/2021 18:08, Rafael J. Wysocki wrote:
>> On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally wrote:
>>>
>>> On 21/01/2021 14:39, Rafael J. Wysocki wrote:
>>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally wrote:
>>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote:
>>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally
>>>>>> wrote:
>>>>>>> Hi Rafael
>>>>>>>
>>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote:
>>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally
>>>>>>>> wrote:
>>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
>>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally
>>>>>>>>>> wrote:
>>>>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to
>>>>>>>>>>> assert
>>>>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that
>>>>>>>>>>> the
>>>>>>>>>>> specification intends. We need to be able to find those devices
>>>>>>>>>>> "from"
>>>>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check
>>>>>>>>>>> if
>>>>>>>>>>> the include the handle of the dependee device in their _DEP buffer.
>>>>>>>>>> What exactly do you need this for?
>>>>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which
>>>>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the
>>>>>>>>> INT3472 device, we need to find the sensors dependent on them.
>>>>>>>>>
>>>>>>>> Well, this is an interesting concept. :-)
>>>>>>>>
>>>>>>>> Why does _DEP need to be used for that? Isn't there any other way to
>>>>>>>> look up the dependent sensors?
>>>>>>>>
>>>>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list
>>>>>>>>>> instead?
>>>>>>>>>>
>>>>>>>>>> Note that supplier drivers may remove entries from there, but does
>>>>>>>>>> that matter for your use case?
>>>>>>>>> Ah - that may work, yes. Thank you, let me test that.
>>>>>>>> Even if that doesn't work right away, but it can be made work, I would
>>>>>>>> very much prefer that to the driver parsing _DEP for every device in
>>>>>>>> the namespace by itself.
>>>>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case
>>>>>>> with acpi_dep_list declared as external var in internal.h)?
>>>>>> Let's put it in scan.c for now, because there is the lock protecting
>>>>>> the list in there too.
>>>>>>
>>>>>> How do you want to implement this? Something like "walk the list and
>>>>>> run a callback for the matching entries" or do you have something else
>>>>>> in mind?
>>>>> Something like this (though with a mutex_lock()). It could be simplified
>>>>> by dropping the prev stuff, but we have seen INT3472 devices with
>>>>> multiple sensors declaring themselves dependent on the same device
>>>>>
>>>>>
>>>>> struct acpi_device *
>>>>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier,
>>>>> struct acpi_device *prev)
>>>>> {
>>>>> struct acpi_dep_data *dep;
>>>>> struct acpi_device *adev;
>>>>> int ret;
>>>>>
>>>>> if (!supplier)
>>>>> return ERR_PTR(-EINVAL);
>>>>>
>>>>> if (prev) {
>>>>> /*
>>>>> * We need to find the previous device in the list, so we know
>>>>> * where to start iterating from.
>>>>> */
>>>>> list_for_each_entry(dep, &acpi_dep_list, node)
>>>>> if (
Re: [PATCH v2 4/7] i2c: i2c-core-acpi: Add i2c_acpi_dev_name()
On 28/01/2021 09:17, Wolfram Sang wrote: >> Just to clarify; "open-code" meaning inline it in the caller like >> Laurent said, right? > Yes. > Thanks - will do that and drop this one then
Re: [PATCH v2 4/7] i2c: i2c-core-acpi: Add i2c_acpi_dev_name()
On 28/01/2021 09:00, Wolfram Sang wrote: >>> There's a real danger of a memory leak, as the function name sounds very >>> similar to dev_name() or acpi_dev_name() and those don't allocate >>> memory. I'm not sure what a better name would be, but given that this >>> function is only used in patch 6/7 and not in the I2C subsystem itself, >>> I wonder if we should inline this kasprintf() call in the caller and >>> drop this patch. >> IMO if this is a one-off usage, it's better to open-code it. > Sounds reasonable to me, too. > Just to clarify; "open-code" meaning inline it in the caller like Laurent said, right?
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi both On 20/01/2021 11:44, Andy Shevchenko wrote: > On Wed, Jan 20, 2021 at 06:18:53AM +0200, Laurent Pinchart wrote: >> On Tue, Jan 19, 2021 at 07:43:15PM +0200, Andy Shevchenko wrote: >>> On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: >>>> On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: >>>>> On Tue, Jan 19, 2021 at 12:11:40AM +, Daniel Scally wrote: >>>>>> On 18/01/2021 21:19, Daniel Scally wrote: > ... > >>>>> See my previous reply. TL;DR: you have to modify clk-gpio.c to export >>>>> couple of >>>>> methods to be able to use it as a library. >>>> That seems really overkill given the very simple implementation of the >>>> clock provided here. >>> Less code in the end is called an overkill? Hmm... >>> I think since we in Linux it's better to utilize what it provides. Do you >>> want >>> me to prepare a patch to show that there is no overkill at all? >> The amount of code we would save it very small. It's not necessarily a >> bad idea, but I think such an improvement could be made on top, it >> shouldn't block this series. > Okay, let's wait what Dan will say on this. > I can probably help to achieve this improvement sooner than later. Well; turns out that we missed an operation we really need to add (clk_recalc_rate) which in our case needs to read a fixed value stored in a buffer in ACPI; most of the code is shared with an existing function in the driver so it's not much extra to add, but I think it kinda precludes using clk-gpio for this anyway >>>>>> (also, Laurent, if we did it this way we wouldn't be able to also handle >>>>>> the led-indicator GPIO here without some fairly major rework) >>>>> LED indicators are done as LED class devices (see plenty of examples in >>>>> PDx86 >>>>> drivers: drivers/platform/x86/) >>>> How do you expose the link between the sensor and its indicator LED to >>>> userspace ? Isn't it better to handle it in the kernel to avoid rogue >>>> userspace turning the camera on without notifying the user ? >>> I didn't get this. It's completely a LED handling driver business. We may >>> expose it to user space or not, but it's orthogonal to the usage of LED >>> class >>> IIUC. Am I mistaken here? >> If it stays internal to the kernel and is solely controlled from the >> int3472 driver, there's no need to involve the LED class. If we want to >> expose the privacy LED to userspace then the LED framework is the way to >> go, but we will also need to find a way to expose the link between the >> camera sensor and the LED to userspace. If there are two privacy LEDs, >> one for the front sensor and one for the back sensor, userspace will >> need to know which is which. > I see. For now we probably can keep GPIO LED implementation internally. >
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
On 21/01/2021 18:08, Rafael J. Wysocki wrote:
> On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally wrote:
>>
>> On 21/01/2021 14:39, Rafael J. Wysocki wrote:
>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally wrote:
>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote:
>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally
>>>>> wrote:
>>>>>> Hi Rafael
>>>>>>
>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote:
>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally
>>>>>>> wrote:
>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally
>>>>>>>>> wrote:
>>>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to
>>>>>>>>>> assert
>>>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that
>>>>>>>>>> the
>>>>>>>>>> specification intends. We need to be able to find those devices
>>>>>>>>>> "from"
>>>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check
>>>>>>>>>> if
>>>>>>>>>> the include the handle of the dependee device in their _DEP buffer.
>>>>>>>>> What exactly do you need this for?
>>>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which
>>>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the
>>>>>>>> INT3472 device, we need to find the sensors dependent on them.
>>>>>>>>
>>>>>>> Well, this is an interesting concept. :-)
>>>>>>>
>>>>>>> Why does _DEP need to be used for that? Isn't there any other way to
>>>>>>> look up the dependent sensors?
>>>>>>>
>>>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list
>>>>>>>>> instead?
>>>>>>>>>
>>>>>>>>> Note that supplier drivers may remove entries from there, but does
>>>>>>>>> that matter for your use case?
>>>>>>>> Ah - that may work, yes. Thank you, let me test that.
>>>>>>> Even if that doesn't work right away, but it can be made work, I would
>>>>>>> very much prefer that to the driver parsing _DEP for every device in
>>>>>>> the namespace by itself.
>>>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case
>>>>>> with acpi_dep_list declared as external var in internal.h)?
>>>>> Let's put it in scan.c for now, because there is the lock protecting
>>>>> the list in there too.
>>>>>
>>>>> How do you want to implement this? Something like "walk the list and
>>>>> run a callback for the matching entries" or do you have something else
>>>>> in mind?
>>>> Something like this (though with a mutex_lock()). It could be simplified
>>>> by dropping the prev stuff, but we have seen INT3472 devices with
>>>> multiple sensors declaring themselves dependent on the same device
>>>>
>>>>
>>>> struct acpi_device *
>>>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier,
>>>> struct acpi_device *prev)
>>>> {
>>>> struct acpi_dep_data *dep;
>>>> struct acpi_device *adev;
>>>> int ret;
>>>>
>>>> if (!supplier)
>>>> return ERR_PTR(-EINVAL);
>>>>
>>>> if (prev) {
>>>> /*
>>>> * We need to find the previous device in the list, so we know
>>>> * where to start iterating from.
>>>> */
>>>> list_for_each_entry(dep, &acpi_dep_list, node)
>>>> if (dep->consumer == prev->handle &&
>>>> dep->supplier == supplier->handle)
>>>> break;
>>>>
>>>> dep = list_next_entry(dep, node);
>>>> } else {
>>>> dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data,
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
On 21/01/2021 14:39, Rafael J. Wysocki wrote:
> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally wrote:
>>
>> On 21/01/2021 11:58, Rafael J. Wysocki wrote:
>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally wrote:
>>>> Hi Rafael
>>>>
>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote:
>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally wrote:
>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally
>>>>>>> wrote:
>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert
>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the
>>>>>>>> specification intends. We need to be able to find those devices "from"
>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check if
>>>>>>>> the include the handle of the dependee device in their _DEP buffer.
>>>>>>> What exactly do you need this for?
>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which
>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the
>>>>>> INT3472 device, we need to find the sensors dependent on them.
>>>>>>
>>>>> Well, this is an interesting concept. :-)
>>>>>
>>>>> Why does _DEP need to be used for that? Isn't there any other way to
>>>>> look up the dependent sensors?
>>>>>
>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead?
>>>>>>>
>>>>>>> Note that supplier drivers may remove entries from there, but does
>>>>>>> that matter for your use case?
>>>>>> Ah - that may work, yes. Thank you, let me test that.
>>>>> Even if that doesn't work right away, but it can be made work, I would
>>>>> very much prefer that to the driver parsing _DEP for every device in
>>>>> the namespace by itself.
>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case
>>>> with acpi_dep_list declared as external var in internal.h)?
>>> Let's put it in scan.c for now, because there is the lock protecting
>>> the list in there too.
>>>
>>> How do you want to implement this? Something like "walk the list and
>>> run a callback for the matching entries" or do you have something else
>>> in mind?
>>
>> Something like this (though with a mutex_lock()). It could be simplified
>> by dropping the prev stuff, but we have seen INT3472 devices with
>> multiple sensors declaring themselves dependent on the same device
>>
>>
>> struct acpi_device *
>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier,
>> struct acpi_device *prev)
>> {
>> struct acpi_dep_data *dep;
>> struct acpi_device *adev;
>> int ret;
>>
>> if (!supplier)
>> return ERR_PTR(-EINVAL);
>>
>> if (prev) {
>> /*
>> * We need to find the previous device in the list, so we know
>> * where to start iterating from.
>> */
>> list_for_each_entry(dep, &acpi_dep_list, node)
>> if (dep->consumer == prev->handle &&
>> dep->supplier == supplier->handle)
>> break;
>>
>> dep = list_next_entry(dep, node);
>> } else {
>> dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data,
>>node);
>> }
>>
>>
>> list_for_each_entry_from(dep, &acpi_dep_list, node) {
>> if (dep->supplier == supplier->handle) {
>> ret = acpi_bus_get_device(dep->consumer, &adev);
>> if (ret)
>> return ERR_PTR(ret);
>>
>> return adev;
>> }
>> }
>>
>> return NULL;
>> }
> That would work I think, but would it be practical to modify
> acpi_walk_dep_device_list() so that it runs a callback for every
> consumer found instead of or in addition to the "delete from the list
> and free the entry" operation?
I think that this would work fine, if that's the way you want to go.
We'd just need to move everything inside the if (dep->supplier ==
handle) block to a new callback, and for my purposes I think also add a
way to stop parsing the list from the callback (so like have the
callbacks return int and stop parsing on a non-zero return). Do you want
to expose that ability to pass a callback outside of ACPI? Or just
export helpers to call each of the callbacks (one to fetch the next
dependent device, one to decrement the unmet dependencies counter)
Otherwise, I'd just need to update the 5 users of that function either
to use the new helper or else to also pass the decrement dependencies
callback.
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
On 21/01/2021 11:58, Rafael J. Wysocki wrote:
> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally wrote:
>> Hi Rafael
>>
>> On 19/01/2021 13:15, Rafael J. Wysocki wrote:
>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally wrote:
>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally wrote:
>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert
>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the
>>>>>> specification intends. We need to be able to find those devices "from"
>>>>>> the dependee, so add a function to parse all ACPI Devices and check if
>>>>>> the include the handle of the dependee device in their _DEP buffer.
>>>>> What exactly do you need this for?
>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which
>>>> refer to those INT3472's in their _DEP method. The driver binds to the
>>>> INT3472 device, we need to find the sensors dependent on them.
>>>>
>>> Well, this is an interesting concept. :-)
>>>
>>> Why does _DEP need to be used for that? Isn't there any other way to
>>> look up the dependent sensors?
>>>
>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead?
>>>>>
>>>>> Note that supplier drivers may remove entries from there, but does
>>>>> that matter for your use case?
>>>> Ah - that may work, yes. Thank you, let me test that.
>>> Even if that doesn't work right away, but it can be made work, I would
>>> very much prefer that to the driver parsing _DEP for every device in
>>> the namespace by itself.
>>
>> This does work; do you prefer it in scan.c, or in utils.c (in which case
>> with acpi_dep_list declared as external var in internal.h)?
> Let's put it in scan.c for now, because there is the lock protecting
> the list in there too.
>
> How do you want to implement this? Something like "walk the list and
> run a callback for the matching entries" or do you have something else
> in mind?
Something like this (though with a mutex_lock()). It could be simplified
by dropping the prev stuff, but we have seen INT3472 devices with
multiple sensors declaring themselves dependent on the same device
struct acpi_device *
acpi_dev_get_next_dependent_dev(struct acpi_device *supplier,
            struct acpi_device *prev)
{
   struct acpi_dep_data *dep;
   struct acpi_device *adev;
   int ret;
   if (!supplier)
      return ERR_PTR(-EINVAL);
   if (prev) {
      /*
       * We need to find the previous device in the list, so we know
       * where to start iterating from.
       */
      list_for_each_entry(dep, &acpi_dep_list, node)
         if (dep->consumer == prev->handle &&
            dep->supplier == supplier->handle)
            break;
      dep = list_next_entry(dep, node);
   } else {
      dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data,
              node);
   }
   list_for_each_entry_from(dep, &acpi_dep_list, node) {
      if (dep->supplier == supplier->handle) {
         ret = acpi_bus_get_device(dep->consumer, &adev);
         if (ret)
            return ERR_PTR(ret);
         return adev;
      }
   }
   return NULL;
}
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
Hi Rafael On 19/01/2021 13:15, Rafael J. Wysocki wrote: > On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally wrote: >> On 18/01/2021 16:14, Rafael J. Wysocki wrote: >>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally wrote: >>>> In some ACPI tables we encounter, devices use the _DEP method to assert >>>> a dependence on other ACPI devices as opposed to the OpRegions that the >>>> specification intends. We need to be able to find those devices "from" >>>> the dependee, so add a function to parse all ACPI Devices and check if >>>> the include the handle of the dependee device in their _DEP buffer. >>> What exactly do you need this for? >> So, in our DSDT we have devices with _HID INT3472, plus sensors which >> refer to those INT3472's in their _DEP method. The driver binds to the >> INT3472 device, we need to find the sensors dependent on them. >> > Well, this is an interesting concept. :-) > > Why does _DEP need to be used for that? Isn't there any other way to > look up the dependent sensors? > >>> Would it be practical to look up the suppliers in acpi_dep_list instead? >>> >>> Note that supplier drivers may remove entries from there, but does >>> that matter for your use case? >> Ah - that may work, yes. Thank you, let me test that. > Even if that doesn't work right away, but it can be made work, I would > very much prefer that to the driver parsing _DEP for every device in > the namespace by itself. This does work; do you prefer it in scan.c, or in utils.c (in which case with acpi_dep_list declared as external var in internal.h)?
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On 20/01/2021 12:57, Andy Shevchenko wrote:
> On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote:
>> On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote:
>>> On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote:
>>>> On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote:
>>>>> On Tue, Jan 19, 2021 at 10:40:42AM +, Daniel Scally wrote:
>>>>>> On 19/01/2021 09:24, Andy Shevchenko wrote:
>>>>>>>>>>> +static struct i2c_driver int3472_tps68470 = {
>>>>>>>>>>> + .driver = {
>>>>>>>>>>> + .name = "int3472-tps68470",
>>>>>>>>>>> + .acpi_match_table = int3472_device_id,
>>>>>>>>>>> + },
>>>>>>>>>>> + .probe_new = skl_int3472_tps68470_probe,
>>>>>>>>>>> +};
>>>>>>>>> I'm not sure we want to have like this. If I'm not mistaken the I²C
>>>>>>>>> driver can
>>>>>>>>> be separated without ACPI IDs (just having I²C IDs) and you may
>>>>>>>>> instantiate it
>>>>>>>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits
>>>>>>>>> better...
>>>>>>>> Sorry, I'm a bit confused by this. The i2c device is already
>>>>>>>> present...we just want the driver to bind to them, so what role do
>>>>>>>> those
>>>>>>>> functions have there?
>>>>>>> What I meant is something like
>>>>>>>
>>>>>>> *_i2c.c
>>>>>>> real I²C driver for the TPS chip, but solely with I²C ID table,
>>>>>>> no ACPI
>>>>>>> involved (and it sounds like it should be mfd/tps one, in which
>>>>>>> you
>>>>>>> just cut out ACPI IDs and convert to pure I²C one, that what I
>>>>>>> had
>>>>>>> suggested in the first place)
>>>>>>
>>>>>> Ahh; sorry - i misunderstood what you meant there. I understand now I
>>>>>> think, but there is one complication; the ACPI subsystem already creates
>>>>>> a client for that i2c adapter and address; i2c_new_client_device()
>>>>>> includes a check to see whether that adapter / address combination has
>>>>>> an i2c device already. So we would have to have the platform driver
>>>>>> with ACPI ID first find the existing i2c_client and unregister it before
>>>>>> registering the new one...the existing clients have a name matching the
>>>>>> ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an
>>>>>> i2c_device_id of course.
>>>>>
>>>>> See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600
>>>>>
>>>>> static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
>>>>> {"BSG1160", },
>>>>> {"BSG2150", },
>>>>> {"INT33FE", },
>>>>> {"INT3515", },
>>>>> {}
>>>>> };
>>>>>
>>>>> So, we quirklist it here and instantiate manually from platform driver
>>>>> (new
>>>>> coming one).
>>>>
>>>> This is documented as used for devices that have multiple I2cSerialBus
>>>> resources. That's not the case for the INT3472 as far as I can tell. I
>>>> don't think we should abuse this mechanism.
>>>
>>> This is quite a similar case to that one. Let's avoid yak shaving, right?
>>
>> Exactly my point, that's why I think this patch is good overall, I don't
>> think it requires a complete rewrite.
>
> The approach in the series is to reinvent the MFD driver which I against of.
> I don;t think we need to kill it there and reborn in a new form and dragging
> code from there to here to there.
>
> On top of that the approach with a quirk driver in the middle seems to me
> cleaner than using different paths how the two drivers are being initialized.
> In the proposed approach there will be one making decision point and easy to
> understand what's going on.
>
> The bad example of two making decision points is acpi_lpss.c vs. individual
>
Re: [PATCH v3] software_node: Add kernel-doc comments to exported symbols
Hi Sakari
On 20/01/2021 10:35, Sakari Ailus wrote:
> Hi Dan,
>
> On Wed, Jan 20, 2021 at 12:03:39AM +0000, Daniel Scally wrote:
>> A number of functions which are exported via EXPORT_SYMBOL_GPL() lack any
>> kernel-doc comments; add those in so all exported symbols are documented.
>>
>> Reviewed-by: Andy Shevchenko
>> Reviewed-by: Heikki Krogerus
>> Reviewed-by: Randy Dunlap
>> Reviewed-by: Sakari Ailus
>> Signed-off-by: Daniel Scally
>> ---
>> Changelog for v3:
>>
>> - s/passed to @parent/passed as @parent
>> - Wrapped a long summary line - TIL that you can do that, thanks
>>Sakari
>>
>> drivers/base/swnode.c | 54 +++
>> 1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 4a4b2008fbc2..39fbb653c58a 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -33,6 +33,13 @@ static struct kset *swnode_kset;
>>
>> static const struct fwnode_operations software_node_ops;
>>
>> +/**
>> + * is_software_node() - check if given fwnode was created from a
>> software_node
>> + * @fwnode: The &struct fwnode_handle to check
>> + *
>> + * This function is used to check whether a given firmware node handle was
>> + * created by registering a &struct software_node or not.
>> + */
>> bool is_software_node(const struct fwnode_handle *fwnode)
>> {
>> return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &software_node_ops;
>> @@ -71,6 +78,15 @@ software_node_to_swnode(const struct software_node *node)
>> return swnode;
>> }
>>
>> +/**
>> + * to_software_node() - Fetch the software node used to create a firmware
>> + * node handle
>> + * @fwnode: The pointer to a &struct fwnode_handle to parse
>> + *
>> + * This function attempts to fetch a pointer to the &struct software_node
>> which
>> + * was used to create the given @fwnode. Note that this will only work if
>> the
>> + * software node has **not** been released.
>> + */
>> const struct software_node *to_software_node(const struct fwnode_handle
>> *fwnode)
>> {
>> const struct swnode *swnode = to_swnode(fwnode);
>> @@ -79,6 +95,14 @@ const struct software_node *to_software_node(const struct
>> fwnode_handle *fwnode)
>> }
>> EXPORT_SYMBOL_GPL(to_software_node);
>>
>> +/**
>> + * software_node_fwnode() - Fetch firmware node associated with a given
>> software node
>> + * @node: The pointer to a &struct software_node to parse
>> + *
>> + * This function attempts to fetch a pointer to the &struct fwnode_handle
>> which
>> + * was created from the given @node. Note that this will only work after the
>> + * software node has been registered.
>> + */
>> struct fwnode_handle *software_node_fwnode(const struct software_node *node)
>> {
>> struct swnode *swnode = software_node_to_swnode(node);
>> @@ -800,6 +824,27 @@ void software_node_unregister(const struct
>> software_node *node)
>> }
>> EXPORT_SYMBOL_GPL(software_node_unregister);
>>
>> +/**
>> + * fwnode_create_software_node() - Create and register a new software_node
>> + * @properties: NULL terminated array of properties to assign to the new
>> node
>> + * @parent: Pointer to a &struct fwnode_handle to assign as parent to the
>> new
>> + * node
>> + *
>> + * NOTE: The pointer passed as @parent **must** be to a firmware node handle
>> + * that was created by registering a software node, meaning
>> is_software_node()
>> + * must return true when passed that pointer.
>> + *
>> + * This function creates a new instance of &struct software_node, assigns
>> it a
>> + * copy of the given array of properties and registers it as a new
>> fwnode_handle.
>> + * Freeing of the allocated memory when the fwnode_handle is no longer
>> needed is
>> + * handled via software_node_release() and does not need to be done
>> separately.
> Please wrap all lines over 80 unless there's a reason to keep them longer.
Apologies; I'll cat | awk for lines over the limit from now on rather
than half-arsing it.
>
>> + *
>> + * Returns:
>> + * * fwnode_handle *- On success
>> + * * -EINVAL- When @parent is not associated with a
>> software_node
>> + * * -ENOMEM- When memory allocation fails
>> + * * -Other
[PATCH v3] software_node: Add kernel-doc comments to exported symbols
A number of functions which are exported via EXPORT_SYMBOL_GPL() lack any
kernel-doc comments; add those in so all exported symbols are documented.
Reviewed-by: Andy Shevchenko
Reviewed-by: Heikki Krogerus
Reviewed-by: Randy Dunlap
Reviewed-by: Sakari Ailus
Signed-off-by: Daniel Scally
---
Changelog for v3:
- s/passed to @parent/passed as @parent
- Wrapped a long summary line - TIL that you can do that, thanks
Sakari
drivers/base/swnode.c | 54 +++
1 file changed, 54 insertions(+)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 4a4b2008fbc2..39fbb653c58a 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -33,6 +33,13 @@ static struct kset *swnode_kset;
static const struct fwnode_operations software_node_ops;
+/**
+ * is_software_node() - check if given fwnode was created from a software_node
+ * @fwnode: The &struct fwnode_handle to check
+ *
+ * This function is used to check whether a given firmware node handle was
+ * created by registering a &struct software_node or not.
+ */
bool is_software_node(const struct fwnode_handle *fwnode)
{
return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &software_node_ops;
@@ -71,6 +78,15 @@ software_node_to_swnode(const struct software_node *node)
return swnode;
}
+/**
+ * to_software_node() - Fetch the software node used to create a firmware
+ * node handle
+ * @fwnode: The pointer to a &struct fwnode_handle to parse
+ *
+ * This function attempts to fetch a pointer to the &struct software_node which
+ * was used to create the given @fwnode. Note that this will only work if the
+ * software node has **not** been released.
+ */
const struct software_node *to_software_node(const struct fwnode_handle
*fwnode)
{
const struct swnode *swnode = to_swnode(fwnode);
@@ -79,6 +95,14 @@ const struct software_node *to_software_node(const struct
fwnode_handle *fwnode)
}
EXPORT_SYMBOL_GPL(to_software_node);
+/**
+ * software_node_fwnode() - Fetch firmware node associated with a given
software node
+ * @node: The pointer to a &struct software_node to parse
+ *
+ * This function attempts to fetch a pointer to the &struct fwnode_handle which
+ * was created from the given @node. Note that this will only work after the
+ * software node has been registered.
+ */
struct fwnode_handle *software_node_fwnode(const struct software_node *node)
{
struct swnode *swnode = software_node_to_swnode(node);
@@ -800,6 +824,27 @@ void software_node_unregister(const struct software_node
*node)
}
EXPORT_SYMBOL_GPL(software_node_unregister);
+/**
+ * fwnode_create_software_node() - Create and register a new software_node
+ * @properties: NULL terminated array of properties to assign to the new node
+ * @parent: Pointer to a &struct fwnode_handle to assign as parent to the new
+ * node
+ *
+ * NOTE: The pointer passed as @parent **must** be to a firmware node handle
+ * that was created by registering a software node, meaning is_software_node()
+ * must return true when passed that pointer.
+ *
+ * This function creates a new instance of &struct software_node, assigns it a
+ * copy of the given array of properties and registers it as a new
fwnode_handle.
+ * Freeing of the allocated memory when the fwnode_handle is no longer needed
is
+ * handled via software_node_release() and does not need to be done separately.
+ *
+ * Returns:
+ * * fwnode_handle * - On success
+ * * -EINVAL - When @parent is not associated with a software_node
+ * * -ENOMEM - When memory allocation fails
+ * * -Other- Propagated errors from sub-functions
+ */
struct fwnode_handle *
fwnode_create_software_node(const struct property_entry *properties,
const struct fwnode_handle *parent)
@@ -832,6 +877,15 @@ fwnode_create_software_node(const struct property_entry
*properties,
}
EXPORT_SYMBOL_GPL(fwnode_create_software_node);
+/**
+ * fwnode_remove_software_node() - Put a reference to a registered
software_node
+ * @fwnode: The pointer to the &struct fwnode_handle you want to release
+ *
+ * Release a reference to a registered &struct software_node. This function
+ * differs from software_node_put() in that it takes no action if the
+ * firmware node handle passed to @fwnode turns out not to have been created by
+ * registering a software_node.
+ */
void fwnode_remove_software_node(struct fwnode_handle *fwnode)
{
struct swnode *swnode = to_swnode(fwnode);
--
2.25.1
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
On 19/01/2021 13:15, Rafael J. Wysocki wrote: > On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally wrote: >> On 18/01/2021 16:14, Rafael J. Wysocki wrote: >>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally wrote: >>>> In some ACPI tables we encounter, devices use the _DEP method to assert >>>> a dependence on other ACPI devices as opposed to the OpRegions that the >>>> specification intends. We need to be able to find those devices "from" >>>> the dependee, so add a function to parse all ACPI Devices and check if >>>> the include the handle of the dependee device in their _DEP buffer. >>> What exactly do you need this for? >> So, in our DSDT we have devices with _HID INT3472, plus sensors which >> refer to those INT3472's in their _DEP method. The driver binds to the >> INT3472 device, we need to find the sensors dependent on them. >> > Well, this is an interesting concept. :-) > > Why does _DEP need to be used for that? Isn't there any other way to > look up the dependent sensors? If there is, I'm not aware of it, I don't see a reference to the sensor in the INT3472 device (named "PMI0", with the corresponding sensor being "CAM0") in DSDTÂ [1] >>> Would it be practical to look up the suppliers in acpi_dep_list instead? >>> >>> Note that supplier drivers may remove entries from there, but does >>> that matter for your use case? >> Ah - that may work, yes. Thank you, let me test that. > Even if that doesn't work right away, but it can be made work, I would > very much prefer that to the driver parsing _DEP for every device in > the namespace by itself. Alright; I haven't looked too closely yet, but I think an iterator over acpi_dep_list exported from the ACPI subsystem would also work in a pretty similar way to the function introduced in this patch does, without much work [1] https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257bfdade47cc2981/dsdt.dsl
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On 19/01/2021 11:11, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 10:56:17AM +, Kieran Bingham wrote: >> On 18/01/2021 00:34, Daniel Scally wrote: > ... > >>> +config INTEL_SKL_INT3472 >>> + tristate "Intel SkyLake ACPI INT3472 Driver" >>> + depends on X86 && ACPI >>> + select REGMAP_I2C >> I've tried compiling this as a built in and a module and on my minimal >> config I had failures on both for regulator_register and >> regulator_unregister. >> >> I suspect this needs to have either a selects or a depends upon >> CONFIG_REGULATOR > Valid point, although it seems no consensus on which is better to use. It > seems > to me that in this case we need to select it. > Yeah; it will be necessary for the gpio-regulator too anyway I expect. Thanks Kieran; I missed that entirely.
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On 19/01/2021 09:24, Andy Shevchenko wrote:
> +static struct i2c_driver int3472_tps68470 = {
> + .driver = {
> + .name = "int3472-tps68470",
> + .acpi_match_table = int3472_device_id,
> + },
> + .probe_new = skl_int3472_tps68470_probe,
> +};
>>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver
>>> can
>>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate
>>> it
>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits
>>> better...
>> Sorry, I'm a bit confused by this. The i2c device is already
>> present...we just want the driver to bind to them, so what role do those
>> functions have there?
> What I meant is something like
>
> *_i2c.c
> real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI
> involved (and it sounds like it should be mfd/tps one, in which you
> just cut out ACPI IDs and convert to pure I²C one, that what I had
> suggested in the first place)
Ahh; sorry - i misunderstood what you meant there. I understand now I
think, but there is one complication; the ACPI subsystem already creates
a client for that i2c adapter and address; i2c_new_client_device()
includes a check to see whether that adapter / address combination has
an i2c device already. So we would have to have the platform driver
with ACPI ID first find the existing i2c_client and unregister it before
registering the new one...the existing clients have a name matching the
ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an
i2c_device_id of course.
>
> *_proxy.c
> GPIO proxy as library
>
> *.c
> platform driver with ACPI ID, in which ->probe() we actually instantiate
> above via calling i2c_acpi_new_device(), *if needed*, along with GPIO
> proxy
>
> ...
>
> +struct int3472_gpio_clock {
> + struct clk *clk;
> + struct clk_hw clk_hw;
> + struct clk_lookup *cl;
> + struct gpio_desc *gpio;
> +};
> +static const struct clk_ops skl_int3472_clock_ops = {
> + .prepare = skl_int3472_clk_prepare,
> + .unprepare = skl_int3472_clk_unprepare,
> + .enable = skl_int3472_clk_enable,
> + .disable = skl_int3472_clk_disable,
> +};
>>> Wondering if this has some similarities with and actually can utilize
>>> clk-gpio
>>> driver.
>>> Yeah, sounds like reinventing clk-gpio.c.
>>>
>>> static const struct clk_ops clk_gpio_gate_ops = {
>>> .enable = clk_gpio_gate_enable,
>>> .disable = clk_gpio_gate_disable,
>>> .is_enabled = clk_gpio_gate_is_enabled,
>>> };
>>>
>>> Or is it mux? It has support there as well.
>>>
>> Hmm, yeah, this looks like it would work actually. So I think I'd need to:
>>
>>
>> 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver
>>
>> 2. Register a platform device to bind to the clk-gpio driver
>>
>> 3. Register a gpio lookup table so that the clk-gpio driver can find the
>> gpio in question using gpiod_get()
>>
>> And that looks like it will work; I'll try it.
> You need to modify clk-gpio.c to export
>
> clk_hw_register_gpio_gate()
> clk_hw_register_gpio_mux()
>
> (perhaps it will require to add *_unregister() counterparts) and call it from
> your code.
>
> See, for example, how clk_hw_unregister_fixed_rate() is being used. Another
> case is to add a helper directly into clk-gpio and call it instead of
> clk_hw_*() one, see how clk_register_fractional_divider() is implemented and
> used.
I'll take a look, thanks
> ...
>
> + /* Lenovo Miix 510-12ISK - OV5648, Rear */
> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL},
> + /* Surface Go 1&2 - OV5693, Front */
> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL},
>>> I'm wondering if you should use same I2C format macro and create this
>>> dynamically? Or rather find a corresponding ACPI device instance and
>>> copy it's name? ...
>> The supply name needs hard-coding really, but the device name I suppose
>> can come from int3472->sensor_name.
> To be strict in terms you are using "device instance name" in the
> REGULATOR_SUPPLY() second parameter. Because "device name" is generic and
> doesn't point to the actual *instance* of the device in the system.
>
> So, and "device name instance" we may get only by traversing through the
> (ACPI)
> bus and find corresponding struct device and derive name from it. Same way
> like
> you have done in previous patch series.
>
> Because there is no guarantee that, e.g., i2c-INT33BE:00 will be present on
> the system and moreover there is no guarantee that if two INT33BE or more
> devices are present you will get :00 always for the one you need!
Mm, good point, hadn't considered two identical sensors on the same
platform. Alright; I'll think about this in more detail, thank you.
> + opregion_dev =
> skl_int3472_register_pdev("tps68470_pmic_opregion",
> +
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Morning Andy On 19/01/2021 09:33, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: >> On 18/01/2021 21:19, Daniel Scally wrote: >> I'm more and more confident that this will work, but it has some >> knock-on effects: >> >> The both clk and regulator gpio driver expects to be able to fetch the >> GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of >> course, so we need to add another GPIO lookup table so those drivers can >> see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will >> be so we can set the .dev_id member of a gpiod_lookup_table to that >> value, but that isn't set until _after_ the pdev is registered (because >> it has to figure out the id, we can't manually set the IDs because there >> could be more than one instance of int3472-discrete bound to multiple >> PMIC devices, and we don't know which id the current one should have). >> Finally, we can't wait until the device is registered because it >> immediately probes, can't find the GPIO and then fails probe. >> >> It's similar problem that causes us to need the i2c-acpi name format >> macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) >> >> Solving it is a bit of a sticky one; perhaps something like moving the >> dev_set_name() part of platform_device_add() [1] to its own function, >> that's called in both platform_device_alloc() and >> platform_device_register(). That way it would be available before the >> device itself was registered, meaning we could create the lookup table >> before it probes the driver. > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple > of > methods to be able to use it as a library. Ack! Ok, I thought about that the wrong way. I'll take another look tonight then. >> (also, Laurent, if we did it this way we wouldn't be able to also handle >> the led-indicator GPIO here without some fairly major rework) > LED indicators are done as LED class devices (see plenty of examples in PDx86 > drivers: drivers/platform/x86/) And this too - thanks very much
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Laurent
On 19/01/2021 06:19, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Mon, Jan 18, 2021 at 08:46:34PM +0000, Daniel Scally wrote:
>> Hi Laurent, thanks for the comments - really appreciate the detail.
>>
>> Some specific responses below but assume a general "will do" to
>> everything you mentioned otherwise...
>>
>> On 18/01/2021 09:15, Laurent Pinchart wrote:
>>>> +PMIC) and one designed for Chrome OS.
>>> How about expanding this a bit to explain what the INT3472 stands for ?
>>>
>>> The INT3472 is an Intel camera power controller, a logical device
>>> found on some Skylake-based systems that can map to different
>>> hardware devices depending on the platform. On machines
>>> designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
>>> machines designed for Windows, it maps to either a TP68470
>>> camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
>>> and power gates.
>> Yeah sure ok
>>
>>>> This driver handles all three
>>>> +situations by discovering information it needs to discern them at
>>>> +runtime.
>>>> +
>>>> +If your device was designed for Chrome OS, this driver will provide
>>>> +an ACPI operation region, which must be available before any of the
>>>> +devices using this are probed. For this reason, you should select Y
>>>> +if your device was designed for ChromeOS. This option also configures
>>>> +the designware-i2c driver to be built-in, for the same reason.
>>> Is the last sentence a leftover ?
>> Oops - it is, but it was supposed to remind me to double check that that
>> was still necessary. I'll take a look, thanks.
>>
>>>> +
>>>> +#include "intel_skl_int3472_common.h"
>>>> +
>>>> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev,
>>>> + struct int3472_cldb *cldb)
>>>> +{
>>>> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>>>> + acpi_handle handle = adev->handle;
>>>> + union acpi_object *obj;
>>>> + acpi_status status;
>>>> + int ret = 0;
>>>> +
>>>> + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer);
>>>> + if (ACPI_FAILURE(status))
>>>> + return -ENODEV;
>>>> +
>>>> + obj = buffer.pointer;
>>>> + if (!obj) {
>>>> + dev_err(&adev->dev, "ACPI device has no CLDB object\n");
>>> Is this the code path that is taken on Chrome OS ? If so an error
>>> message isn't appropriate. I'd drop this message, and instead add an
>>> error message in the discrete PMIC code.
>> Ah yes of course, thanks, I'll move the error message.
>>
>>>> +
>>>> + unsigned int n_gpios; /* how many GPIOs have we seen */
>>>> +
>>>> + struct int3472_gpio_regulator regulator;
>>>> + struct int3472_gpio_clock clock;
>>> You don't necessarily need to define separate structures for this, you
>>> could also write
>>>
>>> struct {
>>> char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
>>> char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
>>> struct gpio_desc *gpio;
>>> struct regulator_dev *rdev;
>>> struct regulator_desc rdesc;
>>> } regulator;
>>>
>>> struct {
>>> struct clk *clk;
>>> struct clk_hw clk_hw;
>>> struct clk_lookup *cl;
>>> struct gpio_desc *gpio;
>>> } clock;
>>>
>>> It's entirely up to you.
>> Ooh yeah I like that more, thanks very much.
>>
>>>> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */
>>>> +static const guid_t int3472_gpio_guid =
>>>> + GUID_INIT(0x79234640, 0x9e10, 0x4fea,
>>>> +0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
>>>> +
>>>> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */
>>>> +static const guid_t cio2_sensor_module_guid =
>>>> + GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>>>> +0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
>>> A comment that explains what those DSM functions do would be useful for
>>> reference. It has taken lots of time to figure it out, let's spare th
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Andy, Laurent
On 18/01/2021 21:19, Daniel Scally wrote:
>>>> +static const struct clk_ops skl_int3472_clock_ops = {
>>>> + .prepare = skl_int3472_clk_prepare,
>>>> + .unprepare = skl_int3472_clk_unprepare,
>>>> + .enable = skl_int3472_clk_enable,
>>>> + .disable = skl_int3472_clk_disable,
>>>> +};
>> Yeah, sounds like reinventing clk-gpio.c.
>>
>> static const struct clk_ops clk_gpio_gate_ops = {
>> .enable = clk_gpio_gate_enable,
>> .disable = clk_gpio_gate_disable,
>> .is_enabled = clk_gpio_gate_is_enabled,
>> };
>>
>> (Or is it mux? It has support there as well.
>>
> Hmm, yeah, this looks like it would work actually. So I think I'd need to:
>
>
> 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver
>
> 2. Register a platform device to bind to the clk-gpio driver
>
> 3. Register a gpio lookup table so that the clk-gpio driver can find the
> gpio in question using gpiod_get()
>
>
> And that looks like it will work; I'll try it.
I'm more and more confident that this will work, but it has some
knock-on effects:
The both clk and regulator gpio driver expects to be able to fetch the
GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of
course, so we need to add another GPIO lookup table so those drivers can
see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will
be so we can set the .dev_id member of a gpiod_lookup_table to that
value, but that isn't set until _after_ the pdev is registered (because
it has to figure out the id, we can't manually set the IDs because there
could be more than one instance of int3472-discrete bound to multiple
PMIC devices, and we don't know which id the current one should have).
Finally, we can't wait until the device is registered because it
immediately probes, can't find the GPIO and then fails probe.
It's similar problem that causes us to need the i2c-acpi name format
macros, but complicated by the dynamic ID part of dev_name(&pdev->dev)
Solving it is a bit of a sticky one; perhaps something like moving the
dev_set_name() part of platform_device_add() [1] to its own function,
that's called in both platform_device_alloc() and
platform_device_register(). That way it would be available before the
device itself was registered, meaning we could create the lookup table
before it probes the driver.
(also, Laurent, if we did it this way we wouldn't be able to also handle
the led-indicator GPIO here without some fairly major rework)
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L563
Re: [PATCH v2 5/7] gpio: gpiolib-acpi: Export acpi_get_gpiod()
On 18/01/2021 13:45, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:26AM +0000, Daniel Scally wrote: >> I need to be able to translate GPIO resources in an acpi_device's _CRS > > ACPI device's > >> into gpio_descs. Those are represented in _CRS as a pathname to a GPIO > > into GPIO descriptor array > >> device plus the pin's index number: this function is perfect for that >> purpose. > > ... > >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > Wrong header. Please use gpio/consumer.h. > Ack to all - thanks.
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Andy - thanks as always for the comments
Some responses below, but if not mentioned I'll follow your suggestion
of course
On 18/01/2021 14:46, Andy Shevchenko wrote:
> On Mon, Jan 18, 2021 at 11:15:21AM +0200, Laurent Pinchart wrote:
>> On Mon, Jan 18, 2021 at 12:34:27AM +, Daniel Scally wrote:
> My comments on top of Laurent's to avoid dups.
>
> First of all, PDx86 has its own prefix pattern: "platform/x86: ..."
Sorry, I probably should have put more effort into figuring out the
right pattern for those
>>> +config INTEL_SKL_INT3472
>>> + tristate "Intel SkyLake ACPI INT3472 Driver"
>>> + depends on X86 && ACPI
> X86 is a dup. Entire lot of PDx86 is a priory dependent on it (find "if X86"
> line in Kconfig).
Ah, oh yeah - thanks. I'll watch for that in future
>>> +static struct i2c_driver int3472_tps68470 = {
>>> + .driver = {
>>> + .name = "int3472-tps68470",
>>> + .acpi_match_table = int3472_device_id,
>>> + },
>>> + .probe_new = skl_int3472_tps68470_probe,
>>> +};
> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can
> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it
> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better...
Sorry, I'm a bit confused by this. The i2c device is already
present...we just want the driver to bind to them, so what role do those
functions have there?
>>> +struct int3472_gpio_clock {
>>> + struct clk *clk;
>>> + struct clk_hw clk_hw;
>>> + struct clk_lookup *cl;
>>> + struct gpio_desc *gpio;
>>> +};
> Wondering if this has some similarities with and actually can utilize clk-gpio
> driver.
>
>
>
>>> +static const struct clk_ops skl_int3472_clock_ops = {
>>> + .prepare = skl_int3472_clk_prepare,
>>> + .unprepare = skl_int3472_clk_unprepare,
>>> + .enable = skl_int3472_clk_enable,
>>> + .disable = skl_int3472_clk_disable,
>>> +};
> Yeah, sounds like reinventing clk-gpio.c.
>
> static const struct clk_ops clk_gpio_gate_ops = {
> .enable = clk_gpio_gate_enable,
> .disable = clk_gpio_gate_disable,
> .is_enabled = clk_gpio_gate_is_enabled,
> };
>
> (Or is it mux? It has support there as well.
>
Hmm, yeah, this looks like it would work actually. So I think I'd need to:
1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver
2. Register a platform device to bind to the clk-gpio driver
3. Register a gpio lookup table so that the clk-gpio driver can find the
gpio in question using gpiod_get()
And that looks like it will work; I'll try it.
>>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */
>>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL},
>>> + /* Surface Go 1&2 - OV5693, Front */
>>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL},
> I'm wondering if you should use same I2C format macro and create this
> dynamically? Or rather find a corresponding ACPI device instance and
> copy it's name? ...
The supply name needs hard-coding really, but the device name I suppose
can come from int3472->sensor_name.
>>> +static struct int3472_sensor_config *
>>> +skl_int3472_get_sensor_module_config(struct int3472_device *int3472)
>>> +{
>>> + unsigned int i = ARRAY_SIZE(int3472_sensor_configs);
>>> + struct int3472_sensor_config *ret;
>>> + union acpi_object *obj;
>>> +
>>> + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle,
>>> + &cio2_sensor_module_guid, 0x00,
>>> + 0x01, NULL, ACPI_TYPE_STRING);
>>> +
>>> + if (!obj) {
>>> + dev_err(&int3472->pdev->dev,
>>> + "Failed to get sensor module string from _DSM\n");
>>> + return ERR_PTR(-ENODEV);
>>> + }
>>> +
>>> + if (obj->string.type != ACPI_TYPE_STRING) {
>>> + dev_err(&int3472->pdev->dev,
>>> + "Sensor _DSM returned a non-string value\n");
>>> + ret = ERR_PTR(-EINVAL);
>>> + goto out_free_obj;
> And after below change you can turn this to
>
> ACPI_FREE(obj);
> return ERR_PTR(-EINVAL);
>
> and remove label completely, but it's up to you.
>
>>> + }
>>> + ret = ERR_PTR(-ENODEV
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
On 18/01/2021 16:14, Rafael J. Wysocki wrote:
> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally wrote:
>> In some ACPI tables we encounter, devices use the _DEP method to assert
>> a dependence on other ACPI devices as opposed to the OpRegions that the
>> specification intends. We need to be able to find those devices "from"
>> the dependee, so add a function to parse all ACPI Devices and check if
>> the include the handle of the dependee device in their _DEP buffer.
> What exactly do you need this for?
So, in our DSDT we have devices with _HID INT3472, plus sensors which
refer to those INT3472's in their _DEP method. The driver binds to the
INT3472 device, we need to find the sensors dependent on them.
> Would it be practical to look up the suppliers in acpi_dep_list instead?
>
> Note that supplier drivers may remove entries from there, but does
> that matter for your use case?
Ah - that may work, yes. Thank you, let me test that.
>
>> Signed-off-by: Daniel Scally
>> ---
>> Changes in v2:
>> - Used acpi_lpss_dep() as Andy suggested.
>>
>> drivers/acpi/utils.c| 34 ++
>> include/acpi/acpi_bus.h | 2 ++
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index 78b38775f18b..ec6a2406a886 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev,
>> acpi_handle handle)
>> return false;
>> }
>>
>> +static int acpi_dev_match_by_dep(struct device *dev, const void *data)
>> +{
>> + struct acpi_device *adev = to_acpi_device(dev);
>> + const struct acpi_device *dependee = data;
>> + acpi_handle handle = dependee->handle;
>> +
>> + if (acpi_lpss_dep(adev, handle))
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * acpi_dev_present - Detect that a given ACPI device is present
>> * @hid: Hardware ID of the device.
>> @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid,
>> s64 hrv)
>> }
>> EXPORT_SYMBOL(acpi_dev_present);
>>
>> +/**
>> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input
>> dev
>> + * @adev: Pointer to the dependee device
>> + * @prev: Pointer to the previous dependent device (or NULL for first match)
>> + *
>> + * Return the next ACPI device which declares itself dependent on @adev in
>> + * the _DEP buffer.
>> + *
>> + * The caller is responsible to call put_device() on the returned device.
>> + */
>> +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev,
>> + struct acpi_device *prev)
>> +{
>> + struct device *start = prev ? &prev->dev : NULL;
>> + struct device *dev;
>> +
>> + dev = bus_find_device(&acpi_bus_type, start, adev,
>> acpi_dev_match_by_dep);
>> +
>> + return dev ? to_acpi_device(dev) : NULL;
>> +}
>> +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev);
>> +
>> /**
>> * 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
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index 02a716a0af5d..33deb22294f2 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -683,6 +683,8 @@ 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_dep_dev(struct acpi_device *adev, struct acpi_device
>> *prev);
>> struct acpi_device *
>> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid,
>> const char *uid, s64 hrv);
>> struct acpi_device *
>> --
>> 2.25.1
>>
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Laurent, thanks for the comments - really appreciate the detail.
Some specific responses below but assume a general "will do" to
everything you mentioned otherwise...
On 18/01/2021 09:15, Laurent Pinchart wrote:
>> + PMIC) and one designed for Chrome OS.
> How about expanding this a bit to explain what the INT3472 stands for ?
>
> The INT3472 is an Intel camera power controller, a logical device
> found on some Skylake-based systems that can map to different
> hardware devices depending on the platform. On machines
> designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
> machines designed for Windows, it maps to either a TP68470
> camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
> and power gates.
Yeah sure ok
>> This driver handles all three
>> + situations by discovering information it needs to discern them at
>> + runtime.
>> +
>> + If your device was designed for Chrome OS, this driver will provide
>> + an ACPI operation region, which must be available before any of the
>> + devices using this are probed. For this reason, you should select Y
>> + if your device was designed for ChromeOS. This option also configures
>> + the designware-i2c driver to be built-in, for the same reason.
> Is the last sentence a leftover ?
Oops - it is, but it was supposed to remind me to double check that that
was still necessary. I'll take a look, thanks.
>> +
>> +#include "intel_skl_int3472_common.h"
>> +
>> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev,
>> +struct int3472_cldb *cldb)
>> +{
>> +struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +acpi_handle handle = adev->handle;
>> +union acpi_object *obj;
>> +acpi_status status;
>> +int ret = 0;
>> +
>> +status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer);
>> +if (ACPI_FAILURE(status))
>> +return -ENODEV;
>> +
>> +obj = buffer.pointer;
>> +if (!obj) {
>> +dev_err(&adev->dev, "ACPI device has no CLDB object\n");
> Is this the code path that is taken on Chrome OS ? If so an error
> message isn't appropriate. I'd drop this message, and instead add an
> error message in the discrete PMIC code.
Ah yes of course, thanks, I'll move the error message.
>> +
>> +unsigned int n_gpios; /* how many GPIOs have we seen */
>> +
>> +struct int3472_gpio_regulator regulator;
>> +struct int3472_gpio_clock clock;
> You don't necessarily need to define separate structures for this, you
> could also write
>
> struct {
> char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
> char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
> struct gpio_desc *gpio;
> struct regulator_dev *rdev;
> struct regulator_desc rdesc;
> } regulator;
>
> struct {
> struct clk *clk;
> struct clk_hw clk_hw;
> struct clk_lookup *cl;
> struct gpio_desc *gpio;
> } clock;
>
> It's entirely up to you.
Ooh yeah I like that more, thanks very much.
>> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */
>> +static const guid_t int3472_gpio_guid =
>> +GUID_INIT(0x79234640, 0x9e10, 0x4fea,
>> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
>> +
>> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */
>> +static const guid_t cio2_sensor_module_guid =
>> +GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
> A comment that explains what those DSM functions do would be useful for
> reference. It has taken lots of time to figure it out, let's spare the
> pain to the next person who tries to understand this :-)
Hah - good point, well made. I'll explain what they're for then.
>> +static int skl_int3472_clk_enable(struct clk_hw *hw)
>> +{
>> +struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> +
>> +gpiod_set_value(clk->gpio, 1);
> The clock enable() and disable() methods are not supposed to sleep,
> while setting a GPIO value may sleep in the general case. Should this be
> moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable()
> and skl_int3472_clk_unprepare().
I was under the assumption the difference between gpiod_set_value() and
gpiod_set_value_cansleep() was that gpiod_set_value() _can't_ sleep, but
actually reading the function's comments it seems it will just complain
if it turns out it can sleep:
* This function can be called from contexts where we cannot sleep, and will
* complain if the GPIO chip functions potentially sleep. It doesn't
complain, on either of my devices, but I guess that can't be guaranteed
for _every_ device, so these calls probably are safer in (un)prepare() yes.
>> +}
>> +
>> +i++;
>> +}
>> +}
>> +
>> +if (!func)
>> +return 0;
> I in
Re: [PATCH v2] software_node: Add kernel-doc comments to exported symbols
On 18/01/2021 10:33, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 11:49:46AM +0200, Sakari Ailus wrote: >> On Wed, Jan 13, 2021 at 12:02:09AM +0000, Daniel Scally wrote: >>> + * to_software_node() - Fetch software node associated with a firmware >>> node handle >> Please wrap lines over 80 (unless there's a reason to keep them longer). > Does kernel-doc behave good when you wrap the function summary line? > My impression that summary should be one line. > Same, but I can shorten the line by s/associated with/linked to or something along those lines
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On 18/01/2021 15:48, andriy.shevche...@linux.intel.com wrote: > On Mon, Jan 18, 2021 at 04:32:54PM +0100, Hans de Goede wrote: >> On 1/18/21 4:23 PM, andriy.shevche...@linux.intel.com wrote: > ... > >> 1. Using a folder is fine, desirable even >> 2. I've some concerns about the name, but I'm not really objecting, >> just giving my 2 cents. > Let's get into compromised summary: > - create a folder for these driver files > - name it without _skl_ while leaving this in the file / driver names > > Does everybody agree on this approach? > Works for me!
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
On 18/01/2021 12:33, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:23AM +0000, Daniel Scally wrote: >> In some ACPI tables we encounter, devices use the _DEP method to assert >> a dependence on other ACPI devices as opposed to the OpRegions that the >> specification intends. We need to be able to find those devices "from" >> the dependee, so add a function to parse all ACPI Devices and check if >> the include the handle of the dependee device in their _DEP buffer. > Fix prefix to be "ACPI / utils: " and rebase on top of function name changes > as > suggested by Laurent. OK. >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input >> dev > Are you expecting some kind of for_each_*() macro to be added and used? > Otherwise there is probably no need to have it "_next_" in the name as it will > be a bit confusing. I thought that somebody might want to do that in the future yes, although I've no need for it at the minute because each of the dummy INT3472 devices only has one dependent sensor that we've seen so far. But for the INT3472 devices representing a physical TPS68470, there are platforms where 2 sensors are called out as dependent on the same PMIC ACPI device (Surface Go2 for example). It can be renamed and just cross that bridge when we come to it though, I have no problem with that. > >> + * @adev: Pointer to the dependee device >> + * @prev: Pointer to the previous dependent device (or NULL for first match) >> + * >> + * Return the next ACPI device which declares itself dependent on @adev in >> + * the _DEP buffer. >> + * >> + * The caller is responsible to call put_device() on the returned device. >> + */
Re: [PATCH v2 1/7] acpi: utils: move acpi_lpss_dep() to utils
On 18/01/2021 12:29, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 09:24:10AM +0200, Laurent Pinchart wrote: >> On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: > ... > >>> +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); >> "lpss" stands for low power subsystem, an Intel device within the PCH >> that handles I2C, SPI, UART, ... I think the function should be renamed, >> as it's now generic. acpi_dev_has_dep() is a potential candidate, I'm >> sure better ones exist. A bit of kerneldoc would also not hurt. > Actually a good suggestions. Please apply my tag after addressing above. Will do, thanks
Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
Morning Laurent
On 18/01/2021 07:34, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Jan 18, 2021 at 12:34:23AM +, Daniel Scally wrote:
>> In some ACPI tables we encounter, devices use the _DEP method to assert
>> a dependence on other ACPI devices as opposed to the OpRegions that the
>> specification intends. We need to be able to find those devices "from"
>> the dependee, so add a function to parse all ACPI Devices and check if
>> the include the handle of the dependee device in their _DEP buffer.
>>
>> Signed-off-by: Daniel Scally
>> ---
>> Changes in v2:
>> - Used acpi_lpss_dep() as Andy suggested.
>>
>> drivers/acpi/utils.c| 34 ++
>> include/acpi/acpi_bus.h | 2 ++
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index 78b38775f18b..ec6a2406a886 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev,
>> acpi_handle handle)
>> return false;
>> }
>>
>> +static int acpi_dev_match_by_dep(struct device *dev, const void *data)
>> +{
>> +struct acpi_device *adev = to_acpi_device(dev);
>> +const struct acpi_device *dependee = data;
>> +acpi_handle handle = dependee->handle;
>> +
>> +if (acpi_lpss_dep(adev, handle))
>> +return 1;
>> +
>> +return 0;
>> +}
>> +
> I think I'd move this just before acpi_dev_get_next_dep_dev() to keep
> the two together.
Will do
>
>> /**
>> * acpi_dev_present - Detect that a given ACPI device is present
>> * @hid: Hardware ID of the device.
>> @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid,
>> s64 hrv)
>> }
>> EXPORT_SYMBOL(acpi_dev_present);
>>
>> +/**
>> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input
>> dev
> Maybe acpi_dev_get_next_dependent_dev() ? "dep" could mean either
> dependent or dependency.
Yes, good point, I agree.
>
>> + * @adev: Pointer to the dependee device
>> + * @prev: Pointer to the previous dependent device (or NULL for first match)
>> + *
>> + * Return the next ACPI device which declares itself dependent on @adev in
>> + * the _DEP buffer.
>> + *
>> + * The caller is responsible to call put_device() on the returned device.
>> + */
>> +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev,
>> + struct acpi_device *prev)
>> +{
>> +struct device *start = prev ? &prev->dev : NULL;
>> +struct device *dev;
>> +
>> +dev = bus_find_device(&acpi_bus_type, start, adev,
>> acpi_dev_match_by_dep);
> Having to loop over all ACPI devices is quite inefficient, but if we
> need a reverse lookup, we don't really have a choice. We could create a
> reverse map, but I don't think it's worth it.
>
>> +
>> +return dev ? to_acpi_device(dev) : NULL;
>> +}
>> +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev);
> I would have used EXPORT_SYMBOL_GPL. I'm not sure what the policy is in
> the ACPI subsystem, and it's also a personal choice.
EXPORT_SYMBOL_GPL would be my usual choice, but the other functions in
the file all use EXPORT_SYMBOL, so I assumed there was some policy that
that be used (since basically everywhere else I've touched in the kernel
so far defaults to the GPL version)
>
> Reviewed-by: Laurent Pinchart
Thanks!
>
>> +
>> /**
>> * 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
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index 02a716a0af5d..33deb22294f2 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -683,6 +683,8 @@ 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_dep_dev(struct acpi_device *adev, struct acpi_device
>> *prev);
>> struct acpi_device *
>> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid,
>> const char *uid, s64 hrv);
>> struct acpi_device *
Re: [PATCH v2 1/7] acpi: utils: move acpi_lpss_dep() to utils
Hi Laurent
On 18/01/2021 07:24, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Jan 18, 2021 at 12:34:22AM +, Daniel Scally wrote:
>> I need to be able to identify devices which declare themselves to be
>> dependent on other devices through _DEP; add this function to utils.c
>> and export it to the rest of the ACPI layer.
>>
>> Suggested-by: Andy Shevchenko
>> Signed-off-by: Daniel Scally
>> ---
>> Changes in v2:
>> - Introduced
>>
>> drivers/acpi/acpi_lpss.c | 24
>> drivers/acpi/internal.h | 1 +
>> drivers/acpi/utils.c | 24
>> 3 files changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index be73974ce449..70c7d9a3f715 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char
>> *hid, const char *uid)
>> return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid);
>> }
>>
>> -static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
>> -{
>> -struct acpi_handle_list dep_devices;
>> -acpi_status status;
>> -int i;
>> -
>> -if (!acpi_has_method(adev->handle, "_DEP"))
>> -return false;
>> -
>> -status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
>> - &dep_devices);
>> -if (ACPI_FAILURE(status)) {
>> -dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
>> -return false;
>> -}
>> -
>> -for (i = 0; i < dep_devices.count; i++) {
>> -if (dep_devices.handles[i] == handle)
>> -return true;
>> -}
>> -
>> -return false;
>> -}
>> -
>> static void acpi_lpss_link_consumer(struct device *dev1,
>> const struct lpss_device_links *link)
>> {
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index cb229e24c563..ee62c0973576 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {}
>> #endif
>>
>> void acpi_apd_init(void);
>> +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle);
> "lpss" stands for low power subsystem, an Intel device within the PCH
> that handles I2C, SPI, UART, ... I think the function should be renamed,
> as it's now generic. acpi_dev_has_dep() is a potential candidate, I'm
> sure better ones exist. A bit of kerneldoc would also not hurt.
Okedokey; I shall add kerneldoc and think of an appropriate name, plus
rename all the uses of it. How about acpi_dev_is_dep()? "has_dep" to me
implies anything at all in _DEP should return true.
>>
>> acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
>> bool acpi_queue_hotplug_work(struct work_struct *work);
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index ddca1550cce6..78b38775f18b 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const
>> void *data)
>> return hrv == match->hrv;
>> }
>>
>> +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
>> +{
>> +struct acpi_handle_list dep_devices;
>> +acpi_status status;
>> +int i;
>> +
>> +if (!acpi_has_method(adev->handle, "_DEP"))
>> +return false;
>> +
>> +status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
>> + &dep_devices);
>> +if (ACPI_FAILURE(status)) {
>> +dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
>> +return false;
>> +}
>> +
>> +for (i = 0; i < dep_devices.count; i++) {
>> +if (dep_devices.handles[i] == handle)
>> +return true;
>> +}
>> +
>> +return false;
>> +}
>> +
>> /**
>> * acpi_dev_present - Detect that a given ACPI device is present
>> * @hid: Hardware ID of the device.
[PATCH v2 1/7] acpi: utils: move acpi_lpss_dep() to utils
I need to be able to identify devices which declare themselves to be
dependent on other devices through _DEP; add this function to utils.c
and export it to the rest of the ACPI layer.
Suggested-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
Changes in v2:
- Introduced
drivers/acpi/acpi_lpss.c | 24
drivers/acpi/internal.h | 1 +
drivers/acpi/utils.c | 24
3 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index be73974ce449..70c7d9a3f715 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char
*hid, const char *uid)
return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid);
}
-static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
-{
- struct acpi_handle_list dep_devices;
- acpi_status status;
- int i;
-
- if (!acpi_has_method(adev->handle, "_DEP"))
- return false;
-
- status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
-&dep_devices);
- if (ACPI_FAILURE(status)) {
- dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
- return false;
- }
-
- for (i = 0; i < dep_devices.count; i++) {
- if (dep_devices.handles[i] == handle)
- return true;
- }
-
- return false;
-}
-
static void acpi_lpss_link_consumer(struct device *dev1,
const struct lpss_device_links *link)
{
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index cb229e24c563..ee62c0973576 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {}
#endif
void acpi_apd_init(void);
+bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle);
acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
bool acpi_queue_hotplug_work(struct work_struct *work);
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index ddca1550cce6..78b38775f18b 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const
void *data)
return hrv == match->hrv;
}
+bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
+{
+ struct acpi_handle_list dep_devices;
+ acpi_status status;
+ int i;
+
+ if (!acpi_has_method(adev->handle, "_DEP"))
+ return false;
+
+ status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
+&dep_devices);
+ if (ACPI_FAILURE(status)) {
+ dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
+ return false;
+ }
+
+ for (i = 0; i < dep_devices.count; i++) {
+ if (dep_devices.handles[i] == handle)
+ return true;
+ }
+
+ return false;
+}
+
/**
* acpi_dev_present - Detect that a given ACPI device is present
* @hid: Hardware ID of the device.
--
2.25.1
[PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
In some ACPI tables we encounter, devices use the _DEP method to assert
a dependence on other ACPI devices as opposed to the OpRegions that the
specification intends. We need to be able to find those devices "from"
the dependee, so add a function to parse all ACPI Devices and check if
the include the handle of the dependee device in their _DEP buffer.
Signed-off-by: Daniel Scally
---
Changes in v2:
- Used acpi_lpss_dep() as Andy suggested.
drivers/acpi/utils.c| 34 ++
include/acpi/acpi_bus.h | 2 ++
2 files changed, 36 insertions(+)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 78b38775f18b..ec6a2406a886 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle
handle)
return false;
}
+static int acpi_dev_match_by_dep(struct device *dev, const void *data)
+{
+ struct acpi_device *adev = to_acpi_device(dev);
+ const struct acpi_device *dependee = data;
+ acpi_handle handle = dependee->handle;
+
+ if (acpi_lpss_dep(adev, handle))
+ return 1;
+
+ return 0;
+}
+
/**
* acpi_dev_present - Detect that a given ACPI device is present
* @hid: Hardware ID of the device.
@@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid,
s64 hrv)
}
EXPORT_SYMBOL(acpi_dev_present);
+/**
+ * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev
+ * @adev: Pointer to the dependee device
+ * @prev: Pointer to the previous dependent device (or NULL for first match)
+ *
+ * Return the next ACPI device which declares itself dependent on @adev in
+ * the _DEP buffer.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ */
+struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev,
+ struct acpi_device *prev)
+{
+ struct device *start = prev ? &prev->dev : NULL;
+ struct device *dev;
+
+ dev = bus_find_device(&acpi_bus_type, start, adev,
acpi_dev_match_by_dep);
+
+ return dev ? to_acpi_device(dev) : NULL;
+}
+EXPORT_SYMBOL(acpi_dev_get_next_dep_dev);
+
/**
* 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
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 02a716a0af5d..33deb22294f2 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -683,6 +683,8 @@ 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_dep_dev(struct acpi_device *adev, struct acpi_device *prev);
struct acpi_device *
acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const
char *uid, s64 hrv);
struct acpi_device *
--
2.25.1
[PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
ACPI devices with _HID INT3472 are currently matched to the tps68470 driver, however this does not cover all situations in which that _HID occurs. We've encountered three possibilities: 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing a physical tps68470 device) that requires a GPIO and OpRegion driver 2. On devices designed for Windows, an ACPI device with _HID INT3472 (again representing a physical tps68470 device) which requires GPIO, Clock and Regulator drivers. 3. On other devices designed for Windows, an ACPI device with _HID INT3472 which does NOT represent a physical tps68470, and is instead used as a dummy device to group some system GPIO lines which are meant to be consumed by the sensor that is dependent on this entry. This commit adds a new module, registering a platform driver to deal with the 3rd scenario plus an i2c-driver to deal with #1 and #2, by querying the CLDB buffer found against INT3472 entries to determine which is most appropriate. Suggested-by: Laurent Pinchart Signed-off-by: Daniel Scally --- Changes in v2: - Switched to a module registering a platform driver to run the dummy ACPI devices, plus an i2c driver to replace and extend the existing tps68470 driver - Added clock handling functions to the int3472-discrete driver - A whole bunch of other changes too numerous to enumerate MAINTAINERS | 5 + drivers/platform/x86/Kconfig | 25 + drivers/platform/x86/Makefile | 4 + .../platform/x86/intel_skl_int3472_common.c | 100 .../platform/x86/intel_skl_int3472_common.h | 100 .../platform/x86/intel_skl_int3472_discrete.c | 496 ++ .../platform/x86/intel_skl_int3472_tps68470.c | 145 + 7 files changed, 875 insertions(+) create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c diff --git a/MAINTAINERS b/MAINTAINERS index a091b496fdd8..c4ed8c3bc58e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9147,6 +9147,11 @@ S: Maintained F: arch/x86/include/asm/intel_scu_ipc.h F: drivers/platform/x86/intel_scu_* +INTEL SKL INT3472 ACPI DEVICE DRIVER +M: Daniel Scally +S: Maintained +F: drivers/platform/x86/intel_skl_int3472_* + INTEL SPEED SELECT TECHNOLOGY M: Srinivas Pandruvada L: platform-driver-...@vger.kernel.org diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 91e6176cdfbd..916b077df2d5 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -844,6 +844,31 @@ config INTEL_CHT_INT33FE device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m for Type-C device. +config INTEL_SKL_INT3472 + tristate "Intel SkyLake ACPI INT3472 Driver" + depends on X86 && ACPI + select REGMAP_I2C + help + This driver adds support for the INT3472 ACPI devices found on some + Intel SkyLake devices. + + There are 3 kinds of INT3472 ACPI device possible; two for devices + designed for Windows (either with or without a physical tps68470 + PMIC) and one designed for Chrome OS. This driver handles all three + situations by discovering information it needs to discern them at + runtime. + + If your device was designed for Chrome OS, this driver will provide + an ACPI operation region, which must be available before any of the + devices using this are probed. For this reason, you should select Y + if your device was designed for ChromeOS. This option also configures + the designware-i2c driver to be built-in, for the same reason. + + Say Y or M here if you have a SkyLake device designed for use + with Windows or ChromeOS. Say N here if you are not sure. + + The module will be named "intel-skl-int3472" + config INTEL_HID_EVENT tristate "INTEL HID Event" depends on ACPI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 581475f59819..ae29c66842ca 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -86,6 +86,10 @@ obj-$(CONFIG_INTEL_HID_EVENT)+= intel-hid.o obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o +obj-$(CONFIG_INTEL_SKL_INT3472)+= intel_skl_int3472.o +intel_skl_int3472-objs := intel_skl_int3472_common.o \ + intel_skl_int3472_discrete.o \ + intel_skl_int3472_tps68470.o obj-$(CONFI
[PATCH v2 4/7] i2c: i2c-core-acpi: Add i2c_acpi_dev_name()
We want to refer to an i2c device by name before it has been
created by the kernel; add a function that constructs the name
from the acpi device instead.
Signed-off-by: Daniel Scally
---
Changes in v2:
- Stopped using devm_kasprintf()
drivers/i2c/i2c-core-acpi.c | 16
include/linux/i2c.h | 5 +
2 files changed, 21 insertions(+)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 37c510d9347a..98c3ba9a2350 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev,
int index,
}
EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
+/**
+ * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI
+ * @adev: ACPI device to construct the name for
+ *
+ * Constructs the name of an i2c device matching the format used by
+ * i2c_dev_set_name() to allow users to refer to an i2c device by name even
+ * before they have been instantiated.
+ *
+ * The caller is responsible for freeing the returned pointer.
+ */
+char *i2c_acpi_dev_name(struct acpi_device *adev)
+{
+ return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_dev_name);
+
#ifdef CONFIG_ACPI_I2C_OPREGION
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
u8 cmd, u8 *data, u8 data_len)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4d40a4b46810..b82aac05b17f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -998,6 +998,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
u32 i2c_acpi_find_bus_speed(struct device *dev);
struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
struct i2c_board_info *info);
+char *i2c_acpi_dev_name(struct acpi_device *adev);
struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
#else
static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
@@ -1014,6 +1015,10 @@ static inline struct i2c_client
*i2c_acpi_new_device(struct device *dev,
{
return ERR_PTR(-ENODEV);
}
+static inline char *i2c_acpi_dev_name(struct acpi_device *adev)
+{
+ return NULL;
+}
static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle
handle)
{
return NULL;
--
2.25.1
[PATCH v2 3/7] i2c: i2c-core-base: Use format macro in i2c_dev_set_name()
Some places in the kernel allow users to map resources to a device
using device name (for example, gpiod_lookup_table). Currently
this involves waiting for the i2c_client to have been registered so we
can use dev_name(&client->dev). We want to add a function to allow users
to refer to an i2c device by name before it has been instantiated, so
create a macro for the format that's accessible outside the i2c layer
and use it in i2c_dev_set_name()
Suggested-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
- Used format macro in i2c_dev_set_name() instead of sub func
drivers/i2c/i2c-core-base.c | 4 ++--
include/linux/i2c.h | 3 +++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf722a424..547b8926cac8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -811,12 +811,12 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
struct acpi_device *adev = ACPI_COMPANION(&client->dev);
if (info && info->dev_name) {
- dev_set_name(&client->dev, "i2c-%s", info->dev_name);
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name);
return;
}
if (adev) {
- dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT,
acpi_dev_name(adev));
return;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b215..4d40a4b46810 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -39,6 +39,9 @@ enum i2c_slave_event;
typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
enum i2c_slave_event event, u8 *val);
+/* I2C Device Name Format - to maintain consistency outside the i2c layer */
+#define I2C_DEV_NAME_FORMAT"i2c-%s"
+
/* I2C Frequency Modes */
#define I2C_MAX_STANDARD_MODE_FREQ 10
#define I2C_MAX_FAST_MODE_FREQ 40
--
2.25.1
[PATCH v2 5/7] gpio: gpiolib-acpi: Export acpi_get_gpiod()
I need to be able to translate GPIO resources in an acpi_device's _CRS
into gpio_descs. Those are represented in _CRS as a pathname to a GPIO
device plus the pin's index number: this function is perfect for that
purpose.
Signed-off-by: Daniel Scally
---
Changes in v2:
-None
drivers/gpio/gpiolib-acpi.c | 3 ++-
include/linux/acpi.h| 5 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e37a57d0a2f0..83f9f85cd0ab 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -111,7 +111,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void
*data)
* controller does not have GPIO chip registered at the moment. This is to
* support probe deferral.
*/
-static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+struct gpio_desc *acpi_get_gpiod(char *path, int pin)
{
struct gpio_chip *chip;
acpi_handle handle;
@@ -127,6 +127,7 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
return gpiochip_get_desc(chip, pin);
}
+EXPORT_SYMBOL_GPL(acpi_get_gpiod);
static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
{
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 2630c2e953f7..5cd272326eb7 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1066,6 +1066,7 @@ void __acpi_handle_debug(struct _ddebug *descriptor,
acpi_handle handle, const c
bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio);
int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index);
+struct gpio_desc *acpi_get_gpiod(char *path, int pin);
#else
static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio)
@@ -1076,6 +1077,10 @@ static inline int acpi_dev_gpio_irq_get(struct
acpi_device *adev, int index)
{
return -ENXIO;
}
+struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+{
+ return NULL;
+}
#endif
/* Device properties */
--
2.25.1
[PATCH v2 7/7] mfd: Remove tps68470 MFD driver
This driver only covered one scenario in which ACPI devices with _HID
INT3472 are found, and its functionality has been taken over by the
intel-skl-int3472 module, so remove it.
Signed-off-by: Daniel Scally
---
Changes in v2:
- Introduced
drivers/acpi/pmic/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/mfd/Kconfig | 18
drivers/mfd/Makefile | 1 -
drivers/mfd/tps68470.c| 97 ---
5 files changed, 118 deletions(-)
delete mode 100644 drivers/mfd/tps68470.c
diff --git a/drivers/acpi/pmic/Kconfig b/drivers/acpi/pmic/Kconfig
index 56bbcb2ce61b..e27d8ef3a32c 100644
--- a/drivers/acpi/pmic/Kconfig
+++ b/drivers/acpi/pmic/Kconfig
@@ -52,7 +52,6 @@ endif # PMIC_OPREGION
config TPS68470_PMIC_OPREGION
bool "ACPI operation region support for TPS68470 PMIC"
- depends on MFD_TPS68470
help
This config adds ACPI operation region support for TI TPS68470 PMIC.
TPS68470 device is an advanced power management unit that powers
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c70f46e80a3b..07ff8f24b0d9 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1343,7 +1343,6 @@ config GPIO_TPS65912
config GPIO_TPS68470
bool "TPS68470 GPIO"
- depends on MFD_TPS68470
help
Select this option to enable GPIO driver for the TPS68470
chip family.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bdfce7b15621..9a1f648efde0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1520,24 +1520,6 @@ config MFD_TPS65217
This driver can also be built as a module. If so, the module
will be called tps65217.
-config MFD_TPS68470
- bool "TI TPS68470 Power Management / LED chips"
- depends on ACPI && PCI && I2C=y
- depends on I2C_DESIGNWARE_PLATFORM=y
- select MFD_CORE
- select REGMAP_I2C
- help
- If you say yes here you get support for the TPS68470 series of
- Power Management / LED chips.
-
- These include voltage regulators, LEDs and other features
- that are often used in portable devices.
-
- This option is a bool as it provides an ACPI operation
- region, which must be available before any of the devices
- using this are probed. This option also configures the
- designware-i2c driver to be built-in, for the same reason.
-
config MFD_TI_LP873X
tristate "TI LP873X Power Management IC"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 14fdb188af02..5994e812f479 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -105,7 +105,6 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
-obj-$(CONFIG_MFD_TPS68470) += tps68470.o
obj-$(CONFIG_MFD_TPS80031) += tps80031.o
obj-$(CONFIG_MENELAUS) += menelaus.o
diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
deleted file mode 100644
index 4a4df4ffd18c..
--- a/drivers/mfd/tps68470.c
+++ /dev/null
@@ -1,97 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * TPS68470 chip Parent driver
- *
- * Copyright (C) 2017 Intel Corporation
- *
- * Authors:
- * Rajmohan Mani
- * Tianshu Qiu
- * Jian Xu Zheng
- * Yuning Pu
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-static const struct mfd_cell tps68470s[] = {
- { .name = "tps68470-gpio" },
- { .name = "tps68470_pmic_opregion" },
-};
-
-static const struct regmap_config tps68470_regmap_config = {
- .reg_bits = 8,
- .val_bits = 8,
- .max_register = TPS68470_REG_MAX,
-};
-
-static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
-{
- unsigned int version;
- int ret;
-
- /* Force software reset */
- ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
- if (ret)
- return ret;
-
- ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
- if (ret) {
- dev_err(dev, "Failed to read revision register: %d\n", ret);
- return ret;
- }
-
- dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
-
- return 0;
-}
-
-static int tps68470_probe(struct i2c_client *client)
-{
- struct device *dev = &client->dev;
- struct regmap *regmap;
- int ret;
-
- regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
- if (IS_ERR(regmap)) {
- dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
- PTR_ERR(regmap));
- return PTR_ERR(regmap);
- }
-
- i2c_set_clientdata(client, regmap);
-
-
[PATCH v2 0/7] Introduce intel_skl_int3472 driver
Hello all v1 for this series was originally 14-18 of this series: https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrsca...@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67 At the moment in the kernel the ACPI _HID INT3472 is taken by the tps68470 MFD driver, but that driver can only handle some of the cases of that _HID that we see. There are at least these three possibilities: 1. INT3472 devices that provide GPIOs through the usual framework and run power and clocks through an operation region; this is the situation that the current module handles and is seen on ChromeOS devices 2. INT3472 devices that provide GPIOs, plus clocks and regulators that are meant to be driven through the usual frameworks, usually seen on devices designed to run Windows 3. INT3472 devices that don't actually represent a physical tps68470, but are being used as a convenient way of grouping a bunch of system GPIO lines that are intended to enable power and clocks for sensors which are called out as dependent on them. Also seen on devices designed to run Windows. This series introduces a new module which registers: 1. An i2c driver that determines which scenario (#1 or #2) applies to the machine and registers platform devices to be bound to GPIO, OpRegion, clock and regulator drivers as appropriate. 2. A platform driver that binds to the dummy INT3472 devices described in #3 The platform driver for the dummy device registers the GPIO lines that enable the clocks and regulators to the sensors via those frameworks so that sensor drivers can consume them in the usual fashion. The existing GPIO and OpRegion tps68470 drivers will work with the i2c driver that's registered. Clock and regulator drivers are currently in the works. The existing mfd/tps68470.c driver being thus superseded, it is removed. This has been tested on a number of devices; but currently **not** on a ChromeOS, which we ideally need to do to ensure no regression caused by replacing the tps68470 MFD driver. Thanks Dan Daniel Scally (7): acpi: utils: move acpi_lpss_dep() to utils acpi: utils: Add function to fetch dependent acpi_devices i2c: i2c-core-base: Use format macro in i2c_dev_set_name() i2c: i2c-core-acpi: Add i2c_acpi_dev_name() gpio: gpiolib-acpi: Export acpi_get_gpiod() platform: x86: Add intel_skl_int3472 driver mfd: Remove tps68470 MFD driver MAINTAINERS | 5 + drivers/acpi/acpi_lpss.c | 24 - drivers/acpi/internal.h | 1 + drivers/acpi/pmic/Kconfig | 1 - drivers/acpi/utils.c | 58 ++ drivers/gpio/Kconfig | 1 - drivers/gpio/gpiolib-acpi.c | 3 +- drivers/i2c/i2c-core-acpi.c | 16 + drivers/i2c/i2c-core-base.c | 4 +- drivers/mfd/Kconfig | 18 - drivers/mfd/Makefile | 1 - drivers/mfd/tps68470.c| 97 drivers/platform/x86/Kconfig | 25 + drivers/platform/x86/Makefile | 4 + .../platform/x86/intel_skl_int3472_common.c | 100 .../platform/x86/intel_skl_int3472_common.h | 100 .../platform/x86/intel_skl_int3472_discrete.c | 496 ++ .../platform/x86/intel_skl_int3472_tps68470.c | 145 + include/acpi/acpi_bus.h | 2 + include/linux/acpi.h | 5 + include/linux/i2c.h | 8 + 21 files changed, 969 insertions(+), 145 deletions(-) delete mode 100644 drivers/mfd/tps68470.c create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c -- 2.25.1
Re: [PATCH v3 0/4] Remove one more platform_device_add_properties() call
On 17/01/2021 21:05, Andy Shevchenko wrote: > On Sat, Jan 16, 2021 at 11:29 PM Daniel Scally wrote: >> On 16/01/2021 20:23, Andy Shevchenko wrote: >>> On Fri, Jan 15, 2021 at 11:52 AM Heikki Krogerus >>> wrote: >>>> Hi, >>>> >>>> I'm now clearing the dev_fwnode(dev)->secondary pointer in >>>> device_remove_software_node() as requested by Daniel and Andy. Thanks >>>> guys, it's much better now. I also took the liberty of including one >>>> more PCI ID patch where I add PCI ID for the Alder Lake-P variant. I >>>> hope that is OK. >>>> >>>> Andy, I dropped your Tested-by tag because of the change I made to the >>>> first patch. If you have time to retest these, I would much appreciate. >>> Since Greg already grabbed a v3 I will test it when it appears in >>> linux-next. >>> >> It seems the grabbed one is the v2 one though actually > In his last message he wrote that he noticed the v3 *as I understand that*. > Greg, is it right? I mean you took v3 eventually? > You're right: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing&id=e68d0119e3284334de5650a1ac42ef4e179f895e My bad; I went off the automated message but didn't check the tree.
Re: [PATCH v3 0/4] Remove one more platform_device_add_properties() call
On 16/01/2021 20:23, Andy Shevchenko wrote: > On Fri, Jan 15, 2021 at 11:52 AM Heikki Krogerus > wrote: >> Hi, >> >> I'm now clearing the dev_fwnode(dev)->secondary pointer in >> device_remove_software_node() as requested by Daniel and Andy. Thanks >> guys, it's much better now. I also took the liberty of including one >> more PCI ID patch where I add PCI ID for the Alder Lake-P variant. I >> hope that is OK. >> >> Andy, I dropped your Tested-by tag because of the change I made to the >> first patch. If you have time to retest these, I would much appreciate. > Since Greg already grabbed a v3 I will test it when it appears in linux-next. > It seems the grabbed one is the v2 one though actually
Re: [PATCH v2] software_node: Add kernel-doc comments to exported symbols
Hi Randy
On 13/01/2021 01:01, Randy Dunlap wrote:
> On 1/12/21 4:02 PM, Daniel Scally wrote:
>> A number of functions which are exported via EXPORT_SYMBOL_GPL() lack any
>> kernel-doc comments; add those in so all exported symbols are documented.
>>
>> Reviewed-by: Andy Shevchenko
>> Reviewed-by: Heikki Krogerus
>> Signed-off-by: Daniel Scally
>> ---
>> Changes in version 2:
>> - Replaced "fwnode_handle" with either @fwnode or natural language
>> reference to a firmware node handle as appropriate.
>>
>> drivers/base/swnode.c | 53 +++
>> 1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 4a4b2008fbc2..e98018aa8b2f 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -33,6 +33,13 @@ static struct kset *swnode_kset;
>>
>> static const struct fwnode_operations software_node_ops;
>>
>> +/**
>> + * is_software_node() - check if given fwnode was created from a
>> software_node
>> + * @fwnode: The &struct fwnode_handle to check
>> + *
>> + * This function is used to check whether a given firmware node handle was
>> + * created by registering a &struct software_node or not.
>> + */
>> bool is_software_node(const struct fwnode_handle *fwnode)
>> {
>> return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &software_node_ops;
>> @@ -71,6 +78,14 @@ software_node_to_swnode(const struct software_node *node)
>> return swnode;
>> }
>>
>> +/**
>> + * to_software_node() - Fetch software node associated with a firmware node
>> handle
>> + * @fwnode: The pointer to a &struct fwnode_handle to parse
>> + *
>> + * This function attempts to fetch a pointer to the &struct software_node
>> which
>> + * was used to create the given @fwnode. Note that this will only work if
>> the
>> + * software node has **not** been released.
>> + */
>> const struct software_node *to_software_node(const struct fwnode_handle
>> *fwnode)
>> {
>> const struct swnode *swnode = to_swnode(fwnode);
>> @@ -79,6 +94,14 @@ const struct software_node *to_software_node(const struct
>> fwnode_handle *fwnode)
>> }
>> EXPORT_SYMBOL_GPL(to_software_node);
>>
>> +/**
>> + * software_node_fwnode() - Fetch firmware node associated with a given
>> software node
>> + * @node: The pointer to a &struct software_node to parse
>> + *
>> + * This function attempts to fetch a pointer to the &struct fwnode_handle
>> which
>> + * was created from the given @node. Note that this will only work after the
>> + * software node has been registered.
>> + */
>> struct fwnode_handle *software_node_fwnode(const struct software_node *node)
>> {
>> struct swnode *swnode = software_node_to_swnode(node);
>> @@ -800,6 +823,27 @@ void software_node_unregister(const struct
>> software_node *node)
>> }
>> EXPORT_SYMBOL_GPL(software_node_unregister);
>>
>> +/**
>> + * fwnode_create_software_node() - Create and register a new software_node
>> + * @properties: NULL terminated array of properties to assign to the new
>> node
>> + * @parent: Pointer to a &struct fwnode_handle to assign as parent to the
>> new
>> + * node
>> + *
>> + * NOTE: The pointer passed to @parent **must** be to a firmware node handle
> maybe: passed as @parent
> ?
Sure, I'll make that change.
> Otherwise, LGTM. Thanks for doing this.
>
> Reviewed-by: Randy Dunlap
Thanks!
>
>> + * that was created by registering a software node, meaning
>> is_software_node()
>> + * must return true when passed that pointer.
>> + *
>> + * This function creates a new instance of &struct software_node, assigns
>> it a
>> + * copy of the given array of properties and registers it as a new
>> fwnode_handle.
>> + * Freeing of the allocated memory when the fwnode_handle is no longer
>> needed is
>> + * handled via software_node_release() and does not need to be done
>> separately.
>> + *
>> + * Returns:
>> + * * fwnode_handle *- On success
>> + * * -EINVAL- When @parent is not associated with a
>> software_node
>> + * * -ENOMEM- When memory allocation fails
>> + * * -Other - Propagated errors from sub-functions
>> + */
>> struct fwnode_handle *
>> fwnode_create_software_node(con
[PATCH v2] software_node: Add kernel-doc comments to exported symbols
A number of functions which are exported via EXPORT_SYMBOL_GPL() lack any
kernel-doc comments; add those in so all exported symbols are documented.
Reviewed-by: Andy Shevchenko
Reviewed-by: Heikki Krogerus
Signed-off-by: Daniel Scally
---
Changes in version 2:
- Replaced "fwnode_handle" with either @fwnode or natural language
reference to a firmware node handle as appropriate.
drivers/base/swnode.c | 53 +++
1 file changed, 53 insertions(+)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 4a4b2008fbc2..e98018aa8b2f 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -33,6 +33,13 @@ static struct kset *swnode_kset;
static const struct fwnode_operations software_node_ops;
+/**
+ * is_software_node() - check if given fwnode was created from a software_node
+ * @fwnode: The &struct fwnode_handle to check
+ *
+ * This function is used to check whether a given firmware node handle was
+ * created by registering a &struct software_node or not.
+ */
bool is_software_node(const struct fwnode_handle *fwnode)
{
return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &software_node_ops;
@@ -71,6 +78,14 @@ software_node_to_swnode(const struct software_node *node)
return swnode;
}
+/**
+ * to_software_node() - Fetch software node associated with a firmware node
handle
+ * @fwnode: The pointer to a &struct fwnode_handle to parse
+ *
+ * This function attempts to fetch a pointer to the &struct software_node which
+ * was used to create the given @fwnode. Note that this will only work if the
+ * software node has **not** been released.
+ */
const struct software_node *to_software_node(const struct fwnode_handle
*fwnode)
{
const struct swnode *swnode = to_swnode(fwnode);
@@ -79,6 +94,14 @@ const struct software_node *to_software_node(const struct
fwnode_handle *fwnode)
}
EXPORT_SYMBOL_GPL(to_software_node);
+/**
+ * software_node_fwnode() - Fetch firmware node associated with a given
software node
+ * @node: The pointer to a &struct software_node to parse
+ *
+ * This function attempts to fetch a pointer to the &struct fwnode_handle which
+ * was created from the given @node. Note that this will only work after the
+ * software node has been registered.
+ */
struct fwnode_handle *software_node_fwnode(const struct software_node *node)
{
struct swnode *swnode = software_node_to_swnode(node);
@@ -800,6 +823,27 @@ void software_node_unregister(const struct software_node
*node)
}
EXPORT_SYMBOL_GPL(software_node_unregister);
+/**
+ * fwnode_create_software_node() - Create and register a new software_node
+ * @properties: NULL terminated array of properties to assign to the new node
+ * @parent: Pointer to a &struct fwnode_handle to assign as parent to the new
+ * node
+ *
+ * NOTE: The pointer passed to @parent **must** be to a firmware node handle
+ * that was created by registering a software node, meaning is_software_node()
+ * must return true when passed that pointer.
+ *
+ * This function creates a new instance of &struct software_node, assigns it a
+ * copy of the given array of properties and registers it as a new
fwnode_handle.
+ * Freeing of the allocated memory when the fwnode_handle is no longer needed
is
+ * handled via software_node_release() and does not need to be done separately.
+ *
+ * Returns:
+ * * fwnode_handle * - On success
+ * * -EINVAL - When @parent is not associated with a software_node
+ * * -ENOMEM - When memory allocation fails
+ * * -Other- Propagated errors from sub-functions
+ */
struct fwnode_handle *
fwnode_create_software_node(const struct property_entry *properties,
const struct fwnode_handle *parent)
@@ -832,6 +876,15 @@ fwnode_create_software_node(const struct property_entry
*properties,
}
EXPORT_SYMBOL_GPL(fwnode_create_software_node);
+/**
+ * fwnode_remove_software_node() - Put a reference to a registered
software_node
+ * @fwnode: The pointer to the &struct fwnode_handle you want to release
+ *
+ * Release a reference to a registered &struct software_node. This function
+ * differs from software_node_put() in that it takes no action if the
+ * firmware node handle passed to @fwnode turns out not to have been created by
+ * registering a software_node.
+ */
void fwnode_remove_software_node(struct fwnode_handle *fwnode)
{
struct swnode *swnode = to_swnode(fwnode);
--
2.25.1
Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()
Hi Heikki
On 11/01/2021 14:10, Heikki Krogerus wrote:
> This helper will register a software node and then assign
> it to device at the same time. The function will also make
> sure that the device can't have more than one software node.
>
> Tested-by: Andy Shevchenko
> Signed-off-by: Heikki Krogerus
> ---
I like this change. One comment below, but for what it's worth:
Reviewed-by: Daniel Scally
> +/**
> + * device_remove_software_node - Remove device's software node
> + * @dev: The device with the software node.
> + *
> + * This function will unregister the software node of @dev.
> + */
> +void device_remove_software_node(struct device *dev)
> +{
> + struct swnode *swnode;
> +
> + swnode = dev_to_swnode(dev);
> + if (!swnode)
> + return;
> +
> + kobject_put(&swnode->kobj);
> +}
> +EXPORT_SYMBOL_GPL(device_remove_software_node);
I wonder if this also ought to set dev_fwnode(dev)->secondary back to
ERR_PTR(-ENODEV)?
> +
> int software_node_notify(struct device *dev, unsigned long action)
> {
> - struct fwnode_handle *fwnode = dev_fwnode(dev);
> struct swnode *swnode;
> int ret;
>
> - if (!fwnode)
> - return 0;
> -
> - if (!is_software_node(fwnode))
> - fwnode = fwnode->secondary;
> - if (!is_software_node(fwnode))
> + swnode = dev_to_swnode(dev);
> + if (!swnode)
> return 0;
>
> - swnode = to_swnode(fwnode);
> -
> switch (action) {
> case KOBJ_ADD:
> ret = sysfs_create_link(&dev->kobj, &swnode->kobj,
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0a9001fe7aeab..b0e413dc59271 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry
> *properties,
> const struct fwnode_handle *parent);
> void fwnode_remove_software_node(struct fwnode_handle *fwnode);
>
> +int device_add_software_node(struct device *dev, const struct software_node
> *swnode);
> +void device_remove_software_node(struct device *dev);
> +
> #endif /* _LINUX_PROPERTY_H_ */
>
Re: [PATCH v5 00/15] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows
Hi Rafael, Sakari On 12/01/2021 19:34, Rafael J. Wysocki wrote: > >> I'm hopeful that most or all of this series could get picked up for 5.12. >> We touch a few different areas (listed below), but I think the easiest >> approach would be to merge everything through media tree. Rafael, Greg, >> Mauro and Sergey; are you ok with that plan, or would you prefer a >> different approach? Mauro; if that plan is ok (and of course assuming that >> the rest of the patches are acked by their respective maintainers) could >> we get a dedicated feature branch just in case the following series ends >> up being ready in time too? >> >> > Please feel free to add > > Acked-by: Rafael J. Wysocki > > to all of the device properties patches in this series if that helps. > > Thanks! Thanks very much (and Greg too). Sakari; unless I'm misunderstanding something, I think this series could be picked up now, right? Would it be ok to do that through your tree? I think the idea of a dedicated feature branch can be dropped, I won't have the second series ready in time for this round anyway. First time doing this, so if I've missed something please let me know!
Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
On 09/01/2021 09:17, Andy Shevchenko wrote: > On Saturday, January 9, 2021, Daniel Scally wrote: > >> Hi Andy >> >> On 08/01/2021 12:17, Andy Shevchenko wrote: >>> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally >> wrote: >>>> On 30/11/2020 20:07, Andy Shevchenko wrote: >>>>> On Mon, Nov 30, 2020 at 01:31:29PM +, Daniel Scally wrote: >>> ... >>> >>>>> It's solely Windows driver design... >>>>> Luckily I found some information and can clarify above table: >>>>> >>>>> 0x00 Reset >>>>> 0x01 Power down >>>>> 0x0b Power enable >>>>> 0x0c Clock enable >>>>> 0x0d LED (active high) >>>>> >>>>> The above text perhaps should go somewhere under Documentation. >>>> Coming back to this; there's a bit of an anomaly with the 0x01 Power >>>> Down pin for at least one platform. As listed above, the OV2680 on one >>>> of my platforms has 3 GPIOs defined, and the table above gives them as >>>> type Reset, Power down and Clock enable. I'd assumed from this table >>>> that "power down" meant a powerdown GPIO (I.E. the one usually called >>>> PWDNB in Omnivision datasheets and "powerdown" in drivers), but the >>>> datasheet for the OV2680 doesn't list a separate reset and powerdown >>>> pin, but rather a single pin that performs both functions. >>> All of them are GPIOs, the question here is how they are actually >>> connected on PCB level and I have no answer to that. You have to find >>> schematics somewhere. >> Yeah; I've been trying to get those but so far, no dice. >> >> > Can you share the exact name / model of the hardware in question here? I > would try to search for the schematics. Lenovo Miix 510-12ISK 80U1 - I also tried asking Lenovo for them but that didn't really go anywhere; but of course I'm just contacting their usual support line and explaining what I'm after, so I didn't really expect it to. > > >>>> Am I wrong to treat that as something that ought to be mapped as a >>>> powerdown GPIO to the sensors? Or do you know of any other way to >>>> reconcile that discrepancy? >>> The GPIOs can go directly to the sensors or be a control pin for >>> separate discrete power gates. >>> So, we can do one of the following: >>> a) present PD GPIO as fixed regulator; >>> b) present PD & Reset GPIOs as regulator; >>> c) provide them as is to the sensor and sensor driver must do what it >>> considers right. >>> >>> Since we don't have schematics (yet?) and we have plenty of variations >>> of sensors, I would go to c) and update the driver of the affected >>> sensor as needed. Because even if you have separate discrete PD for >>> one sensor on one platform there is no guarantee that it will be the >>> same on another. Providing a "virtual" PD in a sensor that doesn't >>> support it is the best choice I think. Let's hear what Sakari and >>> other experienced camera sensor developers say. >>> >>> My vision is purely based on electrical engineering background, >>> experience with existing (not exactly camera) sensor drivers and >>> generic cases. >> Alright; thanks. I'm happy with C being the answer, so unless someone >> thinks differently I'll work on that basis. >> >> > Laurent answered that it is not the best choice from camera sensor driver > perspective. Yep, seen - no problem. I will look at doing it via the method he suggests.
Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
Hi Laurent On 09/01/2021 00:18, Laurent Pinchart wrote: > H Andy and Daniel, > > On Fri, Jan 08, 2021 at 02:17:49PM +0200, Andy Shevchenko wrote: >> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally wrote: >>> On 30/11/2020 20:07, Andy Shevchenko wrote: >>>> On Mon, Nov 30, 2020 at 01:31:29PM +, Daniel Scally wrote: >> ... >> >>>> It's solely Windows driver design... >>>> Luckily I found some information and can clarify above table: >>>> >>>> 0x00 Reset >>>> 0x01 Power down >>>> 0x0b Power enable >>>> 0x0c Clock enable >>>> 0x0d LED (active high) >>>> >>>> The above text perhaps should go somewhere under Documentation. >>> Coming back to this; there's a bit of an anomaly with the 0x01 Power >>> Down pin for at least one platform. As listed above, the OV2680 on one >>> of my platforms has 3 GPIOs defined, and the table above gives them as >>> type Reset, Power down and Clock enable. I'd assumed from this table >>> that "power down" meant a powerdown GPIO (I.E. the one usually called >>> PWDNB in Omnivision datasheets and "powerdown" in drivers), but the >>> datasheet for the OV2680 doesn't list a separate reset and powerdown >>> pin, but rather a single pin that performs both functions. > First question, do we have a confirmation that the OV2680 sensor on that > platform requires GPIO 0x01 to be toggled to work properly ? Yes; without that toggled not even the i2c interface is available. > I'd like to > rule out the option of the GPIO being simply not connected (that would > be best for us, although my experience so far with this terrible ACPI > design doesn't of course give me much hope). Sorry to dash what little hope was left. > Where did you find the OV2680 datasheet by the way, can you share a link > to a leaked version ? Sure. I left the PC already, but I'll do that tomorrow. >> All of them are GPIOs, the question here is how they are actually >> connected on PCB level and I have no answer to that. You have to find >> schematics somewhere. >> >>> Am I wrong to treat that as something that ought to be mapped as a >>> powerdown GPIO to the sensors? Or do you know of any other way to >>> reconcile that discrepancy? >> The GPIOs can go directly to the sensors or be a control pin for >> separate discrete power gates. > GPIO functions 0x00 and 0x01 are meant to control sensor signals, while > GPIO function 0x0b is meant to control a power gate. Of course board > designers may have thought clever to use function 0x01 to control a > second power gate, this can't be ruled out without the schematics (or > reverse engineering of the hardware). > >> So, we can do one of the following: >> a) present PD GPIO as fixed regulator; >> b) present PD & Reset GPIOs as regulator; >> c) provide them as is to the sensor and sensor driver must do what it >> considers right. >> >> Since we don't have schematics (yet?) and we have plenty of variations >> of sensors, I would go to c) and update the driver of the affected >> sensor as needed. Because even if you have separate discrete PD for >> one sensor on one platform there is no guarantee that it will be the >> same on another. Providing a "virtual" PD in a sensor that doesn't >> support it is the best choice I think. Let's hear what Sakari and >> other experienced camera sensor developers say. >> >> My vision is purely based on electrical engineering background, >> experience with existing (not exactly camera) sensor drivers and >> generic cases. > If the OV2680 has indeed no power down pin, that won't work. Adding > support for a non-existent powerdown pin to the corresponding driver > won't be accepted. Workarounds and hacks to support IPU3-based devices > need to be kept out of camera sensor drivers. > > If we need to map GPIO function 0x01 to a sensor GPIO on some platform, > and to a regulator on other platforms, then we will need per-platform > data in the INT3472 driver. For this particular platform, the reset > (0x00) GPIO should be passed to the sensor, and the powerdown (0x01) > GPIO should control a regulator (again assuming that our assumption that > the GPIO is wired to a power gate is correct). Let me think of a neat way to do this then. > >>> Failing that; the only way I can think to handle this is to register >>> proxy GPIO pins assigned to the sensors as you suggested previously, and >>> have them toggle the GPIO's assigned to the INT3472 based on platform >>> specific mapping data (I.E. we register a pin called "reset", which on >>> most platforms just toggles the 0x00 pin, but on this specific platform >>> would drive both 0x00 and 0x01 together. We're already heading that way >>> for the regulator consumer supplies so it's sort of nothing new, but I'd >>> still rather not if it can be avoided.
Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
Hi Andy On 08/01/2021 12:17, Andy Shevchenko wrote: > On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally wrote: >> On 30/11/2020 20:07, Andy Shevchenko wrote: >>> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > ... > >>> It's solely Windows driver design... >>> Luckily I found some information and can clarify above table: >>> >>> 0x00 Reset >>> 0x01 Power down >>> 0x0b Power enable >>> 0x0c Clock enable >>> 0x0d LED (active high) >>> >>> The above text perhaps should go somewhere under Documentation. >> Coming back to this; there's a bit of an anomaly with the 0x01 Power >> Down pin for at least one platform. As listed above, the OV2680 on one >> of my platforms has 3 GPIOs defined, and the table above gives them as >> type Reset, Power down and Clock enable. I'd assumed from this table >> that "power down" meant a powerdown GPIO (I.E. the one usually called >> PWDNB in Omnivision datasheets and "powerdown" in drivers), but the >> datasheet for the OV2680 doesn't list a separate reset and powerdown >> pin, but rather a single pin that performs both functions. > All of them are GPIOs, the question here is how they are actually > connected on PCB level and I have no answer to that. You have to find > schematics somewhere. Yeah; I've been trying to get those but so far, no dice. > >> Am I wrong to treat that as something that ought to be mapped as a >> powerdown GPIO to the sensors? Or do you know of any other way to >> reconcile that discrepancy? > The GPIOs can go directly to the sensors or be a control pin for > separate discrete power gates. > So, we can do one of the following: > a) present PD GPIO as fixed regulator; > b) present PD & Reset GPIOs as regulator; > c) provide them as is to the sensor and sensor driver must do what it > considers right. > > Since we don't have schematics (yet?) and we have plenty of variations > of sensors, I would go to c) and update the driver of the affected > sensor as needed. Because even if you have separate discrete PD for > one sensor on one platform there is no guarantee that it will be the > same on another. Providing a "virtual" PD in a sensor that doesn't > support it is the best choice I think. Let's hear what Sakari and > other experienced camera sensor developers say. > > My vision is purely based on electrical engineering background, > experience with existing (not exactly camera) sensor drivers and > generic cases. Alright; thanks. I'm happy with C being the answer, so unless someone thinks differently I'll work on that basis. >> Failing that; the only way I can think to handle this is to register >> proxy GPIO pins assigned to the sensors as you suggested previously, and >> have them toggle the GPIO's assigned to the INT3472 based on platform >> specific mapping data (I.E. we register a pin called "reset", which on >> most platforms just toggles the 0x00 pin, but on this specific platform >> would drive both 0x00 and 0x01 together. We're already heading that way >> for the regulator consumer supplies so it's sort of nothing new, but I'd >> still rather not if it can be avoided. >
Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
Hi Andy, all
On 30/11/2020 20:07, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
>> On platforms where ACPI is designed for use with Windows, resources
>> that are intended to be consumed by sensor devices are sometimes in
>> the _CRS of a dummy INT3472 device upon which the sensor depends. This
>> driver binds to the dummy acpi device (which does not represent a
> acpi device -> acpi_device
>
>> physical PMIC) and maps them into GPIO lines and regulators for use by
>> the sensor device instead.
> ...
>
>> This patch contains the bits of this process that we're least sure about.
>> The sensors in scope for this work are called out as dependent (in their
>> DSDT entry's _DEP) on a device with _HID INT3472. These come in at least
>> 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore
>> are legitimate tps68470 PMICs that need handling by those drivers - work
>> on that in the future). And those without an I2C device. For those without
>> an I2C device they instead have an array of GPIO pins defined in _CRS. So
>> for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of
>> the _latter_ kind of INT3472 devices, with this _CRS:
>>
>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
>> {
>> Name (SBUF, ResourceTemplate ()
>> {
>> GpioIo (Exclusive, PullDefault, 0x, 0x,
>> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
>> 0x00, ResourceConsumer, ,
>> )
>> { // Pin list
>> 0x0079
>> }
>> GpioIo (Exclusive, PullDefault, 0x, 0x,
>> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
>> 0x00, ResourceConsumer, ,
>> )
>> { // Pin list
>> 0x007A
>> }
>> GpioIo (Exclusive, PullDefault, 0x, 0x,
>> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
>> 0x00, ResourceConsumer, ,
>> )
>> { // Pin list
>> 0x008F
>> }
>> })
>> Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */
>> }
>>
>> and the same device has a _DSM Method, which returns 32-bit ints where
>> the second lowest byte we noticed to match the pin numbers of the GPIO
>> lines:
>>
>> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
>> {
>> If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f")))
>> {
>> If ((Arg2 == One))
>> {
>> Return (0x03)
>> }
>>
>> If ((Arg2 == 0x02))
>> {
>> Return (0x01007900)
>> }
>>
>> If ((Arg2 == 0x03))
>> {
>> Return (0x01007A0C)
>> }
>>
>> If ((Arg2 == 0x04))
>> {
>> Return (0x01008F01)
>> }
>> }
>>
>> Return (Zero)
>> }
>>
>> We know that at least some of those pins have to be toggled active for the
>> sensor devices to be available in i2c, so the conclusion we came to was
>> that those GPIO entries assigned to the INT3472 device actually represent
>> GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya
>> noticed that the lowest byte in the return values of the _DSM method
>> seemed to represent the type or function of the GPIO line, and we
>> confirmed that by testing on each surface device that GPIO lines where the
>> low byte in the _DSM entry for that pin was 0x0d controlled the privacy
>> LED of the cameras.
>>
>> We're guessing as to the exact meaning of the function byte, but I
>> conclude they're something like this:
>>
>> 0x00 - probably a reset GPIO
>> 0x01 - regulator for the sensor
>> 0x0c - regulator for the sensor
>> 0x0b - regulator again, but for a VCM or EEPROM
>> 0x0d - privacy led (only one we're totally confident of since we can see
>>it happen!)
> It's solely Windows driver design...
> Luckily I found some information and can clarify above table:
>
> 0x00 Reset
> 0x01 Power down
> 0x0b Power enable
> 0x0c Clock enable
> 0x0d LED (active high)
>
> The above text perhaps should go somewhere under Documentation.
Coming back to this; there's a bit of an anomaly with the 0x01 Power
Down pin for at least one platform. As listed above, the OV2680 on one
of my platforms ha
Re: [PATCH] software_node: Add kernel-doc comments to exported symbols
Hi Heikki On 07/01/2021 14:19, Heikki Krogerus wrote: > On Tue, Jan 05, 2021 at 03:39:42PM +0000, Daniel Scally wrote: >> Hi Andy >> >> On 05/01/2021 14:53, Andy Shevchenko wrote: >>> On Mon, Jan 04, 2021 at 11:47:36PM +, Daniel Scally wrote: >>>> A number of functions which are exported via EXPORT_SYMBOL_GPL() lack any >>>> kernel-doc comments; add those in so all exported symbols are documented. >>> Thanks, it's helpful! >>> Reviewed-by: Andy Shevchenko >>> after addressing few nitpicks >> Thanks for reviewing >>>> Signed-off-by: Daniel Scally >>>> --- >>>> With a view to maybe writing some documentation once the fwnode_graph_*() >>>> functions are also added. >>> FWIW, Heikki used to have a draft patch of swnode documentation, not sure >>> what's the current status of it. >> Oh cool ok; I'll defer to him then. > I actually had a similar patch prepared as part of the series adding > the documentation for software nodes, but your comments are better > than mine. So, after you have addressed Andy's comments: > > Reviewed-by: Heikki Krogerus Great, thanks - and for the other R-bs just now also. I'll send a v2 of this one tonight/tomorrow (depends how much else I get through)
Re: [PATCH v5 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
On 07/01/2021 13:28, Daniel Scally wrote:
> Currently on platforms designed for Windows, connections between CIO2 and
> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> driver to compensate by building software_node connections, parsing the
> connection properties from the sensor's SSDB buffer.
>
> Suggested-by: Jordan Hand
> Reviewed-by: Laurent Pinchart
> Reviewed-by: Andy Shevchenko
> Reviewed-by: Kieran Bingham
> Signed-off-by: Daniel Scally
> ---
Sorry all, missed the changelog on this one:
---
Changes in v5:
- Changed some macros to declare compound literals
- Switched cio2_check_fwnode_graph() to return int instead of bool
- Added some comments to clarify parts of the code that might look
weird initially
- Some cosmetic changes
> MAINTAINERS | 1 +
> drivers/media/pci/intel/ipu3/Kconfig | 18 +
> drivers/media/pci/intel/ipu3/Makefile | 1 +
> drivers/media/pci/intel/ipu3/cio2-bridge.c| 311 ++
> drivers/media/pci/intel/ipu3/cio2-bridge.h| 125 +++
> drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 34 ++
> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 +
> 7 files changed, 496 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
> create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 92228e8dd868..a091b496fdd8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9014,6 +9014,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
> M: Yong Zhi
> M: Sakari Ailus
> M: Bingbu Cao
> +M: Dan Scally
> R: Tianshu Qiu
> L: linux-me...@vger.kernel.org
> S: Maintained
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig
> b/drivers/media/pci/intel/ipu3/Kconfig
> index 82d7f17e6a02..96a2231b16ad 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
> Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> connected camera.
> The module will be called ipu3-cio2.
> +
> +config CIO2_BRIDGE
> + bool "IPU3 CIO2 Sensors Bridge"
> + depends on VIDEO_IPU3_CIO2
> + help
> + This extension provides an API for the ipu3-cio2 driver to create
> + connections to cameras that are hidden in the SSDB buffer in ACPI.
> + It can be used to enable support for cameras in detachable / hybrid
> + devices that ship with Windows.
> +
> + Say Y here if your device is a detachable / hybrid laptop that comes
> + with Windows installed by the OEM, for example:
> +
> + - Microsoft Surface models (except Surface Pro 3)
> + - The Lenovo Miix line (for example the 510, 520, 710 and 720)
> + - Dell 7285
> +
> + If in doubt, say N here.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile
> b/drivers/media/pci/intel/ipu3/Makefile
> index 429d516452e4..933777e6ea8a 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -2,3 +2,4 @@
> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>
> ipu3-cio2-y += ipu3-cio2-main.o
> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c
> b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> new file mode 100644
> index ..143f3c0f445e
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "cio2-bridge.h"
> +
> +/*
> + * Extend this array with ACPI Hardware IDs of devices known to be working
> + * plus the number of link-frequencies expected by their drivers, along with
> + * the frequency values in hertz. This is somewhat opportunistic way of
> adding
> + * support for this for now in the hopes of a better source for the
> information
> + * (possibly some encoded value in the SSDB buffer that we're unaware of)
> + * becoming apparent in the future.
> + *
> + * Do not add an entry for a sensor that is not actually supported.
> + */
> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> + /* Omnivision OV5693 */
> + CIO2_SENSOR_CONFIG("INT33BE", 0),
> + /* Omnivision OV2680 */
> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
> +};
> +
> +static const struct cio2_property_names prop_names = {
> + .clock_frequency = "clock-frequency&
[PATCH v5 07/15] device property: Define format macros for ports and endpoints
OF, ACPI and software_nodes all implement graphs including nodes for ports
and endpoints. These are all intended to be named with a common schema,
as "port@n" and "endpoint@n" where n is an unsigned int representing the
index of the node. To ensure commonality across the subsystems, provide a
set of macros to define the format.
Suggested-by: Andy Shevchenko
Reviewed-by: Andy Shevchenko
Reviewed-by: Laurent Pinchart
Signed-off-by: Daniel Scally
---
Changes in v5:
- Changed commit subject
include/linux/fwnode.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index fde4ad97564c..77414e431e89 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -50,6 +50,13 @@ struct fwnode_endpoint {
const struct fwnode_handle *local_fwnode;
};
+/*
+ * ports and endpoints defined as software_nodes should all follow a common
+ * naming scheme; use these macros to ensure commonality.
+ */
+#define SWNODE_GRAPH_PORT_NAME_FMT "port@%u"
+#define SWNODE_GRAPH_ENDPOINT_NAME_FMT "endpoint@%u"
+
#define NR_FWNODE_REFERENCE_ARGS 8
/**
--
2.25.1
[PATCH v5 06/15] software_node: unregister software_nodes in reverse order
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
Reported-by: Dan Carpenter
Reviewed-by: Laurent Pinchart
Reviewed-by: Sakari Ailus
Suggested-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
Changes in v5:
- None
drivers/base/swnode.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 166c5cc73f39..6f7443c6d3b5 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -779,16 +779,23 @@ 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 members of the array
are
+ * children of another member then the children must appear later in the list
such
+ * that they are unregistered first.
*/
-void software_node_unregister_node_group(const struct software_node
**node_group)
+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])
+ i++;
+
+ while (i--)
software_node_unregister(node_group[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
2.25.1
[PATCH v5 09/15] lib/test_printf.c: Use helper function to unwind array of software_nodes
Use the software_node_unregister_nodes() helper function to unwind this array in a cleaner way. Acked-by: Petr Mladek Reviewed-by: Andy Shevchenko Reviewed-by: Laurent Pinchart Reviewed-by: Sergey Senozhatsky Suggested-by: Andy Shevchenko Signed-off-by: Daniel Scally --- Changes in v5: - None lib/test_printf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 7ac87f18a10f..7d60f24240a4 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -644,9 +644,7 @@ static void __init fwnode_pointer(void) test(second_name, "%pfwP", software_node_fwnode(&softnodes[1])); test(third_name, "%pfwP", software_node_fwnode(&softnodes[2])); - software_node_unregister(&softnodes[2]); - software_node_unregister(&softnodes[1]); - software_node_unregister(&softnodes[0]); + software_node_unregister_nodes(softnodes); } static void __init -- 2.25.1
[PATCH v5 10/15] ipu3-cio2: Add T: entry to MAINTAINERS
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 Reviewed-by: Andy Shevchenko Signed-off-by: Daniel Scally --- Changes in v5: - None MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 471561d9d55f..92228e8dd868 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9017,6 +9017,7 @@ M:Bingbu Cao R: Tianshu Qiu L: linux-me...@vger.kernel.org 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
[PATCH v5 14/15] media: v4l2-fwnode: Include v4l2_fwnode_bus_type
V4L2 fwnode bus types are enumerated in v4l2-fwnode.c, meaning they aren't
available to the rest of the kernel. Move the enum to the corresponding
header so that I can use the label to refer to those values.
Suggested-by: Andy Shevchenko
Reviewed-by: Laurent Pinchart
Reviewed-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
Changes in v5:
- Changed commit subject
drivers/media/v4l2-core/v4l2-fwnode.c | 11 ---
include/media/v4l2-fwnode.h | 22 ++
2 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
b/drivers/media/v4l2-core/v4l2-fwnode.c
index 5353e37eb950..c1c2b3060532 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -28,17 +28,6 @@
#include
#include
-enum v4l2_fwnode_bus_type {
- V4L2_FWNODE_BUS_TYPE_GUESS = 0,
- V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
- V4L2_FWNODE_BUS_TYPE_CSI1,
- V4L2_FWNODE_BUS_TYPE_CCP2,
- V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
- V4L2_FWNODE_BUS_TYPE_PARALLEL,
- V4L2_FWNODE_BUS_TYPE_BT656,
- NR_OF_V4L2_FWNODE_BUS_TYPE,
-};
-
static const struct v4l2_fwnode_bus_conv {
enum v4l2_fwnode_bus_type fwnode_bus_type;
enum v4l2_mbus_type mbus_type;
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 6d026dadbf98..e7ad95365a40 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -213,6 +213,28 @@ struct v4l2_fwnode_connector {
} connector;
};
+/**
+ * enum v4l2_fwnode_bus_type - Video bus types defined by firmware properties
+ * @V4L2_FWNODE_BUS_TYPE_GUESS: Default value if no bus-type fwnode property
+ * @V4L2_FWNODE_BUS_TYPE_CSI2_CPHY: MIPI CSI-2 bus, C-PHY physical layer
+ * @V4L2_FWNODE_BUS_TYPE_CSI1: MIPI CSI-1 bus
+ * @V4L2_FWNODE_BUS_TYPE_CCP2: SMIA Compact Camera Port 2 bus
+ * @V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: MIPI CSI-2 bus, D-PHY physical layer
+ * @V4L2_FWNODE_BUS_TYPE_PARALLEL: Camera Parallel Interface bus
+ * @V4L2_FWNODE_BUS_TYPE_BT656: BT.656 video format bus-type
+ * @NR_OF_V4L2_FWNODE_BUS_TYPE: Number of bus-types
+ */
+enum v4l2_fwnode_bus_type {
+ V4L2_FWNODE_BUS_TYPE_GUESS = 0,
+ V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
+ V4L2_FWNODE_BUS_TYPE_CSI1,
+ V4L2_FWNODE_BUS_TYPE_CCP2,
+ V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
+ V4L2_FWNODE_BUS_TYPE_PARALLEL,
+ V4L2_FWNODE_BUS_TYPE_BT656,
+ NR_OF_V4L2_FWNODE_BUS_TYPE
+};
+
/**
* v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
* @fwnode: pointer to the endpoint's fwnode handle
--
2.25.1
[PATCH v5 05/15] software_node: Enforce parent before child ordering of nodes arrays
Registering software_nodes with the .parent member set to point to a
currently unregistered software_node has the potential for problems,
so enforce parent -> child ordering in arrays passed in to
software_node_register_nodes().
Software nodes that are children of another software node should be
unregistered before their parent. To allow easy unregistering of an array
of software_nodes ordered parent to child, reverse the order in which
software_node_unregister_nodes() unregisters software_nodes.
Suggested-by: Andy Shevchenko
Reviewed-by: Laurent Pinchart
Signed-off-by: Daniel Scally
---
Changes in v5:
- None
drivers/base/swnode.c | 42 ++
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 4fcc1a6fb724..166c5cc73f39 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -692,7 +692,11 @@ swnode_register(const struct software_node *node, struct
swnode *parent,
* software_node_register_nodes - Register an array of software nodes
* @nodes: Zero terminated array of software nodes to be registered
*
- * Register multiple software nodes at once.
+ * Register multiple software nodes at once. If any node in the array
+ * has its .parent pointer set (which can only be to another software_node),
+ * then its parent **must** have been registered before it is; either outside
+ * of this function or by ordering the array such that parent comes before
+ * child.
*/
int software_node_register_nodes(const struct software_node *nodes)
{
@@ -700,14 +704,23 @@ int software_node_register_nodes(const struct
software_node *nodes)
int i;
for (i = 0; nodes[i].name; i++) {
- ret = software_node_register(&nodes[i]);
- if (ret) {
- software_node_unregister_nodes(nodes);
- return ret;
+ const struct software_node *parent = nodes[i].parent;
+
+ if (parent && !software_node_to_swnode(parent)) {
+ ret = -EINVAL;
+ goto err_unregister_nodes;
}
+
+ ret = software_node_register(&nodes[i]);
+ if (ret)
+ goto err_unregister_nodes;
}
return 0;
+
+err_unregister_nodes:
+ software_node_unregister_nodes(nodes);
+ return ret;
}
EXPORT_SYMBOL_GPL(software_node_register_nodes);
@@ -715,18 +728,23 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
* software_node_unregister_nodes - Unregister an array of software nodes
* @nodes: Zero terminated array of software nodes to be unregistered
*
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. If parent pointers are set up
+ * in any of the software nodes then the array **must** be ordered such that
+ * parents come before their children.
*
- * NOTE: Be careful using this call if the nodes had parent pointers set up in
- * them before registering. If so, it is wiser to remove the nodes
- * individually, in the correct order (child before parent) instead of relying
- * on the sequential order of the list of nodes in the array.
+ * NOTE: If you are uncertain whether the array is ordered such that
+ * parents will be unregistered before their children, it is wiser to
+ * remove the nodes individually, in the correct order (child before
+ * parent).
*/
void software_node_unregister_nodes(const struct software_node *nodes)
{
- int i;
+ unsigned int i = 0;
+
+ while (nodes[i].name)
+ i++;
- for (i = 0; nodes[i].name; i++)
+ while (i--)
software_node_unregister(&nodes[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
--
2.25.1
[PATCH v5 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Currently on platforms designed for Windows, connections between CIO2 and
sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
driver to compensate by building software_node connections, parsing the
connection properties from the sensor's SSDB buffer.
Suggested-by: Jordan Hand
Reviewed-by: Laurent Pinchart
Reviewed-by: Andy Shevchenko
Reviewed-by: Kieran Bingham
Signed-off-by: Daniel Scally
---
MAINTAINERS | 1 +
drivers/media/pci/intel/ipu3/Kconfig | 18 +
drivers/media/pci/intel/ipu3/Makefile | 1 +
drivers/media/pci/intel/ipu3/cio2-bridge.c| 311 ++
drivers/media/pci/intel/ipu3/cio2-bridge.h| 125 +++
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 34 ++
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 +
7 files changed, 496 insertions(+)
create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 92228e8dd868..a091b496fdd8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9014,6 +9014,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
M: Yong Zhi
M: Sakari Ailus
M: Bingbu Cao
+M: Dan Scally
R: Tianshu Qiu
L: linux-me...@vger.kernel.org
S: Maintained
diff --git a/drivers/media/pci/intel/ipu3/Kconfig
b/drivers/media/pci/intel/ipu3/Kconfig
index 82d7f17e6a02..96a2231b16ad 100644
--- a/drivers/media/pci/intel/ipu3/Kconfig
+++ b/drivers/media/pci/intel/ipu3/Kconfig
@@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
connected camera.
The module will be called ipu3-cio2.
+
+config CIO2_BRIDGE
+ bool "IPU3 CIO2 Sensors Bridge"
+ depends on VIDEO_IPU3_CIO2
+ help
+ This extension provides an API for the ipu3-cio2 driver to create
+ connections to cameras that are hidden in the SSDB buffer in ACPI.
+ It can be used to enable support for cameras in detachable / hybrid
+ devices that ship with Windows.
+
+ Say Y here if your device is a detachable / hybrid laptop that comes
+ with Windows installed by the OEM, for example:
+
+ - Microsoft Surface models (except Surface Pro 3)
+ - The Lenovo Miix line (for example the 510, 520, 710 and 720)
+ - Dell 7285
+
+ If in doubt, say N here.
diff --git a/drivers/media/pci/intel/ipu3/Makefile
b/drivers/media/pci/intel/ipu3/Makefile
index 429d516452e4..933777e6ea8a 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
ipu3-cio2-y += ipu3-cio2-main.o
+ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c
b/drivers/media/pci/intel/ipu3/cio2-bridge.c
new file mode 100644
index ..143f3c0f445e
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "cio2-bridge.h"
+
+/*
+ * Extend this array with ACPI Hardware IDs of devices known to be working
+ * plus the number of link-frequencies expected by their drivers, along with
+ * the frequency values in hertz. This is somewhat opportunistic way of adding
+ * support for this for now in the hopes of a better source for the information
+ * (possibly some encoded value in the SSDB buffer that we're unaware of)
+ * becoming apparent in the future.
+ *
+ * Do not add an entry for a sensor that is not actually supported.
+ */
+static const struct cio2_sensor_config cio2_supported_sensors[] = {
+ /* Omnivision OV5693 */
+ CIO2_SENSOR_CONFIG("INT33BE", 0),
+ /* Omnivision OV2680 */
+ CIO2_SENSOR_CONFIG("OVTI2680", 0),
+};
+
+static const struct cio2_property_names prop_names = {
+ .clock_frequency = "clock-frequency",
+ .rotation = "rotation",
+ .bus_type = "bus-type",
+ .data_lanes = "data-lanes",
+ .remote_endpoint = "remote-endpoint",
+ .link_frequencies = "link-frequencies",
+};
+
+static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
+ void *data, u32 size)
+{
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj;
+ acpi_status status;
+ int ret = 0;
+
+ status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ obj = buffer.pointer;
+ if (!obj) {
+ dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
+ return -ENODEV;
+
[PATCH v5 01/15] software_node: Fix refcounts in software_node_get_next_child()
The software_node_get_next_child() function currently does not hold
references to the child software_node that it finds or put the ref that
is held against the old child - fix that.
Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware
node framework")
Reviewed-by: Andy Shevchenko
Reviewed-by: Laurent Pinchart
Reviewed-by: Sakari Ailus
Signed-off-by: Daniel Scally
---
Changes in v5:
- None
drivers/base/swnode.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 4a4b2008fbc2..4fcc1a6fb724 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -443,14 +443,18 @@ software_node_get_next_child(const struct fwnode_handle
*fwnode,
struct swnode *c = to_swnode(child);
if (!p || list_empty(&p->children) ||
- (c && list_is_last(&c->entry, &p->children)))
+ (c && list_is_last(&c->entry, &p->children))) {
+ fwnode_handle_put(child);
return NULL;
+ }
if (c)
c = list_next_entry(c, entry);
else
c = list_first_entry(&p->children, struct swnode, entry);
- return &c->fwnode;
+
+ fwnode_handle_put(child);
+ return fwnode_handle_get(&c->fwnode);
}
static struct fwnode_handle *
--
2.25.1
[PATCH v5 08/15] software_node: Add support for fwnode_graph*() family of functions
From: Heikki Krogerus
This implements the remaining .graph_*() callbacks in the fwnode
operations structure for the software nodes. That makes the
fwnode_graph_*() functions available in the drivers also when software
nodes are used.
The implementation tries to mimic the "OF graph" as much as possible, but
there is no support for the "reg" device property. The ports will need to
have the index in their name which starts with "port@" (for example
"port@0", "port@1", ...) and endpoints will use the index of the software
node that is given to them during creation. The port nodes can also be
grouped under a specially named "ports" subnode, just like in DT, if
necessary.
The remote-endpoints are reference properties under the endpoint nodes
that are named "remote-endpoint".
Reviewed-by: Laurent Pinchart
Reviewed-by: Andy Shevchenko
Signed-off-by: Heikki Krogerus
Co-developed-by: Daniel Scally
Signed-off-by: Daniel Scally
---
Changes in v5:
- Cosmetic changes only
drivers/base/swnode.c | 115 +-
1 file changed, 114 insertions(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 6f7443c6d3b5..9104a0abd531 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,115 @@ software_node_get_reference_args(const struct
fwnode_handle *fwnode,
return 0;
}
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+ struct fwnode_handle *port)
+{
+ struct fwnode_handle *old = port;
+
+ while ((port = software_node_get_next_child(parent, old))) {
+ /*
+* fwnode ports have naming style "port@", so we search for any
+* children that follow that convention.
+*/
+ if (!strncmp(to_swnode(port)->node->name, "port@",
+strlen("port@")))
+ return port;
+ old = port;
+ }
+
+ return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_handle *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ struct fwnode_handle *parent;
+ struct fwnode_handle *port;
+
+ if (!swnode)
+ return NULL;
+
+ if (endpoint) {
+ port = software_node_get_parent(endpoint);
+ parent = software_node_get_parent(port);
+ } else {
+ parent = software_node_get_named_child_node(fwnode, "ports");
+ if (!parent)
+ parent = software_node_get(&swnode->fwnode);
+
+ port = swnode_graph_find_next_port(parent, NULL);
+ }
+
+ for (; port; port = swnode_graph_find_next_port(parent, port)) {
+ endpoint = software_node_get_next_child(port, endpoint);
+ if (endpoint) {
+ fwnode_handle_put(port);
+ break;
+ }
+ }
+
+ fwnode_handle_put(parent);
+
+ return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ const struct software_node_ref_args *ref;
+ const struct property_entry *prop;
+
+ if (!swnode)
+ return NULL;
+
+ prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+ if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+ return NULL;
+
+ ref = prop->pointer;
+
+ return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+
+ swnode = swnode->parent;
+ if (swnode && !strcmp(swnode->node->name, "ports"))
+ swnode = swnode->parent;
+
+ return swnode ? software_node_get(&swnode->fwnode) : NULL;
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_endpoint *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ const char *parent_name = swnode->parent->node->name;
+ int ret;
+
+ if (strlen("port@") >= strlen(parent_name) ||
+ strncmp(parent_name, "port@", strlen("port@")))
+ return -EINVAL;
+
+ /* Ports have naming style "port@n", we need to select the n */
+ ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);
+ if (ret)
+
[PATCH v5 00/15] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows
Hello all
v4:
https://lore.kernel.org/linux-media/20210103231235.792999-1-djrsca...@gmail.com/T/#m11b7cb977e1b73fba1e625c3d6a189e2943a7783
v3:
https://lore.kernel.org/linux-media/20201224010907.263125-1-djrsca...@gmail.com/T/#m37b831bb2b406917d6db5da9acf9ed35df65d72d
v2:
https://lore.kernel.org/linux-media/20201217234337.1983732-1-djrsca...@gmail.com/T/#md93fd090009b42a6a98aed892aff0d38cf07e0cd
v1:
https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrsca...@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67
This series is to start adding support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. This series extends the ipu3-cio2
driver to allow for patching the firmware via software_nodes if endpoints
aren't defined by ACPI.
I'm hopeful that most or all of this series could get picked up for 5.12.
We touch a few different areas (listed below), but I think the easiest
approach would be to merge everything through media tree. Rafael, Greg,
Mauro and Sergey; are you ok with that plan, or would you prefer a
different approach? Mauro; if that plan is ok (and of course assuming that
the rest of the patches are acked by their respective maintainers) could
we get a dedicated feature branch just in case the following series ends
up being ready in time too?
lib
lib/test_printf.c: Use helper function to unwind array of
software_nodes
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
include: fwnode.h: Define format macros for ports and endpoints
acpi
acpi: Add acpi_dev_get_next_match_dev() and helper macro
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
include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
Series-level changelog:
- Rebased onto 5.11-rc1
Thanks
Dan
Andy Shevchenko (1):
media: ipu3-cio2: Add headers that ipu3-cio2.h is direct user of
Daniel Scally (13):
software_node: Fix refcounts in software_node_get_next_child()
device property: Return true in fwnode_device_is_available for NULL
ops
device 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
device property: Define format macros for ports and endpoints
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 / bus: Add acpi_dev_get_next_match_dev() and helper macro
media: v4l2-fwnode: Include v4l2_fwnode_bus_type
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 | 180 --
drivers/media/pci/intel/ipu3/Kconfig | 18 +
drivers/media/pci/intel/ipu3/Makefile | 3 +
drivers/media/pci/intel/ipu3/cio2-bridge.c| 311 ++
drivers/media/pci/intel/ipu3/cio2-bridge.h| 125 +++
.../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c}| 34 ++
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 24 ++
drivers/media/v4l2-core/v4l2-async.c | 8 +
drivers/media/v4l2-core/v4l2-fwnode.c | 11 -
include/acpi/acpi_bus.h | 7 +
include/linux/fwnode.h| 7 +
include/media/v4l2-fwnode.h | 22 ++
lib/test_printf.c | 4 +-
16 files changed, 763 insertions(+), 38 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
[PATCH v5 13/15] ACPI / bus: Add acpi_dev_get_next_match_dev() and helper macro
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
Reviewed-by: Sakari Ailus
Suggested-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
Changes in v5:
- Changed commit subject
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..ddca1550cce6 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 6d1879bf9440..02a716a0af5d 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -683,9 +683,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
[PATCH v5 12/15] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode()
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 Reviewed-by: Laurent Pinchart Signed-off-by: Daniel Scally --- Changes in v5: - 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
[PATCH v5 11/15] ipu3-cio2: Rename ipu3-cio2.c
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
Reviewed-by: Laurent Pinchart
Reviewed-by: Andy Shevchenko
Signed-off-by: Daniel Scally
---
Changes in v5:
- 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
[PATCH v5 03/15] device property: Return true in fwnode_device_is_available for NULL ops
Some types of fwnode_handle do not implement the device_is_available()
check, such as those created by software_nodes. There isn't really a
meaningful way to check for the availability of a device that doesn't
actually exist, so if the check isn't implemented just assume that the
"device" is present.
Suggested-by: Laurent Pinchart
Reviewed-by: Laurent Pinchart
Reviewed-by: Andy Shevchenko
Acked-by: Sakari Ailus
Signed-off-by: Daniel Scally
---
Changes in v5:
- Changed the commit subject
drivers/base/property.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 35b95c6ac0c6..0bf5260f14c6 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -837,9 +837,15 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
/**
* fwnode_device_is_available - check if a device is available for use
* @fwnode: Pointer to the fwnode of the device.
+ *
+ * For fwnode node types that don't implement the .device_is_available()
+ * operation, this function returns true.
*/
bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
{
+ if (!fwnode_has_op(fwnode, device_is_available))
+ return true;
+
return fwnode_call_bool_op(fwnode, device_is_available);
}
EXPORT_SYMBOL_GPL(fwnode_device_is_available);
--
2.25.1
[PATCH v5 04/15] device property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
This function is used to find fwnode endpoints against a device. In some instances those endpoints are software nodes which are children of fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to find those endpoints by recursively calling itself passing the ptr to fwnode->secondary in the event no endpoint is found for the primary. Reviewed-by: Andy Shevchenko Reviewed-by: Laurent Pinchart Acked-by: Sakari Ailus Signed-off-by: Daniel Scally --- Changes in v5: - Changed the commit subject drivers/base/property.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/base/property.c b/drivers/base/property.c index 0bf5260f14c6..1421e9548857 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -1215,7 +1215,14 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode, best_ep_id = fwnode_ep.id; } - return best_ep; + if (best_ep) + return best_ep; + + if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, + endpoint, flags); + + return NULL; } EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id); -- 2.25.1
[PATCH v5 02/15] media: ipu3-cio2: Add headers that ipu3-cio2.h is direct user of
From: Andy Shevchenko Add headers that ipu3-cio2.h is direct user of. Signed-off-by: Andy Shevchenko Reviewed-by: Laurent Pinchart Reviewed-by: Daniel Scally Tested-by: Daniel Scally Signed-off-by: Daniel Scally --- Changes in v5: - Added my Signed-off-by this time, apparently I need to do that. drivers/media/pci/intel/ipu3/ipu3-cio2.h | 18 ++ 1 file changed, 18 insertions(+) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h index ccf0b85ae36f..62187ab5ae43 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h @@ -4,8 +4,26 @@ #ifndef __IPU3_CIO2_H #define __IPU3_CIO2_H +#include +#include +#include +#include #include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +struct cio2_fbpt_entry;/* defined here, after the first usage */ +struct pci_dev; + #define CIO2_NAME "ipu3-cio2" #define CIO2_DEVICE_NAME "Intel IPU3 CIO2" #define CIO2_ENTITY_NAME "ipu3-csi2" -- 2.25.1
Re: [PATCH] software_node: Add kernel-doc comments to exported symbols
Hi Andy On 05/01/2021 14:53, Andy Shevchenko wrote: > On Mon, Jan 04, 2021 at 11:47:36PM +0000, Daniel Scally wrote: >> A number of functions which are exported via EXPORT_SYMBOL_GPL() lack any >> kernel-doc comments; add those in so all exported symbols are documented. > Thanks, it's helpful! > Reviewed-by: Andy Shevchenko > after addressing few nitpicks Thanks for reviewing >> Signed-off-by: Daniel Scally >> --- >> With a view to maybe writing some documentation once the fwnode_graph_*() >> functions are also added. > FWIW, Heikki used to have a draft patch of swnode documentation, not sure > what's the current status of it. Oh cool ok; I'll defer to him then. >> + * copy of the given array of properties and registers it as a new >> fwnode_handle. >> + * Freeing of the allocated memory when the fwnode_handle is no longer >> needed is >> + * handled via software_node_release() and does not need to be done >> separately. >> + * >> + * Returns: >> + * * fwnode_handle *- On success >> + * * -EINVAL- When @parent is not associated with a >> software_node >> + * * -ENOMEM- When memory allocation fails >> + * * -Other - Propagated errors from sub-functions >> + */ >> struct fwnode_handle * >> fwnode_create_software_node(const struct property_entry *properties, >> const struct fwnode_handle *parent) >> @@ -832,6 +875,15 @@ fwnode_create_software_node(const struct property_entry >> *properties, >> } >> EXPORT_SYMBOL_GPL(fwnode_create_software_node); >> >> +/** >> + * fwnode_remove_software_node() - Put a reference to a registered >> software_node >> + * @fwnode: The pointer to the &struct fwnode_handle you want to release >> + * >> + * Release a reference to a registered &struct software_node. This function >> + * differs from software_node_put() in that it takes no action if the >> + * fwnode_handle passed to @fwnode turns out not to have been created by >> + * registering a software_node > Period at the end. > > I'm a bit confused by amount of fwnode_handle in the comments, can you replace > them with better approach depending on the case: > - &struct fwnode_handle > - a parameter as @fwnode or so > - a general mention (better to use plain English here, something like firmware > node handle or so) Yeah ok, I was trying to do &struct fwnode_handle on the first reference (or at least earliest that it would fit) and then fwnode_handle thereafter, but I think I like the suggestion to drop to plain English at that point instead, so I'll do that (and ditto for software_node / software node)
Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Morning Kieran
On 05/01/2021 06:55, Kieran Bingham wrote:
> Hi Dan,
>
> On 04/01/2021 22:02, Daniel Scally wrote:
>>>>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>>>>> +/*
>>>>>>>> + * Extend this array with ACPI Hardware IDs of devices known to be
>>>>>>>> working
>>>>>>>> + * plus the number of link-frequencies expected by their drivers,
>>>>>>>> along with
>>>>>>>> + * the frequency values in hertz. This is somewhat opportunistic way
>>>>>>>> of adding
>>>>>>>> + * support for this for now in the hopes of a better source for the
>>>>>>>> information
>>>>>>>> + * (possibly some encoded value in the SSDB buffer that we're unaware
>>>>>>>> of)
>>>>>>>> + * becoming apparent in the future.
>>>>>>>> + *
>>>>>>>> + * Do not add an entry for a sensor that is not actually supported.
>>>>>>>> + */
>>>>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>>>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>>>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>>> I don't know if these are expressed anywhere else but would it be
>>>>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>>>>> that is represented by this HID?
>>>>>>>
>>>>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>>>>
>>>>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>>>>> anyone looking here to add a new sensor might want to investigate what
>>>>>>> was already in the table?
>>>>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>>>>
>>>>>>
>>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>>> +/* Sensor OVTI5693 */
>>>>>> +CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>>> +/* Sensor OVTI2680 */
>>>>>> +CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>>
>>>>>> As an inline comment won't fit for the sensors that we know
>>>>>> link-frequencies for. That sound ok?
>>>>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>>>>
>>>>> + /* Omnivision OV5693 */
>>>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>> + /* Omnivision OV2680 */
>>>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>
>>>>> but otherwise, yes a comment the line before works for me, as you are
>>>>> right - at the end would not be practical.
>>>> Works for me
>>>>>>>> +static void cio2_bridge_create_fwnode_properties(
>>>>>>>> + struct cio2_sensor *sensor,
>>>>>>>> + const struct cio2_sensor_config *cfg)
>>>>>>>> +{
>>>>>>>> + unsigned int i;
>>>>>>>> +
>>>>>>>> + sensor->prop_names = prop_names;
>>>>>>>> +
>>>>>>>> + for (i = 0; i < CIO2_MAX_LANES; i++)
>>>>>>>> + sensor->data_lanes[i] = i + 1;
>>>>>>> Does something support lane swapping somewhere?
>>>>>>> I assume this is just mapping each lane directly through.
>>>>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>>>>> is just mapping them directly
>>>>> So is this needed? Or is it some future compatibility thing?
>>>>>
>>>>> I haven't seen where it's used yet, but I'm not too worried about it
>>>>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>>>>> constant...
>>>> The endpoints need to have the data-lanes property which passes an array
>>>> of data lanes, but there may well be a better way of doing this. I'm
>>>> just us
