[PATCH 1/1] Adding QEP driver to Beaglebone bsp

[Chris Johns]

On 23/8/20 7:51 pm, James Fitzsimons wrote:
> On Sat, 22 Aug 2020 at 20:41, Chris Johns <chrisj at rtems.org
> <mailto:chrisj at rtems.org>> wrote:
> 
>     On 21/8/20 8:01 pm, James Fitzsimons wrote:
>     ...
>     > +
>     > +
>     > +typedef void (*bbb_eqep_timer_callback)(BBB_PWMSS);
> 
>     What is BBB_PWMSS used for as an argument?
> 
> The BBB_PWMSS argument is provided to the user call back function so that the
> user knows which QEP module raised the interrupt.

Great, please add a comment to say this.

> It is quite likely that a user
> would have multiple QEP modules configured. Thinking about this now, the call
> back could also have the count provided as an argument so the user doesn't need
> to call the "int32_t beagle_qep_get_position(BBB_PWMSS pwmss_id);" function from
> their call back handler.

Sounds good. Adding something like this as a comment helps new users to the code.

>     Could a `void* user` be passed back? It helps if the interrupt provides a user
>     specific data pointer.
> 
> I'm sorry I don't understand what you mean here. Do you mean the argument to the
> call back should be "void* user" which presumably would be a pointer to an
> arbitrary data structure?

Add another argument that is a 'void*' that is a transparent user set value. A
user provides a pointer or NULL when configuring the QEP module and it is
returned here as an argument. The driver does not touch the value.

>     > +/**
>     > + * @brief This structure represents an eQEP module instance.
>     > + *
>     > + * The members are closely modelled on the FDT structure. *
>     > + */
>     > +typedef struct {
>     > +  const BBB_PWMSS pwmss_id;
>     > +  const uint32_t mmio_base;
>     > +  const rtems_vector_number irq;
>     > +  bbb_eqep_timer_callback timer_callback;
> 
>     Is this part of the FDT? I think some extra documentation here would be good to
>     have.
> 
> Good catch - that comment needs updating. When I first added the bbb_eqep
> structure it did closely resemble the structure used in the FDT configuration
> (at least for Linux). However, I then added additional members such as the
> rtems_vector_number and the bbb_eqep_timer_callback which have no relationship
> to the FDT. I can update the comment to more accurately reflect the current state.

Thank you.
 
>     Can we have a `void* user;`?
> 
> Are you suggesting replacing "bbb_eqep_timer_callback timer_callback;" with
> "void* user;"? If so yes I can do that. Is there a convention for passing
> arguments to that function from the interrupt handler?

No it is additional. I can then provide a private pointer to some private data I
have that is transparently passed back as an argument to the callback handler.
For example I may 2 or more separate pieces of code controlling separate QEP
modules each with a different set of data held in different type structures.
Configuing a pointer per QEP modules means I do not need to collect and hold
these pointers in global data structures I need to lock somehow to access from
the callback handler. I simply cast the `void*` to my data and then use it.

>  
> Many thanks for your review and comments!
> 

No problem and thank you. It is close.

Chris