All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Ondrej Zary <linux@rainbow-software.org>
Cc: Clemens Ladisch <clemens@ladisch.de>,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel] [PATCH 3/4] Add Wolfson Microelectronics WM8776 codec ALSA driver
Date: Wed, 18 Apr 2012 07:54:55 +0200	[thread overview]
Message-ID: <s5hr4vloa8g.wl%tiwai@suse.de> (raw)
In-Reply-To: <201204172307.58016.linux@rainbow-software.org>

At Tue, 17 Apr 2012 23:07:48 +0200,
Ondrej Zary wrote:
> 
> On Tuesday 17 April 2012 21:12:33 you wrote:
> > At Tue, 17 Apr 2012 20:15:06 +0200,
> >
> > Ondrej Zary wrote:
> > > On Tuesday 17 April 2012 20:06:19 Takashi Iwai wrote:
> > > > At Tue, 17 Apr 2012 18:04:31 +0100,
> > > >
> > > > Mark Brown wrote:
> > > > > On Tue, Apr 17, 2012 at 06:52:49PM +0200, Takashi Iwai wrote:
> > > > > > Mark Brown wrote:
> > > > > > > This isn't just adding something into a specific driver which
> > > > > > > fails at abstraction, it's adding generic code.  If it were
> > > > > > > adding something to the ice17xx driver then that'd be one thing
> > > > > > > but look at the subject line and location of the file...  this
> > > > > > > stuff should be buried inside the driver if it's too painful to
> > > > > > > make the driver sane.
> > > > > >
> > > > > > The codes in sound/i2c are mostly oly for ice1712/ice1724 drivers
> > > > > > after all...  They could be used by others, but I don't think there
> > > > > > will be any more at this point.
> > > > >
> > > > > If they're specific to that driver we should make them specific to
> > > > > that driver and make sure the pain is confined there.  We really
> > > > > don't want to end up going back to the bad old days of having to do
> > > > > per-CPU/card drivers for CODECs because nobody had thought to
> > > > > abstract this stuff, that just makes everyone miserable.
> > > > >
> > > > > Looking at these commits I'd not expect anyone to figure out that
> > > > > this isn't how we want or expect people to add generic CODEC drivers.
> > > >
> > > > Yeah, these stuff can be better put in pci/ice1712/ directory only
> > > > for ice1724 driver.  In that way, you can avoid unnecessary exported
> > > > symbols, too.
> > >
> > > Xonar (oxygen) has a private version of these drivers too. It could be
> > > converted to use common code instead.
> >
> > Hm, only if the generated controls do match with your case,..
> > It's case by case, especally if there is only one another candidate.
> 
> Control names can be changed (and also disabled) by the driver. In fact, 
> psc724 does exactly this as is needs the WM8776 controls to be named 
> as "front" and WM8766 as "rear".
> I don't see any problem if something needs to be changed for this code to be 
> used by Xonar (or some other driver).

OK, it depends on Clemens, whether the shared code is preferred (and
maintainable).

If sharing your code between ice1724 and xonar is planned, sound/i2c
is the best place to put indeed.  In that case, better to start from
moving the code from xonar to sound/i2c, then reuse it in ice1724,
instead of having double codes in xonar and ice1724.

OTOH, if it isn't worthwhile, better to make all your code local to
ice1724.


thanks,

Takashi

WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de>
To: Ondrej Zary <linux@rainbow-software.org>
Cc: alsa-devel@alsa-project.org,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	Clemens Ladisch <clemens@ladisch.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] Add Wolfson Microelectronics WM8776 codec ALSA driver
Date: Wed, 18 Apr 2012 07:54:55 +0200	[thread overview]
Message-ID: <s5hr4vloa8g.wl%tiwai@suse.de> (raw)
In-Reply-To: <201204172307.58016.linux@rainbow-software.org>

