On 11/04/16 17:11, Crestez Dan Leonard wrote:
> On 04/07/2016 10:59 PM, Peter Meerwald-Stadler wrote:
>>> static int max44000_probe(struct i2c_client *client,
>>> const struct i2c_device_id *id)
>>> {
>>> @@ -513,6 +569,12 @@ static int max44000_probe(struct i2c_client *client,
>>> return ret;
>>> }
>>>
>>> + ret = iio_triggered_buffer_setup(indio_dev, NULL, max44000_trigger_handler, NULL);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "iio triggered buffer setup failed\n");
>>> + return ret;
>>> + }
>>> +
>>> return iio_device_register(indio_dev);
>>
>> no devm_ possible anymore :-)
>
> It's not clear to me why explicit calls to
> iio_triggered_buffer_cleanup are done in every driver. Wouldn't it be
> possible for iio_dev_release to call iio_triggered_buffer_cleanup?
> That would require triggered_buffer_cleanup to explictly NULL the
> fields it deallocates so that duplicate cleanups don't crash.
Because not all devices are using the triggered buffer setup function
thus they can't use the cleanup one. There are different buffers for
starters and the core has no way to identify them (very deliberately
as it should never care what they are).

There is also the general obvious correctness of code question.
If you do one thing in probe, it should be reverse in remove...

We could in theory do a devm version of the iio_triggered_buffer_cleanup
function though as then the tear down would occur in the write order and
as it is a devm_ call in the probe we'd know how the unwind was being
done...

> --
> Regards,
> Leonard

2016-04-18 10:32:37

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 1/5] max44000: Initial commit

On Sun, Apr 17, 2016 at 09:36:10AM +0100, Jonathan Cameron wrote:
> On 11/04/16 16:08, Crestez Dan Leonard wrote:

Please leave blank lines between paragraphs, it makes things much easier
to read.

> > Would it be
> > acceptable to just expand the REGMASK_* into a large or-ing of (1 <<
> > MAX44000_REG_*)? Then it would be clear in the source what's going on
> > but binary will be the same.

> Would be interesting to see but I doubt the optimized code will be that
> different, and the switch is pretty much the 'standard' way of handling
> these long register lists cleanly.

> Often it comes down to doing things the way people expect them to
> be done as that makes review easier for a tiny possible cost in
> run time.

You can also specify ranges of registers if the map mostly has large
blocks of contiguous registers, a switch statement tends to be easier
and is probably more efficient for most register maps.

> >>>> + .use_single_rw = 1,
> >>>> + .cache_type = REGCACHE_FLAT,

> >> This always seems like a good idea, but tends to cause issues.
> >> FLAT is really only meant for very high performance devices, you
> >> are probably better with something else here. If you are doing this
> >> deliberately to make the below writes actually occur, then please
> >> add a comment here.

> > I used REGCACHE_FLAT because my device has a very small number of
> > registers and I assume it uses less memory. Honestly it would make
> > sense for regmap to include a REGCACHE_AUTO cache_type and pick the
> > cache implementation automatically based on number of registers.

> I've fallen for that one in the past as well. AUTO would indeed
> be good if it was easy to do.

It's extremely easy to do. Unless you've got a good reason to do
anything else you should always be using an rbtree. The core would
never select anything else.

> > Yes. It would not work otherwise since the regmap cache is explicitly
> > initialized with my listed defaults.
> > As far as I can tell regmap_write will always write to the hardware.

> Interesting and counter intuitive if true...

No, if the driver asked to write then we write. If the driver wants to
do a read/modify/write cycle it should use regmap_update_bits().

> > If the device had a reset command I should have used that, right?
> > What is happening is that I am implementing a reset command in
> > software.

> Not necessarily. Lots of drivers don't - but instead have their interfaces
> reflect their current state on startup. Reset's are often there to get
> the internal state of the device cleaned up if it is in an unknowable state
> rather than to get the defaults to any particular state. They are always
> read from the hardware or a known good cache when queried from userspace
> anyway.

That's not entirely it. Doing a reset is often faster than rewriting
the entire register map and is more robust against undocumented
registers or things the driver didn't think about which means that
the behaviour is going to be more consistent.

Attachments:

(No filename) (2.95 kB)
signature.asc (473.00 B)
Download all attachments

2016-04-18 10:59:19

On Mon, Apr 18, 2016 at 03:15:54PM +0300, Crestez Dan Leonard wrote:

> As a further clarification: regmap_write will write to hardware even if
> the cache is known to be up-to-date and no matter the regcache_type. Did
> I understand this correctly?

> I'm basing this on reading the code, it seems to me that map->reg_write
> is only avoided on error paths or if map->cache_only is set to true.

> This always-write guarantee is not obvious and if it's OK for drivers to
> rely on it perhaps it should be explicitly documented on regmap_write.

Yes. I have to say that you are the first person I've encountered who
has been confused by this, I'm not sure why you'd expect writes to be
discarded.

Attachments:

(No filename) (698.00 B)
signature.asc (473.00 B)
Download all attachments

