| Message ID | 20161024150449.11132-3-h.morris@cascoda.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Schmidt |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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