diff mbox

i2c: i2c-scmi: add a MS HID

Message ID 1489154315-26499-1-git-send-email-vkrasnov@dev.rtsoft.ru
State Superseded
Headers show

Commit Message

Viktor Krasnov March 10, 2017, 1:58 p.m. UTC
From: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>

Description of the problem:
 - i2c-scmi driver contains only two identifiers "SMBUS01" and "SMBUSIBM";
 - the fist HID (SMBUS01) is clearly defined in "SMBus Control Method
   Interface Specification, version 1.0": "Each device must specify 
   'SMBUS01' as its _HID and use a unique _UID value";
 - unfortunately, BIOS vendors (like AMI) seem to ignore this requirement
   and implement "SMB0001" HID instead of "SMBUS01";
 - I speculate that they do this because only "SMB0001" is hard coded in
   Windows SMBus driver produced by Microsoft.

This leads to following situation:
 - SMBus works out of box in Windows but not in Linux;
 - board vendors are forced to add correct "SMBUS01" HID to BIOS to make
   SMBus work in Linux. Moreover the same board vendors complain that
   tools (3-rd party ASL compiler) do not like the "SMBUS01" identifier
   and produce errors.  So they need to constantly patch the compiler for 
   each new version of BIOS.

As it is very unlikely that BIOS vendors implement a correct HID in
future, I would propose to consider whether it is possible to work around
the problem by adding MS HID to the Linux i2c-scmi driver.

Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>
---
 drivers/i2c/busses/i2c-scmi.c | 1 +
 include/acpi/acpi_drivers.h   | 2 ++
 2 files changed, 3 insertions(+)

Comments

Wolfram Sang March 23, 2017, 8:47 p.m. UTC | #1
On Fri, Mar 10, 2017 at 04:58:35PM +0300, Viktor Krasnov wrote:

Thanks for the patch!

adding linux-acpi to CC. This is more an ACPI question than I2C.

