On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:
> On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:
...
> > Thanks for this intermediate update, looks much better.
> > So, there are a few comments below and we are almost ready.
>
> Thanks, I would also like feedback on the usage of channel's
> ext_info to handle calibration instead of using raw attributes as
> suggested by Matt
Better to wait for Jonathan.
...
> > > + depends on SYSFS
>
> Do you think I need this ? The driver includes iio/sysfs.h but I found
> no symbol nor dependency for that
Ditto.
...
> > Also, since it's one time use, please drop the definition completely and use
> > literal in-place.
> I got two usages
> ...
> iio_dev->name = DRIVER_NAME;
>
> ...
> .driver = {
> .name = DRIVER_NAME,
>
> Is it ok to keep it ?
Ah, okay then!
...
> > > + errors = (const unsigned long *)&value;
> >
> > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.
>
> The usage of a pointer kind of puzzled me in first place, and I found
> no pattern to copy in the existing code base. I have probbaly not
> looked hard enough ?
>
> > unsigned long errors;
> >
> > ...
> >
> > errors = value;
> > for_each_set_bit(..., &errors, ...)
>
> I can do so but, for my education mostly, why do you think it would
> oops ? '*errors' is a pointer to a variable allocated on the stack as
> much as 'errors' you suggested is a local stack variable. I might be a
> bit slow today, but I'm missing the difference. FWIW I tested the
> function, even if there were no error bits set at the moment I tested, but
> the for_each_set_bit() macro was indeed run.
Because you theoretically access bytes behind 16-bit. That castings just ugly
and compiler should warn you about if it is clever enough.
If you found it in the existing code, please, fix it there, so quite bad
pattern won't spread.
> > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
...
> > > +static struct regmap_config sunrise_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> >
> > Does it need a lock?
> > (I think it does, but I would like to confirm)
>
> You know, I had the same doubt, but the description of a few fields of
> struct regmap_config lead me to think there's a locking mechanism
> already inplace
Exactly! But see below.
> * @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.
> * @lock: Optional lock callback (overrides regmap's default lock
> * function, based on spinlock or mutex).
>
> As you can see 'lock' is option and is said to override regmap's default
> lock functions. Locking seems to have be disabled explicitely
> through 'disable_locking' too. I was then expecting a reference to a
> locally declared mutex/spinlock to be passed through regmap_config
> if the regmap's locking mechanism requires driver-allocated locking
> primitives. This suggests to me there's an internal locking.
>
> In facts, regmap's core seems to have an internal mutex and a built-in
> locking mechanism, as shown by __regmap_init(), which selects the
> opportune locking primitive according to how regmap_config is
> initialized. In our case it seems it relies on the usage of the
> regmap_lock_mutex() and regmap_unlock_mutex() functions, which use
> struct regmap.mutex.
>
> I think we're then safe locking-wise, do you agree ?
My point is do you need regmap locking mechanism to be used or not?
> That said, as I'm not pushing to have the driver accepted for this
> merge window, for which we might be late already, I would wait for
> comments on the usage of the ext_channel abstraction from IIO people
> before submitting a new version if that's fine with everyone.
--
With Best Regards,
Andy Shevchenko
