| Message ID | 20230424105011.70674-1-n-francis@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for ESM | expand |
On 24/04/2023 12:50, Neha Malcom Francis wrote: > Resending as no major changes, commit subject change only. That's a new version. Keep changelog - for some reason it disappeared. > Best regards, Krzysztof
On 4/24/23 03:50, Neha Malcom Francis wrote: > Resending as no major changes, commit subject change only. > Maybe you consider changing the subject of the bindings from "misc" to "hwmon" as not being a major change, but it made me aware that you are trying to sneak bindings which in my opinion don't belong there into the hwmon bindings directory. This is not a hardware monitoring device, it doesn't have anything to do with hardware monitoring, and the bindings do not belong into bindings/hwmon/. Maybe you can convince the devicetree maintainers to accept the bindings into the suggested location, but that will be over my objection. Guenter > ESM (Error Signaling Module) is a fundamental IP responsible for > handling safety events. The driver currently present in U-Boot is > responsible for configuring ESM. This patch series adds dt-binding and > nodes for J721E and J7200. This goes towards end goal of having DTB sync > with that of U-Boot as well as ensuring completeness of hardware > description in devicetree. > > Neha Malcom Francis (3): > dt-bindings: hwmon: esm: Add ESM support for TI K3 devices > arm64: dts: ti: k3-j721e: Add ESM support > arm64: dts: ti: k3-j7200: Add ESM support > > .../bindings/hwmon/ti,j721e-esm.yaml | 53 +++++++++++++++++++ > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 6 +++ > arch/arm64/boot/dts/ti/k3-j7200.dtsi | 1 + > arch/arm64/boot/dts/ti/k3-j721e.dtsi | 1 + > 4 files changed, 61 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/ti,j721e-esm.yaml >
Hi Guenter On 24/04/23 20:27, Guenter Roeck wrote: > On 4/24/23 03:50, Neha Malcom Francis wrote: >> Resending as no major changes, commit subject change only. >> > > Maybe you consider changing the subject of the bindings from "misc" > to "hwmon" as not being a major change, but it made me aware that you > are trying to sneak bindings which in my opinion don't belong there > into the hwmon bindings directory. This is not a hardware monitoring > device, it doesn't have anything to do with hardware monitoring, and the > bindings do not belong into bindings/hwmon/. > I understand, it's a thin line across which I pushed ESM into hwmon; my reasoning was ESM also actively looks for signals that it aggregates, and is overall monitoring the device health. But if there was an option, in order of fitting: fault/ > misc/ > hwmon/ Using misc/ was questioned in an earlier review; and fault/ is not yet created and I did not think there were enough instances to back me up on creating a new dt-bindings dir To come to a common solution, let us keep this binding in misc/ along with other fault detection mechanisms existing and take it up as a follow up action to create a fault/ ? > Maybe you can convince the devicetree maintainers to accept the bindings > into the suggested location, but that will be over my objection. > > Guenter > > >> ESM (Error Signaling Module) is a fundamental IP responsible for >> handling safety events. The driver currently present in U-Boot is >> responsible for configuring ESM. This patch series adds dt-binding and >> nodes for J721E and J7200. This goes towards end goal of having DTB sync >> with that of U-Boot as well as ensuring completeness of hardware >> description in devicetree. >> >> Neha Malcom Francis (3): >> dt-bindings: hwmon: esm: Add ESM support for TI K3 devices >> arm64: dts: ti: k3-j721e: Add ESM support >> arm64: dts: ti: k3-j7200: Add ESM support >> >> .../bindings/hwmon/ti,j721e-esm.yaml | 53 +++++++++++++++++++ >> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 6 +++ >> arch/arm64/boot/dts/ti/k3-j7200.dtsi | 1 + >> arch/arm64/boot/dts/ti/k3-j721e.dtsi | 1 + >> 4 files changed, 61 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/hwmon/ti,j721e-esm.yaml >> >
On 4/25/23 01:49, Neha Malcom Francis wrote: > Hi Guenter > > On 24/04/23 20:27, Guenter Roeck wrote: >> On 4/24/23 03:50, Neha Malcom Francis wrote: >>> Resending as no major changes, commit subject change only. >>> >> >> Maybe you consider changing the subject of the bindings from "misc" >> to "hwmon" as not being a major change, but it made me aware that you >> are trying to sneak bindings which in my opinion don't belong there >> into the hwmon bindings directory. This is not a hardware monitoring >> device, it doesn't have anything to do with hardware monitoring, and the >> bindings do not belong into bindings/hwmon/. >> > > I understand, it's a thin line across which I pushed ESM into hwmon; my reasoning was ESM also actively looks for signals that it aggregates, and is overall monitoring the device health. But if there was an option, in order of fitting: fault/ > misc/ > hwmon/ > That is really a stretch. It doesn't monitor anything. It is a signal routing mechanism. With that logic every transistor would be a hardware monitoring device. Guenter
On 25/04/2023 16:34, Guenter Roeck wrote: > On 4/25/23 01:49, Neha Malcom Francis wrote: >> Hi Guenter >> >> On 24/04/23 20:27, Guenter Roeck wrote: >>> On 4/24/23 03:50, Neha Malcom Francis wrote: >>>> Resending as no major changes, commit subject change only. >>>> >>> >>> Maybe you consider changing the subject of the bindings from "misc" >>> to "hwmon" as not being a major change, but it made me aware that you >>> are trying to sneak bindings which in my opinion don't belong there >>> into the hwmon bindings directory. This is not a hardware monitoring >>> device, it doesn't have anything to do with hardware monitoring, and the >>> bindings do not belong into bindings/hwmon/. >>> >> >> I understand, it's a thin line across which I pushed ESM into hwmon; my reasoning was ESM also actively looks for signals that it aggregates, and is overall monitoring the device health. But if there was an option, in order of fitting: fault/ > misc/ > hwmon/ >> > > That is really a stretch. It doesn't monitor anything. It is a signal > routing mechanism. > > With that logic every transistor would be a hardware monitoring device. Then let's move it to misc/ as I don't have other ideas for the placement. Best regards, Krzysztof