At Tue, 27 Sep 2011 10:48:39 -0500, Pierre-Louis Bossart wrote:

Thanks for your review Takashi!

...

Wouldn't it be better to strip the buffer data and size in the error case? For example, radeon driver gives often size=0. Then it's translated as size=128 for a workaround, then again handled as an error by checking the version.

Yes, I wasn't sure what to do here and expected some comments. Should the strategy be that if there's a single error we discard the entire ELD by setting the size to zero? No issues for the other changes.

It's a good question. I'm inclined to discard all for any errors, in the case of ELD. AFAIK, most of errors are really broken ELD, and no need to care the rest of bits.

...

Set the access field to SNDRV_CTL_ELEM_ACCESS_READ. Also, unless we implement a notification, better to set SNDRV_CTL_ELEM_ACCESS_VOLATILE flag, too.

The intent was to read the ELD after the device is opened, I don't think the ELD can change during playback, the updates can only happen on a pin-sensing event. I can't think of a good reason why a notification would be needed?

It's just a matter of definition. If the data may be changed without notification, the element should set VOLATILE flag. You don't have to implement the notification mechanism if you don't want :)

thanks,

Takashi