> From: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> 
> Description of the problem:
>  - i2c-scmi driver contains only two identifiers "SMBUS01" and "SMBUSIBM";
>  - the fist HID (SMBUS01) is clearly defined in "SMBus Control Method
>    Interface Specification, version 1.0": "Each device must specify 
>    'SMBUS01' as its _HID and use a unique _UID value";
>  - unfortunately, BIOS vendors (like AMI) seem to ignore this requirement
>    and implement "SMB0001" HID instead of "SMBUS01";
>  - I speculate that they do this because only "SMB0001" is hard coded in
>    Windows SMBus driver produced by Microsoft.
> 
> This leads to following situation:
>  - SMBus works out of box in Windows but not in Linux;
>  - board vendors are forced to add correct "SMBUS01" HID to BIOS to make
>    SMBus work in Linux. Moreover the same board vendors complain that
>    tools (3-rd party ASL compiler) do not like the "SMBUS01" identifier
>    and produce errors.  So they need to constantly patch the compiler for 
>    each new version of BIOS.
> 
> As it is very unlikely that BIOS vendors implement a correct HID in
> future, I would propose to consider whether it is possible to work around
> the problem by adding MS HID to the Linux i2c-scmi driver.
> 
> Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
> Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>
> ---
>  drivers/i2c/busses/i2c-scmi.c | 1 +
>  include/acpi/acpi_drivers.h   | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
> index dfc98df..fb9aee0 100644
> --- a/drivers/i2c/busses/i2c-scmi.c
> +++ b/drivers/i2c/busses/i2c-scmi.c
> @@ -51,6 +51,7 @@ struct acpi_smbus_cmi {
>  static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
>  	{"SMBUS01", (kernel_ulong_t)&smbus_methods},
>  	{ACPI_SMBUS_IBM_HID, (kernel_ulong_t)&ibm_smbus_methods},
> +	{ACPI_SMBUS_MS_HID, (kernel_ulong_t)&smbus_methods},
>  	{"", 0}
>  };
>  MODULE_DEVICE_TABLE(acpi, acpi_smbus_cmi_ids);
> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> index 29c6912..d34538b 100644
> --- a/include/acpi/acpi_drivers.h
> +++ b/include/acpi/acpi_drivers.h
> @@ -60,6 +60,8 @@
>  #define ACPI_DOCK_HID			"LNXDOCK"
>  /* Quirk for broken IBM BIOSes */
>  #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> +/* SMBUS HID definition as supported by Microsoft Windows */
> +#define ACPI_SMBUS_MS_HID		"SMB0001"
>  
>  /*
>   * For fixed hardware buttons, we fabricate acpi_devices with HID
> -- 
> 1.9.3
>
Jean Delvare March 24, 2017, 9:16 a.m. UTC | #2
On Fri, 10 Mar 2017 16:58:35 +0300, Viktor Krasnov wrote:
> From: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> 
> Description of the problem:
>  - i2c-scmi driver contains only two identifiers "SMBUS01" and "SMBUSIBM";
>  - the fist HID (SMBUS01) is clearly defined in "SMBus Control Method
>    Interface Specification, version 1.0": "Each device must specify 
>    'SMBUS01' as its _HID and use a unique _UID value";
>  - unfortunately, BIOS vendors (like AMI) seem to ignore this requirement
>    and implement "SMB0001" HID instead of "SMBUS01";
>  - I speculate that they do this because only "SMB0001" is hard coded in
>    Windows SMBus driver produced by Microsoft.
> 
> This leads to following situation:
>  - SMBus works out of box in Windows but not in Linux;
>  - board vendors are forced to add correct "SMBUS01" HID to BIOS to make
>    SMBus work in Linux. Moreover the same board vendors complain that
>    tools (3-rd party ASL compiler) do not like the "SMBUS01" identifier
>    and produce errors.  So they need to constantly patch the compiler for 
>    each new version of BIOS.
> 
> As it is very unlikely that BIOS vendors implement a correct HID in
> future, I would propose to consider whether it is possible to work around
> the problem by adding MS HID to the Linux i2c-scmi driver.
> 
> Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
> Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>
> ---
>  drivers/i2c/busses/i2c-scmi.c | 1 +
>  include/acpi/acpi_drivers.h   | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
> index dfc98df..fb9aee0 100644
> --- a/drivers/i2c/busses/i2c-scmi.c
> +++ b/drivers/i2c/busses/i2c-scmi.c
> @@ -51,6 +51,7 @@ struct acpi_smbus_cmi {
>  static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
>  	{"SMBUS01", (kernel_ulong_t)&smbus_methods},
>  	{ACPI_SMBUS_IBM_HID, (kernel_ulong_t)&ibm_smbus_methods},
> +	{ACPI_SMBUS_MS_HID, (kernel_ulong_t)&smbus_methods},
>  	{"", 0}
>  };
>  MODULE_DEVICE_TABLE(acpi, acpi_smbus_cmi_ids);
> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> index 29c6912..d34538b 100644
> --- a/include/acpi/acpi_drivers.h
> +++ b/include/acpi/acpi_drivers.h
> @@ -60,6 +60,8 @@
>  #define ACPI_DOCK_HID			"LNXDOCK"
>  /* Quirk for broken IBM BIOSes */
>  #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> +/* SMBUS HID definition as supported by Microsoft Windows */
> +#define ACPI_SMBUS_MS_HID		"SMB0001"
>  
>  /*
>   * For fixed hardware buttons, we fabricate acpi_devices with HID

No objection from me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Wolfram Sang March 30, 2017, 3:52 p.m. UTC | #3
On Thu, Mar 23, 2017 at 09:47:47PM +0100, Wolfram Sang wrote:
> On Fri, Mar 10, 2017 at 04:58:35PM +0300, Viktor Krasnov wrote:
> 
> Thanks for the patch!
> 
> adding linux-acpi to CC. This is more an ACPI question than I2C.

Mika? Andy? Jarkko? Any input on this?

> 
> > From: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > 
> > Description of the problem:
> >  - i2c-scmi driver contains only two identifiers "SMBUS01" and "SMBUSIBM";
> >  - the fist HID (SMBUS01) is clearly defined in "SMBus Control Method
> >    Interface Specification, version 1.0": "Each device must specify 
> >    'SMBUS01' as its _HID and use a unique _UID value";
> >  - unfortunately, BIOS vendors (like AMI) seem to ignore this requirement
> >    and implement "SMB0001" HID instead of "SMBUS01";
> >  - I speculate that they do this because only "SMB0001" is hard coded in
> >    Windows SMBus driver produced by Microsoft.
> > 
> > This leads to following situation:
> >  - SMBus works out of box in Windows but not in Linux;
> >  - board vendors are forced to add correct "SMBUS01" HID to BIOS to make
> >    SMBus work in Linux. Moreover the same board vendors complain that
> >    tools (3-rd party ASL compiler) do not like the "SMBUS01" identifier
> >    and produce errors.  So they need to constantly patch the compiler for 
> >    each new version of BIOS.
> > 
> > As it is very unlikely that BIOS vendors implement a correct HID in
> > future, I would propose to consider whether it is possible to work around
> > the problem by adding MS HID to the Linux i2c-scmi driver.
> > 
> > Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
> > Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>
> > ---
> >  drivers/i2c/busses/i2c-scmi.c | 1 +
> >  include/acpi/acpi_drivers.h   | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
> > index dfc98df..fb9aee0 100644
> > --- a/drivers/i2c/busses/i2c-scmi.c
> > +++ b/drivers/i2c/busses/i2c-scmi.c
> > @@ -51,6 +51,7 @@ struct acpi_smbus_cmi {
> >  static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
> >  	{"SMBUS01", (kernel_ulong_t)&smbus_methods},
> >  	{ACPI_SMBUS_IBM_HID, (kernel_ulong_t)&ibm_smbus_methods},
> > +	{ACPI_SMBUS_MS_HID, (kernel_ulong_t)&smbus_methods},
> >  	{"", 0}
> >  };
> >  MODULE_DEVICE_TABLE(acpi, acpi_smbus_cmi_ids);
> > diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> > index 29c6912..d34538b 100644
> > --- a/include/acpi/acpi_drivers.h
> > +++ b/include/acpi/acpi_drivers.h
> > @@ -60,6 +60,8 @@
> >  #define ACPI_DOCK_HID			"LNXDOCK"
> >  /* Quirk for broken IBM BIOSes */
> >  #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> > +/* SMBUS HID definition as supported by Microsoft Windows */
> > +#define ACPI_SMBUS_MS_HID		"SMB0001"
> >  
> >  /*
> >   * For fixed hardware buttons, we fabricate acpi_devices with HID
> > -- 
> > 1.9.3
> >
Mika Westerberg March 31, 2017, 7:20 a.m. UTC | #4
On Thu, Mar 30, 2017 at 05:52:33PM +0200, Wolfram Sang wrote:
> On Thu, Mar 23, 2017 at 09:47:47PM +0100, Wolfram Sang wrote:
> > On Fri, Mar 10, 2017 at 04:58:35PM +0300, Viktor Krasnov wrote:
> > 
> > Thanks for the patch!
> > 
> > adding linux-acpi to CC. This is more an ACPI question than I2C.
> 
> Mika? Andy? Jarkko? Any input on this?
> 
> > 
> > > From: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > > 
> > > Description of the problem:
> > >  - i2c-scmi driver contains only two identifiers "SMBUS01" and "SMBUSIBM";
> > >  - the fist HID (SMBUS01) is clearly defined in "SMBus Control Method
> > >    Interface Specification, version 1.0": "Each device must specify 
> > >    'SMBUS01' as its _HID and use a unique _UID value";
> > >  - unfortunately, BIOS vendors (like AMI) seem to ignore this requirement
> > >    and implement "SMB0001" HID instead of "SMBUS01";
> > >  - I speculate that they do this because only "SMB0001" is hard coded in
> > >    Windows SMBus driver produced by Microsoft.
> > > 
> > > This leads to following situation:
> > >  - SMBus works out of box in Windows but not in Linux;
> > >  - board vendors are forced to add correct "SMBUS01" HID to BIOS to make
> > >    SMBus work in Linux. Moreover the same board vendors complain that
> > >    tools (3-rd party ASL compiler) do not like the "SMBUS01" identifier
> > >    and produce errors.  So they need to constantly patch the compiler for 
> > >    each new version of BIOS.
> > > 
> > > As it is very unlikely that BIOS vendors implement a correct HID in
> > > future, I would propose to consider whether it is possible to work around
> > > the problem by adding MS HID to the Linux i2c-scmi driver.
> > > 
> > > Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > > Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
> > > Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>
> > > ---
> > >  drivers/i2c/busses/i2c-scmi.c | 1 +
> > >  include/acpi/acpi_drivers.h   | 2 ++
> > >  2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
> > > index dfc98df..fb9aee0 100644
> > > --- a/drivers/i2c/busses/i2c-scmi.c
> > > +++ b/drivers/i2c/busses/i2c-scmi.c
> > > @@ -51,6 +51,7 @@ struct acpi_smbus_cmi {
> > >  static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
> > >  	{"SMBUS01", (kernel_ulong_t)&smbus_methods},
> > >  	{ACPI_SMBUS_IBM_HID, (kernel_ulong_t)&ibm_smbus_methods},
> > > +	{ACPI_SMBUS_MS_HID, (kernel_ulong_t)&smbus_methods},
> > >  	{"", 0}
> > >  };
> > >  MODULE_DEVICE_TABLE(acpi, acpi_smbus_cmi_ids);
> > > diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> > > index 29c6912..d34538b 100644
> > > --- a/include/acpi/acpi_drivers.h
> > > +++ b/include/acpi/acpi_drivers.h
> > > @@ -60,6 +60,8 @@
> > >  #define ACPI_DOCK_HID			"LNXDOCK"
> > >  /* Quirk for broken IBM BIOSes */
> > >  #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> > > +/* SMBUS HID definition as supported by Microsoft Windows */
> > > +#define ACPI_SMBUS_MS_HID		"SMB0001"

