I like the changes. The savings are a little less than what I had hoped
for, but a lot of the redundant code that gets copied to each driver
is gone.
On Thursday 24 March 2011, Jonathan Cameron wrote:
> -/**
> - * lis3l02dq_spi_read_reg_8() - read single byte from a single register
> - * @dev: device asosciated with child of actual device (iio_dev or iio_trig)
> - * @reg_address: the address of the register to be read
> - * @val: pass back the resulting value
> - **/
> -int lis3l02dq_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
> +static int __lis3l02dq_spi_read_reg_8(struct lis3l02dq_state *st,
> + u8 reg_address, u8 *val)
> {
> - int ret;
> struct spi_message msg;
> - struct iio_dev *indio_dev = dev_get_drvdata(dev);
> - struct iio_sw_ring_helper_state *h = iio_dev_get_devdata(indio_dev);
> - struct lis3l02dq_state *st = lis3l02dq_h_to_s(h);
> -
> + int ret;
> struct spi_transfer xfer = {
> .tx_buf = st->tx,
> .rx_buf = st->rx,
>
> @@ -73,6 +64,19 @@ int lis3l02dq_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
>
> return ret;
> }
> +/**
> + * lis3l02dq_spi_read_reg_8() - read single byte from a single register
> + * @dev: device asosciated with child of actual device (iio_dev or iio_trig)
> + * @reg_address: the address of the register to be read
> + * @val: pass back the resulting value
> + **/
> +int lis3l02dq_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct iio_sw_ring_helper_state *h = iio_dev_get_devdata(indio_dev);
> + struct lis3l02dq_state *st = lis3l02dq_h_to_s(h);
> + return __lis3l02dq_spi_read_reg_8(st, reg_address, val);
> +}
This change seems unrelated to the main intention of the driver.
I think the cleanest way to handle this would be to always pass the
iio_dev to the low-level functions. Passing a bare struct device
is hardly ever helpful, and from the iio_dev, you can easily get to
both the device and the lis3l02dq_state.
> +enum lis3l02dq_rm_ind {
> + LIS3L02DQ_ACCEL,
> + LIS3L02DQ_GAIN,
> + LIS3L02DQ_BIAS,
> +};
>
> +static u8 lis3l02dq_axis_map[3][3] = {
> + [LIS3L02DQ_ACCEL] = { LIS3L02DQ_REG_OUT_X_L_ADDR,
> + LIS3L02DQ_REG_OUT_Y_L_ADDR,
> + LIS3L02DQ_REG_OUT_Z_L_ADDR },
> + [LIS3L02DQ_GAIN] = { LIS3L02DQ_REG_GAIN_X_ADDR,
> + LIS3L02DQ_REG_GAIN_Y_ADDR,
> + LIS3L02DQ_REG_GAIN_Z_ADDR },
> + [LIS3L02DQ_BIAS] = { LIS3L02DQ_REG_OFFSET_X_ADDR,
> + LIS3L02DQ_REG_OFFSET_Y_ADDR,
> + LIS3L02DQ_REG_OFFSET_Z_ADDR }
> +};
The table looks nice and small, but I can't see where the BIAS
value is used. Is that something that the hardware supports but
the driver does not?
> +static int lis3l02dq_read_thresh(struct iio_dev *indio_dev,
> + int e,
> + int *val)
> {
> + struct iio_sw_ring_helper_state *h
> + = iio_dev_get_devdata(indio_dev);
> + struct lis3l02dq_state *st = lis3l02dq_h_to_s(h);
>
> + return lis3l02dq_read_16bit_s(st, LIS3L02DQ_REG_THS_L_ADDR, val);
> }
>
> +static int lis3l02dq_write_thresh(struct iio_dev *indio_dev,
> + int event_code,
> + int val)
> {
> + u16 value = val;
> + return lis3l02dq_spi_write_reg_s16(&indio_dev->dev,
> + LIS3L02DQ_REG_THS_L_ADDR,
> + value);
> }
Can be made even smaller if you drop the u16 value variable and
rely on the implicit type conversion ;-)
> +static int lis3l02dq_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec *chan,
> + int *val,
> + long mask)
> {
I guess that in the mask work, you can only pass one bit at most,
right? How about passing the bit number instead?
> +/* A shared handler for a number of threshold types */
> +IIO_EVENT_SH(threshold, &lis3l02dq_thresh_handler_th);
>
> +#define LIS3L02DQ_INFO_MASK \
> + (1 << IIO_CHAN_INFO_SCALE_SHARED) | \
> + (1 << IIO_CHAN_INFO_CALIBSCALE_SEPARATE) | \
> + (1 << IIO_CHAN_INFO_CALIBBIAS_SEPARATE)
> +
> +#define LIS3L02DQ_EVENT_MASK \
> + IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING) | \
> + IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING)
> +
> +static struct iio_chan_spec lis3l02dq_channels[] = {
> + IIO_CHAN_EV(IIO_ACCEL, 'x', LIS3L02DQ_INFO_MASK,
> + 0, 0, IIO_ST('s', 12, 16, 0),
> + LIS3L02DQ_EVENT_MASK, &iio_event_threshold),
> + IIO_CHAN_EV(IIO_ACCEL, 'y', LIS3L02DQ_INFO_MASK,
> + 1, 1, IIO_ST('s', 12, 16, 0),
> + LIS3L02DQ_EVENT_MASK, &iio_event_threshold),
> + IIO_CHAN(IIO_ACCEL, 'z', LIS3L02DQ_INFO_MASK,
> + 2, 2, IIO_ST('s', 12, 16, 0)),
> + IIO_CHAN_SOFT_TIMESTAMP(3)
> +};
Hmm, this has turned out more complex than I had hoped for,
but that's probably the way things go when you put them into
real code.
> +static ssize_t lis3l02dq_read_event_config(struct iio_dev *indio_dev,
> + int event_code)
> +{
> +
> + u8 val;
> + int ret;
> + u8 mask = (1 << (IIO_EVENT_CODE_EXTRACT_MODIFIER(event_code)*2 +
> + (IIO_EVENT_CODE_EXTRACT_DIR(event_code) ==
> + IIO_EV_DIR_RISING)));
> + ret = lis3l02dq_spi_read_reg_8(&indio_dev->dev,
> LIS3L02DQ_REG_WAKE_UP_CFG_ADDR,
> + &val);
> + if (ret < 0)
> + return ret;
> +
> + return !!(val & mask);
> +}
I need some background information here. Why do you expose the configuration
register directly? Is this something that users normally access?
I would assume that it's highly device specific and would need to be
abstracted by the kernel.
> @@ -813,7 +690,13 @@ static int __devinit lis3l02dq_probe(struct spi_device *spi)
>
> st->help.indio_dev->dev.parent = &spi->dev;
> st->help.indio_dev->num_interrupt_lines = 1;
> - st->help.indio_dev->event_attrs = &lis3l02dq_event_attribute_group;
> + st->help.indio_dev->channels = lis3l02dq_channels;
> + st->help.indio_dev->num_channels = ARRAY_SIZE(lis3l02dq_channels);
> + st->help.indio_dev->read_raw = &lis3l02dq_read_raw;
> + st->help.indio_dev->read_event_value = &lis3l02dq_read_thresh;
> + st->help.indio_dev->write_event_value = &lis3l02dq_write_thresh;
> + st->help.indio_dev->write_event_config = &lis3l02dq_write_event_config;
> + st->help.indio_dev->read_event_config = &lis3l02dq_read_event_config;
> st->help.indio_dev->attrs = &lis3l02dq_attribute_group;
> st->help.indio_dev->dev_data = (void *)(&st->help);
> st->help.indio_dev->driver_module = THIS_MODULE;
I think a lof of this can be turned from code into data if you define
a new structure for all the constant members:
static const struct iio_dev_ops = {
.owner = THIS_MODULE,
.attrs = &lis3l02dq_attribute_group,
.channels = lis3l02dq_channels,
...
};
st->help.indio_dev->ops = &iio_dev_ops;
st->help.indio_dev->dev_data = (void *)(&st->help);
=> somewhat more readable, and smaller object code.
Arnd
--
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
