| Message ID | 20240221003630.2535938-1-jogletre@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add support for CS40L50 | expand |
On Wed, Feb 21, 2024 at 12:36:26AM +0000, James Ogletree wrote: > A write sequencer is a sequence of register addresses > and values executed by some Cirrus DSPs following > power state transitions. > > Add support for Cirrus drivers to update or add to a > write sequencer present in firmware. > > Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > --- Think this one looks good to me: Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
On Wed, 21 Feb 2024, James Ogletree wrote: > Introduce support for Cirrus Logic Device CS40L50: a > haptic driver with waveform memory, integrated DSP, > and closed-loop algorithms. > > The MFD component registers and initializes the device. > > Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > --- > MAINTAINERS | 2 + > drivers/mfd/Kconfig | 30 ++ > drivers/mfd/Makefile | 4 + > drivers/mfd/cs40l50-core.c | 531 ++++++++++++++++++++++++++++++++++++ > drivers/mfd/cs40l50-i2c.c | 69 +++++ > drivers/mfd/cs40l50-spi.c | 69 +++++ > include/linux/mfd/cs40l50.h | 142 ++++++++++ > 7 files changed, 847 insertions(+) > create mode 100644 drivers/mfd/cs40l50-core.c > create mode 100644 drivers/mfd/cs40l50-i2c.c > create mode 100644 drivers/mfd/cs40l50-spi.c > create mode 100644 include/linux/mfd/cs40l50.h Mostly fine, just a few nits. Assumingly this needs to go in via one tree (usually MFD). I can't do so until the other maintainers have provided Acks. [...] > +static struct regmap_irq_chip cs40l50_irq_chip = { > + .name = "CS40L50 IRQ Controller", > + > + .status_base = CS40L50_IRQ1_INT_1, > + .mask_base = CS40L50_IRQ1_MASK_1, > + .ack_base = CS40L50_IRQ1_INT_1, > + .num_regs = 22, > + > + .irqs = cs40l50_reg_irqs, > + .num_irqs = ARRAY_SIZE(cs40l50_reg_irqs), > + > + .runtime_pm = true, > +}; > + > +int cs40l50_dsp_write(struct device *dev, struct regmap *regmap, u32 val) > +{ > + int err, i; > + u32 ack; > + > + /* Device NAKs if exiting hibernation, so optionally retry here */ > + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) { > + err = regmap_write(regmap, CS40L50_DSP_QUEUE, val); > + if (!err) > + break; > + > + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100); > + } > + > + /* If we never wrote, don't bother checking for ACK */ "If the write never took place, no need to check for the ACK" > + if (i == CS40L50_DSP_TIMEOUT_COUNT) { > + dev_err(dev, "Timed out writing %#X to DSP: %d\n", val, err); > + return err; > + } > + > + err = regmap_read_poll_timeout(regmap, CS40L50_DSP_QUEUE, ack, !ack, > + CS40L50_DSP_POLL_US, > + CS40L50_DSP_POLL_US * CS40L50_DSP_TIMEOUT_COUNT); > + if (err) > + dev_err(dev, "DSP did not ack %#X: %d\n", val, err); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(cs40l50_dsp_write); > + > +static const struct cs_dsp_region cs40l50_dsp_regions[] = { > + { .type = WMFW_HALO_PM_PACKED, .base = CS40L50_PMEM_0 }, > + { .type = WMFW_HALO_XM_PACKED, .base = CS40L50_XMEM_PACKED_0 }, > + { .type = WMFW_HALO_YM_PACKED, .base = CS40L50_YMEM_PACKED_0 }, > + { .type = WMFW_ADSP2_XM, .base = CS40L50_XMEM_UNPACKED24_0 }, > + { .type = WMFW_ADSP2_YM, .base = CS40L50_YMEM_UNPACKED24_0 }, > +}; > + > +static void cs40l50_dsp_remove(void *data) > +{ > + cs_dsp_remove((struct cs_dsp *)data); Is the cast required? Where is this function? > +} > + > +static const struct cs_dsp_client_ops cs40l50_client_ops; What's this for? Where is it used? In general, I'm not a fan of empty struct definitions like this. > +static int cs40l50_dsp_init(struct cs40l50 *cs40l50) > +{ > + int err; > + > + cs40l50->dsp.num = 1; > + cs40l50->dsp.type = WMFW_HALO; > + cs40l50->dsp.dev = cs40l50->dev; > + cs40l50->dsp.regmap = cs40l50->regmap; > + cs40l50->dsp.base = CS40L50_CORE_BASE; > + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID; > + cs40l50->dsp.mem = cs40l50_dsp_regions; > + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions); > + cs40l50->dsp.no_core_startstop = true; > + cs40l50->dsp.client_ops = &cs40l50_client_ops; > + > + err = cs_dsp_halo_init(&cs40l50->dsp); > + if (err) > + return err; > + > + return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove, > + &cs40l50->dsp); > +} [...] > +++ b/drivers/mfd/cs40l50-i2c.c > @@ -0,0 +1,69 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * CS40L50 Advanced Haptic Driver with waveform memory, > + * integrated DSP, and closed-loop algorithms > + * > + * Copyright 2024 Cirrus Logic, Inc. > + * > + * Author: James Ogletree <james.ogletree@cirrus.com> > + */ > + > +#include <linux/i2c.h> > +#include <linux/mfd/cs40l50.h> > + > +static int cs40l50_i2c_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct cs40l50 *cs40l50; > + > + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL); > + if (!cs40l50) > + return -ENOMEM; > + > + i2c_set_clientdata(client, cs40l50); > + > + cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap); > + if (IS_ERR(cs40l50->regmap)) > + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap), > + "Failed to initialize register map\n"); > + > + cs40l50->dev = dev; > + cs40l50->irq = client->irq; > + > + return cs40l50_probe(cs40l50); > +} > + > +static void cs40l50_i2c_remove(struct i2c_client *client) > +{ > + struct cs40l50 *cs40l50 = i2c_get_clientdata(client); > + > + cs40l50_remove(cs40l50); > +} > + > +static const struct i2c_device_id cs40l50_id_i2c[] = { > + {"cs40l50"}, Spaces either side of the "s > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c); > + > +static const struct of_device_id cs40l50_of_match[] = { > + { .compatible = "cirrus,cs40l50" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, cs40l50_of_match); > + > +static struct i2c_driver cs40l50_i2c_driver = { > + .driver = { > + .name = "cs40l50", > + .of_match_table = cs40l50_of_match, > + .pm = pm_ptr(&cs40l50_pm_ops), > + }, > + .id_table = cs40l50_id_i2c, > + .probe = cs40l50_i2c_probe, > + .remove = cs40l50_i2c_remove, > +}; > +module_i2c_driver(cs40l50_i2c_driver); > + > +MODULE_DESCRIPTION("CS40L50 I2C Driver"); > +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c > new file mode 100644 > index 000000000000..9e18bb74eae0 > --- /dev/null > +++ b/drivers/mfd/cs40l50-spi.c > @@ -0,0 +1,69 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * CS40L50 Advanced Haptic Driver with waveform memory, > + * integrated DSP, and closed-loop algorithms > + * > + * Copyright 2024 Cirrus Logic, Inc. > + * > + * Author: James Ogletree <james.ogletree@cirrus.com> > + */ > + > +#include <linux/mfd/cs40l50.h> > +#include <linux/spi/spi.h> > + > +static int cs40l50_spi_probe(struct spi_device *spi) > +{ > + struct cs40l50 *cs40l50; > + struct device *dev = &spi->dev; > + > + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL); > + if (!cs40l50) > + return -ENOMEM; > + > + spi_set_drvdata(spi, cs40l50); > + > + cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap); > + if (IS_ERR(cs40l50->regmap)) > + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap), > + "Failed to initialize register map\n"); > + > + cs40l50->dev = dev; > + cs40l50->irq = spi->irq; > + > + return cs40l50_probe(cs40l50); > +} > + > +static void cs40l50_spi_remove(struct spi_device *spi) > +{ > + struct cs40l50 *cs40l50 = spi_get_drvdata(spi); > + > + cs40l50_remove(cs40l50); > +} > + > +static const struct spi_device_id cs40l50_id_spi[] = { > + {"cs40l50"}, As above. > + {} > +}; > +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi); > + > +static const struct of_device_id cs40l50_of_match[] = { > + { .compatible = "cirrus,cs40l50" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, cs40l50_of_match); > + > +static struct spi_driver cs40l50_spi_driver = { > + .driver = { > + .name = "cs40l50", > + .of_match_table = cs40l50_of_match, > + .pm = pm_ptr(&cs40l50_pm_ops), > + }, > + .id_table = cs40l50_id_spi, > + .probe = cs40l50_spi_probe, > + .remove = cs40l50_spi_remove, > +}; > +module_spi_driver(cs40l50_spi_driver); > + > +MODULE_DESCRIPTION("CS40L50 SPI Driver"); > +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h > new file mode 100644 > index 000000000000..d855784a88a9 > --- /dev/null > +++ b/include/linux/mfd/cs40l50.h > @@ -0,0 +1,142 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * CS40L50 Advanced Haptic Driver with waveform memory, > + * integrated DSP, and closed-loop algorithms > + * > + * Copyright 2024 Cirrus Logic, Inc. > + * > + * Author: James Ogletree <james.ogletree@cirrus.com> > + */ > + > +#ifndef __CS40L50_H__ > +#define __CS40L50_H__ MFD_ And at the bottom of this file. [...] > +int cs40l50_dsp_write(struct device *dev, struct regmap *regmap, u32 val); > +int cs40l50_probe(struct cs40l50 *cs40l50); > +int cs40l50_remove(struct cs40l50 *cs40l50); > + > +extern const struct regmap_config cs40l50_regmap; > +extern const struct dev_pm_ops cs40l50_pm_ops; > + > +#endif /* __CS40L50_H__ */ Here.
Hi Lee, Thanks for the review. > On Mar 1, 2024, at 3:17 AM, Lee Jones <lee@kernel.org> wrote: > > On Wed, 21 Feb 2024, James Ogletree wrote: > >> Introduce support for Cirrus Logic Device CS40L50: a >> haptic driver with waveform memory, integrated DSP, >> and closed-loop algorithms. >> >> The MFD component registers and initializes the device. >> >> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> >> --- >> MAINTAINERS | 2 + >> drivers/mfd/Kconfig | 30 ++ >> drivers/mfd/Makefile | 4 + >> drivers/mfd/cs40l50-core.c | 531 ++++++++++++++++++++++++++++++++++++ >> drivers/mfd/cs40l50-i2c.c | 69 +++++ >> drivers/mfd/cs40l50-spi.c | 69 +++++ >> include/linux/mfd/cs40l50.h | 142 ++++++++++ >> 7 files changed, 847 insertions(+) >> create mode 100644 drivers/mfd/cs40l50-core.c >> create mode 100644 drivers/mfd/cs40l50-i2c.c >> create mode 100644 drivers/mfd/cs40l50-spi.c >> create mode 100644 include/linux/mfd/cs40l50.h > > Mostly fine, just a few nits. > > Assumingly this needs to go in via one tree (usually MFD). > > I can't do so until the other maintainers have provided Acks. > Yes, understood. >> +static const struct cs_dsp_region cs40l50_dsp_regions[] = { >> + { .type = WMFW_HALO_PM_PACKED, .base = CS40L50_PMEM_0 }, >> + { .type = WMFW_HALO_XM_PACKED, .base = CS40L50_XMEM_PACKED_0 }, >> + { .type = WMFW_HALO_YM_PACKED, .base = CS40L50_YMEM_PACKED_0 }, >> + { .type = WMFW_ADSP2_XM, .base = CS40L50_XMEM_UNPACKED24_0 }, >> + { .type = WMFW_ADSP2_YM, .base = CS40L50_YMEM_UNPACKED24_0 }, >> +}; >> + >> +static void cs40l50_dsp_remove(void *data) >> +{ >> + cs_dsp_remove((struct cs_dsp *)data); > > Is the cast required? > > Where is this function? Seems that the cast is redundant, so I will remove. The function cs_dsp_remove() is exported from linux/firmware/cirrus/cs_dsp.h. > >> +} >> + >> +static const struct cs_dsp_client_ops cs40l50_client_ops; > > What's this for? Where is it used? > > In general, I'm not a fan of empty struct definitions like this. From the same cs_dsp module as mentioned above, it is a structure containing callbacks that gives the client driver an opportunity to respond to certain events during the DSP's lifecycle. It just so happens that this driver does not need to do anything. However, no struct definition will result in a null pointer dereference by cs_dsp when it checks for the callbacks. Best, James
On Fri, 01 Mar 2024, James Ogletree wrote: > Hi Lee, > > Thanks for the review. > > > On Mar 1, 2024, at 3:17 AM, Lee Jones <lee@kernel.org> wrote: > > > > On Wed, 21 Feb 2024, James Ogletree wrote: > > > >> Introduce support for Cirrus Logic Device CS40L50: a > >> haptic driver with waveform memory, integrated DSP, > >> and closed-loop algorithms. > >> > >> The MFD component registers and initializes the device. > >> > >> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > >> --- > >> MAINTAINERS | 2 + > >> drivers/mfd/Kconfig | 30 ++ > >> drivers/mfd/Makefile | 4 + > >> drivers/mfd/cs40l50-core.c | 531 ++++++++++++++++++++++++++++++++++++ > >> drivers/mfd/cs40l50-i2c.c | 69 +++++ > >> drivers/mfd/cs40l50-spi.c | 69 +++++ > >> include/linux/mfd/cs40l50.h | 142 ++++++++++ > >> 7 files changed, 847 insertions(+) > >> create mode 100644 drivers/mfd/cs40l50-core.c > >> create mode 100644 drivers/mfd/cs40l50-i2c.c > >> create mode 100644 drivers/mfd/cs40l50-spi.c > >> create mode 100644 include/linux/mfd/cs40l50.h > > > > Mostly fine, just a few nits. > > > > Assumingly this needs to go in via one tree (usually MFD). > > > > I can't do so until the other maintainers have provided Acks. > > > > Yes, understood. > > >> +static const struct cs_dsp_region cs40l50_dsp_regions[] = { > >> + { .type = WMFW_HALO_PM_PACKED, .base = CS40L50_PMEM_0 }, > >> + { .type = WMFW_HALO_XM_PACKED, .base = CS40L50_XMEM_PACKED_0 }, > >> + { .type = WMFW_HALO_YM_PACKED, .base = CS40L50_YMEM_PACKED_0 }, > >> + { .type = WMFW_ADSP2_XM, .base = CS40L50_XMEM_UNPACKED24_0 }, > >> + { .type = WMFW_ADSP2_YM, .base = CS40L50_YMEM_UNPACKED24_0 }, > >> +}; > >> + > >> +static void cs40l50_dsp_remove(void *data) > >> +{ > >> + cs_dsp_remove((struct cs_dsp *)data); > > > > Is the cast required? > > > > Where is this function? > > Seems that the cast is redundant, so I will remove. > > The function cs_dsp_remove() is exported from linux/firmware/cirrus/cs_dsp.h. > > > > >> +} > >> + > >> +static const struct cs_dsp_client_ops cs40l50_client_ops; > > > > What's this for? Where is it used? > > > > In general, I'm not a fan of empty struct definitions like this. > > From the same cs_dsp module as mentioned above, it is a structure containing > callbacks that gives the client driver an opportunity to respond to certain events > during the DSP's lifecycle. It just so happens that this driver does not need to do > anything. However, no struct definition will result in a null pointer dereference by > cs_dsp when it checks for the callbacks. Then check for NULL before dereferencing it?
On Tue, Mar 05, 2024 at 09:58:18AM +0000, Lee Jones wrote: > On Fri, 01 Mar 2024, James Ogletree wrote: > > >> +static const struct cs_dsp_client_ops cs40l50_client_ops; > > > > > > What's this for? Where is it used? > > > > > > In general, I'm not a fan of empty struct definitions like this. > > > > From the same cs_dsp module as mentioned above, it is a structure containing > > callbacks that gives the client driver an opportunity to respond to certain events > > during the DSP's lifecycle. It just so happens that this driver does not need to do > > anything. However, no struct definition will result in a null pointer dereference by > > cs_dsp when it checks for the callbacks. > > Then check for NULL before dereferencing it? It would probably make sense to move the cs40l50_stop_core writes into the pre_run callback, and the CLOCK_ENABLE / cs40l50_configure_dsp stuff into the post_run callback. Which is probably a slightly more idiomatic way to use the API of cs_dsp and would mean some of the callbacks are initialised, thus dodging this problem. We could check for there being no client_ops in the cs_dsp core, but it feels kinda redundant since there really should generally be client ops. Thanks, Charles
> On Mar 5, 2024, at 4:20 AM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > On Tue, Mar 05, 2024 at 09:58:18AM +0000, Lee Jones wrote: >> On Fri, 01 Mar 2024, James Ogletree wrote: >>>>> +static const struct cs_dsp_client_ops cs40l50_client_ops; >>>> >>>> What's this for? Where is it used? >>>> >>>> In general, I'm not a fan of empty struct definitions like this. >>> >>> From the same cs_dsp module as mentioned above, it is a structure containing >>> callbacks that gives the client driver an opportunity to respond to certain events >>> during the DSP's lifecycle. It just so happens that this driver does not need to do >>> anything. However, no struct definition will result in a null pointer dereference by >>> cs_dsp when it checks for the callbacks. >> >> Then check for NULL before dereferencing it? > > It would probably make sense to move the cs40l50_stop_core writes > into the pre_run callback, and the CLOCK_ENABLE / > cs40l50_configure_dsp stuff into the post_run callback. Which is > probably a slightly more idiomatic way to use the API of cs_dsp > and would mean some of the callbacks are initialised, thus > dodging this problem. > > We could check for there being no client_ops in the cs_dsp core, > but it feels kinda redundant since there really should generally > be client ops. I will adopt the first solution here from Charles for v9. Best, James