Message ID | 20230830115744.4102929-1-Naresh.Solanki@9elements.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [1/2] dt-binding: Add custom property for MAX7357 | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/patch-applied | fail | build log |
Hi On Wed, 30 Aug 2023 at 20:08, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 30/08/2023 13:57, Naresh Solanki wrote: > > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > Add a custom property "maxim,bus-lockup-fix" to enable proprietary > > features on MAX7357. The driver configures MAX7357 to isolate the > > failing channel and trigger a flush-out sequence for bus lock-up > > resolution. > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. Ack > > It is "dt-bindings" not binding and several other fields are needed. Ack > > Also "add custom property" is quite generic. When you add next custom > property you are going to have two commits with the same subject. Just > make it descriptive - "Add foobar for MAX7357" Missed Properties in this Patch Series, Will Be Addressed in V2. So in that case I guess the below title should be fine? dt-bindings: i2c: Add custom properties for MAX7357/MAX7358 > > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > --- > > .../devicetree/bindings/i2c/i2c-mux-pca954x.yaml | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > > index 2d7bb998b0e9..984d4614a270 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > > @@ -71,6 +71,11 @@ properties: > > description: A voltage regulator supplying power to the chip. On PCA9846 > > the regulator supplies power to VDD2 (core logic) and optionally to VDD1. > > > > + maxim,bus-lockup-fix: > > + type: boolean > > + description: Isolates only the stuck channel and generates a flush-out sequence > > + to attempt to clear the bus lock-up. > > Why wouldn't you want it to be enabled all the time? Why should it be > configurable per-board? The chip doesn't enable these features by default & it is left to discretion of board designer to enable the same. Regards, Naresh > > Best regards, > Krzysztof >
On 31/08/2023 11:45, Naresh Solanki wrote: > Hi > > On Wed, 30 Aug 2023 at 20:08, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 30/08/2023 13:57, Naresh Solanki wrote: >>> From: Patrick Rudolph <patrick.rudolph@9elements.com> >>> >>> Add a custom property "maxim,bus-lockup-fix" to enable proprietary >>> features on MAX7357. The driver configures MAX7357 to isolate the >>> failing channel and trigger a flush-out sequence for bus lock-up >>> resolution. >> >> Please use subject prefixes matching the subsystem. You can get them for >> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory >> your patch is touching. > Ack >> >> It is "dt-bindings" not binding and several other fields are needed. > Ack >> >> Also "add custom property" is quite generic. When you add next custom >> property you are going to have two commits with the same subject. Just >> make it descriptive - "Add foobar for MAX7357" > Missed Properties in this Patch Series, Will Be Addressed in V2. > So in that case I guess the below title should be fine? > dt-bindings: i2c: Add custom properties for MAX7357/MAX7358 No, because you do not solve that part of my feedback: >> Also "add custom property" is quite generic. When you add next custom >> property you are going to have two commits with the same subject. I said, everything will be now "add custom properties", right? The prefix is dt-bindings: i2c: pca954x: > >> >> >> >>> >>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> >>> --- >>> .../devicetree/bindings/i2c/i2c-mux-pca954x.yaml | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >>> index 2d7bb998b0e9..984d4614a270 100644 >>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >>> @@ -71,6 +71,11 @@ properties: >>> description: A voltage regulator supplying power to the chip. On PCA9846 >>> the regulator supplies power to VDD2 (core logic) and optionally to VDD1. >>> >>> + maxim,bus-lockup-fix: >>> + type: boolean >>> + description: Isolates only the stuck channel and generates a flush-out sequence >>> + to attempt to clear the bus lock-up. >> >> Why wouldn't you want it to be enabled all the time? Why should it be >> configurable per-board? > The chip doesn't enable these features by default & it is left to > discretion of board designer to enable the same. That does not explain anything. Enable them by default in such case or come with some argument. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml index 2d7bb998b0e9..984d4614a270 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml @@ -71,6 +71,11 @@ properties: description: A voltage regulator supplying power to the chip. On PCA9846 the regulator supplies power to VDD2 (core logic) and optionally to VDD1. + maxim,bus-lockup-fix: + type: boolean + description: Isolates only the stuck channel and generates a flush-out sequence + to attempt to clear the bus lock-up. + required: - compatible - reg @@ -95,6 +100,16 @@ allOf: "#interrupt-cells": false interrupt-controller: false + - if: + not: + properties: + compatible: + contains: + const: maxim,max7357 + then: + properties: + maxim,bus-lockup-fix: false + unevaluatedProperties: false examples: