Hi Hector,
> Dear Lars,
>
> On 07/05/2013 12:32 PM, Lars-Peter Clausen wrote:
> > On 07/05/2013 10:30 AM, Hector Palacios wrote:
> >> Some LRADC channels have a fixed pre-divider, and all can have enabled
> >> an optional divisor by two which allows a maximum input voltage of
> >> VDDIO - 50mV.
> >>
> >> This patch
> >>
> >> - adds the scaling info flag to all channels
> >> - adds a lookup table with max reference voltage per channel
> >>
> >> (where the fixed pre-dividers apply)
> >>
> >> - allows to read the scaling attribute (computed from the Vref)
> >>
> >> Signed-off-by: Hector Palacios <hector.palacios@xxxxxxxx>
> >
> > Looks good, one minor issue.
> >
> > [...]
> >
> >> -
> >> - /* Validate the channel if it doesn't intersect with reserved chans.
> >> */ - bitmap_set(&mask, chan->channel, 1);
> >> - ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> >> - if (ret)
> >> - return -EINVAL;
> >
> > This will conflict with Marek's "iio: mxs-lradc: Remove useless check in
> > read_raw", can you apply Marek's patch to your tree and rebase this
> > series on-top of it?
>
> Yes, sorry, I missed Marek's patch when preparing this series.
>
> >> +
> >> +/*
> >> + * Raw I/O operations
> >> + */
> >> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> >> + const struct iio_chan_spec *chan,
> >> + int *val, int *val2, long m)
> >> +{
> >> + struct mxs_lradc *lradc = iio_priv(iio_dev);
> >> + int ret;
> >> +
> >> + /*
> >> + * See if there is no buffered operation in progress. If there is,
> >> simply + * bail out. This can be improved to support both buffered and
> >> raw IO at + * the same time, yet the code becomes horribly
> >> complicated. Therefore I + * applied KISS principle here.
> >> + */
> >> + ret = mutex_trylock(&lradc->lock);
> >> + if (!ret)
> >> + return -EBUSY;
> >
> > I think the locking in this driver needs some re-work (in a separate
> > patch). There is really no reason why you shouldn't be able to query the
> > scale while the device is running in buffered mode. The common idiom to
> > protect against concurrent buffer mode and raw access is to take the
> > indio_dev->mlock in read_raw, check iio_buffer_enabled(), if it returns
> > true, unlock the lock and return -EBUSY. Otherwise continue to read the
> > raw value.
>
> Actually I didn't touch this code. It's the original code by Marek.
> It appears as difference because I moved part of the code into a function
> called mxs_lradc_read_single() for clarity.
> @Marek, will you look at this suggestion?
Feel free to implement it, I fully agree with the comment. I will gladly review
it.
> >> +
> >> + /* Check for invalid channel */
> >> + if (chan->channel > LRADC_MAX_TOTAL_CHANS
> >> + ret = -EINVAL;
> >
> > This looks wrong. The code will still continue to read and value or the
> > scale and overwrite 'ret'. The original code did check this condition
> > before taking the lock and did return an error. But on the other hand
> > the condition will never be true anyway since you have no channels where
> > the chan value is larger than MAX_TOTAL_CHANS, so maybe just drop this
> > completely.
>
> Same here. This is Marek's original code that was moved. I agree that the
> check is redundant.
> @Marek, will you take care of it as well?
The patch should be rather easy, so feel free to take care of it.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
