[coreboot-gerrit] [M] Change in coreboot[main]: soc/mediatek/mt8196: Add PMIC MT6363 ADC driver

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin.

Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85128?usp=email )

Change subject: soc/mediatek/mt8196: Add PMIC MT6363 ADC driver
......................................................................

Patch Set 14:

(4 comments)

File src/soc/mediatek/common/mt6363_sdmadc.c:

https://review.coreboot.org/c/coreboot/+/85128/comment/b2741a3d_810e9043?usp... :
PS4, Line 61: static struct pmif *pmif_arb = NULL;
            : 
            : static u32 mt6363_auxadc_read_value(u32 reg)
            : {
            : 	u32 data;
            : 
            : 	assert(pmif_arb);
            : 	pmif_arb->read(pmif_arb, SPMI_SLAVE_4, reg, &data);
            : 	return data;
            : }
            : 
            : static void mt6363_auxadc_write_value(u32 reg, u8 reg_val)
            : {
            : 	assert(pmif_arb);
            : 	pmif_arb->write(pmif_arb, SPMI_SLAVE_4, reg, reg_val);
            : }
            : 
            : static u32 mt6363_auxadc_read_value16(u32 reg)
            : {
            : 	u16 rdata = 0;
            : 
            : 	assert(pmif_arb);
            : 	pmif_arb->read16(pmif_arb, SPMI_SLAVE_4, reg, &rdata);
            : 	return rdata;
            : }
> The APIs for adc code, adc code and 6363 code has been split into two patches.

If you remove `mt6363_sdmadc_init` from this file, then these functions must be moved to mt6363.c as well. Otherwise, `pmif_arb` will also be NULL.

I agree with Yidi that these should be moved to mt6363.c. As these functions are similar to the read/write functions in common/mt6363.c, I'd prefer adding them in CB:85127. You'll need to add them to mt6363.h as well.

```
u8 mt6363_read8(u32 reg);  // this is mt6363_auxadc_read_value
u16 mt6363_read16(u32 reg);  // this is mt6363_auxadc_read_value16
void mt6363_write8(u32 reg, u8 data);  // this is mt6363_auxadc_write_value
```

File src/soc/mediatek/common/mt6363_sdmadc.c:

https://review.coreboot.org/c/coreboot/+/85128/comment/c4bbd3d6_8a4e0315?usp... :
PS14, Line 46:  
remove

https://review.coreboot.org/c/coreboot/+/85128/comment/ccd9acd7_db770173?usp... :
PS14, Line 52: 	SDMADC_CHAN_SPEC(1),
`[AUXADC_CHAN_VIN1] = SDMADC_CHAN_SPEC(1),`

https://review.coreboot.org/c/coreboot/+/85128/comment/229fe5ab_71f3bd64?usp... :
PS14, Line 60: 
```
_Static_assert(ARRAY_SIZE(mt6363_sdmadc_chan_specs) == AUXADC_CHAN_MAX),
               "Wrong array size for mt6363_sdmadc_chan_specs");
```