On 04/06/11 15:02, Alan Cox wrote: >>>> +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? > > That was actually my starting point but they are quite different - one > does the gpio check each loop then a read, the other does a read each > loop and then nothing. Mixed together you get > > while() { > if > if > else > if > > } > if > > else > > and it looks like spagetti .. Good point. I clearly didn't look closely enough. Acked-by: Jonathan Cameron <jic23@xxxxxxxxx> > > > > >>>> +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@xxxxxxxxxxxxxxx >>> 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@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ > > -- 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