| Message ID | 20230915152607.18116-4-william-tw.lin@mediatek.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtk-socinfo driver implementation | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 15/09/2023 17:26, William-tw Lin wrote: > dt-binding documentation for mtk-socinfo driver. Here and in subject, drop driver and instead descrbe hardware. > mtk-socinfo driver provides SoC-related information. > Such information includes manufacturer information, SoC name, > SoC segment name, and SoC marketing name. > > Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com> > --- > .../bindings/hwinfo/mtk-socinfo.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml > > diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml > new file mode 100644 > index 000000000000..74f03f1dc404 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml Nothing improved. This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek SoC ChipID > + > +maintainers: > + - William Lin <william-tw.lin@mediatek.com> > + - Matthias Brugger <matthias.bgg@gmail.com> > + - Kevin Hilman <khilman@kernel.org> > + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > + > +description: > + MediaTek SoCs store various product information in eFuses, including > + Chip ID and Revision fields, usable to identify the manufacturer, > + SoC version, plus segment and marketing names. > + > +properties: > + compatible: > + const: mediatek,socinfo What happened to compatibles? No, this is just wrong and no explained. You ignored other comments as well. Really, that's not the way to go. NAK Best regards, Krzysztof
Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto: > On 15/09/2023 17:26, William-tw Lin wrote: >> dt-binding documentation for mtk-socinfo driver. > > Here and in subject, drop driver and instead descrbe hardware. > >> mtk-socinfo driver provides SoC-related information. >> Such information includes manufacturer information, SoC name, >> SoC segment name, and SoC marketing name. >> >> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com> >> --- >> .../bindings/hwinfo/mtk-socinfo.yaml | 48 +++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >> >> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >> new file mode 100644 >> index 000000000000..74f03f1dc404 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml > > Nothing improved. > > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. > > Thank you. > >> @@ -0,0 +1,48 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: MediaTek SoC ChipID >> + >> +maintainers: >> + - William Lin <william-tw.lin@mediatek.com> >> + - Matthias Brugger <matthias.bgg@gmail.com> >> + - Kevin Hilman <khilman@kernel.org> >> + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> + >> +description: >> + MediaTek SoCs store various product information in eFuses, including >> + Chip ID and Revision fields, usable to identify the manufacturer, >> + SoC version, plus segment and marketing names. >> + >> +properties: >> + compatible: >> + const: mediatek,socinfo > > What happened to compatibles? No, this is just wrong and no explained. > You ignored other comments as well. Really, that's not the way to go. > Practically, having different compatibles for each SoC is not needed, as the only thing that changes between SoCs is the eFuse(s) that you read to get the information - and that's all. So ... we either use this driver with devicetree, giving it the right eFuses to read from, or we duplicate the mtk-efuse driver, or we statically assign the eFuses in the driver itself and we set compatibles like "mediatek,mt8195-socinfo" to select that... ideas? Cheers, Angelo > NAK > > Best regards, > Krzysztof >
On 18/09/2023 10:47, AngeloGioacchino Del Regno wrote: > Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto: >> On 15/09/2023 17:26, William-tw Lin wrote: >>> dt-binding documentation for mtk-socinfo driver. >> >> Here and in subject, drop driver and instead descrbe hardware. >> >>> mtk-socinfo driver provides SoC-related information. >>> Such information includes manufacturer information, SoC name, >>> SoC segment name, and SoC marketing name. >>> >>> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com> >>> --- >>> .../bindings/hwinfo/mtk-socinfo.yaml | 48 +++++++++++++++++++ >>> 1 file changed, 48 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >>> new file mode 100644 >>> index 000000000000..74f03f1dc404 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >> >> Nothing improved. >> >> This is a friendly reminder during the review process. >> >> It seems my previous comments were not fully addressed. Maybe my >> feedback got lost between the quotes, maybe you just forgot to apply it. >> Please go back to the previous discussion and either implement all >> requested changes or keep discussing them. >> >> Thank you. >> >>> @@ -0,0 +1,48 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: MediaTek SoC ChipID >>> + >>> +maintainers: >>> + - William Lin <william-tw.lin@mediatek.com> >>> + - Matthias Brugger <matthias.bgg@gmail.com> >>> + - Kevin Hilman <khilman@kernel.org> >>> + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>> + >>> +description: >>> + MediaTek SoCs store various product information in eFuses, including >>> + Chip ID and Revision fields, usable to identify the manufacturer, >>> + SoC version, plus segment and marketing names. >>> + >>> +properties: >>> + compatible: >>> + const: mediatek,socinfo >> >> What happened to compatibles? No, this is just wrong and no explained. >> You ignored other comments as well. Really, that's not the way to go. >> > > Practically, having different compatibles for each SoC is not needed, as > the only thing that changes between SoCs is the eFuse(s) that you read to > get the information - and that's all. And how do you guarantee that no future SoC will have any difference? How can you even predict it? > > So ... we either use this driver with devicetree, giving it the right eFuses I am talking about bindings, no driver. > to read from, or we duplicate the mtk-efuse driver, or we statically assign > the eFuses in the driver itself and we set compatibles like > "mediatek,mt8195-socinfo" to select that... ideas? Device specific compatibles followed by fallback, just like for every other review coming from me. Best regards, Krzysztof
On Mon, Sep 18, 2023 at 10:47:53AM +0200, AngeloGioacchino Del Regno wrote: > Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto: > > On 15/09/2023 17:26, William-tw Lin wrote: > > > dt-binding documentation for mtk-socinfo driver. > > > > Here and in subject, drop driver and instead descrbe hardware. > > > > > mtk-socinfo driver provides SoC-related information. > > > Such information includes manufacturer information, SoC name, > > > SoC segment name, and SoC marketing name. > > > > > > Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com> > > > --- > > > .../bindings/hwinfo/mtk-socinfo.yaml | 48 +++++++++++++++++++ > > > 1 file changed, 48 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml > > > new file mode 100644 > > > index 000000000000..74f03f1dc404 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml > > > > Nothing improved. > > > > This is a friendly reminder during the review process. > > > > It seems my previous comments were not fully addressed. Maybe my > > feedback got lost between the quotes, maybe you just forgot to apply it. > > Please go back to the previous discussion and either implement all > > requested changes or keep discussing them. > > > > Thank you. > > > > > @@ -0,0 +1,48 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: MediaTek SoC ChipID > > > + > > > +maintainers: > > > + - William Lin <william-tw.lin@mediatek.com> > > > + - Matthias Brugger <matthias.bgg@gmail.com> > > > + - Kevin Hilman <khilman@kernel.org> > > > + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > + > > > +description: > > > + MediaTek SoCs store various product information in eFuses, including > > > + Chip ID and Revision fields, usable to identify the manufacturer, > > > + SoC version, plus segment and marketing names. > > > + > > > +properties: > > > + compatible: > > > + const: mediatek,socinfo > > > > What happened to compatibles? No, this is just wrong and no explained. > > You ignored other comments as well. Really, that's not the way to go. > > > > Practically, having different compatibles for each SoC is not needed, as > the only thing that changes between SoCs is the eFuse(s) that you read to > get the information - and that's all. > > So ... we either use this driver with devicetree, giving it the right eFuses > to read from, or we duplicate the mtk-efuse driver, or we statically assign > the eFuses in the driver itself and we set compatibles like > "mediatek,mt8195-socinfo" to select that... ideas? So this one is just a virtual device to instantiate a driver and there is no socinfo hw block? If so, that's definitely a NAK. Either instantiate your socinfo driver from the efuse driver or register as a socinfo provider in it. Rob
Il 18/09/23 13:59, Krzysztof Kozlowski ha scritto: > On 18/09/2023 10:47, AngeloGioacchino Del Regno wrote: >> Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto: >>> On 15/09/2023 17:26, William-tw Lin wrote: >>>> dt-binding documentation for mtk-socinfo driver. >>> >>> Here and in subject, drop driver and instead descrbe hardware. >>> >>>> mtk-socinfo driver provides SoC-related information. >>>> Such information includes manufacturer information, SoC name, >>>> SoC segment name, and SoC marketing name. >>>> >>>> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com> >>>> --- >>>> .../bindings/hwinfo/mtk-socinfo.yaml | 48 +++++++++++++++++++ >>>> 1 file changed, 48 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >>>> new file mode 100644 >>>> index 000000000000..74f03f1dc404 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml >>> >>> Nothing improved. >>> >>> This is a friendly reminder during the review process. >>> >>> It seems my previous comments were not fully addressed. Maybe my >>> feedback got lost between the quotes, maybe you just forgot to apply it. >>> Please go back to the previous discussion and either implement all >>> requested changes or keep discussing them. >>> >>> Thank you. >>> >>>> @@ -0,0 +1,48 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: MediaTek SoC ChipID >>>> + >>>> +maintainers: >>>> + - William Lin <william-tw.lin@mediatek.com> >>>> + - Matthias Brugger <matthias.bgg@gmail.com> >>>> + - Kevin Hilman <khilman@kernel.org> >>>> + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>>> + >>>> +description: >>>> + MediaTek SoCs store various product information in eFuses, including >>>> + Chip ID and Revision fields, usable to identify the manufacturer, >>>> + SoC version, plus segment and marketing names. >>>> + >>>> +properties: >>>> + compatible: >>>> + const: mediatek,socinfo >>> >>> What happened to compatibles? No, this is just wrong and no explained. >>> You ignored other comments as well. Really, that's not the way to go. >>> >> >> Practically, having different compatibles for each SoC is not needed, as >> the only thing that changes between SoCs is the eFuse(s) that you read to >> get the information - and that's all. > > > And how do you guarantee that no future SoC will have any difference? > How can you even predict it? > You're right, it's too much long sighted and at some point in the future this assumption will be *inevitably* wrong. Thanks, Angelo >> >> So ... we either use this driver with devicetree, giving it the right eFuses > > I am talking about bindings, no driver. > >> to read from, or we duplicate the mtk-efuse driver, or we statically assign >> the eFuses in the driver itself and we set compatibles like >> "mediatek,mt8195-socinfo" to select that... ideas? > > Device specific compatibles followed by fallback, just like for every > other review coming from me. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml new file mode 100644 index 000000000000..74f03f1dc404 --- /dev/null +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MediaTek SoC ChipID + +maintainers: + - William Lin <william-tw.lin@mediatek.com> + - Matthias Brugger <matthias.bgg@gmail.com> + - Kevin Hilman <khilman@kernel.org> + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> + +description: + MediaTek SoCs store various product information in eFuses, including + Chip ID and Revision fields, usable to identify the manufacturer, + SoC version, plus segment and marketing names. + +properties: + compatible: + const: mediatek,socinfo + + nvmem-cells: + maxItems: 2 + description: Phandle to nvmem cells containing SoC identification data + + nvmem-cell-names: + minItems: 1 + items: + - const: socinfo-data1 + - const: socinfo-data2 + +required: + - compatible + - nvmem-cells + - nvmem-cell-names + +additionalProperties: false + +examples: + - | + socinfo { + compatible = "mediatek,socinfo"; + nvmem-cells = <&socinfo_data1>, <&socinfo_data2>; + nvmem-cell-names = "socinfo-data1", "socinfo-data2"; + }; +
dt-binding documentation for mtk-socinfo driver. mtk-socinfo driver provides SoC-related information. Such information includes manufacturer information, SoC name, SoC segment name, and SoC marketing name. Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com> --- .../bindings/hwinfo/mtk-socinfo.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml