Thanks for all the input. I will look at the adc driver in a bit more detail to see which option is best. My original thought was to do what you had done Andrzej: if the device is already opened, just return OK. But I will look at the various locks and such to determine if there will be issues here.
Thanks all > On Jun 6, 2018, at 3:17 PM, Andrzej Kaczmarek <andrzej.kaczma...@codecoup.pl> > wrote: > > Hi, > > +1 for proper ref counting > btw, trng_nrf52 already works this way except it does not check ref > counter explicitly but OS_DEV_F_STATUS_OPEN flag instead. This is also > already used in Mynewt as NimBLE host, controller and TinyCrypt can > (depending on configuration) just open trng device on its own and > there is no need to open it "globally". > > One thing that may need to be fixed though is some locking when > calling open/close handlers since they are now called unlocked and ref > counter is updated later. Since ref counter is checked in these > handlers this may cause device to be initialized more than once and > also to be not un-initialized when all references are dropped. Both > are not a problem for trng_nrf52 driver so I did not worry about it > too much ;-) > > Best, > Andrzej > > > On Wed, Jun 6, 2018 at 10:56 PM will sanfilippo <wi...@runtime.io> wrote: >> >> Chris and Vipul: >> >> That does indeed seem to be a decent option! I will see if others chime in >> and if not, add that change. >> >> Thanks!!! >> >>> On Jun 6, 2018, at 1:32 PM, Vipul Rahane <vi...@runtime.io> wrote: >>> >>> >>>> On Jun 6, 2018, at 1:01 PM, Christopher Collins <ch...@runtime.io> wrote: >>>> >>>> On Wed, Jun 06, 2018 at 11:57:34AM -0700, will sanfilippo wrote: >>>>> Chris: >>>>> >>>>> I might be missing something here, but given that os_dev already has a >>>>> reference count and that handles multiple folks opening/closing the >>>>> device, does the underlying adc driver need a reference count itself? If >>>>> it just returned no error if opened again this would be fine. >>>>> >>>>> I do note that os_dev_open() and os_dev_close() always call the >>>>> open/close handlers regardless of reference count. I wonder if that >>>>> should be changed (meaning only call open/close once)? >>>> >>>> No, you aren't missing anything; I just misunderstood the os_dev >>>> reference counting. Thanks for setting me straight :). >>>> >>>> Another option: the ADC open function checks its os_dev's reference >>>> count. If the value is greater than zero, then return without doing >>>> anything. >>>> >>> >>> I was going to suggest that as well. Seems like a simple solution :-) >>> >>>> Chris >>>> >>>>> >>>>> >>>>>> On Jun 6, 2018, at 10:13 AM, Christopher Collins <ch...@runtime.io> >>>>>> wrote: >>>>>> >>>>>> On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: >>>>>>> Hello: >>>>>>> >>>>>>> I am not the most familiar with the ADC device so it is possible that >>>>>>> it was being used incorrectly but in any event I ran across something I >>>>>>> wanted to discuss. The call to os_dev_open() allows a device to be >>>>>>> opened multiple times (there is a reference count there). However, the >>>>>>> call to nrf52_adc_open() returns an error (OS_EBUSY) if the device has >>>>>>> already been opened. >>>>>>> >>>>>>> This presented a problem in the following case: consider two >>>>>>> independent packages both of which want to use ADC_0. Each package is >>>>>>> going to attempt to open the ADC device (since it has no idea if it was >>>>>>> already opened) but the second attempt to open the device will result >>>>>>> in an error code returned. Depending on how the code is written in the >>>>>>> package, this could be a problem. Given that an ADC is almost always a >>>>>>> mutli-channel peripheral (one adc device has multple channels) I would >>>>>>> suspect the above case to be common: multiple packages wanting an ADC >>>>>>> channel from a single device. >>>>>>> >>>>>>> I am not sure if anything needs to be done here; just wanted to see if >>>>>>> folks thought there should different behavior with regards to the >>>>>>> function returning an error if the device was already opened. If not, >>>>>>> folks are going to have to be careful when they write code using the >>>>>>> adc device. Seems to me if nothing is going to change we have two >>>>>>> options: >>>>>>> >>>>>>> 1) The device gets created and opened in some place and handed to the >>>>>>> packages that need it. >>>>>>> 2) The device gets created (say by the bsp) and each package can >>>>>>> attempt to open the device. If os_dev_lookup() returns !NULL but >>>>>>> os_dev_open() returns NULL it means that the device has already been >>>>>>> opened. >>>>>>> >>>>>>> Something about #2 just sort of bothers me. I do not like ambiguous >>>>>>> stuff like that; how do you know if there was an error for another >>>>>>> reason? >>>>>> >>>>>> Why not: >>>>>> >>>>>> 3) Make the ADC driver consistent with other drivers by adding a >>>>>> reference count. >>>>>> >>>>>> ? >>>>>> >>>>>> I know something less than nothing about the ADC code, so I could >>>>>> certainly be missing something. >>>>>> >>>>>> Chris >>>>> >>> >>> - Vipul >>
