Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1401078 > unrolled thread

Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Robert Jarzmik <robert.jarzmik@free.fr> - 2016-05-14 12:00 +0200
    Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Takashi Iwai <tiwai@suse.de> - 2016-05-14 17:20 +0200
      Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Robert Jarzmik <robert.jarzmik@free.fr> - 2016-05-16 04:10 +0200
        Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Takashi Iwai <tiwai@suse.de> - 2016-05-16 07:50 +0200
          Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Robert Jarzmik <robert.jarzmik@free.fr> - 2016-05-16 11:00 +0200
            Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Takashi Iwai <tiwai@suse.de> - 2016-05-16 15:00 +0200
              Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Mark Brown <broonie@kernel.org> - 2016-05-16 15:20 +0200

#1401078 — Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus

Takashi Iwai <tiwai@suse.de> writes:

> On Sat, 30 Apr 2016 23:15:34 +0200,
> Robert Jarzmik wrote:
>> 
>> diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h
>> new file mode 100644
>> index 000000000000..4b8b3e570892
>> --- /dev/null
>> +++ b/include/sound/ac97/codec.h
>> @@ -0,0 +1,98 @@
>> +/*
>> + *  Copyright (C) 2016 Robert Jarzmik <robert.jarzmik@free.fr>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef AC97_CODEC_H
>> +#define AC97_CODEC_H
>
> Let's be careful about the choice of the guard.
Ok, would _SND_AC97_CODEC_H be better ?

>> +#define AC97_ID(vendor_id1, vendor_id2) \
>> +	(((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff))
>> +#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \
>> +	{ .id = ((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff), \
>> +	  .mask = ((mask_id1 & 0xffff) << 16) | (mask_id2 & 0xffff), \
>> +	  .data = _data }
>
> Give parentheses around the macro arguments.
Right, for RFC v2.

>> +struct ac97_codec_device {
>> +	struct device		dev;	/* Must stay first member */
>
> This doesn't have to be the first element as long as you use container_of().
Ah yes, that's a leftover from a former idea, I'll remove that comment.

In the initial code I'd done "struct ac97_codec_device" was hidden from this
file (ie. there was only a "struct ac97_codec_device;" statement), the body of
the struct was contained in sound/ac97/ac97_core.h.

The only provided macro to access the "struct device" inside "struct
ac97_codec_device" was relying on this "trick" (that's a bit like in the
video4linux area).

Anyway, good point, I'll remove that.

>> +struct ac97_codec_driver {
>> +	struct device_driver	driver;
>> +	int			(*probe)(struct ac97_codec_device *);
>> +	int			(*remove)(struct ac97_codec_device *);
>> +	int			(*suspend)(struct ac97_codec_device *);
>> +	int			(*resume)(struct ac97_codec_device *);
>> +	void			(*shutdown)(struct ac97_codec_device *);
>> +	struct ac97_id		*id_table;
>
> Missing const?
Ah no, unfortunately not, or rather not yet.

I tried that one, not very hard, but at least ac97_bus_match() with the pair
"struct ac97_id *id = adrv->id_table" and "do { } while (++id->id);" is not
possible AFAIK with a const.

I will see if I can come up with something better for ac97_bus_match, such as
array usage instead of pointer arithmetics.

>> +};
>> +
>> +int ac97_codec_driver_register(struct ac97_codec_driver *drv);
>> +void ac97_codec_driver_unregister(struct ac97_codec_driver *drv);
>> +
>> +static inline struct device *
>> +ac97_codec_dev2dev(const struct ac97_codec_device *adev)
>> +{
>> +	return (struct device *)(adev);
>
> What's wrong with the simple &adev->dev ?  Cast looks scary.
The same leftover than above, I'll change that for RFC v2.

>> +struct ac97_controller {
>> +	const struct ac97_controller_ops *ops;
>> +	struct list_head controllers;
>> +	struct device *dev;
>> +	int bus_idx;
>
> What is this bus_idx for?
Initially it was to distinguish 2 different AC97 controllers. In the current
patchset state, it's not usefull anymore AFAICS.
So let's remove it.

>> +	int bound_codecs;
The same comment would apply here. I don't think that information is important
anymore. I thought I would use that to prevent AC97 controler removal while
codecs are still bound.

In a second thought what would be better is to have get_device() called for each
bound codec which will prevent ac97_digital_controller_unregister() to succeed
(-EBUSY).

>> +	struct list_head codecs;
>> +};
>> +
>> +int ac97_digital_controller_register(const struct ac97_controller_ops *ops,
>> +				     struct device *dev);
>> +int ac97_digital_controller_unregister(const struct device *dev);
>> +
>> +#endif
>> diff --git a/sound/ac97/Kconfig b/sound/ac97/Kconfig
>> new file mode 100644
>> index 000000000000..fd2c2d031e62
>> --- /dev/null
>> +++ b/sound/ac97/Kconfig
>> @@ -0,0 +1,9 @@
>> +#
>> +# PCI configuration
>> +#
>  
> Still only for PCI? :)
Ouch ;) I'll amend that for RFC v2.
>
>> +
>> +config AC97
>> +	bool "AC97 bus"
>> +	help
>> +	   Say Y here if you want to have AC97 devices, which are sound oriented
>> +	   devices around an AC-Link.
>> diff --git a/sound/ac97/Makefile b/sound/ac97/Makefile
>> new file mode 100644
>> index 000000000000..5575909d46e2
>> --- /dev/null
>> +++ b/sound/ac97/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# make for AC97 bus drivers
>> +#
>> +
>> +obj-y	+= bus.o codec.o snd_ac97_compat.o
>
> No possibility for modules?
There should be, so I'll put that on my TODO list for RFC v2.