I agree with the reasoning but why do you add the ID here also? Wouldn't
it suffice if added only to the i2c-scmi.c driver?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viktor Krasnov March 31, 2017, 7:31 a.m. UTC | #5
Hello Mika,

thanks for the review.

> > --- a/include/acpi/acpi_drivers.h
> > +++ b/include/acpi/acpi_drivers.h
> > @@ -60,6 +60,8 @@
> > #define ACPI_DOCK_HID			"LNXDOCK"
> > /* Quirk for broken IBM BIOSes */
> > #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> > +/* SMBUS HID definition as supported by Microsoft Windows */
> > +#define ACPI_SMBUS_MS_HID		"SMB0001"

> I agree with the reasoning but why do you add the ID here also? Wouldn't
> it suffice if added only to the i2c-scmi.c driver?

This has been done by analogy with existing ACPI_SMBUS_IBM_HID which is
defined in acpi_drivers.h. I think that it may look confusing when some
HIDs are defined in i2c-scmi.c and others - in acpi_drivers.h. Could you
please clarify this moment and confirm that all HIDs must be defined
only in i2c-scmi.c? I will rework the patch then.

Best regards,
Viktor Krasnov.

On Fri, 2017-03-31 at 10:20 +0300, Mika Westerberg wrote:
> On Thu, Mar 30, 2017 at 05:52:33PM +0200, Wolfram Sang wrote:
> > On Thu, Mar 23, 2017 at 09:47:47PM +0100, Wolfram Sang wrote:
> > > On Fri, Mar 10, 2017 at 04:58:35PM +0300, Viktor Krasnov wrote:
> > > 
> > > Thanks for the patch!
> > > 
> > > adding linux-acpi to CC. This is more an ACPI question than I2C.
> > 
> > Mika? Andy? Jarkko? Any input on this?
> > 
> > > 
> > > > From: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > > > 
> > > > Description of the problem:
> > > >  - i2c-scmi driver contains only two identifiers "SMBUS01" and "SMBUSIBM";
> > > >  - the fist HID (SMBUS01) is clearly defined in "SMBus Control Method
> > > >    Interface Specification, version 1.0": "Each device must specify 
> > > >    'SMBUS01' as its _HID and use a unique _UID value";
> > > >  - unfortunately, BIOS vendors (like AMI) seem to ignore this requirement
> > > >    and implement "SMB0001" HID instead of "SMBUS01";
> > > >  - I speculate that they do this because only "SMB0001" is hard coded in
> > > >    Windows SMBus driver produced by Microsoft.
> > > > 
> > > > This leads to following situation:
> > > >  - SMBus works out of box in Windows but not in Linux;
> > > >  - board vendors are forced to add correct "SMBUS01" HID to BIOS to make
> > > >    SMBus work in Linux. Moreover the same board vendors complain that
> > > >    tools (3-rd party ASL compiler) do not like the "SMBUS01" identifier
> > > >    and produce errors.  So they need to constantly patch the compiler for 
> > > >    each new version of BIOS.
> > > > 
> > > > As it is very unlikely that BIOS vendors implement a correct HID in
> > > > future, I would propose to consider whether it is possible to work around
> > > > the problem by adding MS HID to the Linux i2c-scmi driver.
> > > > 
> > > > Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > > > Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
> > > > Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>
> > > > ---
> > > >  drivers/i2c/busses/i2c-scmi.c | 1 +
> > > >  include/acpi/acpi_drivers.h   | 2 ++
> > > >  2 files changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
> > > > index dfc98df..fb9aee0 100644
> > > > --- a/drivers/i2c/busses/i2c-scmi.c
> > > > +++ b/drivers/i2c/busses/i2c-scmi.c
> > > > @@ -51,6 +51,7 @@ struct acpi_smbus_cmi {
> > > >  static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
> > > >  	{"SMBUS01", (kernel_ulong_t)&smbus_methods},
> > > >  	{ACPI_SMBUS_IBM_HID, (kernel_ulong_t)&ibm_smbus_methods},
> > > > +	{ACPI_SMBUS_MS_HID, (kernel_ulong_t)&smbus_methods},
> > > >  	{"", 0}
> > > >  };
> > > >  MODULE_DEVICE_TABLE(acpi, acpi_smbus_cmi_ids);
> > > > diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> > > > index 29c6912..d34538b 100644
> > > > --- a/include/acpi/acpi_drivers.h
> > > > +++ b/include/acpi/acpi_drivers.h
> > > > @@ -60,6 +60,8 @@
> > > >  #define ACPI_DOCK_HID			"LNXDOCK"
> > > >  /* Quirk for broken IBM BIOSes */
> > > >  #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> > > > +/* SMBUS HID definition as supported by Microsoft Windows */
> > > > +#define ACPI_SMBUS_MS_HID		"SMB0001"
> 
> I agree with the reasoning but why do you add the ID here also? Wouldn't
> it suffice if added only to the i2c-scmi.c driver?


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg March 31, 2017, 7:47 a.m. UTC | #6
On Fri, Mar 31, 2017 at 10:31:07AM +0300, Viktor Krasnov wrote:
> Hello Mika,
> 
> thanks for the review.
> 
> > > --- a/include/acpi/acpi_drivers.h
> > > +++ b/include/acpi/acpi_drivers.h
> > > @@ -60,6 +60,8 @@
> > > #define ACPI_DOCK_HID			"LNXDOCK"
> > > /* Quirk for broken IBM BIOSes */
> > > #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> > > +/* SMBUS HID definition as supported by Microsoft Windows */
> > > +#define ACPI_SMBUS_MS_HID		"SMB0001"
> 
> > I agree with the reasoning but why do you add the ID here also? Wouldn't
> > it suffice if added only to the i2c-scmi.c driver?
> 
> This has been done by analogy with existing ACPI_SMBUS_IBM_HID which is
> defined in acpi_drivers.h. I think that it may look confusing when some
> HIDs are defined in i2c-scmi.c and others - in acpi_drivers.h. Could you
> please clarify this moment and confirm that all HIDs must be defined
> only in i2c-scmi.c? I will rework the patch then.

