| Message ID | 20230718062639.2339589-2-quic_fenglinw@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support | expand |
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(+)