[v2,5/5] hdata/i2c: add support to the i2c array version 2

Message ID 1504163557-14432-6-git-send-email-cclaudio@linux.vnet.ibm.com
State Rejected
Headers show
Series
  • hdata/i2c: add support to i2c array version 2 in P9
Related show

Commit Message

Claudio Carvalho Aug. 31, 2017, 7:12 a.m.
This adds the description field to the i2c array, as specified in the i2c
array version 2.

When the description field is populated, we should be able to get the
description (name, compat and label) of new i2c devices from it, instead
of having to input these information manually in the list of known i2c
devices.

Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
---
 hdata/i2c.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Oliver Sept. 1, 2017, 4:17 a.m. | #1
On Thu, Aug 31, 2017 at 5:12 PM, Claudio Carvalho
<cclaudio@linux.vnet.ibm.com> wrote:
> This adds the description field to the i2c array, as specified in the i2c
> array version 2.
>
> When the description field is populated, we should be able to get the
> description (name, compat and label) of new i2c devices from it, instead
> of having to input these information manually in the list of known i2c
> devices.
>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
> ---
>  hdata/i2c.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hdata/i2c.c b/hdata/i2c.c
> index 8a2e392..c365a92 100644
> --- a/hdata/i2c.c
> +++ b/hdata/i2c.c
> @@ -22,6 +22,7 @@ struct i2c_dev {
>         __be32 purpose;
>         __be32 i2c_link;
>         __be16 slca_index;
> +       char desc[64];
>  };
>
>  #define P9_I2CM_XSCOM_SIZE 0x1000
> @@ -202,11 +203,8 @@ int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
>                 version = be32_to_cpu(ahdr->version);
>         }
>
> -       if (version == 2) {
> -               prerror("I2C: v%d found, but not supported. Parsing as v1\n",
> -                       version);
> -       } else if (version > 2) {
> -               prerror("I2C: v%d found, but not supported! THIS IS A BUG\n",
> +       if (version != 1 && version != 2) {
> +               prerror("I2C: HDAT version %d not supported! THIS IS A BUG\n",
>                         version);
>                 return -1;
>         }
> --
> 2.7.4
>

NAK

I added that error in the first place since the spec for the v2
strings  hadn't been finalised and I didn't want it going off into the
weeds if something changed. In hindsight this was a mistake since the
V2 format only extends the V1 format so parsing it as V1 is always
going to work.  This has (almost) always been the case with HDAT so
there's no need to make this a hard error.

I'd just drop this patch until we have support for parsing the
description strings (and they're actually populated).

Oliver
Claudio Carvalho Sept. 4, 2017, 2:53 p.m. | #2
On 01/09/2017 01:17, Oliver wrote:
> On Thu, Aug 31, 2017 at 5:12 PM, Claudio Carvalho
> <cclaudio@linux.vnet.ibm.com> wrote:
>> This adds the description field to the i2c array, as specified in the i2c
>> array version 2.
>>
>> When the description field is populated, we should be able to get the
>> description (name, compat and label) of new i2c devices from it, instead
>> of having to input these information manually in the list of known i2c
>> devices.
>>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
>> ---
>>   hdata/i2c.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/hdata/i2c.c b/hdata/i2c.c
>> index 8a2e392..c365a92 100644
>> --- a/hdata/i2c.c
>> +++ b/hdata/i2c.c
>> @@ -22,6 +22,7 @@ struct i2c_dev {
>>          __be32 purpose;
>>          __be32 i2c_link;
>>          __be16 slca_index;
>> +       char desc[64];
>>   };
>>
>>   #define P9_I2CM_XSCOM_SIZE 0x1000
>> @@ -202,11 +203,8 @@ int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
>>                  version = be32_to_cpu(ahdr->version);
>>          }
>>
>> -       if (version == 2) {
>> -               prerror("I2C: v%d found, but not supported. Parsing as v1\n",
>> -                       version);
>> -       } else if (version > 2) {
>> -               prerror("I2C: v%d found, but not supported! THIS IS A BUG\n",
>> +       if (version != 1 && version != 2) {
>> +               prerror("I2C: HDAT version %d not supported! THIS IS A BUG\n",
>>                          version);
>>                  return -1;
>>          }
>> --
>> 2.7.4
>>
> NAK
>
> I added that error in the first place since the spec for the v2
> strings  hadn't been finalised and I didn't want it going off into the
> weeds if something changed. In hindsight this was a mistake since the
> V2 format only extends the V1 format so parsing it as V1 is always
> going to work.  This has (almost) always been the case with HDAT so
> there's no need to make this a hard error.
>
> I'd just drop this patch until we have support for parsing the
> description strings (and they're actually populated).
>
> Oliver
>

Yeah, makes sense. We can drop this patch for now.

Patch

diff --git a/hdata/i2c.c b/hdata/i2c.c
index 8a2e392..c365a92 100644
--- a/hdata/i2c.c
+++ b/hdata/i2c.c
@@ -22,6 +22,7 @@  struct i2c_dev {
 	__be32 purpose;
 	__be32 i2c_link;
 	__be16 slca_index;
+	char desc[64];
 };
 
 #define P9_I2CM_XSCOM_SIZE 0x1000
@@ -202,11 +203,8 @@  int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
 		version = be32_to_cpu(ahdr->version);
 	}
 
-	if (version == 2) {
-		prerror("I2C: v%d found, but not supported. Parsing as v1\n",
-			version);
-	} else if (version > 2) {
-		prerror("I2C: v%d found, but not supported! THIS IS A BUG\n",
+	if (version != 1 && version != 2) {
+		prerror("I2C: HDAT version %d not supported! THIS IS A BUG\n",
 			version);
 		return -1;
 	}