diff mbox series

[3/9] mfd: mt6397: Split MediaTek MT6366 PMIC out of MT6358

Message ID 20230803074249.3065586-4-wenst@chromium.org
State New
Headers
Series regulator: mt6366: Split out of MT6358 and cleanup |

Commit Message

Chen-Yu Tsai Aug. 3, 2023, 7:42 a.m. UTC 
  The MT6366 PMIC is mostly, but not fully, compatible with MT6358. It has
a different set of regulators. Specifically, it lacks the camera related
VCAM* LDOs, but has additional VM18, VMDDR, and VSRAM_CORE LDOs.

Add a separate compatible for the MT6366 PMIC. The regulator cell for
this new entry uses a new compatible string matching MT6366.

Fixes: c47383f84909 ("mfd: Add support for the MediaTek MT6366 PMIC")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/mfd/mt6397-core.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

AngeloGioacchino Del Regno Aug. 3, 2023, 9:01 a.m. UTC | #1 
Il 03/08/23 09:42, Chen-Yu Tsai ha scritto:
> The MT6366 PMIC is mostly, but not fully, compatible with MT6358. It has
> a different set of regulators. Specifically, it lacks the camera related
> VCAM* LDOs, but has additional VM18, VMDDR, and VSRAM_CORE LDOs.
> 
> Add a separate compatible for the MT6366 PMIC. The regulator cell for
> this new entry uses a new compatible string matching MT6366.
> 
> Fixes: c47383f84909 ("mfd: Add support for the MediaTek MT6366 PMIC")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

I agree in that the LDOs are a bit different, but that's handled by the
mt6358-regulator driver regardless of the actual devicetree compatible,
as that's selected through a chip_id check.

Finally, looking at the driver implementation itself, the addition of a
specific mt6366 compatible here seems redundant, because the actual HW is
  - Handled by drivers, but
  - Described by bindings

Any other opinions on this?

Regards,
Angelo

