| Message ID | 20161115131830.90383-1-s@mlng.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 15/11/16 14:18, smlng wrote: Please fix your mail from line to have your real name to show up as patch author. > The mrf24j40 requires a high on the RESET pin, otherwise it will reset > itself over and over. With this patch the driver will ensure that a GPIO > (if connected) will be set to high during device initialization. Details: > > - set reset to high on driver probe > - make pin configurable via device tree (overlay) > - similar to the at86rf233 driver > - update devicetree documentation Thanks for taking the time to submit this patch! This looks good to me in general. I cc'ed Alan Ott as the driver maintainer now to see what he thinks about this patch. What I miss here is a bit of context for the wake pin. The commit message only covers the reset pin handling. What do we do with the wake pin here? > Signed-off-by: smlng <s@mlng.net> Same as the from line. Please use your real name for the SOB here. > --- > .../bindings/net/ieee802154/mrf24j40.txt | 4 ++ > drivers/net/ieee802154/mrf24j40.c | 59 ++++++++++++++++++++-- > include/linux/spi/mrf24j40.h | 9 ++++ > 3 files changed, 69 insertions(+), 3 deletions(-) > create mode 100644 include/linux/spi/mrf24j40.h > > diff --git a/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt b/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt > index a4ed2efb5b73..2fabba041e48 100644 > --- a/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt > +++ b/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt > @@ -9,6 +9,10 @@ Required properties: > - reg: the chipselect index > - interrupts: the interrupt generated by the device. > > +Optional properties: > + - reset-gpio: GPIO spec for the rstn pin > + - wake-gpio: GPIO spec for the wake pin > + > Example: > > mrf24j40ma@0 { > diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c > index 7b131f8e4093..275b54d6bad3 100644 > --- a/drivers/net/ieee802154/mrf24j40.c > +++ b/drivers/net/ieee802154/mrf24j40.c > @@ -16,9 +16,11 @@ > */ > > #include <linux/spi/spi.h> > +#include <linux/spi/mrf24j40.h> > #include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/regmap.h> > +#include <linux/of_gpio.h> > #include <linux/ieee802154.h> > #include <linux/irq.h> > #include <net/cfg802154.h> > @@ -1276,16 +1278,67 @@ static void mrf24j40_phy_setup(struct mrf24j40 *devrec) > } > } > > +static int > +mrf24j40_get_pdata(struct spi_device *spi, int *rstn, int *wake) > +{ > + struct mrf24j40_platform_data *pdata = spi->dev.platform_data; > + > + if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) { > + if (!pdata) > + return -ENOENT; > + > + *rstn = pdata->rstn; > + *wake = pdata->wake; > + return 0; > + } > + > + *rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0); > + *wake = of_get_named_gpio(spi->dev.of_node, "wake-gpio", 0); > + return 0; > +} > + > static int mrf24j40_probe(struct spi_device *spi) > { > int ret = -ENOMEM, irq_type; > + int rc, rstn, wake; > struct ieee802154_hw *hw; > struct mrf24j40 *devrec; > > - dev_info(&spi->dev, "probe(). IRQ: %d\n", spi->irq); You removed this line entirely. Was that on purpose? Having the IRQ printed in dmesg might be handy for debugging. > + if (!spi->irq) { > + dev_err(&spi->dev, "no IRQ specified\n"); > + return -EINVAL; > + } > > - /* Register with the 802154 subsystem */ > + rc = mrf24j40_get_pdata(spi, &rstn, &wake); > + if (rc < 0) { > + dev_err(&spi->dev, "failed to parse platform_data: %d\n", rc); > + return rc; > + } > + > + if (gpio_is_valid(rstn)) { > + rc = devm_gpio_request_one(&spi->dev, rstn, > + GPIOF_OUT_INIT_HIGH, "rstn"); > + if (rc) > + return rc; > + } > > + if (gpio_is_valid(wake)) { > + rc = devm_gpio_request_one(&spi->dev, wake, > + GPIOF_OUT_INIT_LOW, "wake"); > + if (rc) > + return rc; > + } > + > + /* Reset */ > + if (gpio_is_valid(rstn)) { > + udelay(1); > + gpio_set_value(rstn, 0); > + udelay(1); > + gpio_set_value(rstn, 1); > + usleep_range(120, 240); > + } > + > + /* Register with the 802154 subsystem */ > hw = ieee802154_alloc_hw(sizeof(*devrec), &mrf24j40_ops); > if (!hw) > goto err_ret; > @@ -1350,7 +1403,7 @@ static int mrf24j40_probe(struct spi_device *spi) > goto err_register_device; > } > > - dev_dbg(printdev(devrec), "registered mrf24j40\n"); > + dev_info(printdev(devrec), "registered mrf24j40\n"); Not related to the rest of this patch. The change itself is good though. Maybe combine this with IRQ printout from above and have this as an additional patch? > ret = ieee802154_register_hw(devrec->hw); > if (ret) > goto err_register_device; > diff --git a/include/linux/spi/mrf24j40.h b/include/linux/spi/mrf24j40.h > new file mode 100644 > index 000000000000..60810194cae9 > --- /dev/null > +++ b/include/linux/spi/mrf24j40.h > @@ -0,0 +1,9 @@ > +#ifndef _SPI_MRF24J40_H > +#define _SPI_MRF24J40_H > + > +struct mrf24j40_platform_data { > + int rstn; > + int wake; > +}; > + > +#endif /* _SPI_MRF24J40_H */ > Just to clarify this. This header file is needed for the usage of platform data, right? If not I would argue that we do not create a header for this in include/spi but have the struct in the driver itself. I vaguely remember that we need this for the usage of pdata. 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/mrf24j40.txt b/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt index a4ed2efb5b73..2fabba041e48 100644 --- a/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt +++ b/Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt @@ -9,6 +9,10 @@ Required properties: - reg: the chipselect index - interrupts: the interrupt generated by the device. +Optional properties: + - reset-gpio: GPIO spec for the rstn pin + - wake-gpio: GPIO spec for the wake pin + Example: mrf24j40ma@0 { diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c index 7b131f8e4093..275b54d6bad3 100644 --- a/drivers/net/ieee802154/mrf24j40.c +++ b/drivers/net/ieee802154/mrf24j40.c @@ -16,9 +16,11 @@ */ #include <linux/spi/spi.h> +#include <linux/spi/mrf24j40.h> #include <linux/interrupt.h> #include <linux/module.h> #include <linux/regmap.h> +#include <linux/of_gpio.h> #include <linux/ieee802154.h> #include <linux/irq.h> #include <net/cfg802154.h> @@ -1276,16 +1278,67 @@ static void mrf24j40_phy_setup(struct mrf24j40 *devrec) } } +static int +mrf24j40_get_pdata(struct spi_device *spi, int *rstn, int *wake) +{ + struct mrf24j40_platform_data *pdata = spi->dev.platform_data; + + if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) { + if (!pdata) + return -ENOENT; + + *rstn = pdata->rstn; + *wake = pdata->wake; + return 0; + } + + *rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0); + *wake = of_get_named_gpio(spi->dev.of_node, "wake-gpio", 0); + return 0; +} + static int mrf24j40_probe(struct spi_device *spi) { int ret = -ENOMEM, irq_type; + int rc, rstn, wake; struct ieee802154_hw *hw; struct mrf24j40 *devrec; - dev_info(&spi->dev, "probe(). IRQ: %d\n", spi->irq); + if (!spi->irq) { + dev_err(&spi->dev, "no IRQ specified\n"); + return -EINVAL; + } - /* Register with the 802154 subsystem */ + rc = mrf24j40_get_pdata(spi, &rstn, &wake); + if (rc < 0) { + dev_err(&spi->dev, "failed to parse platform_data: %d\n", rc); + return rc; + } + + if (gpio_is_valid(rstn)) { + rc = devm_gpio_request_one(&spi->dev, rstn, + GPIOF_OUT_INIT_HIGH, "rstn"); + if (rc) + return rc; + } + if (gpio_is_valid(wake)) { + rc = devm_gpio_request_one(&spi->dev, wake, + GPIOF_OUT_INIT_LOW, "wake"); + if (rc) + return rc; + } + + /* Reset */ + if (gpio_is_valid(rstn)) { + udelay(1); + gpio_set_value(rstn, 0); + udelay(1); + gpio_set_value(rstn, 1); + usleep_range(120, 240); + } + + /* Register with the 802154 subsystem */ hw = ieee802154_alloc_hw(sizeof(*devrec), &mrf24j40_ops); if (!hw) goto err_ret; @@ -1350,7 +1403,7 @@ static int mrf24j40_probe(struct spi_device *spi) goto err_register_device; } - dev_dbg(printdev(devrec), "registered mrf24j40\n"); + dev_info(printdev(devrec), "registered mrf24j40\n"); ret = ieee802154_register_hw(devrec->hw); if (ret) goto err_register_device; diff --git a/include/linux/spi/mrf24j40.h b/include/linux/spi/mrf24j40.h new file mode 100644 index 000000000000..60810194cae9 --- /dev/null +++ b/include/linux/spi/mrf24j40.h @@ -0,0 +1,9 @@ +#ifndef _SPI_MRF24J40_H +#define _SPI_MRF24J40_H + +struct mrf24j40_platform_data { + int rstn; + int wake; +}; + +#endif /* _SPI_MRF24J40_H */
The mrf24j40 requires a high on the RESET pin, otherwise it will reset itself over and over. With this patch the driver will ensure that a GPIO (if connected) will be set to high during device initialization. Details: - set reset to high on driver probe - make pin configurable via device tree (overlay) - similar to the at86rf233 driver - update devicetree documentation Signed-off-by: smlng <s@mlng.net> --- .../bindings/net/ieee802154/mrf24j40.txt | 4 ++ drivers/net/ieee802154/mrf24j40.c | 59 ++++++++++++++++++++-- include/linux/spi/mrf24j40.h | 9 ++++ 3 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 include/linux/spi/mrf24j40.h