| Message ID | 20211217115708.882525-8-tanureal@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for CS35L41 in HDA systems | expand |
On Fri, Dec 17, 2021 at 11:57:05AM +0000, Lucas Tanure wrote: > Add support for CS35L41 using a new separated driver > that can be used in all upcoming designs > > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> > --- > + mtl_revid = reg_revid & CS35L41_MTLREVID_MASK; > + > + chipid = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID; > + if (regid != chipid) { > + dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n", regid, chipid); > + ret = -ENODEV; > + goto err; > + } > + > + ret = cs35l41_register_errata_patch(cs35l41->dev, cs35l41->regmap, reg_revid); > + if (ret) > + goto err; > + > + ret = cs35l41_otp_unpack(cs35l41->dev, cs35l41->regmap); > + if (ret) { > + dev_err(cs35l41->dev, "OTP Unpack failed: %d\n", ret); > + goto err; > + } > + > + ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg); > + if (ret) > + goto err; > + kfree(acpi_hw_cfg); > + > + if (cs35l41->reg_seq->probe) { > + ret = regmap_register_patch(cs35l41->regmap, cs35l41->reg_seq->probe, > + cs35l41->reg_seq->num_probe); > + if (ret) { > + dev_err(cs35l41->dev, "Fail to apply probe reg patch: %d\n", ret); > + goto err; > + } > + } Probably shouldn't use regmap_register_patch here, cs35l41_register_errata_patch has already registered a patch to the regmap, and this will then overwrite that patch. Probably better to do this stuff as a multi-write. Thanks, Charles
On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure <tanureal@opensource.cirrus.com> wrote: > > Add support for CS35L41 using a new separated driver > that can be used in all upcoming designs > +config SND_HDA_SCODEC_CS35L41_I2C > + tristate "Build CS35L41 HD-audio side codec support for I2C Bus" > + depends on ACPI > + select SND_HDA_GENERIC > + select SND_SOC_CS35L41_LIB > + select SND_HDA_SCODEC_CS35L41 > + help > + Say Y or M here to include CS35L41 I2C HD-audio side codec support > + in snd-hda-intel driver, such as ALC287. > + > +comment "Set to Y if you want auto-loading the side codec driver" > + depends on SND_HDA=y && SND_HDA_SCODEC_CS35L41_I2C=m > + > +config SND_HDA_SCODEC_CS35L41_SPI > + tristate "Build CS35L41 HD-audio codec support for SPI Bus" > + depends on ACPI > + select SND_HDA_GENERIC > + select SND_SOC_CS35L41_LIB > + select SND_HDA_SCODEC_CS35L41 > + help > + Say Y or M here to include CS35L41 SPI HD-audio side codec support > + in snd-hda-intel driver, such as ALC287. ... > +// cs35l41.c -- CS35l41 ALSA HDA audio driver It's an additional burden in case the file will be renamed. i..o.w. drop the names of the files from the files. > +#include <linux/acpi.h> > +#include <linux/module.h> > +#include <sound/hda_codec.h> > +#include "hda_local.h" > +#include "hda_auto_parser.h" > +#include "hda_jack.h" > +#include "hda_generic.h" > +#include "hda_component.h" > +#include "cs35l41_hda.h" ... > + { CS35L41_PWR_CTRL1, 0x00000001, 2000 }, //GLOBAL_EN = 1 Here and everywhere else, missed space after // ... > +static void cs35l41_hda_playback_hook(struct device *dev, int action) > +{ > + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > + const struct cs35l41_hda_reg_sequence *reg_seq = cs35l41->reg_seq; > + struct regmap *reg = cs35l41->regmap; > + int ret = 0; > + > + switch (action) { > + case HDA_GEN_PCM_ACT_OPEN: > + if (reg_seq->open) > + ret = regmap_multi_reg_write(reg, reg_seq->open, reg_seq->num_open); > + break; > + case HDA_GEN_PCM_ACT_PREPARE: > + if (reg_seq->prepare) > + ret = regmap_multi_reg_write(reg, reg_seq->prepare, reg_seq->num_prepare); > + break; > + case HDA_GEN_PCM_ACT_CLEANUP: > + if (reg_seq->cleanup) > + ret = regmap_multi_reg_write(reg, reg_seq->cleanup, reg_seq->num_cleanup); > + break; > + case HDA_GEN_PCM_ACT_CLOSE: > + if (reg_seq->close) > + ret = regmap_multi_reg_write(reg, reg_seq->close, reg_seq->num_close); > + break; default case? > + } > + > + if (ret) > + dev_warn(cs35l41->dev, "Failed to apply multi reg write: %d\n", ret); > + Redundant blank line. > +} ... > + if (comps && cs35l41->index >= 0 && cs35l41->index < HDA_MAX_COMPONENTS) > + comps = &comps[cs35l41->index]; > + else > + return -EINVAL; Can you check first? In such a case you won't need the 'else' branch at all. ... > + if (!comps->dev) { Why not a positive check and standard pattern as per above? > + comps->dev = dev; > + strscpy(comps->name, dev_name(dev), sizeof(comps->name)); > + comps->playback_hook = cs35l41_hda_playback_hook; > + comps->set_channel_map = cs35l41_hda_channel_map; > + return 0; > + } > + > + return -EBUSY; > +} ... > + switch (hw_cfg->gpio1_func) { > + case CS35l41_VSPK_SWITCH: > + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, > + CS35L41_GPIO1_CTRL_MASK, 1 << CS35L41_GPIO1_CTRL_SHIFT); > + break; > + case CS35l41_SYNC: > + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, > + CS35L41_GPIO1_CTRL_MASK, 2 << CS35L41_GPIO1_CTRL_SHIFT); > + break; default case? Same for all switch-cases in your code. > + } ... > + ret = cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos); > + if (ret) > + return ret; > + > + return 0; return cs35l41_hda_...(...); ... > + property = "cirrus,dev-index"; > + ret = device_property_count_u32(acpi_dev, property); Please, name adev the pointer to ACPI device. Ah, what a mess, you have named acpi_dev the pointer to the struct device. Please, find a better name, like dev, or physdev or so. > + if (ret <= 0) Shouldn't you override the error code for the 0 case? > + goto no_acpi_dsd; ... > + if (ret > ARRAY_SIZE(values)) { > + ret = -EINVAL; > + goto err; > + } Is it really the issue? I would expect the issue when you have less than expected, and not otherwise. ... > + /* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */ Can you elaborate why devm can't be used? > + cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index, Please, do not dereference fwnode pointers. Also, why can't you use the device instead of fwnode? > + GPIOD_OUT_LOW, "cs35l41-reset"); ... > + hw_cfg = kzalloc(sizeof(*hw_cfg), GFP_KERNEL); Why not devm? > + if (!hw_cfg) { > + ret = -ENOMEM; > + goto err; > + } ... > + property = "cirrus,speaker-position"; > + ret = device_property_read_u32_array(acpi_dev, property, values, nval); > + if (ret) > + goto err_free; > + hw_cfg->spk_pos = values[cs35l41->index]; This and further is weird. Why do you need to retrieve all values for just one? Use indexed APIs for that. If there are none, create them. ... > +no_acpi_dsd: > + /* > + * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work. So, you need to add mapping tables and switch to regular APIs, tell me, why it won't work. > + * And devices created by i2c-multi-instantiate don't have their device struct pointing to > + * the correct fwnode, so acpi_dev must be used here > + * And devm functions expect that the device requesting the resource has the correct > + * fwnode You missed grammar periods and what else? Please, update your comments to use proper English grammar. > + */ > + if (strncmp(hid, "CLSA0100", 8) != 0) > + return ERR_PTR(-EINVAL); > + > + /* check I2C address to assign the index */ > + cs35l41->index = id == 0x40 ? 0 : 1; > + cs35l41->reset_gpio = gpiod_get_index(acpi_dev, NULL, 0, GPIOD_OUT_HIGH); > + cs35l41->vspk_always_on = true; > + put_device(acpi_dev); > + > + return NULL; > +} ... > +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, > + struct regmap *regmap) > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); Why? ... > + if (IS_ERR(cs35l41->reset_gpio)) { You should use _optinal variants instead, > + ret = PTR_ERR(cs35l41->reset_gpio); > + cs35l41->reset_gpio = NULL; > + if (ret == -EBUSY) { > + dev_info(cs35l41->dev, "Reset line busy, assuming shared reset\n"); > + } else { > + if (ret != -EPROBE_DEFER) > + dev_err(cs35l41->dev, "Failed to get reset GPIO: %d\n", ret); > + goto err; We have dev_err_probe() for a few releases already. > + } > + } ... > + ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts); > + if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) { > + dev_err(cs35l41->dev, "OTP Boot error\n"); > + ret = -EIO; Why shadowing error code? Why not use dev_err_probe()? > + goto err; > + } ... > +EXPORT_SYMBOL_GPL(cs35l41_hda_probe); Please, use the namespace variant and avoid polluting the global namespace with your symbols. ... > + * cs35l41_hda.h -- CS35L41 ALSA HDA audio driver No file names in the files. ... > +#include <linux/regulator/consumer.h> > +#include <linux/gpio/consumer.h> > +#include <linux/device.h> > +#include <sound/cs35l41.h> Please revisit this. You need to add here only the headers that you are a direct user of (or in some cases their top level ones, like types.h for compiler_attributes.h). ... > +#ifdef CONFIG_ACPI Drop this ugliness. > +static const struct acpi_device_id cs35l41_acpi_hda_match[] = { > + {"CLSA0100", 0 }, > + {"CSC3551", 0 }, I believe these IDs are officially allocated by the Cirrus Logic, right? > + { }, No comma for terminator line here and everywhere else where it's the case. > +}; > +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_hda_match); > +#endif > + > +static struct i2c_driver cs35l41_i2c_driver = { > + .driver = { > + .name = "cs35l41-hda", > + .acpi_match_table = ACPI_PTR(cs35l41_acpi_hda_match), ACPI_PTR() as well. > + }, > + .id_table = cs35l41_hda_i2c_id, > + .probe = cs35l41_hda_i2c_probe, > + .remove = cs35l41_hda_i2c_remove, > +}; > + No need to have a blank line here. > +module_i2c_driver(cs35l41_i2c_driver); I stopped here, so this code needs more work and can't be applied like this. I believe that current Cirrus Logic drivers are written in the same (semi-) bad style and have to be fixed in the future. Put this to your TODO list, please.
Tom,below my review where you may find a lot to be improved in the code, including the place where you should fix it instead of your approach. On Thu, Jan 6, 2022 at 2:29 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure > <tanureal@opensource.cirrus.com> wrote: > > > > Add support for CS35L41 using a new separated driver > > that can be used in all upcoming designs > > > > > +config SND_HDA_SCODEC_CS35L41_I2C > > + tristate "Build CS35L41 HD-audio side codec support for I2C Bus" > > + depends on ACPI > > + select SND_HDA_GENERIC > > + select SND_SOC_CS35L41_LIB > > + select SND_HDA_SCODEC_CS35L41 > > + help > > + Say Y or M here to include CS35L41 I2C HD-audio side codec support > > + in snd-hda-intel driver, such as ALC287. > > + > > +comment "Set to Y if you want auto-loading the side codec driver" > > + depends on SND_HDA=y && SND_HDA_SCODEC_CS35L41_I2C=m > > + > > +config SND_HDA_SCODEC_CS35L41_SPI > > + tristate "Build CS35L41 HD-audio codec support for SPI Bus" > > + depends on ACPI > > + select SND_HDA_GENERIC > > + select SND_SOC_CS35L41_LIB > > + select SND_HDA_SCODEC_CS35L41 > > + help > > + Say Y or M here to include CS35L41 SPI HD-audio side codec support > > + in snd-hda-intel driver, such as ALC287. > > ... > > > +// cs35l41.c -- CS35l41 ALSA HDA audio driver > > It's an additional burden in case the file will be renamed. i..o.w. > drop the names of the files from the files. > > > > +#include <linux/acpi.h> > > +#include <linux/module.h> > > +#include <sound/hda_codec.h> > > +#include "hda_local.h" > > +#include "hda_auto_parser.h" > > +#include "hda_jack.h" > > +#include "hda_generic.h" > > +#include "hda_component.h" > > +#include "cs35l41_hda.h" > > ... > > > + { CS35L41_PWR_CTRL1, 0x00000001, 2000 }, //GLOBAL_EN = 1 > > Here and everywhere else, missed space after // > > ... > > > +static void cs35l41_hda_playback_hook(struct device *dev, int action) > > +{ > > + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > > + const struct cs35l41_hda_reg_sequence *reg_seq = cs35l41->reg_seq; > > + struct regmap *reg = cs35l41->regmap; > > + int ret = 0; > > + > > + switch (action) { > > + case HDA_GEN_PCM_ACT_OPEN: > > + if (reg_seq->open) > > + ret = regmap_multi_reg_write(reg, reg_seq->open, reg_seq->num_open); > > + break; > > + case HDA_GEN_PCM_ACT_PREPARE: > > + if (reg_seq->prepare) > > + ret = regmap_multi_reg_write(reg, reg_seq->prepare, reg_seq->num_prepare); > > + break; > > + case HDA_GEN_PCM_ACT_CLEANUP: > > + if (reg_seq->cleanup) > > + ret = regmap_multi_reg_write(reg, reg_seq->cleanup, reg_seq->num_cleanup); > > + break; > > + case HDA_GEN_PCM_ACT_CLOSE: > > + if (reg_seq->close) > > + ret = regmap_multi_reg_write(reg, reg_seq->close, reg_seq->num_close); > > + break; > > default case? > > > + } > > + > > + if (ret) > > + dev_warn(cs35l41->dev, "Failed to apply multi reg write: %d\n", ret); > > > + > > Redundant blank line. > > > +} > > > ... > > > + if (comps && cs35l41->index >= 0 && cs35l41->index < HDA_MAX_COMPONENTS) > > + comps = &comps[cs35l41->index]; > > + else > > + return -EINVAL; > > Can you check first? In such a case you won't need the 'else' branch at all. > > ... > > > + if (!comps->dev) { > > Why not a positive check and standard pattern as per above? > > > > + comps->dev = dev; > > + strscpy(comps->name, dev_name(dev), sizeof(comps->name)); > > + comps->playback_hook = cs35l41_hda_playback_hook; > > + comps->set_channel_map = cs35l41_hda_channel_map; > > + return 0; > > + } > > + > > + return -EBUSY; > > +} > > ... > > > + switch (hw_cfg->gpio1_func) { > > + case CS35l41_VSPK_SWITCH: > > + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, > > + CS35L41_GPIO1_CTRL_MASK, 1 << CS35L41_GPIO1_CTRL_SHIFT); > > + break; > > + case CS35l41_SYNC: > > + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, > > + CS35L41_GPIO1_CTRL_MASK, 2 << CS35L41_GPIO1_CTRL_SHIFT); > > + break; > > default case? > > Same for all switch-cases in your code. > > > + } > > ... > > > + ret = cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos); > > + if (ret) > > + return ret; > > + > > + return 0; > > return cs35l41_hda_...(...); > > ... > > > + property = "cirrus,dev-index"; > > + ret = device_property_count_u32(acpi_dev, property); > > Please, name adev the pointer to ACPI device. Ah, what a mess, you > have named acpi_dev the pointer to the struct device. Please, find a > better name, like dev, or physdev or so. > > > + if (ret <= 0) > > Shouldn't you override the error code for the 0 case? > > > + goto no_acpi_dsd; > > ... > > > + if (ret > ARRAY_SIZE(values)) { > > + ret = -EINVAL; > > + goto err; > > + } > > Is it really the issue? I would expect the issue when you have less > than expected, and not otherwise. > > ... > > > + /* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */ > > Can you elaborate why devm can't be used? > > > + cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index, > > Please, do not dereference fwnode pointers. > Also, why can't you use the device instead of fwnode? > > > + GPIOD_OUT_LOW, "cs35l41-reset"); > > ... > > > + hw_cfg = kzalloc(sizeof(*hw_cfg), GFP_KERNEL); > > Why not devm? > > > + if (!hw_cfg) { > > + ret = -ENOMEM; > > + goto err; > > + } > > ... > > > + property = "cirrus,speaker-position"; > > + ret = device_property_read_u32_array(acpi_dev, property, values, nval); > > + if (ret) > > + goto err_free; > > + hw_cfg->spk_pos = values[cs35l41->index]; > > This and further is weird. Why do you need to retrieve all values for > just one? Use indexed APIs for that. If there are none, create them. > > ... > > > +no_acpi_dsd: > > + /* > > + * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work. > > So, you need to add mapping tables and switch to regular APIs, tell > me, why it won't work. > > > + * And devices created by i2c-multi-instantiate don't have their device struct pointing to > > + * the correct fwnode, so acpi_dev must be used here > > + * And devm functions expect that the device requesting the resource has the correct > > + * fwnode > > You missed grammar periods and what else? Please, update your comments > to use proper English grammar. > > > + */ > > + if (strncmp(hid, "CLSA0100", 8) != 0) > > + return ERR_PTR(-EINVAL); > > + > > + /* check I2C address to assign the index */ > > + cs35l41->index = id == 0x40 ? 0 : 1; > > + cs35l41->reset_gpio = gpiod_get_index(acpi_dev, NULL, 0, GPIOD_OUT_HIGH); > > + cs35l41->vspk_always_on = true; > > + put_device(acpi_dev); > > + > > + return NULL; > > +} > > ... > > > +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, > > + struct regmap *regmap) > > > + if (IS_ERR(regmap)) > > + return PTR_ERR(regmap); > > Why? > > ... > > > + if (IS_ERR(cs35l41->reset_gpio)) { > > You should use _optinal variants instead, > > > + ret = PTR_ERR(cs35l41->reset_gpio); > > + cs35l41->reset_gpio = NULL; > > + if (ret == -EBUSY) { > > + dev_info(cs35l41->dev, "Reset line busy, assuming shared reset\n"); > > + } else { > > + if (ret != -EPROBE_DEFER) > > + dev_err(cs35l41->dev, "Failed to get reset GPIO: %d\n", ret); > > + goto err; > > We have dev_err_probe() for a few releases already. > > > + } > > + } > > ... > > > + ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts); > > + if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) { > > + dev_err(cs35l41->dev, "OTP Boot error\n"); > > + ret = -EIO; > > Why shadowing error code? > Why not use dev_err_probe()? > > > + goto err; > > + } > > ... > > > +EXPORT_SYMBOL_GPL(cs35l41_hda_probe); > > Please, use the namespace variant and avoid polluting the global > namespace with your symbols. > > ... > > > + * cs35l41_hda.h -- CS35L41 ALSA HDA audio driver > > No file names in the files. > > ... > > > +#include <linux/regulator/consumer.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/device.h> > > +#include <sound/cs35l41.h> > > Please revisit this. You need to add here only the headers that you > are a direct user of (or in some cases their top level ones, like > types.h for compiler_attributes.h). > > ... > > > +#ifdef CONFIG_ACPI > > Drop this ugliness. > > > +static const struct acpi_device_id cs35l41_acpi_hda_match[] = { > > + {"CLSA0100", 0 }, > > + {"CSC3551", 0 }, > > I believe these IDs are officially allocated by the Cirrus Logic, right? > > > + { }, > > No comma for terminator line here and everywhere else where it's the case. > > > +}; > > +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_hda_match); > > +#endif > > + > > +static struct i2c_driver cs35l41_i2c_driver = { > > + .driver = { > > + .name = "cs35l41-hda", > > + .acpi_match_table = ACPI_PTR(cs35l41_acpi_hda_match), > > ACPI_PTR() as well. > > > + }, > > + .id_table = cs35l41_hda_i2c_id, > > + .probe = cs35l41_hda_i2c_probe, > > + .remove = cs35l41_hda_i2c_remove, > > +}; > > > + > > No need to have a blank line here. > > > +module_i2c_driver(cs35l41_i2c_driver); > > I stopped here, so this code needs more work and can't be applied like this. > > I believe that current Cirrus Logic drivers are written in the same > (semi-) bad style and have to be fixed in the future. Put this to your > TODO list, please. > > -- > With Best Regards, > Andy Shevchenko
On 1/6/22 12:29, Andy Shevchenko wrote: > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure > <tanureal@opensource.cirrus.com> wrote: >> >> Add support for CS35L41 using a new separated driver >> that can be used in all upcoming designs > > > >> +config SND_HDA_SCODEC_CS35L41_I2C >> + tristate "Build CS35L41 HD-audio side codec support for I2C Bus" >> + depends on ACPI >> + select SND_HDA_GENERIC >> + select SND_SOC_CS35L41_LIB >> + select SND_HDA_SCODEC_CS35L41 >> + help >> + Say Y or M here to include CS35L41 I2C HD-audio side codec support >> + in snd-hda-intel driver, such as ALC287. >> + >> +comment "Set to Y if you want auto-loading the side codec driver" >> + depends on SND_HDA=y && SND_HDA_SCODEC_CS35L41_I2C=m >> + >> +config SND_HDA_SCODEC_CS35L41_SPI >> + tristate "Build CS35L41 HD-audio codec support for SPI Bus" >> + depends on ACPI >> + select SND_HDA_GENERIC >> + select SND_SOC_CS35L41_LIB >> + select SND_HDA_SCODEC_CS35L41 >> + help >> + Say Y or M here to include CS35L41 SPI HD-audio side codec support >> + in snd-hda-intel driver, such as ALC287. > > ... > >> +// cs35l41.c -- CS35l41 ALSA HDA audio driver > > It's an additional burden in case the file will be renamed. i..o.w. > drop the names of the files from the files. > > >> +#include <linux/acpi.h> >> +#include <linux/module.h> >> +#include <sound/hda_codec.h> >> +#include "hda_local.h" >> +#include "hda_auto_parser.h" >> +#include "hda_jack.h" >> +#include "hda_generic.h" >> +#include "hda_component.h" >> +#include "cs35l41_hda.h" > > ... > >> + { CS35L41_PWR_CTRL1, 0x00000001, 2000 }, //GLOBAL_EN = 1 > > Here and everywhere else, missed space after // > > ... > >> +static void cs35l41_hda_playback_hook(struct device *dev, int action) >> +{ >> + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); >> + const struct cs35l41_hda_reg_sequence *reg_seq = cs35l41->reg_seq; >> + struct regmap *reg = cs35l41->regmap; >> + int ret = 0; >> + >> + switch (action) { >> + case HDA_GEN_PCM_ACT_OPEN: >> + if (reg_seq->open) >> + ret = regmap_multi_reg_write(reg, reg_seq->open, reg_seq->num_open); >> + break; >> + case HDA_GEN_PCM_ACT_PREPARE: >> + if (reg_seq->prepare) >> + ret = regmap_multi_reg_write(reg, reg_seq->prepare, reg_seq->num_prepare); >> + break; >> + case HDA_GEN_PCM_ACT_CLEANUP: >> + if (reg_seq->cleanup) >> + ret = regmap_multi_reg_write(reg, reg_seq->cleanup, reg_seq->num_cleanup); >> + break; >> + case HDA_GEN_PCM_ACT_CLOSE: >> + if (reg_seq->close) >> + ret = regmap_multi_reg_write(reg, reg_seq->close, reg_seq->num_close); >> + break; > > default case? > >> + } >> + >> + if (ret) >> + dev_warn(cs35l41->dev, "Failed to apply multi reg write: %d\n", ret); > >> + > > Redundant blank line. > >> +} > > > ... > >> + if (comps && cs35l41->index >= 0 && cs35l41->index < HDA_MAX_COMPONENTS) >> + comps = &comps[cs35l41->index]; >> + else >> + return -EINVAL; > > Can you check first? In such a case you won't need the 'else' branch at all. > > ... > >> + if (!comps->dev) { > > Why not a positive check and standard pattern as per above? > > >> + comps->dev = dev; >> + strscpy(comps->name, dev_name(dev), sizeof(comps->name)); >> + comps->playback_hook = cs35l41_hda_playback_hook; >> + comps->set_channel_map = cs35l41_hda_channel_map; >> + return 0; >> + } >> + >> + return -EBUSY; >> +} > > ... > >> + switch (hw_cfg->gpio1_func) { >> + case CS35l41_VSPK_SWITCH: >> + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, >> + CS35L41_GPIO1_CTRL_MASK, 1 << CS35L41_GPIO1_CTRL_SHIFT); >> + break; >> + case CS35l41_SYNC: >> + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, >> + CS35L41_GPIO1_CTRL_MASK, 2 << CS35L41_GPIO1_CTRL_SHIFT); >> + break; > > default case? > > Same for all switch-cases in your code. > >> + } > > ... > >> + ret = cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos); >> + if (ret) >> + return ret; >> + >> + return 0; > > return cs35l41_hda_...(...); > > ... > >> + property = "cirrus,dev-index"; >> + ret = device_property_count_u32(acpi_dev, property); > > Please, name adev the pointer to ACPI device. Ah, what a mess, you > have named acpi_dev the pointer to the struct device. Please, find a > better name, like dev, or physdev or so. > >> + if (ret <= 0) > > Shouldn't you override the error code for the 0 case? > >> + goto no_acpi_dsd; > > ... > >> + if (ret > ARRAY_SIZE(values)) { >> + ret = -EINVAL; >> + goto err; >> + } > > Is it really the issue? I would expect the issue when you have less > than expected, and not otherwise. > > ... > >> + /* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */ > > Can you elaborate why devm can't be used? > >> + cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index, > > Please, do not dereference fwnode pointers. > Also, why can't you use the device instead of fwnode? > >> + GPIOD_OUT_LOW, "cs35l41-reset"); > > ... > >> + hw_cfg = kzalloc(sizeof(*hw_cfg), GFP_KERNEL); > > Why not devm? > >> + if (!hw_cfg) { >> + ret = -ENOMEM; >> + goto err; >> + } > > ... > >> + property = "cirrus,speaker-position"; >> + ret = device_property_read_u32_array(acpi_dev, property, values, nval); >> + if (ret) >> + goto err_free; >> + hw_cfg->spk_pos = values[cs35l41->index]; > > This and further is weird. Why do you need to retrieve all values for > just one? Use indexed APIs for that. If there are none, create them. > > ... > >> +no_acpi_dsd: >> + /* >> + * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work. > > So, you need to add mapping tables and switch to regular APIs, tell > me, why it won't work. I will submit a patch series to fix most of the issues you pointed out. The part about how the driver access the ACPI table is going to be improved later if possible. The laptop has already shipped and doesn't have a _DSD node, so the driver needs to read the reset GPIO from a hard coded index inside a node that contains more than one cs35l41. > >> + * And devices created by i2c-multi-instantiate don't have their device struct pointing to >> + * the correct fwnode, so acpi_dev must be used here >> + * And devm functions expect that the device requesting the resource has the correct >> + * fwnode > > You missed grammar periods and what else? Please, update your comments > to use proper English grammar. > >> + */ >> + if (strncmp(hid, "CLSA0100", 8) != 0) >> + return ERR_PTR(-EINVAL); >> + >> + /* check I2C address to assign the index */ >> + cs35l41->index = id == 0x40 ? 0 : 1; >> + cs35l41->reset_gpio = gpiod_get_index(acpi_dev, NULL, 0, GPIOD_OUT_HIGH); >> + cs35l41->vspk_always_on = true; >> + put_device(acpi_dev); >> + >> + return NULL; >> +} > > ... > >> +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, >> + struct regmap *regmap) > >> + if (IS_ERR(regmap)) >> + return PTR_ERR(regmap); > > Why? It is up to the I2C/SPI module to create the regmap and provide to the main module. If that fails the main module can't continue. > > ... > >> + if (IS_ERR(cs35l41->reset_gpio)) { > > You should use _optinal variants instead, > >> + ret = PTR_ERR(cs35l41->reset_gpio); >> + cs35l41->reset_gpio = NULL; >> + if (ret == -EBUSY) { >> + dev_info(cs35l41->dev, "Reset line busy, assuming shared reset\n"); >> + } else { >> + if (ret != -EPROBE_DEFER) >> + dev_err(cs35l41->dev, "Failed to get reset GPIO: %d\n", ret); >> + goto err; > > We have dev_err_probe() for a few releases already. > >> + } >> + } > > ... > >> + ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts); >> + if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) { >> + dev_err(cs35l41->dev, "OTP Boot error\n"); >> + ret = -EIO; > > Why shadowing error code? > Why not use dev_err_probe()? > >> + goto err; >> + } > > ... > >> +EXPORT_SYMBOL_GPL(cs35l41_hda_probe); > > Please, use the namespace variant and avoid polluting the global > namespace with your symbols. > > ... > >> + * cs35l41_hda.h -- CS35L41 ALSA HDA audio driver > > No file names in the files. > > ... > >> +#include <linux/regulator/consumer.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/device.h> >> +#include <sound/cs35l41.h> > > Please revisit this. You need to add here only the headers that you > are a direct user of (or in some cases their top level ones, like > types.h for compiler_attributes.h). > > ... > >> +#ifdef CONFIG_ACPI > > Drop this ugliness. > >> +static const struct acpi_device_id cs35l41_acpi_hda_match[] = { >> + {"CLSA0100", 0 }, >> + {"CSC3551", 0 }, > > I believe these IDs are officially allocated by the Cirrus Logic, right? CLSA010* is not a valid id for Cirrus Logic, but the Bios is already in production, so we must support it. CSC3551 is a valid id for Cirrus Logic. > >> + { }, > > No comma for terminator line here and everywhere else where it's the case. > >> +}; >> +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_hda_match); >> +#endif >> + >> +static struct i2c_driver cs35l41_i2c_driver = { >> + .driver = { >> + .name = "cs35l41-hda", >> + .acpi_match_table = ACPI_PTR(cs35l41_acpi_hda_match), > > ACPI_PTR() as well. > >> + }, >> + .id_table = cs35l41_hda_i2c_id, >> + .probe = cs35l41_hda_i2c_probe, >> + .remove = cs35l41_hda_i2c_remove, >> +}; > >> + > > No need to have a blank line here. > >> +module_i2c_driver(cs35l41_i2c_driver); > > I stopped here, so this code needs more work and can't be applied like this. > > I believe that current Cirrus Logic drivers are written in the same > (semi-) bad style and have to be fixed in the future. Put this to your > TODO list, please. > Thanks for your comments. I will address the remaining ones in due time. Lucas
On Thu, Jan 13, 2022 at 6:53 PM Lucas tanure <tanureal@opensource.cirrus.com> wrote: > On 1/6/22 12:29, Andy Shevchenko wrote: > > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure > > <tanureal@opensource.cirrus.com> wrote: ... > >> + * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work. > > > > So, you need to add mapping tables and switch to regular APIs, tell > > me, why it won't work. > I will submit a patch series to fix most of the issues you pointed out. Thanks! > The part about how the driver access the ACPI table is going to be > improved later if possible. > The laptop has already shipped and doesn't have a _DSD node, so the > driver needs to read the reset GPIO from a hard coded index inside a > node that contains more than one cs35l41. We have a lot of cases like this, hint: `git grep -n -w devm_acpi_dev_add_driver_gpios`. ... > >> +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, > >> + struct regmap *regmap) > > > >> + if (IS_ERR(regmap)) > >> + return PTR_ERR(regmap); > > > > Why? > It is up to the I2C/SPI module to create the regmap and provide to the > main module. If that fails the main module can't continue. So, this is band-aiding the issue, which is in the caller. Caller shouldn't call this function without the regmap being ready-to-use. ... > >> + {"CLSA0100", 0 }, > >> + {"CSC3551", 0 }, > > > > I believe these IDs are officially allocated by the Cirrus Logic, right? > CLSA010* is not a valid id for Cirrus Logic, but the Bios is already in > production, so we must support it. > CSC3551 is a valid id for Cirrus Logic. Thank you for elaborating. Can we be sure that there won't be any abuse of ACPI specification in the future (meaning all IDs will follow the ACPI/PNP registries and be allocated by certain vendor)?
On Thu, Jan 13, 2022 at 8:13 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jan 13, 2022 at 6:53 PM Lucas tanure > <tanureal@opensource.cirrus.com> wrote: > > On 1/6/22 12:29, Andy Shevchenko wrote: > > > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure > > > <tanureal@opensource.cirrus.com> wrote: ... > > >> + * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work. > > > > > > So, you need to add mapping tables and switch to regular APIs, tell > > > me, why it won't work. > > The part about how the driver access the ACPI table is going to be > > improved later if possible. > > The laptop has already shipped and doesn't have a _DSD node, so the > > driver needs to read the reset GPIO from a hard coded index inside a > > node that contains more than one cs35l41. > > We have a lot of cases like this, hint: `git grep -n -w > devm_acpi_dev_add_driver_gpios`. It's all described here: Documentation/firmware-guide/acpi/gpio-properties.rst
Sorry for resurrecting old thread... On Thu, Jan 06, 2022 at 02:29:58PM +0200, Andy Shevchenko wrote: > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure > <tanureal@opensource.cirrus.com> wrote: > > > > Add support for CS35L41 using a new separated driver > > that can be used in all upcoming designs ... > > + /* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */ > > Can you elaborate why devm can't be used? > > > + cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index, > > Please, do not dereference fwnode pointers. > Also, why can't you use the device instead of fwnode? We are doing "acpi_dev_put(adev);" a few lines above, so using adev in the call to fwnode_gpiod_get_index() is technically use-after-free, isn't it? Also, why can't we do cs35l41->reset_gpio = gpiod_get_index(acpi_dev, "reset", cs35l41->index, GPIOD_OUT_LOW); since acpi_dev is device structure corresponding to adev and we are getting the rest of the properties from it? I saw downthread that there was supposed to be a patch addressing several issues raised by Andy, was it ever submitted? Thanks.
On Mon, Oct 31, 2022 at 7:38 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Thu, Jan 06, 2022 at 02:29:58PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure > > <tanureal@opensource.cirrus.com> wrote: ... > > > + cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index, > > > > Please, do not dereference fwnode pointers. > > Also, why can't you use the device instead of fwnode? > > We are doing "acpi_dev_put(adev);" a few lines above, so using adev in > the call to fwnode_gpiod_get_index() is technically use-after-free, > isn't it? Right, but I believe this is in response to the author and not to me. > Also, why can't we do > > cs35l41->reset_gpio = gpiod_get_index(acpi_dev, "reset", > cs35l41->index, > GPIOD_OUT_LOW); > > since acpi_dev is device structure corresponding to adev and we are > getting the rest of the properties from it? I remembered that I have also stumbled over that, but IIRC the point here is that ACPI tables might be broken (since the multi-instance device is a gray area to begin with). So we need clarification from Cirrus to understand what the cases they want to cover with this twisted code to get a GPIO. > I saw downthread that there was supposed to be a patch addressing > several issues raised by Andy, was it ever submitted?
diff --git a/MAINTAINERS b/MAINTAINERS index 5964e047bc04..f12243c9700e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4555,10 +4555,12 @@ F: drivers/media/cec/i2c/ch7322.c CIRRUS LOGIC AUDIO CODEC DRIVERS M: James Schulman <james.schulman@cirrus.com> M: David Rhodes <david.rhodes@cirrus.com> +M: Lucas Tanure <tanureal@opensource.cirrus.com> L: alsa-devel@alsa-project.org (moderated for non-subscribers) L: patches@opensource.cirrus.com S: Maintained F: Documentation/devicetree/bindings/sound/cirrus,cs* +F: sound/pci/hda/cs* F: sound/soc/codecs/cs* CIRRUS LOGIC DSP FIRMWARE DRIVER diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index ab9d2746e804..84cefc006f29 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -91,6 +91,35 @@ config SND_HDA_PATCH_LOADER start up. The "patch" file can be specified via patch module option, such as patch=hda-init. +config SND_HDA_SCODEC_CS35L41 + tristate + +config SND_HDA_SCODEC_CS35L41_I2C + tristate "Build CS35L41 HD-audio side codec support for I2C Bus" + depends on ACPI + select SND_HDA_GENERIC + select SND_SOC_CS35L41_LIB + select SND_HDA_SCODEC_CS35L41 + help + Say Y or M here to include CS35L41 I2C HD-audio side codec support + in snd-hda-intel driver, such as ALC287. + +comment "Set to Y if you want auto-loading the side codec driver" + depends on SND_HDA=y && SND_HDA_SCODEC_CS35L41_I2C=m + +config SND_HDA_SCODEC_CS35L41_SPI + tristate "Build CS35L41 HD-audio codec support for SPI Bus" + depends on ACPI + select SND_HDA_GENERIC + select SND_SOC_CS35L41_LIB + select SND_HDA_SCODEC_CS35L41 + help + Say Y or M here to include CS35L41 SPI HD-audio side codec support + in snd-hda-intel driver, such as ALC287. + +comment "Set to Y if you want auto-loading the side codec driver" + depends on SND_HDA=y && SND_HDA_SCODEC_CS35L41_SPI=m + config SND_HDA_CODEC_REALTEK tristate "Build Realtek HD-audio codec support" select SND_HDA_GENERIC diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index b8fa682ce66a..3e7bc608d45f 100644 --- a/sound/pci/hda/Makefile +++ b/sound/pci/hda/Makefile @@ -27,6 +27,11 @@ snd-hda-codec-conexant-objs := patch_conexant.o snd-hda-codec-via-objs := patch_via.o snd-hda-codec-hdmi-objs := patch_hdmi.o hda_eld.o +# side codecs +snd-hda-scodec-cs35l41-objs := cs35l41_hda.o +snd-hda-scodec-cs35l41-i2c-objs := cs35l41_hda_i2c.o +snd-hda-scodec-cs35l41-spi-objs := cs35l41_hda_spi.o + # common driver obj-$(CONFIG_SND_HDA) := snd-hda-codec.o @@ -45,6 +50,11 @@ obj-$(CONFIG_SND_HDA_CODEC_CONEXANT) += snd-hda-codec-conexant.o obj-$(CONFIG_SND_HDA_CODEC_VIA) += snd-hda-codec-via.o obj-$(CONFIG_SND_HDA_CODEC_HDMI) += snd-hda-codec-hdmi.o +# side codecs +obj-$(CONFIG_SND_HDA_SCODEC_CS35L41) += snd-hda-scodec-cs35l41.o +obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_I2C) += snd-hda-scodec-cs35l41-i2c.o +obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_SPI) += snd-hda-scodec-cs35l41-spi.o + # this must be the last entry after codec drivers; # otherwise the codec patches won't be hooked before the PCI probe # when built in kernel diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c new file mode 100644 index 000000000000..aa5bb6977792 --- /dev/null +++ b/sound/pci/hda/cs35l41_hda.c @@ -0,0 +1,527 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// cs35l41.c -- CS35l41 ALSA HDA audio driver +// +// Copyright 2021 Cirrus Logic, Inc. +// +// Author: Lucas Tanure <tanureal@opensource.cirrus.com> + +#include <linux/acpi.h> +#include <linux/module.h> +#include <sound/hda_codec.h> +#include "hda_local.h" +#include "hda_auto_parser.h" +#include "hda_jack.h" +#include "hda_generic.h" +#include "hda_component.h" +#include "cs35l41_hda.h" + +static const struct reg_sequence cs35l41_hda_config[] = { + { CS35L41_PLL_CLK_CTRL, 0x00000430 }, //3200000Hz, BCLK Input, PLL_REFCLK_EN = 1 + { CS35L41_GLOBAL_CLK_CTRL, 0x00000003 }, //GLOBAL_FS = 48 kHz + { CS35L41_SP_ENABLES, 0x00010000 }, //ASP_RX1_EN = 1 + { CS35L41_SP_RATE_CTRL, 0x00000021 }, //ASP_BCLK_FREQ = 3.072 MHz + { CS35L41_SP_FORMAT, 0x20200200 }, //24 bits, I2S, BCLK Slave, FSYNC Slave + { CS35L41_DAC_PCM1_SRC, 0x00000008 }, //DACPCM1_SRC = ASPRX1 + { CS35L41_AMP_DIG_VOL_CTRL, 0x00000000 }, //AMP_VOL_PCM 0.0 dB + { CS35L41_AMP_GAIN_CTRL, 0x00000084 }, //AMP_GAIN_PCM 4.5 dB + { CS35L41_PWR_CTRL2, 0x00000001 }, //AMP_EN = 1 +}; + +static const struct reg_sequence cs35l41_hda_start_bst[] = { + { CS35L41_PWR_CTRL2, 0x00000021 }, //BST_EN = 10, AMP_EN = 1 + { CS35L41_PWR_CTRL1, 0x00000001, 3000}, // set GLOBAL_EN = 1 +}; + +static const struct reg_sequence cs35l41_hda_stop_bst[] = { + { CS35L41_PWR_CTRL1, 0x00000000, 3000}, // set GLOBAL_EN = 0 +}; + +// only on amps where GPIO1 is used to control ext. VSPK switch +static const struct reg_sequence cs35l41_start_ext_vspk[] = { + { 0x00000040, 0x00000055 }, + { 0x00000040, 0x000000AA }, + { 0x00007438, 0x00585941 }, + { 0x00007414, 0x08C82222 }, + { 0x0000742C, 0x00000009 }, + { 0x00011008, 0x00008001 }, + { 0x0000742C, 0x0000000F }, + { 0x0000742C, 0x00000079 }, + { 0x00007438, 0x00585941 }, + { CS35L41_PWR_CTRL1, 0x00000001, 3000}, // set GLOBAL_EN = 1 + { 0x0000742C, 0x000000F9 }, + { 0x00007438, 0x00580941 }, + { 0x00000040, 0x000000CC }, + { 0x00000040, 0x00000033 }, +}; + +//only on amps where GPIO1 is used to control ext. VSPK switch +static const struct reg_sequence cs35l41_stop_ext_vspk[] = { + { 0x00000040, 0x00000055 }, + { 0x00000040, 0x000000AA }, + { 0x00007438, 0x00585941 }, + { 0x00002014, 0x00000000, 3000}, //set GLOBAL_EN = 0 + { 0x0000742C, 0x00000009 }, + { 0x00007438, 0x00580941 }, + { 0x00011008, 0x00000001 }, + { 0x0000393C, 0x000000C0, 6000}, + { 0x0000393C, 0x00000000 }, + { 0x00007414, 0x00C82222 }, + { 0x0000742C, 0x00000000 }, + { 0x00000040, 0x000000CC }, + { 0x00000040, 0x00000033 }, +}; + +static const struct reg_sequence cs35l41_safe_to_active[] = { + { 0x00000040, 0x00000055 }, + { 0x00000040, 0x000000AA }, + { 0x0000742C, 0x0000000F }, + { 0x0000742C, 0x00000079 }, + { 0x00007438, 0x00585941 }, + { CS35L41_PWR_CTRL1, 0x00000001, 2000 }, //GLOBAL_EN = 1 + { 0x0000742C, 0x000000F9 }, + { 0x00007438, 0x00580941 }, + { 0x00000040, 0x000000CC }, + { 0x00000040, 0x00000033 }, +}; + +static const struct reg_sequence cs35l41_active_to_safe[] = { + { 0x00000040, 0x00000055 }, + { 0x00000040, 0x000000AA }, + { 0x00007438, 0x00585941 }, + { CS35L41_AMP_DIG_VOL_CTRL, 0x0000A678 }, //AMP_VOL_PCM Mute + { CS35L41_PWR_CTRL2, 0x00000000 }, //AMP_EN = 0 + { CS35L41_PWR_CTRL1, 0x00000000 }, + { 0x0000742C, 0x00000009, 2000 }, + { 0x00007438, 0x00580941 }, + { 0x00000040, 0x000000CC }, + { 0x00000040, 0x00000033 }, +}; + +static const struct reg_sequence cs35l41_reset_to_safe[] = { + { 0x00000040, 0x00000055 }, + { 0x00000040, 0x000000AA }, + { 0x00007438, 0x00585941 }, + { 0x00007414, 0x08C82222 }, + { 0x0000742C, 0x00000009 }, + { 0x00000040, 0x000000CC }, + { 0x00000040, 0x00000033 }, +}; + +static const struct cs35l41_hda_reg_sequence cs35l41_hda_reg_seq_no_bst = { + .probe = cs35l41_reset_to_safe, + .num_probe = ARRAY_SIZE(cs35l41_reset_to_safe), + .open = cs35l41_hda_config, + .num_open = ARRAY_SIZE(cs35l41_hda_config), + .prepare = cs35l41_safe_to_active, + .num_prepare = ARRAY_SIZE(cs35l41_safe_to_active), + .cleanup = cs35l41_active_to_safe, + .num_cleanup = ARRAY_SIZE(cs35l41_active_to_safe), +}; + +static const struct cs35l41_hda_reg_sequence cs35l41_hda_reg_seq_ext_bst = { + .open = cs35l41_hda_config, + .num_open = ARRAY_SIZE(cs35l41_hda_config), + .prepare = cs35l41_start_ext_vspk, + .num_prepare = ARRAY_SIZE(cs35l41_start_ext_vspk), + .cleanup = cs35l41_stop_ext_vspk, + .num_cleanup = ARRAY_SIZE(cs35l41_stop_ext_vspk), +}; + +static const struct cs35l41_hda_reg_sequence cs35l41_hda_reg_seq_int_bst = { + .open = cs35l41_hda_config, + .num_open = ARRAY_SIZE(cs35l41_hda_config), + .prepare = cs35l41_hda_start_bst, + .num_prepare = ARRAY_SIZE(cs35l41_hda_start_bst), + .cleanup = cs35l41_hda_stop_bst, + .num_cleanup = ARRAY_SIZE(cs35l41_hda_stop_bst), +}; + +static void cs35l41_hda_playback_hook(struct device *dev, int action) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + const struct cs35l41_hda_reg_sequence *reg_seq = cs35l41->reg_seq; + struct regmap *reg = cs35l41->regmap; + int ret = 0; + + switch (action) { + case HDA_GEN_PCM_ACT_OPEN: + if (reg_seq->open) + ret = regmap_multi_reg_write(reg, reg_seq->open, reg_seq->num_open); + break; + case HDA_GEN_PCM_ACT_PREPARE: + if (reg_seq->prepare) + ret = regmap_multi_reg_write(reg, reg_seq->prepare, reg_seq->num_prepare); + break; + case HDA_GEN_PCM_ACT_CLEANUP: + if (reg_seq->cleanup) + ret = regmap_multi_reg_write(reg, reg_seq->cleanup, reg_seq->num_cleanup); + break; + case HDA_GEN_PCM_ACT_CLOSE: + if (reg_seq->close) + ret = regmap_multi_reg_write(reg, reg_seq->close, reg_seq->num_close); + break; + } + + if (ret) + dev_warn(cs35l41->dev, "Failed to apply multi reg write: %d\n", ret); + +} + +static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsigned int *tx_slot, + unsigned int rx_num, unsigned int *rx_slot) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + + return cs35l41_set_channels(cs35l41->dev, cs35l41->regmap, tx_num, tx_slot, rx_num, + rx_slot); +} + +static int cs35l41_hda_bind(struct device *dev, struct device *master, void *master_data) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + struct hda_component *comps = master_data; + + if (comps && cs35l41->index >= 0 && cs35l41->index < HDA_MAX_COMPONENTS) + comps = &comps[cs35l41->index]; + else + return -EINVAL; + + if (!comps->dev) { + comps->dev = dev; + strscpy(comps->name, dev_name(dev), sizeof(comps->name)); + comps->playback_hook = cs35l41_hda_playback_hook; + comps->set_channel_map = cs35l41_hda_channel_map; + return 0; + } + + return -EBUSY; +} + +static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *master_data) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + struct hda_component *comps = master_data; + + if (comps[cs35l41->index].dev == dev) + memset(&comps[cs35l41->index], 0, sizeof(*comps)); +} + +static const struct component_ops cs35l41_hda_comp_ops = { + .bind = cs35l41_hda_bind, + .unbind = cs35l41_hda_unbind, +}; + +static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41, + const struct cs35l41_hda_hw_config *hw_cfg) +{ + bool internal_boost = false; + int ret; + + if (!hw_cfg) { + cs35l41->reg_seq = &cs35l41_hda_reg_seq_no_bst; + return 0; + } + + if (hw_cfg->bst_ind || hw_cfg->bst_cap || hw_cfg->bst_ipk) + internal_boost = true; + + switch (hw_cfg->gpio1_func) { + case CS35l41_VSPK_SWITCH: + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, + CS35L41_GPIO1_CTRL_MASK, 1 << CS35L41_GPIO1_CTRL_SHIFT); + break; + case CS35l41_SYNC: + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, + CS35L41_GPIO1_CTRL_MASK, 2 << CS35L41_GPIO1_CTRL_SHIFT); + break; + } + + switch (hw_cfg->gpio2_func) { + case CS35L41_INTERRUPT: + regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL, + CS35L41_GPIO2_CTRL_MASK, 2 << CS35L41_GPIO2_CTRL_SHIFT); + break; + } + + if (internal_boost) { + cs35l41->reg_seq = &cs35l41_hda_reg_seq_int_bst; + if (!(hw_cfg->bst_ind && hw_cfg->bst_cap && hw_cfg->bst_ipk)) + return -EINVAL; + ret = cs35l41_boost_config(cs35l41->dev, cs35l41->regmap, + hw_cfg->bst_ind, hw_cfg->bst_cap, hw_cfg->bst_ipk); + if (ret) + return ret; + } else { + cs35l41->reg_seq = &cs35l41_hda_reg_seq_ext_bst; + } + + ret = cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos); + if (ret) + return ret; + + return 0; +} + +static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, + const char *hid, int id) +{ + struct cs35l41_hda_hw_config *hw_cfg; + u32 values[HDA_MAX_COMPONENTS]; + struct acpi_device *adev; + struct device *acpi_dev; + char *property; + size_t nval; + int i, ret; + + adev = acpi_dev_get_first_match_dev(hid, NULL, -1); + if (!adev) { + dev_err(cs35l41->dev, "Failed to find an ACPI device for %s\n", hid); + return ERR_PTR(-ENODEV); + } + + acpi_dev = get_device(acpi_get_first_physical_node(adev)); + acpi_dev_put(adev); + + property = "cirrus,dev-index"; + ret = device_property_count_u32(acpi_dev, property); + if (ret <= 0) + goto no_acpi_dsd; + + if (ret > ARRAY_SIZE(values)) { + ret = -EINVAL; + goto err; + } + nval = ret; + + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret) + goto err; + + cs35l41->index = -1; + for (i = 0; i < nval; i++) { + if (values[i] == id) { + cs35l41->index = i; + break; + } + } + if (cs35l41->index == -1) { + dev_err(cs35l41->dev, "No index found in %s\n", property); + ret = -ENODEV; + goto err; + } + + /* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */ + cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index, + GPIOD_OUT_LOW, "cs35l41-reset"); + + hw_cfg = kzalloc(sizeof(*hw_cfg), GFP_KERNEL); + if (!hw_cfg) { + ret = -ENOMEM; + goto err; + } + + property = "cirrus,speaker-position"; + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret) + goto err_free; + hw_cfg->spk_pos = values[cs35l41->index]; + + property = "cirrus,gpio1-func"; + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret) + goto err_free; + hw_cfg->gpio1_func = values[cs35l41->index]; + + property = "cirrus,gpio2-func"; + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret) + goto err_free; + hw_cfg->gpio2_func = values[cs35l41->index]; + + property = "cirrus,boost-peak-milliamp"; + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret == 0) + hw_cfg->bst_ipk = values[cs35l41->index]; + + property = "cirrus,boost-ind-nanohenry"; + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret == 0) + hw_cfg->bst_ind = values[cs35l41->index]; + + property = "cirrus,boost-cap-microfarad"; + ret = device_property_read_u32_array(acpi_dev, property, values, nval); + if (ret == 0) + hw_cfg->bst_cap = values[cs35l41->index]; + + put_device(acpi_dev); + + return hw_cfg; + +err_free: + kfree(hw_cfg); +err: + put_device(acpi_dev); + dev_err(cs35l41->dev, "Failed property %s: %d\n", property, ret); + + return ERR_PTR(ret); + +no_acpi_dsd: + /* + * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work. + * And devices created by i2c-multi-instantiate don't have their device struct pointing to + * the correct fwnode, so acpi_dev must be used here + * And devm functions expect that the device requesting the resource has the correct + * fwnode + */ + if (strncmp(hid, "CLSA0100", 8) != 0) + return ERR_PTR(-EINVAL); + + /* check I2C address to assign the index */ + cs35l41->index = id == 0x40 ? 0 : 1; + cs35l41->reset_gpio = gpiod_get_index(acpi_dev, NULL, 0, GPIOD_OUT_HIGH); + cs35l41->vspk_always_on = true; + put_device(acpi_dev); + + return NULL; +} + +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, + struct regmap *regmap) +{ + unsigned int int_sts, regid, reg_revid, mtl_revid, chipid, int_status; + struct cs35l41_hda_hw_config *acpi_hw_cfg; + struct cs35l41_hda *cs35l41; + int ret; + + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + cs35l41 = devm_kzalloc(dev, sizeof(*cs35l41), GFP_KERNEL); + if (!cs35l41) + return -ENOMEM; + + cs35l41->dev = dev; + cs35l41->irq = irq; + cs35l41->regmap = regmap; + dev_set_drvdata(dev, cs35l41); + + acpi_hw_cfg = cs35l41_hda_read_acpi(cs35l41, device_name, id); + if (IS_ERR(acpi_hw_cfg)) + return PTR_ERR(acpi_hw_cfg); + + if (IS_ERR(cs35l41->reset_gpio)) { + ret = PTR_ERR(cs35l41->reset_gpio); + cs35l41->reset_gpio = NULL; + if (ret == -EBUSY) { + dev_info(cs35l41->dev, "Reset line busy, assuming shared reset\n"); + } else { + if (ret != -EPROBE_DEFER) + dev_err(cs35l41->dev, "Failed to get reset GPIO: %d\n", ret); + goto err; + } + } + if (cs35l41->reset_gpio) { + usleep_range(2000, 2100); + gpiod_set_value_cansleep(cs35l41->reset_gpio, 1); + } + + usleep_range(2000, 2100); + + ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS4, int_status, + int_status & CS35L41_OTP_BOOT_DONE, 1000, 100000); + if (ret) { + dev_err(cs35l41->dev, "Failed waiting for OTP_BOOT_DONE: %d\n", ret); + goto err; + } + + ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts); + if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) { + dev_err(cs35l41->dev, "OTP Boot error\n"); + ret = -EIO; + goto err; + } + + ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, ®id); + if (ret) { + dev_err(cs35l41->dev, "Get Device ID failed: %d\n", ret); + goto err; + } + + ret = regmap_read(cs35l41->regmap, CS35L41_REVID, ®_revid); + if (ret) { + dev_err(cs35l41->dev, "Get Revision ID failed: %d\n", ret); + goto err; + } + + mtl_revid = reg_revid & CS35L41_MTLREVID_MASK; + + chipid = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID; + if (regid != chipid) { + dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n", regid, chipid); + ret = -ENODEV; + goto err; + } + + ret = cs35l41_register_errata_patch(cs35l41->dev, cs35l41->regmap, reg_revid); + if (ret) + goto err; + + ret = cs35l41_otp_unpack(cs35l41->dev, cs35l41->regmap); + if (ret) { + dev_err(cs35l41->dev, "OTP Unpack failed: %d\n", ret); + goto err; + } + + ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg); + if (ret) + goto err; + kfree(acpi_hw_cfg); + + if (cs35l41->reg_seq->probe) { + ret = regmap_register_patch(cs35l41->regmap, cs35l41->reg_seq->probe, + cs35l41->reg_seq->num_probe); + if (ret) { + dev_err(cs35l41->dev, "Fail to apply probe reg patch: %d\n", ret); + goto err; + } + } + + ret = component_add(cs35l41->dev, &cs35l41_hda_comp_ops); + if (ret) { + dev_err(cs35l41->dev, "Register component failed: %d\n", ret); + goto err; + } + + dev_info(cs35l41->dev, "Cirrus Logic CS35L41 (%x), Revision: %02X\n", regid, reg_revid); + + return 0; + +err: + kfree(acpi_hw_cfg); + if (!cs35l41->vspk_always_on) + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); + gpiod_put(cs35l41->reset_gpio); + + return ret; +} +EXPORT_SYMBOL_GPL(cs35l41_hda_probe); + +int cs35l41_hda_remove(struct device *dev) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + + component_del(cs35l41->dev, &cs35l41_hda_comp_ops); + + if (!cs35l41->vspk_always_on) + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); + gpiod_put(cs35l41->reset_gpio); + + return 0; +} +EXPORT_SYMBOL_GPL(cs35l41_hda_remove); + + +MODULE_DESCRIPTION("CS35L41 HDA Driver"); +MODULE_AUTHOR("Lucas Tanure, Cirrus Logic Inc, <tanureal@opensource.cirrus.com>"); +MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h new file mode 100644 index 000000000000..76c69a8a22f6 --- /dev/null +++ b/sound/pci/hda/cs35l41_hda.h @@ -0,0 +1,69 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * cs35l41_hda.h -- CS35L41 ALSA HDA audio driver + * + * Copyright 2021 Cirrus Logic, Inc. + * + * Author: Lucas Tanure <tanureal@opensource.cirrus.com> + */ + +#ifndef __CS35L41_HDA_H__ +#define __CS35L41_HDA_H__ + +#include <linux/regulator/consumer.h> +#include <linux/gpio/consumer.h> +#include <linux/device.h> +#include <sound/cs35l41.h> + +enum cs35l41_hda_spk_pos { + CS35l41_LEFT, + CS35l41_RIGHT, +}; + +enum cs35l41_hda_gpio_function { + CS35L41_NOT_USED, + CS35l41_VSPK_SWITCH, + CS35L41_INTERRUPT, + CS35l41_SYNC, +}; + +struct cs35l41_hda_reg_sequence { + const struct reg_sequence *probe; + unsigned int num_probe; + const struct reg_sequence *open; + unsigned int num_open; + const struct reg_sequence *prepare; + unsigned int num_prepare; + const struct reg_sequence *cleanup; + unsigned int num_cleanup; + const struct reg_sequence *close; + unsigned int num_close; +}; + +struct cs35l41_hda_hw_config { + unsigned int spk_pos; + unsigned int gpio1_func; + unsigned int gpio2_func; + int bst_ind; + int bst_ipk; + int bst_cap; +}; + +struct cs35l41_hda { + struct device *dev; + struct regmap *regmap; + struct gpio_desc *reset_gpio; + const struct cs35l41_hda_reg_sequence *reg_seq; + + int irq; + int index; + + /* Don't put the AMP in reset of VSPK can not be turned off */ + bool vspk_always_on; +}; + +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, + struct regmap *regmap); +int cs35l41_hda_remove(struct device *dev); + +#endif /*__CS35L41_HDA_H__*/ diff --git a/sound/pci/hda/cs35l41_hda_i2c.c b/sound/pci/hda/cs35l41_hda_i2c.c new file mode 100644 index 000000000000..4a9462fb5c14 --- /dev/null +++ b/sound/pci/hda/cs35l41_hda_i2c.c @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// cs35l41.c -- CS35l41 HDA I2C driver +// +// Copyright 2021 Cirrus Logic, Inc. +// +// Author: Lucas Tanure <tanureal@opensource.cirrus.com> + +#include <linux/module.h> +#include <linux/i2c.h> +#include <linux/acpi.h> + +#include "cs35l41_hda.h" + +static int cs35l41_hda_i2c_probe(struct i2c_client *clt, const struct i2c_device_id *id) +{ + const char *device_name; + + /* Compare against the device name so it works for I2C, normal ACPI + * and for ACPI by i2c-multi-instantiate matching cases + */ + if (strstr(dev_name(&clt->dev), "CLSA0100")) + device_name = "CLSA0100"; + else if (strstr(dev_name(&clt->dev), "CSC3551")) + device_name = "CSC3551"; + else + return -ENODEV; + + return cs35l41_hda_probe(&clt->dev, device_name, clt->addr, clt->irq, + devm_regmap_init_i2c(clt, &cs35l41_regmap_i2c)); +} + +static int cs35l41_hda_i2c_remove(struct i2c_client *clt) +{ + return cs35l41_hda_remove(&clt->dev); +} + +static const struct i2c_device_id cs35l41_hda_i2c_id[] = { + { "cs35l41-hda", 0 }, + {} +}; + +#ifdef CONFIG_ACPI +static const struct acpi_device_id cs35l41_acpi_hda_match[] = { + {"CLSA0100", 0 }, + {"CSC3551", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_hda_match); +#endif + +static struct i2c_driver cs35l41_i2c_driver = { + .driver = { + .name = "cs35l41-hda", + .acpi_match_table = ACPI_PTR(cs35l41_acpi_hda_match), + }, + .id_table = cs35l41_hda_i2c_id, + .probe = cs35l41_hda_i2c_probe, + .remove = cs35l41_hda_i2c_remove, +}; + +module_i2c_driver(cs35l41_i2c_driver); + +MODULE_DESCRIPTION("HDA CS35L41 driver"); +MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>"); +MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/cs35l41_hda_spi.c b/sound/pci/hda/cs35l41_hda_spi.c new file mode 100644 index 000000000000..77426e96c58f --- /dev/null +++ b/sound/pci/hda/cs35l41_hda_spi.c @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// cs35l41.c -- CS35l41 HDA SPI driver +// +// Copyright 2021 Cirrus Logic, Inc. +// +// Author: Lucas Tanure <tanureal@opensource.cirrus.com> + +#include <linux/acpi.h> +#include <linux/module.h> +#include <linux/spi/spi.h> + +#include "cs35l41_hda.h" + +static int cs35l41_hda_spi_probe(struct spi_device *spi) +{ + const char *device_name; + + /* Compare against the device name so it works for SPI, normal ACPI + * and for ACPI by spi-multi-instantiate matching cases + */ + if (strstr(dev_name(&spi->dev), "CSC3551")) + device_name = "CSC3551"; + else + return -ENODEV; + + return cs35l41_hda_probe(&spi->dev, device_name, spi->chip_select, spi->irq, + devm_regmap_init_spi(spi, &cs35l41_regmap_spi)); +} + +static int cs35l41_hda_spi_remove(struct spi_device *spi) +{ + return cs35l41_hda_remove(&spi->dev); +} + +static const struct spi_device_id cs35l41_hda_spi_id[] = { + { "cs35l41-hda", 0 }, + {} +}; + +#ifdef CONFIG_ACPI +static const struct acpi_device_id cs35l41_acpi_hda_match[] = { + { "CSC3551", 0 }, + {}, +}; +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_hda_match); +#endif + +static struct spi_driver cs35l41_spi_driver = { + .driver = { + .name = "cs35l41_hda", + .acpi_match_table = ACPI_PTR(cs35l41_acpi_hda_match), + }, + .id_table = cs35l41_hda_spi_id, + .probe = cs35l41_hda_spi_probe, + .remove = cs35l41_hda_spi_remove, +}; + +module_spi_driver(cs35l41_spi_driver); + +MODULE_DESCRIPTION("HDA CS35L41 driver"); +MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>"); +MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h new file mode 100644 index 000000000000..2e52be6db9c2 --- /dev/null +++ b/sound/pci/hda/hda_component.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * HD audio Component Binding Interface + * + * Copyright (C) 2021 Cirrus Logic, Inc. and + * Cirrus Logic International Semiconductor Ltd. + */ + +#include <linux/component.h> + +#define HDA_MAX_COMPONENTS 4 +#define HDA_MAX_NAME_SIZE 50 + +struct hda_component { + struct device *dev; + char name[HDA_MAX_NAME_SIZE]; + void (*playback_hook)(struct device *dev, int action); + int (*set_channel_map)(struct device *dev, unsigned int rx_num, unsigned int *rx_slot, + unsigned int tx_num, unsigned int *tx_slot); +};
Add support for CS35L41 using a new separated driver that can be used in all upcoming designs Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> --- MAINTAINERS | 2 + sound/pci/hda/Kconfig | 29 ++ sound/pci/hda/Makefile | 10 + sound/pci/hda/cs35l41_hda.c | 527 ++++++++++++++++++++++++++++++++ sound/pci/hda/cs35l41_hda.h | 69 +++++ sound/pci/hda/cs35l41_hda_i2c.c | 66 ++++ sound/pci/hda/cs35l41_hda_spi.c | 63 ++++ sound/pci/hda/hda_component.h | 20 ++ 8 files changed, 786 insertions(+) create mode 100644 sound/pci/hda/cs35l41_hda.c create mode 100644 sound/pci/hda/cs35l41_hda.h create mode 100644 sound/pci/hda/cs35l41_hda_i2c.c create mode 100644 sound/pci/hda/cs35l41_hda_spi.c create mode 100644 sound/pci/hda/hda_component.h