lkml.org 
[lkml]   [2010]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor


    >-----Original Message-----
    >From: ext Jonathan Cameron [mailto:jic23@cam.ac.uk]
    >Sent: 03 September, 2010 10:23
    >To: Onkalo Samu.P (Nokia-MS/Tampere)
    >Cc: alan@lxorguk.ukuu.org.uk; achew@nvidia.com; linux-
    >kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
    >i2c@vger.kernel.org; akpm@linux-foundation.org; khali@linux-fr.org;
    >ldewangan@nvidia.com
    >Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
    >
    >On 09/03/10 06:18, samu.p.onkalo@nokia.com wrote:
    >>
    >>
    >>> -----Original Message-----
    >>> From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-
    >>> owner@vger.kernel.org] On Behalf Of ext Alan Cox
    >>> Sent: 03 September, 2010 01:20
    >>> To: achew@nvidia.com
    >>> Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
    >>> i2c@vger.kernel.org; akpm@linux-foundation.org; khali@linux-fr.org;
    >>> ldewangan@nvidia.com
    >>> Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
    >>>
    >>>> +/*
    >>>> + * Shows the device's mode. 0 = off, 1 = on.
    >>>> + */
    >>>
    >>> Should this not be handled by runtime pm nowdays ?
    >>>
    >>>> + if ((oval < 0) || (oval > 1)) {
    >>>> + dev_err(dev, "mode value is not supported\n");
    >>>> + return -EINVAL;
    >>>> + }
    >>>
    >>> ulong cannot be < 0 and doesn't need all the brackets
    >>>
    >>>
    >>>> + /* Wait for the conversion to complete. */
    >>>> + while (timeout_ms) {
    >>>> + msleep(AK8975_CONVERSION_DONE_POLL_TIME);
    >>>> + state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
    >>>> + if (state)
    >>>> + break;
    >>>> + timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
    >>>> + }
    >>>
    >>> This makes some fairly specific wiring assumptions about how the
    >ak8975
    >>> is configured. I'm looking at the ak8974 driver in our tree and also
    >>> wondering if they can be combined sanely.
    >>
    >> With ak8974 chip, it is possible to have similar functionality without
    >interrupt
    >> pin. This is most probably true also for ak8975 chip. It is not good
    >to assume
    >> that everyone uses interrupt pin if the same functionally can be
    >achieved
    >> another way. I mean polling via I2C instead of checking GPIO state
    >after the
    >> sleep.
    >Of course this can be done, but it's up to Andrew to decide whether he
    >wants to.
    >I think the usual principal of writing only what people currently need
    >applies
    >here. Perhaps a comment in the code to point out this could be done is
    >a sensible
    >compromise?

    Sure. And the one who needs it may freely send patches.

    >>
    >> Based on the this driver it seems that ak8974 and ak8975 are quite
    >similar, but
    >> also there are many differences like different register map. Maybe
    >combining
    >> these two makes implementation just messy.
    >>
    >>
    >>>
    >>>> + status = ak8975_read_data(client, AK8975_REG_ST1, 1,
    >>> &read_status);
    >>>> + if (!status) {
    >>>> + dev_err(&client->dev, "Error in reading ST1\n");
    >>>> + return false;
    >>>
    >>> I would have expected these to return a meaningful error code not 0 ?
    >>>
    >>>> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode,
    >>> store_mode, 0);
    >>>> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale,
    >>> NULL, 0);
    >>>> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale,
    >>> NULL, 1);
    >>>> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale,
    >>> NULL, 2);
    >>>> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
    >>>> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
    >>>> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
    >>>
    >>> This seems odd as an interface as it's raw when the maths to provide
    >>> non-raw (and thus abstract and easy for user space) data is trivial
    >>> enough to do in kernel
    >>>
    >>> (but then I still suspect it should jusst be an input device of
    >course)
    >>>
    >>>> +static int ak8975_probe(struct i2c_client *client,
    >>>> + const struct i2c_device_id *id)
    >>>> +{
    >>>> + struct ak8975_data *data;
    >>>> + int err;
    >>>> +
    >>>> + /* Allocate our device context. */
    >>>> + data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
    >>>> + if (!data) {
    >>>> + dev_err(&client->dev, "Memory allocation fails\n");
    >>>> + err = -ENOMEM;
    >>>> + goto exit;
    >>>> + }
    >>>> +
    >>>> + i2c_set_clientdata(client, data);
    >>>> + data->client = client;
    >>>> +
    >>>> + mutex_init(&data->lock);
    >>>> +
    >>>> + /* Grab and set up the supplied GPIO. */
    >>>> + data->eoc_irq = client->irq;
    >>>> + data->eoc_gpio = irq_to_gpio(client->irq);
    >>>
    >>> It may not be via a GPIO. Better to do the GPIO handling in platform
    >>> abstraction or accept passing IRQ and no GPIO value to mean "just use
    >>> the
    >>> IRQ". Ie do all the gpio foo if (data->eoc_gpio) { ... }
    >>>
    >>>
    >>>> +
    >>>> + err = gpio_request(data->eoc_gpio, "ak_8975");
    >>>> + if (err < 0) {
    >>>> + dev_err(&client->dev, "failed to request GPIO %d, error
    >>> %d\n",
    >>>> + data->eoc_gpio, err);
    >>>> + goto exit_free;
    >>>> + }
    >>>> +
    >>>> + err = gpio_direction_input(data->eoc_gpio);
    >>>> + if (err < 0) {
    >>>> + dev_err(&client->dev, "Failed to configure input direction
    >>> for"
    >>>> + " GPIO %d, error %d\n", data->eoc_gpio, err);
    >>>> + gpio_free(data->eoc_gpio);
    >>>
    >>> This frees the GPIO twice ?
    >>>
    >>> Looks basically sound to me.
    >>>
    >>> Alan


    \
     
     \ /
      Last update: 2010-09-03 09:25    [W:0.042 / U:0.108 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site