2016-04-18 19:36:24

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 1/5] max44000: Initial commit

On 18/04/16 13:34, Mark Brown wrote:
> On Mon, Apr 18, 2016 at 03:15:54PM +0300, Crestez Dan Leonard wrote:
>
>> As a further clarification: regmap_write will write to hardware even if
>> the cache is known to be up-to-date and no matter the regcache_type. Did
>> I understand this correctly?
>
>> I'm basing this on reading the code, it seems to me that map->reg_write
>> is only avoided on error paths or if map->cache_only is set to true.
>
>> This always-write guarantee is not obvious and if it's OK for drivers to
>> rely on it perhaps it should be explicitly documented on regmap_write.
>
> Yes. I have to say that you are the first person I've encountered who
> has been confused by this, I'm not sure why you'd expect writes to be
> discarded.
>
It confused me too :) To my mind a classic cache optimization would be
to not write to the hardware if the value is already known to be as
desired.

Still, I guess it would add another check to identify which
registers you really wanted to hammer whatever vs which can be assumed
not to read the write is not worth the effort for this case that
inherently won't be hit that often.

Jonathan

2016-04-18 19:38:23

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 1/5] max44000: Initial commit

On 18/04/16 11:32, Mark Brown wrote:
> On Sun, Apr 17, 2016 at 09:36:10AM +0100, Jonathan Cameron wrote:
>> On 11/04/16 16:08, Crestez Dan Leonard wrote:
>
> Please leave blank lines between paragraphs, it makes things much easier
> to read.
>
>>> Would it be
>>> acceptable to just expand the REGMASK_* into a large or-ing of (1 <<
>>> MAX44000_REG_*)? Then it would be clear in the source what's going on
>>> but binary will be the same.
>
>> Would be interesting to see but I doubt the optimized code will be that
>> different, and the switch is pretty much the 'standard' way of handling
>> these long register lists cleanly.
>
>> Often it comes down to doing things the way people expect them to
>> be done as that makes review easier for a tiny possible cost in
>> run time.
>
> You can also specify ranges of registers if the map mostly has large
> blocks of contiguous registers, a switch statement tends to be easier
> and is probably more efficient for most register maps.
>
>>>>>> + .use_single_rw = 1,
>>>>>> + .cache_type = REGCACHE_FLAT,
>
>>>> This always seems like a good idea, but tends to cause issues.
>>>> FLAT is really only meant for very high performance devices, you
>>>> are probably better with something else here. If you are doing this
>>>> deliberately to make the below writes actually occur, then please
>>>> add a comment here.
>
>>> I used REGCACHE_FLAT because my device has a very small number of
>>> registers and I assume it uses less memory. Honestly it would make
>>> sense for regmap to include a REGCACHE_AUTO cache_type and pick the
>>> cache implementation automatically based on number of registers.
>
>> I've fallen for that one in the past as well. AUTO would indeed
>> be good if it was easy to do.
>
> It's extremely easy to do. Unless you've got a good reason to do
> anything else you should always be using an rbtree. The core would
> never select anything else.
>
>>> Yes. It would not work otherwise since the regmap cache is explicitly
>>> initialized with my listed defaults.
>>> As far as I can tell regmap_write will always write to the hardware.
>
>> Interesting and counter intuitive if true...
>
> No, if the driver asked to write then we write. If the driver wants to
> do a read/modify/write cycle it should use regmap_update_bits().
>
>>> If the device had a reset command I should have used that, right?
>>> What is happening is that I am implementing a reset command in
>>> software.
>
>> Not necessarily. Lots of drivers don't - but instead have their interfaces
>> reflect their current state on startup. Reset's are often there to get
>> the internal state of the device cleaned up if it is in an unknowable state
>> rather than to get the defaults to any particular state. They are always
>> read from the hardware or a known good cache when queried from userspace
>> anyway.
>
> That's not entirely it. Doing a reset is often faster than rewriting
> the entire register map and is more robust against undocumented
> registers or things the driver didn't think about which means that
> the behaviour is going to be more consistent.
Hmm. Fair enough on the undocumented register argument...
>

2016-04-19 09:07:14

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 1/5] max44000: Initial commit

On Mon, Apr 18, 2016 at 08:36:19PM +0100, Jonathan Cameron wrote:
> On 18/04/16 13:34, Mark Brown wrote:

> > Yes. I have to say that you are the first person I've encountered who
> > has been confused by this, I'm not sure why you'd expect writes to be
> > discarded.

> It confused me too :) To my mind a classic cache optimization would be
> to not write to the hardware if the value is already known to be as
> desired.

> Still, I guess it would add another check to identify which
> registers you really wanted to hammer whatever vs which can be assumed
> not to read the write is not worth the effort for this case that
> inherently won't be hit that often.

We'd also have to add another API for cases where someone explicitly
wants to write the same thing to the hardware, you get things like
latched "do it" bits in registers.

Attachments:

(No filename) (840.00 B)
signature.asc (473.00 B)
Download all attachments