| Message ID | 20230718062639.2339589-1-quic_fenglinw@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for vibrator in multiple PMICs | 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 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 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 >