Messages in this thread | | | Date | Fri, 4 Aug 2023 08:39:19 +0200 | Subject | Re: [PATCH 3/9] mfd: mt6397: Split MediaTek MT6366 PMIC out of MT6358 | From | AngeloGioacchino Del Regno <> |
| |
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, >> >>
| |