Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
On Mon, 1 Jun 2020, Geert Uytterhoeven wrote: > > > > Sure, it could be absorbed by both asm/mac_iop.h and > > drivers/macintosh/adb-iop.c [...] > > asm/mac_iop.h doesn't include asm/adb_iop.h (at least not in my tree, > but perhaps you have plans to change that?), so there's only a single > user. What I meant by "both" was that part of asm/adb_iop.h could be absorbed by drivers/macintosh.adb-iop.c and the rest by asm/mac_iop.h. (And some of it could be tossed out.) I suspect that much of arch/m68k/include/asm could get the same treatment. But I doubt that there is any pay off, because the headers rarely change where they relate to hardware characteristics.
Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
Hi Finn,
On Mon, Jun 1, 2020 at 2:15 AM Finn Thain wrote:
> On Sun, 31 May 2020, Geert Uytterhoeven wrote:
> > On Sun, May 31, 2020 at 1:20 AM Finn Thain
> > wrote:
> > > arch/m68k/include/asm/adb_iop.h | 1 +
> > > drivers/macintosh/adb-iop.c | 32 ++--
> >
> > As this header file is used by a single source file only, perhaps it
> > should just be absorbed by the latter?
>
> Sure, it could be absorbed by both asm/mac_iop.h and
> drivers/macintosh/adb-iop.c but I don't see the point...
asm/mac_iop.h doesn't include asm/adb_iop.h (at least not in my tree,
but perhaps you have plans to change that?), so there's only a single
user.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
Hi Brad,
On Sun, May 31, 2020 at 10:01 PM Brad Boyer wrote:
> On Sun, May 31, 2020 at 10:38:04AM +0200, Geert Uytterhoeven wrote:
> > > arch/m68k/include/asm/adb_iop.h | 1 +
> > As this header file is used by a single source file only, perhaps it should
> > just be absorbed by the latter?
> > Then you no longer need my Acked-by for future changes ;-)
>
> While I don't really feel involved in this specific change (although I
> was one of the testers when the driver was first written), I am a little
> curious about the current coding standards. This header is pretty much
> just a declaration of the hardware interface, of which there are many
> in this directory. Is it better to just define all the offsets and bits
> for hardware registers inside the driver? We used to always put them in
> headers like this, but is that not the current standard? Would it be
> cleaner to put such headers in the directory with the driver effectively
> making them private? I seem to see quite a bit of that as well.
The idea behind header files is to share definitions among multipe
source files, right? It seems there is only one user of this header
file, so no sharing is involved, and thus these definitions are
de-facto private to the driver. Hence, the hardware declarations
could be absorbed by the driver source file.
In this case having two separate files makes maintenance more
difficult, as the two files belong to different maintainer spaces
(drivers/macintosh/ and arch/m68k/).
In general, moving header files to the driver directory is an option,
if nothing outside the driver directory needs it.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
On Sun, 31 May 2020, Geert Uytterhoeven wrote: > Hi Finn, > > On Sun, May 31, 2020 at 1:20 AM Finn Thain wrote: > > The adb_driver.autopoll method is needed during ADB bus scan and device > > address assignment. Implement this method so that the IOP's list of > > device addresses can be updated. When the list is empty, disable SRQ > > autopolling. > > > > Cc: Joshua Thompson > > Cc: Geert Uytterhoeven > > Tested-by: Stan Johnson > > Signed-off-by: Finn Thain > > Thanks for your patch! > > Acked-by: Geert Uytterhoeven > Thanks for your tag. > > arch/m68k/include/asm/adb_iop.h | 1 + > > drivers/macintosh/adb-iop.c | 32 ++-- > > As this header file is used by a single source file only, perhaps it > should just be absorbed by the latter? Sure, it could be absorbed by both asm/mac_iop.h and drivers/macintosh/adb-iop.c but I don't see the point... > Then you no longer need my Acked-by for future changes ;-) > But you shouldn't need to ack this kind of change in the first place. IMHO, the arch/m68k/mac maintainer should be the one to ack changes to both arch/m68k/include/asm/adb_iop.h and drivers/macintosh/adb-iop.c. Not that I'm complaining (thanks again for your ack!)
Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
On Sun, May 31, 2020 at 10:38:04AM +0200, Geert Uytterhoeven wrote: > > arch/m68k/include/asm/adb_iop.h | 1 + > > As this header file is used by a single source file only, perhaps it should > just be absorbed by the latter? > Then you no longer need my Acked-by for future changes ;-) While I don't really feel involved in this specific change (although I was one of the testers when the driver was first written), I am a little curious about the current coding standards. This header is pretty much just a declaration of the hardware interface, of which there are many in this directory. Is it better to just define all the offsets and bits for hardware registers inside the driver? We used to always put them in headers like this, but is that not the current standard? Would it be cleaner to put such headers in the directory with the driver effectively making them private? I seem to see quite a bit of that as well. Thank you for your advice. Brad Boyer f...@allandria.com
Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
Hi Finn,
On Sun, May 31, 2020 at 1:20 AM Finn Thain wrote:
> The adb_driver.autopoll method is needed during ADB bus scan and device
> address assignment. Implement this method so that the IOP's list of
> device addresses can be updated. When the list is empty, disable SRQ
> autopolling.
>
> Cc: Joshua Thompson
> Cc: Geert Uytterhoeven
> Tested-by: Stan Johnson
> Signed-off-by: Finn Thain
Thanks for your patch!
Acked-by: Geert Uytterhoeven
> arch/m68k/include/asm/adb_iop.h | 1 +
> drivers/macintosh/adb-iop.c | 32 ++--
As this header file is used by a single source file only, perhaps it should
just be absorbed by the latter?
Then you no longer need my Acked-by for future changes ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
The adb_driver.autopoll method is needed during ADB bus scan and device
address assignment. Implement this method so that the IOP's list of
device addresses can be updated. When the list is empty, disable SRQ
autopolling.
Cc: Joshua Thompson
Cc: Geert Uytterhoeven
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/m68k/include/asm/adb_iop.h | 1 +
drivers/macintosh/adb-iop.c | 32 ++--
2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/arch/m68k/include/asm/adb_iop.h b/arch/m68k/include/asm/adb_iop.h
index 195d7fb1268c..6aecd020e2fc 100644
--- a/arch/m68k/include/asm/adb_iop.h
+++ b/arch/m68k/include/asm/adb_iop.h
@@ -29,6 +29,7 @@
#define ADB_IOP_EXPLICIT 0x80/* nonzero if explicit command */
#define ADB_IOP_AUTOPOLL 0x40/* auto/SRQ polling enabled*/
+#define ADB_IOP_SET_AUTOPOLL 0x20/* set autopoll device list*/
#define ADB_IOP_SRQ0x04/* SRQ detected*/
#define ADB_IOP_TIMEOUT0x02/* nonzero if timeout
*/
diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index 8594e4f9a830..f3d1a460fbce 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -7,10 +7,6 @@
* 1999-07-01 (jmt) - First implementation for new driver architecture.
*
* 1999-07-31 (jmt) - First working version.
- *
- * TODO:
- *
- * o Implement SRQ handling.
*/
#include
@@ -28,6 +24,7 @@
static struct adb_request *current_req;
static struct adb_request *last_req;
+static unsigned int autopoll_devs;
static enum adb_iop_state {
idle,
@@ -123,7 +120,7 @@ static void adb_iop_listen(struct iop_msg *msg)
amsg->flags & ADB_IOP_AUTOPOLL);
}
- msg->reply[0] = ADB_IOP_AUTOPOLL;
+ msg->reply[0] = autopoll_devs ? ADB_IOP_AUTOPOLL : 0;
iop_complete_message(msg);
if (req_done)
@@ -231,9 +228,32 @@ static int adb_iop_write(struct adb_request *req)
return 0;
}
+static void adb_iop_set_ap_complete(struct iop_msg *msg)
+{
+ struct adb_iopmsg *amsg = (struct adb_iopmsg *)msg->message;
+
+ autopoll_devs = (amsg->data[1] << 8) | amsg->data[0];
+}
+
static int adb_iop_autopoll(int devs)
{
- /* TODO: how do we enable/disable autopoll? */
+ struct adb_iopmsg amsg;
+ unsigned long flags;
+ unsigned int mask = (unsigned int)devs & 0xFFFE;
+
+ local_irq_save(flags);
+
+ amsg.flags = ADB_IOP_SET_AUTOPOLL | (mask ? ADB_IOP_AUTOPOLL : 0);
+ amsg.count = 2;
+ amsg.cmd = 0;
+ amsg.data[0] = mask & 0xFF;
+ amsg.data[1] = (mask >> 8) & 0xFF;
+
+ iop_send_message(ADB_IOP, ADB_CHAN, NULL, sizeof(amsg), (__u8 *),
+adb_iop_set_ap_complete);
+
+ local_irq_restore(flags);
+
return 0;
}
--
2.26.2
