| Message ID | 20170814201425.20991-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Mon, Aug 14, 2017 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote: > At least one BIOS enumerates the max17047 both through the INT33FE ACPI > device (it is right there in the resources table) as well as through a > separate MAX17047 device. > > This commit checks for the max17047 already being enumerated through > a separate MAX17047 ACPI device and if so it uses the i2c-client > instantiated for this and attaches the device-props for the max17047 to > that i2c-client. > +int cht_int33fe_check_for_max17047(struct device *dev, void *data) > +{ > + const char *name = dev_name(dev); > + struct i2c_client **max17047 = data; > + > + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { Can we stop using bad practice of comparing against _instance_? If device is suppose to be single in the system, wouldn't _HID be enough? > + *max17047 = to_i2c_client(dev); > + return 1; > + } > + > + return 0; > +}
Hi, On 14-08-17 22:45, Andy Shevchenko wrote: > On Mon, Aug 14, 2017 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> At least one BIOS enumerates the max17047 both through the INT33FE ACPI >> device (it is right there in the resources table) as well as through a >> separate MAX17047 device. >> >> This commit checks for the max17047 already being enumerated through >> a separate MAX17047 ACPI device and if so it uses the i2c-client >> instantiated for this and attaches the device-props for the max17047 to >> that i2c-client. > >> +int cht_int33fe_check_for_max17047(struct device *dev, void *data) >> +{ >> + const char *name = dev_name(dev); >> + struct i2c_client **max17047 = data; >> + >> + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { > > Can we stop using bad practice of comparing against _instance_? > If device is suppose to be single in the system, wouldn't _HID be enough? Yes _HID would be enough, but that takes some extra code with little gain IMHO, we are effectively checking the HID here as that is where the device-name comes from. Anyways if you strongly prefer a HID check I can do a v2 doing that either way let me know. Regards, Hans > >> + *max17047 = to_i2c_client(dev); >> + return 1; >> + } >> + >> + return 0; >> +} > >
Hi, On 14-08-17 22:14, Hans de Goede wrote: > At least one BIOS enumerates the max17047 both through the INT33FE ACPI > device (it is right there in the resources table) as well as through a > separate MAX17047 device. > > This commit checks for the max17047 already being enumerated through > a separate MAX17047 ACPI device and if so it uses the i2c-client > instantiated for this and attaches the device-props for the max17047 to > that i2c-client. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> What is the status of this patch ? It would be nice to get this fix into 4.14. Regards, Hans > --- > drivers/platform/x86/intel_cht_int33fe.c | 64 +++++++++++++++++++++++++++----- > 1 file changed, 54 insertions(+), 10 deletions(-) > > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c > index da706e2c4232..5f1924fb3190 100644 > --- a/drivers/platform/x86/intel_cht_int33fe.c > +++ b/drivers/platform/x86/intel_cht_int33fe.c > @@ -34,6 +34,35 @@ struct cht_int33fe_data { > struct i2c_client *pi3usb30532; > }; > > +/* > + * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates > + * the max17047 both through the INT33FE ACPI device (it is right there > + * in the resources table) as well as through a separate MAX17047 device. > + * > + * These helpers are used to work around this by checking if an i2c-client > + * for the max17047 has already been registered. > + */ > +int cht_int33fe_check_for_max17047(struct device *dev, void *data) > +{ > + const char *name = dev_name(dev); > + struct i2c_client **max17047 = data; > + > + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { > + *max17047 = to_i2c_client(dev); > + return 1; > + } > + > + return 0; > +} > + > +struct i2c_client *cht_int33fe_find_max17047(void) > +{ > + struct i2c_client *max17047 = NULL; > + > + i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047); > + return max17047; > +} > + > static const char * const max17047_suppliers[] = { "bq24190-charger" }; > > static const struct property_entry max17047_props[] = { > @@ -46,9 +75,10 @@ static int cht_int33fe_probe(struct i2c_client *client) > struct device *dev = &client->dev; > struct i2c_board_info board_info; > struct cht_int33fe_data *data; > + struct i2c_client *max17047; > unsigned long long ptyp; > acpi_status status; > - int fusb302_irq; > + int ret, fusb302_irq; > > status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp); > if (ACPI_FAILURE(status)) { > @@ -75,13 +105,25 @@ static int cht_int33fe_probe(struct i2c_client *client) > if (!data) > return -ENOMEM; > > - memset(&board_info, 0, sizeof(board_info)); > - strlcpy(board_info.type, "max17047", I2C_NAME_SIZE); > - board_info.properties = max17047_props; > - > - data->max17047 = i2c_acpi_new_device(dev, 1, &board_info); > - if (!data->max17047) > - return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */ > + /* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */ > + max17047 = cht_int33fe_find_max17047(); > + if (max17047) { > + /* Pre-existing i2c-client for the max17047, add device-props */ > + ret = device_add_properties(&max17047->dev, max17047_props); > + if (ret) > + return ret; > + /* And re-probe to get the new device-props applied. */ > + ret = device_reprobe(&max17047->dev); > + if (ret) > + dev_warn(dev, "Reprobing max17047 error: %d\n", ret); > + } else { > + memset(&board_info, 0, sizeof(board_info)); > + strlcpy(board_info.type, "max17047", I2C_NAME_SIZE); > + board_info.properties = max17047_props; > + data->max17047 = i2c_acpi_new_device(dev, 1, &board_info); > + if (!data->max17047) > + return -EPROBE_DEFER; /* Wait for i2c-adapter to load */ > + } > > memset(&board_info, 0, sizeof(board_info)); > strlcpy(board_info.type, "fusb302", I2C_NAME_SIZE); > @@ -106,7 +148,8 @@ static int cht_int33fe_probe(struct i2c_client *client) > i2c_unregister_device(data->fusb302); > > out_unregister_max17047: > - i2c_unregister_device(data->max17047); > + if (data->max17047) > + i2c_unregister_device(data->max17047); > > return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */ > } > @@ -117,7 +160,8 @@ static int cht_int33fe_remove(struct i2c_client *i2c) > > i2c_unregister_device(data->pi3usb30532); > i2c_unregister_device(data->fusb302); > - i2c_unregister_device(data->max17047); > + if (data->max17047) > + i2c_unregister_device(data->max17047); > > return 0; > } >
On Mon, Aug 14, 2017 at 11:52 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On 14-08-17 22:45, Andy Shevchenko wrote: >> On Mon, Aug 14, 2017 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> >> wrote: >>> +int cht_int33fe_check_for_max17047(struct device *dev, void *data) >>> +{ >>> + const char *name = dev_name(dev); >>> + struct i2c_client **max17047 = data; >>> + >>> + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { >> >> >> Can we stop using bad practice of comparing against _instance_? >> If device is suppose to be single in the system, wouldn't _HID be enough? > Yes _HID would be enough, but that takes some extra code with little > gain IMHO, we are effectively checking the HID here as that is where > the device-name comes from. > > Anyways if you strongly prefer a HID check I can do a v2 doing that > either way let me know. Currently we have the following modules where ACPI instance is used in: drivers/acpi/acpi_lpss.c drivers/input/touchscreen/goodix.c drivers/platform/x86/silead_dmi.c drivers/power/supply/axp288_charger.c and plenty under sound/soc/intel. I do not care right now about sound/soc/intel stuff, while everywhere else would be better to avoid this. Mika, Rafael, what're yours opinions regarding to use ACPI instances in the drivers? For me it sounds fragile.
On 08/31/2017 07:04 PM, Andy Shevchenko wrote: > On Mon, Aug 14, 2017 at 11:52 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> On 14-08-17 22:45, Andy Shevchenko wrote: >>> On Mon, Aug 14, 2017 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> >>> wrote: > >>>> +int cht_int33fe_check_for_max17047(struct device *dev, void *data) >>>> +{ >>>> + const char *name = dev_name(dev); >>>> + struct i2c_client **max17047 = data; >>>> + >>>> + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { >>> >>> >>> Can we stop using bad practice of comparing against _instance_? >>> If device is suppose to be single in the system, wouldn't _HID be enough? > >> Yes _HID would be enough, but that takes some extra code with little >> gain IMHO, we are effectively checking the HID here as that is where >> the device-name comes from. >> >> Anyways if you strongly prefer a HID check I can do a v2 doing that >> either way let me know. > > Currently we have the following modules where ACPI instance is used in: > > drivers/acpi/acpi_lpss.c > drivers/input/touchscreen/goodix.c > drivers/platform/x86/silead_dmi.c > drivers/power/supply/axp288_charger.c > > and plenty under sound/soc/intel. > > I do not care right now about sound/soc/intel stuff, while everywhere > else would be better to avoid this. > My 2 cents: sound subsystem needs to match exactly against instance since we (must) know how each component are wired in HW. For instance nothing prevents to have multiple audio codec chips with the same HID. There are couple examples under sound/soc/intel where system have multiple codecs with the same HID. (I don't know do we have yet a better way in ACPI to describe audio HW than matching know instances and tying them together with a code).
On Thu, Aug 31, 2017 at 07:04:46PM +0300, Andy Shevchenko wrote: > On Mon, Aug 14, 2017 at 11:52 PM, Hans de Goede <hdegoede@redhat.com> wrote: > > On 14-08-17 22:45, Andy Shevchenko wrote: > >> On Mon, Aug 14, 2017 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> > >> wrote: > > >>> +int cht_int33fe_check_for_max17047(struct device *dev, void *data) > >>> +{ > >>> + const char *name = dev_name(dev); > >>> + struct i2c_client **max17047 = data; > >>> + > >>> + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { > >> > >> > >> Can we stop using bad practice of comparing against _instance_? > >> If device is suppose to be single in the system, wouldn't _HID be enough? > > > Yes _HID would be enough, but that takes some extra code with little > > gain IMHO, we are effectively checking the HID here as that is where > > the device-name comes from. > > > > Anyways if you strongly prefer a HID check I can do a v2 doing that > > either way let me know. > > Currently we have the following modules where ACPI instance is used in: > > drivers/acpi/acpi_lpss.c > drivers/input/touchscreen/goodix.c > drivers/platform/x86/silead_dmi.c > drivers/power/supply/axp288_charger.c > > and plenty under sound/soc/intel. > > I do not care right now about sound/soc/intel stuff, while everywhere > else would be better to avoid this. > > Mika, Rafael, what're yours opinions regarding to use ACPI instances > in the drivers? Since the instance name is generated by the ACPI core and in theory could change, I agree this is pretty fragile. Using _HID/_UID should be the preferred way. However, it is not always possible so we end up doing hacks like this.
Hi, On 09/01/2017 11:19 AM, Mika Westerberg wrote: > On Thu, Aug 31, 2017 at 07:04:46PM +0300, Andy Shevchenko wrote: >> On Mon, Aug 14, 2017 at 11:52 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> On 14-08-17 22:45, Andy Shevchenko wrote: >>>> On Mon, Aug 14, 2017 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> >>>> wrote: >> >>>>> +int cht_int33fe_check_for_max17047(struct device *dev, void *data) >>>>> +{ >>>>> + const char *name = dev_name(dev); >>>>> + struct i2c_client **max17047 = data; >>>>> + >>>>> + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { >>>> >>>> >>>> Can we stop using bad practice of comparing against _instance_? >>>> If device is suppose to be single in the system, wouldn't _HID be enough? >> >>> Yes _HID would be enough, but that takes some extra code with little >>> gain IMHO, we are effectively checking the HID here as that is where >>> the device-name comes from. >>> >>> Anyways if you strongly prefer a HID check I can do a v2 doing that >>> either way let me know. >> >> Currently we have the following modules where ACPI instance is used in: >> >> drivers/acpi/acpi_lpss.c >> drivers/input/touchscreen/goodix.c >> drivers/platform/x86/silead_dmi.c >> drivers/power/supply/axp288_charger.c >> >> and plenty under sound/soc/intel. >> >> I do not care right now about sound/soc/intel stuff, while everywhere >> else would be better to avoid this. >> >> Mika, Rafael, what're yours opinions regarding to use ACPI instances >> in the drivers? > > Since the instance name is generated by the ACPI core and in theory > could change, I agree this is pretty fragile. Using _HID/_UID should be > the preferred way. However, it is not always possible so we end up doing > hacks like this. Given that on the device in question where we need this workaround there is only ever 1 max17047, including the instance number in the check really is a non-problem, but if there is a strong preference for me to change the check over to a _HID check then I can do a v2 doing so. Regards, Hans
On Fri, Sep 01, 2017 at 11:58:55PM +0200, Hans de Goede wrote: > Given that on the device in question where we need this workaround there > is only ever 1 max17047, including the instance number in the check really > is a non-problem, but if there is a strong preference for me to change > the check over to a _HID check then I can do a v2 doing so. I would still prefer _HID check if possible. Otherwise someone finds this hack later and uses it in another driver thinking it is the right thing to do ;-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c index da706e2c4232..5f1924fb3190 100644 --- a/drivers/platform/x86/intel_cht_int33fe.c +++ b/drivers/platform/x86/intel_cht_int33fe.c @@ -34,6 +34,35 @@ struct cht_int33fe_data { struct i2c_client *pi3usb30532; }; +/* + * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates + * the max17047 both through the INT33FE ACPI device (it is right there + * in the resources table) as well as through a separate MAX17047 device. + * + * These helpers are used to work around this by checking if an i2c-client + * for the max17047 has already been registered. + */ +int cht_int33fe_check_for_max17047(struct device *dev, void *data) +{ + const char *name = dev_name(dev); + struct i2c_client **max17047 = data; + + if (name && strcmp(name, "i2c-MAX17047:00") == 0) { + *max17047 = to_i2c_client(dev); + return 1; + } + + return 0; +} + +struct i2c_client *cht_int33fe_find_max17047(void) +{ + struct i2c_client *max17047 = NULL; + + i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047); + return max17047; +} + static const char * const max17047_suppliers[] = { "bq24190-charger" }; static const struct property_entry max17047_props[] = { @@ -46,9 +75,10 @@ static int cht_int33fe_probe(struct i2c_client *client) struct device *dev = &client->dev; struct i2c_board_info board_info; struct cht_int33fe_data *data; + struct i2c_client *max17047; unsigned long long ptyp; acpi_status status; - int fusb302_irq; + int ret, fusb302_irq; status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp); if (ACPI_FAILURE(status)) { @@ -75,13 +105,25 @@ static int cht_int33fe_probe(struct i2c_client *client) if (!data) return -ENOMEM; - memset(&board_info, 0, sizeof(board_info)); - strlcpy(board_info.type, "max17047", I2C_NAME_SIZE); - board_info.properties = max17047_props; - - data->max17047 = i2c_acpi_new_device(dev, 1, &board_info); - if (!data->max17047) - return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */ + /* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */ + max17047 = cht_int33fe_find_max17047(); + if (max17047) { + /* Pre-existing i2c-client for the max17047, add device-props */ + ret = device_add_properties(&max17047->dev, max17047_props); + if (ret) + return ret; + /* And re-probe to get the new device-props applied. */ + ret = device_reprobe(&max17047->dev); + if (ret) + dev_warn(dev, "Reprobing max17047 error: %d\n", ret); + } else { + memset(&board_info, 0, sizeof(board_info)); + strlcpy(board_info.type, "max17047", I2C_NAME_SIZE); + board_info.properties = max17047_props; + data->max17047 = i2c_acpi_new_device(dev, 1, &board_info); + if (!data->max17047) + return -EPROBE_DEFER; /* Wait for i2c-adapter to load */ + } memset(&board_info, 0, sizeof(board_info)); strlcpy(board_info.type, "fusb302", I2C_NAME_SIZE); @@ -106,7 +148,8 @@ static int cht_int33fe_probe(struct i2c_client *client) i2c_unregister_device(data->fusb302); out_unregister_max17047: - i2c_unregister_device(data->max17047); + if (data->max17047) + i2c_unregister_device(data->max17047); return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */ } @@ -117,7 +160,8 @@ static int cht_int33fe_remove(struct i2c_client *i2c) i2c_unregister_device(data->pi3usb30532); i2c_unregister_device(data->fusb302); - i2c_unregister_device(data->max17047); + if (data->max17047) + i2c_unregister_device(data->max17047); return 0; }
At least one BIOS enumerates the max17047 both through the INT33FE ACPI device (it is right there in the resources table) as well as through a separate MAX17047 device. This commit checks for the max17047 already being enumerated through a separate MAX17047 ACPI device and if so it uses the i2c-client instantiated for this and attaches the device-props for the max17047 to that i2c-client. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/intel_cht_int33fe.c | 64 +++++++++++++++++++++++++++----- 1 file changed, 54 insertions(+), 10 deletions(-)