archive mirror
 help / color / mirror / Atom feed
Subject: Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
Date: Tue, 30 Oct 2018 09:50:41 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

>> 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.


  reply	other threads:[~2018-10-30 13:50 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
  -- 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).