Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751575AbdGYVmP (ORCPT ); Tue, 25 Jul 2017 17:42:15 -0400 Received: from mx2.suse.de ([195.135.220.15]:49191 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750919AbdGYVmN (ORCPT ); Tue, 25 Jul 2017 17:42:13 -0400 Date: Tue, 25 Jul 2017 23:42:10 +0200 Message-ID: From: Takashi Iwai To: "Robert Jarzmik" Cc: "Dmitry Torokhov" , "Haojian Zhuang" , "Liam Girdwood" , "Mark Brown" , "Lee Jones" , "Lars-Peter Clausen" , "Charles Keepax" , "Jaroslav Kysela" , "Daniel Mack" , , , , , Subject: Re: [PATCH v4 02/12] ALSA: ac97: add an ac97 bus In-Reply-To: <20170724204928.29505-3-robert.jarzmik@free.fr> References: <20170724204928.29505-1-robert.jarzmik@free.fr> <20170724204928.29505-3-robert.jarzmik@free.fr> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7313 Lines: 251 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 > --- > 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 > + * > + * 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 > +#include 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 > + * > + * 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 > + > +#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 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