| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Sep 2010 09:20:19 +0200
From: <samu.p.onkalo@...ia.com>
To: <jic23@....ac.uk>
CC: <alan@...rguk.ukuu.org.uk>, <achew@...dia.com>,
<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <akpm@...ux-foundation.org>,
<khali@...ux-fr.org>, <ldewangan@...dia.com>
Subject: RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
>-----Original Message-----
>From: ext Jonathan Cameron [mailto:jic23@....ac.uk]
>Sent: 03 September, 2010 10:23
>To: Onkalo Samu.P (Nokia-MS/Tampere)
>Cc: alan@...rguk.ukuu.org.uk; achew@...dia.com; linux-
>kernel@...r.kernel.org; linux-iio@...r.kernel.org; linux-
>i2c@...r.kernel.org; akpm@...ux-foundation.org; khali@...ux-fr.org;
>ldewangan@...dia.com
>Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
>
>On 09/03/10 06:18, samu.p.onkalo@...ia.com wrote:
>>
>>
>>> -----Original Message-----
>>> From: linux-iio-owner@...r.kernel.org [mailto:linux-iio-
>>> owner@...r.kernel.org] On Behalf Of ext Alan Cox
>>> Sent: 03 September, 2010 01:20
>>> To: achew@...dia.com
>>> Cc: linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org; linux-
>>> i2c@...r.kernel.org; akpm@...ux-foundation.org; khali@...ux-fr.org;
>>> ldewangan@...dia.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists