Patchwork [1/5] rayer_spi: Improve support for different pinouts

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2013-04-06 01:12:22
Message ID <515F7676.6080002@gmx.net>
Download mbox | patch
Permalink /patch/3931/
State Superseded
Headers show

Comments

Carl-Daniel Hailfinger - 2013-04-06 01:12:22
Here is the same patch, with my suggested changes and some other stuff
on top (constification, naming the Xilinx DLC-5 cable "dlc5" in
anticipation of the buffered DLC-5 variant) to avoid changing things
twice. I tried to dig up the history of this patch, hopefully I got it
right.

Am 01.04.2013 23:55 schrieb Kyösti Mälkki:
> Create a list of programmer types with names. This list could be
> listed with flashrom -L in follow-up patches.
>
> Handle a bit in status register that is inverted, this will be used
> in different future programmer types.
>
> Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
> Tested-by: Maksim Kuleshov <mmcx@mail.ru>
> Acked-by: Kyösti Mälkki <kyosti.malkki@gmail.com>

rayer_spi: Rework handling of programmer types

Store rayer_spi programmer types with configuration data in an array.
Bit 7 of the LPT status register is inverted, automatically handle this
for future users.
The Xilinx DLC-5 cable is now selected with type=dlc-5 instead of
dev=xilinx.

Patch originally by Maksim Kuleshov, reworked by Kyösti Mälkki and
Carl-Daniel Hailfinger.

Maksim/Kyösti, can I get your signoff?

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Kyösti Mälkki - 2013-04-06 03:37:54
On Sat, 2013-04-06 at 03:12 +0200, Carl-Daniel Hailfinger wrote:
> Here is the same patch, with my suggested changes and some other stuff
> on top (constification, naming the Xilinx DLC-5 cable "dlc5" in
> anticipation of the buffered DLC-5 variant) to avoid changing things
> twice. I tried to dig up the history of this patch, hopefully I got it
> right.
> 
> Am 01.04.2013 23:55 schrieb Kyösti Mälkki:
> > Create a list of programmer types with names. This list could be
> > listed with flashrom -L in follow-up patches.
> >
> > Handle a bit in status register that is inverted, this will be used
> > in different future programmer types.
> >
> > Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
> > Tested-by: Maksim Kuleshov <mmcx@mail.ru>
> > Acked-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
> 
> rayer_spi: Rework handling of programmer types
> 
> Store rayer_spi programmer types with configuration data in an array.
> Bit 7 of the LPT status register is inverted, automatically handle this
> for future users.
> The Xilinx DLC-5 cable is now selected with type=dlc-5 instead of
> dev=xilinx.
> 
> Patch originally by Maksim Kuleshov, reworked by Kyösti Mälkki and
> Carl-Daniel Hailfinger.
> 
> Maksim/Kyösti, can I get your signoff?
> 

Why did You remove the Signed-off by lines from the patch in the first
place? I thought there was a policy to only add at the end of
Signed-off-by lines. And if you only do minimal rebase or rework, note
that between Your own sign-off.

Yes, you can return my Signed-off-by in there.

Kyösti
Carl-Daniel Hailfinger - 2013-04-06 17:08:20
Am 06.04.2013 05:37 schrieb Kyösti Mälkki:
> On Sat, 2013-04-06 at 03:12 +0200, Carl-Daniel Hailfinger wrote:
>> Here is the same patch, with my suggested changes and some other stuff
>> on top (constification, naming the Xilinx DLC-5 cable "dlc5" in
>> anticipation of the buffered DLC-5 variant) to avoid changing things
>> twice. I tried to dig up the history of this patch, hopefully I got it
>> right.
>>
>> Am 01.04.2013 23:55 schrieb Kyösti Mälkki:
>>> Create a list of programmer types with names. This list could be
>>> listed with flashrom -L in follow-up patches.
>>>
>>> Handle a bit in status register that is inverted, this will be used
>>> in different future programmer types.
>>>
>>> Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
>>> Tested-by: Maksim Kuleshov <mmcx@mail.ru>
>>> Acked-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
>> rayer_spi: Rework handling of programmer types
>>
>> Store rayer_spi programmer types with configuration data in an array.
>> Bit 7 of the LPT status register is inverted, automatically handle this
>> for future users.
>> The Xilinx DLC-5 cable is now selected with type=dlc-5 instead of
>> dev=xilinx.
>>
>> Patch originally by Maksim Kuleshov, reworked by Kyösti Mälkki and
>> Carl-Daniel Hailfinger.
>>
>> Maksim/Kyösti, can I get your signoff?
>>
> Why did You remove the Signed-off by lines from the patch in the first
> place? I thought there was a policy to only add at the end of
> Signed-off-by lines. And if you only do minimal rebase or rework, note
> that between Your own sign-off.

