On Fri, Jun 09, 2023 at 04:56:05PM +0100, Conor Dooley wrote:
> On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> > 
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> > 
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> > 
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/regulator/mt6358-regulator.c       | 65 +++++++++++++++++-----
> >  include/linux/regulator/mt6358-regulator.h |  6 +-
> >  2 files changed, 52 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> >  	2800000, 2900000, 3000000,
> >  };
> >  
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> >  	3300000, 3400000, 3500000,
> >  };
> >  
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> >  	0, 7, 9, 10, 11, 12,
> >  };
> >  
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> >  	1, 2, 3,
> >  };
> >  
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> >  		   MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> >  	MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> >  		   MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > -	MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > -		   vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > -		   0, MT6358_VCN33_ANA_CON0, 0x300),
> > -	MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > -		   vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > -		   0, MT6358_VCN33_ANA_CON0, 0x300),
> > +	MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > +		   MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> 
> Excuse me if I am being daft here, but could you explain how this change
> is compatible with existing devicetrees?

Ah, I see in the binding commit there's a "Luckily no device tree actually
uses them." Does that just cover the kernel, or does it consider other
operating systems/bootloaders?

Cheers,
Conor.