| Message ID | 20220909135334.98220-1-povik+lin@cutebit.org |
|---|---|
| Headers | show |
| Series | Support for CS42L83 on Apple machines | expand |
On 09/09/2022 15:53, Martin Povišer wrote: > The CS42L83 part is a headphone jack codec found in recent Apple > machines. It is a publicly undocumented part but as far as can be told > it is identical to CS42L42 except for two points: > (...) > + regmap = devm_regmap_init_i2c(i2c_client, &cs42l83_regmap); > + if (IS_ERR(regmap)) { > + ret = PTR_ERR(regmap); > + dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret); > + return ret; Use dev_err_probe() > + } > + > + cs42l83->devid = CS42L83_CHIP_ID; > + cs42l83->dev = dev; > + cs42l83->regmap = regmap; > + cs42l83->irq = i2c_client->irq; > + > + ret = cs42l42_common_probe(cs42l83, &cs42l42_soc_component, &cs42l42_dai); > + if (ret) > + return ret; > + > + ret = cs42l42_init(cs42l83); > + if (ret) > + cs42l42_common_remove(cs42l83); > + > + return ret; > +} > + > +static int cs42l83_i2c_remove(struct i2c_client *i2c_client) > +{ > + struct cs42l42_private *cs42l83 = dev_get_drvdata(&i2c_client->dev); > + > + cs42l42_common_remove(cs42l83); > + > + return 0; > +} > + > +static int __maybe_unused cs42l83_i2c_resume(struct device *dev) > +{ > + int ret; > + > + ret = cs42l42_resume(dev); > + if (ret) > + return ret; > + > + cs42l42_resume_restore(dev); > + > + return 0; > +} > + > +static const struct dev_pm_ops cs42l83_i2c_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l83_i2c_resume) > +}; > + > +static const struct of_device_id __maybe_unused cs42l83_of_match[] = { > + { .compatible = "cirrus,cs42l83", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, cs42l83_of_match); > + > +static struct i2c_driver cs42l83_i2c_driver = { > + .driver = { > + .name = "cs42l83", > + .pm = &cs42l83_i2c_pm_ops, > + .of_match_table = of_match_ptr(cs42l83_of_match), This should complain with compile testing. Usually it comes with __maybe_unused/ Best regards, Krzysztof
> On 9. 9. 2022, at 16:47, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 09/09/2022 15:53, Martin Povišer wrote: >> The CS42L83 part is a headphone jack codec found in recent Apple >> machines. It is a publicly undocumented part but as far as can be told >> it is identical to CS42L42 except for two points: >> (...) >> +static const struct dev_pm_ops cs42l83_i2c_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l83_i2c_resume) >> +}; >> + >> +static const struct of_device_id __maybe_unused cs42l83_of_match[] = { >> + { .compatible = "cirrus,cs42l83", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, cs42l83_of_match); >> + >> +static struct i2c_driver cs42l83_i2c_driver = { >> + .driver = { >> + .name = "cs42l83", >> + .pm = &cs42l83_i2c_pm_ops, >> + .of_match_table = of_match_ptr(cs42l83_of_match), > > This should complain with compile testing. Usually it comes with > __maybe_unused/ Which symbol? cs42l83_of_match has maybe_unused. Best, Martin > > > Best regards, > Krzysztof
On 09/09/2022 17:10, Martin Povišer wrote: >>> +}; >>> +MODULE_DEVICE_TABLE(of, cs42l83_of_match); >>> + >>> +static struct i2c_driver cs42l83_i2c_driver = { >>> + .driver = { >>> + .name = "cs42l83", >>> + .pm = &cs42l83_i2c_pm_ops, >>> + .of_match_table = of_match_ptr(cs42l83_of_match), >> >> This should complain with compile testing. Usually it comes with >> __maybe_unused/ > > Which symbol? cs42l83_of_match has maybe_unused. > Ah, I missed it completely. It's fine. Best regards, Krzysztof
On 09/09/2022 14:53, Martin Povišer wrote: > +static int cs42l42_i2c_probe(struct i2c_client *i2c_client) > +{ > + struct device *dev = &i2c_client->dev; > + struct cs42l42_private *cs42l42; > + struct regmap *regmap; > + int ret; > + > + cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL); > + if (!cs42l42) > + return -ENOMEM; > + > + regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap); > + if (IS_ERR(regmap)) { > + ret = PTR_ERR(regmap); > + dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret); > + return ret; > + } > + > + cs42l42->dev = dev; > + cs42l42->regmap = regmap; > + cs42l42->irq = i2c_client->irq; > + > + ret = cs42l42_common_probe(cs42l42, &cs42l42_soc_component, &cs42l42_dai); > + if (ret) > + return ret; > + > + ret = cs42l42_init(cs42l42); > + if (ret) > + cs42l42_common_remove(cs42l42); This introduces a bug that regulator_bulk_disable() is called twice if there is an error. cs42l42_init() was supposed to clean up if it returns an error, which it nearly does, but my original patch is missing the call to free_irq() in the error paths of cs42l42_init().
> On 9. 9. 2022, at 17:40, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > On 09/09/2022 14:53, Martin Povišer wrote: >> +static int cs42l42_i2c_probe(struct i2c_client *i2c_client) >> +{ >> + struct device *dev = &i2c_client->dev; >> + struct cs42l42_private *cs42l42; >> + struct regmap *regmap; >> + int ret; >> + >> + cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL); >> + if (!cs42l42) >> + return -ENOMEM; >> + >> + regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap); >> + if (IS_ERR(regmap)) { >> + ret = PTR_ERR(regmap); >> + dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret); >> + return ret; >> + } >> + >> + cs42l42->dev = dev; >> + cs42l42->regmap = regmap; >> + cs42l42->irq = i2c_client->irq; >> + >> + ret = cs42l42_common_probe(cs42l42, &cs42l42_soc_component, &cs42l42_dai); >> + if (ret) >> + return ret; >> + >> + ret = cs42l42_init(cs42l42); >> + if (ret) >> + cs42l42_common_remove(cs42l42); > > This introduces a bug that regulator_bulk_disable() is called > twice if there is an error. > > cs42l42_init() was supposed to clean up if it returns an error, which > it nearly does, but my original patch is missing the call to free_irq() > in the error paths of cs42l42_init(). Ah! I didn’t inspect it closely enough then, I only ran into the missing free_irq. Martin
On 09/09/2022 14:53, Martin Povišer wrote: > +static int cs42l42_init(struct cs42l42_private *cs42l42) > +{ > + unsigned int reg; > + int devid, ret; > + > /* initialize codec */ > devid = cirrus_read_device_id(cs42l42->regmap, CS42L42_DEVID_AB); > if (devid < 0) { > @@ -2320,15 +2331,15 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client) > /* Setup headset detection */ > cs42l42_setup_hs_type_detect(cs42l42); > > + /* > + * Set init_done before unmasking interrupts so any triggered > + * immediately will be handled. > + */ > + cs42l42->init_done = true; > + > /* Mask/Unmask Interrupts */ > cs42l42_set_interrupt_masks(cs42l42); > > - /* Register codec for machine driver */ > - ret = devm_snd_soc_register_component(cs42l42->dev, > - &soc_component_dev_cs42l42, &cs42l42_dai, 1); > - if (ret < 0) > - goto err_shutdown; > - > return 0; > > err_shutdown: > @@ -2337,34 +2348,69 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client) > regmap_write(cs42l42->regmap, CS42L42_PWR_CTL1, 0xff); > > err_disable: > - if (i2c_client->irq) > - free_irq(i2c_client->irq, cs42l42); > - These 3 lines should not be removed. cs42l42_init() must free the irq in the error paths.
On 09/09/2022 16:44, Martin Povišer wrote: > >> On 9. 9. 2022, at 17:40, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >> >> On 09/09/2022 14:53, Martin Povišer wrote: >>> +static int cs42l42_i2c_probe(struct i2c_client *i2c_client) >>> +{ >>> + struct device *dev = &i2c_client->dev; >>> + struct cs42l42_private *cs42l42; >>> + struct regmap *regmap; >>> + int ret; >>> + >>> + cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL); >>> + if (!cs42l42) >>> + return -ENOMEM; >>> + >>> + regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap); >>> + if (IS_ERR(regmap)) { >>> + ret = PTR_ERR(regmap); >>> + dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + cs42l42->dev = dev; >>> + cs42l42->regmap = regmap; >>> + cs42l42->irq = i2c_client->irq; >>> + >>> + ret = cs42l42_common_probe(cs42l42, &cs42l42_soc_component, &cs42l42_dai); >>> + if (ret) >>> + return ret; >>> + >>> + ret = cs42l42_init(cs42l42); >>> + if (ret) >>> + cs42l42_common_remove(cs42l42); >> >> This introduces a bug that regulator_bulk_disable() is called >> twice if there is an error. >> >> cs42l42_init() was supposed to clean up if it returns an error, which >> it nearly does, but my original patch is missing the call to free_irq() >> in the error paths of cs42l42_init(). > > Ah! I didn’t inspect it closely enough then, I only ran into the missing > free_irq. > Yes, that's a bug. I just put a comment on the patch that introduced it. When I split probe() into two, I accidentally missed out those two lines to call free_irq(). > Martin >
On 09/09/2022 14:53, Martin Povišer wrote: > Hi all, > > there's a CS42L83 headphone jack codec found in Apple computers (in the > recent 'Apple Silicon' ones as well as in earlier models, one example > [1]). The part isn't publicly documented, but it appears almost > identical to CS42L42, for which we have a driver in kernel. This series > adapts the CS42L42 driver to the new part, and makes one change in > anticipation of a machine driver for the Apple computers. > > Patch 1 adds new compatible to the cs42l42 schema. > > Patches 2 to 7 are taken from Richard's recent series [2] adding > soundwire support to cs42l42. They are useful refactorings to build on > in later patches, and also this way our work doesn't diverge. I made > one fix: I added a call of common_remove at the end of i2c_probe should > the cs42l42_init call fail (both before and after the split to > cs42l42-i2c.c). Also s/Soundwire/SoundWire/ in the changelogs. > Mark: I've no objection to you taking my patches from this chain instead of waiting for me to re-send them myself. I can rebase my remaining patches onto this chain. But I do have comments on patches #4 and #7. I've been very busy and don't have time right now to deal with re-sending my original patch chain.
On Fri, Sep 09, 2022 at 05:16:48PM +0100, Richard Fitzgerald wrote: > Mark: I've no objection to you taking my patches from this chain instead > of waiting for me to re-send them myself. I can rebase my remaining > patches onto this chain. But I do have comments on patches #4 and #7. > I've been very busy and don't have time right now to deal with > re-sending my original patch chain. OK, great - I guess applying stuff will make less work for you later.
> On 9. 9. 2022, at 15:53, Martin Povišer <povik+lin@cutebit.org> wrote: > > To prepare for adding SoundWire the probe must be split into three > parts: > > 1) The bus-specific probe > 2) Common bus-agnostic probe steps > 3) Initialization of the peripheral registers > > Step (3) must be separate because on SoundWire devices the probe must > enable power supplies and release reset so that the peripheral can be > enumerated by the bus, but it isn't possible to access registers until > enumeration has completed. > > The call to devm_snd_soc_register_component() must be done at stage (2) > so that it can EPROBE_DEFER if necessary. In SoundWire systems stage (3) > is not a probe event so a deferral at this stage would not result in > re-probing dependencies. > > A new init_done flag indicates that the chip has been identified and > initialized. This is used to prevent cs42l42_remove(), cs42l42_suspend(), > cs42l42_restore() and cs42l42_irq_thread() from attempting register > accesses if the chip was not successfully initialized. Although this > cannot happen on I2C, because the entire probe would fail, it is > possible on SoundWire if probe succeeds but the cs42l42 is never > enumerated. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Signed-off-by: Martin Povišer <povik+lin@cutebit.org> Preparing the next iteration of the series, I noticed that I reset the authorship of this patch in the course of git manipulations. The author of this patch is of course Richard, apologies for that. It will be fixed for v2. Martin