Indeed. But your comments in response to the "[flashrom] [PATCH] Support
device lists for programmers without PCI/USB IDs" thread sounded like
you didn't want to be associated with the rework I was doing, and I
wanted to avoid a situation where your signoff is associated with a
patch you don't like.

Do you agree with the patch summary I posted above, or is it incorrect?
I tried to dig up all the mails similar to this patch and hope I got the
"original by/reworked by" comment right.

> Yes, you can return my Signed-off-by in there.

Thanks. I'll wait for Maksim's signoff confirmation before committing.

Regards,
Carl-Daniel
Maksim Kuleshov - 2013-04-06 22:02:43
Hi.

I don't understand why change "xilinx" on "dlc-5". Changes command line , but 
does not change the behavior of the program. Users have worked well the current 
version, now there will be problems. Call flashrom can be integrated into the 
various programmes, and change the startup options can be very difficult. In the 
documents of the Xilinx name of the "dlc-5" is very rare. More often adapter is called 
"Parallel Cable III".It is probably better not to change the name of "xilinx". 

> Thanks. I'll wait for Maksim's signoff confirmation before committing.

I tested only the ByteblasterMV and Wiggler, but their 
support is not included in this patch.


Maksim
Stefan Tauner - 2013-04-06 22:43:14
On Sun, 7 Apr 2013 02:02:43 +0400
Maksim Kuleshov <mmcx@mail.ru> wrote:

> I don't understand why change "xilinx" on "dlc-5". Changes command line , but 
> does not change the behavior of the program. Users have worked well the current 
> version, now there will be problems. Call flashrom can be integrated into the 
> various programmes, and change the startup options can be very difficult. In the 
> documents of the Xilinx name of the "dlc-5" is very rare. More often adapter is called 
> "Parallel Cable III".It is probably better not to change the name of "xilinx". 

Thanks for your considerations regarding the name, that's a valid
point. Using some consistent scheme to differentiate the various types
is favorable, therefore using the model name instead of the company
name generally speaking makes sense. Also, I consider breaking
applications that use the CLI instead of libflashrom a good thing. We
should do that more often so that either they get so annoyed that their
authors review the libflashrom patches or annoy us back enough so that
we eventually integrate libflashrom. This was somewhat sarcastic, but my
main point is: the CLI should not be used by other programs. Regarding
users... that's an excellent point because I am not sure how well the
help texts (manpage and --L output) are after this change. Carl-Daniel?

> > Thanks. I'll wait for Maksim's signoff confirmation before committing.
> 
> I tested only the ByteblasterMV and Wiggler, but their 
> support is not included in this patch.

Signing off is about the code, not about testing or reviewing. It
indicates that you were authorized to contribute that code under the
given license to this project. And in this case Carl-Daniel implies
with it general consent to what he has done with your code when refining
the patches. I interpret your considerations regarding the type
parameter as disagreement, but I might be wrong...
Maksim Kuleshov - 2013-04-06 23:57:09
> Thanks for your considerations regarding the name, that's a valid

> point. Using some consistent scheme to differentiate the various types

> is favorable, therefore using the model name instead of the company

> name generally speaking makes sense. Also, I consider breaking

> applications that use the CLI instead of libflashrom a good thing. We

> should do that more often so that either they get so annoyed that their

> authors review the libflashrom patches or annoy us back enough so that

> we eventually integrate libflashrom. This was somewhat sarcastic, but my

> main point is: the CLI should not be used by other programs. Regarding

> users... that's an excellent point because I am not sure how well the

> help texts (manpage and --L output) are after this change. Carl-Daniel?


Coercion, is always worse than the possible choices. To use the flashrom not need to be программистом. libflashrom requires C 	ABI compatibility. How to be with the integration of python, php, perl, ocaml, lisp, etc.? Who write for these language bindings, and will keep them up to date? A good practice in case of need to delete or change the name, is the creation of a new name as a synonym of the old, and the announcement of the old name as "deprecated".

> Signing off is about the code, not about testing or reviewing. It

> indicates that you were authorized to contribute that code under the

> given license to this project. And in this case Carl-Daniel implies

> with it general consent to what he has done with your code when refining

> the patches. I interpret your considerations regarding the type

> parameter as disagreement, but I might be wrong...


I will sign an agreement with the license for this patch.
Kyösti Mälkki - 2013-04-07 05:44:27
On Sat, 2013-04-06 at 19:08 +0200, Carl-Daniel Hailfinger wrote:
> Am 06.04.2013 05:37 schrieb Kyösti Mälkki:
> > On Sat, 2013-04-06 at 03:12 +0200, Carl-Daniel Hailfinger wrote:
> >> Here is the same patch, with my suggested changes and some other stuff
> >> on top (constification, naming the Xilinx DLC-5 cable "dlc5" in
> >> anticipation of the buffered DLC-5 variant) to avoid changing things
> >> twice. I tried to dig up the history of this patch, hopefully I got it
> >> right.
> >>
> >> Am 01.04.2013 23:55 schrieb Kyösti Mälkki:
> >>> Create a list of programmer types with names. This list could be
> >>> listed with flashrom -L in follow-up patches.
> >>>
> >>> Handle a bit in status register that is inverted, this will be used
> >>> in different future programmer types.
> >>>
> >>> Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
> >>> Tested-by: Maksim Kuleshov <mmcx@mail.ru>
> >>> Acked-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
> >> rayer_spi: Rework handling of programmer types
> >>
> >> Store rayer_spi programmer types with configuration data in an array.
> >> Bit 7 of the LPT status register is inverted, automatically handle this
> >> for future users.
> >> The Xilinx DLC-5 cable is now selected with type=dlc-5 instead of
> >> dev=xilinx.
> >>
> >> Patch originally by Maksim Kuleshov, reworked by Kyösti Mälkki and
> >> Carl-Daniel Hailfinger.
> >>
> >> Maksim/Kyösti, can I get your signoff?
> >>
> > Why did You remove the Signed-off by lines from the patch in the first
> > place? I thought there was a policy to only add at the end of
> > Signed-off-by lines. And if you only do minimal rebase or rework, note
> > that between Your own sign-off.
> 
> Indeed. But your comments in response to the "[flashrom] [PATCH] Support
> device lists for programmers without PCI/USB IDs" thread sounded like
> you didn't want to be associated with the rework I was doing, and I
> wanted to avoid a situation where your signoff is associated with a
> patch you don't like.
> 

I have submitted the patch with my Signed-off-by previously. The fact
that I do not like the rework You have done here cannot change the
"Chain of Trust" or "Certificate of Origin" the Signed-off-by procedure
is about.

More importantly: For further changes in this rayer_spi patchset, remove
the Tested-By and Acked-By lines.

> Do you agree with the patch summary I posted above, or is it incorrect?
> I tried to dig up all the mails similar to this patch and hope I got the
> "original by/reworked by" comment right.

Summary is fine.

> > Yes, you can return my Signed-off-by in there.
> 
> Thanks. I'll wait for Maksim's signoff confirmation before committing.

