| Message ID | 20240716-majesty-antler-d9bedc7fd0af@wendy (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | [v1] riscv: dts: starfive: remove non-existant spi device from jh7110-common.dtsi | expand |
On 16/07/2024 12:54, Conor Dooley wrote: > There is no rohm,dh2228fv on any of supported JH7110 boards - in fact > the dh2228fv almost certainly does not exist as it is not a valid Rohm > part number. Likely a typo by Maxime when adding the device originally, > and should have been bh2228fv, but these boards do not have a bh2228fv > either! Remove it from jh7110-common.dtsi - pretending to have a device > so that the spidev driver will be bound by Linux is not acceptable. > > Fixes: 74fb20c8f05d ("riscv: dts: starfive: Add spi node and pins configuration") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Conor Dooley wrote: > There is no rohm,dh2228fv on any of supported JH7110 boards - in fact > the dh2228fv almost certainly does not exist as it is not a valid Rohm > part number. Likely a typo by Maxime when adding the device originally, > and should have been bh2228fv, but these boards do not have a bh2228fv > either! Remove it from jh7110-common.dtsi - pretending to have a device > so that the spidev driver will be bound by Linux is not acceptable. Hi Conor, This patch is correct, but as you mention the fake device was most likely added in order to use spidev from userspace with random devices added on the exposed pins. In case someone actually makes use of this wouldn't this be a regression? What is the right way to support this? /Emil > > Fixes: 74fb20c8f05d ("riscv: dts: starfive: Add spi node and pins configuration") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > CC: Emil Renner Berthing <kernel@esmil.dk> > CC: Conor Dooley <conor@kernel.org> > CC: Rob Herring <robh@kernel.org> > CC: Krzysztof Kozlowski <krzk+dt@kernel.org> > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: Albert Ou <aou@eecs.berkeley.edu> > CC: William Qiu <william.qiu@starfivetech.com> > CC: linux-riscv@lists.infradead.org > CC: devicetree@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi > index 8ff6ea64f048..395436ec0f97 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi > @@ -346,12 +346,6 @@ &spi0 { > pinctrl-names = "default"; > pinctrl-0 = <&spi0_pins>; > status = "okay"; > - > - spi_dev0: spi@0 { > - compatible = "rohm,dh2228fv"; > - reg = <0>; > - spi-max-frequency = <10000000>; > - }; > }; > > &sysgpio { > -- > 2.43.2 >
On Fri, Jul 26, 2024 at 08:29:39AM -0400, Emil Renner Berthing wrote: > Conor Dooley wrote: > > There is no rohm,dh2228fv on any of supported JH7110 boards - in fact > > the dh2228fv almost certainly does not exist as it is not a valid Rohm > > part number. Likely a typo by Maxime when adding the device originally, > > and should have been bh2228fv, but these boards do not have a bh2228fv > > either! Remove it from jh7110-common.dtsi - pretending to have a device > > so that the spidev driver will be bound by Linux is not acceptable. > > This patch is correct, but as you mention the fake device was most likely added > in order to use spidev from userspace with random devices added on the exposed > pins. In case someone actually makes use of this wouldn't this be a regression? > What is the right way to support this? Unfortunately, there's no "right way" that's supported for for this particular case. If people want to use spidev for their device, they should either document it in the bindings, add the compatible to the spidev driver and use an overlay to add the device to the dts or they can r bind the spidev driver to the device from userspace. The other thing, which doesn't exist yet, is a connector binding. The folks are Beagle are currently working on creating a connector binding for the Mikrobus connector - but that's rather far from complete at the moment. Cheers, Conor. > > Fixes: 74fb20c8f05d ("riscv: dts: starfive: Add spi node and pins configuration") > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > CC: Emil Renner Berthing <kernel@esmil.dk> > > CC: Conor Dooley <conor@kernel.org> > > CC: Rob Herring <robh@kernel.org> > > CC: Krzysztof Kozlowski <krzk+dt@kernel.org> > > CC: Paul Walmsley <paul.walmsley@sifive.com> > > CC: Palmer Dabbelt <palmer@dabbelt.com> > > CC: Albert Ou <aou@eecs.berkeley.edu> > > CC: William Qiu <william.qiu@starfivetech.com> > > CC: linux-riscv@lists.infradead.org > > CC: devicetree@vger.kernel.org > > CC: linux-kernel@vger.kernel.org > > --- > > arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi > > index 8ff6ea64f048..395436ec0f97 100644 > > --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi > > +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi > > @@ -346,12 +346,6 @@ &spi0 { > > pinctrl-names = "default"; > > pinctrl-0 = <&spi0_pins>; > > status = "okay"; > > - > > - spi_dev0: spi@0 { > > - compatible = "rohm,dh2228fv"; > > - reg = <0>; > > - spi-max-frequency = <10000000>; > > - }; > > }; > > > > &sysgpio { > > -- > > 2.43.2 > >
Conor Dooley wrote: > On Fri, Jul 26, 2024 at 08:29:39AM -0400, Emil Renner Berthing wrote: > > Conor Dooley wrote: > > > There is no rohm,dh2228fv on any of supported JH7110 boards - in fact > > > the dh2228fv almost certainly does not exist as it is not a valid Rohm > > > part number. Likely a typo by Maxime when adding the device originally, > > > and should have been bh2228fv, but these boards do not have a bh2228fv > > > either! Remove it from jh7110-common.dtsi - pretending to have a device > > > so that the spidev driver will be bound by Linux is not acceptable. > > > > This patch is correct, but as you mention the fake device was most likely added > > in order to use spidev from userspace with random devices added on the exposed > > pins. In case someone actually makes use of this wouldn't this be a regression? > > What is the right way to support this? > > Unfortunately, there's no "right way" that's supported for for this > particular case. If people want to use spidev for their device, they > should either document it in the bindings, add the compatible to the > spidev driver and use an overlay to add the device to the dts or they > can r bind the spidev driver to the device from userspace. > > The other thing, which doesn't exist yet, is a connector binding. The > folks are Beagle are currently working on creating a connector binding > for the Mikrobus connector - but that's rather far from complete at the > moment. > I see. Thanks for the explanation. At least now there is this thread any potential users might find. Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
From: Conor Dooley <conor.dooley@microchip.com> On Tue, 16 Jul 2024 11:54:00 +0100, Conor Dooley wrote: > There is no rohm,dh2228fv on any of supported JH7110 boards - in fact > the dh2228fv almost certainly does not exist as it is not a valid Rohm > part number. Likely a typo by Maxime when adding the device originally, > and should have been bh2228fv, but these boards do not have a bh2228fv > either! Remove it from jh7110-common.dtsi - pretending to have a device > so that the spidev driver will be bound by Linux is not acceptable. > > [...] Applied to riscv-dt-fixes, thanks! [1/1] riscv: dts: starfive: remove non-existant spi device from jh7110-common.dtsi https://git.kernel.org/conor/c/db6efa5e81a5 Thanks, Conor.
diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi index 8ff6ea64f048..395436ec0f97 100644 --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi @@ -346,12 +346,6 @@ &spi0 { pinctrl-names = "default"; pinctrl-0 = <&spi0_pins>; status = "okay"; - - spi_dev0: spi@0 { - compatible = "rohm,dh2228fv"; - reg = <0>; - spi-max-frequency = <10000000>; - }; }; &sysgpio {
There is no rohm,dh2228fv on any of supported JH7110 boards - in fact the dh2228fv almost certainly does not exist as it is not a valid Rohm part number. Likely a typo by Maxime when adding the device originally, and should have been bh2228fv, but these boards do not have a bh2228fv either! Remove it from jh7110-common.dtsi - pretending to have a device so that the spidev driver will be bound by Linux is not acceptable. Fixes: 74fb20c8f05d ("riscv: dts: starfive: Add spi node and pins configuration") Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- CC: Emil Renner Berthing <kernel@esmil.dk> CC: Conor Dooley <conor@kernel.org> CC: Rob Herring <robh@kernel.org> CC: Krzysztof Kozlowski <krzk+dt@kernel.org> CC: Paul Walmsley <paul.walmsley@sifive.com> CC: Palmer Dabbelt <palmer@dabbelt.com> CC: Albert Ou <aou@eecs.berkeley.edu> CC: William Qiu <william.qiu@starfivetech.com> CC: linux-riscv@lists.infradead.org CC: devicetree@vger.kernel.org CC: linux-kernel@vger.kernel.org --- arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 6 ------ 1 file changed, 6 deletions(-)