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 >
