[PATCH] Beagle BSP Improvements (GPIO driver)

[Ben Gras]

All,

Big improvement. The video shows that the bit logic for setting &
clearing is likely sound - for the first byte at least ;-). Good test.

My comments about the code are
  - gpio_select_pin write a whole 0 byte instead of updating a single
bit, as we talked about on IRC
  - gedare's suggestion to use a single function to update bits in
registers is good - you can use this for gpio_select_pin, gpio_set and
gpio_clear.
  - i don't like the addition of arm_delay() in the bsp code. if it's
not bsp-specific (and it isn't), it shouldn't be there.
  - the AM335X-specific code isn't made conditional

My comment about the API that is:

I don't like it that hardware-specific info (GPIO base registers) is
somewhat external to the BSP. To my mind, the application should only
deal with bank numbers and pin numbers. So either we need 128 more
#defines of the GPIO_PIN struct in the current vein, meaning inline
struct initializers are passed around (ew) and we rely on quite a lot
of app discipline to keep it clean (ew); or we need an api like

gpio_config(&gpio_conf);  /* retrieve number of gpio banks & pins per
bank, possibly more info */
gpio_select_pin(&gpio_pin, bank_number, pin_number, DIGITAL_OUTPUT);
/* fill in gpio_pin struct to use this pin */
gpio_set(&gpio_pin);
gpio_clear(&gpio_pin);
gpio_release_pin(&gpio_pin);  /* allows repurposing this pin */

this way the configuring of gpio_pin (containing hardware-specific
info) is more internal to the BSP, and the app would have to actively
subvert the api (look in the gpio_pin struct) and is less prone to be
written badly by accident (hardcoding hardware addresses in the app).

Thoughts anyone?



On Thu, Apr 23, 2015 at 8:35 PM, Ketul Shah <ketulshah1993 at gmail.com> wrote:
> Hi all,
>
> Ben Gras thanks for your comments and encouragement for merging with
> mainline as a motivation for me.
>
> I worked and redeveloped code for gpio driver also tried to approach towards
> common api. I tried to address most of the comments. Herewith I attached my
> patch. Would be happy to hear comments on that.
>
> You can also find out my gist or commit on https://github.com/RTEMS/rtems .
> I have been updating my work on https://github.com/ketul93/RTEMS-on-BBB.
>
> Live demo of the updated code is found on :- https://youtu.be/bXurelOA3i0
>
> Thanks.
>
> Regards,
> ketul
>
>
> On 20 April 2015 at 19:50, Ben Gras <beng at shrike-systems.com> wrote:
>>
>> All,
>>
>> Good contribution, thank you.
>>
>> For GSOC, this is good proof of being able to progress and get
>> something real working based on documentation. Great!
>>
>> If you want this merged with mainline - which I fully encourage! -
>> then I suggest the following should be added to/changed first:
>>
>>   - make the code that uses AM335X (beaglebone) specific registers,
>> AM335X-specific :) i.e. add  #if IS_AM335X around code that should
>> only execute there. this BSP can be compiled for 2 different SOCs.
>>   - let it control all GPIO's - there are 4 banks of 32 each on the
>> AM335x, but you only let the user control GPIO1. there should be a
>> clean interface to control them all (clean means mostly: without
>> duplicating code)
>>   - as Gedare mentioned on IRC, copy the RPi API. this is a first
>> (second?) step to finding a generalized GPIO API.
>>   - don't add beagleboard.h, but add your definitions to
>> libcpu/arm/shared/include/am335x.h
>>   - the code should use a more consistent indenting style, and make
>> variable names more descriptive than 'tmp'.
>>   - don't leave the configure changes in like in acinclude.m4
>>
>> bonus:
>>   - add DM3730 (beagleboard) support.
>>
>> Good luck!
>>
>>
>> On Fri, Apr 17, 2015 at 7:51 PM, Ketul Shah <ketulshah1993 at gmail.com>
>> wrote:
>> > Hello,
>> >
>> > I have proposed in GSoC on Beagle BSP Improvements. As starting I
>> > started
>> > working for gpio driver development . At this stage I am able to
>> > demonstrate
>> > USR LEDs pattern.
>> >
>> > Videos of that can be found here on YouTube.
>> > https://youtu.be/B3mSsfo-PAQ &
>> > https://youtu.be/M1aKpOhUvv4.
>> >
>> > Herewith I have attached patch.txt file. Alternatively you can have
>> > https://gist.github.com/ketul93/2e0d2c4b8b586be62e1d that includes the
>> > newly
>> > added files code. I would be happy to hear your reviews and changes on
>> > my
>> > work.
>> >
>> > And also I have been updating my work on
>> > https://github.com/ketul93/RTEMS-on-BBB repo.
>> >
>> > It would be great to have your comments on my proposal.
>> >
>> > Thanks and regards,
>> > ketul
>> >
>> > _______________________________________________
>> > devel mailing list
>> > devel at rtems.org
>> > http://lists.rtems.org/mailman/listinfo/devel
>
>