Maksim, would you keep track of the effort (in time) You need to put in
re-testing the patchsets and commenting on ML.

> Regards,
> Carl-Daniel
> 

Thanks,
  Kyösti

Patch

Index: flashrom-maksim_rayer_spi_rework_type_selection/rayer_spi.c
===================================================================
--- flashrom-maksim_rayer_spi_rework_type_selection/rayer_spi.c	(Revision 1667)
+++ flashrom-maksim_rayer_spi_rework_type_selection/rayer_spi.c	(Arbeitskopie)
@@ -19,6 +19,7 @@ 
 
 /* Driver for the SPIPGM hardware by "RayeR" Martin Rehak.
  * See http://rayer.ic.cz/elektro/spipgm.htm for schematics and instructions.
+ * Other LPT-based SPI programming hardware is supported as well.
  */
 
 /* This driver uses non-portable direct I/O port accesses which won't work on
@@ -37,22 +38,50 @@ 
 #include "programmer.h"
 #include "hwaccess.h"
 
-enum rayer_type {
-	TYPE_RAYER,
-	TYPE_XILINX_DLC5,
-};
-
 /* We have two sets of pins, out and in. The numbers for both sets are
  * independent and are bitshift values, not real pin numbers.
  * Default settings are for the RayeR hardware.
  */
-/* Pins for master->slave direction */
-static int rayer_cs_bit = 5;
-static int rayer_sck_bit = 6;
-static int rayer_mosi_bit = 7;
-/* Pins for slave->master direction */
-static int rayer_miso_bit = 6;
 
+struct noid_dev_entry {
+	const char *type;
+	const enum test_state status;
+	const char *description;
+	const void *dev_data;
+};
+
+struct rayer_pinout {
+	uint8_t cs_bit;
+	uint8_t sck_bit;
+	uint8_t mosi_bit;
+	uint8_t miso_bit;
+	void (*init)(void *);
+	int (*shutdown)(void *);
+};
+
+static const struct rayer_pinout rayer_spipgm = {
+	.cs_bit = 5,
+	.sck_bit = 6,
+	.mosi_bit = 7,
+	.miso_bit = 6,
+};
+
+static const struct rayer_pinout xilinx_dlc5 = {
+	.cs_bit = 2,
+	.sck_bit = 1,
+	.mosi_bit = 0,
+	.miso_bit = 4,
+};
+
+/* List of supported devices, first one is the default. */
+static const struct noid_dev_entry rayer_spi_devs[] = {
+	{"rayer",	NT,	"RayeR SPIPGM",			  	&rayer_spipgm},
+	{"dlc-5",	NT,	"Xilinx Parallel Cable III (DLC 5)", 	&xilinx_dlc5},
+	{0},
+};
+
+static struct rayer_pinout *pinout = NULL;
+
 static uint16_t lpt_iobase;
 
 /* Cached value of last byte sent. */
@@ -60,22 +89,22 @@ 
 
 static void rayer_bitbang_set_cs(int val)
 {
-	lpt_outbyte &= ~(1 << rayer_cs_bit);
-	lpt_outbyte |= (val << rayer_cs_bit);
+	lpt_outbyte &= ~(1 << pinout->cs_bit);
+	lpt_outbyte |= (val << pinout->cs_bit);
 	OUTB(lpt_outbyte, lpt_iobase);
 }
 
 static void rayer_bitbang_set_sck(int val)
 {
-	lpt_outbyte &= ~(1 << rayer_sck_bit);
-	lpt_outbyte |= (val << rayer_sck_bit);
+	lpt_outbyte &= ~(1 << pinout->sck_bit);
+	lpt_outbyte |= (val << pinout->sck_bit);
 	OUTB(lpt_outbyte, lpt_iobase);
 }
 
 static void rayer_bitbang_set_mosi(int val)
 {
-	lpt_outbyte &= ~(1 << rayer_mosi_bit);
-	lpt_outbyte |= (val << rayer_mosi_bit);
+	lpt_outbyte &= ~(1 << pinout->mosi_bit);
+	lpt_outbyte |= (val << pinout->mosi_bit);
 	OUTB(lpt_outbyte, lpt_iobase);
 }
 
