| Message ID | 1504163557-14432-3-git-send-email-cclaudio@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hdata/i2c: add support to i2c array version 2 in P9 | expand |
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>
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> >
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.
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
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);
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(+)