Hi,
On 17-09-2019 21:45, Yauhen Kharuzhy wrote:
Existing intel_cht_int33fe ACPI pseudo-device driver assumes that
hardware has TypeC connector and register related devices described as
I2C connections in the _CRS resource.
There is at least one hardware (Lenovo Yoga Book YB1-91L/F) with micro
USB B connector exists. It has INT33FE device in the DSDT table but
there are only two I2C connection described: PMIC and BQ27452 battery
fuel gauge.
Splitting existing INT33FE driver allow to maintain code for USB type B
(AB) connector variant separately and make it simpler.
Split driver to intel_cht_int33fe_common.c and
intel_cht_int33fe_{typeb,typec}.c. Compile all this source to one .ko
module to make user experience easier.
Signed-off-by: Yauhen Kharuzhy <jekhor@xxxxxxxxx>
Thank you for doing this, this version looks much better IMHO.
Note that this does not apply to Linus' current master, please
rebase. Specifically this conflicts with this patch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=78cd4bf53635d3aeb73435bc89eb6eb39450f315
Which just got merged. Instead of rebasing on Linus' master
(which is always a bit adventurous to use during the merge window)
you can also cherry-pick that single commit on top of v5.3
and use that as a base.
Note that that patch makes changes to struct cht_int33fe_data
specifically it drops the:
struct fwnode_handle *mux;
Member, so when rebasing you should drop that in the new
version of the struct on common.h .
Besides that this need a rebase, overall this looks good, I have some
small remarks inline:
<snip>
diff --git a/drivers/platform/x86/intel_cht_int33fe_common.c b/drivers/platform/x86/intel_cht_int33fe_common.c
new file mode 100644
index 000000000000..4a3d7ebd37dd
--- /dev/null
+++ b/drivers/platform/x86/intel_cht_int33fe_common.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common code for Intel Cherry Trail ACPI INT33FE pseudo device drivers
+ * (USB TypeB and TypeC connector variants).
+ *
+ * Copyright (c) 2019 Yauhen Kharuzhy <jekhor@xxxxxxxxx>
+ */
+
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "intel_cht_int33fe_common.h"
+
+#define EXPECTED_PTYPE 4
+
+static int cht_int33fe_i2c_res_filter(struct acpi_resource *ares, void *data)
+{
+ struct acpi_resource_i2c_serialbus *sb;
+ int *count = data;
+
+ if (i2c_acpi_get_i2c_resource(ares, &sb))
+ (*count)++;
+
+ return 1;
+}
+
+static int cht_int33fe_count_i2c_clients(struct device *dev)
+{
+ struct acpi_device *adev;
+ LIST_HEAD(resource_list);
+ int count = 0;
+
+ adev = ACPI_COMPANION(dev);
+ if (!adev)
+ return -EINVAL;
+
+ acpi_dev_get_resources(adev, &resource_list,
+ cht_int33fe_i2c_res_filter, &count);
+
+ acpi_dev_free_resource_list(&resource_list);
+
+ return count;
+}
+
+static int cht_int33fe_check_hw_type(struct device *dev)
+{
+ unsigned long long ptyp;
+ acpi_status status;
+ int ret;
+
+ status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
+ if (ACPI_FAILURE(status)) {
+ dev_err(dev, "Error getting PTYPE\n");
+ return -ENODEV;
+ }
+
+ /*
+ * The same ACPI HID is used for different configurations check PTYP
+ * to ensure that we are dealing with the expected config.
+ */
+ if (ptyp != EXPECTED_PTYPE)
+ return -ENODEV;
+
+ /* Check presence of INT34D3 (hardware-rev 3) expected for ptype == 4 */
+ if (!acpi_dev_present("INT34D3", "1", 3)) {
+ dev_err(dev, "Error PTYPE == %d, but no INT34D3 device\n",
+ EXPECTED_PTYPE);
+ return -ENODEV;
+ }
+
+ ret = cht_int33fe_count_i2c_clients(dev);
+ if (ret < 0)
+ return ret;
+
+ switch (ret) {
+ case 2:
+ return INT33FE_HW_TYPEB;
+ case 4:
+ return INT33FE_HW_TYPEC;
+ default:
+ return -ENODEV;
+ }
+}
+
+static int cht_int33fe_probe(struct platform_device *pdev)
+{
+ struct cht_int33fe_data *data;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ ret = cht_int33fe_check_hw_type(dev);
+ if (ret < 0)
+ return ret;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->hw_type = ret;
+ data->dev = dev;
+
+ switch (data->hw_type) {
I suggested adding hw_type to data so that it could be used to
select the right remove function in cht_int33fe_remove(),
since you are using a remove function pointer in data for this
(which is fine), there is no reason to store hw_type in
cht_int33fe_data anymore, please drop this and change the
switch (data->hw_type) {
to:
switch (ret) {
+ case INT33FE_HW_TYPEB:
+ data->probe = cht_int33fe_typeb_probe;
+ data->remove = cht_int33fe_typeb_remove;
+ break;
+
+ case INT33FE_HW_TYPEC:
+ data->probe = cht_int33fe_typec_probe;
+ data->remove = cht_int33fe_typec_remove;
+ break;
+ }
+
+ platform_set_drvdata(pdev, data);
+
+ return data->probe(data);
+}
+
+static int cht_int33fe_remove(struct platform_device *pdev)
+{
+ struct cht_int33fe_data *data = platform_get_drvdata(pdev);
+
+ return data->remove(data);
+}
<snip>
diff --git a/drivers/platform/x86/intel_cht_int33fe_common.h b/drivers/platform/x86/intel_cht_int33fe_common.h
new file mode 100644
index 000000000000..cb0cc4552017
--- /dev/null
+++ b/drivers/platform/x86/intel_cht_int33fe_common.h
@@ -0,0 +1,44 @@
<snip>
+int cht_int33fe_typec_remove(struct cht_int33fe_data *data);
+
+#endif /* _INTEL_CHT_INT33FE_COMMON_H */
+
Please drop the empty line at the end of intel_cht_int33fe_common.h .
diff --git a/drivers/platform/x86/intel_cht_int33fe_typeb.c b/drivers/platform/x86/intel_cht_int33fe_typeb.c
new file mode 100644
index 000000000000..905c29f2f714
--- /dev/null
+++ b/drivers/platform/x86/intel_cht_int33fe_typeb.c
@@ -0,0 +1,64 @@
<snip>
+int cht_int33fe_typeb_remove(struct cht_int33fe_data *data)
+{
+ i2c_unregister_device(data->battery_fg);
+
+ return 0;
+}
+
diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe_typec.c
Please drop the empty line at the end of intel_cht_int33fe_typeb.c .
Regards,
Hans
p.s.
I've done the rebase myself and I'm building a kernel with my re-based version of this
patch to test it on a typec device. I will get back to you with the results
(I expect things will just work, just making sure).
