Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1724383 > unrolled thread

Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on some devices

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on  some devices Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-08-31 18:10 +0200
    Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on  some devices Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2017-09-01 08:30 +0200
    Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on  some devices Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-09-01 11:20 +0200
      Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on  some devices Hans de Goede <hdegoede@redhat.com> - 2017-09-02 00:00 +0200
        Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on  some devices Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-09-02 09:20 +0200

#1724383 — Re: [PATCH] platform/x86: intel_cht_int33fe: Work around BIOS bug on some devices

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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [next] | [standalone]

#1724802

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).

-- 
Jarkko

[toc] | [prev] | [next] | [standalone]

#1724899

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.

[toc] | [prev] | [next] | [standalone]

#1725379

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

[toc] | [prev] | [next] | [standalone]

#1725461

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 ;-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web