linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
@ 2018-10-25 15:20 thesven73
  2018-10-30  9:55 ` Linus Walleij
  0 siblings, 1 reply; 6+ messages in thread
From: thesven73 @ 2018-10-25 15:20 UTC (permalink / raw)
  To: svendev, lee.jones, robh+dt, mark.rutland, afaerber, treding,
	david, noralf, johan, monstr, michal.vokac, arnd, gregkh,
	john.garry, geert+renesas, robin.murphy, paul.gortmaker,
	sebastien.bourdelin, icenowy, yuanzhichang, stuyoder,
	linus.walleij, maxime.ripard, bogdan.purcareata
  Cc: linux-kernel, devicetree

Hi Linus, thanks a million for the detailed patch review, you've given this
patch a lot more attention than I was expecting. Great !!

> What you need to add is a bit of how the driver is architected.

I agree. Written once, read/maintained 100s of times, right?

> This is really needed because the driver is starting threads and
> running completions and tasks and whatnot to the left and
> right, and when random people come in to maintain this code they
> will be puzzled. 

I had originally architected this driver to be much simpler, with everything
running in the context of the userspace threads (except obviously the
interrupt). But it stood zero chance, the presence of the soft interrupt + the
h/w requirement to lock/unlock the region when acking the soft interrupt meant
that there were circular locking dependencies that always resulted in
deadlock :(

This single-thread message-queue architecture is harder to understand,
but much easier and safer in terms of synchronization.

I find it hard myself to keep track of functions that run in userland thread
context, and those that run in the kernel thread. Should I prefix kernel thread
functions with kt_* just to keep them apart?

> also "buss" is a germanism isn't it?  It should be just "anybus"?

There are several different types of anybus:
Anybus-M
Anybus-S
Anybus-CompactCOM

This driver implements Anybus-S only. I had originally prefixed files and
functions with anybus-s and anybus_s respectively, but it looked horrible
visually:

struct anybus_s_host *cd = data;
drivers/bus/anybus-s-host.c
include/linux/anybus-s-client.h

I'm completely open to suggestions on this one.
anybuss?
anybus-s?
just anybus?

>> +static irqreturn_t irq_handler(int irq, void *data)
>> +{
>> +       struct anybuss_host *cd = data;
>> +       int ind_ab;
>> +
>> +       /* reading the anybus indicator register acks the interrupt */
>> +       ind_ab = read_ind_ab(cd->regmap);
>> +       if (ind_ab < 0)
>> +               return IRQ_NONE;
>> +       atomic_set(&cd->ind_ab, ind_ab);
>> +       complete(&cd->card_boot);
>> +       wake_up(&cd->wq);
>> +       return IRQ_HANDLED;
>> +}

> It looks a bit like you reinvent threaded interrupts but enlighten me
> on the architecture and I might be able to get it.

HMS Industrial Networks designed the anybus interrupt line to be dual purpose.
In addition to being a 'real' interrupt during normal operation, it also signals
that the card has initialized when coming out of reset. As this is a one-time
thing, it needs a 'struct completion', not a wait_queue.

It is of course possible to emulate a struct completion using a waitqueue and
an atomic variable, but wasn't struct completion invented to eliminate the
subtle dangers of this?

So this is why the irq_handler has to call both complete() and wake_up(), so
it can't be modelled by a threaded interrupt.

Maybe if we use two separate irq_handlers: put the first one in place during
initialization, then when the card is initialized, remove it and put a threaded
one in place? Would this be a bit too complicated?

> This looks complex. Why can't you just sleep() and then
> retry this instead of shuffleing around different "tasks"?
> Are you actually modeling a state machine? In that case
> I can kind of understand it, then you just need one
> thread/work and assign it an enum of states or
> something, maybe name that "state" rather than task
> so we see what is going on.

Yes, I am modeling a state machine.
When userspace asks the anybus to do something, it throws a task into a queue,
and waits on the completion of that task.
The anybus processes the tasks sequentially, each task will go through
multiple states before completing:

userspace processes 	A	B	C
			|	|	|
			v	v	v
			-----------------
			| task waiting	|
			| task waiting	|
			| task waiting	|
			|---------------|
			| task running	|
			-----------------
				^
				|
			-----------------
			| anybus process |
			| single-thread	 |
			| h/w access	 |
			------------------

There is only one single kernel thread that accesses the hardware and the queue.
This prevents various subtle synchronization/deadlock issues related to the
soft interrupts.

The tasks change state by re-assigning their own task_fn callback:

function do_state_1(self) {
	...
	if (need state 2)
		self->task_fn = do_state_2;
}

function do_state_2(self) {
	...
	if (need_state_1)
		self->task_fn = do_state_1;
}

I could have opted for a classic state machine in a single callback:

function do_state(self) {
	switch (self->state) {
	case state_1:
		...
		if (need_state_2)
			self->state = state_2;
		break;
	case state_2:
		...
		if (need_state_1)
			self->state = state_1;
		break;
	}
}

But the former seemed easier to understand.
Obviously it's more important that it's easy to understand not to me, but to
most developers who read the code. So tell me if the callback approach is
too exotic.

>> +static void softint_ack(struct anybuss_host *cd)
>> +static void process_softint(struct anybuss_host *cd)
>
> This looks like MSI (message signalled interrupt) and makes me think
> that you should probably involve the irqchip maintainers. Interrupts
> shall be represented in the irqchip abstraction.

This is not a *real* software interrupt - it's just a bit in an internal
register that gets set by the anybus on a certain condition, and needs
to be ACKed. When the bit is set, the bus driver then tells userspace
about it - anyone who is running poll/select on the sysfs node or devnode.

The anybus documentation calls this a 'software interrupt'.

>> +       cd->reset = pdata->reset;
>
> This callback thing to handle reset doesn't seem right.

I agree, and I've gone through the exact same thought process before.

Right now I'm using platform_data for the bus driver's dependencies:
(the irq is passed out-of-band, via platform_get_resource())

+/**
+ * Platform data of the Anybus-S host controller.
+ *
+ * @regmap: provides access to the card dpram.
+ *		MUST NOT use caching
+ *		MUST NOT sleep
+ * @reset:  controls the card reset line.
+ */
+struct anybuss_host_pdata {
+	struct regmap *regmap;
+	anybuss_reset_t reset;
+};

Now imagine that the underlying anybus bridge is defined as a reset controller.
I could not find a way to pass a reset controller handle through platform_data.
It seemed possible via the devicetree only. I was developing on 4.9 at the time.

So what if we pass the dependencies not via platform_data, but via the
devicetree? In that case, I cannot find a way to pass the struct regmap
via the devicetree...

Wait... are you talking about
reset_controller_add_lookup() / devm_reset_control_get_exclusive() ?
That's new to me, and only used in a single driver right now. Would that work?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
  2018-10-25 15:20 [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus thesven73
@ 2018-10-30  9:55 ` Linus Walleij
  2018-10-30 13:50   ` thesven73
  0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2018-10-30  9:55 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: svendev, Lee Jones, Rob Herring, Mark Rutland,
	Andreas Färber, Thierry Reding, David Lechner,
	Noralf Trønnes, Johan Hovold, Michal Simek, michal.vokac,
	Arnd Bergmann, Greg KH, john.garry, Geert Uytterhoeven,
	Robin Murphy, Paul Gortmaker, Sebastien Bourdelin, Icenowy Zheng,
	yuanzhichang, Stuart Yoder, Maxime Ripard, bogdan.purcareata,
	linux-kernel,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Sven,

some tries to answer questions... (I am no expert in this but
I try my best)

On Thu, Oct 25, 2018 at 5:21 PM <thesven73@gmail.com> wrote:

> I had originally architected this driver to be much simpler, with everything
> running in the context of the userspace threads (except obviously the
> interrupt). But it stood zero chance, the presence of the soft interrupt + the
> h/w requirement to lock/unlock the region when acking the soft interrupt meant
> that there were circular locking dependencies that always resulted in
> deadlock :(

I think the kernel should run all hardware drivers so IMO you did the
right thing.

> > also "buss" is a germanism isn't it?  It should be just "anybus"?
>
> There are several different types of anybus:
> Anybus-M
> Anybus-S
> Anybus-CompactCOM
>
> This driver implements Anybus-S only. I had originally prefixed files and
> functions with anybus-s and anybus_s respectively, but it looked horrible
> visually:
>
> struct anybus_s_host *cd = data;
> drivers/bus/anybus-s-host.c
> include/linux/anybus-s-client.h

Hm I think this looks pretty neat actually. Anyways, in the overall
architecture explain the three Anybus:es and why things pertaining
to Anybus-s are named as they are.

> >> +static irqreturn_t irq_handler(int irq, void *data)
> >> +{
> >> +       struct anybuss_host *cd = data;
> >> +       int ind_ab;
> >> +
> >> +       /* reading the anybus indicator register acks the interrupt */
> >> +       ind_ab = read_ind_ab(cd->regmap);
> >> +       if (ind_ab < 0)
> >> +               return IRQ_NONE;
> >> +       atomic_set(&cd->ind_ab, ind_ab);
> >> +       complete(&cd->card_boot);
> >> +       wake_up(&cd->wq);
> >> +       return IRQ_HANDLED;
> >> +}
>
> > It looks a bit like you reinvent threaded interrupts but enlighten me
> > on the architecture and I might be able to get it.
>
> HMS Industrial Networks designed the anybus interrupt line to be dual purpose.
> In addition to being a 'real' interrupt during normal operation, it also signals
> that the card has initialized when coming out of reset. As this is a one-time
> thing, it needs a 'struct completion', not a wait_queue.

OK but you also have an atomic_set() going on and the struct completion
already contains a waitqueue and I start to worry about overuse of
primitives here. How does ps aux look on your system when running this?

> It is of course possible to emulate a struct completion using a waitqueue and
> an atomic variable, but wasn't struct completion invented to eliminate the
> subtle dangers of this?

The completion is a waitqueue entry and a spinlock, essentially.
But you're right, if you're waiting in process context for something
to happen, such as an ack interrupt for something you initiated,
a completion is the right abstraction to use.

> So this is why the irq_handler has to call both complete() and wake_up(), so
> it can't be modelled by a threaded interrupt.

It's just that when you do things like this:

   complete(&cd->card_boot);
   wake_up(&cd->wq);

I start asking: OK so why is that waitqueue not woken up from
wherever you are doing wait_for_completion()? I hope it is not
because you are waiting for the completion in the very same
waitqueue. That would be really messy.

So I guess what we like is clear program flow, as easy as possible
to follow what happens in the driver.

> Maybe if we use two separate irq_handlers: put the first one in place during
> initialization, then when the card is initialized, remove it and put a threaded
> one in place? Would this be a bit too complicated?

Nah one irq handler is fine, but you can have an optional
bottom half:

request_threaded_irq(irq, fastpath, slowpath...):

From fastpath you return IRQ_WAKE_THREAD only
if you want to continue running the slowpath callback
in process context, else just complete() and
return IRQ_HANDLED and the slowpath thread will not be
executed.

> Yes, I am modeling a state machine.
> When userspace asks the anybus to do something, it throws a task into a queue,
> and waits on the completion of that task.
> The anybus processes the tasks sequentially, each task will go through
> multiple states before completing:
>
> userspace processes     A       B       C
>                         |       |       |
>                         v       v       v
>                         -----------------
>                         | task waiting  |
>                         | task waiting  |
>                         | task waiting  |
>                         |---------------|
>                         | task running  |
>                         -----------------
>                                 ^
>                                 |
>                         -----------------
>                         | anybus process |
>                         | single-thread  |
>                         | h/w access     |
>                         ------------------
>
> There is only one single kernel thread that accesses the hardware and the queue.
> This prevents various subtle synchronization/deadlock issues related to the
> soft interrupts.

This should all go into the comment section on the top of the
C file so we understand what is going on :)
(Good explanation BTW.)

> The tasks change state by re-assigning their own task_fn callback:
>
> function do_state_1(self) {
>         ...
>         if (need state 2)
>                 self->task_fn = do_state_2;
> }
>
> function do_state_2(self) {
>         ...
>         if (need_state_1)
>                 self->task_fn = do_state_1;
> }
>
> I could have opted for a classic state machine in a single callback:
>
> function do_state(self) {
>         switch (self->state) {
>         case state_1:
>                 ...
>                 if (need_state_2)
>                         self->state = state_2;
>                 break;
>         case state_2:
>                 ...
>                 if (need_state_1)
>                         self->state = state_1;
>                 break;
>         }
> }
>
> But the former seemed easier to understand.

OK I honestly don't know what is the best way. But what about
calling that "task" a "state" instead so we know what is going
on (i.e. you are switching between different states in process
context).

> >> +static void softint_ack(struct anybuss_host *cd)
> >> +static void process_softint(struct anybuss_host *cd)
> >
> > This looks like MSI (message signalled interrupt) and makes me think
> > that you should probably involve the irqchip maintainers. Interrupts
> > shall be represented in the irqchip abstraction.
>
> This is not a *real* software interrupt - it's just a bit in an internal
> register that gets set by the anybus on a certain condition, and needs
> to be ACKed. When the bit is set, the bus driver then tells userspace
> about it - anyone who is running poll/select on the sysfs node or devnode.
>
> The anybus documentation calls this a 'software interrupt'.

OK I get it... I think.

Silly question but why does userspace need to know about
this software interrupt? Can't you just hide that?
Userspace ABIs should be minimal.

> Right now I'm using platform_data for the bus driver's dependencies:
> (the irq is passed out-of-band, via platform_get_resource())
(...)
> +/**
> + * Platform data of the Anybus-S host controller.
> + *
> + * @regmap: provides access to the card dpram.
> + *             MUST NOT use caching
> + *             MUST NOT sleep
> + * @reset:  controls the card reset line.
> + */
> +struct anybuss_host_pdata {
> +       struct regmap *regmap;
> +       anybuss_reset_t reset;
> +};
>
> Now imagine that the underlying anybus bridge is defined as a reset controller.
> I could not find a way to pass a reset controller handle through platform_data.
> It seemed possible via the devicetree only. I was developing on 4.9 at the time.

I don't get the question. The reset controller is a provider just
like a clock controller or regulator. You look up the resource
associated with your device.

The platform_data is surely associated with a struct device *
and the reset controller handle should be associated with the
same struct device* and obtained with
[devm_]reset_control_get().

If you mean that you can't figure out how to associate a reset
controller handle with a device in the first place, then we need
to fix the reset controller subsystem to provide a mechanism for
that if there is not one already, not work around its shortcomings.
But reset_controller_add_lookup() seems to do what you want.
See for example drivers/clk/davinci/psc-da850.c
for an example of how to use that.

> So what if we pass the dependencies not via platform_data, but via the
> devicetree? In that case, I cannot find a way to pass the struct regmap
> via the devicetree...

The abstraction you associate resources with is struct device *
not device tree (nodes).

When I pass struct regmap to subdrivers I usually define a struct
that I pass in driver_data one way or another.
Sometimes I look up the driver_data of a parent by just
walking the ->parent hierarchy in struct device.

> Wait... are you talking about
> reset_controller_add_lookup() / devm_reset_control_get_exclusive() ?
> That's new to me, and only used in a single driver right now. Would that work?

Yeah I think so.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
  2018-10-30  9:55 ` Linus Walleij
