2011-07-21 12:30:07     AD73311 driver development catastrophe

Document created by Aaronwu Employee on Aug 27, 2013
Version 1Show Document
  • View in full screen mode

2011-07-21 12:30:07     AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102599   

 

Along with an update of a board specific branch to a more recent kernel (from 2.6.34.7), I had to inspect the recent history because massive changes appeared relative to the  "known" state. In detail, a little catastrophe occurred as the whole control additions disappeared suddenly as per commit 1b7676 (rewritten history SHA1, "[!no_src_qa!] update to linux-2.6.37"). For whatever reason, the state with controls has not been reverted but instead hand-added in portions (volume controls as per dfdc93 and later the SEEN bit control as per 75d64f). Apparently without any awareness about pre-catastrophe state, the default values of the controls were changed and - more impactive - the amixer control names have been changed without special notice in the commit message. Of course this would break customer applications where controls are already used.

 

The focus of my contributions to the reworked driver was october 2010. At that time there was some cooperation with Bob Liu. Before this, I sponsored this development through engaging an external programmer. At that time I presented a complete patch which was even more comprehensive than the current state, in that the controls are stored in dynamic objects. The current state is still defining the controls as static objects. A question about this is separately discussed in next posting of mine in this thread.

 

My patches (  blackfin.uclinux.org/gf/forummessage/94568) were not processed for trunk as far as I can conclude this. But Bob shouldn't be blamed for that because he was heavily engaged with a China trade fair at that time.

 

What is even more disappointing about the recent driver history is the fact that from commit 4efb8d on (separation of AD74111 in own files) the GPIO_SE and GPIO_SELECT were no longer configurable via Kconfig. They were simply set to a fixed 4. There is no text about this in the commit message. Im general, it is regrettable that some commits do not separate pure renaming and/or reordering from algorithmical changes.

 

A final note: One reason why I have to countercheck history is the fact that the RESET signal has a special behaviour in order to prevent multiple audible clicks upon each amixer setting. I'm preparing once again a new patch against trunk, in the hope that this behaviour will be adopted for trunk  - which would simplify future updates.

 

The above text serves as an inventory of status, and I think that ADI people will agree that it is regrettable that SELECT & RESET assignments are no longer configurable. Regarding the amixer control names: I'm aware of the fact that the new names are common in terms of usage with other ADI codec products. Question: is it possible to define aliases, so that for a single control two or more names can be used?

QuoteReplyEditDelete

 

 

2011-07-21 12:56:56     Re: AD73311 driver development catastrophe

Mike Frysinger (UNITED STATES)

Message: 102600   

 

yes, the ALSA state is a mess, but it is slowly improving.  hardcoding of any settings, or even via Kconfig, is wrong.  it needs to be pushed to the platform resources.  i believe there is a tracker item open on the issue.

QuoteReplyEditDelete

 

 

2011-07-21 14:04:07     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102603   

 

Mike, I agree, pin assignments should move to board file. I have checked the stamp board file for this, and indeed, a struct for ad73311 is introduced here (commit ebfc30). Of course, without these lines the codec wouldn't work. Without seeing it with awareness I have copied it over into by own boardfile.

 

The commit above refers to #6435. There is no tracker with this id.

 

The struct contains no parameters yet beyond name and id. So, in fact old shoes were already thrown away before the new ones were ready for use...

QuoteReplyEditDelete

 

 

2011-07-21 14:20:18     Re: AD73311 driver development catastrophe

Mike Frysinger (UNITED STATES)

Message: 102604   

 

sure there is ...

  blackfin.uclinux.org/gf/tracker/6435

 

