| Message ID | 20211125165412.535063-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data | expand |
Hi Hans, Thank you for the patch. On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote: > From: Daniel Scally <djrscally@gmail.com> > > The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic > can be forwarded to a device connected to the PMIC as though it were > connected directly to the system bus. Enable this mode when the chip > is initialised. Is there any drawback doing this unconditionally, if nothing is connected to the bus on the other side (including no pull-ups) ? > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > .../x86/intel/int3472/intel_skl_int3472_tps68470.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c > index c05b4cf502fe..42e688f4cad4 100644 > --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c > +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c > @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) > return ret; > } > > + /* Enable I2C daisy chain */ > + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); > + if (ret) { > + dev_err(dev, "Failed to enable i2c daisy chain\n"); > + return ret; > + } > + > dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); > > return 0;
Hi, On 11/26/21 00:39, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote: >> From: Daniel Scally <djrscally@gmail.com> >> >> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic >> can be forwarded to a device connected to the PMIC as though it were >> connected directly to the system bus. Enable this mode when the chip >> is initialised. > > Is there any drawback doing this unconditionally, if nothing is > connected to the bus on the other side (including no pull-ups) ? I actually never took a really close look at this patch, I just sorta inherited it from Daniel. Now that I have taken a close look, I see that it is setting the exact same bits as which get set when enabling the VSIO regulator. The idea here is that the I2C-passthrough only gets enabled when the VSIO regulator is turned on, because some sensors end up shorting the I2C pins to ground when the sensor is not powered. Since we set these bits when powering up the VSIO regulator and since we do that before trying to talk to the sensor I don't think that we need this (hack) anymore. I will give things a try without this change and if things still work I will drop this patch from the set. Daniel, what do you think? Regards, Hans > >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> .../x86/intel/int3472/intel_skl_int3472_tps68470.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >> index c05b4cf502fe..42e688f4cad4 100644 >> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) >> return ret; >> } >> >> + /* Enable I2C daisy chain */ >> + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); >> + if (ret) { >> + dev_err(dev, "Failed to enable i2c daisy chain\n"); >> + return ret; >> + } >> + >> dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); >> >> return 0; >
Hello On 26/11/2021 11:30, Hans de Goede wrote: > Hi, > > On 11/26/21 00:39, Laurent Pinchart wrote: >> Hi Hans, >> >> Thank you for the patch. >> >> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote: >>> From: Daniel Scally <djrscally@gmail.com> >>> >>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic >>> can be forwarded to a device connected to the PMIC as though it were >>> connected directly to the system bus. Enable this mode when the chip >>> is initialised. >> Is there any drawback doing this unconditionally, if nothing is >> connected to the bus on the other side (including no pull-ups) ? > I actually never took a really close look at this patch, I just > sorta inherited it from Daniel. > > Now that I have taken a close look, I see that it is setting the > exact same bits as which get set when enabling the VSIO regulator. > > The idea here is that the I2C-passthrough only gets enabled when > the VSIO regulator is turned on, because some sensors end up > shorting the I2C pins to ground when the sensor is not powered. > > Since we set these bits when powering up the VSIO regulator > and since we do that before trying to talk to the sensor > I don't think that we need this (hack) anymore. > > I will give things a try without this change and if things > still work I will drop this patch from the set. > > Daniel, what do you think? Humm, we're only using the VSIO regulator with the VCM though right? Which might not be on when the ov8865 tries to probe. I haven't tried without this patch to be honest; I set it because that was what Windows does when powering on the PMIC. > Regards, > > Hans > > > > > > >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>> --- >>> .../x86/intel/int3472/intel_skl_int3472_tps68470.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>> index c05b4cf502fe..42e688f4cad4 100644 >>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) >>> return ret; >>> } >>> >>> + /* Enable I2C daisy chain */ >>> + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); >>> + if (ret) { >>> + dev_err(dev, "Failed to enable i2c daisy chain\n"); >>> + return ret; >>> + } >>> + >>> dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); >>> >>> return 0;
Hi, On 11/26/21 12:39, Daniel Scally wrote: > Hello > > On 26/11/2021 11:30, Hans de Goede wrote: >> Hi, >> >> On 11/26/21 00:39, Laurent Pinchart wrote: >>> Hi Hans, >>> >>> Thank you for the patch. >>> >>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote: >>>> From: Daniel Scally <djrscally@gmail.com> >>>> >>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic >>>> can be forwarded to a device connected to the PMIC as though it were >>>> connected directly to the system bus. Enable this mode when the chip >>>> is initialised. >>> Is there any drawback doing this unconditionally, if nothing is >>> connected to the bus on the other side (including no pull-ups) ? >> I actually never took a really close look at this patch, I just >> sorta inherited it from Daniel. >> >> Now that I have taken a close look, I see that it is setting the >> exact same bits as which get set when enabling the VSIO regulator. >> >> The idea here is that the I2C-passthrough only gets enabled when >> the VSIO regulator is turned on, because some sensors end up >> shorting the I2C pins to ground when the sensor is not powered. >> >> Since we set these bits when powering up the VSIO regulator >> and since we do that before trying to talk to the sensor >> I don't think that we need this (hack) anymore. >> >> I will give things a try without this change and if things >> still work I will drop this patch from the set. >> >> Daniel, what do you think? > > > Humm, we're only using the VSIO regulator with the VCM though right? Nope, there is a mapping from VSIO to dovdd for the ov8865 in the board_data; and I'm pretty sure I copied that from your earlier attempts at getting regulator lookups registered :) And even if the VSIO regulator was only used by the VCM, then it would get turned off after probing the VCM, clearing the 2 bits which this commit sets. Which would break things if we did not re-enable it when the ov8865 needs it. > Which might not be on when the ov8865 tries to probe. I haven't tried > without this patch to be honest; I set it because that was what Windows > does when powering on the PMIC. See above, I'm pretty sure we can do without this patch which means that the INT3472 code will no longer be poking at the PMIC directly itself, which is good :) Anyways I'll give this a try sometime next week and then drop the patch. Regards, Hans >>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>>> --- >>>> .../x86/intel/int3472/intel_skl_int3472_tps68470.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>> index c05b4cf502fe..42e688f4cad4 100644 >>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) >>>> return ret; >>>> } >>>> >>>> + /* Enable I2C daisy chain */ >>>> + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); >>>> + if (ret) { >>>> + dev_err(dev, "Failed to enable i2c daisy chain\n"); >>>> + return ret; >>>> + } >>>> + >>>> dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); >>>> >>>> return 0; >
On 26/11/2021 11:45, Hans de Goede wrote: > Hi, > > On 11/26/21 12:39, Daniel Scally wrote: >> Hello >> >> On 26/11/2021 11:30, Hans de Goede wrote: >>> Hi, >>> >>> On 11/26/21 00:39, Laurent Pinchart wrote: >>>> Hi Hans, >>>> >>>> Thank you for the patch. >>>> >>>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote: >>>>> From: Daniel Scally <djrscally@gmail.com> >>>>> >>>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic >>>>> can be forwarded to a device connected to the PMIC as though it were >>>>> connected directly to the system bus. Enable this mode when the chip >>>>> is initialised. >>>> Is there any drawback doing this unconditionally, if nothing is >>>> connected to the bus on the other side (including no pull-ups) ? >>> I actually never took a really close look at this patch, I just >>> sorta inherited it from Daniel. >>> >>> Now that I have taken a close look, I see that it is setting the >>> exact same bits as which get set when enabling the VSIO regulator. >>> >>> The idea here is that the I2C-passthrough only gets enabled when >>> the VSIO regulator is turned on, because some sensors end up >>> shorting the I2C pins to ground when the sensor is not powered. >>> >>> Since we set these bits when powering up the VSIO regulator >>> and since we do that before trying to talk to the sensor >>> I don't think that we need this (hack) anymore. >>> >>> I will give things a try without this change and if things >>> still work I will drop this patch from the set. >>> >>> Daniel, what do you think? >> >> Humm, we're only using the VSIO regulator with the VCM though right? > Nope, there is a mapping from VSIO to dovdd for the ov8865 in the > board_data; and I'm pretty sure I copied that from your earlier > attempts at getting regulator lookups registered :) Oh yeah derp; I was looking at the supply names rather than the regulator names, my bad! > And even if the VSIO regulator was only used by the VCM, then it would > get turned off after probing the VCM, clearing the 2 bits which this > commit sets. Which would break things if we did not re-enable it when > the ov8865 needs it. > >> Which might not be on when the ov8865 tries to probe. I haven't tried >> without this patch to be honest; I set it because that was what Windows >> does when powering on the PMIC. > See above, I'm pretty sure we can do without this patch which means > that the INT3472 code will no longer be poking at the PMIC directly > itself, which is good :) Yeah, in that case I think you're right and this can be dropped. > Anyways I'll give this a try sometime next week and then drop the > patch. Sounds good > > Regards, > > Hans > > > > >>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>>>> --- >>>>> .../x86/intel/int3472/intel_skl_int3472_tps68470.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>>> index c05b4cf502fe..42e688f4cad4 100644 >>>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) >>>>> return ret; >>>>> } >>>>> >>>>> + /* Enable I2C daisy chain */ >>>>> + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); >>>>> + if (ret) { >>>>> + dev_err(dev, "Failed to enable i2c daisy chain\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); >>>>> >>>>> return 0;
Hi, On 11/26/21 12:45, Hans de Goede wrote: > Hi, > > On 11/26/21 12:39, Daniel Scally wrote: >> Hello >> >> On 26/11/2021 11:30, Hans de Goede wrote: >>> Hi, >>> >>> On 11/26/21 00:39, Laurent Pinchart wrote: >>>> Hi Hans, >>>> >>>> Thank you for the patch. >>>> >>>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote: >>>>> From: Daniel Scally <djrscally@gmail.com> >>>>> >>>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic >>>>> can be forwarded to a device connected to the PMIC as though it were >>>>> connected directly to the system bus. Enable this mode when the chip >>>>> is initialised. >>>> Is there any drawback doing this unconditionally, if nothing is >>>> connected to the bus on the other side (including no pull-ups) ? >>> I actually never took a really close look at this patch, I just >>> sorta inherited it from Daniel. >>> >>> Now that I have taken a close look, I see that it is setting the >>> exact same bits as which get set when enabling the VSIO regulator. >>> >>> The idea here is that the I2C-passthrough only gets enabled when >>> the VSIO regulator is turned on, because some sensors end up >>> shorting the I2C pins to ground when the sensor is not powered. >>> >>> Since we set these bits when powering up the VSIO regulator >>> and since we do that before trying to talk to the sensor >>> I don't think that we need this (hack) anymore. >>> >>> I will give things a try without this change and if things >>> still work I will drop this patch from the set. >>> >>> Daniel, what do you think? >> >> >> Humm, we're only using the VSIO regulator with the VCM though right? > > Nope, there is a mapping from VSIO to dovdd for the ov8865 in the > board_data; and I'm pretty sure I copied that from your earlier > attempts at getting regulator lookups registered :) > > And even if the VSIO regulator was only used by the VCM, then it would > get turned off after probing the VCM, clearing the 2 bits which this > commit sets. Which would break things if we did not re-enable it when > the ov8865 needs it. > >> Which might not be on when the ov8865 tries to probe. I haven't tried >> without this patch to be honest; I set it because that was what Windows >> does when powering on the PMIC. > > See above, I'm pretty sure we can do without this patch which means > that the INT3472 code will no longer be poking at the PMIC directly > itself, which is good :) > > Anyways I'll give this a try sometime next week and then drop the > patch. I can confirm that this patch indeed is no longer necessary with the current regulator code already taking care of this. I will post version 7 of this patch-set soon, with this patch dropped. Regards, Hans >>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>>>> --- >>>>> .../x86/intel/int3472/intel_skl_int3472_tps68470.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>>> index c05b4cf502fe..42e688f4cad4 100644 >>>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c >>>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) >>>>> return ret; >>>>> } >>>>> >>>>> + /* Enable I2C daisy chain */ >>>>> + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); >>>>> + if (ret) { >>>>> + dev_err(dev, "Failed to enable i2c daisy chain\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); >>>>> >>>>> return 0; >>
diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c index c05b4cf502fe..42e688f4cad4 100644 --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap) return ret; } + /* Enable I2C daisy chain */ + ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03); + if (ret) { + dev_err(dev, "Failed to enable i2c daisy chain\n"); + return ret; + } + dev_info(dev, "TPS68470 REVID: 0x%02x\n", version); return 0;