Hello,

On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote:

On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:

quoted

On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:

quoted

On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:

...

quoted

quoted

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.

...

quoted

quoted

quoted

+	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.

...

quoted

quoted

Also, since it's one time use, please drop the definition completely and use
literal in-place.

quoted

I got two usages
        ...
	iio_dev->name = DRIVER_NAME;

        ...
	.driver = {
		.name = DRIVER_NAME,

Is it ok to keep it ?

Ah, okay then!

...

quoted

quoted

quoted

+	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 ?

quoted

	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.

I don't think there's such a risk as I limit the search space to
ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change
to what you suggested

If you found it in the existing code, please, fix it there, so quite bad
pattern won't spread.

My point was that I was not able to find any pattern to copy from :)

quoted

quoted

quoted

+	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
+		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);

...

quoted

quoted

quoted

+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.

quoted

 * @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?

Oh I see, sorry for the useless digression, you were asking if I
should not disable locking, not the other way around!

Anyway, there's a risk for a concurrent read/write when in example
reading the error status and triggering a calibration. There's no
locking at the driver level in those functions as they do not access
shared driver's fields, but on the wire there might be conflicting
transactions, so I think I should keep locking in place.

Thanks
   j

quoted

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