| Message ID | 20230718062639.2339589-2-quic_fenglinw@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support | expand |
On 18/07/2023 08:26, Fenglin Wu wrote: > Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B > PMICs. > > Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> > --- I don't see changelog. No changes then? > Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml > index c8832cd0d7da..481163105d24 100644 > --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml > +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml > @@ -15,6 +15,9 @@ properties: > - qcom,pm8058-vib > - qcom,pm8916-vib > - qcom,pm8921-vib > + - qcom,pmi632-vib > + - qcom,pm7250b-vib > + - qcom,pm7325b-vib Not much improved. With missing changelog, it seems you ignored the feedback. Best regards, Krzysztof
On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote: > On 18/07/2023 08:26, Fenglin Wu wrote: >> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B >> PMICs. >> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >> --- > > I don't see changelog. No changes then? > Sorry, I updated the change log in the cover letter which didn't seems to be sent to a wider audience, I will resend it by adding more receivers in the to list Fenglin >> Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >> index c8832cd0d7da..481163105d24 100644 >> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >> @@ -15,6 +15,9 @@ properties: >> - qcom,pm8058-vib >> - qcom,pm8916-vib >> - qcom,pm8921-vib >> + - qcom,pmi632-vib >> + - qcom,pm7250b-vib >> + - qcom,pm7325b-vib > > Not much improved. With missing changelog, it seems you ignored the > feedback. > > > Best regards, > Krzysztof >
On 7/18/2023 2:38 PM, Fenglin Wu wrote: > > > On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote: >> On 18/07/2023 08:26, Fenglin Wu wrote: >>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B >>> PMICs. >>> >>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >>> --- >> >> I don't see changelog. No changes then? >> > Sorry, I updated the change log in the cover letter which didn't seems > to be sent to a wider audience, I will resend it by adding more > receivers in the to list > > Fenglin Just FYI,the change log was updated in the cover letter here: https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f Also the commit text and the driver change were also updated accordingly to address your review comment by removing 'pm7550ba-vib' compatible string. Since the changes are receiving review comments, I will not resend it. I will add a larger to-list when pushing the next patchset. >>> Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >>> b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >>> index c8832cd0d7da..481163105d24 100644 >>> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >>> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml >>> @@ -15,6 +15,9 @@ properties: >>> - qcom,pm8058-vib >>> - qcom,pm8916-vib >>> - qcom,pm8921-vib >>> + - qcom,pmi632-vib >>> + - qcom,pm7250b-vib >>> + - qcom,pm7325b-vib >> >> Not much improved. With missing changelog, it seems you ignored the >> feedback. >> >> >> Best regards, >> Krzysztof >>
On 18/07/2023 09:06, Fenglin Wu wrote: > > > On 7/18/2023 2:38 PM, Fenglin Wu wrote: >> >> >> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote: >>> On 18/07/2023 08:26, Fenglin Wu wrote: >>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B >>>> PMICs. >>>> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >>>> --- >>> >>> I don't see changelog. No changes then? >>> >> Sorry, I updated the change log in the cover letter which didn't seems >> to be sent to a wider audience, I will resend it by adding more >> receivers in the to list >> >> Fenglin > > Just FYI,the change log was updated in the cover letter here: > https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f > > Also the commit text and the driver change were also updated accordingly > to address your review comment by removing 'pm7550ba-vib' compatible string. Removing compatible was never my feedback. Did you read: https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 ? Best regards, Krzysztof
On 7/18/2023 3:20 PM, Krzysztof Kozlowski wrote: > On 18/07/2023 09:06, Fenglin Wu wrote: >> >> >> On 7/18/2023 2:38 PM, Fenglin Wu wrote: >>> >>> >>> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote: >>>> On 18/07/2023 08:26, Fenglin Wu wrote: >>>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B >>>>> PMICs. >>>>> >>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >>>>> --- >>>> >>>> I don't see changelog. No changes then? >>>> >>> Sorry, I updated the change log in the cover letter which didn't seems >>> to be sent to a wider audience, I will resend it by adding more >>> receivers in the to list >>> >>> Fenglin >> >> Just FYI,the change log was updated in the cover letter here: >> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f >> >> Also the commit text and the driver change were also updated accordingly >> to address your review comment by removing 'pm7550ba-vib' compatible string. > > Removing compatible was never my feedback. Did you read: > https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 > ? > Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible like this? properties: compatible: - enum: - - qcom,pm8058-vib - - qcom,pm8916-vib - - qcom,pm8921-vib - - qcom,pmi632-vib - - qcom,pm7250b-vib - - qcom,pm7325b-vib + oneOf: + - enum: + - qcom,pm8058-vib + - qcom,pm8916-vib + - qcom,pm8921-vib + - qcom,pmi632-vib + - qcom,pm7250b-vib + - qcom,pm7325b-vib + - items: + - enum: + - qcom,pm7550ba-vib + - const: qcom,pm7325b-vib > Best regards, > Krzysztof >
On 18/07/2023 09:59, Fenglin Wu wrote: >>> Just FYI,the change log was updated in the cover letter here: >>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f >>> >>> Also the commit text and the driver change were also updated accordingly >>> to address your review comment by removing 'pm7550ba-vib' compatible string. >> >> Removing compatible was never my feedback. Did you read: >> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 >> ? >> > Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible > like this? > > properties: > compatible: > - enum: > - - qcom,pm8058-vib > - - qcom,pm8916-vib > - - qcom,pm8921-vib > - - qcom,pmi632-vib > - - qcom,pm7250b-vib > - - qcom,pm7325b-vib > + oneOf: > + - enum: > + - qcom,pm8058-vib > + - qcom,pm8916-vib > + - qcom,pm8921-vib > + - qcom,pmi632-vib > + - qcom,pm7250b-vib > + - qcom,pm7325b-vib > + - items: > + - enum: > + - qcom,pm7550ba-vib > + - const: qcom,pm7325b-vib > Yes Best regards, Krzysztof
On 18.07.2023 08:38, Fenglin Wu wrote: > > > On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote: >> On 18/07/2023 08:26, Fenglin Wu wrote: >>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B >>> PMICs. >>> >>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >>> --- >> >> I don't see changelog. No changes then? >> > Sorry, I updated the change log in the cover letter which didn't seems to be sent to a wider audience, I will resend it by adding more receivers in the to list Please consider using the b4 tool which takes care of all that https://b4.docs.kernel.org/en/latest/index.html Konrad
On 7/18/2023 6:51 PM, Konrad Dybcio wrote: > On 18.07.2023 08:38, Fenglin Wu wrote: >> >> >> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote: >>> On 18/07/2023 08:26, Fenglin Wu wrote: >>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B >>>> PMICs. >>>> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >>>> --- >>> >>> I don't see changelog. No changes then? >>> >> Sorry, I updated the change log in the cover letter which didn't seems to be sent to a wider audience, I will resend it by adding more receivers in the to list > Please consider using the b4 tool which takes care of all that > > https://b4.docs.kernel.org/en/latest/index.html > Thanks Konrad, I will check and update at my side. > Konrad
On 18/07/2023 10:02, Krzysztof Kozlowski wrote: > On 18/07/2023 09:59, Fenglin Wu wrote: > >>>> Just FYI,the change log was updated in the cover letter here: >>>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f >>>> >>>> Also the commit text and the driver change were also updated accordingly >>>> to address your review comment by removing 'pm7550ba-vib' compatible string. >>> >>> Removing compatible was never my feedback. Did you read: >>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 >>> ? >>> >> Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible >> like this? >> >> properties: >> compatible: >> - enum: >> - - qcom,pm8058-vib >> - - qcom,pm8916-vib >> - - qcom,pm8921-vib >> - - qcom,pmi632-vib >> - - qcom,pm7250b-vib >> - - qcom,pm7325b-vib >> + oneOf: >> + - enum: >> + - qcom,pm8058-vib >> + - qcom,pm8916-vib >> + - qcom,pm8921-vib >> + - qcom,pmi632-vib >> + - qcom,pm7250b-vib >> + - qcom,pm7325b-vib >> + - items: >> + - enum: >> + - qcom,pm7550ba-vib >> + - const: qcom,pm7325b-vib >> > > Yes I wonder why this approved change turned out to something incorrect in your v3 patch... Best regards, Krzysztof
On 7/27/2023 3:10 PM, Krzysztof Kozlowski wrote: > On 18/07/2023 10:02, Krzysztof Kozlowski wrote: >> On 18/07/2023 09:59, Fenglin Wu wrote: >> >>>>> Just FYI,the change log was updated in the cover letter here: >>>>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f >>>>> >>>>> Also the commit text and the driver change were also updated accordingly >>>>> to address your review comment by removing 'pm7550ba-vib' compatible string. >>>> >>>> Removing compatible was never my feedback. Did you read: >>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 >>>> ? >>>> >>> Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible >>> like this? >>> >>> properties: >>> compatible: >>> - enum: >>> - - qcom,pm8058-vib >>> - - qcom,pm8916-vib >>> - - qcom,pm8921-vib >>> - - qcom,pmi632-vib >>> - - qcom,pm7250b-vib >>> - - qcom,pm7325b-vib >>> + oneOf: >>> + - enum: >>> + - qcom,pm8058-vib >>> + - qcom,pm8916-vib >>> + - qcom,pm8921-vib >>> + - qcom,pmi632-vib >>> + - qcom,pm7250b-vib >>> + - qcom,pm7325b-vib >>> + - items: >>> + - enum: >>> + - qcom,pm7550ba-vib >>> + - const: qcom,pm7325b-vib >>> >> >> Yes > > I wonder why this approved change turned out to something incorrect in > your v3 patch... > Since I got review comments in the driver change and I was told to refactor the driver before adding new HW support. I added the HW type logic in the driver and I was thinking it might be good to add some generic compatible strings to match with the HW type introduced in the driver change. Anyway, I will update it to what you suggested in next patch. > Best regards, > Krzysztof >
On 27/07/2023 09:54, Fenglin Wu wrote: >>>> + - enum: >>>> + - qcom,pm7550ba-vib >>>> + - const: qcom,pm7325b-vib >>>> >>> >>> Yes >> >> I wonder why this approved change turned out to something incorrect in >> your v3 patch... >> > Since I got review comments in the driver change and I was told to > refactor the driver before adding new HW support. I added the HW type > logic in the driver and I was thinking it might be good to add some > generic compatible strings to match with the HW type introduced in the > driver change. > > Anyway, I will update it to what you suggested in next patch. Drivers are not really related to bindings, so whatever HW type you add in driver, is not a reason to change bindings. Reason to change bindings could be for example: because hardware is like that. Best regards, Krzysztof
On 7/27/2023 4:29 PM, Krzysztof Kozlowski wrote: > On 27/07/2023 09:54, Fenglin Wu wrote: >>>>> + - enum: >>>>> + - qcom,pm7550ba-vib >>>>> + - const: qcom,pm7325b-vib >>>>> >>>> >>>> Yes >>> >>> I wonder why this approved change turned out to something incorrect in >>> your v3 patch... >>> >> Since I got review comments in the driver change and I was told to >> refactor the driver before adding new HW support. I added the HW type >> logic in the driver and I was thinking it might be good to add some >> generic compatible strings to match with the HW type introduced in the >> driver change. >> >> Anyway, I will update it to what you suggested in next patch. > > Drivers are not really related to bindings, so whatever HW type you add > in driver, is not a reason to change bindings. Reason to change bindings > could be for example: because hardware is like that. > Understood. Sorry, I forgot to mention, in v3, I added the 'reg' value to the register offset and no longer hard code the 16-bit register address, that makes the vibrators inside PMI632/PM7250B/PM7325B/PM7550BA all compatible, and that was another motivation of adding a generic compatible string and make the others as the fallback. This will be still the case in v4, I might keep it similar in v3 but just drop "qcom,spmi-vib-gen1" > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml index c8832cd0d7da..481163105d24 100644 --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml @@ -15,6 +15,9 @@ properties: - qcom,pm8058-vib - qcom,pm8916-vib - qcom,pm8921-vib + - qcom,pmi632-vib + - qcom,pm7250b-vib + - qcom,pm7325b-vib reg: maxItems: 1
Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B PMICs. Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> --- Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 3 +++ 1 file changed, 3 insertions(+)