Re: [PATCH 3/3] soc: mediatek: pwrap: add mt6572 support

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/15/26 13:24, Roman Vivchar wrote:
> Hi AngeloGioacchino,
> 
> On Wednesday, July 15th, 2026 at 1:21 PM, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> On 7/14/26 14:42, Roman Vivchar via B4 Relay wrote:
> 
> ...
>   
>>>
>>> +static const struct pmic_wrapper_type pwrap_mt6572 = {
>>> +	.regs = mt6572_regs,
>>
>> MT6572 only has PWRAP_OP_TYPE, PWRAP_MSB_FIRST, PWRAP_GPS_STA additional registers
>> compared to MT2701, and the three are currently unused.
> 
> GPS_STA is indeed unused. As for others, downstream kernel includes mt6320 path.
> 
> //###############################
> //Enable 6320 option
> //###############################
> #ifdef SLV_6320
> WRAP_WR32(PMIC_WRAP_OP_TYPE ,OP_TYPE_CSL);
> WRAP_WR32(PMIC_WRAP_MSB_FIRST , LSB);
> #endif
> 
> Though I haven't seen any mt6572 device with the mt6320 so far, as well as
> pwrap_init_chip_select_ext is configured for the mt6323.
> 
>> Instead of adding yet one more huge array for 3 more registers, since I'm mostly
>> sure that those will never be used anyway (happy if you prove me wrong!), you can
>> at this point just use mt2701_regs for mt6572.
>>
>> Perhaps, add a comment saying that MT6572 does indeed have those additional regs
>> but were omitted because currently unused.
>>
>> So, something like
>>
>> /*
>>    * MT6572 has additional registers OP_TYPE (0x10), MSB_FIRST (0x14), GPS_STA (0x40)
>>    * which were omitted as they're currently unused in this driver.
>>    * Apart from that, the register map matches the one from MT2701.
>>    */
>> static const struct pmic_wrapper_type pwrap_mt6572 = {
>> 	.regs = mt2701_regs,
>>
> 
> I'm not a big fan of dead code, but at some point the mt6572+mt6320 may exist.
> Would you suggest keeping mt6572 array or drop it until some mt6320
> device appears?
> 

Nah, please, drop it until some MT6320 device appears.

Besides, you also said that you haven't seen any MT6572 w/6320 so far, so it's
even more unlikely that we'll ever see one.

While at it, could you please also add the information about MT6320 cases in
the commit description?
Something like "blahblah additional registers blahblah omitted, those seem to
be used only when the MT6572 is paired with a MT6320 PMIC blahblah" :-)

Cheers,
Angelo