Re: [PATCH v4 02/12] ALSA: ac97: add an ac97 bus

[Takashi Iwai]

On Mon, 24 Jul 2017 22:49:18 +0200,
Robert Jarzmik wrote:
> 
> AC97 is a bus for sound usage. It enables for a AC97 AC-Link to link one
> controller to 0 to 4 AC97 codecs.
> 
> The goal of this new implementation is to implement a device/driver
> model for AC97, with an automatic scan of the bus and automatic
> discovery of AC97 codec devices.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@xxxxxxx>
> ---
> Since RFCv1:
>  - Takashi's review
>    - changed the codec.h guard ... a better name could be found ...
>    - added the AC97_* macros missing parenthesis
>    - constantified the id_table in the codec driver structure
>    - changed the 4 codecs linked list into an array
>    - enabled the ac97 bus to be a module
>    - added a slots_available to snd_ac97_controller_register() to have a
>      way to prevent scanning and probing of unconnected codecs
>    - removed useless ac97 bus index
>    - all exported functions begin with snd_ac97_*()
>    - change bus operations to controller+slot parameters instead of
>      codec device
> 
>  - Mark's review
>    - changed ac97_digital_controller into ac97_controller
>    - rename ac97_digital_controller_*() into ac97_controller_*()
>    - add the ac97 ac-link clock to the codec device (ie. the AC'97
>      BIT_CLK)
> 
> Since RFCv2:
>  - more snd_ac97 namespace review
>  - change the compat allocation prototype to force the user to provide
>    and ac97_codec_device structure pointer
> 
> Since v1:
>  - took into account all Lars comments
> 
> Since v3:
>  - took into account Takashi's comments (ac97bus naming and module_exit)

It looks mostly OK, but some nitpicking:


> diff --git a/include/sound/ac97/compat.h b/include/sound/ac97/compat.h
> new file mode 100644
> index 000000000000..d876464bf7e4
> --- /dev/null
> +++ b/include/sound/ac97/compat.h
> @@ -0,0 +1,21 @@
> +/*
> + *  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.
> + *
> + * This file is for backward compatibility with snd_ac97 structure and its
> + * multiple usages, such as the snd_ac97_bus and snd_ac97_build_ops.
> + *
> + */
> +#ifndef AC97_COMPAT_H
> +#define AC97_COMPAT_H
> +
> +#include <sound/ac97_codec.h>
> +#include <sound/soc.h>

Is this inclusion needed?  The code here doesn't look ASoC-specific at
all.

> --- /dev/null
> +++ b/include/sound/ac97/controller.h
> @@ -0,0 +1,84 @@
> +/*
> + *  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_CONTROLLER_H
> +#define AC97_CONTROLLER_H
> +
> +#include <linux/list.h>
> +
> +#define AC97_BUS_MAX_CODECS 4
> +#define AC97_SLOTS_AVAILABLE_ALL 0xf
> +
> +struct device;

You need the definition struct device in below (it's no pointer), thus
you have to include <linux/device.h> instead.

> +
> +/**
> + * struct ac97_controller - The AC97 controller of the AC-Link
> + * @ops:		the AC97 operations.
> + * @controllers:	linked list of all existing controllers.
> + * @adap:		the shell device ac97-%d, ie. ac97 adapter
> + * @nr:			the number of the shell device
> + * @parent:		the device providing the AC97 controller.
> + * @slots_available:	the mask of accessible/scanable codecs.
> + * @codecs:		the 4 possible AC97 codecs (NULL if none found).
> + * @codecs_pdata:	platform_data for each codec (NULL if no pdata).
> + *
> + * This structure is internal to AC97 bus, and should not be used by the
> + * controllers themselves, excepting for using @dev.
> + */
> +struct ac97_controller {
> +	const struct ac97_controller_ops *ops;

The struct isn't declared beforehand?  GCC will warn.

> +	struct list_head controllers;
> +	struct device adap;
> +	int nr;
> +	struct device *parent;
> +	unsigned short slots_available;

I'd move parent field below, so that 64bit pointer can be aligned
better.

> +static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx,
> +		   unsigned int vendor_id)
> +{
> +	struct ac97_codec_device *codec;
> +	int ret;
> +
> +	codec = kzalloc(sizeof(*codec), GFP_KERNEL);
> +	if (!codec)
> +		return -ENOMEM;
> +	ac97_ctrl->codecs[idx] = codec;
> +	codec->vendor_id = vendor_id;
> +	codec->dev.release = ac97_codec_release;
> +	codec->dev.bus = &ac97_bus_type;
> +	codec->dev.parent = &ac97_ctrl->adap;
> +	codec->num = idx;
> +	codec->ac97_ctrl = ac97_ctrl;
> +
> +	device_initialize(&codec->dev);
> +	dev_set_name(&codec->dev, "%s:%u", dev_name(ac97_ctrl->parent), idx);
> +
> +	ret = device_add(&codec->dev);
> +	if (ret)
> +		goto err_free_codec;
> +
> +	return 0;
> +err_free_codec:
> +	kfree(codec);

This may leave the device name string.  You need to call
put_device() even if device_add() returns an error.


> +	ac97_ctrl->codecs[idx] = NULL;
> +
> +	return ret;
> +}
> +
> +unsigned int snd_ac97_bus_scan_one(struct ac97_controller *adrv,
> +				   unsigned int codec_num)
> +{
> +	unsigned short vid1, vid2;
> +	int ret;
> +
> +	ret = adrv->ops->read(adrv, codec_num, AC97_VENDOR_ID1);
> +	vid1 = (ret & 0xffff);
> +	if (ret < 0)
> +		return 0;
> +
> +	ret = adrv->ops->read(adrv, codec_num, AC97_VENDOR_ID2);
> +	vid2 = (ret & 0xffff);
> +	if (ret < 0)
> +		return 0;
> +
> +	dev_dbg(&adrv->adap, "%s(codec_num=%u): vendor_id=0x%08x\n",
> +		__func__, codec_num, AC97_ID(vid1, vid2));
> +	return AC97_ID(vid1, vid2);
> +}
> +
> +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;
> +		if (!(ac97_ctrl->slots_available & BIT(i)))
> +			continue;
> +		vendor_id = snd_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;
> +	}
> +	return 0;
> +}
> +
> +static int ac97_bus_reset(struct ac97_controller *ac97_ctrl)
> +{
> +	ac97_ctrl->ops->reset(ac97_ctrl);
> +
> +	return 0;
> +}
> +
> +/**
> + * snd_ac97_codec_driver_register - register an AC97 codec driver
> + * @dev: AC97 driver codec to register
> + *
> + * Register an AC97 codec driver to the ac97 bus driver, aka. the AC97 digital
> + * controller.
> + *
> + * Returns 0 on success or error code
> + */
> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
> +{
> +	int ret;
> +
> +	drv->driver.bus = &ac97_bus_type;
> +	ret = driver_register(&drv->driver);
> +
> +	return ret;

This can be simplified.

> +}
> +EXPORT_SYMBOL(snd_ac97_codec_driver_register);

No GPL?  (Ditto for other entries, too)

> +int snd_ac97_reset(struct snd_ac97 *ac97, bool try_warm, unsigned int id,
> +	unsigned int id_mask)
> +{
> +	struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
> +	struct ac97_controller *actrl = adev->ac97_ctrl;
> +
> +	if (try_warm) {
> +		compat_ac97_warm_reset(ac97);
> +		if (snd_ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id)

Can we ignore id_mask here?  I'm not quite sure whether it's fixed...


thanks,

Takashi