Message-ID: <2fd5400e-e587-54d2-1071-ad8df49a8a68@redhat.com>
Date:   Fri, 26 Nov 2021 12:45:59 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain
Content-Language: en-US
To:     Daniel Scally <djrscally@gmail.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Mark Gross <markgross@kernel.org>,
        Andy Shevchenko <andy@infradead.org>,
        Wolfram Sang <wsa@the-dreams.de>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, Len Brown <lenb@kernel.org>,
        linux-acpi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Kate Hsuan <hpa@redhat.com>, linux-media@vger.kernel.org,
        linux-clk@vger.kernel.org,
        Andy Shevchenko <andy.shevchenko@gmail.com>
References: <20211125165412.535063-1-hdegoede@redhat.com>
 <20211125165412.535063-8-hdegoede@redhat.com>
 <YaAel9HuAvemRg2s@pendragon.ideasonboard.com>
 <03306e12-40ec-39ab-3b40-42b0395e1b65@redhat.com>
 <4ab5efa7-65b0-009c-293a-d7a49776e78d@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <4ab5efa7-65b0-009c-293a-d7a49776e78d@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;
>