diff mbox series

[v1,2/4] ARM: dts: rockchip: remove unlikly-to-exist DAC from elgin-r1

Message ID 20240717-parrot-malt-83cc04bf6b36@spud (mailing list archive)
State New, archived
Headers show
Series Removal of non-existent DAC nodes | expand

Commit Message

Conor Dooley July 17, 2024, 9:37 a.m. UTC 
From: Conor Dooley <conor.dooley@microchip.com>

The Rohm dh2228fv (really the bh2228fv, the compatible in the kernel has
a typo) does not support frequencies above 10 MHz, nor per the
datasheet appear to use either CPOL or CPHA. I suspect that this
devicetree is abusing the compatible in order to bind the spidev driver
in Linux. Pretending to have devices on a board for this purpose is not
acceptable, so remove it.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I could not find any documentation for this board online, and it does
not blatantly say that the device is a "spidev" like other [ab]users, so
it is possible there's actually a DAC here - but I doubt it is a
bh2228fv given the other incompatibilities.
---
 arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts | 8 --------
 1 file changed, 8 deletions(-)

Comments

Krzysztof Kozlowski July 17, 2024, 9:39 a.m. UTC | #1 
On 17/07/2024 11:37, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The Rohm dh2228fv (really the bh2228fv, the compatible in the kernel has
> a typo) does not support frequencies above 10 MHz, nor per the
> datasheet appear to use either CPOL or CPHA. I suspect that this
> devicetree is abusing the compatible in order to bind the spidev driver
> in Linux. Pretending to have devices on a board for this purpose is not
> acceptable, so remove it.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I could not find any documentation for this board online, and it does
> not blatantly say that the device is a "spidev" like other [ab]users, so
> it is possible there's actually a DAC here - but I doubt it is a
> bh2228fv given the other incompatibilities.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Heiko Stübner July 17, 2024, 10:18 a.m. UTC | #2 
adding Otavio,

Am Mittwoch, 17. Juli 2024, 11:37:54 CEST schrieb Conor Dooley:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The Rohm dh2228fv (really the bh2228fv, the compatible in the kernel has
> a typo) does not support frequencies above 10 MHz, nor per the
> datasheet appear to use either CPOL or CPHA. I suspect that this
> devicetree is abusing the compatible in order to bind the spidev driver
> in Linux. Pretending to have devices on a board for this purpose is not
> acceptable, so remove it.

Reasoning is sound, so I'll pick this up after the merge window.


> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I could not find any documentation for this board online, and it does
> not blatantly say that the device is a "spidev" like other [ab]users, so
> it is possible there's actually a DAC here - but I doubt it is a
> bh2228fv given the other incompatibilities.

Otavio, as the original submitter of the Elgin R1 [0], do you happen
to know what type of device this is? Especially as there really do not
seem to be any schematics around for that board.


Heiko



[0] https://patchwork.kernel.org/project/linux-rockchip/patch/20190104014023.17973-4-otavio@ossystems.com.br/

