Re: [alsa-devel] [PATCH] [MT6660] Mediatek Smart Amplifier Driver

[Richtek Jeff Chang <richtek.jeff.chang@gmail.com>]

Dear Mark:

     Thanks for your reply.

     But I still have some questions. please check the red comment.

Mark Brown 於 2019/9/4 下午7:56 寫道:
> On Wed, Sep 04, 2019 at 03:07:06PM +0800, Richtek Jeff Chang wrote:
>
>>>> +static int32_t mt6660_i2c_update_bits(struct mt6660_chip *chip,
>>>> +	uint32_t addr, uint32_t mask, uint32_t data)
>>>> +{
>>> It would be good to implement a regmap rather than open coding
>>> *everything* - it'd give you things like this without needing to open
>>> code them.  Providing you don't use the cache code it should cope fine
>>> with variable register sizes.
>> Due to our hardware design, it is hard to implement regmap for MT6660.
> You definitely can't use all the functionality due to the variable
> register sizes but using reg_write() and reg_read() should get you most
> of it.


     How can I fill the val_bits for variable register size?

     I try to use all 32 bits val_bits, but our chip some registers are 
overlap...

     Do you have any suggestion for this issue?  Thank you very much!

>
>>>> +static int mt6660_i2c_init_setting(struct mt6660_chip *chip)
>>>> +{
>>>> +	int i, len, ret;
>>>> +	const struct codec_reg_val *init_table;
>>>> +
>>>> +	init_table = e4_reg_inits;
>>>> +	len = ARRAY_SIZE(e4_reg_inits);
>>>> +
>>>> +	for (i = 0; i < len; i++) {
>>>> +		ret = mt6660_i2c_update_bits(chip, init_table[i].addr,
>>>> +				init_table[i].mask, init_table[i].data);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>> Why are we not using the chip defaults here?
>> Because MT6660 needs this initial setting for working well.
> What are these settings?  Are you sure they are generic settings and
> not board specific?

Yes, they are generic setting. It comes from our hardware designers.


>>>> +	if (on_off) {
>>>> +		if (chip->pwr_cnt == 0) {
>>>> +			ret = mt6660_i2c_update_bits(chip,
>>>> +				MT6660_REG_SYSTEM_CTRL, 0x01, 0x00);
>>>> +			val = mt6660_i2c_read(chip, MT6660_REG_IRQ_STATUS1);
>>>> +			dev_info(chip->dev,
>>>> +				"%s reg0x05 = 0x%x\n", __func__, val);
>>>> +		}
>>>> +		chip->pwr_cnt++;
>>> This looks like you're open coding runtime PM stuff?  AFAICT the issue
>>> is that you need to write to this register to do any I/O.  Just
>>> implement this via the standard runtime PM framework, you'll need to do
>>> something about the register I/O in the controls (ideally in the
>>> framework, it'd be a lot easier if you did have a cache) but you could
>>> cut out this bit.
>> In our experience, some Customer platform doesn't support runtime PM.
> Tell your customers to turn it on, it's a standard kernel framework and
> there's really no excuse for open coding it.  If there's some reason why
> runtime PM can't work for them then we should get that fixed but it
> really is *very* widely deployed.


We already implement it in runtime PM.

For all your comment, regmap is a hard thing to modify. I also survey 
others driver about variable register size. But it seems no one like our 
MT6660 chip...

Should I send new patch file to you in this mail loop, or I should send 
new patch via new Email Loop?

Thanks a lot.

Richtek Jeff Chang


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel