| Message ID | 20230828170525.335671-3-vkarpovi@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/7] ASoC: cs35l45: Add support for Chip ID 0x35A460 | expand |
On Mon, Aug 28, 2023 at 12:05:21PM -0500, Vlad Karpovich wrote: > Checks the index computed by the virq offset before printing the > error condition in cs35l45_spk_safe_err() handler. > > Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com> > Signed-off-by: Vlad Karpovich <vkarpovi@opensource.cirrus.com> > --- > sound/soc/codecs/cs35l45.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c > index 40fb64904260..2c9b41171a05 100644 > --- a/sound/soc/codecs/cs35l45.c > +++ b/sound/soc/codecs/cs35l45.c > @@ -1023,7 +1023,10 @@ static irqreturn_t cs35l45_spk_safe_err(int irq, void *data) > > i = irq - regmap_irq_get_virq(cs35l45->irq_data, 0); > > - dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name); > + if (i < 0 || i > 6) This looks a little odd, there appear to be 8 IRQs attached to this function whereas this says 6. Also this check seems like it will be hard to keep in sync as things change. Assuming this error check is actually necessary, would it be perhaps better to check i is smaller than ARRAY_SIZE(cs35l45_irqs) and check that the attached function is cs35l45_spk_safe_err. That should be more robust against future changes to the IRQs. Thanks, Charles
On Tue, 29 Aug 2023 10:07:51 +0000 Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Mon, Aug 28, 2023 at 12:05:21PM -0500, Vlad Karpovich wrote: > > Checks the index computed by the virq offset before printing the > > error condition in cs35l45_spk_safe_err() handler. > > > > Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com> > > Signed-off-by: Vlad Karpovich <vkarpovi@opensource.cirrus.com> > > --- > > sound/soc/codecs/cs35l45.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c > > index 40fb64904260..2c9b41171a05 100644 > > --- a/sound/soc/codecs/cs35l45.c > > +++ b/sound/soc/codecs/cs35l45.c > > @@ -1023,7 +1023,10 @@ static irqreturn_t cs35l45_spk_safe_err(int irq, void *data) > > > > i = irq - regmap_irq_get_virq(cs35l45->irq_data, 0); > > > > - dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name); > > + if (i < 0 || i > 6) > > This looks a little odd, there appear to be 8 IRQs attached to > this function whereas this says 6. Also this check seems like > it will be hard to keep in sync as things change. > > Assuming this error check is actually necessary, would it be > perhaps better to check i is smaller than ARRAY_SIZE(cs35l45_irqs) > and check that the attached function is cs35l45_spk_safe_err. > That should be more robust against future changes to the IRQs. > > Thanks, > Charles Thank you. It was picked up from a different branch,I will change to ARRAY_SIZE.
diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c index 40fb64904260..2c9b41171a05 100644 --- a/sound/soc/codecs/cs35l45.c +++ b/sound/soc/codecs/cs35l45.c @@ -1023,7 +1023,10 @@ static irqreturn_t cs35l45_spk_safe_err(int irq, void *data) i = irq - regmap_irq_get_virq(cs35l45->irq_data, 0); - dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name); + if (i < 0 || i > 6) + dev_err(cs35l45->dev, "Unspecified global error condition (%d) detected!\n", irq); + else + dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name); return IRQ_HANDLED; }