all ADI codecs go through the device-drivers team now while the machine drivers (sound/soc/blackfin/*) go through the Blackfin Linux team

QuoteReplyEditDelete

 

 

2011-07-21 14:50:23     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102605   

 

OK, I searched in the bugs section...

 

Now I also found a ad73311-specific #6506. I'll check it.

QuoteReplyEditDelete

 

 

2011-07-21 15:55:26     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102606   

 

As announced on top of this thread, I'm recalling the topic of dynamic allocation of control parameters, now in conjunction with board file settings. Afterwards a reopen of  "on work" id #6506 should be considered, since the requirement of moving gpio assignments to board file is not fullfilled.

 

Recent trunk state says (snippet):

 

 

 

static int ad73311_igs_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)

{

    ucontrol->value.integer.value[0] = ad73311_ctrls.igs;

    return 0;

}

 

 

My own branch since oct. 15, 2010 says:

 

static int ad73311_igain_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *uvalue)

{

    struct ad73311_priv *ad73311 = snd_kcontrol_chip(kcontrol);

 

    uvalue->value.integer.value[0] = ad73311->igain;

    return 0;

}

 

 

The mechanism behind these dynamically allocated controls is here:

 

struct ad73311_priv {

    struct snd_soc_codec codec;   ///< the codec struct is incorporated in the controls struct

    unsigned int ogain:3;

    unsigned int igain:3;

    unsigned int rfseen:1;

    unsigned int srate:2;

    unsigned int data:1;

    unsigned int reset_required:1;

};

 

 

In this scenario, the default values are set in the soc_probe function.

 

Now I'm considering the following: when the boardfile struct contains a chip_select member (just as an analogy to spi_board_info), I'd also be able to define the physical struct in the boardfile along with the controls, since this would allow definition of control defaults per board file. I'm not sure if such struct members operate in SOC/alsa as well as we know it from other devices as e.g. SPI. Any confirmation regarding this idea (and how-to) would be appreciated.

 

QuoteReplyEditDelete

 

 

2011-07-21 16:32:43     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102607   

 

Yet one further technical contribution:

 

In trunk, I find this:

 

static const struct snd_kcontrol_new ad73311_snd_controls[] = {

    SOC_SINGLE_EXT("ADC Capture Volume", CTRL_REG_D, 0, 7, 0,

        ad73311_igs_get, ad73311_igs_put),

    SOC_SINGLE_EXT("DAC Playback Volume", CTRL_REG_D, 4, 7, 0,

        ad73311_ogs_get, ad73311_ogs_put),

    SOC_SINGLE_EXT("Decimation/Interpolation Rate", CTRL_REG_B, 0, 3, 0,

        ad73311_dirate_get, ad73311_dirate_put),

    SOC_SINGLE_EXT("Single-Ended Enable Switch", CTRL_REG_F, 5, 1, 0,

        ad73311_seen_get, ad73311_seen_put),

};

 

I don't understand why hardware register names are referenced in a file that is hiearchically one level higher, this would also make uncessessary problems with the combination of two chips in one driver. Instead it was designed so, that the hardware driver refers to the codec level controls (via pointer). I verified it yet while setting all register macros to a fixed 0: the controls still operate. Well, when the register reference is not necessary here, I was able to successfully restore an ENUM for the sample rate register (see bold displayed line):

 

static const char *ad73311_srate[] = {"8kHz", "16kHz", "32kHz", "64kHz"};

 

static const struct soc_enum ad73311_srate_enum =

    SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(ad73311_srate), ad73311_srate);

 

static const struct snd_kcontrol_new ad73311_snd_controls[] = {

    SOC_SINGLE_EXT("IGAIN", 0, 0, 7, 0,

        ad73311_igs_get, ad73311_igs_put),

    SOC_SINGLE_EXT("OGAIN", 0, 4, 7, 0,

        ad73311_ogs_get, ad73311_ogs_put),

    SOC_ENUM_EXT("SRATE", ad73311_srate_enum,

        ad73311_dirate_get, ad73311_dirate_put),

    // was originally SOC_SINGLE_BOOL_EXT - but SINGLE_EXT now also with off&on texts

    SOC_SINGLE_EXT("SEEN", 0, 5, 1, 0,

        ad73311_seen_get, ad73311_seen_put),

};

 

 

For the sake of completeness, I have to emphasize that I'm not that familiar with these SOC constructs to finally judge about it.

 

++++++

 

So far two technical contributions - including previous posting - that I consider as preparations for (re)opening an "on work" item. Currently I'm working with the pre-change-to-separate-chips-file state of this driver (reason: not to make a too big leap from an earlier version).

QuoteReplyEditDelete

 

 

2011-07-21 18:09:52     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102608   

 

To round the discussion, some further remarks follow:

 

- the defaults are not changed (my claim on top is false).

 

- amixer name 'aliases' as I asked about are very simpe: just extra entries in ad73311_snd_controls. I persist that the 'short' names (e.g. IGAIN) refer to register values as I intended, while long names (if desired by the ADI teamers) could interpret as 0...100% values for e.g. volume - like in other codec drivers (using some magnitude conversion helpers).

 

Please take notice of   blackfin.uclinux.org/gf/forummessage/94567, where it is clarified that my specific RESET approach allows designed-in devices without RESET assigned to a GPIO, can also use the controls (only after power-up).

 

Attached patch that contains the specific RESET behaviour might help in any discussion. This patch is related to 2.6.38.1 (commit 30e359).

 

ad73311.patch

QuoteReplyEditDelete

 

 

2011-08-01 05:47:07     Re: AD73311 driver development catastrophe

Scott Jiang (CHINA)

Message: 102746   

 

1. the name is named according ALSA rules, it is display to user so the name should be recognized by user not by developer.

 

2. board specific info should be put into board file.

 

3. why do you like bit field structure? just save space?

 

what is most important is that we don't have your board. We can't test, we don't know why it needs this step, so it's hard for us to maintain a customer patch. Hope we can get it better.

QuoteReplyEditDelete

 

 

2011-08-15 17:02:20     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 102934   

 

Scott, thanks for replying - and apologize for my (longer) delay.

 

1. I understand. Well, as I said: nothing hinders me to assign other names in coexistence with the ALSA standard, where - by the way - strictly said percentages should be subject to user input when doing a setting - thus needing magnitude conversion for the "long name" amixer settings.

 

2. Indeed. Since this is a generic requirement, I'd appreciate it when the basis for this is prepared by the team. The dynamic allocation of the device's setting struct according to my state of this development (as recalled in this thread) can be utilized. I can an might also contribute, in order to get a sort of final result that makes me happy as well as other users of the device. FYI: in the next few days, I'll be working on getting a newer kernel prepared for my customer, including work on the ad73311 subsystem.

 

3. Why bit fields: no, it is not a space argument. I could also ask why it was replaced by normal structs (OK, I know, a commit was accidentally removing the work until January). To the point: the bit fields reflect the bit structure of AD73311 registers, and inherently, no magnitude range checking is needed. Moreover, this shows the programmer immediately the range of a parameter without looking at the chip driver code.

 

Regarding the board: If you refer to my specific RESET behaviour, I can understand this. You'd be able to hear the difference, but I'd suggest that an eval board would behave comparable as my board. Upon desire, I can send you a board as well as grant access to my own git server in order to allow board operations with the proper private kernel branch.

 

-Rob

QuoteReplyEditDelete

 

 

2011-08-16 04:23:50     Re: AD73311 driver development catastrophe

Scott Jiang (CHINA)

Message: 102942   

 

Hi Rob, I suggest you commit your patch to the mainline kernel. The mailing list is alsa-devel@alsa-project.org, the maintainer can give your more advice on it.

QuoteReplyEditDelete

 

 

2011-08-23 16:54:02     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 103095   

 

I compared a bit with the mainline kernel. Well, it can be seen that some of the changes that are carried out in the blackfin git are transferred to mainline kernel, but only the version without  the controls stuff. So I'd suggest that Mike's opinion is that the controls be transferred over to mainline when this stuff is 'mature'.

 

Well, neither is it my job, nor is it my aim to contribute in changes towards mainline. I'm only contributing within the blackfin community.

 

Meanwhile I see that you have started with getting the driver ready as a platform driver. However, the recent changes in git aren't very concise. E.g. some of the commits as of date 17-8-2011 would better be a single commit. As of now, changes are not put in direct relationship whereas committing a functional change that affects multiple source files in a single commit makes things quite better readable, and better usable when I browse a history filtered to changes in ad73311. Then I don't see the changes in stamp.c (and I won't add this in the log filter, since stamp.c is affected in a lot more commits than ad73311. Do you capture what I mean?.

 

As a thumb of rule: please don't commit your work to upstream too quickly. Let it 'settle' somewhat. Then you have a chance to --amend commits, or rebase a bit, before pushing. I'd be grateful for better reasoned commits. Another example for good practice: put algorithmic changes and typo/code reorder changes in separate commits.

QuoteReplyEditDelete

 

 

2011-08-24 05:00:19     Re: AD73311 driver development catastrophe

Sonic Zhang (CHINA)

Message: 103107   

 

The change to stamp.c is in the other commit, because patches to blackfin arch tree should be in separate patch. Please update you git tree.

 

What do you mean "upstream"? Is it the GIT repository on blackfin.uclinux.org? If so, we can't, because our developing model is to review the patches after they are commited to the ADI central GIT repository. So, patches to fix bugs in former patches are expected behavior. When we push a new driver to kernel mainline, we will merge all these patches and send out one.

QuoteReplyEditDelete

 

 

2011-08-24 17:15:38     Re: AD73311 driver development catastrophe

Rob Maris (GERMANY)

Message: 103115   

 

Sonic: with upstream I indeed meant the repository that is maintained here (project linux-kernel). It is where I fetch changes from (for me, upstream is NOT the kernel.org repo).

 

Regarding the separate commits: I understand it, when Mike has a commit that e.g. removes the -codec tail in source text of several drivers that are around here. The case I was complaining about is still not plausible for me, since commit 928faf1 specifies changes in stamp.c where solely changes related to ad73311-driver are carried out in stamp.c, so I don't see a reason to commit it separately from the ascendent commit that changes ad73311.c itself. I'd think that your developing model does not restrict that mentioned reviewing when the two patches were supplied as one patch.

 

BTW: Sure, this is not worth doing a public discussion about, so I'd suggest to treat this subtopic as closed (but of course, if you aren't bored - any response will be noticed, however).

Attachments

Outcomes