Re: [PATCH 00/11] ALSA: firewire-motu: add ioctl commands to retrieve information in messages delivered by isoc packet

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Sun, Oct 17, 2021 at 09:02:30AM +0200, Takashi Iwai wrote:
> On Sun, 17 Oct 2021 03:22:31 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On Fri, Oct 15, 2021 at 05:54:13PM +0200, Takashi Iwai wrote:
> > > On Fri, 15 Oct 2021 10:08:15 +0200,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > The purpose of this patchset is to add message parser to ALSA
> > > > firewire-motu driver so that userspace applications can read information
> > > > in message delivered by isochronous packet as well as PCM frames.
> > > > The message includes information about hardware meter and user action
> > > > over hardware component such as knob, and MIDI message bytes.
> > > > 
> > > > Models in MOTU FireWire series can be categorized to 4 groups in regard
> > > > of message mechanism:
> > > > 
> > > > Group 1. 828 and 896
> > > >  * quadlet message to registered destination address
> > > > Group 2. 828mk2, 896hd, Traveler, 8 pre, Ultralite, 4 pre, and Audio Express
> > > >  * quadlet message to registered destination address
> > > >  * message delivered by isochronous packet
> > > > Group 3. 828mk3, 896mk3, Ultralite mk3, Traveler mk3, and Track 16
> > > >  * quadlet message to registered destination address
> > > >  * message delivered by isochronous packet
> > > >  * block message to registered destination address, including command
> > > > Group 4. V3HD/V4HD
> > > >  * quadlet message to registered destination address
> > > >  * block message to registered destination address
> > > > 
> > > > The patchset is for message delivered by isochronous packet in group 2
> > > > and 3. In Group 2, the message includes information of hardware meter,
> > > > information of user action over hardware component. The model in Group
> > > > 2 is called as 'register DSP' in the patchset since parameters of DSP
> > > > can be configured by asynchronous transaction for register access. In
> > > > Group 3, the message includes information of hardware meter only. The
> > > > model in Group 3 is called as 'command DSP' since software and device
> > > > communicate with commands transferred by asynchronous transaction in
> > > > regard of DSP parameters. Two types of message parser is going to be
> > > > added so that the driver caches images for the information. The cache
> > > > is available for userspace application by ioctl commands newly introduced.
> > > > 
> > > > I note that no control element is added for the hardware meters and user
> > > > actions. It's expected for userspace application to retrieve and parse the
> > > > information of image then operate for user-defined control element set.
> > > > 
> > > > Takashi Sakamoto (11):
> > > >   ALSA: firewire-motu: add message parser to gather meter information in
> > > >     register DSP model
> > > >   ALSA: firewire-motu: add message parser for meter information in
> > > >     command DSP model
> > > >   ALSA: firewire-motu: add ioctl command to read cached hardware meter
> > > >   ALSA: firewire-motu: parse messages for mixer source parameters in
> > > >     register-DSP model
> > > >   ALSA: firewire-motu: parse messages for mixer output parameters in
> > > >     register DSP model
> > > >   ALSA: firewire-motu: parse messages for output parameters in register
> > > >     DSP model
> > > >   ALSA: firewire-motu: parse messages for line input parameters in
> > > >     register DSP model
> > > >   ALSA: firewire-motu: parse messages for input parameters in register
> > > >     DSP model
> > > >   ALSA: firewire-motu: add ioctl command to read cached DSP parameters
> > > >   ALSA: firewire-motu: queue event for parameter change in register DSP
> > > >     model
> > > >   ALSA: firewire-motu: notify event for parameter change in register DSP
> > > >     model
> > > 
> > > Applied all patches now.  Thanks.
> > 
> > Thanks for your applying the above patches into your tree. I have some
> > slight concerns about them. I'd like to ask your opinion.
> > 
> > The snd_firewire_motu_command_dsp_meter structure includes array of 32 bit
> > storage elements. As I added its documentation, the storage includes
> > IEEE 764 binary32 values between 0.0 and +1.0. In the patchset I use __u32
> > type since I can find just a few usage of float type in UAPI header. In
> > driver side, no floating point arithmetic is used since the float value
> > is just constructed by gathering messages from target device. In the case,
> > is it adequate to expose the value as float type in UAPI?
> >
> > Additionally, current ALSA control interface have no support for control
> > element with float value, like SNDRV_CTL_ELEM_TYPE_IEEE764_BINARY32. As
> > long as I know, no discussion about it in the list for recent decades.
> > Have you ever seen such discussion in the list? (Here I intensionally
> > ignore that we have several binary expressions for floating point since
> > I'm just interested in the existence of discussion.)
> 
> It's not been proposed, AFAIK.
> 
> The biggest concern is that, *if* any reference or calculation of the
> float type is required, what to do.  e.g. the kernel has a validation
> code for each values (min/max check), and how could it be implemented
> for the float type?

Indeed. It's probably unavoidable to min/max check and it brings issue
to ALSA control core. It's absolutely out of my scope and thanks for
your indication.

Would I ask you opinion about my concern about firewire UAPI header? Is
it allowed to use float type instead of __u32 type?

> thanks,
> 
> Takashi


Thanks

Takashi Sakamoto