[alsa-devel] [PATCH v6 1/2] ASoC: cs35l33: Initial commit of the cs35l33 CODEC driver.

[Handrigan, Paul]

On 6/6/16, 1:31 PM, "Mark Brown" <broonie at kernel.org> wrote:

>On Fri, Jun 03, 2016 at 03:09:05PM -0500, Paul.Handrigan at cirrus.com wrote:
>
>> +		regmap_update_bits(cs35l33->regmap, CS35L33_CLK_CTL,
>> +			CS35L33_MCLKDIV2, CS35L33_MCLKDIV2);
>> +			cs35l33->mclk_int = freq/2;
>
>Coding style, spaces around /.
Thanks!


>
>> +       if (CS35L33_VBSTMON_OVFL & sticky_val2)
>> +               dev_err(codec->dev,
>> +                       "ERROR: VPMON Overflow Interrupt\n");
>
>Cut'n'paste.

Thanks!
>
>> +	/* Make sure that the bits are cleared */
>> +	for (i = 0; i < CS35L33_INT_ATTEMPTS; i++) {
>> +		regmap_read(cs35l33->regmap, CS35L33_INT_STATUS_1,
>> +			&sticky_val1);
>> +		regmap_read(cs35l33->regmap, CS35L33_INT_STATUS_2,
>> +			&sticky_val2);
>> +
>> +		if (!(sticky_val1 & ~mask1) && !(sticky_val2 & ~mask2))
>> +			break;
>> +	}
>
>I still don't understand this.  If these interrupts are clear on read
>(which seems to be the case since we don't ack anything?) then these
>repeated reads are obviously just going to ignore interrupts.  I really
>hope that the masking also applies to the clear on read behaviour but
>that's a separate thing.
>
>I'm guessing the hardware is just broken here but it's not clear to me
>that just letting the interrupt fire a couple of extra times isn't the
>best solution and we need a clear explanation of what this is trying to
>accomplish and why so that the code is maintainable.

It is just to ensure that the unmasked bits are cleared. We can remove this
code all together.


>
>> +       ret = request_threaded_irq(i2c_client->irq, NULL,
>>cs35l33_irq_thread,
>> +                       IRQF_ONESHOT | IRQF_TRIGGER_LOW, "cs35l33",
>>cs35l33);
>> +       if (ret != 0)
>> +               dev_err(&i2c_client->dev, "Failed to request IRQ:
>>%d\n", ret);
>
>But otherwise ignore the error?

This really should be a warning.  The device can be used without an
interrupt pin.
This should be devm_ as well.

>