On Mon, 16 May 2016 10:53:56 +0200, Robert Jarzmik wrote: > > Takashi Iwai <tiwai@xxxxxxx> writes: > > > On Sun, 15 May 2016 23:29:27 +0200, > > Robert Jarzmik wrote: > >> > >> Takashi Iwai <tiwai@xxxxxxx> writes: > >> > >> > On Sat, 14 May 2016 11:50:50 +0200, > >> > > >> > No, my concern is that it's creating a dummy codec object temporarily > >> > on the stack just by copying some fields and calling the ops with it. > >> > (And actually the current code may work wrongly because lack of > >> > zero-clear of the object.) > >> Ah yes, I remember now, the on-stack generated device, indeed ugly. > >> > >> > IMO, a cleaner way would be to define the ops passed with both > >> > controller and codec objects as arguments, and pass NULL codec here. > >> It's rather unusual to need both the device and its controller in bus > >> operations. I must admit I have no better idea so far, so I'll try that just to > >> see how it looks like, and let's see next ... > > > > Thinking of this again, I wonder now why we need to pass the codec > > object at all. It's the read/write ops via ac97, so we just need the > > ac97_controller object and the address slot of the accessed codec? > So far it would work. The only objection I would see is if in the future the bus > operation needs a specialization which is ac97 codec dependent, such as a flag > or a mask in ac97_codec_device structure. > > Even if I'd like to not have these in bus operations, the struct snd_ac97 had a > need for a 'caps', 'ext_id', ... fields for example. Yet these could be > contained in the ac97_codec_device structure and not exposed to bus operations. Do we have any example of such exceptions? For AC97, we don't need to think of future extensions at all. If any, we may provide two levels of ops abstractions: the lower ac97_controller_ops.read/write, and the upper ac97_codec_ops read/write that may override if defined, but as default it just wraps over controller ops. But I don't think we'd need such unless we really see the demand to be exposed outside the codec driver itself. > Another worry is the pattern (as an example) in atmel_ac97c_write() in > sound/atmel/ac97.c, where the codec structure is used to get the controller > through a container_of() type call. Yet passing the controller to bus operations > takes care of this one. Right. > From a "purely API" point of view the couple (ac97_controller, ac97_slot_id) is > what will route an ac97 bus operation, be that a read/write/reset/..., the > remaining question is will it cover the cases we've not thought of ? The remaining ops are rather codec-specific operations, and they don't have to be implemented in the bus (controller) level. We should keep the bus ops as small as possible. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