@ 2018-10-30 13:50   ` thesven73
  0 siblings, 0 replies; 6+ messages in thread
From: thesven73 @ 2018-10-30 13:50 UTC (permalink / raw)
  To: svendev, lee.jones, robh+dt, mark.rutland, afaerber, treding,
	david, noralf, johan, monstr, michal.vokac, arnd, gregkh,
	john.garry, geert+renesas, robin.murphy, paul.gortmaker,
	sebastien.bourdelin, icenowy, stuyoder, linus.walleij,
	maxime.ripard
  Cc: linux-kernel, devicetree

>> struct anybus_s_host *cd = data;
>> drivers/bus/anybus-s-host.c
>> include/linux/anybus-s-client.h
>
> Hm I think this looks pretty neat actually. Anyways, in the overall
> architecture explain the three Anybus:es and why things pertaining
> to Anybus-s are named as they are.

Ok, so should I rename anybuss -> anybus[_|-]s ?
Or maybe wait until a future patch iteration?

> OK but you also have an atomic_set() going on and the struct completion
> already contains a waitqueue and I start to worry about overuse of
> primitives here. How does ps aux look on your system when running this?

ps -aux only lists a single kernel thread associated with this driver,
no other theads, as it should:

$ ps
...
   78 root     [anybuss-host.0.]
...

> It's just that when you do things like this:
> 
>    complete(&cd->card_boot);
>    wake_up(&cd->wq);
>
> I start asking: OK so why is that waitqueue not woken up from
> wherever you are doing wait_for_completion()? I hope it is not
> because you are waiting for the completion in the very same
> waitqueue. That would be really messy.

The two primitives are there because the irq handler is dual purpose, and that
is because the interrupt is dual purpose:
- complete card boot while in the initialization stage
- tell the driver that something has happened while running

You may have a point about the atomic_set(). I'll think about why it's there,
and move it out if unnecessary. Or document why we need it there.

> Nah one irq handler is fine, but you can have an optional
> bottom half:
>
> request_threaded_irq(irq, fastpath, slowpath...):

I don't think any of these abstractions fit :(
Let's wait for the v2 patch with the architecture doc, and you will see why I
believe that's the case. We can then take the discussion from there.

> OK I honestly don't know what is the best way. But what about
> calling that "task" a "state" instead so we know what is going
> on (i.e. you are switching between different states in process
> context).

Let's wait for the architecture docs in the v2 patch.

> Silly question but why does userspace need to know about
> this software interrupt? Can't you just hide that?
> Userspace ABIs should be minimal.

I agree, but in this case the "s/w interrupt" indicates to the user that
someone on the network changed the contents of the card's internal memory.

A client application using this driver will typically sit in a loop, running
poll()/select() on this driver's devnode, waking up when someone modifies
the internal memory.

Example, imagine the card's internal memory is 10 bytes in size.
The internal memory is exposed over the network, and anyone may read
or change it.

|T.h.e. .Q.u.i.c.k]	original contents
				read("/dev/profinet0", 10) => "The Quick"
				poll("/dev/profinet0", PRI|ERR) blocks
|T.h.X. .Q.u.i.c.k]	someone on the network changed position 2, interrupt !
				poll("/dev/profinet0", PRI|ERR) releases
				read("/dev/profinet0", 10) => "ThX Quick"	

Perhaps there is a better / clearer abstraction to accomplish this?

>> Wait... are you talking about
>> reset_controller_add_lookup() / devm_reset_control_get_exclusive() ?
>> That's new to me, and only used in a single driver right now. Would that work?
>
> Yeah I think so.

Good ! I'll put that mechanism in v2, which should be ready soon.

Yours,
Sven

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
  2018-10-24 14:24 ` [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus Sven Van Asbroeck
  2018-10-24 15:58   ` Randy Dunlap
@ 2018-10-25 11:08   ` Linus Walleij
  1 sibling, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2018-10-25 11:08 UTC (permalink / raw)
  To: svendev
  Cc: Lee Jones, Rob Herring, Mark Rutland, Andreas Färber,
	Thierry Reding, David Lechner, Noralf Trønnes, Johan Hovold,
	Michal Simek, michal.vokac, Arnd Bergmann, Greg KH, john.garry,
	Andy Shevchenko, Geert Uytterhoeven, Robin Murphy,
	Paul Gortmaker, Sebastien Bourdelin, Icenowy Zheng, yuanzhichang,
	Stuart Yoder, Maxime Ripard, bogdan.purcareata, linux-kernel,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Sven,

thanks for your patch!

On Wed, Oct 24, 2018 at 4:25 PM Sven Van Asbroeck <svendev@arcx.com> wrote:

> This driver implementation is designed to be almost completely independent
> from the way the Anybus-S hardware is accessed by the SoC. All it needs is:
>
> - a regmap which accesses the underlying Anybus-S dpram by whatever means;
> - an interrupt line, ultimately connected to the Anybus-S card;
> - a reset function.

Overall this commit message is a good start! You explain what this thing is
and what it does.

What you need to add is a bit of how the driver is architected. That can also
be added as comment in the header of the driver file, maybe that is even
better, i.e. here:

+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HMS Anybus-S Host Driver

<architecture description goes here>

This is really needed because the driver is starting threads and
running completions and tasks and whatnot to the left and
right, and when random people come in to maintain this code they
will be puzzled. You need an overarching description of how the
driver is constructed here.

Please add proper kerneldoc to the struct anybus_host
also "buss" is a germanism isn't it?  It should be just "anybus"?

> +struct anybuss_host {
> +       struct device *dev;
> +       struct device *parent;
> +       struct anybuss_client *client;

There as well?

Just search/replace "s/buss/bus/g" everywhere I suspect.

> +static irqreturn_t irq_handler(int irq, void *data)
> +{
> +       struct anybuss_host *cd = data;
> +       int ind_ab;
> +
> +       /* reading the anybus indicator register acks the interrupt */
> +       ind_ab = read_ind_ab(cd->regmap);
> +       if (ind_ab < 0)
> +               return IRQ_NONE;
> +       atomic_set(&cd->ind_ab, ind_ab);
> +       complete(&cd->card_boot);
> +       wake_up(&cd->wq);
> +       return IRQ_HANDLED;
> +}

It looks a bit like you reinvent threaded interrupts but enlighten me
on the architecture and I might be able to get it.

> +static int task_fn_power_on_2(struct anybuss_host *cd,
> +                               struct ab_task *t)
> +{
> +       if (completion_done(&cd->card_boot)) {
> +               cd->power_on = true;
> +               return 0;
> +       }
> +       if (time_after(jiffies, t->start_jiffies + TIMEOUT)) {
> +               disable_irq(cd->irq);
> +               ab_reset(cd, true);
> +               dev_err(cd->dev, "power on timed out");
> +               return -ETIMEDOUT;
> +       }
> +       return -EINPROGRESS;
> +}
> +
> +static int task_fn_power_on(struct anybuss_host *cd,
> +                               struct ab_task *t)
> +{
> +       unsigned int dummy;
> +
> +       if (cd->power_on)
> +               return 0;
> +       /* anybus docs: prevent false 'init done' interrupt by
> +        * doing a dummy read of IND_AB register while in reset.
> +        */
> +       regmap_read(cd->regmap, REG_IND_AB, &dummy);
> +       reinit_completion(&cd->card_boot);
> +       enable_irq(cd->irq);
> +       ab_reset(cd, false);
> +       t->task_fn = task_fn_power_on_2;
> +       return -EINPROGRESS;
> +}

This looks complex. Why can't you just sleep() and then
retry this instead of shuffleing around different "tasks"?
Are you actually modeling a state machine? In that case
I can kind of understand it, then you just need one
thread/work and assign it an enum of states or
something, maybe name that "state" rather than task
so we see what is going on.

> +static int task_fn_area_3(struct anybuss_host *cd, struct ab_task *t)
> +static int task_fn_area_2(struct anybuss_host *cd, struct ab_task *t)
> +static int task_fn_area(struct anybuss_host *cd, struct ab_task *t)
> +static struct ab_task *
> +create_area_reader(struct kmem_cache *qcache, u16 flags, u16 addr,
> +                               size_t count)
> +static struct ab_task *
> +create_area_writer(struct kmem_cache *qcache, u16 flags, u16 addr,
> +                               const void *buf, size_t count)
> +static struct ab_task *
> +create_area_user_writer(struct kmem_cache *qcache, u16 flags, u16 addr,
> +                               const void __user *buf, size_t count)

So there are many different tasks going on.

Are they just created to get something going in process context?

> +static void softint_ack(struct anybuss_host *cd)
> +static void process_softint(struct anybuss_host *cd)

This looks like MSI (message signalled interrupt) and makes me think
that you should probably involve the irqchip maintainers. Interrupts
shall be represented in the irqchip abstraction.

> +int anybuss_client_driver_register(struct anybuss_client_driver *drv)
> +{
> +       drv->driver.bus = &anybus_bus;
> +       return driver_register(&drv->driver);
> +}

There is nice use of the bus abstractions here.

> +       cd->reset = pdata->reset;

This callback thing to handle reset doesn't seem right.

The kernel has a reset for assert/deassert och just assert()
reset handling that you can find in
drivers/reset/*

Maybe that is what you should be using instead of
rolling your own reset handling?

> +       cd->reset(parent, true);

So this would just be something like
#include <linux/reset.h>

r = devm_reset_control_get_exclusive(dev, id);
ret = reset_control_assert(r);

> +       regmap_bulk_read(cd->regmap, REG_SERIAL_NO, val, 4);
> +       dev_info(dev, "Serial number: %02X%02X%02X%02X",
> +               val[0], val[1], val[2], val[3]);

This looks like device-unique data so please do this:

#include <linux/random.h>

add_device_randomness(val, 4);

I guess I can provide better review once you add some
information on this task state machine business and how
it is engineered, looking forward to the next iteration!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
  2018-10-24 14:24 ` [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus Sven Van Asbroeck
@ 2018-10-24 15:58   ` Randy Dunlap
  2018-10-25 11:08   ` Linus Walleij
  1 sibling, 0 replies; 6+ messages in thread
From: Randy Dunlap @ 2018-10-24 15:58 UTC (permalink / raw)
  To: Sven Van Asbroeck, lee.jones, robh+dt, mark.rutland, afaerber,
	treding, david, noralf, johan, monstr, michal.vokac, arnd,
	gregkh, john.garry, andriy.shevchenko, geert+renesas,
	robin.murphy, paul.gortmaker, sebastien.bourdelin, icenowy,
	yuanzhichang, stuyoder, linus.walleij, maxime.ripard,
	bogdan.purcareata
  Cc: linux-kernel, devicetree

On 10/24/18 7:24 AM, Sven Van Asbroeck wrote:
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 1851112ccc29..68869648b9ab 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -45,6 +45,17 @@ config IMX_WEIM
>  	  The WEIM(Wireless External Interface Module) works like a bus.
>  	  You can attach many different devices on it, such as NOR, onenand.
>  
> +config HMS_ANYBUSS_HOST
> +	tristate "HMS Anybus-S Host/Bus Driver"
> +	select REGMAP
> +	depends on OF
> +	default n

Please drop the "default n".  That is already the default in Kconfig files.

> +	help
> +	  Driver for the HMS Industrial Networks Anybus-S bus.
> +	  You can attach Anybus-S compatible cards to it, which
> +	  typically provide fieldbus and industrial ethernet
> +	  functionality.
> +
>  config MIPS_CDMM
>  	bool "MIPS Common Device Memory Map (CDMM) Driver"
>  	depends on CPU_MIPSR2


Also please check the multi-line comments for kernel comment style.

thanks,
-- 
~Randy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
  2018-10-24 14:24 [PATCH anybus v1 0/4] Support HMS Profinet Card over Anybus Sven Van Asbroeck
@ 2018-10-24 14:24 ` Sven Van Asbroeck
  2018-10-24 15:58   ` Randy Dunlap
  2018-10-25 11:08   ` Linus Walleij
  0 siblings, 2 replies; 6+ messages in thread
From: Sven Van Asbroeck @ 2018-10-24 14:24 UTC (permalink / raw)
  To: svendev, lee.jones, robh+dt, mark.rutland, afaerber, treding,
	david, noralf, johan, monstr, michal.vokac, arnd, gregkh,
	john.garry, andriy.shevchenko, geert+renesas, robin.murphy,
	paul.gortmaker, sebastien.bourdelin, icenowy, yuanzhichang,
	stuyoder, linus.walleij, maxime.ripard, bogdan.purcareata
  Cc: linux-kernel, devicetree

The Anybus-S/Anybus-M is a series of interchangeable fieldbus communication
modules featuring on board memory and processing power. All software and
hardware functionality required to communicate on the fieldbus is
incorporated in the module itself, allowing the application to focus on
other tasks.

Typical applications are frequency inverters, HMI and visualization
devices, instruments, scales, robotics, PLC’s and intelligent measuring
devices.

Official documentation:
https://www.anybus.com/docs/librariesprovider7/default-document-library/
manuals-design-guides/hms-hmsi-27-275.pdf

This driver implementation is designed to be almost completely independent
from the way the Anybus-S hardware is accessed by the SoC. All it needs is:

- a regmap which accesses the underlying Anybus-S dpram by whatever means;
- an interrupt line, ultimately connected to the Anybus-S card;
- a reset function.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 drivers/bus/Kconfig            |   11 +
 drivers/bus/Makefile           |    1 +
 drivers/bus/anybuss-host.c     | 1301 ++++++++++++++++++++++++++++++++
 include/linux/anybuss-client.h |  100 +++
 4 files changed, 1413 insertions(+)
 create mode 100644 drivers/bus/anybuss-host.c
 create mode 100644 include/linux/anybuss-client.h

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 1851112ccc29..68869648b9ab 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -45,6 +45,17 @@ config IMX_WEIM
 	  The WEIM(Wireless External Interface Module) works like a bus.
 	  You can attach many different devices on it, such as NOR, onenand.
 
+config HMS_ANYBUSS_HOST
+	tristate "HMS Anybus-S Host/Bus Driver"
+	select REGMAP
+	depends on OF
+	default n
+	help
+	  Driver for the HMS Industrial Networks Anybus-S bus.
+	  You can attach Anybus-S compatible cards to it, which
+	  typically provide fieldbus and industrial ethernet
+	  functionality.
+
 config MIPS_CDMM
 	bool "MIPS Common Device Memory Map (CDMM) Driver"
 	depends on CPU_MIPSR2
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index ca300b1914ce..f67c6468126b 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_BRCMSTB_GISB_ARB)	+= brcmstb_gisb.o
 obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
 
 obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
+obj-$(CONFIG_HMS_ANYBUSS_HOST)	+= anybuss-host.o
 obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
 obj-$(CONFIG_MVEBU_MBUS) 	+= mvebu-mbus.o
 
diff --git a/drivers/bus/anybuss-host.c b/drivers/bus/anybuss-host.c
new file mode 100644
index 000000000000..41d62937a5eb
--- /dev/null
+++ b/drivers/bus/anybuss-host.c
@@ -0,0 +1,1301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HMS Anybus-S Host Driver
+ *
+ * Copyright (C) 2018 Arcx Inc
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/atomic.h>
+#include <linux/kthread.h>
+#include <linux/kfifo.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+
+#include <linux/anybuss-host.h>
+#include <linux/anybuss-client.h>
+
+#define DPRAM_SIZE		0x800
+#define MAX_MBOX_MSG_SZ		0x0FF
+#define TIMEOUT			(HZ*2)
+#define MAX_DATA_AREA_SZ	0x200
+#define MAX_FBCTRL_AREA_SZ	0x1BE
+
+#define REG_BOOTLOADER_V	0x7C0
+#define REG_API_V		0x7C2
+#define REG_FIELDBUS_V		0x7C4
+#define REG_SERIAL_NO		0x7C6
+#define REG_FIELDBUS_TYPE	0x7CC
+#define REG_MODULE_SW_V		0x7CE
+#define REG_IND_AB		0x7FF
+#define REG_IND_AP		0x7FE
+#define REG_EVENT_CAUSE		0x7ED
+#define MBOX_IN_AREA		0x400
+#define MBOX_OUT_AREA		0x520
+#define DATA_IN_AREA		0x000
+#define DATA_OUT_AREA		0x200
+#define FBCTRL_AREA		0x640
+
+#define EVENT_CAUSE_DC          0x01
+#define EVENT_CAUSE_FBOF        0x02
+#define EVENT_CAUSE_FBON        0x04
+
+#define IND_AB_UPDATED		0x08
+#define IND_AX_MIN		0x80
+#define IND_AX_MOUT		0x40
+#define IND_AX_IN		0x04
+#define IND_AX_OUT		0x02
+#define IND_AX_FBCTRL		0x01
+#define IND_AP_LOCK		0x08
+#define IND_AP_ACTION		0x10
+#define IND_AX_EVNT		0x20
+#define IND_AP_ABITS		(IND_AX_IN | IND_AX_OUT | \
+					IND_AX_FBCTRL | \
+					IND_AP_ACTION | IND_AP_LOCK)
+
+#define INFO_TYPE_FB		0x0002
+#define INFO_TYPE_APP		0x0001
+#define INFO_COMMAND		0x4000
+
+#define OP_MODE_FBFC		0x0002
+#define OP_MODE_FBS		0x0004
+#define OP_MODE_CD		0x0200
+
+#define CMD_START_INIT		0x0001
+#define CMD_ANYBUS_INIT		0x0002
+#define CMD_END_INIT		0x0003
+
+/* ------------- ref counted tasks ------------- */
+
+struct ab_task;
+typedef int (*ab_task_fn_t)(struct anybuss_host *cd,
+					struct ab_task *t);
+typedef void (*ab_done_fn_t)(struct anybuss_host *cd);
+
+struct area_priv {
+	bool is_write;
+	u16 flags;
+	u16 addr;
+	size_t count;
+	u8 buf[MAX_DATA_AREA_SZ];
+};
+
+struct anybus_mbox_hdr {
+	u16 id;
+	u16 info;
+	u16 cmd_num;
+	u16 data_size;
+	u16 frame_count;
+	u16 frame_num;
+	u16 offset_high;
+	u16 offset_low;
+	u16 extended[8];
+} __packed;
+
+struct mbox_priv {
+	struct anybus_mbox_hdr hdr;
+	size_t msg_out_sz;
+	size_t msg_in_sz;
+	u8 msg[MAX_MBOX_MSG_SZ];
+};
+
+struct ab_task {
+	struct kmem_cache	*cache;
+	atomic_t		refs;
+	ab_task_fn_t		task_fn;
+	ab_done_fn_t		done_fn;
+	int			result;
+	struct completion	done;
+	unsigned long		start_jiffies;
+	union {
+		struct area_priv area_pd;
+		struct mbox_priv mbox_pd;
+	};
+};
+
+static struct ab_task *ab_task_create_get(struct kmem_cache *cache,
+			ab_task_fn_t task_fn)
+{
+	struct ab_task *t;
+
+	t = kmem_cache_alloc(cache, GFP_KERNEL);
+	if (!t)
+		return NULL;
+	t->cache = cache;
+	atomic_set(&t->refs, 1);
+	t->task_fn = task_fn;
+	t->done_fn = NULL;
+	t->result = 0;
+	init_completion(&t->done);
+	return t;
+}
+
+static void ab_task_put(struct ab_task *t)
+{
+	struct kmem_cache *cache = t->cache;
+
+	if (atomic_dec_and_test(&t->refs))
+		kmem_cache_free(cache, t);
+}
+
+static struct ab_task *__ab_task_get(struct ab_task *t)
+{
+	atomic_inc(&t->refs);
+	return t;
+}
+
+static void
+__ab_task_finish(struct anybuss_host *cd, struct ab_task *t)
+{
+	if (t->done_fn)
+		t->done_fn(cd);
+	complete(&t->done);
+}
+
+static void
+ab_task_dequeue_finish_put(struct anybuss_host *cd, struct kfifo *q)
+{
+	int ret;
+	struct ab_task *t;
+
+	ret = kfifo_out(q, &t, sizeof(t));
+	WARN_ON(!ret);
+	__ab_task_finish(cd, t);
+	ab_task_put(t);
+}
+
+static int
+ab_task_enqueue(wait_queue_head_t *wq, struct kfifo *q,
+			struct ab_task *t, spinlock_t *slock)
+{
+	int ret;
+
+	t->start_jiffies = jiffies;
+	ret = kfifo_in_spinlocked(q, &t, sizeof(t), slock);
+	if (!ret)
+		return -ENOMEM;
+	__ab_task_get(t);
+	wake_up(wq);
+	return 0;
+}
+
+static int
+ab_task_enqueue_wait(wait_queue_head_t *wq, struct kfifo *q,
+			struct ab_task *t, spinlock_t *slock)
+{
+	int ret;
+
+	ret = ab_task_enqueue(wq, q, t, slock);
+	if (ret)
+		return ret;
+	ret = wait_for_completion_interruptible(&t->done);
+	if (ret)
+		return ret;
+	return t->result;
+}
+
+/* ------------------------ anybus hardware ------------------------ */
+
+struct anybuss_host {
+	struct device *dev;
+	struct device *parent;
+	struct anybuss_client *client;
+	anybuss_reset_t reset;
+	struct regmap *regmap;
+	int irq;
+	struct task_struct *qthread;
+	wait_queue_head_t wq;
+	struct completion card_boot;
+	atomic_t ind_ab;
+	spinlock_t qlock;
+	struct kmem_cache *qcache;
+	struct kfifo qs[3];
+	struct kfifo *powerq;
+	struct kfifo *mboxq;
+	struct kfifo *areaq;
+	bool power_on;
+	bool softint_pending;
+	atomic_t dc_event;
+	wait_queue_head_t dc_wq;
+	atomic_t fieldbus_online;
+	struct kernfs_node *fieldbus_online_sd;
+};
+
+static int test_dpram(struct regmap *regmap)
+{
+	int i;
+	unsigned int val;
+
+	for (i = 0; i < DPRAM_SIZE; i++)
+		regmap_write(regmap, i, (u8)i);
+	for (i = 0; i < DPRAM_SIZE; i++) {
+		regmap_read(regmap, i, &val);
+		if ((u8)val != (u8)i)
+			return -EIO;
+	}
+	return 0;
+}
+
+static void ab_reset(struct anybuss_host *cd, bool reset)
+{
+	cd->reset(cd->parent, reset);
+}
+
+static int read_ind_ab(struct regmap *regmap)
+{
+	unsigned long timeout = jiffies + HZ/2;
+	unsigned int a, b;
+
+	while (time_before_eq(jiffies, timeout)) {
+		regmap_read(regmap, REG_IND_AB, &a);
+		regmap_read(regmap, REG_IND_AB, &b);
+		if (likely(a == b))
+			return (int)a;
+		cpu_relax();
+	}
+	WARN(1, "IND_AB register not stable");
+	return -ETIMEDOUT;
+}
+
+static int write_ind_ap(struct regmap *regmap, unsigned int ind_ap)
+{
+	unsigned long timeout = jiffies + HZ/2;
+	unsigned int v;
+
+	while (time_before_eq(jiffies, timeout)) {
+		regmap_write(regmap, REG_IND_AP, ind_ap);
+		regmap_read(regmap, REG_IND_AP, &v);
+		if (likely(ind_ap == v))
+			return 0;
+		cpu_relax();
+	}
+	WARN(1, "IND_AP register not stable");
+	return -ETIMEDOUT;
+}
+
+static irqreturn_t irq_handler(int irq, void *data)
+{
+	struct anybuss_host *cd = data;
+	int ind_ab;
+
+	/* reading the anybus indicator register acks the interrupt */
+	ind_ab = read_ind_ab(cd->regmap);
+	if (ind_ab < 0)
+		return IRQ_NONE;
+	atomic_set(&cd->ind_ab, ind_ab);
+	complete(&cd->card_boot);
+	wake_up(&cd->wq);
+	return IRQ_HANDLED;
+}
+
+/* ------------------------ power on/off tasks --------------------- */
+
+static int task_fn_power_off(struct anybuss_host *cd,
+				struct ab_task *t)
+{
+	if (!cd->power_on)
+		return 0;
+	disable_irq(cd->irq);
+	ab_reset(cd, true);
+	atomic_set(&cd->ind_ab, IND_AB_UPDATED);
+	atomic_set(&cd->fieldbus_online, 0);
+	sysfs_notify_dirent(cd->fieldbus_online_sd);
+	cd->power_on = false;
+	return 0;
+}
+
+static int task_fn_power_on_2(struct anybuss_host *cd,
+				struct ab_task *t)
+{
+	if (completion_done(&cd->card_boot)) {
+		cd->power_on = true;
+		return 0;
+	}
+	if (time_after(jiffies, t->start_jiffies + TIMEOUT)) {
+		disable_irq(cd->irq);
+		ab_reset(cd, true);
+		dev_err(cd->dev, "power on timed out");
+		return -ETIMEDOUT;
+	}
+	return -EINPROGRESS;
+}
+
+static int task_fn_power_on(struct anybuss_host *cd,
+				struct ab_task *t)
+{
+	unsigned int dummy;
+
+	if (cd->power_on)
+		return 0;
+	/* anybus docs: prevent false 'init done' interrupt by
+	 * doing a dummy read of IND_AB register while in reset.
+	 */
+	regmap_read(cd->regmap, REG_IND_AB, &dummy);
+	reinit_completion(&cd->card_boot);
+	enable_irq(cd->irq);
+	ab_reset(cd, false);
+	t->task_fn = task_fn_power_on_2;
+	return -EINPROGRESS;
+}
+
+int anybuss_set_power(struct anybuss_client *client, bool power_on)
+{
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int err;
+
+	t = ab_task_create_get(cd->qcache, power_on ?
+				task_fn_power_on : task_fn_power_off);
+	if (!t)
+		return -ENOMEM;
+	err = ab_task_enqueue_wait(&cd->wq, cd->powerq, t, &cd->qlock);
+	ab_task_put(t);
+	return err;
+}
+EXPORT_SYMBOL_GPL(anybuss_set_power);
+
+/* ---------------------------- area tasks ------------------------ */
+
+static int task_fn_area_3(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct area_priv *pd = &t->area_pd;
+
+	if (!cd->power_on)
+		return -EIO;
+	if (atomic_read(&cd->ind_ab) & pd->flags) {
+		/* area not released yet */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT))
+			return -ETIMEDOUT;
+		return -EINPROGRESS;
+	}
+	return 0;
+}
+
+static int task_fn_area_2(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct area_priv *pd = &t->area_pd;
+	unsigned int ind_ap;
+	int ret;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if (!(atomic_read(&cd->ind_ab) & pd->flags)) {
+		/* we don't own the area yet */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT)) {
+			dev_warn(cd->dev, "timeout waiting for area");
+			dump_stack();
+			return -ETIMEDOUT;
+		}
+		return -EINPROGRESS;
+	}
+	/* we own the area, do what we're here to do */
+	if (pd->is_write)
+		regmap_bulk_write(cd->regmap, pd->addr, pd->buf,
+							pd->count);
+	else
+		regmap_bulk_read(cd->regmap, pd->addr, pd->buf,
+							pd->count);
+	/* ask to release the area, must use unlocked release */
+	ind_ap &= ~IND_AP_ABITS;
+	ind_ap |= pd->flags;
+	ret = write_ind_ap(cd->regmap, ind_ap);
+	if (ret)
+		return ret;
+	t->task_fn = task_fn_area_3;
+	return -EINPROGRESS;
+}
+
+static int task_fn_area(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct area_priv *pd = &t->area_pd;
+	unsigned int ind_ap;
+	int ret;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	/* ask to take the area */
+	ind_ap &= ~IND_AP_ABITS;
+	ind_ap |= pd->flags | IND_AP_ACTION | IND_AP_LOCK;
+	ret = write_ind_ap(cd->regmap, ind_ap);
+	if (ret)
+		return ret;
+	t->task_fn = task_fn_area_2;
+	return -EINPROGRESS;
+}
+
+static struct ab_task *
+create_area_reader(struct kmem_cache *qcache, u16 flags, u16 addr,
+				size_t count)
+{
+	struct ab_task *t;
+	struct area_priv *ap;
+
+	t = ab_task_create_get(qcache, task_fn_area);
+	if (!t)
+		return NULL;
+	ap = &t->area_pd;
+	ap->flags = flags;
+	ap->addr = addr;
+	ap->is_write = false;
+	ap->count = count;
+	return t;
+}
+
+static struct ab_task *
+create_area_writer(struct kmem_cache *qcache, u16 flags, u16 addr,
+				const void *buf, size_t count)
+{
+	struct ab_task *t;
+	struct area_priv *ap;
+
+	t = ab_task_create_get(qcache, task_fn_area);
+	if (!t)
+		return NULL;
+	ap = &t->area_pd;
+	ap->flags = flags;
+	ap->addr = addr;
+	ap->is_write = true;
+	ap->count = count;
+	memcpy(ap->buf, buf, count);
+	return t;
+}
+
+static struct ab_task *
+create_area_user_writer(struct kmem_cache *qcache, u16 flags, u16 addr,
+				const void __user *buf, size_t count)
+{
+	struct ab_task *t;
+	struct area_priv *ap;
+
+	t = ab_task_create_get(qcache, task_fn_area);
+	if (!t)
+		return ERR_PTR(-ENOMEM);
+	ap = &t->area_pd;
+	ap->flags = flags;
+	ap->addr = addr;
+	ap->is_write = true;
+	ap->count = count;
+	if (copy_from_user(ap->buf, buf, count)) {
+		ab_task_put(t);
+		return ERR_PTR(-EFAULT);
+	}
+	return t;
+}
+
+static bool area_range_ok(u16 addr, size_t count, u16 area_start,
+							size_t area_sz)
+{
+	u16 area_end_ex = area_start + area_sz;
+	u16 addr_end_ex;
+
+	if (addr < area_start)
+		return false;
+	if (addr >= area_end_ex)
+		return false;
+	addr_end_ex = addr + count;
+	if (addr_end_ex > area_end_ex)
+		return false;
+	return true;
+}
+
+/* -------------------------- mailbox tasks ----------------------- */
+
+struct msgAnybusInit {
+	u16 input_io_len;
+	u16 input_dpram_len;
+	u16 input_total_len;
+	u16 output_io_len;
+	u16 output_dpram_len;
+	u16 output_total_len;
+	u16 op_mode;
+	u16 notif_config;
+	u16 wd_val;
+} __packed;
+
+static int task_fn_mbox_2(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct mbox_priv *pd = &t->mbox_pd;
+	unsigned int ind_ap;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if (((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_MOUT) == 0) {
+		/* output message not here */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT))
+			return -ETIMEDOUT;
+		return -EINPROGRESS;
+	}
+	/* grab the returned header and msg */
+	regmap_bulk_read(cd->regmap, MBOX_OUT_AREA, &pd->hdr,
+		sizeof(pd->hdr));
+	regmap_bulk_read(cd->regmap, MBOX_OUT_AREA + sizeof(pd->hdr),
+		pd->msg, pd->msg_in_sz);
+	/* tell anybus we've consumed the message */
+	ind_ap ^= IND_AX_MOUT;
+	return write_ind_ap(cd->regmap, ind_ap);
+}
+
+static int task_fn_mbox(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct mbox_priv *pd = &t->mbox_pd;
+	unsigned int ind_ap;
+	int ret;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if ((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_MIN) {
+		/* mbox input area busy */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT))
+			return -ETIMEDOUT;
+		return -EINPROGRESS;
+	}
+	/* write the header and msg to input area */
+	regmap_bulk_write(cd->regmap, MBOX_IN_AREA, &pd->hdr,
+					sizeof(pd->hdr));
+	regmap_bulk_write(cd->regmap, MBOX_IN_AREA + sizeof(pd->hdr),
+					pd->msg, pd->msg_out_sz);
+	/* tell anybus we gave it a message */
+	ind_ap ^= IND_AX_MIN;
+	ret = write_ind_ap(cd->regmap, ind_ap);
+	if (ret)
+		return ret;
+	t->start_jiffies = jiffies;
+	t->task_fn = task_fn_mbox_2;
+	return -EINPROGRESS;
+}
+
+static const char * const O_MSGS[] = {
+	"Incorrect length of input I/O",		/* bit 0 */
+	"Incorrect length of DPRAM input",		/* bit 1 */
+	"Incorrect length of total input",		/* bit 2 */
+	"(reserved)",					/* bit 3 */
+	"Incorrect length of output I/O",		/* bit 4 */
+	"Incorrect length of DPRAM output",		/* bit 5 */
+	"Incorrect length of total output",		/* bit 6 */
+	"(reserved)",					/* bit 7 */
+	"Incorrect config of Module Status reg",	/* bit 8 */
+	"Incorrect config of Event Notification reg",	/* bit 9 */
+	"Incorrect Watchdog Counter difference value",	/* bit 10 */
+};
+
+static void log_invalid_other(struct device *dev,
+				struct anybus_mbox_hdr *hdr)
+{
+	int i;
+	size_t ext_offs = sizeof(hdr->extended) - 1;
+	u16 code = be16_to_cpu(hdr->extended[ext_offs]);
+
+	dev_err(dev, "   Invalid other: [0x%02X]", code);
+	for (i = 0; code != 0; code >>= 1, i++)
+		if ((code & 1) && (i < ARRAY_SIZE(O_MSGS)))
+			dev_err(dev, "\t%s", O_MSGS[i]);
+}
+
+static const char * const EMSGS[] = {
+	"Invalid Message ID",
+	"Invalid Message Type",
+	"Invalid Command",
+	"Invalid Data Size",
+	"Message Header Malformed (offset 008h)",
+	"Message Header Malformed (offset 00Ah)",
+	"Message Header Malformed (offset 00Ch - 00Dh)",
+	"Invalid Address",
+	"Invalid Response",
+	"Flash Config Error",
+};
+
+static int mbox_cmd_err(struct device *dev, struct mbox_priv *mpriv)
+{
+	int i;
+	u8 ecode;
+	struct anybus_mbox_hdr *hdr = &mpriv->hdr;
+	u16 info = be16_to_cpu(hdr->info);
+	u8 *phdr = (u8 *)hdr;
+	u8 *pmsg = mpriv->msg;
+
+	if (!(info & 0x8000))
+		return 0;
+	ecode = (info >> 8) & 0x0F;
+	dev_err(dev, "mailbox command failed:");
+	if (ecode == 0x0F)
+		log_invalid_other(dev, hdr);
+	else if (ecode < ARRAY_SIZE(EMSGS))
+		dev_err(dev, "   Error code: %s (0x%02X)",
+			EMSGS[ecode], ecode);
+	else
+		dev_err(dev, "   Error code: 0x%02X\n", ecode);
+	dev_err(dev, "Failed command:");
+	dev_err(dev, "Message Header:");
+	for (i = 0; i < sizeof(mpriv->hdr); i += 2)
+		dev_err(dev, "%02X%02X", phdr[i], phdr[i+1]);
+	dev_err(dev, "Message Data:");
+	for (i = 0; i < mpriv->msg_in_sz; i += 2)
+		dev_err(dev, "%02X%02X", pmsg[i], pmsg[i+1]);
+	dev_err(dev, "Stack dump:");
+	dump_stack();
+	return -EIO;
+}
+
+static int _anybus_mbox_cmd(struct anybuss_host *cd,
+				u16 cmd_num, bool is_fb_cmd,
+				const void *msg_out, size_t msg_out_sz,
+				void *msg_in, size_t msg_in_sz,
+				const void *ext, size_t ext_sz)
+{
+	struct ab_task *t;
+	struct mbox_priv *pd;
+	struct anybus_mbox_hdr *h;
+	size_t msg_sz = max(msg_in_sz, msg_out_sz);
+	u16 info;
+	int err;
+
+	if (msg_sz > MAX_MBOX_MSG_SZ)
+		return -EINVAL;
+	if (ext && (ext_sz > sizeof(h->extended)))
+		return -EINVAL;
+	t = ab_task_create_get(cd->qcache, task_fn_mbox);
+	if (!t)
+		return -ENOMEM;
+	pd = &t->mbox_pd;
+	h = &pd->hdr;
+	info = is_fb_cmd ? INFO_TYPE_FB : INFO_TYPE_APP;
+	/* prevent uninitialized memory in the header from being sent
+	 * across the anybus
+	 */
+	memset(h, 0, sizeof(*h));
+	h->info = cpu_to_be16(info | INFO_COMMAND);
+	h->cmd_num = cpu_to_be16(cmd_num);
+	h->data_size = cpu_to_be16(msg_out_sz);
+	h->frame_count = cpu_to_be16(1);
+	h->frame_num = cpu_to_be16(1);
+	h->offset_high = cpu_to_be16(0);
+	h->offset_low = cpu_to_be16(0);
+	if (ext)
+		memcpy(h->extended, ext, ext_sz);
+	memcpy(pd->msg, msg_out, msg_out_sz);
+	pd->msg_out_sz = msg_out_sz;
+	pd->msg_in_sz = msg_in_sz;
+	err = ab_task_enqueue_wait(&cd->wq, cd->mboxq, t, &cd->qlock);
+	if (err)
+		goto out;
+	/* mailbox mechanism worked ok, but maybe the mbox response
+	 * contains an error ?
+	 */
+	err = mbox_cmd_err(cd->dev, pd);
+	if (err)
+		goto out;
+	memcpy(msg_in, pd->msg, msg_in_sz);
+out:
+	ab_task_put(t);
+	return err;
+}
+
+/* ------------------------ anybus queues ------------------------ */
+
+static void process_q(struct anybuss_host *cd, struct kfifo *q)
+{
+	struct ab_task *t;
+	int ret;
+
+	ret = kfifo_out_peek(q, &t, sizeof(t));
+	if (!ret)
+		return;
+	t->result = t->task_fn(cd, t);
+	if (t->result != -EINPROGRESS)
+		ab_task_dequeue_finish_put(cd, q);
+}
+
+static bool qs_have_work(struct kfifo *qs, size_t num)
+{
+	size_t i;
+	struct ab_task *t;
+	int ret;
+
+	for (i = 0; i < num; i++, qs++) {
+		ret = kfifo_out_peek(qs, &t, sizeof(t));
+		if (ret && (t->result != -EINPROGRESS))
+			return true;
+	}
+	return false;
+}
+
+static void process_qs(struct anybuss_host *cd)
+{
+	size_t i;
+	struct kfifo *qs = cd->qs;
+	size_t nqs = ARRAY_SIZE(cd->qs);
+
+	for (i = 0; i < nqs; i++, qs++)
+		process_q(cd, qs);
+}
+
+static void softint_ack(struct anybuss_host *cd)
+{
+	unsigned int ind_ap;
+
+	cd->softint_pending = false;
+	if (!cd->power_on)
+		return;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	ind_ap &= ~IND_AX_EVNT;
+	ind_ap |= atomic_read(&cd->ind_ab) & IND_AX_EVNT;
+	write_ind_ap(cd->regmap, ind_ap);
+}
+
+static void process_softint(struct anybuss_host *cd)
+{
+	static const u8 zero;
+	int ret;
+	unsigned int ind_ap, ev;
+	struct ab_task *t;
+
+	if (!cd->power_on)
+		return;
+	if (cd->softint_pending)
+		return;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if (!((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_EVNT))
+		return;
+	/* process software interrupt */
+	regmap_read(cd->regmap, REG_EVENT_CAUSE, &ev);
+	if (ev & EVENT_CAUSE_FBON) {
+		atomic_set(&cd->fieldbus_online, 1);
+		sysfs_notify_dirent(cd->fieldbus_online_sd);
+		dev_dbg(cd->dev, "Fieldbus ON");
+	}
+	if (ev & EVENT_CAUSE_FBOF) {
+		atomic_set(&cd->fieldbus_online, 0);
+		sysfs_notify_dirent(cd->fieldbus_online_sd);
+		dev_dbg(cd->dev, "Fieldbus OFF");
+	}
+	if (ev & EVENT_CAUSE_DC) {
+		atomic_inc(&cd->dc_event);
+		wake_up_all(&cd->dc_wq);
+		dev_dbg(cd->dev, "Fieldbus data changed");
+	}
+	/* reset the event cause bits.
+	 * this must be done while owning the fbctrl area, so we'll
+	 * enqueue a task to do that.
+	 */
+	t = create_area_writer(cd->qcache, IND_AX_FBCTRL,
+		REG_EVENT_CAUSE, &zero, sizeof(zero));
+	if (!t) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	t->done_fn = softint_ack;
+	ret = ab_task_enqueue(&cd->wq, cd->areaq, t, &cd->qlock);
+	ab_task_put(t);
+	cd->softint_pending = true;
+out:
+	WARN_ON(ret);
+	if (ret)
+		softint_ack(cd);
+}
+
+static int qthread_fn(void *data)
+{
+	struct anybuss_host *cd = data;
+	struct kfifo *qs = cd->qs;
+	size_t nqs = ARRAY_SIZE(cd->qs);
+	unsigned int ind_ab;
+
+	while (!kthread_should_stop()) {
+		ind_ab = atomic_read(&cd->ind_ab);
+		process_qs(cd);
+		process_softint(cd);
+		wait_event_timeout(cd->wq,
+			(atomic_read(&cd->ind_ab) != ind_ab) ||
+				qs_have_work(qs, nqs) ||
+				kthread_should_stop(),
+			HZ);
+	}
+
+	return 0;
+}
+
+/* ------------------------ anybus exports ------------------------ */
+
+int anybuss_start_init(struct anybuss_client *client,
+			const struct anybuss_memcfg *cfg)
+{
+	int ret;
+	u16 op_mode;
+	struct anybuss_host *cd = client->host;
+	struct msgAnybusInit msg = {
+		.input_io_len = cpu_to_be16(cfg->input_io),
+		.input_dpram_len = cpu_to_be16(cfg->input_dpram),
+		.input_total_len = cpu_to_be16(cfg->input_total),
+		.output_io_len = cpu_to_be16(cfg->output_io),
+		.output_dpram_len = cpu_to_be16(cfg->output_dpram),
+		.output_total_len = cpu_to_be16(cfg->output_total),
+		.notif_config = cpu_to_be16(0x000F),
+		.wd_val = cpu_to_be16(0),
+	};
+
+	switch (cfg->offl_mode) {
+	case AB_OFFL_MODE_CLEAR:
+		op_mode = 0;
+		break;
+	case AB_OFFL_MODE_FREEZE:
+		op_mode = OP_MODE_FBFC;
+		break;
+	case AB_OFFL_MODE_SET:
+		op_mode = OP_MODE_FBS;
+		break;
+	default:
+		return -EINVAL;
+	}
+	msg.op_mode = cpu_to_be16(op_mode | OP_MODE_CD);
+	ret = _anybus_mbox_cmd(cd, CMD_START_INIT, false, NULL, 0,
+						NULL, 0, NULL, 0);
+	if (ret)
+		return ret;
+	return _anybus_mbox_cmd(cd, CMD_ANYBUS_INIT, false,
+			&msg, sizeof(msg), NULL, 0, NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_start_init);
+
+int anybuss_finish_init(struct anybuss_client *client)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, CMD_END_INIT, false, NULL, 0,
+					NULL, 0, NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_finish_init);
+
+int anybuss_read_fbctrl(struct anybuss_client *client, u16 addr,
+				void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int ret;
+
+	if (count == 0)
+		return 0;
+	if (!area_range_ok(addr, count, FBCTRL_AREA,
+					MAX_FBCTRL_AREA_SZ))
+		return -EFAULT;
+	t = create_area_reader(cd->qcache, IND_AX_FBCTRL, addr, count);
+	if (!t)
+		return -ENOMEM;
+	ret = ab_task_enqueue_wait(&cd->wq, cd->areaq, t, &cd->qlock);
+	if (ret)
+		goto out;
+	memcpy(buf, t->area_pd.buf, count);
+out:
+	ab_task_put(t);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(anybuss_read_fbctrl);
+
+int anybuss_write_input(struct anybuss_client *client,
+				const char __user *buf, size_t size,
+				loff_t *offset)
+{
+	ssize_t len = min_t(loff_t, MAX_DATA_AREA_SZ - *offset, size);
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int ret;
+
+	if (len <= 0)
+		return 0;
+	t = create_area_user_writer(cd->qcache, IND_AX_IN,
+		DATA_IN_AREA + *offset, buf, len);
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	ret = ab_task_enqueue_wait(&cd->wq, cd->areaq, t, &cd->qlock);
+	ab_task_put(t);
+	if (ret)
+		return ret;
+	/* success */
+	*offset += len;
+	return len;
+}
+EXPORT_SYMBOL_GPL(anybuss_write_input);
+
+int anybuss_read_output(struct anybuss_client *client, int *dc_event,
+				char __user *buf, size_t size,
+				loff_t *offset)
+{
+	ssize_t len = min_t(loff_t, MAX_DATA_AREA_SZ - *offset, size);
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int ret;
+
+	if (len <= 0)
+		return 0;
+	t = create_area_reader(cd->qcache, IND_AX_OUT,
+		DATA_OUT_AREA + *offset, len);
+	if (!t)
+		return -ENOMEM;
+	*dc_event = atomic_read(&cd->dc_event);
+	ret = ab_task_enqueue_wait(&cd->wq, cd->areaq, t, &cd->qlock);
+	if (ret)
+		goto out;
+	if (copy_to_user(buf, t->area_pd.buf, len))
+		ret = -EFAULT;
+out:
+	ab_task_put(t);
+	if (ret)
+		return ret;
+	/* success */
+	*offset += len;
+	return len;
+}
+EXPORT_SYMBOL_GPL(anybuss_read_output);
+
+unsigned int anybuss_poll(struct anybuss_client *client, int dc_event,
+				struct file *filp, poll_table *wait)
+{
+	struct anybuss_host *cd = client->host;
+	unsigned int mask = POLLIN | POLLRDNORM | POLLOUT | POLLWRNORM;
+
+	poll_wait(filp, &cd->dc_wq, wait);
+	/* data changed ? */
+	if (atomic_read(&cd->dc_event) != dc_event)
+		mask |= POLLPRI | POLLERR;
+	return mask;
+}
+EXPORT_SYMBOL_GPL(anybuss_poll);
+
+int anybuss_send_msg(struct anybuss_client *client, u16 cmd_num,
+				const void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, cmd_num, true, buf, count, NULL, 0,
+					NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_send_msg);
+
+int anybuss_send_ext(struct anybuss_client *client, u16 cmd_num,
+	const void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, cmd_num, true, NULL, 0, NULL, 0,
+					buf, count);
+}
+EXPORT_SYMBOL_GPL(anybuss_send_ext);
+
+int anybuss_recv_msg(struct anybuss_client *client, u16 cmd_num,
+	void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, cmd_num, true, NULL, 0, buf, count,
+					NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_recv_msg);
+
+/* ------------------------ bus functions ------------------------ */
+
+static int anybus_bus_match(struct device *dev,
+				struct device_driver *drv)
+{
+	struct anybuss_client_driver *adrv =
+		to_anybuss_client_driver(drv);
+	struct anybuss_client *adev =
+		to_anybuss_client(dev);
+
+	return adrv->fieldbus_type == adev->fieldbus_type;
+}
+
+static int anybus_bus_probe(struct device *dev)
+{
+	struct anybuss_client_driver *adrv =
+		to_anybuss_client_driver(dev->driver);
+	struct anybuss_client *adev =
+		to_anybuss_client(dev);
+
+	if (!adrv->probe)
+		return -ENODEV;
+	return adrv->probe(adev);
+}
+
+static int anybus_bus_remove(struct device *dev)
+{
+	struct anybuss_client_driver *adrv =
+		to_anybuss_client_driver(dev->driver);
+
+	if (adrv->remove)
+		return adrv->remove(to_anybuss_client(dev));
+	return 0;
+}
+
+static struct bus_type anybus_bus = {
+	.name		= "anybuss",
+	.match		= anybus_bus_match,
+	.probe		= anybus_bus_probe,
+	.remove		= anybus_bus_remove,
+};
+
+int anybuss_client_driver_register(struct anybuss_client_driver *drv)
+{
+	drv->driver.bus = &anybus_bus;
+	return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(anybuss_client_driver_register);
+
+void anybuss_client_driver_unregister(struct anybuss_client_driver *drv)
+{
+	return driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(anybuss_client_driver_unregister);
+
+/* ------------------------ attributes ------------------------ */
+
+static ssize_t state_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct anybuss_host *cd = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+		atomic_read(&cd->fieldbus_online) ?
+			"online" : "offline");
+}
+
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *ctrl_attrs[] = {
+	&dev_attr_state.attr,
+	NULL
+};
+
+static struct attribute_group ctrl_group = { .attrs = ctrl_attrs };
+
+static void client_device_release(struct device *dev)
+{
+	kfree(to_anybuss_client(dev));
+}
+
+static int taskq_alloc(struct device *dev, struct kfifo *q)
+{
+	void *buf;
+	size_t size = 64 * sizeof(struct ab_task *);
+
+	buf = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!buf)
+		return -EIO;
+	return kfifo_init(q, buf, size);
+}
+
+static int anybus_host_probe(struct platform_device *pdev)
+{
+	int ret, i;
+	u8 val[4];
+	u16 fieldbus_type;
+	struct anybuss_host *cd;
+	struct device *dev = &pdev->dev;
+	struct device *parent = dev->parent;
+	struct resource *res;
+	struct anybuss_host_pdata *pdata =
+				dev_get_platdata(&pdev->dev);
+
+	if (!pdata) {
+		dev_err(dev, "need platform data");
+		return -EINVAL;
+	}
+	if (!pdev->dev.parent) {
+		dev_err(dev, "need parent device");
+		return -EINVAL;
+	}
+	cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+	cd->dev = dev;
+	cd->parent = parent;
+	init_completion(&cd->card_boot);
+	init_waitqueue_head(&cd->wq);
+	init_waitqueue_head(&cd->dc_wq);
+	for (i = 0; i < ARRAY_SIZE(cd->qs); i++) {
+		ret = taskq_alloc(dev, &cd->qs[i]);
+		if (ret)
+			return ret;
+	}
+	if (WARN_ON(ARRAY_SIZE(cd->qs) < 3))
+		return -EINVAL;
+	cd->powerq = &cd->qs[0];
+	cd->mboxq = &cd->qs[1];
+	cd->areaq = &cd->qs[2];
+	cd->reset = pdata->reset;
+	cd->regmap = pdata->regmap;
+	spin_lock_init(&cd->qlock);
+	atomic_set(&cd->dc_event, 0);
+	atomic_set(&cd->fieldbus_online, 0);
+	cd->qcache = kmem_cache_create(dev_name(dev),
+		sizeof(struct ab_task), 0, 0, NULL);
+	if (!cd->qcache)
+		return -ENOMEM;
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(dev, "need irq resource");
+		ret = -ENOENT;
+		goto err_qcache;
+	}
+	cd->irq = res->start;
+	/* use a dpram test to check if a card is present, this is only
+	 * possible while in reset.
+	 */
+	cd->reset(parent, true);
+	if (test_dpram(cd->regmap)) {
+		dev_err(dev, "no Anybus-S card in slot");
+		ret = -ENODEV;
+		goto err_qcache;
+	}
+	ret = request_irq(cd->irq, irq_handler, IRQF_TRIGGER_LOW,
+			dev_name(dev), cd);
+	if (ret) {
+		dev_err(dev, "could not request irq");
+		goto err_qcache;
+	}
+	/* startup sequence:
+	 *   perform dummy IND_AB read to prevent false 'init done' irq
+	 *     (already done by test_dpram() above)
+	 *   release reset
+	 *   wait for first interrupt
+	 *   interrupt came in: ready to go !
+	 */
+	cd->reset(parent, false);
+	ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+	if (ret < 0)
+		goto err_irq;
+	/* according to the anybus docs, we're allowed to read these
+	 * without handshaking / reserving the area
+	 */
+	dev_info(dev, "Anybus-S card detected");
+	regmap_bulk_read(cd->regmap, REG_BOOTLOADER_V, val, 2);
+	dev_info(dev, "Bootloader version: %02X%02X",
+			val[0], val[1]);
+	regmap_bulk_read(cd->regmap, REG_API_V, val, 2);
+	dev_info(dev, "API version: %02X%02X", val[0], val[1]);
+	regmap_bulk_read(cd->regmap, REG_FIELDBUS_V, val, 2);
+	dev_info(dev, "Fieldbus version: %02X%02X", val[0], val[1]);
+	regmap_bulk_read(cd->regmap, REG_SERIAL_NO, val, 4);
+	dev_info(dev, "Serial number: %02X%02X%02X%02X",
+		val[0], val[1], val[2], val[3]);
+	regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type,
+				sizeof(fieldbus_type));
+	fieldbus_type = be16_to_cpu(fieldbus_type);
+	dev_info(dev, "Fieldbus type: %04X", fieldbus_type);
+	regmap_bulk_read(cd->regmap, REG_MODULE_SW_V, val, 2);
+	dev_info(dev, "Module SW version: %02X%02X",
+		val[0], val[1]);
+	/* put card back reset until a client driver releases it */
+	disable_irq(cd->irq);
+	cd->reset(parent, true);
+	atomic_set(&cd->ind_ab, IND_AB_UPDATED);
+	/* attributes */
+	ret = sysfs_create_group(&dev->kobj, &ctrl_group);
+	if (ret < 0)
+		goto err_irq;
+	cd->fieldbus_online_sd =
+		sysfs_get_dirent(dev->kobj.sd, "state");
+	if (!cd->fieldbus_online_sd) {
+		ret = -ENODEV;
+		goto err_sysfs;
+	}
+	/* fire up the queue thread */
+	cd->qthread = kthread_run(qthread_fn, cd, dev_name(dev));
+	if (IS_ERR(cd->qthread)) {
+		dev_err(dev, "could not create kthread");
+		ret = PTR_ERR(cd->qthread);
+		goto err_dirent;
+	}
+	/* now advertise that we've detected a client device (card).
+	 * the bus infrastructure will match it to a client driver.
+	 */
+	cd->client = kzalloc(sizeof(*cd->client), GFP_KERNEL);
+	if (!cd->client) {
+		ret = -ENOMEM;
+		goto err_kthread;
+	}
+	cd->client->fieldbus_type = fieldbus_type;
+	cd->client->host = cd;
+	cd->client->dev.bus = &anybus_bus;
+	cd->client->dev.parent = dev;
+	cd->client->dev.id = pdev->id;
+	cd->client->dev.release = client_device_release;
+	dev_set_name(&cd->client->dev, "%s.card0",
+				dev_name(&pdev->dev));
+	ret = device_register(&cd->client->dev);
+	if (ret)
+		goto err_client;
+	platform_set_drvdata(pdev, cd);
+	dev_set_drvdata(dev, cd);
+	return 0;
+err_client:
+	put_device(&cd->client->dev);
+err_kthread:
+	kthread_stop(cd->qthread);
+err_dirent:
+	sysfs_put(cd->fieldbus_online_sd);
+err_sysfs:
+	sysfs_remove_group(&dev->kobj, &ctrl_group);
+err_irq:
+	free_irq(cd->irq, cd);
+	cd->reset(parent, true);
+err_qcache:
+	kmem_cache_destroy(cd->qcache);
+	return ret;
+}
+
+static int anybus_host_remove(struct platform_device *pdev)
+{
+	struct anybuss_host *cd = platform_get_drvdata(pdev);
+
+	device_unregister(&cd->client->dev);
+	kthread_stop(cd->qthread);
+	sysfs_put(cd->fieldbus_online_sd);
+	sysfs_remove_group(&cd->dev->kobj, &ctrl_group);
+	free_irq(cd->irq, cd);
+	cd->reset(cd->parent, true);
+	kmem_cache_destroy(cd->qcache);
+	return 0;
+}
+
+static struct platform_driver anybus_host_driver = {
+	.probe = anybus_host_probe,
+	.remove = anybus_host_remove,
+	.driver	= {
+		.name = "anybuss-host",
+	},
+};
+
+static int __init anybus_init(void)
+{
+	int ret;
+
+	ret = bus_register(&anybus_bus);
+	if (ret) {
+		pr_err("could not register Anybus-S bus: %d\n", ret);
+		return ret;
+	}
+	return platform_driver_register(&anybus_host_driver);
+}
+module_init(anybus_init);
+
+static void __exit anybus_exit(void)
+{
+	platform_driver_unregister(&anybus_host_driver);
+	bus_unregister(&anybus_bus);
+}
+module_exit(anybus_exit);
+
+MODULE_DESCRIPTION("HMS Anybus-S Host Driver");
+MODULE_AUTHOR("Sven Van Asbroeck <svendev@arcx.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/anybuss-client.h b/include/linux/anybuss-client.h
new file mode 100644
index 000000000000..9d439d1a496f
--- /dev/null
+++ b/include/linux/anybuss-client.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Anybus-S client adapter definitions
+ *
+ * Copyright 2018 Arcx Inc
+ */
+
+#ifndef __LINUX_ANYBUSS_CLIENT_H__
+#define __LINUX_ANYBUSS_CLIENT_H__
+
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/poll.h>
+
+struct anybuss_host;
+
+struct anybuss_client {
+	struct device dev;
+	struct anybuss_host *host;
+	u16 fieldbus_type;
+};
+
+struct anybuss_client_driver {
+	struct device_driver driver;
+	int (*probe)(struct anybuss_client *adev);
+	int (*remove)(struct anybuss_client *adev);
+	u16 fieldbus_type;
+};
+
+int anybuss_client_driver_register(struct anybuss_client_driver *drv);
+void anybuss_client_driver_unregister(
+				struct anybuss_client_driver *drv);
+
+static inline struct anybuss_client *to_anybuss_client(
+					struct device *dev)
+{
+	return container_of(dev, struct anybuss_client, dev);
+}
+
+static inline struct anybuss_client_driver *to_anybuss_client_driver(
+					struct device_driver *drv)
+{
+	return container_of(drv, struct anybuss_client_driver, driver);
+}
+
+static inline void *
+anybuss_get_drvdata(const struct anybuss_client *client)
+{
+	return dev_get_drvdata(&client->dev);
+}
+
+static inline void
+anybuss_set_drvdata(struct anybuss_client *client, void *data)
+{
+	dev_set_drvdata(&client->dev, data);
+}
+
+int anybuss_set_power(struct anybuss_client *client, bool power_on);
+
+enum anybuss_offl_mode {
+	AB_OFFL_MODE_CLEAR = 0,
+	AB_OFFL_MODE_FREEZE,
+	AB_OFFL_MODE_SET
+};
+
+struct anybuss_memcfg {
+	u16 input_io;
+	u16 input_dpram;
+	u16 input_total;
+
+	u16 output_io;
+	u16 output_dpram;
+	u16 output_total;
+
+	enum anybuss_offl_mode offl_mode;
+};
+
+int anybuss_start_init(struct anybuss_client *client,
+			const struct anybuss_memcfg *cfg);
+int anybuss_finish_init(struct anybuss_client *client);
+int anybuss_read_fbctrl(struct anybuss_client *client, u16 addr,
+				void *buf, size_t count);
+int anybuss_send_msg(struct anybuss_client *client, u16 cmd_num,
+	const void *buf, size_t count);
+int anybuss_send_ext(struct anybuss_client *client, u16 cmd_num,
+	const void *buf, size_t count);
+int anybuss_recv_msg(struct anybuss_client *client, u16 cmd_num,
+	void *buf, size_t count);
+
+/* these help clients make a struct file_operations */
+int anybuss_write_input(struct anybuss_client *client,
+				const char __user *buf, size_t size,
+				loff_t *offset);
+int anybuss_read_output(struct anybuss_client *client, int *dc_event,
+				char __user *buf, size_t size,
+				loff_t *offset);
+unsigned int anybuss_poll(struct anybuss_client *client,
+		int dc_event, struct file *filp, poll_table *wait);
+
+#endif /* __LINUX_ANYBUSS_CLIENT_H__ */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-10-30 13:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 15:20 [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus thesven73
2018-10-30  9:55 ` Linus Walleij
2018-10-30 13:50   ` thesven73
  -- strict thread matches above, loose matches on Subject: below --
2018-10-24 14:24 [PATCH anybus v1 0/4] Support HMS Profinet Card over Anybus Sven Van Asbroeck
2018-10-24 14:24 ` [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus Sven Van Asbroeck
2018-10-24 15:58   ` Randy Dunlap
2018-10-25 11:08   ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).