> ---
>   drivers/mfd/mt6397-core.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index f6c1f80f94a4..3f8dfe60a59b 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -206,6 +206,26 @@ static const struct mfd_cell mt6359_devs[] = {
>   	},
>   };
>   
> +static const struct mfd_cell mt6366_devs[] = {
> +	{
> +		.name = "mt6358-regulator",
> +		.of_compatible = "mediatek,mt6366-regulator"
> +	}, {
> +		.name = "mt6358-rtc",
> +		.num_resources = ARRAY_SIZE(mt6358_rtc_resources),
> +		.resources = mt6358_rtc_resources,
> +		.of_compatible = "mediatek,mt6358-rtc",
> +	}, {
> +		.name = "mt6358-sound",
> +		.of_compatible = "mediatek,mt6358-sound"
> +	}, {
> +		.name = "mt6358-keys",
> +		.num_resources = ARRAY_SIZE(mt6358_keys_resources),
> +		.resources = mt6358_keys_resources,
> +		.of_compatible = "mediatek,mt6358-keys"
> +	},
> +};
> +
>   static const struct mfd_cell mt6397_devs[] = {
>   	{
>   		.name = "mt6397-rtc",
> @@ -280,6 +300,14 @@ static const struct chip_data mt6359_core = {
>   	.irq_init = mt6358_irq_init,
>   };
>   
> +static const struct chip_data mt6366_core = {
> +	.cid_addr = MT6358_SWCID,
> +	.cid_shift = 8,
> +	.cells = mt6366_devs,
> +	.cell_size = ARRAY_SIZE(mt6366_devs),
> +	.irq_init = mt6358_irq_init,
> +};
> +
>   static const struct chip_data mt6397_core = {
>   	.cid_addr = MT6397_CID,
>   	.cid_shift = 0,
> @@ -358,6 +386,9 @@ static const struct of_device_id mt6397_of_match[] = {
>   	}, {
>   		.compatible = "mediatek,mt6359",
>   		.data = &mt6359_core,
> +	}, {
> +		.compatible = "mediatek,mt6366",
> +		.data = &mt6366_core,
>   	}, {
>   		.compatible = "mediatek,mt6397",
>   		.data = &mt6397_core,
Chen-Yu Tsai Aug. 4, 2023, 3:47 a.m. UTC | #2 
On Thu, Aug 3, 2023 at 5:01 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 03/08/23 09:42, Chen-Yu Tsai ha scritto:
> > The MT6366 PMIC is mostly, but not fully, compatible with MT6358. It has
> > a different set of regulators. Specifically, it lacks the camera related
> > VCAM* LDOs, but has additional VM18, VMDDR, and VSRAM_CORE LDOs.
> >
> > Add a separate compatible for the MT6366 PMIC. The regulator cell for
> > this new entry uses a new compatible string matching MT6366.
> >
> > Fixes: c47383f84909 ("mfd: Add support for the MediaTek MT6366 PMIC")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>
> I agree in that the LDOs are a bit different, but that's handled by the
> mt6358-regulator driver regardless of the actual devicetree compatible,
> as that's selected through a chip_id check.
>
> Finally, looking at the driver implementation itself, the addition of a
> specific mt6366 compatible here seems redundant, because the actual HW is
>   - Handled by drivers, but
>   - Described by bindings
>
> Any other opinions on this?

Well, on the bindings side, we can't have MT6366 fall back to MT6358,
neither for the whole PMIC nor just for the regulators. For the latter
it's because neither is a subset of the other, which a) makes them not
fallback compatible as required by the spirit of fallback compatibles,
and b) cannot be described with a fallback compatible, as the fallback
one will have properties/nodes that are not valid for the other, and
vice versa.

Without a fallback compatible to lean in for the regulator driver, we
will need to split out the compatible at the mfd level as well. AFAIU
the mfd core matches mfd-cells based on the compatible strings it is
given in the driver.

ChenYu

> Regards,
> Angelo
>
> > ---
> >   drivers/mfd/mt6397-core.c | 31 +++++++++++++++++++++++++++++++
> >   1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> > index f6c1f80f94a4..3f8dfe60a59b 100644
> > --- a/drivers/mfd/mt6397-core.c
> > +++ b/drivers/mfd/mt6397-core.c
> > @@ -206,6 +206,26 @@ static const struct mfd_cell mt6359_devs[] = {
> >       },
> >   };
> >
> > +static const struct mfd_cell mt6366_devs[] = {
> > +     {
> > +             .name = "mt6358-regulator",
> > +             .of_compatible = "mediatek,mt6366-regulator"
> > +     }, {
> > +             .name = "mt6358-rtc",
> > +             .num_resources = ARRAY_SIZE(mt6358_rtc_resources),
> > +             .resources = mt6358_rtc_resources,
> > +             .of_compatible = "mediatek,mt6358-rtc",
> > +     }, {
> > +             .name = "mt6358-sound",
> > +             .of_compatible = "mediatek,mt6358-sound"
> > +     }, {
> > +             .name = "mt6358-keys",
> > +             .num_resources = ARRAY_SIZE(mt6358_keys_resources),
> > +             .resources = mt6358_keys_resources,
> > +             .of_compatible = "mediatek,mt6358-keys"
> > +     },
> > +};
> > +
> >   static const struct mfd_cell mt6397_devs[] = {
> >       {
> >               .name = "mt6397-rtc",
> > @@ -280,6 +300,14 @@ static const struct chip_data mt6359_core = {
> >       .irq_init = mt6358_irq_init,
> >   };
> >
> > +static const struct chip_data mt6366_core = {
> > +     .cid_addr = MT6358_SWCID,
> > +     .cid_shift = 8,
> > +     .cells = mt6366_devs,
> > +     .cell_size = ARRAY_SIZE(mt6366_devs),
> > +     .irq_init = mt6358_irq_init,
> > +};
> > +
> >   static const struct chip_data mt6397_core = {
> >       .cid_addr = MT6397_CID,
> >       .cid_shift = 0,
> > @@ -358,6 +386,9 @@ static const struct of_device_id mt6397_of_match[] = {
> >       }, {
> >               .compatible = "mediatek,mt6359",
> >               .data = &mt6359_core,
> > +     }, {
> > +             .compatible = "mediatek,mt6366",
> > +             .data = &mt6366_core,
> >       }, {
> >               .compatible = "mediatek,mt6397",
> >               .data = &mt6397_core,
>
>
AngeloGioacchino Del Regno Aug. 4, 2023, 6:39 a.m. UTC | #3 
Il 04/08/23 05:47, Chen-Yu Tsai ha scritto:
> On Thu, Aug 3, 2023 at 5:01 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 03/08/23 09:42, Chen-Yu Tsai ha scritto:
>>> The MT6366 PMIC is mostly, but not fully, compatible with MT6358. It has
>>> a different set of regulators. Specifically, it lacks the camera related
>>> VCAM* LDOs, but has additional VM18, VMDDR, and VSRAM_CORE LDOs.
>>>
>>> Add a separate compatible for the MT6366 PMIC. The regulator cell for
>>> this new entry uses a new compatible string matching MT6366.
>>>
>>> Fixes: c47383f84909 ("mfd: Add support for the MediaTek MT6366 PMIC")
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>
>> I agree in that the LDOs are a bit different, but that's handled by the
>> mt6358-regulator driver regardless of the actual devicetree compatible,
>> as that's selected through a chip_id check.
>>
>> Finally, looking at the driver implementation itself, the addition of a
>> specific mt6366 compatible here seems redundant, because the actual HW is
>>    - Handled by drivers, but
>>    - Described by bindings
>>
>> Any other opinions on this?
> 
> Well, on the bindings side, we can't have MT6366 fall back to MT6358,
> neither for the whole PMIC nor just for the regulators. For the latter
> it's because neither is a subset of the other, which a) makes them not
> fallback compatible as required by the spirit of fallback compatibles,
> and b) cannot be described with a fallback compatible, as the fallback
> one will have properties/nodes that are not valid for the other, and
> vice versa.
> 
> Without a fallback compatible to lean in for the regulator driver, we
> will need to split out the compatible at the mfd level as well. AFAIU
> the mfd core matches mfd-cells based on the compatible strings it is
> given in the driver.
> 

Hmm... you might actually be right on this.
But! I just want to be sure that we're doing things the right way.. and
I'd like to get an opinion from a bindings person, as I think that's the
most appropriate thing that can be done.

Krzysztof, please, can you check this one?

Thanks!
Angelo

> ChenYu
> 
>> Regards,
>> Angelo
>>
>>> ---
>>>    drivers/mfd/mt6397-core.c | 31 +++++++++++++++++++++++++++++++
>>>    1 file changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
>>> index f6c1f80f94a4..3f8dfe60a59b 100644
>>> --- a/drivers/mfd/mt6397-core.c
>>> +++ b/drivers/mfd/mt6397-core.c
>>> @@ -206,6 +206,26 @@ static const struct mfd_cell mt6359_devs[] = {
>>>        },
>>>    };
>>>
>>> +static const struct mfd_cell mt6366_devs[] = {
>>> +     {
>>> +             .name = "mt6358-regulator",
>>> +             .of_compatible = "mediatek,mt6366-regulator"
>>> +     }, {
>>> +             .name = "mt6358-rtc",
>>> +             .num_resources = ARRAY_SIZE(mt6358_rtc_resources),
>>> +             .resources = mt6358_rtc_resources,
>>> +             .of_compatible = "mediatek,mt6358-rtc",
>>> +     }, {
>>> +             .name = "mt6358-sound",
>>> +             .of_compatible = "mediatek,mt6358-sound"
>>> +     }, {
>>> +             .name = "mt6358-keys",
>>> +             .num_resources = ARRAY_SIZE(mt6358_keys_resources),
>>> +             .resources = mt6358_keys_resources,
>>> +             .of_compatible = "mediatek,mt6358-keys"
>>> +     },
>>> +};
>>> +
>>>    static const struct mfd_cell mt6397_devs[] = {
>>>        {
>>>                .name = "mt6397-rtc",
>>> @@ -280,6 +300,14 @@ static const struct chip_data mt6359_core = {
>>>        .irq_init = mt6358_irq_init,
>>>    };
>>>
>>> +static const struct chip_data mt6366_core = {
>>> +     .cid_addr = MT6358_SWCID,
>>> +     .cid_shift = 8,
>>> +     .cells = mt6366_devs,
>>> +     .cell_size = ARRAY_SIZE(mt6366_devs),
>>> +     .irq_init = mt6358_irq_init,
>>> +};
>>> +
>>>    static const struct chip_data mt6397_core = {
>>>        .cid_addr = MT6397_CID,
>>>        .cid_shift = 0,
>>> @@ -358,6 +386,9 @@ static const struct of_device_id mt6397_of_match[] = {
>>>        }, {
>>>                .compatible = "mediatek,mt6359",
>>>                .data = &mt6359_core,
>>> +     }, {
>>> +             .compatible = "mediatek,mt6366",
>>> +             .data = &mt6366_core,
>>>        }, {
>>>                .compatible = "mediatek,mt6397",
>>>                .data = &mt6397_core,
>>
>>
diff mbox series

Patch

diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index f6c1f80f94a4..3f8dfe60a59b 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -206,6 +206,26 @@  static const struct mfd_cell mt6359_devs[] = {
 	},
 };
 
+static const struct mfd_cell mt6366_devs[] = {
+	{
+		.name = "mt6358-regulator",
+		.of_compatible = "mediatek,mt6366-regulator"
+	}, {
+		.name = "mt6358-rtc",
+		.num_resources = ARRAY_SIZE(mt6358_rtc_resources),
+		.resources = mt6358_rtc_resources,
+		.of_compatible = "mediatek,mt6358-rtc",
+	}, {
+		.name = "mt6358-sound",
+		.of_compatible = "mediatek,mt6358-sound"
+	}, {
+		.name = "mt6358-keys",
+		.num_resources = ARRAY_SIZE(mt6358_keys_resources),
+		.resources = mt6358_keys_resources,
+		.of_compatible = "mediatek,mt6358-keys"
+	},
+};
+
 static const struct mfd_cell mt6397_devs[] = {
 	{
 		.name = "mt6397-rtc",
@@ -280,6 +300,14 @@  static const struct chip_data mt6359_core = {
 	.irq_init = mt6358_irq_init,
 };
 
+static const struct chip_data mt6366_core = {
+	.cid_addr = MT6358_SWCID,
+	.cid_shift = 8,
+	.cells = mt6366_devs,
+	.cell_size = ARRAY_SIZE(mt6366_devs),
+	.irq_init = mt6358_irq_init,
+};
+
 static const struct chip_data mt6397_core = {
 	.cid_addr = MT6397_CID,
 	.cid_shift = 0,
@@ -358,6 +386,9 @@  static const struct of_device_id mt6397_of_match[] = {
 	}, {
 		.compatible = "mediatek,mt6359",
 		.data = &mt6359_core,
+	}, {
+		.compatible = "mediatek,mt6366",
+		.data = &mt6366_core,
 	}, {
 		.compatible = "mediatek,mt6397",
 		.data = &mt6397_core,