| 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
| ||
|
Message-ID: <4D9C6F82.5060404@cam.ac.uk>
Date: Wed, 06 Apr 2011 14:49:54 +0100
From: Jonathan Cameron <jic23@....ac.uk>
To: Jonathan Cameron <jic23@....ac.uk>
CC: Alan Cox <alan@...ux.jf.intel.com>, linux-kernel@...r.kernel.org,
greg@...ah.com,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH] oaktrail AK8975 support via IIO
On 04/06/11 14:39, Jonathan Cameron wrote:
> On 04/06/11 13:31, Alan Cox wrote:
>> Actually this is generic 'I have no GPIO' support for the AK8975, instead
>> we have to go bus polling.
Can we rename the patch to reflect that?
>>
>> Huang Liang produced an initial patch which worked by removing the support
>> for GPIO pins. This patch instead makes GPIO support optional and Huang then
>> fixed some bugs in it.
Patch is fine, but could perhaps be a little shorter and tidier as suggested
below.
>
> cc'd linux-iio
>> Signed-off-by: Alan Cox <alan@...ux.intel.com>
>> ---
>>
>> drivers/staging/iio/magnetometer/ak8975.c | 120 ++++++++++++++++++++---------
>> 1 files changed, 83 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
>> index 420f206..d71904a 100644
>> --- a/drivers/staging/iio/magnetometer/ak8975.c
>> +++ b/drivers/staging/iio/magnetometer/ak8975.c
>> @@ -206,7 +206,7 @@ static int ak8975_setup(struct i2c_client *client)
>> }
>>
>> /* Precalculate scale factor for each axis and
>> - store in the device data. */
>> + store in the device data. */
>> data->raw_to_gauss[0] = ((data->asa[0] + 128) * 30) >> 8;
>> data->raw_to_gauss[1] = ((data->asa[1] + 128) * 30) >> 8;
>> data->raw_to_gauss[2] = ((data->asa[2] + 128) * 30) >> 8;
>> @@ -316,6 +316,59 @@ static ssize_t show_scale(struct device *dev, struct device_attribute *devattr,
>> return sprintf(buf, "%ld\n", data->raw_to_gauss[this_attr->address]);
>> }
>>
>> +static int wait_conversion_complete_gpio(struct ak8975_data *data)
>> +{
>> + struct i2c_client *client = data->client;
>> + u8 read_status;
>> + u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
>> + int ret;
>> +
>> + /* Wait for the conversion to complete. */
>> + while (timeout_ms) {
>> + msleep(AK8975_CONVERSION_DONE_POLL_TIME);
>> + if (gpio_get_value(data->eoc_gpio))
>> + break;
>> + timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
>> + }
>> + if (!timeout_ms) {
>> + dev_err(&client->dev, "Conversion timeout happened\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "Error in reading ST1\n");
>> + return ret;
>> + }
>> + return read_status;
>> +}
>> +
Its not immediately obvious to me why we need these two separate functions.
They only differ by a couple of lines. Perhaps push querying if the gpio
down into a single function?
>> +static int wait_conversion_complete_polled(struct ak8975_data *data)
>> +{
>> + struct i2c_client *client = data->client;
>> + u8 read_status;
>> + u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
>> + int ret;
>> +
>> + /* Wait for the conversion to complete. */
>> + while (timeout_ms) {
>> + msleep(AK8975_CONVERSION_DONE_POLL_TIME);
>> + ret = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "Error in reading ST1\n");
>> + return ret;
>> + }
>> + if (read_status)
>> + break;
>> + timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
>> + }
>> + if (!timeout_ms) {
>> + dev_err(&client->dev, "Conversion timeout happened\n");
>> + return -EINVAL;
>> + }
>> + return read_status;
>> +}
>> +
>> /*
>> * Emits the raw flux value for the x, y, or z axis.
>> */
>> @@ -326,7 +379,6 @@ static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
>> struct ak8975_data *data = indio_dev->dev_data;
>> struct i2c_client *client = data->client;
>> struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
>> - u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
>> u16 meas_reg;
>> s16 raw;
>> u8 read_status;
>> @@ -352,23 +404,14 @@ static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
>> }
>>
>> /* Wait for the conversion to complete. */
>> - while (timeout_ms) {
>> - msleep(AK8975_CONVERSION_DONE_POLL_TIME);
>> - if (gpio_get_value(data->eoc_gpio))
>> - break;
>> - timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
>> - }
>> - if (!timeout_ms) {
>> - dev_err(&client->dev, "Conversion timeout happened\n");
>> - ret = -EINVAL;
>> + if (data->eoc_gpio)
>> + ret = wait_conversion_complete_gpio(data);
>> + else
>> + ret = wait_conversion_complete_polled(data);
>> + if (ret < 0)
>> goto exit;
>> - }
>>
>> - ret = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
>> - if (ret < 0) {
>> - dev_err(&client->dev, "Error in reading ST1\n");
>> - goto exit;
>> - }
>> + read_status = ret;
>>
>> if (read_status & AK8975_REG_ST1_DRDY_MASK) {
>> ret = ak8975_read_data(client, AK8975_REG_ST2, 1, &read_status);
>> @@ -454,25 +497,26 @@ static int ak8975_probe(struct i2c_client *client,
>> data->eoc_irq = client->irq;
>> data->eoc_gpio = irq_to_gpio(client->irq);
>>
>> - if (!data->eoc_gpio) {
>> - dev_err(&client->dev, "failed, no valid GPIO\n");
>> - err = -EINVAL;
>> - goto exit_free;
>> - }
>> -
>> - 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;
>> - }
>> + /* We may not have a GPIO based IRQ to scan, that is fine, we will
>> + poll if so */
>> + if (data->eoc_gpio > 0) {
>> + 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);
>> - goto exit_gpio;
>> - }
>> + 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);
>> + goto exit_gpio;
Not really relevant to this patch, but this handling could be cleaned up using gpio_request_one.
>> + }
>> + } else
>> + data->eoc_gpio = 0; /* No GPIO available */
>>
>> /* Perform some basic start-of-day setup of the device. */
>> err = ak8975_setup(client);
>> @@ -503,7 +547,8 @@ static int ak8975_probe(struct i2c_client *client,
>> exit_free_iio:
>> iio_free_device(data->indio_dev);
>> exit_gpio:
>> - gpio_free(data->eoc_gpio);
>> + if (data->eoc_gpio)
>> + gpio_free(data->eoc_gpio);
>> exit_free:
>> kfree(data);
>> exit:
>> @@ -517,7 +562,8 @@ static int ak8975_remove(struct i2c_client *client)
>> iio_device_unregister(data->indio_dev);
>> iio_free_device(data->indio_dev);
>>
>> - gpio_free(data->eoc_gpio);
>> + if (data->eoc_gpio)
>> + gpio_free(data->eoc_gpio);
>>
>> kfree(data);
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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