Add DT Overlay parameter for drive mode

[quorten]

No description provided.

[quorten]

For the most part I'm putting this pull request out to get comments on the ideas, I've only done basic testing but I have to admit I like the improvements so far to state the HD setting upon driver load. For one thing, the README changes make reference to a hardware v5 that does not exist yet.

[garlick]

I think you should split off the uninstall + whitespace changes into another PR (two separate commits) that we can merge without delay.

Making small changes to the hardware to support very basic SCSI, as proposed in #40, is a cool idea. However, and I mean this as a real question, not rhetorically: why should the parport driver stack be involved? Why not instead just implement a SCSI driver that talks directly to the hardware? Or for a user space only solution, instead of using ppdev, what about controlling the GPIOs directly from user space? (Note I have not studied the RASCSI project so I'm not sure what approach is taken over there)

Assuming there a use case for both totem pole and open drain drive in the parport_gpio driver, I would suggest that we use a DTS option to set the value instead of mapping it to a control register biit, since it's not likely to need to be switched back and forth while the driver is loaded. For example, config.txt could contain

dtoverlay=parport-gpio,hd=1

As for mapping CONTROL5 to an extra GPIO, that's probably fine to just do unconditionally. (I think CONTROL4 is the one that was once used to enable/disable interrupts and should be left alone?). Obviously, if you planned to appropriate a pin on the 26 pin connector we'd need to be careful about how that's implemented.

Finally on documentation, I think it might be clearer to document the conversion of a future V5 board to a SCSI interface on a wiki page that we perhaps reference from the main README, rather than in the README proper, to retain project focus on the IEEE 1284-ish parport.

[quorten]

Regarding implementation of additional control signals, really I was mainly adding that because I thought there might exist some Linux software that can emulate a "device" side (like a printer) via the parport driver stack, but that is an unproven assumption. Otherwise, for SCSI for example, RaSCSI does it all via the BCM GPIO registers. For the drive signal configuration, I agree that it would be better to do the config via DeviceTree for initialization. The main reason why I thought about adding it as a control option would be if we had some software that wanted to do the configuration all via ppdev.

Yeah, so I guess if you put the theoretical assumptions aside, this would be mainly about improving configuration of the HD pin when loading and printing out the configured state upon load.

[garlick]

OK, I'm open to adding the device tree option to set HD if that's useful. Sorry to push you into the device tree swamp - I really hate dealing with the device tree source syntax.

Just a thought: when you print the hd pin status, it might be useful to print the human translation of the bit value, e.g.

parport-gpio ppgpio@0: hd=1 (totem pole) on pin 20

[quorten]

I've made most of the necessary changes in my local repository, but one thing that seems confusing is how this will play out with auto-loading from DT Overlay specified in the EEPROM, it doesn't seem that you can rewrite the EEPROM to set the parameters there unless you store the entire DT Overlay with the parameter "baked" into the EEPROM.

[garlick]

I don't think that's necessarily a show stopper. People wanting to change the default can always use config.txt to load the overlay.

[quorten]

I pushed my changes and they're ready for review, in case you didn't get a notification from the push.

[garlick]

Oh thanks, I missed the notification. This looks good to me. I would give it some testing but I'm away from home for the week. Can you vouch that the dt parameter works as advertised?

Edit: I mean can you verify at least that the hd value follows the override in config.txt? (I wouldn't expect you to have hardware yet to verify the drive characteristics!)

[quorten]

Sure, I've been testing with dtoverlay to pass the parameter and here is the corresponding dmesg output. I'd say it looks like it's working, considering the fact that we read the pin state value when showing the output.

[1442404.505275] parport-gpio ppgpio@0: data on pins [22,23,24,10,25,9,8,11]
[1442404.505307] parport-gpio ppgpio@0: status on pins [18,17,4,3,2]
[1442404.505322] parport-gpio ppgpio@0: control on pins [26,19,6,13]
[1442404.505339] parport-gpio ppgpio@0: hd=1 (totem pole) on pin 20
[1442404.505351] parport-gpio ppgpio@0: dir on pin 21
[1442448.828859] parport-gpio ppgpio@0: data on pins [22,23,24,10,25,9,8,11]
[1442448.828891] parport-gpio ppgpio@0: status on pins [18,17,4,3,2]
[1442448.828907] parport-gpio ppgpio@0: control on pins [26,19,6,13]
[1442448.828923] parport-gpio ppgpio@0: hd=0 (open drain) on pin 20
[1442448.828934] parport-gpio ppgpio@0: dir on pin 21

EDIT: This should be the same as specifying the parameter in config.txt but without needing to wait for the restart every time during testing!

[quorten]

I just pushed one checkstyle fix but otherwise no functional changes.

[garlick]

Nice! Thank you for iterating on that. Let's merge!