Re: [PATCH 2/2] mfd: mt6397: Add basic support for MT6331+MT6332 PMIC

From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	matthias.bgg@gmail.com, johnson.wang@mediatek.com,
	hsin-hsiung.wang@mediatek.com, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] mfd: mt6397: Add basic support for MT6331+MT6332 PMIC
Date: Thu, 16 Jun 2022 10:43:51 +0200	[thread overview]
Message-ID: <c46dc265-115b-d27a-9e74-a7c48497cdfc@collabora.com> (raw)
In-Reply-To: <YqpUmE5+Kmq9rxwB@google.com>

Il 15/06/22 23:52, Lee Jones ha scritto:
> On Fri, 20 May 2022, AngeloGioacchino Del Regno wrote:
> 
>> Add support for the MT6331 PMIC with MT6332 Companion PMIC, found
>> in MT6795 Helio X10 smartphone platforms.
>>
>> This combo has support for multiple devices but, for a start,
>> only the following have been implemented:
>> - Regulators (two instances, one in MT6331, one in MT6332)
>> - RTC (MT6331)
>> - Keys (MT6331)
>> - Interrupts (MT6331 also dispatches MT6332's interrupts)
>>
>> There's more to be implemented, especially for MT6332, which
>> will come at a later stage.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/mfd/mt6397-core.c            |  47 ++
>>   drivers/mfd/mt6397-irq.c             |   9 +-
>>   include/linux/mfd/mt6331/core.h      |  53 +++
>>   include/linux/mfd/mt6331/registers.h | 584 ++++++++++++++++++++++++
>>   include/linux/mfd/mt6332/core.h      |  53 +++
>>   include/linux/mfd/mt6332/registers.h | 642 +++++++++++++++++++++++++++
>>   include/linux/mfd/mt6397/core.h      |   2 +
>>   7 files changed, 1389 insertions(+), 1 deletion(-)
>>   create mode 100644 include/linux/mfd/mt6331/core.h
>>   create mode 100644 include/linux/mfd/mt6331/registers.h
>>   create mode 100644 include/linux/mfd/mt6332/core.h
>>   create mode 100644 include/linux/mfd/mt6332/registers.h
> 
> [...]
> 
>> +#endif /* __MFD_MT6332_REGISTERS_H__ */
>> diff --git a/include/linux/mfd/mt6397/core.h b/include/linux/mfd/mt6397/core.h
>> index 1cf78726503b..4d2b80319db3 100644
>> --- a/include/linux/mfd/mt6397/core.h
>> +++ b/include/linux/mfd/mt6397/core.h
>> @@ -12,6 +12,8 @@
>>   
>>   enum chip_id {
>>   	MT6323_CHIP_ID = 0x23,
> 
>> +	MT6331_CHIP_ID = 0x20,
>> +	MT6332_CHIP_ID = 0x20,
> 
> Are these meant to be the same?
> 

Yes, because this list is referring to the first 8 bits of the CID register,
which is the same for both 6331 and 6332, this is because these two chips are
meant to be used together (at least from my understanding), one as main PMIC
and one as a companion PMIC.

The difference belongs in the higher 8 bits, but there's no support for this
check in the mt6397 driver (yet), and I chose not to implement that to not
complicate this work *at least for now*, as I have no idea about what the
already implemented PMICs read in the high 8 bits (I can check only two of
them, but not the others as I don't have the hardware).

Besides, checking the lower 8 bits is important only because we get knowledge
of the chip *revision* and not the actual model (for which, we use devicetree
anyway).

Regards,
Angelo