В сообщении от 29 августа 2010 20:37:03 автор Marek Vasut написал:
> > + void *control_data;
>
> It's already in codec->control_data, isn't it ?
>
I'm using priv->control_data as temporary storage to initialize codec-
>control_data later, as there's no snd_soc_codec in i2c_probe
> > @@ -145,7 +185,6 @@ static void uda1380_flush_work(struct work_struct
> > *work) uda1380_read_reg_cache(uda1380_codec, reg));
> >
> > clear_bit(bit, &uda1380_cache_dirty);
> >
> > }
> >
> > -
>
> Remove
It improves code formatting.
> > @@ -560,18 +599,40 @@ static int uda1380_set_bias_level(struct
> > snd_soc_codec *codec, enum snd_soc_bias_level level)
> >
> > {
> >
> > int pm = uda1380_read_reg_cache(codec, UDA1380_PM);
> >
> > + struct uda1380_platform_data *pdata = codec->dev->platform_data;
> > +
> > + if (codec->bias_level == level)
> > + return 0;
> >
> > switch (level) {
> > case SND_SOC_BIAS_ON:
> >
> > case SND_SOC_BIAS_PREPARE:
> > + /* ADC, DAC on */
> >
> > uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);
> > break;
> >
> > case SND_SOC_BIAS_STANDBY:
> > - uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
> > - break;
> > - case SND_SOC_BIAS_OFF:
> > + if (codec->bias_level == SND_SOC_BIAS_OFF) {
> > + if (pdata->gpio_power != -EINVAL) {
> > + gpio_set_value(pdata->gpio_power, 1);
> > + uda1380_reset(codec);
> > + }
> > +
> > + uda1380_sync_cache(codec);
> > + }
> >
> > uda1380_write(codec, UDA1380_PM, 0x0);
>
> Maybe some comment won't hurt here about what that 0x0 does.
Well, code explain it pretty well, it puts codec into stand-by mode :)
> > break;
> >
> > + case SND_SOC_BIAS_OFF:
> if (pdata->gpio_power == -EINVAL)
> break;
> ...code...
>
> might help your alignment below.
Ok
> > + if (pdata->gpio_power != -EINVAL) {
> > + int reg;
> > + gpio_set_value(pdata->gpio_power, 0);
> > +
> > + /* Mark mixer regs cache dirty to sync them with
> > + * codec regs on power on.
> > + */
> > + for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM;
> > + reg++)
> > + set_bit(reg - 0x10, &uda1380_cache_dirty);
> > + }
> >
> > }
> > codec->bias_level = level;
> > return 0;
> >
> > @@ -651,16 +712,6 @@ static int uda1380_suspend(struct snd_soc_codec
> > *codec, pm_message_t state)
> >
> > static int uda1380_resume(struct snd_soc_codec *codec)
> > {
> >
> > - int i;
> > - u8 data[2];
> > - u16 *cache = codec->reg_cache;
> > -
> > - /* Sync reg_cache with the hardware */
> > - for (i = 0; i < ARRAY_SIZE(uda1380_reg); i++) {
> > - data[0] = (i << 1) | ((cache[i] >> 8) & 0x0001);
> > - data[1] = cache[i] & 0x00ff;
> > - codec->hw_write(codec->control_data, data, 2);
> > - }
> >
> > uda1380_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> > return 0;
> >
> > }
> >
> > @@ -671,29 +722,32 @@ static int uda1380_probe(struct snd_soc_codec
> > *codec)
> >
> > struct uda1380_priv *uda1380 = snd_soc_codec_get_drvdata(codec);
> > int ret;
> >
> > + uda1380->codec = codec;
> > +
> >
> > codec->hw_write = (hw_write_t)i2c_master_send;
> >
> > + codec->control_data = uda1380->control_data;
> >
> > - if (!pdata || !pdata->gpio_power || !pdata->gpio_reset)
> > + if (!pdata)
> >
> > return -EINVAL;
> >
> > - ret = gpio_request(pdata->gpio_power, "uda1380 power");
> > - if (ret)
> > - return ret;
> > - ret = gpio_request(pdata->gpio_reset, "uda1380 reset");
> > - if (ret)
> > - goto err_gpio;
> > -
> > - gpio_direction_output(pdata->gpio_power, 1);
> > -
> > - /* we may need to have the clock running here - pH5 */
> > - gpio_direction_output(pdata->gpio_reset, 1);
> > - udelay(5);
> > - gpio_set_value(pdata->gpio_reset, 0);
>
> if (gpio_is_valid(...gpio...)) {
>
> }
Will not work for s3c24xx:
static inline int gpio_is_valid(int number)
{
/* only some non-negative numbers are valid */
return ((unsigned)number) < ARCH_NR_GPIOS;
}
> > + if (pdata->gpio_reset != -EINVAL) {
> > + ret = gpio_request(pdata->gpio_reset, "uda1380 reset");
> > + if (ret)
> > + goto err_out;
> > + gpio_direction_output(pdata->gpio_reset, 0);
> Handle return value and dont depend on this setting the GPIO value, use
> gpio_set_value() too please.
This code handles reset-less machines (for example, h1940).
gpio_direction_output is neccessary here to ensure that pin is in output mode.
> > + }
> >
> > - ret = uda1380_reset(codec);
> > - if (ret < 0) {
> > - dev_err(codec->dev, "Failed to issue reset\n");
> > - goto err_reset;
> > + if (pdata->gpio_power != -EINVAL) {
> > + ret = gpio_request(pdata->gpio_power, "uda1380 power");
> > + if (ret)
> > + goto err_gpio;
> > + gpio_direction_output(pdata->gpio_power, 0);
> > + } else {
> > + ret = uda1380_reset(codec);
> > + if (ret) {
> > + dev_err(codec->dev, "Failed to issue reset\n");
> > + goto err_reset;
> > + }
> >
> > }
>
> Ditto.
Handling here machine which lacks power pin.
> > + uda1380->control_data = i2c;
>
> So is this needed ? Can't you access codec->control_data ?
See comment above.
> > kfree(uda1380);
> >
> > +
>
> Remove
It improves formatting.
> > return ret;
> >
> > }
>
> Cheers
thanks,
Regards
Vasily
Attachment:
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
