On Mon, Apr 09, 2012 at 22:51:45 +0200, Jonathan Cameron wrote:
> On 04/09/2012 12:34 PM, Srinidhi KASAGAR wrote:
> > Add support for lsm303dlh magnetometer device.
> >
> > Signed-off-by: srinidhi kasagar <srinidhi.kasagar@xxxxxxxxxxxxxx>
> > Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Hi Srinidhi,
>
> Basically a sound driver with a few easy bits to fix.
>
> Few nitpicks and the error paths in probe need another look.
Hi Jonathan,
Thanks a lot for the review.
>
> The config attribute needs to go. If there is stuff in there you
> need access to then it needs an abi that doesn't involve magic
> numbers. (I'm hoping this is just a left over from debugging!)
I agree, its a useless attribute, removed.
[...]
> We have the old question of _range attributes as well and their
> interaction with scale. I've always been dubious about these,
> mainly because it's not entirely obvious how to format the
> weirder general cases. Do you have a pressing need for range
> or could it just be dropped in favour of allowing write for the
> scale parameter? If nothing else, scale is available for inkernel
> users and range isn't.
This one too, will drop range attribute, while keeping scale for the
in kernel usage only.
[...]
> > +}
> > +
> > +static inline int is_device_on(struct lsm303dlh_m_data *data)
> > +{
> Just roll this next line into the place it is used.
Not sure if I understood this properly.
[...]
> > +static ssize_t set_operating_mode(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct lsm303dlh_m_data *data = iio_priv(indio_dev);
> > + struct i2c_client *client = data->client;
> > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > + int error;
> > + unsigned long mode = 0;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + error = kstrtoul(buf, 10, &mode);
> > + if (error) {
> > + count = error;
> > + goto exit;
> > + }
> > +
> Hmm.. This is magic number teritory so needs a rething in the long run.
Would like to keep this for the moment.
> > + if (mode > SLEEP_MODE) {
> > + dev_err(&client->dev, "trying to set invalid mode\n");
> > + count = -EINVAL;
> > + goto exit;
> > + }
[...]
> > +static int set_configuration(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> This looks like direct register access to me. Magic numbers
> are pretty much a no no so please get rid of this and associated
> attribute
OK, will remove.
[...]
> > +static struct attribute *lsm303dlh_m_attributes[] = {
> > + &iio_dev_attr_config.dev_attr.attr,
> > + &iio_dev_attr_mode.dev_attr.attr,
> gah. Not your fault at all but we really need to standardise
> this 'mode' type attributes (far from just an iio thing!)
>
> > + &iio_dev_attr_in_magn_range.dev_attr.attr,
> hmm.. range isn't techincally in the abi (though I know there are
> a few drivers using it). The issue is the interaction with scale
> and scale is a lot easier to define a format for.
> Could let it go here. It's not a parameter I'm particularly worried
> about going through a slow deprecation for and there are a couple
> of other users in tree. (particularly as you support scale here
> as well!) Actually, do you have a strong reason for supplying range
> at all?
Not really, I will keep the scale factors, and remove the range.
Will fix the rest of the comments and send out the v2 version
of the patch.
Thanks,
srinidhi
--
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
