| Message ID | 20220516115846.58328-3-max.oss.09@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: imx6q-apalis: Misc improvements and newly added carrier | expand |
On 16.05.22 13:58, Max Krummenacher wrote: > From: Max Krummenacher <max.krummenacher@toradex.com> > > The STMPE MFD device binding requires the child node to have a fixed > name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen > drivers will not be probed. IMO, the Linux driver should be fixed and the requirement to use a fixed node name be dropped from the binding. The driver itself already probes by compatible, the node name seems only to be used by the MFD driver to detect which functions to enable. It could do the same by checking for compatibles. Otherwise you invite a game of cat and mouse, where in future, this is changed back again reintroducing the regression.. > Fixes: 56086b5e804f ("ARM: dts: imx6qdl-apalis: Avoid underscore in node name") > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> Anyway, as a fix, this looks ok for now: Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Cheers, Ahmad > --- > > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > index bd763bae596b..da919d0544a8 100644 > --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > @@ -315,7 +315,7 @@ > /* ADC conversion time: 80 clocks */ > st,sample-time = <4>; > > - stmpe_touchscreen: stmpe-touchscreen { > + stmpe_touchscreen: stmpe_touchscreen { > compatible = "st,stmpe-ts"; > /* 8 sample average control */ > st,ave-ctrl = <3>; > @@ -332,7 +332,7 @@ > st,touch-det-delay = <5>; > }; > > - stmpe_adc: stmpe-adc { > + stmpe_adc: stmpe_adc { > compatible = "st,stmpe-adc"; > /* forbid to use ADC channels 3-0 (touch) */ > st,norequest-mask = <0x0F>;
On Mon, May 16, 2022 at 02:49:12PM +0200, Ahmad Fatoum wrote: > On 16.05.22 13:58, Max Krummenacher wrote: > > From: Max Krummenacher <max.krummenacher@toradex.com> > > > > The STMPE MFD device binding requires the child node to have a fixed > > name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen > > drivers will not be probed. > > IMO, the Linux driver should be fixed and the requirement to use a fixed > node name be dropped from the binding. The driver itself already probes > by compatible, the node name seems only to be used by the MFD driver to > detect which functions to enable. It could do the same by checking for > compatibles. Otherwise you invite a game of cat and mouse, where in > future, this is changed back again reintroducing the regression.. How would you handle in general such kind of change? Would you keep the driver probing for both the old name with the `_` and the compatible or you just break the compatibility? Francesco
Hello Francesco, On 16.05.22 16:53, Francesco Dolcini wrote: > On Mon, May 16, 2022 at 02:49:12PM +0200, Ahmad Fatoum wrote: >> On 16.05.22 13:58, Max Krummenacher wrote: >>> From: Max Krummenacher <max.krummenacher@toradex.com> >>> >>> The STMPE MFD device binding requires the child node to have a fixed >>> name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen >>> drivers will not be probed. >> >> IMO, the Linux driver should be fixed and the requirement to use a fixed >> node name be dropped from the binding. The driver itself already probes >> by compatible, the node name seems only to be used by the MFD driver to >> detect which functions to enable. It could do the same by checking for >> compatibles. Otherwise you invite a game of cat and mouse, where in >> future, this is changed back again reintroducing the regression.. > > How would you handle in general such kind of change? Would you keep the > driver probing for both the old name with the `_` and the compatible or > you just break the compatibility? The Binding requires child nodes to have both a specific node name and compatible. So if we remove the node name restriction, we stay compliant to the binding. The MFD driver requires specific node names, while the MFD cell drivers seem to be matched by compatibles. It's thus should be safe to replace the node name readout in the MFD driver with a compatible check. Existing device trees will continue to work. Newer device trees can use dashes. Once the binding is converted to YAML, we could enforce a name to get everyone aligned, but it will be just a binding checker warning, not a breakage on update. Cheers, Ahmad > > Francesco > >
On Mon, May 16, 2022 at 01:58:30PM +0200, Max Krummenacher wrote: > From: Max Krummenacher <max.krummenacher@toradex.com> > > The STMPE MFD device binding requires the child node to have a fixed > name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen > drivers will not be probed. > > Fixes: 56086b5e804f ("ARM: dts: imx6qdl-apalis: Avoid underscore in node name") > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > --- > > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > index bd763bae596b..da919d0544a8 100644 > --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > @@ -315,7 +315,7 @@ > /* ADC conversion time: 80 clocks */ > st,sample-time = <4>; > > - stmpe_touchscreen: stmpe-touchscreen { > + stmpe_touchscreen: stmpe_touchscreen { In any case, the correct name would have been 'touchscreen' and 'adc'. > compatible = "st,stmpe-ts"; > /* 8 sample average control */ > st,ave-ctrl = <3>; > @@ -332,7 +332,7 @@ > st,touch-det-delay = <5>; > }; > > - stmpe_adc: stmpe-adc { > + stmpe_adc: stmpe_adc { > compatible = "st,stmpe-adc"; > /* forbid to use ADC channels 3-0 (touch) */ > st,norequest-mask = <0x0F>; > -- > 2.20.1 >
diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi index bd763bae596b..da919d0544a8 100644 --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi @@ -315,7 +315,7 @@ /* ADC conversion time: 80 clocks */ st,sample-time = <4>; - stmpe_touchscreen: stmpe-touchscreen { + stmpe_touchscreen: stmpe_touchscreen { compatible = "st,stmpe-ts"; /* 8 sample average control */ st,ave-ctrl = <3>; @@ -332,7 +332,7 @@ st,touch-det-delay = <5>; }; - stmpe_adc: stmpe-adc { + stmpe_adc: stmpe_adc { compatible = "st,stmpe-adc"; /* forbid to use ADC channels 3-0 (touch) */ st,norequest-mask = <0x0F>;