@@ -83,8 +112,8 @@ 
 {
 	uint8_t tmp;
 
-	tmp = INB(lpt_iobase + 1);
-	tmp = (tmp >> rayer_miso_bit) & 0x1;
+	tmp = INB(lpt_iobase + 1) ^ 0x80; // bit 7 is inverted
+	tmp = (tmp >> pinout->miso_bit) & 0x1;
 	return tmp;
 }
 
@@ -99,8 +128,9 @@ 
 
 int rayer_spi_init(void)
 {
+	/* Pick the first entry in rayer_spi_devs as default. */
+	const struct noid_dev_entry *dev = rayer_spi_devs;
 	char *arg = NULL;
-	enum rayer_type rayer_type = TYPE_RAYER;
 
 	/* Non-default port requested? */
 	arg = extract_programmer_param("iobase");
@@ -138,36 +168,18 @@ 
 
 	arg = extract_programmer_param("type");
 	if (arg) {
-		if (!strcasecmp(arg, "rayer")) {
-			rayer_type = TYPE_RAYER;
-		} else if (!strcasecmp(arg, "xilinx")) {
-			rayer_type = TYPE_XILINX_DLC5;
-		} else {
-			msg_perr("Error: Invalid device type specified.\n");
+		for (; dev->type; ++dev)
+			if (!strcasecmp(arg, dev->type))
+				break;
+		if(!dev->type) {
+			msg_perr("Error: Invalid device type \"%s\"specified.\n", arg);
 			free(arg);
 			return 1;
 		}
+		free(arg);
 	}
-	free(arg);
-	switch (rayer_type) {
-	case TYPE_RAYER:
-		msg_pdbg("Using RayeR SPIPGM pinout.\n");
-		/* Bits for master->slave direction */
-		rayer_cs_bit = 5;
-		rayer_sck_bit = 6;
-		rayer_mosi_bit = 7;
-		/* Bits for slave->master direction */
-		rayer_miso_bit = 6;
-		break;
-	case TYPE_XILINX_DLC5:
-		msg_pdbg("Using Xilinx Parallel Cable III (DLC 5) pinout.\n");
-		/* Bits for master->slave direction */
-		rayer_cs_bit = 2;
-		rayer_sck_bit = 1;
-		rayer_mosi_bit = 0;
-		/* Bits for slave->master direction */
-		rayer_miso_bit = 4;
-	}
+	msg_pinfo("Using %s pinout.\n", dev->description);
+	pinout = (struct rayer_pinout *) dev->dev_data;
 
 	if (rget_io_perms())
 		return 1;
@@ -175,6 +187,11 @@ 
 	/* Get the initial value before writing to any line. */
 	lpt_outbyte = INB(lpt_iobase);
 
+	if (pinout->shutdown)
+		register_shutdown(pinout->shutdown, pinout);
+	if (pinout->init)
+		pinout->init(pinout);
+
 	if (bitbang_spi_init(&bitbang_spi_master_rayer))
 		return 1;
 
Index: flashrom-maksim_rayer_spi_rework_type_selection/flashrom.8
===================================================================
--- flashrom-maksim_rayer_spi_rework_type_selection/flashrom.8	(Revision 1667)
+++ flashrom-maksim_rayer_spi_rework_type_selection/flashrom.8	(Arbeitskopie)
@@ -715,7 +715,7 @@ 
 syntax where
 .B model
 can be
-.BR rayer " for the RayeR cable or " xilinx " for the Xilinx Parallel Cable III
+.BR rayer " for the RayeR cable or " dlc-5 " for the Xilinx Parallel Cable III
 (DLC 5).
 .sp
 More information about the RayeR hardware is available at