Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 1/1] platform/x86/intel_cht_int33fe: Split code to USB TypeB and TypeC variants | From | Hans de Goede <> | Date | Wed, 18 Sep 2019 11:20:21 +0200 |
| |
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@gmail.com>
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@gmail.com> > + */ > + > +#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).
| |