If there is only one file using a HID, then I think it is fine to keep
it in the driver itself. If multiple files share the HID then it makes
sense to put it to include/acpi/acpi_drivers.h.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viktor Krasnov March 31, 2017, 8:04 a.m. UTC | #7
Hello Mika,

> If there is only one file using a HID, then I think it is fine to keep
> it in the driver itself. If multiple files share the HID then it makes
> sense to put it to include/acpi/acpi_drivers.h.

Understood, thanks for the detailed explanation. Indeed,
ACPI_SMBUS_IBM_HID is referenced in several files, so it explains why
this HID is defined in include/acpi/acpi_drivers.h and not in i2c-scmi.c
itself.

Ok, I will correct the patch, retest it and resend in next few days.

Best regards,
Viktor Krasnov.

On Fri, 2017-03-31 at 10:47 +0300, Mika Westerberg wrote:
> On Fri, Mar 31, 2017 at 10:31:07AM +0300, Viktor Krasnov wrote:
> > Hello Mika,
> > 
> > thanks for the review.
> > 
> > > > --- a/include/acpi/acpi_drivers.h
> > > > +++ b/include/acpi/acpi_drivers.h
> > > > @@ -60,6 +60,8 @@
> > > > #define ACPI_DOCK_HID			"LNXDOCK"
> > > > /* Quirk for broken IBM BIOSes */
> > > > #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
> > > > +/* SMBUS HID definition as supported by Microsoft Windows */
> > > > +#define ACPI_SMBUS_MS_HID		"SMB0001"
> > 
> > > I agree with the reasoning but why do you add the ID here also? Wouldn't
> > > it suffice if added only to the i2c-scmi.c driver?
> > 
> > This has been done by analogy with existing ACPI_SMBUS_IBM_HID which is
> > defined in acpi_drivers.h. I think that it may look confusing when some
> > HIDs are defined in i2c-scmi.c and others - in acpi_drivers.h. Could you
> > please clarify this moment and confirm that all HIDs must be defined
> > only in i2c-scmi.c? I will rework the patch then.
> 
> If there is only one file using a HID, then I think it is fine to keep
> it in the driver itself. If multiple files share the HID then it makes
> sense to put it to include/acpi/acpi_drivers.h.


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko March 31, 2017, 10:01 a.m. UTC | #8
On Thu, 2017-03-30 at 17:52 +0200, Wolfram Sang wrote:
> On Thu, Mar 23, 2017 at 09:47:47PM +0100, Wolfram Sang wrote:
> > On Fri, Mar 10, 2017 at 04:58:35PM +0300, Viktor Krasnov wrote:

