| Message ID | 20230803074249.3065586-4-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | regulator: mt6366: Split out of MT6358 and cleanup | expand |
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,
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, > >
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 --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,
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(+)