Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx>
>> + *
>> + * 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