> > > Signed-off-by: Edgar Cherkasov <echerkasov@dev.rtsoft.ru>
> > > Signed-off-by: Michael Brunner <Michael.Brunner@kontron.com>
> > > Acked-by: Viktor Krasnov <vkrasnov@dev.rtsoft.ru>


A bit of offtopic here, but I have noticed kontron.com address in SoB
lines.

I would like to know if Kontron will follow ACPI spec when assembling
firmwares for its board [1]?

Table name LPW8 for SSDT is not allowed by spec and can't be just
decompiled without hacking either table or decompiler.

[1] http://www.kontron.com/products/boards-and-standard-form-factors/sma
rc/smarc-sxbt.html

P.S. We may continue privately to not contaminate mailing list with
unnecessary noise.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
index dfc98df..fb9aee0 100644
--- a/drivers/i2c/busses/i2c-scmi.c
+++ b/drivers/i2c/busses/i2c-scmi.c
@@ -51,6 +51,7 @@  struct acpi_smbus_cmi {
 static const struct acpi_device_id acpi_smbus_cmi_ids[] = {
 	{"SMBUS01", (kernel_ulong_t)&smbus_methods},
 	{ACPI_SMBUS_IBM_HID, (kernel_ulong_t)&ibm_smbus_methods},
+	{ACPI_SMBUS_MS_HID, (kernel_ulong_t)&smbus_methods},
 	{"", 0}
 };
 MODULE_DEVICE_TABLE(acpi, acpi_smbus_cmi_ids);
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 29c6912..d34538b 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -60,6 +60,8 @@ 
 #define ACPI_DOCK_HID			"LNXDOCK"
 /* Quirk for broken IBM BIOSes */
 #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
+/* SMBUS HID definition as supported by Microsoft Windows */
+#define ACPI_SMBUS_MS_HID		"SMB0001"
 
 /*
  * For fixed hardware buttons, we fabricate acpi_devices with HID