At Tue, 17 Apr 2012 23:07:48 +0200,
Ondrej Zary wrote:
> 
> On Tuesday 17 April 2012 21:12:33 you wrote:
> > At Tue, 17 Apr 2012 20:15:06 +0200,
> >
> > Ondrej Zary wrote:
> > > On Tuesday 17 April 2012 20:06:19 Takashi Iwai wrote:
> > > > At Tue, 17 Apr 2012 18:04:31 +0100,
> > > >
> > > > Mark Brown wrote:
> > > > > On Tue, Apr 17, 2012 at 06:52:49PM +0200, Takashi Iwai wrote:
> > > > > > Mark Brown wrote:
> > > > > > > This isn't just adding something into a specific driver which
> > > > > > > fails at abstraction, it's adding generic code.  If it were
> > > > > > > adding something to the ice17xx driver then that'd be one thing
> > > > > > > but look at the subject line and location of the file...  this
> > > > > > > stuff should be buried inside the driver if it's too painful to
> > > > > > > make the driver sane.
> > > > > >
> > > > > > The codes in sound/i2c are mostly oly for ice1712/ice1724 drivers
> > > > > > after all...  They could be used by others, but I don't think there
> > > > > > will be any more at this point.
> > > > >
> > > > > If they're specific to that driver we should make them specific to
> > > > > that driver and make sure the pain is confined there.  We really
> > > > > don't want to end up going back to the bad old days of having to do
> > > > > per-CPU/card drivers for CODECs because nobody had thought to
> > > > > abstract this stuff, that just makes everyone miserable.
> > > > >
> > > > > Looking at these commits I'd not expect anyone to figure out that
> > > > > this isn't how we want or expect people to add generic CODEC drivers.
> > > >
> > > > Yeah, these stuff can be better put in pci/ice1712/ directory only
> > > > for ice1724 driver.  In that way, you can avoid unnecessary exported
> > > > symbols, too.
> > >
> > > Xonar (oxygen) has a private version of these drivers too. It could be
> > > converted to use common code instead.
> >
> > Hm, only if the generated controls do match with your case,..
> > It's case by case, especally if there is only one another candidate.
> 
> Control names can be changed (and also disabled) by the driver. In fact, 
> psc724 does exactly this as is needs the WM8776 controls to be named 
> as "front" and WM8766 as "rear".
> I don't see any problem if something needs to be changed for this code to be 
> used by Xonar (or some other driver).

OK, it depends on Clemens, whether the shared code is preferred (and
maintainable).

If sharing your code between ice1724 and xonar is planned, sound/i2c
is the best place to put indeed.  In that case, better to start from
moving the code from xonar to sound/i2c, then reuse it in ice1724,
instead of having double codes in xonar and ice1724.

OTOH, if it isn't worthwhile, better to make all your code local to
ice1724.


thanks,

Takashi

  reply	other threads:[~2012-04-18  5:54 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-16 21:18 [RFC PATCH 0/4] snd-ice1712: Add Philips PSC724 Ultimate Edge Ondrej Zary
2012-04-16 21:18 ` [PATCH 1/4] snd-ice1712: add chip_exit callback Ondrej Zary
2012-04-16 21:18 ` [PATCH 2/4] Add Wolfson Microelectronics WM8766 codec ALSA driver Ondrej Zary
2012-04-17 14:59   ` Mark Brown
2012-04-17 16:35     ` Ondrej Zary
2012-04-17 16:54       ` Mark Brown
2012-04-16 21:18 ` [PATCH 3/4] Add Wolfson Microelectronics WM8776 " Ondrej Zary
2012-04-17 15:02   ` Mark Brown
2012-04-17 16:13     ` Ondrej Zary
2012-04-17 16:18       ` Mark Brown
2012-04-17 16:32         ` [alsa-devel] " Takashi Iwai
2012-04-17 16:32           ` Takashi Iwai
2012-04-17 16:50           ` [alsa-devel] " Mark Brown
2012-04-17 16:52             ` Takashi Iwai
2012-04-17 16:52               ` Takashi Iwai
2012-04-17 17:04               ` [alsa-devel] " Mark Brown
2012-04-17 18:06                 ` Takashi Iwai
2012-04-17 18:15                   ` Ondrej Zary
2012-04-17 18:15                     ` Ondrej Zary
2012-04-17 18:22                     ` [alsa-devel] " Mark Brown
2012-04-17 18:22                       ` Mark Brown
2012-04-17 19:14                       ` [alsa-devel] " Takashi Iwai
2012-04-17 19:43                         ` Mark Brown
2012-04-17 19:43                           ` Mark Brown
2012-04-17 20:16                           ` Pavel Hofman
2012-04-17 21:29                             ` Mark Brown
2012-04-18  9:06                               ` Mark Brown
2012-04-18  9:27                                 ` Pavel Hofman
2012-04-18 10:34                                   ` Mark Brown
2012-04-17 19:12                     ` [alsa-devel] " Takashi Iwai
2012-04-17 21:07                       ` Ondrej Zary
2012-04-17 21:07                         ` Ondrej Zary
2012-04-18  5:54                         ` Takashi Iwai [this message]
2012-04-18  5:54                           ` Takashi Iwai
2012-04-18  6:52                           ` [alsa-devel] " Clemens Ladisch
2012-04-18  6:52                             ` Clemens Ladisch
2012-04-16 21:18 ` [PATCH 4/4] snd-ice1712: Add Philips PSC724 Ultimate Edge Ondrej Zary

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5hr4vloa8g.wl%tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=clemens@ladisch.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rainbow-software.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.