[PATCH 0/3] ieee802154: add support for Cascoda CA8210

[PATCH 0/3] ieee802154: add support for Cascoda CA8210

[harrymorris12]

From: Harry Morris <h.morris@cascoda.com>

This patchset adds a new device driver, documentation and build support for
Cascoda's CA8210 IEEE 802.15.4 radio transceiver:
http://www.cascoda.com/products/ca-821x/

Harry Morris (3):
  ieee802154: Add CA8210 IEEE 802.15.4 device driver
  ieee802154: Add device tree documentation for CA8210
  ieee802154: Add entry in MAINTAINTERS for CA8210 driver

 .../devicetree/bindings/net/ieee802154/ca8210.txt  |   28 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 MAINTAINERS                                        |    9 +
 drivers/net/ieee802154/Kconfig                     |   18 +
 drivers/net/ieee802154/Makefile                    |    1 +
 drivers/net/ieee802154/ca8210.c                    | 3533 ++++++++++++++++++++
 6 files changed, 3590 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
 create mode 100644 drivers/net/ieee802154/ca8210.c

--
2.9.3.windows.1

[PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[harrymorris12]

From: Harry Morris <h.morris@cascoda.com>

Signed-off-by: Harry Morris <h.morris@cascoda.com>
---
 .../devicetree/bindings/net/ieee802154/ca8210.txt  | 28 ++++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 2 files changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ieee802154/ca8210.txt

diff --git a/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt b/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
new file mode 100644
index 0000000..0297ce8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
@@ -0,0 +1,28 @@
+* CA8210 IEEE 802.15.4 *
+
+Required properties:
+	- compatible:           should be "cascoda,ca8210"
+	- reg:                  Controlling chip select
+	- spi-max-frequency:    Maximum clock speed, should be *less than*
+	                        4000000
+	- spi-cpol:             Invert clock polarity
+	- reset-gpio:           GPIO attached to reset
+	- irq-gpio:             GPIO attached to IRQ
+Optional properties:
+	- extclock-enable:      Include for the ca8210 to route its 16MHz clock
+	                        to an output
+	- extclock-freq:        Frequency in Hz of the external clock
+	- extclock-gpio:        GPIO of the ca8210 to output the clock on
+
+Example:
+	ca8210@0 {
+		compatible = "cascoda,ca8210";
+		reg = <0>;
+		spi-max-frequency = <3000000>;
+		spi-cpol;
+		reset-gpio = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+		irq-gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+		extclock-enable;
+		extclock-freq = 16000000;
+		extclock-gpio = 2;
+	};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index d415b38..725a996 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -26,6 +26,7 @@ brcm	Broadcom Corporation
 buffalo	Buffalo, Inc.
 calxeda	Calxeda
 capella	Capella Microsystems, Inc
+cascoda	Cascoda Ltd.
 cavium	Cavium, Inc.
 cdns	Cadence Design Systems Inc.
 chrp	Common Hardware Reference Platform
-- 
2.9.3.windows.1

[PATCH 3/3] ieee802154: Add entry in MAINTAINTERS for CA8210 driver

[harrymorris12]

From: Harry Morris <h.morris@cascoda.com>

Signed-off-by: Harry Morris <h.morris@cascoda.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 05e5450..37df5b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2130,6 +2130,15 @@ W:	http://www.linux-c6x.org/wiki/index.php/Main_Page
 S:	Maintained
 F:	arch/c6x/
 
+CA8210 IEEE-802.15.4 RADIO DRIVER
+M:	Harry Morris <h.morris@cascoda.com>
+M:	linuxdev@cascoda.com
+L:	linux-wpan@vger.kernel.org
+W:	https://github.com/Cascoda/ca8210-linux.git
+S:	Maintained
+F:	drivers/net/ieee802154/ca8210.c
+F:	Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
+
 CACHEFILES: FS-CACHE BACKEND FOR CACHING ON MOUNTED FILESYSTEMS
 M:	David Howells <dhowells@redhat.com>
 L:	linux-cachefs@redhat.com
-- 
2.9.3.windows.1

Re: [PATCH 0/3] ieee802154: add support for Cascoda CA8210

[Stefan Schmidt]

Hello.

On 24/10/16 17:04, harrymorris12@gmail.com wrote:
> From: Harry Morris <h.morris@cascoda.com>
>
> This patchset adds a new device driver, documentation and build support for
> Cascoda's CA8210 IEEE 802.15.4 radio transceiver:
> http://www.cascoda.com/products/ca-821x/

First of all I'm happy to see another company stepping up to support 
their 802.15.4 transceiver directly in Linux mainline. Much appreciated!

I will reviewing the driver over the next days and provide you some 
feedback.

regards
Stefan Schmidt

Re: [PATCH 1/3] ieee802154: Add CA8210 IEEE 802.15.4 device driver

[Stefan Schmidt]

Hello.

Against what kernel did you prepare this patches?

I tried to apply them against bluetooth-next and got the following:
Applying: ieee802154: Add CA8210 IEEE 802.15.4 device driver
error: patch failed: drivers/net/ieee802154/Kconfig:63
error: drivers/net/ieee802154/Kconfig: patch does not apply
error: patch failed: drivers/net/ieee802154/Makefile:3
error: drivers/net/ieee802154/Makefile: patch does not apply
Patch failed at 0001 ieee802154: Add CA8210 IEEE 802.15.4 device driver
The copy of the patch that failed is found in:
     /home/stefan/Kernel/bluetooth-next-rpi/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

A guess in the blue from my side would be that you did this patches 
against the latest kernel release (4.8) and not the bluetooth-next 
kernel repo?

https://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/

You would need to rebase your patches on this one so we can apply them 
cleanly and test build, run checker scripts, etc.

Given the driver file itself is isolated it will likely just we some 
small conflicts in the Makefile and Kconfig.

Patch 2 and 3 fail to apply as well.

regards
Stefan Schmidt

Re: [PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[Stefan Schmidt]

Hello.

On 24/10/16 17:04, harrymorris12@gmail.com wrote:
> From: Harry Morris <h.morris@cascoda.com>
>
> Signed-off-by: Harry Morris <h.morris@cascoda.com>
> ---
>  .../devicetree/bindings/net/ieee802154/ca8210.txt  | 28 ++++++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>  2 files changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
>
> diff --git a/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt b/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
> new file mode 100644
> index 0000000..0297ce8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
> @@ -0,0 +1,28 @@
> +* CA8210 IEEE 802.15.4 *
> +
> +Required properties:
> +	- compatible:           should be "cascoda,ca8210"


Start should with a capital S here as you do on all other lines. Total 
nitpick. :)

> +	- reg:                  Controlling chip select
> +	- spi-max-frequency:    Maximum clock speed, should be *less than*
> +	                        4000000
> +	- spi-cpol:             Invert clock polarity

Is this a mandatory field for the bindings? I would expect one being the 
default and used while the other one being an optional property and only 
used when the hardware setup needs this. Does this make sense?


> +	- reset-gpio:           GPIO attached to reset
> +	- irq-gpio:             GPIO attached to IRQ
> +Optional properties:
> +	- extclock-enable:      Include for the ca8210 to route its 16MHz clock
> +	                        to an output
> +	- extclock-freq:        Frequency in Hz of the external clock
> +	- extclock-gpio:        GPIO of the ca8210 to output the clock on
> +
> +Example:
> +	ca8210@0 {
> +		compatible = "cascoda,ca8210";
> +		reg = <0>;
> +		spi-max-frequency = <3000000>;
> +		spi-cpol;
> +		reset-gpio = <&gpio1 1 GPIO_ACTIVE_HIGH>;
> +		irq-gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
> +		extclock-enable;
> +		extclock-freq = 16000000;
> +		extclock-gpio = 2;
> +	};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index d415b38..725a996 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -26,6 +26,7 @@ brcm	Broadcom Corporation
>  buffalo	Buffalo, Inc.
>  calxeda	Calxeda
>  capella	Capella Microsystems, Inc
> +cascoda	Cascoda Ltd.
>  cavium	Cavium, Inc.
>  cdns	Cadence Design Systems Inc.
>  chrp	Common Hardware Reference Platform
>

The rest looks good.

regards
Stefan Schmidt

Re: [PATCH 3/3] ieee802154: Add entry in MAINTAINTERS for CA8210 driver

[Stefan Schmidt]

Hello.

On 24/10/16 17:04, harrymorris12@gmail.com wrote:
> From: Harry Morris <h.morris@cascoda.com>
>
> Signed-off-by: Harry Morris <h.morris@cascoda.com>
> ---
>  MAINTAINERS | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 05e5450..37df5b0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2130,6 +2130,15 @@ W:	http://www.linux-c6x.org/wiki/index.php/Main_Page
>  S:	Maintained
>  F:	arch/c6x/
>
> +CA8210 IEEE-802.15.4 RADIO DRIVER
> +M:	Harry Morris <h.morris@cascoda.com>
> +M:	linuxdev@cascoda.com
> +L:	linux-wpan@vger.kernel.org
> +W:	https://github.com/Cascoda/ca8210-linux.git
> +S:	Maintained
> +F:	drivers/net/ieee802154/ca8210.c
> +F:	Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
> +
>  CACHEFILES: FS-CACHE BACKEND FOR CACHING ON MOUNTED FILESYSTEMS
>  M:	David Howells <dhowells@redhat.com>
>  L:	linux-cachefs@redhat.com
>

Looks good to me. You can add my:

Acked-by: Stefan Schmidt <stefan@osg.samsung.com>

regards
Stefan Schmidt

Re: [PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[Harry Morris]

Hi Stefan,

On 25/10/2016 16:54, Stefan Schmidt wrote:
> Start should with a capital S here as you do on all other lines. Total 
> nitpick. :)
Perfect, I'm sure there's a few more elsewhere I've missed
>> +    - reg:                  Controlling chip select
>> +    - spi-max-frequency:    Maximum clock speed, should be *less than*
>> +                            4000000
>> +    - spi-cpol:             Invert clock polarity
>
> Is this a mandatory field for the bindings? I would expect one being 
> the default and used while the other one being an optional property 
> and only used when the hardware setup needs this. Does this make sense?
I'm afraid not, my understanding is that the spi-cpol property is used 
by the spi master driver rather than the device driver, so if I need 
CPOL=1 (which the CA8210 always does) then the user doesn't have a 
choice but to specify spi-cpol here? As omitting would cause the spi 
master to use CPOL=0 for the attached slave.

Regards,
Harry

Re: [PATCH 1/3] ieee802154: Add CA8210 IEEE 802.15.4 device driver

[Harry Morris]

Hi Stefan,

You're absolutely right, my apologies. I've rebuilt my patches against 
the bluetooth-next tree and will re-submit a v2.

Regards,

Harry

On 25/10/2016 16:54, Stefan Schmidt wrote:
> Hello.
>
> Against what kernel did you prepare this patches?
>
> I tried to apply them against bluetooth-next and got the following:
> Applying: ieee802154: Add CA8210 IEEE 802.15.4 device driver
> error: patch failed: drivers/net/ieee802154/Kconfig:63
> error: drivers/net/ieee802154/Kconfig: patch does not apply
> error: patch failed: drivers/net/ieee802154/Makefile:3
> error: drivers/net/ieee802154/Makefile: patch does not apply
> Patch failed at 0001 ieee802154: Add CA8210 IEEE 802.15.4 device driver
> The copy of the patch that failed is found in:
>     /home/stefan/Kernel/bluetooth-next-rpi/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> A guess in the blue from my side would be that you did this patches 
> against the latest kernel release (4.8) and not the bluetooth-next 
> kernel repo?
>
> https://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/ 
>
>
> You would need to rebase your patches on this one so we can apply them 
> cleanly and test build, run checker scripts, etc.
>
> Given the driver file itself is isolated it will likely just we some 
> small conflicts in the Makefile and Kconfig.
>
> Patch 2 and 3 fail to apply as well.
>
> regards
> Stefan Schmidt
>

Re: [PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[Stefan Schmidt]

Hello.

On 25/10/16 18:36, Harry Morris wrote:
> Hi Stefan,
>
> On 25/10/2016 16:54, Stefan Schmidt wrote:
>> Start should with a capital S here as you do on all other lines. Total
>> nitpick. :)
> Perfect, I'm sure there's a few more elsewhere I've missed
>>> +    - reg:                  Controlling chip select
>>> +    - spi-max-frequency:    Maximum clock speed, should be *less than*
>>> +                            4000000
>>> +    - spi-cpol:             Invert clock polarity
>>
>> Is this a mandatory field for the bindings? I would expect one being
>> the default and used while the other one being an optional property
>> and only used when the hardware setup needs this. Does this make sense?
> I'm afraid not, my understanding is that the spi-cpol property is used
> by the spi master driver rather than the device driver, so if I need
> CPOL=1 (which the CA8210 always does) then the user doesn't have a
> choice but to specify spi-cpol here? As omitting would cause the spi
> master to use CPOL=0 for the attached slave.

I would expect there is a way to set this in the driver itself as it is 
the default and the only working mode. Having to set it in the device 
tree bindings seems odd as this is really no option or parameter that 
can be changed.

Did you look around in other SPI driver so see how they handle such a 
situation?

regards
Stefan Schmidt

Re: [PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[Stefan Schmidt]

Hello.

On 27/10/16 00:33, Stefan Schmidt wrote:
> Hello.
>
> On 25/10/16 18:36, Harry Morris wrote:
>> Hi Stefan,
>>
>> On 25/10/2016 16:54, Stefan Schmidt wrote:
>>> Start should with a capital S here as you do on all other lines. Total
>>> nitpick. :)
>> Perfect, I'm sure there's a few more elsewhere I've missed
>>>> +    - reg:                  Controlling chip select
>>>> +    - spi-max-frequency:    Maximum clock speed, should be *less than*
>>>> +                            4000000
>>>> +    - spi-cpol:             Invert clock polarity
>>>
>>> Is this a mandatory field for the bindings? I would expect one being
>>> the default and used while the other one being an optional property
>>> and only used when the hardware setup needs this. Does this make sense?
>> I'm afraid not, my understanding is that the spi-cpol property is used
>> by the spi master driver rather than the device driver, so if I need
>> CPOL=1 (which the CA8210 always does) then the user doesn't have a
>> choice but to specify spi-cpol here? As omitting would cause the spi
>> master to use CPOL=0 for the attached slave.
>
> I would expect there is a way to set this in the driver itself as it is
> the default and the only working mode. Having to set it in the device
> tree bindings seems odd as this is really no option or parameter that
> can be changed.
>
> Did you look around in other SPI driver so see how they handle such a
> situation?

I did look around myself quickly and it seems indeed the way to set the 
clock polarity. Just keep it. Maybe adjust the description to "Requires 
invert clock polarity".

regards
Stefan Schmidt

Re: [PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[Harry Morris]

Hi Stefan,

On 26/10/2016 23:51, Stefan Schmidt wrote:
> Hello.
>
> On 27/10/16 00:33, Stefan Schmidt wrote:
>> Hello.
>>
>> On 25/10/16 18:36, Harry Morris wrote:
>>> Hi Stefan,
>>>
>>> On 25/10/2016 16:54, Stefan Schmidt wrote:
>>>> Start should with a capital S here as you do on all other lines. Total
>>>> nitpick. :)
>>> Perfect, I'm sure there's a few more elsewhere I've missed
>>>>> +    - reg: Controlling chip select
>>>>> +    - spi-max-frequency:    Maximum clock speed, should be *less 
>>>>> than*
>>>>> +                            4000000
>>>>> +    - spi-cpol:             Invert clock polarity
>>>>
>>>> Is this a mandatory field for the bindings? I would expect one being
>>>> the default and used while the other one being an optional property
>>>> and only used when the hardware setup needs this. Does this make 
>>>> sense?
>>> I'm afraid not, my understanding is that the spi-cpol property is used
>>> by the spi master driver rather than the device driver, so if I need
>>> CPOL=1 (which the CA8210 always does) then the user doesn't have a
>>> choice but to specify spi-cpol here? As omitting would cause the spi
>>> master to use CPOL=0 for the attached slave.
>>
>> I would expect there is a way to set this in the driver itself as it is
>> the default and the only working mode. Having to set it in the device
>> tree bindings seems odd as this is really no option or parameter that
>> can be changed.
>>
>> Did you look around in other SPI driver so see how they handle such a
>> situation?
>
> I did look around myself quickly and it seems indeed the way to set 
> the clock polarity. Just keep it. Maybe adjust the description to 
> "Requires invert clock polarity".

I'm not sure which is the "correct" approach but I think you're right 
actually, I can indeed set the mode in the spi_device struct from the 
driver, eliminating the need to invert the polarity in the device tree. 
Since the mode is fixed for the hardware I agree it makes more sense to 
set it in the driver rather than the device tree. I'm happy to make the 
change.

Thanks,

Harry

Re: [PATCH 2/3] ieee802154: Add device tree documentation for CA8210

[Stefan Schmidt]

Hello.

On 27/10/16 11:52, Harry Morris wrote:
> Hi Stefan,
>
> On 26/10/2016 23:51, Stefan Schmidt wrote:
>> Hello.
>>
>> On 27/10/16 00:33, Stefan Schmidt wrote:
>>> Hello.
>>>
>>> On 25/10/16 18:36, Harry Morris wrote:
>>>> Hi Stefan,
>>>>
>>>> On 25/10/2016 16:54, Stefan Schmidt wrote:
>>>>> Start should with a capital S here as you do on all other lines. Total
>>>>> nitpick. :)
>>>> Perfect, I'm sure there's a few more elsewhere I've missed
>>>>>> +    - reg: Controlling chip select
>>>>>> +    - spi-max-frequency:    Maximum clock speed, should be *less
>>>>>> than*
>>>>>> +                            4000000
>>>>>> +    - spi-cpol:             Invert clock polarity
>>>>>
>>>>> Is this a mandatory field for the bindings? I would expect one being
>>>>> the default and used while the other one being an optional property
>>>>> and only used when the hardware setup needs this. Does this make
>>>>> sense?
>>>> I'm afraid not, my understanding is that the spi-cpol property is used
>>>> by the spi master driver rather than the device driver, so if I need
>>>> CPOL=1 (which the CA8210 always does) then the user doesn't have a
>>>> choice but to specify spi-cpol here? As omitting would cause the spi
>>>> master to use CPOL=0 for the attached slave.
>>>
>>> I would expect there is a way to set this in the driver itself as it is
>>> the default and the only working mode. Having to set it in the device
>>> tree bindings seems odd as this is really no option or parameter that
>>> can be changed.
>>>
>>> Did you look around in other SPI driver so see how they handle such a
>>> situation?
>>
>> I did look around myself quickly and it seems indeed the way to set
>> the clock polarity. Just keep it. Maybe adjust the description to
>> "Requires invert clock polarity".
>
> I'm not sure which is the "correct" approach but I think you're right
> actually, I can indeed set the mode in the spi_device struct from the
> driver, eliminating the need to invert the polarity in the device tree.
> Since the mode is fixed for the hardware I agree it makes more sense to
> set it in the driver rather than the device tree. I'm happy to make the
> change.

Keep it like you have it right now (through the bindings). It seems that 
this is the way other drivers handle it as well. It just puzled me at first.

Changing the description should help to make it clear that this is 
actually needed for the hardware to function.

regards
Stefan Schmidt