Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
Hi Linus, On 02/03/2017 08:51 AM, Linus Walleij wrote: > Your child nodes I guess will be instatiated as devices as well. > > These devices will have the NBUS driver as .parent in their > struct device I guess. Else the design of this bus is tilted. > > If the NBUS driver use dev_set_drvdata(dev, state_container_cookie) > the children can use dev_get_drvdata(dev->parent); to get a pointer to > the same cookie. > > The subdrivers don't even need to know the members of the state > container as long as you're just passing a pointer to it. It's > enough if you forward-declare it as a "pointer to some struct": > > struct foo; > > { >struct foo *fooptr = dev_get_drvdata(dev->parent); >write(fooptr, 0x10); > > etc Thanks a lot, this is perfect! Best Regards, Sebastien.
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
Hi Linus, On 02/03/2017 08:51 AM, Linus Walleij wrote: > Your child nodes I guess will be instatiated as devices as well. > > These devices will have the NBUS driver as .parent in their > struct device I guess. Else the design of this bus is tilted. > > If the NBUS driver use dev_set_drvdata(dev, state_container_cookie) > the children can use dev_get_drvdata(dev->parent); to get a pointer to > the same cookie. > > The subdrivers don't even need to know the members of the state > container as long as you're just passing a pointer to it. It's > enough if you forward-declare it as a "pointer to some struct": > > struct foo; > > { >struct foo *fooptr = dev_get_drvdata(dev->parent); >write(fooptr, 0x10); > > etc Thanks a lot, this is perfect! Best Regards, Sebastien.
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
On Wed, Feb 1, 2017 at 8:56 PM, Sebastien Bourdelinwrote: > On 12/30/2016 02:58 AM, Linus Walleij wrote: >>> +static struct ts_nbus *ts_nbus; >> >> Nopes. No singletons please. >> >> Use the state container pattern: >> Documentation/driver-model/design-patterns.txt > > I understand the idea but have problem to find a good way to implement it. > > Other drivers using the NBUS which are child nodes in the device tree > will use the ts_nbus_write() and ts_nbus_read() functions, it means these > drivers should have a pointer to the allocated ts_nbus and pass it to > the write() and read() functions as an argument if i'm not using a > singleton here. > But i'm lacking knowledge on how to properly share this pointer when > initializing the NBUS driver with the child nodes. Your child nodes I guess will be instatiated as devices as well. These devices will have the NBUS driver as .parent in their struct device I guess. Else the design of this bus is tilted. If the NBUS driver use dev_set_drvdata(dev, state_container_cookie) the children can use dev_get_drvdata(dev->parent); to get a pointer to the same cookie. The subdrivers don't even need to know the members of the state container as long as you're just passing a pointer to it. It's enough if you forward-declare it as a "pointer to some struct": struct foo; { struct foo *fooptr = dev_get_drvdata(dev->parent); write(fooptr, 0x10); etc Yours, Linus Walleij
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
On Wed, Feb 1, 2017 at 8:56 PM, Sebastien Bourdelin wrote: > On 12/30/2016 02:58 AM, Linus Walleij wrote: >>> +static struct ts_nbus *ts_nbus; >> >> Nopes. No singletons please. >> >> Use the state container pattern: >> Documentation/driver-model/design-patterns.txt > > I understand the idea but have problem to find a good way to implement it. > > Other drivers using the NBUS which are child nodes in the device tree > will use the ts_nbus_write() and ts_nbus_read() functions, it means these > drivers should have a pointer to the allocated ts_nbus and pass it to > the write() and read() functions as an argument if i'm not using a > singleton here. > But i'm lacking knowledge on how to properly share this pointer when > initializing the NBUS driver with the child nodes. Your child nodes I guess will be instatiated as devices as well. These devices will have the NBUS driver as .parent in their struct device I guess. Else the design of this bus is tilted. If the NBUS driver use dev_set_drvdata(dev, state_container_cookie) the children can use dev_get_drvdata(dev->parent); to get a pointer to the same cookie. The subdrivers don't even need to know the members of the state container as long as you're just passing a pointer to it. It's enough if you forward-declare it as a "pointer to some struct": struct foo; { struct foo *fooptr = dev_get_drvdata(dev->parent); write(fooptr, 0x10); etc Yours, Linus Walleij
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
Hi Linus, Thanks for your feedback. I have a question regarding your recommendation, see below. On 12/30/2016 02:58 AM, Linus Walleij wrote: >> + >> +static DEFINE_MUTEX(ts_nbus_lock); >> +static bool ts_nbus_ready; > > Why not move this to the struct ts_nbus state container? > > It seems to be per-bus not per-system. > >> +#define TS_NBUS_READ_MODE 0 >> +#define TS_NBUS_WRITE_MODE 1 >> +#define TS_NBUS_DIRECTION_IN 0 >> +#define TS_NBUS_DIRECTION_OUT 1 >> +#define TS_NBUS_WRITE_ADR 0 >> +#define TS_NBUS_WRITE_VAL 1 >> + >> +struct ts_nbus { >> + struct pwm_device *pwm; >> + int num_data; >> + int *data; >> + int csn; >> + int txrx; >> + int strobe; >> + int ale; >> + int rdy; >> +}; >> + >> +static struct ts_nbus *ts_nbus; > > Nopes. No singletons please. > > Use the state container pattern: > Documentation/driver-model/design-patterns.txt > I understand the idea but have problem to find a good way to implement it. Other drivers using the NBUS which are child nodes in the device tree will use the ts_nbus_write() and ts_nbus_read() functions, it means these drivers should have a pointer to the allocated ts_nbus and pass it to the write() and read() functions as an argument if i'm not using a singleton here. But i'm lacking knowledge on how to properly share this pointer when initializing the NBUS driver with the child nodes. Perhaps my design is not appropriate for what i'm doing, if someone can point me on a similar problematic it will be really helpful. Best Regards, Sebastien.
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
Hi Linus, Thanks for your feedback. I have a question regarding your recommendation, see below. On 12/30/2016 02:58 AM, Linus Walleij wrote: >> + >> +static DEFINE_MUTEX(ts_nbus_lock); >> +static bool ts_nbus_ready; > > Why not move this to the struct ts_nbus state container? > > It seems to be per-bus not per-system. > >> +#define TS_NBUS_READ_MODE 0 >> +#define TS_NBUS_WRITE_MODE 1 >> +#define TS_NBUS_DIRECTION_IN 0 >> +#define TS_NBUS_DIRECTION_OUT 1 >> +#define TS_NBUS_WRITE_ADR 0 >> +#define TS_NBUS_WRITE_VAL 1 >> + >> +struct ts_nbus { >> + struct pwm_device *pwm; >> + int num_data; >> + int *data; >> + int csn; >> + int txrx; >> + int strobe; >> + int ale; >> + int rdy; >> +}; >> + >> +static struct ts_nbus *ts_nbus; > > Nopes. No singletons please. > > Use the state container pattern: > Documentation/driver-model/design-patterns.txt > I understand the idea but have problem to find a good way to implement it. Other drivers using the NBUS which are child nodes in the device tree will use the ts_nbus_write() and ts_nbus_read() functions, it means these drivers should have a pointer to the allocated ts_nbus and pass it to the write() and read() functions as an argument if i'm not using a singleton here. But i'm lacking knowledge on how to properly share this pointer when initializing the NBUS driver with the child nodes. Perhaps my design is not appropriate for what i'm doing, if someone can point me on a similar problematic it will be really helpful. Best Regards, Sebastien.
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
On Thu, Dec 15, 2016 at 12:12 AM, Sebastien Bourdelinwrote: > This driver implements a GPIOs bit-banged bus, called the NBUS by > Technologic Systems. It is used to communicate with the peripherals in > the FPGA on the TS-4600 SoM. > > Signed-off-by: Sebastien Bourdelin (...) > +#include Use: #include instead, and deal with the fallout. > +#include > +#include > +#include > +#include > +#include Don't use this. > +#include > +#include > + > +static DEFINE_MUTEX(ts_nbus_lock); > +static bool ts_nbus_ready; Why not move this to the struct ts_nbus state container? It seems to be per-bus not per-system. > +#define TS_NBUS_READ_MODE 0 > +#define TS_NBUS_WRITE_MODE 1 > +#define TS_NBUS_DIRECTION_IN 0 > +#define TS_NBUS_DIRECTION_OUT 1 > +#define TS_NBUS_WRITE_ADR 0 > +#define TS_NBUS_WRITE_VAL 1 > + > +struct ts_nbus { > + struct pwm_device *pwm; > + int num_data; > + int *data; > + int csn; > + int txrx; > + int strobe; > + int ale; > + int rdy; > +}; > + > +static struct ts_nbus *ts_nbus; Nopes. No singletons please. Use the state container pattern: Documentation/driver-model/design-patterns.txt > +/* > + * request all gpios required by the bus. > + */ > +static int ts_nbus_init(struct platform_device *pdev) > +{ > + int err; > + int i; > + > + for (i = 0; i < ts_nbus->num_data; i++) { > + err = devm_gpio_request_one(>dev, ts_nbus->data[i], > + GPIOF_OUT_INIT_HIGH, > + "TS NBUS data"); > + if (err) > + return err; > + } DO not use the legacy GPIO API anywhere. Reference the device and use gpiod_get() simple, fair and square. Store struct gpio_desc * pointers in your state container instead. > +static int ts_nbus_get_of_pdata(struct device *dev, struct device_node *np) > +{ > + int num_data; > + int *data; > + int ret; > + int i; > + > + ret = of_gpio_named_count(np, "data-gpios"); > + if (ret < 0) { > + dev_err(dev, > + "failed to count GPIOs in DT property data-gpios\n"); > + return ret; > + } Do not reinvent the wheel. Use devm_gpiod_get_array(). > + ret = of_get_named_gpio(np, "csn-gpios", 0); And again use devm_gpiod_get(), and gpiod_* accessors. Applies everywhere. Yours, Linus Walleij
Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS
On Thu, Dec 15, 2016 at 12:12 AM, Sebastien Bourdelin wrote: > This driver implements a GPIOs bit-banged bus, called the NBUS by > Technologic Systems. It is used to communicate with the peripherals in > the FPGA on the TS-4600 SoM. > > Signed-off-by: Sebastien Bourdelin (...) > +#include Use: #include instead, and deal with the fallout. > +#include > +#include > +#include > +#include > +#include Don't use this. > +#include > +#include > + > +static DEFINE_MUTEX(ts_nbus_lock); > +static bool ts_nbus_ready; Why not move this to the struct ts_nbus state container? It seems to be per-bus not per-system. > +#define TS_NBUS_READ_MODE 0 > +#define TS_NBUS_WRITE_MODE 1 > +#define TS_NBUS_DIRECTION_IN 0 > +#define TS_NBUS_DIRECTION_OUT 1 > +#define TS_NBUS_WRITE_ADR 0 > +#define TS_NBUS_WRITE_VAL 1 > + > +struct ts_nbus { > + struct pwm_device *pwm; > + int num_data; > + int *data; > + int csn; > + int txrx; > + int strobe; > + int ale; > + int rdy; > +}; > + > +static struct ts_nbus *ts_nbus; Nopes. No singletons please. Use the state container pattern: Documentation/driver-model/design-patterns.txt > +/* > + * request all gpios required by the bus. > + */ > +static int ts_nbus_init(struct platform_device *pdev) > +{ > + int err; > + int i; > + > + for (i = 0; i < ts_nbus->num_data; i++) { > + err = devm_gpio_request_one(>dev, ts_nbus->data[i], > + GPIOF_OUT_INIT_HIGH, > + "TS NBUS data"); > + if (err) > + return err; > + } DO not use the legacy GPIO API anywhere. Reference the device and use gpiod_get() simple, fair and square. Store struct gpio_desc * pointers in your state container instead. > +static int ts_nbus_get_of_pdata(struct device *dev, struct device_node *np) > +{ > + int num_data; > + int *data; > + int ret; > + int i; > + > + ret = of_gpio_named_count(np, "data-gpios"); > + if (ret < 0) { > + dev_err(dev, > + "failed to count GPIOs in DT property data-gpios\n"); > + return ret; > + } Do not reinvent the wheel. Use devm_gpiod_get_array(). > + ret = of_get_named_gpio(np, "csn-gpios", 0); And again use devm_gpiod_get(), and gpiod_* accessors. Applies everywhere. Yours, Linus Walleij
[PATCH 4/6] bus: add driver for the Technologic Systems NBUS
This driver implements a GPIOs bit-banged bus, called the NBUS by Technologic Systems. It is used to communicate with the peripherals in the FPGA on the TS-4600 SoM. Signed-off-by: Sebastien Bourdelin--- drivers/bus/Kconfig | 9 + drivers/bus/Makefile| 1 + drivers/bus/ts-nbus.c | 451 include/linux/ts-nbus.h | 17 ++ 4 files changed, 478 insertions(+) create mode 100644 drivers/bus/ts-nbus.c create mode 100644 include/linux/ts-nbus.h diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 7875105..74e72b3 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -150,6 +150,15 @@ config TEGRA_ACONNECT Driver for the Tegra ACONNECT bus which is used to interface with the devices inside the Audio Processing Engine (APE) for Tegra210. +config TS_NBUS + tristate "Technologic Systems NBUS Driver" + default y + depends on SOC_IMX28 + depends on OF_GPIO && PWM + help + Driver for the Technologic Systems NBUS which is used to interface + with the peripherals in the FPGA of the TS-4600 SoM. + config UNIPHIER_SYSTEM_BUS tristate "UniPhier System Bus driver" depends on ARCH_UNIPHIER && OF diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index c6cfa6b..83f874a 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -19,5 +19,6 @@ obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o obj-$(CONFIG_SUNXI_RSB)+= sunxi-rsb.o obj-$(CONFIG_SIMPLE_PM_BUS)+= simple-pm-bus.o obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o +obj-$(CONFIG_TS_NBUS) += ts-nbus.o obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c new file mode 100644 index 000..44fc89d --- /dev/null +++ b/drivers/bus/ts-nbus.c @@ -0,0 +1,451 @@ +/* + * NBUS driver for TS-4600 based boards + * + * Copyright (c) 2016 - Savoir-faire Linux + * Author: Sebastien Bourdelin + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + * + * This driver implements a GPIOs bit-banged bus, called the NBUS by Technologic + * Systems. It is used to communicate with the peripherals in the FPGA on the + * TS-4600 SoM. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +static DEFINE_MUTEX(ts_nbus_lock); +static bool ts_nbus_ready; + +#define TS_NBUS_READ_MODE 0 +#define TS_NBUS_WRITE_MODE 1 +#define TS_NBUS_DIRECTION_IN 0 +#define TS_NBUS_DIRECTION_OUT 1 +#define TS_NBUS_WRITE_ADR 0 +#define TS_NBUS_WRITE_VAL 1 + +struct ts_nbus { + struct pwm_device *pwm; + int num_data; + int *data; + int csn; + int txrx; + int strobe; + int ale; + int rdy; +}; + +static struct ts_nbus *ts_nbus; + +/* + * request all gpios required by the bus. + */ +static int ts_nbus_init(struct platform_device *pdev) +{ + int err; + int i; + + for (i = 0; i < ts_nbus->num_data; i++) { + err = devm_gpio_request_one(>dev, ts_nbus->data[i], + GPIOF_OUT_INIT_HIGH, + "TS NBUS data"); + if (err) + return err; + } + + err = devm_gpio_request_one(>dev, ts_nbus->csn, + GPIOF_OUT_INIT_HIGH, + "TS NBUS csn"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->txrx, + GPIOF_OUT_INIT_HIGH, + "TS NBUS txrx"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->strobe, + GPIOF_OUT_INIT_HIGH, + "TS NBUS strobe"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->ale, + GPIOF_OUT_INIT_HIGH, + "TS NBUS ale"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->rdy, + GPIOF_IN, + "TS NBUS rdy"); + if (err) + return err; + + return 0; +} + +/* + * retrieve all gpios used by the bus from the device tree. + */ +static int ts_nbus_get_of_pdata(struct device *dev, struct device_node *np) +{ + int num_data; + int *data; + int ret; + int i; + + ret = of_gpio_named_count(np, "data-gpios"); + if (ret < 0) { + dev_err(dev, +
[PATCH 4/6] bus: add driver for the Technologic Systems NBUS
This driver implements a GPIOs bit-banged bus, called the NBUS by Technologic Systems. It is used to communicate with the peripherals in the FPGA on the TS-4600 SoM. Signed-off-by: Sebastien Bourdelin --- drivers/bus/Kconfig | 9 + drivers/bus/Makefile| 1 + drivers/bus/ts-nbus.c | 451 include/linux/ts-nbus.h | 17 ++ 4 files changed, 478 insertions(+) create mode 100644 drivers/bus/ts-nbus.c create mode 100644 include/linux/ts-nbus.h diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 7875105..74e72b3 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -150,6 +150,15 @@ config TEGRA_ACONNECT Driver for the Tegra ACONNECT bus which is used to interface with the devices inside the Audio Processing Engine (APE) for Tegra210. +config TS_NBUS + tristate "Technologic Systems NBUS Driver" + default y + depends on SOC_IMX28 + depends on OF_GPIO && PWM + help + Driver for the Technologic Systems NBUS which is used to interface + with the peripherals in the FPGA of the TS-4600 SoM. + config UNIPHIER_SYSTEM_BUS tristate "UniPhier System Bus driver" depends on ARCH_UNIPHIER && OF diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index c6cfa6b..83f874a 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -19,5 +19,6 @@ obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o obj-$(CONFIG_SUNXI_RSB)+= sunxi-rsb.o obj-$(CONFIG_SIMPLE_PM_BUS)+= simple-pm-bus.o obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o +obj-$(CONFIG_TS_NBUS) += ts-nbus.o obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c new file mode 100644 index 000..44fc89d --- /dev/null +++ b/drivers/bus/ts-nbus.c @@ -0,0 +1,451 @@ +/* + * NBUS driver for TS-4600 based boards + * + * Copyright (c) 2016 - Savoir-faire Linux + * Author: Sebastien Bourdelin + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + * + * This driver implements a GPIOs bit-banged bus, called the NBUS by Technologic + * Systems. It is used to communicate with the peripherals in the FPGA on the + * TS-4600 SoM. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +static DEFINE_MUTEX(ts_nbus_lock); +static bool ts_nbus_ready; + +#define TS_NBUS_READ_MODE 0 +#define TS_NBUS_WRITE_MODE 1 +#define TS_NBUS_DIRECTION_IN 0 +#define TS_NBUS_DIRECTION_OUT 1 +#define TS_NBUS_WRITE_ADR 0 +#define TS_NBUS_WRITE_VAL 1 + +struct ts_nbus { + struct pwm_device *pwm; + int num_data; + int *data; + int csn; + int txrx; + int strobe; + int ale; + int rdy; +}; + +static struct ts_nbus *ts_nbus; + +/* + * request all gpios required by the bus. + */ +static int ts_nbus_init(struct platform_device *pdev) +{ + int err; + int i; + + for (i = 0; i < ts_nbus->num_data; i++) { + err = devm_gpio_request_one(>dev, ts_nbus->data[i], + GPIOF_OUT_INIT_HIGH, + "TS NBUS data"); + if (err) + return err; + } + + err = devm_gpio_request_one(>dev, ts_nbus->csn, + GPIOF_OUT_INIT_HIGH, + "TS NBUS csn"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->txrx, + GPIOF_OUT_INIT_HIGH, + "TS NBUS txrx"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->strobe, + GPIOF_OUT_INIT_HIGH, + "TS NBUS strobe"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->ale, + GPIOF_OUT_INIT_HIGH, + "TS NBUS ale"); + if (err) + return err; + + err = devm_gpio_request_one(>dev, ts_nbus->rdy, + GPIOF_IN, + "TS NBUS rdy"); + if (err) + return err; + + return 0; +} + +/* + * retrieve all gpios used by the bus from the device tree. + */ +static int ts_nbus_get_of_pdata(struct device *dev, struct device_node *np) +{ + int num_data; + int *data; + int ret; + int i; + + ret = of_gpio_named_count(np, "data-gpios"); + if (ret < 0) { + dev_err(dev, + "failed to count GPIOs in DT property data-gpios\n"); +