On Fri, Jul 02, 2021 at 03:51:26PM -0500, David Rhodes wrote:
This looks pretty good - a few fairly small issues and some queries
below:
> index 000000000000..7ee60ff0de26
> --- /dev/null
> +++ b/include/sound/cs35l41.h
> @@ -0,0 +1,79 @@
> +struct cs35l41_private {
> + struct snd_soc_codec *codec;
> + struct cs35l41_platform_data pdata;
Why does anything looking in include/sound need the driver private data?
It's not entirely clear why this is in include/sound at all.
> + struct regulator_bulk_data supplies[2];
> + int num_supplies;
Why might the number of supplies vary?
> +++ b/sound/soc/codecs/cs35l41-i2c.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * cs35l41-i2c.c -- CS35l41 I2C driver
Please make the entire comment a C++ one so things look more
intentional.
> +#include <linux/slab.h>
> +#include <linux/version.h>
You probably don't need at least version.h here.
> +static void cs35l41_spi_otp_setup(struct cs35l41_private *cs35l41,
> + bool is_pre_setup, unsigned int *freq)
> +{
> + struct spi_device *spi;
> + u32 orig_spi_freq;
> +
> + spi = to_spi_device(cs35l41->dev);
> +
> + if (!spi)
> + return;
Why might this reasonably happen, shouldn't we complain loudly?
> +#ifdef CONFIG_OF
> +static const struct of_device_id cs35l41_of_match[] = {
> + {.compatible = "cirrus,cs35l40"},
> + {.compatible = "cirrus,cs35l41"},
> + {},
Coding style - spaces around the { } like everywhere else.
> + /* Check to see if unmasked bits are active */
> + if (!(status[0] & ~masks[0]) && !(status[1] & ~masks[1]) &&
> + !(status[2] & ~masks[2]) && !(status[3] & ~masks[3]))
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
This seems to handle any asserted interrupt, is it clear on read?
> + case SND_SOC_DAPM_POST_PMD:
> + regmap_read(cs35l41->regmap, CS35L41_PWR_CTRL1, &val);
> + if (val & CS35L41_GLOBAL_EN_MASK) {
> + regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL1,
> + CS35L41_GLOBAL_EN_MASK, 0);
I can't see any references to GLOBAL_EN outside this function, why might
it not be set?
> +static int cs35l41_set_pdata(struct cs35l41_private *cs35l41)
> +{
> + struct cs35l41_classh_cfg *classh = &cs35l41->pdata.classh_config;
> + int ret;
> +
> + /* Set Platform Data */
> + /* Required */
> + if (cs35l41->pdata.bst_ipk &&
> + cs35l41->pdata.bst_ind && cs35l41->pdata.bst_cap) {
The indentation here is weird, normally the second line would be
indented with the (.
> + if (cs35l41->pdata.dout_hiz <= CS35L41_ASP_DOUT_HIZ_MASK &&
> + cs35l41->pdata.dout_hiz >= 0)
> + regmap_update_bits(cs35l41->regmap, CS35L41_SP_HIZ_CTRL,
> + CS35L41_ASP_DOUT_HIZ_MASK,
> + cs35l41->pdata.dout_hiz);
No error or warning on out of range?
> +static int cs35l41_component_probe(struct snd_soc_component *component)
> +{
> + struct cs35l41_private *cs35l41 =
> + snd_soc_component_get_drvdata(component);
> +
> + component->regmap = cs35l41->regmap;
> +
> + return cs35l41_set_pdata(cs35l41);
> +}
Why wait until here to apply the configuration, why not just do it
immediately on the bus level probe? I didn't notice anything that
needed the card, just the regmap.
> +static const struct reg_sequence cs35l41_reva0_errata_patch[] = {
> + {0x00000040, 0x00005555},
> + {0x00000040, 0x0000AAAA},
Spaces around the { }.
> +
> + do {
> + if (timeout == 0) {
> + dev_err(cs35l41->dev,
> + "Timeout waiting for OTP_BOOT_DONE\n");
> + ret = -EBUSY;
> + goto err;
> + }
> + usleep_range(1000, 1100);
> + regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS4, &int_status);
> + timeout--;
> + } while (!(int_status & CS35L41_OTP_BOOT_DONE));
It would be clearer to assign timeout at the top of the loop rather than
when declaring the variable so people don't have to search back and make
sure it was set.
> + ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL,
> + cs35l41_irq, IRQF_ONESHOT | IRQF_SHARED | irq_pol,
> + "cs35l41", cs35l41);
> +
> + /* CS35L41 needs INT for PDN_DONE */
> + if (ret != 0) {
> + dev_err(cs35l41->dev, "Failed to request IRQ: %d\n", ret);
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + /* Set interrupt masks for critical errors */
> + regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1,
> + CS35L41_INT1_MASK_DEFAULT);
Shouldn't this be configured prior to requesting interrupts or might
there be a race?
> + switch (reg_revid) {
> + case CS35L41_REVID_A0:
> + ret = regmap_register_patch(cs35l41->regmap,
> + cs35l41_reva0_errata_patch,
> + ARRAY_SIZE(cs35l41_reva0_errata_patch));
> + if (ret < 0) {
> + dev_err(cs35l41->dev,
> + "Failed to apply A0 errata patch %d\n", ret);
> + goto err;
> + }
> + break;
> + case CS35L41_REVID_B0:
> + ret = regmap_register_patch(cs35l41->regmap,
> + cs35l41_revb0_errata_patch,
> + ARRAY_SIZE(cs35l41_revb0_errata_patch));
I'd also expect patching to be done prior to requesting interrupts so
the device is in a known good state first. Perhaps it doesn't make any
difference but the patches are undocumented so...
Attachment:
signature.asc
Description: PGP signature
