| Message ID | 20240119094105.98312-1-angelogioacchino.delregno@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt-bindings: usb: mt6360-tcpc: Drop interrupt-names | expand |
On 24/01/2024 09:48, AngeloGioacchino Del Regno wrote: > Il 23/01/24 18:14, Conor Dooley ha scritto: >> On Mon, Jan 22, 2024 at 11:32:30AM +0100, AngeloGioacchino Del Regno wrote: >>> Il 19/01/24 17:32, Conor Dooley ha scritto: >>>> On Fri, Jan 19, 2024 at 10:41:04AM +0100, AngeloGioacchino Del Regno wrote: >>>>> This IP has only one interrupt, hence interrupt-names is not necessary >>>>> to have. >>>>> Since there is no user yet, simply remove interrupt-names. >>>> >>>> I'm a bit confused chief. Patch 2 in this series removes a user of this >>>> property from a driver, so can you explain how this statement is true? >>>> >>>> Maybe I need to drink a few cans of Monster and revisit this patchset? >>>> >>> >>> What I mean with "there is no user" is that there's no device tree with any >>> mt6360-tcpc node upstream yet, so there is no meaningful ABI breakage. >>> Different story would be if there was a device tree using this already, in >>> which case, you can make a required property optional but not remove it. >> >> Not every devicetree lives within the kernel.. If the driver is using >> it, I'm not inclined to agree that it should be removed. > > I get the point, but as far as I remember, it's not the first time that this > kind of change is upstreamed. > > I'm fine with keeping things as they are but, since my intention is to actually > introduce an actual user of this binding upstream, and that actually depends on > if this change is accepted or not (as I have to know whether I can omit adding > the interrupt-names property or not).... > > ....may I ask for more feedback/opinions from Rob and/or Krzk? Driver is the user and this is an old binding (released!), thus there can be out-of-kernel users already. Minor cleanup is not really a reason to affect ABI. You could deprecate it, though. Driver change is fine. Best regards, Krzysztof
On Thu, Jan 25, 2024 at 12:41:57PM +0100, AngeloGioacchino Del Regno wrote: > Il 25/01/24 11:32, Krzysztof Kozlowski ha scritto: > > On 24/01/2024 09:48, AngeloGioacchino Del Regno wrote: > > > Il 23/01/24 18:14, Conor Dooley ha scritto: > > > > On Mon, Jan 22, 2024 at 11:32:30AM +0100, AngeloGioacchino Del Regno wrote: > > > > > Il 19/01/24 17:32, Conor Dooley ha scritto: > > > > > > On Fri, Jan 19, 2024 at 10:41:04AM +0100, AngeloGioacchino Del Regno wrote: > > > > > > > This IP has only one interrupt, hence interrupt-names is not necessary > > > > > > > to have. > > > > > > > Since there is no user yet, simply remove interrupt-names. > > > > > > > > > > > > I'm a bit confused chief. Patch 2 in this series removes a user of this > > > > > > property from a driver, so can you explain how this statement is true? > > > > > > > > > > > > Maybe I need to drink a few cans of Monster and revisit this patchset? > > > > > > > > > > > > > > > > What I mean with "there is no user" is that there's no device tree with any > > > > > mt6360-tcpc node upstream yet, so there is no meaningful ABI breakage. > > > > > Different story would be if there was a device tree using this already, in > > > > > which case, you can make a required property optional but not remove it. > > > > > > > > Not every devicetree lives within the kernel.. If the driver is using > > > > it, I'm not inclined to agree that it should be removed. > > > > > > I get the point, but as far as I remember, it's not the first time that this > > > kind of change is upstreamed. > > > > > > I'm fine with keeping things as they are but, since my intention is to actually > > > introduce an actual user of this binding upstream, and that actually depends on > > > if this change is accepted or not (as I have to know whether I can omit adding > > > the interrupt-names property or not).... > > > > > > ....may I ask for more feedback/opinions from Rob and/or Krzk? > > > > Driver is the user and this is an old binding (released!), thus there > > can be out-of-kernel users already. > > > > Minor cleanup is not really a reason to affect ABI. You could deprecate > > it, though. Driver change is fine. > > > > Thanks for the clarification. If USB maintainers want to take the driver part only > without me resending this, I'd appreciate that. > > The interrupt-names is not a required property in this binding anyway... :-) Having -names properties that are not required when the base property is always seem so pointless to me, except in cases where they're not required for the case where there's one item but required when there are more than one. Ultimately they're pointless if not required since they can't be relied on. I think dropping it from the driver is required for correctness.
Il 25/01/24 18:02, Conor Dooley ha scritto: > On Thu, Jan 25, 2024 at 04:57:33PM +0000, Conor Dooley wrote: >> On Thu, Jan 25, 2024 at 12:41:57PM +0100, AngeloGioacchino Del Regno wrote: >>> Il 25/01/24 11:32, Krzysztof Kozlowski ha scritto: >>>> On 24/01/2024 09:48, AngeloGioacchino Del Regno wrote: >>>>> Il 23/01/24 18:14, Conor Dooley ha scritto: >>>>>> On Mon, Jan 22, 2024 at 11:32:30AM +0100, AngeloGioacchino Del Regno wrote: >>>>>>> Il 19/01/24 17:32, Conor Dooley ha scritto: >>>>>>>> On Fri, Jan 19, 2024 at 10:41:04AM +0100, AngeloGioacchino Del Regno wrote: >>>>>>>>> This IP has only one interrupt, hence interrupt-names is not necessary >>>>>>>>> to have. >>>>>>>>> Since there is no user yet, simply remove interrupt-names. >>>>>>>> >>>>>>>> I'm a bit confused chief. Patch 2 in this series removes a user of this >>>>>>>> property from a driver, so can you explain how this statement is true? >>>>>>>> >>>>>>>> Maybe I need to drink a few cans of Monster and revisit this patchset? >>>>>>>> >>>>>>> >>>>>>> What I mean with "there is no user" is that there's no device tree with any >>>>>>> mt6360-tcpc node upstream yet, so there is no meaningful ABI breakage. >>>>>>> Different story would be if there was a device tree using this already, in >>>>>>> which case, you can make a required property optional but not remove it. >>>>>> >>>>>> Not every devicetree lives within the kernel.. If the driver is using >>>>>> it, I'm not inclined to agree that it should be removed. >>>>> >>>>> I get the point, but as far as I remember, it's not the first time that this >>>>> kind of change is upstreamed. >>>>> >>>>> I'm fine with keeping things as they are but, since my intention is to actually >>>>> introduce an actual user of this binding upstream, and that actually depends on >>>>> if this change is accepted or not (as I have to know whether I can omit adding >>>>> the interrupt-names property or not).... >>>>> >>>>> ....may I ask for more feedback/opinions from Rob and/or Krzk? >>>> >>>> Driver is the user and this is an old binding (released!), thus there >>>> can be out-of-kernel users already. >>>> >>>> Minor cleanup is not really a reason to affect ABI. You could deprecate >>>> it, though. Driver change is fine. >>>> >>> >>> Thanks for the clarification. If USB maintainers want to take the driver part only >>> without me resending this, I'd appreciate that. >>> >> >>> The interrupt-names is not a required property in this binding anyway... :-) >> >> Having -names properties that are not required when the base property is >> always seem so pointless to me, except in cases where they're not >> required for the case where there's one item but required when there are >> more than one. Ultimately they're pointless if not required since they >> can't be relied on. I think dropping it from the driver is required for >> correctness. > > Actually, looking at the binding again: > > | required: > | - compatible > | - interrupts > | - interrupt-names > > It looks like it is a required property after all! Apparently my brain's binding had required: - blindness :-P Yeah, I have no idea why I didn't see that, sorry! Cheers, Angelo
On Fri, Jan 26, 2024 at 10:15:54AM +0100, AngeloGioacchino Del Regno wrote: > > | required: > > | - compatible > > | - interrupts > > | - interrupt-names > > > > It looks like it is a required property after all! > > Apparently my brain's binding had > > required: > - blindness > > :-P > > Yeah, I have no idea why I didn't see that, sorry! Possibly because your patch never removed it from required in the first place, if you only looked back at that, and not the binding (or Rob's bot's report), I can see how you could miss it.
diff --git a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml index 053264e60583..339bc9c00ac0 100644 --- a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml @@ -22,10 +22,6 @@ properties: interrupts: maxItems: 1 - interrupt-names: - items: - - const: PD_IRQB - connector: type: object $ref: ../connector/usb-connector.yaml# @@ -58,7 +54,6 @@ examples: tcpc { compatible = "mediatek,mt6360-tcpc"; interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>; - interrupt-names = "PD_IRQB"; connector { compatible = "usb-c-connector";
This IP has only one interrupt, hence interrupt-names is not necessary to have. Since there is no user yet, simply remove interrupt-names. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- .../devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 5 ----- 1 file changed, 5 deletions(-)