Re: [PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 12 Oct 2023 01:07:10 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> On 11/10/23 01:08, Jonathan Cameron wrote:
> > 
> > No need to wrap the patch description quite so short. Aim
> > for up to 75 char for a commit message (and 80 for the code)
> > Here you are under 60.
> >   
> Thank you for taking time to point out these small issues.
> 
> >>
> >> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> >>  
> > There is a tag for datasheets in the format tags block so
> > Datasheet: https://docs.broadcom.com/doc/AV02-4755EN  
> >> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>  
> > 
> > I took a quick look at the most similar part number adps9300 and
> > this does look substantially different but could you confirm you've
> > taken a look at the plausible drivers to which support for this part
> > could be added and perhaps mention why that doesn't make sense
> > I think it will be mainly feature set being different here, but also
> > it seems they have completely different register maps despite similar
> > part numbers!  
> I have taken a look at quiet a few light sensor drivers including
> apds9960 and apds9300, as you said that they are different. There are
> another two drivers apds990x and apds9802als in drivers/misc which are
> also very different but I can't say that I have been through all the
> driver files.
> 

Great.  Then as expected this separate driver make sense even if the
DT bindings can be combined.  Would be nice if they standardised
the interface, but some companies seem to feel the need to start from
scratch for each device they produce :(


> 
> > The interrupt controller for starters takes to no locks and can run concurrently
> > with other accesses from other CPUs.  That seems unwise.
> >  
> Well, regarding device access, interrupt handler just reads the status registers
> thereby clearing the interrupt status flag and releasing the physical interrupt line.
> What can be the issue if I don't use a lock?
Gah. I was far too sleepy that day.  Glad you interpreted my comment as intended :)

Hmm. You will be relying on the internal implementation of the regmap bus interface
resulting in locks being taken in the i2c controller.  That may be fine, but
it makes me a little nervous that it's relying on a particular implementation.

My normal assumption is that any driver that turns off locking in regmap is doing
so because it has various complex read modify write cycles so needs to have it's
own locks - but that it also applies those locks everywhere regmap would have
done (so for duration of every regmap call).

You may be fine, but you aren't meeting the requirements documented.
The flag to disable locking in regmap states:

 * @disable_locking: This regmap is either protected by external means or
 *                   is guaranteed not to be accessed from multiple threads.
 *                   Don't use any locking mechanisms.

It doesn't say you are fine for simple accesses and there are multiple threads
accessing 'the regmap'.

Unless you really care about it, I'd just leave regmap locking enabled.
The likely performance hit on a device on a slow bus is low and it avoids
us having to think too hard about this.


> >> +	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);  
> > 
> > Why at this point? I'd have thought it wasn't powered up until init_device()
> > which follows?  So I'd expect to see this call after that, not before.
> >   
> Right. I will do a bit more reading on this before using this. I assumed this
> functions registers the callback which gets called at driver release by the
> subsystem similar to release().

That's true, but with the addition that it is called in the reverse order of
being add to the devm managed release list.  So ordering matters.


> 
> Thank you Jonathan for the review. I'll get the changes done in the next version.
> 
No problem.   As a side note, feel free to just crop out any responses where
you agree with a review.  Default assumption is that if you don't comment that
is the case and it cuts down on scrolling when reviewer next looks.
They are also much more likely to take a look at a short reply than a long one!

Jonathan

> Regards,
> Subhajit Ghosh
> 
> 
> 
>