> ---
>  arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> index 2d9994379eb2..9df1cef406c5 100644
> --- a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> +++ b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> @@ -167,14 +167,6 @@ &spi {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&spim1_clk &spim1_cs0 &spim1_tx &spim1_rx>;
>  	status = "okay";
> -
> -	dh2228fv: dac@0 {
> -		compatible = "rohm,dh2228fv";
> -		reg = <0>;
> -		spi-max-frequency = <24000000>;
> -		spi-cpha;
> -		spi-cpol;
> -	};
>  };
>  
>  &u2phy {
>
Fabio Estevam July 17, 2024, 3:34 p.m. UTC | #3 
On Wed, Jul 17, 2024 at 11:38 AM Otavio Salvador
<otavio.salvador@ossystems.com.br> wrote:

> @Fabio Estevam can you check this patch?

Correct, the rv1108-elgin-r1.dts board does not contain the DAC.

There is an LCD controlled via spidev though.

Conor,

If spidev is needed, what is the recommended way to describe it in the
devicetree?

Thanks
Conor Dooley July 17, 2024, 4:39 p.m. UTC | #4 
On Wed, Jul 17, 2024 at 12:34:03PM -0300, Fabio Estevam wrote:
> On Wed, Jul 17, 2024 at 11:38 AM Otavio Salvador
> <otavio.salvador@ossystems.com.br> wrote:
> 
> > @Fabio Estevam can you check this patch?
> 
> Correct, the rv1108-elgin-r1.dts board does not contain the DAC.
> 
> There is an LCD controlled via spidev though.
> 
> Conor,
> 
> If spidev is needed, what is the recommended way to describe it in the
> devicetree?

Describe the device you actually have in a binding, and add that
compatible to the spidev driver. In this case, given there's a
particular LCD there, that'll work. There's nothing wrong with using
spidev, it's just lying about what hardware that is there that's
problematic.

Otherwise, when it is an open connector where you can put anything, it's
a wee bit messier... A connector binding is required then, something
like the WIP mikrobus stuff that the beagle lads are working on. That's
far from ready, at least on the devicetree side of things, though so
you need to apply an overlay in your bootloader in that case.
Fabio Estevam July 17, 2024, 4:46 p.m. UTC | #5 
On Wed, Jul 17, 2024 at 6:38 AM Conor Dooley <conor@kernel.org> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> The Rohm dh2228fv (really the bh2228fv, the compatible in the kernel has
> a typo) does not support frequencies above 10 MHz, nor per the
> datasheet appear to use either CPOL or CPHA. I suspect that this
> devicetree is abusing the compatible in order to bind the spidev driver
> in Linux. Pretending to have devices on a board for this purpose is not
> acceptable, so remove it.

In the Subject: s/unlikly/unlikely

Reviewed-by: Fabio Estevam <festevam@gmail.com>
Dragan Simic July 18, 2024, 4:20 a.m. UTC | #6 
Hello Conor,

On 2024-07-17 11:37, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The Rohm dh2228fv (really the bh2228fv, the compatible in the kernel 
> has
> a typo) does not support frequencies above 10 MHz, nor per the
> datasheet appear to use either CPOL or CPHA. I suspect that this
> devicetree is abusing the compatible in order to bind the spidev driver
> in Linux. Pretending to have devices on a board for this purpose is not
> acceptable, so remove it.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

There's a small typo in the patch subject:
s/unlikly/unlikely/

> ---
> I could not find any documentation for this board online, and it does
> not blatantly say that the device is a "spidev" like other [ab]users, 
> so
> it is possible there's actually a DAC here - but I doubt it is a
> bh2228fv given the other incompatibilities.
> ---
>  arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> index 2d9994379eb2..9df1cef406c5 100644
> --- a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> +++ b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
> @@ -167,14 +167,6 @@ &spi {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&spim1_clk &spim1_cs0 &spim1_tx &spim1_rx>;
>  	status = "okay";
> -
> -	dh2228fv: dac@0 {
> -		compatible = "rohm,dh2228fv";
> -		reg = <0>;
> -		spi-max-frequency = <24000000>;
> -		spi-cpha;
> -		spi-cpol;
> -	};
>  };
> 
>  &u2phy {
Dragan Simic July 18, 2024, 4:46 a.m. UTC | #7 
On 2024-07-18 06:20, Dragan Simic wrote:
> Hello Conor,
> 
> On 2024-07-17 11:37, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@microchip.com>
>> 
>> The Rohm dh2228fv (really the bh2228fv, the compatible in the kernel 
>> has
>> a typo) does not support frequencies above 10 MHz, nor per the
>> datasheet appear to use either CPOL or CPHA. I suspect that this
>> devicetree is abusing the compatible in order to bind the spidev 
>> driver
>> in Linux. Pretending to have devices on a board for this purpose is 
>> not
>> acceptable, so remove it.
>> 
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> There's a small typo in the patch subject:
> s/unlikly/unlikely/

Ah, sorry for the noise, I see now it's been pointed out already.

>> ---
>> I could not find any documentation for this board online, and it does
>> not blatantly say that the device is a "spidev" like other [ab]users, 
>> so
>> it is possible there's actually a DAC here - but I doubt it is a
>> bh2228fv given the other incompatibilities.
>> ---
>>  arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts | 8 --------
>>  1 file changed, 8 deletions(-)
>> 
>> diff --git a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
>> b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
>> index 2d9994379eb2..9df1cef406c5 100644
>> --- a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
>> +++ b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
>> @@ -167,14 +167,6 @@ &spi {
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&spim1_clk &spim1_cs0 &spim1_tx &spim1_rx>;
>>  	status = "okay";
>> -
>> -	dh2228fv: dac@0 {
>> -		compatible = "rohm,dh2228fv";
>> -		reg = <0>;
>> -		spi-max-frequency = <24000000>;
>> -		spi-cpha;
>> -		spi-cpol;
>> -	};
>>  };
>> 
>>  &u2phy {
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
index 2d9994379eb2..9df1cef406c5 100644
--- a/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
+++ b/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dts
@@ -167,14 +167,6 @@  &spi {
 	pinctrl-names = "default";
 	pinctrl-0 = <&spim1_clk &spim1_cs0 &spim1_tx &spim1_rx>;
 	status = "okay";
-
-	dh2228fv: dac@0 {
-		compatible = "rohm,dh2228fv";
-		reg = <0>;
-		spi-max-frequency = <24000000>;
-		spi-cpha;
-		spi-cpol;
-	};
 };
 
 &u2phy {