Hi
On Wed, 30 Aug 2023 at 20:08, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 30/08/2023 13:57, Naresh Solanki wrote:
> > From: Patrick Rudolph <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Naresh Solanki <[email protected]>
> > ---
> > .../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
> <[email protected]> wrote:
>>
>> On 30/08/2023 13:57, Naresh Solanki wrote:
>>> From: Patrick Rudolph <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Naresh Solanki <[email protected]>
>>> ---
>>> .../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