On Tue, Nov 13, 2018 at 12:49:16PM -0600, James Schulman wrote:
> Add driver support for Cirrus Logic CS35L36 boosted
> speaker amplifier
>
> Signed-off-by: James Schulman <james.schulman@xxxxxxxxxx>
> ---
Mostly just some small nitpicky comments from me:
> +#include "cs35l36.h"
> +
> +struct reg_default cs35l36_reg[CS35L36_MAX_CACHE_REG] = {
Your allocating an array that is much larger than you need here.
This array will have an entry for all possible register addresses
on the device, whereas only a very small subset of those
addresses have defaults infact most of them arn't even registers.
> + {CS35L36_TESTKEY_CTRL, 0x00000000},
...
> +struct cs35l36_private {
> + struct device *dev;
> + struct cs35l36_platform_data pdata;
> + struct regmap *regmap;
> + struct regulator_bulk_data supplies[2];
> + int num_supplies;
> + int clksrc;
> + int prev_clksrc;
> + int extclk_freq;
> + int extclk_cfg;
> + int fll_igain;
> + int sclk;
> + int chip_version;
> + int rev_id;
> + struct gpio_desc *reset_gpio;
> + struct completion global_pup_done;
> + struct completion global_pdn_done;
These two completions are unused.
> +static int cs35l36_dai_set_sysclk(struct snd_soc_dai *dai,
> + int clk_id, unsigned int freq, int dir)
> +{
> + struct snd_soc_component *component = dai->component;
> + struct cs35l36_private *cs35l36 =
> + snd_soc_component_get_drvdata(component);
> + int fs1_val = 0;
> + int fs2_val = 0;
> +
> + /* Need the SCLK Frequency regardless of sysclk source */
> + cs35l36->sclk = freq;
Yet it is only used locally in this function.
> + if (cs35l36->sclk > 6000000) {
> + fs1_val = 3 * 4 + 4;
> + fs2_val = 8 * 4 + 4;
> + }
> +
> + if (cs35l36->sclk <= 6000000) {
> + fs1_val = 3 * ((24000000 + cs35l36->sclk - 1) / cs35l36->sclk)
> + + 4;
> + fs2_val = 5 * ((24000000 + cs35l36->sclk - 1) / cs35l36->sclk)
> + + 4;
> + }
Nitpicking: Since both paths add 4 you could initialise fs1_val and
fs2_val to 4, then use += would avoid the line wraps for the + 4's
here.
> +static int cs35l36_pcm_startup(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + if (!substream->runtime)
> + return 0;
This null check shouldn't be needed now since:
8053f21675b0 ("ASoC: dapm: Add a dummy snd_pcm_runtime to avoid NULL pointer access")
> +
> + snd_pcm_hw_constraint_list(substream->runtime, 0,
> + SNDRV_PCM_HW_PARAM_RATE, &cs35l36_constraints);
> + return 0;
> +}
...
> +static int cs35l36_component_set_sysclk(struct snd_soc_component *component,
> + int clk_id, int source, unsigned int freq,
> + int dir)
> +{
> + struct cs35l36_private *cs35l36 =
> + snd_soc_component_get_drvdata(component);
> + int ret;
> +
> + cs35l36->extclk_freq = freq;
extclk_freq doesn't appear to be used at all, is there an
expectation it will be in the future?
> + cs35l36->prev_clksrc = cs35l36->clksrc;
Should this be stored in cs35l36_private, given it is only used
locally in this function?
> +
> + switch (clk_id) {
> + case 0:
> + cs35l36->clksrc = CS35L36_PLLSRC_SCLK;
> + break;
> + case 1:
> + cs35l36->clksrc = CS35L36_PLLSRC_LRCLK;
> + break;
> + case 2:
> + cs35l36->clksrc = CS35L36_PLLSRC_PDMCLK;
> + break;
> + case 3:
> + cs35l36->clksrc = CS35L36_PLLSRC_SELF;
> + break;
> + case 4:
> + cs35l36->clksrc = CS35L36_PLLSRC_MCLK;
Seems clksrc is also only used locally, is there some future work
expected on the clocking?
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = cs35l36_get_clk_config(cs35l36, freq);
> +
Would it be simpler just to return a pointer to the
cs35l36_pll_sysclk_config structure?
> + if (cs35l36->rev_id == CS35L36_REV_A0) {
> + if (cs35l36->pdata.dcm_mode) {
Nitpick:
Could probably use an && here.
> +static int cs35l36_pac(struct cs35l36_private *cs35l36)
> +{
> + int ret, count;
> + unsigned int val;
> +
> + if (cs35l36->rev_id == CS35L36_REV_B0) {
Nitpick:
This is already checked before you call the function, so I would
be inclined to drop it but if doing it here it would probably be
better to invert the logic and do:
if (cs35l36->rev_id != CS35L36_REV_B0)
return 0;
> + switch (cs35l36->rev_id) {
> + case CS35L36_REV_A0:
> + ret = regmap_register_patch(cs35l36->regmap,
> + cs35l36_reva0_errata_patch,
> + ARRAY_SIZE(cs35l36_reva0_errata_patch));
> + if (ret < 0) {
> + dev_err(dev, "Failed to apply A0 errata patch %d\n",
> + ret);
> + goto err;
> + }
> + ret = regmap_register_patch(cs35l36->regmap,
> + cs35l36_pac_int_patch,
> + ARRAY_SIZE(cs35l36_pac_int_patch));
> + if (ret < 0) {
> + dev_err(dev, "Failed to apply A0PAC errata patch %d\n",
> + ret);
> + goto err;
> + }
Is this really what you want, having two calls to
remap_register_patch will apply the settings from both here, but
only the second of those calls would be reapplied on cache_sync
later. Now I guess you don't have any cache_syncs so it won't
cause a problem now but feels like it has the potential to cause
confusion down the road.
> + ret = devm_request_threaded_irq(dev, i2c_client->irq, NULL, cs35l36_irq,
> + IRQF_ONESHOT |
> + irq_pol,
Nitpick probably fire this up on the same line as the ONESHOT.
> + "cs35l36", cs35l36);
Thanks,
Charles
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
