[v2,2/5] hdata/i2c: ignore i2c devs marked as unknown in the hdat

Message ID 1504163557-14432-3-git-send-email-cclaudio@linux.vnet.ibm.com
State Under Review
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.
Some I2C devices are marked as unknown in the hdat. We don't need to add
them to the device tree, so this just log them.

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

Comments

Oliver Sept. 1, 2017, 4:03 a.m. | #1
On Thu, Aug 31, 2017 at 5:12 PM, Claudio Carvalho
<cclaudio@linux.vnet.ibm.com> wrote:
> Some I2C devices are marked as unknown in the hdat. We don't need to add
> them to the device tree, so this just log them.
>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
> ---
>  hdata/i2c.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hdata/i2c.c b/hdata/i2c.c
> index cc127d5..bfb5b48 100644
> --- a/hdata/i2c.c
> +++ b/hdata/i2c.c
> @@ -226,6 +226,17 @@ int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
>                  */
>                 i2c_addr = dev->i2c_addr >> 1;
>
> +               /*
> +                * Some i2c devs are marked as unknown in the hdat.
> +                * Ignoring them.
> +                */
> +               if (dev->type == 0xFF) {
> +                       prlog(PR_INFO, "HDAT I2C: ignoring e%dp%d - unknown@%x "
> +                             "(port 0x%x)\n", dev->i2cm_engine,
> +                             dev->i2cm_port, i2c_addr, dev->i2c_port);
> +                       continue;
> +               }
> +

I kept these in the device-tree since I figured that we would want to
know about them even if we don't know what they are. I don't have any
strong opinions about it though so just logging it is fine with me.

>                 prlog(PR_TRACE, "HDAT I2C: found e%dp%d - %x\n",
>                         dev->i2cm_engine, dev->i2cm_port, i2c_addr);
>
> --
> 2.7.4
>

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
Claudio Carvalho Sept. 4, 2017, 3:12 p.m. | #2
On 01/09/2017 01:03, Oliver wrote:
> On Thu, Aug 31, 2017 at 5:12 PM, Claudio Carvalho
> <cclaudio@linux.vnet.ibm.com> wrote:
>> Some I2C devices are marked as unknown in the hdat. We don't need to add
>> them to the device tree, so this just log them.
>>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
>> ---
>>   hdata/i2c.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hdata/i2c.c b/hdata/i2c.c
>> index cc127d5..bfb5b48 100644
>> --- a/hdata/i2c.c
>> +++ b/hdata/i2c.c
>> @@ -226,6 +226,17 @@ int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
>>                   */
>>                  i2c_addr = dev->i2c_addr >> 1;
>>
>> +               /*
>> +                * Some i2c devs are marked as unknown in the hdat.
>> +                * Ignoring them.
>> +                */
>> +               if (dev->type == 0xFF) {
>> +                       prlog(PR_INFO, "HDAT I2C: ignoring e%dp%d - unknown@%x "
>> +                             "(port 0x%x)\n", dev->i2cm_engine,
>> +                             dev->i2cm_port, i2c_addr, dev->i2c_port);
>> +                       continue;
>> +               }
>> +
> I kept these in the device-tree since I figured that we would want to
> know about them even if we don't know what they are. I don't have any
> strong opinions about it though so just logging it is fine with me.

I also don't have any strong opinion about that. I proposed this patch 
because I don't know if an unknown i2c device like that could cause any 
problem in the kernel. Probably it would be better to keep them in the 
device tree even if firmware knows nothing about them.

I will drop this patch

Thanks

Claudio

>
>>                  prlog(PR_TRACE, "HDAT I2C: found e%dp%d - %x\n",
>>                          dev->i2cm_engine, dev->i2cm_port, i2c_addr);
>>
>> --
>> 2.7.4
>>
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
>
Stewart Smith Sept. 20, 2017, 5:44 a.m. | #3
Claudio Carvalho <cclaudio@linux.vnet.ibm.com> writes:
> On 01/09/2017 01:03, Oliver wrote:
>> On Thu, Aug 31, 2017 at 5:12 PM, Claudio Carvalho
>> <cclaudio@linux.vnet.ibm.com> wrote:
>>> Some I2C devices are marked as unknown in the hdat. We don't need to add
>>> them to the device tree, so this just log them.
>>>
>>> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
>>> ---
>>>   hdata/i2c.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/hdata/i2c.c b/hdata/i2c.c
>>> index cc127d5..bfb5b48 100644
>>> --- a/hdata/i2c.c
>>> +++ b/hdata/i2c.c
>>> @@ -226,6 +226,17 @@ int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
>>>                   */
>>>                  i2c_addr = dev->i2c_addr >> 1;
>>>
>>> +               /*
>>> +                * Some i2c devs are marked as unknown in the hdat.
>>> +                * Ignoring them.
>>> +                */
>>> +               if (dev->type == 0xFF) {
>>> +                       prlog(PR_INFO, "HDAT I2C: ignoring e%dp%d - unknown@%x "
>>> +                             "(port 0x%x)\n", dev->i2cm_engine,
>>> +                             dev->i2cm_port, i2c_addr, dev->i2c_port);
>>> +                       continue;
>>> +               }
>>> +
>> I kept these in the device-tree since I figured that we would want to
>> know about them even if we don't know what they are. I don't have any
>> strong opinions about it though so just logging it is fine with me.
>
> I also don't have any strong opinion about that. I proposed this patch 
> because I don't know if an unknown i2c device like that could cause any 
> problem in the kernel. Probably it would be better to keep them in the 
> device tree even if firmware knows nothing about them.
>
> I will drop this patch

I don't think we're in a great position to bet on what is and isn't good
in the HDAT and we'll likely only learn by seeing what's in it and what
happens rather than theorising.

What I might do is take the patch but just have it PR_WARNING out while
still adding the device and just see what shows up for a while.
Claudio Carvalho Sept. 27, 2017, 9:43 p.m. | #4
On 20/09/2017 02:44, Stewart Smith wrote:
> Claudio Carvalho <cclaudio@linux.vnet.ibm.com> writes:
>> On 01/09/2017 01:03, Oliver wrote:
>>> On Thu, Aug 31, 2017 at 5:12 PM, Claudio Carvalho
>>> <cclaudio@linux.vnet.ibm.com> wrote:
>>>> Some I2C devices are marked as unknown in the hdat. We don't need to add
>>>> them to the device tree, so this just log them.
>>>>
>>>> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
>>>> ---
>>>>    hdata/i2c.c | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/hdata/i2c.c b/hdata/i2c.c
>>>> index cc127d5..bfb5b48 100644
>>>> --- a/hdata/i2c.c
>>>> +++ b/hdata/i2c.c
>>>> @@ -226,6 +226,17 @@ int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
>>>>                    */
>>>>                   i2c_addr = dev->i2c_addr >> 1;
>>>>
>>>> +               /*
>>>> +                * Some i2c devs are marked as unknown in the hdat.
>>>> +                * Ignoring them.
>>>> +                */
>>>> +               if (dev->type == 0xFF) {
>>>> +                       prlog(PR_INFO, "HDAT I2C: ignoring e%dp%d - unknown@%x "
>>>> +                             "(port 0x%x)\n", dev->i2cm_engine,
>>>> +                             dev->i2cm_port, i2c_addr, dev->i2c_port);
>>>> +                       continue;
>>>> +               }
>>>> +
>>> I kept these in the device-tree since I figured that we would want to
>>> know about them even if we don't know what they are. I don't have any
>>> strong opinions about it though so just logging it is fine with me.
>> I also don't have any strong opinion about that. I proposed this patch
>> because I don't know if an unknown i2c device like that could cause any
>> problem in the kernel. Probably it would be better to keep them in the
>> device tree even if firmware knows nothing about them.
>>
>> I will drop this patch
> I don't think we're in a great position to bet on what is and isn't good
> in the HDAT and we'll likely only learn by seeing what's in it and what
> happens rather than theorising.
>
> What I might do is take the patch but just have it PR_WARNING out while
> still adding the device and just see what shows up for a while.

Yeah, sounds good. I will update the patch.

Thanks,

Claudio

Patch

diff --git a/hdata/i2c.c b/hdata/i2c.c
index cc127d5..bfb5b48 100644
--- a/hdata/i2c.c
+++ b/hdata/i2c.c
@@ -226,6 +226,17 @@  int parse_i2c_devs(const struct HDIF_common_hdr *hdr, int idata_index,
 		 */
 		i2c_addr = dev->i2c_addr >> 1;
 
+		/*
+		 * Some i2c devs are marked as unknown in the hdat.
+		 * Ignoring them.
+		 */
+		if (dev->type == 0xFF) {
+			prlog(PR_INFO, "HDAT I2C: ignoring e%dp%d - unknown@%x "
+			      "(port 0x%x)\n", dev->i2cm_engine,
+			      dev->i2cm_port, i2c_addr, dev->i2c_port);
+			continue;
+		}
+
 		prlog(PR_TRACE, "HDAT I2C: found e%dp%d - %x\n",
 			dev->i2cm_engine, dev->i2cm_port, i2c_addr);