* [PATCH v4] iio: adc: mcp3911: simplify code with guard macro @ 2023-12-06 18:39 Marcus Folkesson 2023-12-10 11:36 ` Jonathan Cameron 0 siblings, 1 reply; 3+ messages in thread From: Marcus Folkesson @ 2023-12-06 18:39 UTC (permalink / raw) To: Kent Gustavsson, Jonathan Cameron, Lars-Peter Clausen Cc: linux-iio, linux-kernel, Marcus Folkesson Use the guard(mutex) macro for handle mutex lock/unlocks. Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> --- Changes in v4: - Remove unreachable breaks - Link to v3: https://lore.kernel.org/r/20231205-mcp3911-guard-v3-1-df83e956d1e9@gmail.com Changes in v3: - Return early in good paths as well - Rebase against master - Link to v2: https://lore.kernel.org/r/20231127-mcp3911-guard-v2-1-9462630dca1e@gmail.com Changes in v2: - Return directly instead of goto label - Link to v1: https://lore.kernel.org/r/20231125-mcp3911-guard-v1-1-2748d16a3f3f@gmail.com --- drivers/iio/adc/mcp3911.c | 55 +++++++++++++---------------------------------- 1 file changed, 15 insertions(+), 40 deletions(-) diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c index d864558bc087..dfcb6cb7570f 100644 --- a/drivers/iio/adc/mcp3911.c +++ b/drivers/iio/adc/mcp3911.c @@ -7,6 +7,7 @@ */ #include <linux/bitfield.h> #include <linux/bits.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/err.h> @@ -318,44 +319,28 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, struct mcp3911 *adc = iio_priv(indio_dev); int ret = -EINVAL; - mutex_lock(&adc->lock); + guard(mutex)(&adc->lock); switch (mask) { case IIO_CHAN_INFO_RAW: ret = mcp3911_read(adc, MCP3911_CHANNEL(channel->channel), val, 3); if (ret) - goto out; + return ret; *val = sign_extend32(*val, 23); - - ret = IIO_VAL_INT; - break; - + return IIO_VAL_INT; case IIO_CHAN_INFO_OFFSET: - ret = adc->chip->get_offset(adc, channel->channel, val); - if (ret) - goto out; - - ret = IIO_VAL_INT; - break; + return (ret) ? ret : IIO_VAL_INT; case IIO_CHAN_INFO_OVERSAMPLING_RATIO: ret = adc->chip->get_osr(adc, val); - if (ret) - goto out; - - ret = IIO_VAL_INT; - break; - + return (ret) ? ret : IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: *val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0]; *val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1]; - ret = IIO_VAL_INT_PLUS_NANO; - break; + return IIO_VAL_INT_PLUS_NANO; } -out: - mutex_unlock(&adc->lock); return ret; } @@ -364,9 +349,8 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, int val2, long mask) { struct mcp3911 *adc = iio_priv(indio_dev); - int ret = -EINVAL; - mutex_lock(&adc->lock); + guard(mutex)(&adc->lock); switch (mask) { case IIO_CHAN_INFO_SCALE: for (int i = 0; i < MCP3911_NUM_SCALES; i++) { @@ -374,32 +358,24 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, val2 == mcp3911_scale_table[i][1]) { adc->gain[channel->channel] = BIT(i); - ret = adc->chip->set_scale(adc, channel->channel, i); + return adc->chip->set_scale(adc, channel->channel, i); } } break; case IIO_CHAN_INFO_OFFSET: - if (val2 != 0) { - ret = -EINVAL; - goto out; - } - - ret = adc->chip->set_offset(adc, channel->channel, val); - break; + if (val2 != 0) + return -EINVAL; + return adc->chip->set_offset(adc, channel->channel, val); case IIO_CHAN_INFO_OVERSAMPLING_RATIO: for (int i = 0; i < ARRAY_SIZE(mcp3911_osr_table); i++) { if (val == mcp3911_osr_table[i]) { - ret = adc->chip->set_osr(adc, i); - break; + return adc->chip->set_osr(adc, i); } } break; } - -out: - mutex_unlock(&adc->lock); - return ret; + return -EINVAL; } static int mcp3911_calc_scale_table(struct mcp3911 *adc) @@ -532,7 +508,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p) int i = 0; int ret; - mutex_lock(&adc->lock); + guard(mutex)(&adc->lock); adc->tx_buf = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr); ret = spi_sync_transfer(adc->spi, xfer, ARRAY_SIZE(xfer)); if (ret < 0) { @@ -549,7 +525,6 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p) iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, iio_get_time_ns(indio_dev)); out: - mutex_unlock(&adc->lock); iio_trigger_notify_done(indio_dev->trig); return IRQ_HANDLED; --- base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a change-id: 20231125-mcp3911-guard-866591e2c947 Best regards, -- Marcus Folkesson <marcus.folkesson@gmail.com> ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] iio: adc: mcp3911: simplify code with guard macro 2023-12-06 18:39 [PATCH v4] iio: adc: mcp3911: simplify code with guard macro Marcus Folkesson @ 2023-12-10 11:36 ` Jonathan Cameron 2023-12-10 13:44 ` Marcus Folkesson 0 siblings, 1 reply; 3+ messages in thread From: Jonathan Cameron @ 2023-12-10 11:36 UTC (permalink / raw) To: Marcus Folkesson Cc: Kent Gustavsson, Lars-Peter Clausen, linux-iio, linux-kernel On Wed, 06 Dec 2023 19:39:04 +0100 Marcus Folkesson <marcus.folkesson@gmail.com> wrote: > Use the guard(mutex) macro for handle mutex lock/unlocks. > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> Various changes inline - most are focused around simplifying the path for any given flow through the code. Rather than going for a v5 - I've applied this with the following diff on top diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c index dfcb6cb7570f..7a32e7a1be9d 100644 --- a/drivers/iio/adc/mcp3911.c +++ b/drivers/iio/adc/mcp3911.c @@ -317,7 +317,7 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, int *val2, long mask) { struct mcp3911 *adc = iio_priv(indio_dev); - int ret = -EINVAL; + int ret; guard(mutex)(&adc->lock); switch (mask) { @@ -331,17 +331,23 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, return IIO_VAL_INT; case IIO_CHAN_INFO_OFFSET: ret = adc->chip->get_offset(adc, channel->channel, val); - return (ret) ? ret : IIO_VAL_INT; + if (ret) + return ret; + + return IIO_VAL_INT; case IIO_CHAN_INFO_OVERSAMPLING_RATIO: ret = adc->chip->get_osr(adc, val); - return (ret) ? ret : IIO_VAL_INT; + if (ret) + return ret; + + return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: *val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0]; *val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1]; return IIO_VAL_INT_PLUS_NANO; + default: + return -EINVAL; } - - return ret; } static int mcp3911_write_raw(struct iio_dev *indio_dev, @@ -361,7 +367,7 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, return adc->chip->set_scale(adc, channel->channel, i); } } - break; + return -EINVAL; case IIO_CHAN_INFO_OFFSET: if (val2 != 0) return -EINVAL; @@ -373,9 +379,10 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, return adc->chip->set_osr(adc, i); } } - break; + return -EINVAL; + default: + return -EINVAL; } - return -EINVAL; } hope that's ok with you! Jonathan > --- > Changes in v4: > - Remove unreachable breaks > - Link to v3: https://lore.kernel.org/r/20231205-mcp3911-guard-v3-1-df83e956d1e9@gmail.com > > Changes in v3: > - Return early in good paths as well > - Rebase against master > - Link to v2: https://lore.kernel.org/r/20231127-mcp3911-guard-v2-1-9462630dca1e@gmail.com > > Changes in v2: > - Return directly instead of goto label > - Link to v1: https://lore.kernel.org/r/20231125-mcp3911-guard-v1-1-2748d16a3f3f@gmail.com > --- > drivers/iio/adc/mcp3911.c | 55 +++++++++++++---------------------------------- > 1 file changed, 15 insertions(+), 40 deletions(-) > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c > index d864558bc087..dfcb6cb7570f 100644 > --- a/drivers/iio/adc/mcp3911.c > +++ b/drivers/iio/adc/mcp3911.c > @@ -7,6 +7,7 @@ > */ > #include <linux/bitfield.h> > #include <linux/bits.h> > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/err.h> > @@ -318,44 +319,28 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, > struct mcp3911 *adc = iio_priv(indio_dev); > int ret = -EINVAL; > > - mutex_lock(&adc->lock); > + guard(mutex)(&adc->lock); > switch (mask) { > case IIO_CHAN_INFO_RAW: > ret = mcp3911_read(adc, > MCP3911_CHANNEL(channel->channel), val, 3); > if (ret) > - goto out; > + return ret; > > *val = sign_extend32(*val, 23); > - > - ret = IIO_VAL_INT; > - break; > - > + return IIO_VAL_INT; > case IIO_CHAN_INFO_OFFSET: > - > ret = adc->chip->get_offset(adc, channel->channel, val); > - if (ret) > - goto out; > - > - ret = IIO_VAL_INT; > - break; > + return (ret) ? ret : IIO_VAL_INT; Don't do this. The ternary just makes it harder to read. if (ret) return ret; return IIO_VAL_INT; + the brackets around (ret) are unnecessary. > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > ret = adc->chip->get_osr(adc, val); > - if (ret) > - goto out; > - > - ret = IIO_VAL_INT; > - break; > - > + return (ret) ? ret : IIO_VAL_INT; here as well > case IIO_CHAN_INFO_SCALE: > *val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0]; > *val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1]; > - ret = IIO_VAL_INT_PLUS_NANO; > - break; > + return IIO_VAL_INT_PLUS_NANO; > } > > -out: > - mutex_unlock(&adc->lock); > return ret; This ret is only reachable if we don't hit any of the switch entries so better to add add a default and return -EINVAL to make that apparent and let the compiler detect if we are missing a return earlier. > } > > @@ -364,9 +349,8 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, > int val2, long mask) > { > struct mcp3911 *adc = iio_priv(indio_dev); > - int ret = -EINVAL; > > - mutex_lock(&adc->lock); > + guard(mutex)(&adc->lock); > switch (mask) { > case IIO_CHAN_INFO_SCALE: > for (int i = 0; i < MCP3911_NUM_SCALES; i++) { > @@ -374,32 +358,24 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, > val2 == mcp3911_scale_table[i][1]) { > > adc->gain[channel->channel] = BIT(i); > - ret = adc->chip->set_scale(adc, channel->channel, i); > + return adc->chip->set_scale(adc, channel->channel, i); > } > } > break; > case IIO_CHAN_INFO_OFFSET: > - if (val2 != 0) { > - ret = -EINVAL; > - goto out; > - } > - > - ret = adc->chip->set_offset(adc, channel->channel, val); > - break; > + if (val2 != 0) > + return -EINVAL; > > + return adc->chip->set_offset(adc, channel->channel, val); > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > for (int i = 0; i < ARRAY_SIZE(mcp3911_osr_table); i++) { > if (val == mcp3911_osr_table[i]) { > - ret = adc->chip->set_osr(adc, i); > - break; > + return adc->chip->set_osr(adc, i); > } > } > break; return -INVAL; here and similar above to simplify the flow. > } > - > -out: > - mutex_unlock(&adc->lock); > - return ret; > + return -EINVAL; Move this to a default. > } > > static int mcp3911_calc_scale_table(struct mcp3911 *adc) > @@ -532,7 +508,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p) > int i = 0; > int ret; > > - mutex_lock(&adc->lock); > + guard(mutex)(&adc->lock); > adc->tx_buf = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr); > ret = spi_sync_transfer(adc->spi, xfer, ARRAY_SIZE(xfer)); > if (ret < 0) { > @@ -549,7 +525,6 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p) > iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, > iio_get_time_ns(indio_dev)); > out: > - mutex_unlock(&adc->lock); > iio_trigger_notify_done(indio_dev->trig); > > return IRQ_HANDLED; > > --- > base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a > change-id: 20231125-mcp3911-guard-866591e2c947 > > Best regards, ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] iio: adc: mcp3911: simplify code with guard macro 2023-12-10 11:36 ` Jonathan Cameron @ 2023-12-10 13:44 ` Marcus Folkesson 0 siblings, 0 replies; 3+ messages in thread From: Marcus Folkesson @ 2023-12-10 13:44 UTC (permalink / raw) To: Jonathan Cameron Cc: Kent Gustavsson, Lars-Peter Clausen, linux-iio, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2928 bytes --] On Sun, Dec 10, 2023 at 11:36:32AM +0000, Jonathan Cameron wrote: > On Wed, 06 Dec 2023 19:39:04 +0100 > Marcus Folkesson <marcus.folkesson@gmail.com> wrote: > > > Use the guard(mutex) macro for handle mutex lock/unlocks. > > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> > Various changes inline - most are focused around simplifying the > path for any given flow through the code. > > Rather than going for a v5 - I've applied this with the following diff on top > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c > index dfcb6cb7570f..7a32e7a1be9d 100644 > --- a/drivers/iio/adc/mcp3911.c > +++ b/drivers/iio/adc/mcp3911.c > @@ -317,7 +317,7 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, > int *val2, long mask) > { > struct mcp3911 *adc = iio_priv(indio_dev); > - int ret = -EINVAL; > + int ret; > > guard(mutex)(&adc->lock); > switch (mask) { > @@ -331,17 +331,23 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_INT; > case IIO_CHAN_INFO_OFFSET: > ret = adc->chip->get_offset(adc, channel->channel, val); > - return (ret) ? ret : IIO_VAL_INT; > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > ret = adc->chip->get_osr(adc, val); > - return (ret) ? ret : IIO_VAL_INT; > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > case IIO_CHAN_INFO_SCALE: > *val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0]; > *val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1]; > return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > } > - > - return ret; > } > > static int mcp3911_write_raw(struct iio_dev *indio_dev, > @@ -361,7 +367,7 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, > return adc->chip->set_scale(adc, channel->channel, i); > } > } > - break; > + return -EINVAL; > case IIO_CHAN_INFO_OFFSET: > if (val2 != 0) > return -EINVAL; > @@ -373,9 +379,10 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, > return adc->chip->set_osr(adc, i); > } > } > - break; > + return -EINVAL; > + default: > + return -EINVAL; > } > - return -EINVAL; > } > > hope that's ok with you! Thank you Jonathan Best regards Marcus Folkesson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-10 13:40 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-06 18:39 [PATCH v4] iio: adc: mcp3911: simplify code with guard macro Marcus Folkesson 2023-12-10 11:36 ` Jonathan Cameron 2023-12-10 13:44 ` Marcus Folkesson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox