| Message ID | 20241203111314.2420473-7-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: rzg2l_adc: Add support for RZ/G3S | expand |
Hi Claudiu, On 03/12/2024 11:13, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Simplify the locking scheme in rzg2l_adc_read_raw() by saving the converted > value only if the rzg2l_adc_conversion() returns success. The approach > simplifies the addition of thermal sensor support (that will be done in the > next commits). The downside is that the ret variable need to be checked > twice. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/iio/adc/rzg2l_adc.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > index 62932f9295b6..eed2944bd98d 100644 > --- a/drivers/iio/adc/rzg2l_adc.c > +++ b/drivers/iio/adc/rzg2l_adc.c > @@ -227,14 +227,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev, > mutex_lock(&adc->lock); > ch = chan->channel & RZG2L_ADC_CHN_MASK; > ret = rzg2l_adc_conversion(indio_dev, adc, ch); > - if (ret) { > - mutex_unlock(&adc->lock); > - return ret; > - } > - *val = adc->last_val[ch]; > + if (!ret) > + *val = adc->last_val[ch]; > mutex_unlock(&adc->lock); > > - return IIO_VAL_INT; > + return ret ? ret : IIO_VAL_INT; It would be maybe slightly neater to use: if (!ret) { *val = adc->last_val[ch]; ret = IIO_VAL_INT; } mutex_unlock(&adc->lock); return ret; Thanks,
On Tue, 3 Dec 2024 13:03:29 +0000 Paul Barker <paul.barker.ct@bp.renesas.com> wrote: > Hi Claudiu, > > On 03/12/2024 11:13, Claudiu wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Simplify the locking scheme in rzg2l_adc_read_raw() by saving the converted > > value only if the rzg2l_adc_conversion() returns success. The approach > > simplifies the addition of thermal sensor support (that will be done in the > > next commits). The downside is that the ret variable need to be checked > > twice. > > > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > --- > > drivers/iio/adc/rzg2l_adc.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > > index 62932f9295b6..eed2944bd98d 100644 > > --- a/drivers/iio/adc/rzg2l_adc.c > > +++ b/drivers/iio/adc/rzg2l_adc.c > > @@ -227,14 +227,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev, > > mutex_lock(&adc->lock); > > ch = chan->channel & RZG2L_ADC_CHN_MASK; > > ret = rzg2l_adc_conversion(indio_dev, adc, ch); > > - if (ret) { > > - mutex_unlock(&adc->lock); > > - return ret; > > - } > > - *val = adc->last_val[ch]; > > + if (!ret) > > + *val = adc->last_val[ch]; > > mutex_unlock(&adc->lock); > > > > - return IIO_VAL_INT; > > + return ret ? ret : IIO_VAL_INT; > > It would be maybe slightly neater to use: > > if (!ret) { > *val = adc->last_val[ch]; > ret = IIO_VAL_INT; > } > mutex_unlock(&adc->lock); > > return ret; > Better I think to use {} for scope and guard(mutex)() ... if (ret) return ret; *val = adc->last_val[ch]; Where possible keeping the error path as the out of line element is easier to follow on basis that is most common pattern so what a reviewers eye is 'trained' to see. Jonathan > Thanks, >
diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c index 62932f9295b6..eed2944bd98d 100644 --- a/drivers/iio/adc/rzg2l_adc.c +++ b/drivers/iio/adc/rzg2l_adc.c @@ -227,14 +227,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev, mutex_lock(&adc->lock); ch = chan->channel & RZG2L_ADC_CHN_MASK; ret = rzg2l_adc_conversion(indio_dev, adc, ch); - if (ret) { - mutex_unlock(&adc->lock); - return ret; - } - *val = adc->last_val[ch]; + if (!ret) + *val = adc->last_val[ch]; mutex_unlock(&adc->lock); - return IIO_VAL_INT; + return ret ? ret : IIO_VAL_INT; default: return -EINVAL;