>> +static struct ac97_codec_device *
>> +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num)
>> +{
>> +	struct ac97_codec_device *codec;
>> +
>> +	list_for_each_entry(codec, &ac97_ctrl->codecs, list)
>> +		if (codec->num == codec_num)
>> +			return codec;
>> +
>> +	return NULL;
>> +}
>
> It's a question whether we need to manage the codecs in the linked
> list.  There can be at most 4 codecs, so it fits in an array well,
> too.  Then some codes like this would be simpler. (And it'll even
> reduce the footprint, too.)
Agreed. For RFC v2.

>> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97,
>> +				      int codec_num)
>> +{
>> +	struct ac97_codec_device codec;
>> +	unsigned short vid1, vid2;
>> +	int ret;
>> +
>> +	codec.dev = *ac97->dev;
>> +	codec.num = codec_num;
>> +	ret = ac97->ops->read(&codec, AC97_VENDOR_ID1);
>> +	vid1 = (ret & 0xffff);
>> +	if (ret < 0)
>> +		return 0;
>
> Hmm.  This looks pretty hackish and dangerous.
You mean returning 0 even if the read failed, right ?
A better prototype would probably be (for RFC v2):
  int ac97_bus_scan_one(struct ac97_controller *ac97, int codec_num,
                        unsigned int *vendor_id);

>> +static int ac97_bus_scan(struct ac97_controller *ac97_ctrl)
>> +{
>> +	int ret, i;
>> +	unsigned int vendor_id;
>> +
>> +	for (i = 0; i < AC97_BUS_MAX_CODECS; i++) {
>> +		if (ac97_codec_find(ac97_ctrl, i))
>> +			continue;
>> +		vendor_id = ac97_bus_scan_one(ac97_ctrl, i);
>> +		if (!vendor_id)
>> +			continue;
>> +
>> +		ret = ac97_codec_add(ac97_ctrl, i, vendor_id);
>> +		if (ret < 0)
>> +			return ret;
>
> This is one of concerns: we don't know whether the device really
> reacts well if you access to a non-existing slot.  At least, it'd be
> safer to have the masks for the devices we already know the slots.

Ah you mean upon ac97 controller registration, the
ac97_digital_controller_register() should provide the information for each of
the 4 slots :
 - does the controller enable this slot (default yes)
 - does the controller support auto-scan for this slot (default yes)
   I'm not sure this "feature" is required, it looks a bit over-engineered.

That could be a matter of 1 or 2 masks as input parameters to
ac97_digital_controller_register().

>> +static int ac97_bus_reset(struct ac97_controller *ac97_ctrl)
>> +{
>> +	struct ac97_codec_device codec;
>> +
>> +	memset(&codec, 0, sizeof(codec));
>> +	codec.dev = *ac97_ctrl->dev;
>> +
>> +	ac97_ctrl->ops->reset(&codec);
>
> So, this assumes that reset ops is mandatory?  Then document it at
> least.
Ok, for RFC v2.

Thanks for your review and feedbacks Takashi, I'll work on both Mark and your
comments in the next weeks.

Cheers.

-- 
Robert

[toc] | [next] | [standalone]

#1401095

On Sat, 14 May 2016 11:50:50 +0200,
Robert Jarzmik wrote:
> >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97,
> >> +				      int codec_num)
> >> +{
> >> +	struct ac97_codec_device codec;
> >> +	unsigned short vid1, vid2;
> >> +	int ret;
> >> +
> >> +	codec.dev = *ac97->dev;
> >> +	codec.num = codec_num;
> >> +	ret = ac97->ops->read(&codec, AC97_VENDOR_ID1);
> >> +	vid1 = (ret & 0xffff);
> >> +	if (ret < 0)
> >> +		return 0;
> >
> > Hmm.  This looks pretty hackish and dangerous.
> You mean returning 0 even if the read failed, right ?

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

IMO, a cleaner way would be to define the ops passed with both
controller and codec objects as arguments, and pass NULL codec here.


Takashi

[toc] | [prev] | [next] | [standalone]

#1401252

Takashi Iwai <tiwai@suse.de> writes:

> On Sat, 14 May 2016 11:50:50 +0200,
> Robert Jarzmik wrote:
>> >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97,
>> >> +				      int codec_num)
>> >> +{
>> >> +	struct ac97_codec_device codec;
>> >> +	unsigned short vid1, vid2;
>> >> +	int ret;
>> >> +
>> >> +	codec.dev = *ac97->dev;
>> >> +	codec.num = codec_num;
>> >> +	ret = ac97->ops->read(&codec, AC97_VENDOR_ID1);
>> >> +	vid1 = (ret & 0xffff);
>> >> +	if (ret < 0)
>> >> +		return 0;
>> >
>> > Hmm.  This looks pretty hackish and dangerous.
>> You mean returning 0 even if the read failed, right ?
>
> 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 ...

Cheers.

-- 
Robert

[toc] | [prev] | [next] | [standalone]

#1401320

On Sun, 15 May 2016 23:29:27 +0200,
Robert Jarzmik wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > On Sat, 14 May 2016 11:50:50 +0200,
> > Robert Jarzmik wrote:
> >> >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97,
> >> >> +				      int codec_num)
> >> >> +{
> >> >> +	struct ac97_codec_device codec;
> >> >> +	unsigned short vid1, vid2;
> >> >> +	int ret;
> >> >> +
> >> >> +	codec.dev = *ac97->dev;
> >> >> +	codec.num = codec_num;
> >> >> +	ret = ac97->ops->read(&codec, AC97_VENDOR_ID1);
> >> >> +	vid1 = (ret & 0xffff);
> >> >> +	if (ret < 0)
> >> >> +		return 0;
> >> >
> >> > Hmm.  This looks pretty hackish and dangerous.
> >> You mean returning 0 even if the read failed, right ?
> >
> > 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?


Takashi

[toc] | [prev] | [next] | [standalone]

#1401380

Takashi Iwai <tiwai@suse.de> writes:

> On Sun, 15 May 2016 23:29:27 +0200,
> Robert Jarzmik wrote:
>> 
>> Takashi Iwai <tiwai@suse.de> 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.

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.

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 ?

Cheers.

-- 
Robert

[toc] | [prev] | [next] | [standalone]

#1401482

On Mon, 16 May 2016 10:53:56 +0200,
Robert Jarzmik wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > On Sun, 15 May 2016 23:29:27 +0200,
> > Robert Jarzmik wrote:
> >> 
> >> Takashi Iwai <tiwai@suse.de> 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

[toc] | [prev] | [next] | [standalone]

#1401491

[Multipart message — attachments visible in raw view] — view raw

On Mon, May 16, 2016 at 02:58:13PM +0200, Takashi Iwai wrote:
> Robert Jarzmik wrote:

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

I can't think of any, even for some of the more fancy CODECs used in
embedded systems.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web