Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

2017-12-05 Thread Jonathan Cameron 
On Mon, 4 Dec 2017 22:05:41 +
Mark Brown  wrote:

> On Sun, Dec 03, 2017 at 08:43:47PM +0100, Wolfram Sang wrote:
> 
> > > It's a bit different in that it's much more likely that a SPI controller
> > > will actually do DMA than an I2C one since the speeds are higher and
> > > there's frequent applications that do large transfers so it's more
> > > likely that people will do the right thing as issues would tend to come
> > > up if they don't.  
> 
> > Yes, for SPI this is true. I was thinking more of regmap with its
> > usage of different transport mechanisms. But I guess they should all be
> > DMA safe because some of them need to be DMA safe?  
> 
> Possibly.  Hopefully.  I guess we'll find out.
> 

Yeah, optimistic assumption. Plenty of drivers use regmap for the
convenience of it's caching and field access etc rather than
because they support multiple buses.

I'll audit the IIO drivers and see where we have issues if we
start assuming DMA safe for regmap (which makes sense to me).

Probably worth fixing them all up anyway and tends to be straightforward.

Jonathan

Re: [PATCH v6 6/9] i2c: smbus: use DMA safe buffers for emulated SMBus transactions

2017-11-10 Thread Jonathan Cameron 
On Sat, 4 Nov 2017 21:20:06 +0100
Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:

> For all block commands, try to allocate a DMA safe buffer and mark it
> accordingly. Only use the stack, if the buffers cannot be allocated.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Hmm. Interesting balance here as this adds allocations to paths where the i2c
master can't take advantage.  Not ideal, but perhaps not worth the hassle
of working around it?

It's only for the block calls I guess so not that major an issue.

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
> ---
>  drivers/i2c/i2c-core-smbus.c | 45 
> ++--
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 4bb9927afd0106..931c274fe26809 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define CREATE_TRACE_POINTS
>  #include 
> @@ -291,6 +292,22 @@ s32 i2c_smbus_write_i2c_block_data(const struct 
> i2c_client *client, u8 command,
>  }
>  EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
>  
> +static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
> +{
> + bool is_read = msg->flags & I2C_M_RD;
> + unsigned char *dma_buf;
> +
> + dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
> + if (!dma_buf)
> + return;
> +
> + msg->buf = dma_buf;
> + msg->flags |= I2C_M_DMA_SAFE;
> +
> + if (init_val)
> + msg->buf[0] = init_val;
> +
> +}
>  /* Simulate a SMBus command using the i2c protocol
> No checking of parameters is done!  */
>  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> @@ -368,6 +385,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   msg[1].flags |= I2C_M_RECV_LEN;
>   msg[1].len = 1; /* block length will be added by
>  the underlying bus driver */
> + i2c_smbus_try_get_dmabuf([1], 0);
>   } else {
>   msg[0].len = data->block[0] + 2;
>   if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
> @@ -376,8 +394,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   data->block[0]);
>   return -EINVAL;
>   }
> +
> + i2c_smbus_try_get_dmabuf([0], command);
>   for (i = 1; i < msg[0].len; i++)
> - msgbuf0[i] = data->block[i-1];
> + msg[0].buf[i] = data->block[i-1];
>   }
>   break;
>   case I2C_SMBUS_BLOCK_PROC_CALL:
> @@ -389,16 +409,21 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   data->block[0]);
>   return -EINVAL;
>   }
> +
>   msg[0].len = data->block[0] + 2;
> + i2c_smbus_try_get_dmabuf([0], command);
>   for (i = 1; i < msg[0].len; i++)
> - msgbuf0[i] = data->block[i-1];
> + msg[0].buf[i] = data->block[i-1];
> +
>   msg[1].flags |= I2C_M_RECV_LEN;
>   msg[1].len = 1; /* block length will be added by
>  the underlying bus driver */
> + i2c_smbus_try_get_dmabuf([1], 0);
>   break;
>   case I2C_SMBUS_I2C_BLOCK_DATA:
>   if (read_write == I2C_SMBUS_READ) {
>   msg[1].len = data->block[0];
> + i2c_smbus_try_get_dmabuf([1], 0);
>   } else {
>   msg[0].len = data->block[0] + 1;
>   if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 1) {
> @@ -407,8 +432,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   data->block[0]);
>   return -EINVAL;
>   }
> +
> + i2c_smbus_try_get_dmabuf([0], command);
>   for (i = 1; i <= data->block[0]; i++)
> - msgbuf0[i] = data->block[i];
> + msg[0].buf[i] = data->block[i];
>   }
>   break;
>   default:
> @@ -456,14 +483,20 @@ static s32

Re: [PATCH v6 5/9] i2c: add i2c_master_{send|recv}_dmasafe

2017-11-10 Thread Jonathan Cameron 
On Sat, 4 Nov 2017 21:20:05 +0100
Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:

> Use the new helper to create variants of i2c_master_{send|recv} which
> mark their buffers as DMA safe.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
Can't really argue with such a simple patch ;)
Acked-by: Jonathan Cameron <jonathan.came...@huawei.com>

> ---
>  include/linux/i2c.h | 31 +++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index ef1a8791c1ae24..8c144e3cbfb261 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -81,6 +81,22 @@ static inline int i2c_master_recv(const struct i2c_client 
> *client,
>  };
>  
>  /**
> + * i2c_master_recv_dmasafe - issue a single I2C message in master receive 
> mode
> + *using a DMA safe buffer
> + * @client: Handle to slave device
> + * @buf: Where to store data read from slave, must be safe to use with DMA
> + * @count: How many bytes to read, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes read.
> + */
> +static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
> +   char *buf, int count)
> +{
> + return i2c_transfer_buffer_flags(client, buf, count,
> +  I2C_M_RD | I2C_M_DMA_SAFE);
> +};
> +
> +/**
>   * i2c_master_send - issue a single I2C message in master transmit mode
>   * @client: Handle to slave device
>   * @buf: Data that will be written to the slave
> @@ -93,6 +109,21 @@ static inline int i2c_master_send(const struct i2c_client 
> *client,
>  {
>   return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
>  };
> +/**
> + * i2c_master_send_dmasafe - issue a single I2C message in master transmit 
> mode
> + *using a DMA safe buffer
> + * @client: Handle to slave device
> + * @buf: Data that will be written to the slave, must be safe to use with DMA
> + * @count: How many bytes to write, must be less than 64k since msg.len is 
> u16
> + *
> + * Returns negative errno, or else the number of bytes written.
> + */
> +static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
> +   const char *buf, int count)
> +{
> + return i2c_transfer_buffer_flags(client, (char *)buf, count,
> +  I2C_M_DMA_SAFE);
> +};
>  
>  /* Transfer num messages.
>   */

Re: [PATCH v6 4/9] i2c: refactor i2c_master_{send_recv}

2017-11-10 Thread Jonathan Cameron 
On Sat, 4 Nov 2017 21:20:04 +0100
Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:

> Those two functions are very similar, the only differences are that one
> needs the I2C_M_RD flag for its message while the other one needs the
> buffer casted to drop the const. Introduce a generic helper which
> allows to specify the flags (also needed later for DMA safe variants of
> these calls) and let the casting be done in the inlining fuctions which
> are now calling the new helper function.

The casting away of the const is a bit nasty, but short of making this
whole thing a macro, I can't see a better way.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
> ---
>  drivers/i2c/i2c-core-base.c | 64 
> +
>  include/linux/i2c.h | 34 +---
>  2 files changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index de1850bd440659..206c47c85c98c5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1972,63 +1972,35 @@ int i2c_transfer(struct i2c_adapter *adap, struct 
> i2c_msg *msgs, int num)
>  EXPORT_SYMBOL(i2c_transfer);
>  
>  /**
> - * i2c_master_send - issue a single I2C message in master transmit mode
> + * i2c_transfer_buffer_flags - issue a single I2C message transferring data
> + *  to/from a buffer
>   * @client: Handle to slave device
> - * @buf: Data that will be written to the slave
> - * @count: How many bytes to write, must be less than 64k since msg.len is 
> u16
> + * @buf: Where the data is stored
> + * @count: How many bytes to transfer, must be less than 64k since msg.len 
> is u16
> + * @flags: The flags to be used for the message, e.g. I2C_M_RD for reads
>   *
> - * Returns negative errno, or else the number of bytes written.
> + * Returns negative errno, or else the number of bytes transferred.
>   */
> -int i2c_master_send(const struct i2c_client *client, const char *buf, int 
> count)
> +int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
> +   int count, u16 flags)
>  {
>   int ret;
> - struct i2c_adapter *adap = client->adapter;
> - struct i2c_msg msg;
> -
> - msg.addr = client->addr;
> - msg.flags = client->flags & I2C_M_TEN;
> - msg.len = count;
> - msg.buf = (char *)buf;
> -
> - ret = i2c_transfer(adap, , 1);
> -
> - /*
> -  * If everything went ok (i.e. 1 msg transmitted), return #bytes
> -  * transmitted, else error code.
> -  */
> - return (ret == 1) ? count : ret;
> -}
> -EXPORT_SYMBOL(i2c_master_send);
> -
> -/**
> - * i2c_master_recv - issue a single I2C message in master receive mode
> - * @client: Handle to slave device
> - * @buf: Where to store data read from slave
> - * @count: How many bytes to read, must be less than 64k since msg.len is u16
> - *
> - * Returns negative errno, or else the number of bytes read.
> - */
> -int i2c_master_recv(const struct i2c_client *client, char *buf, int count)
> -{
> - struct i2c_adapter *adap = client->adapter;
> - struct i2c_msg msg;
> - int ret;
> -
> - msg.addr = client->addr;
> - msg.flags = client->flags & I2C_M_TEN;
> - msg.flags |= I2C_M_RD;
> - msg.len = count;
> - msg.buf = buf;
> + struct i2c_msg msg = {
> + .addr = client->addr,
> + .flags = flags | (client->flags & I2C_M_TEN),
> + .len = count,
> + .buf = buf,
> + };
>  
> - ret = i2c_transfer(adap, , 1);
> + ret = i2c_transfer(client->adapter, , 1);
>  
>   /*
> -  * If everything went ok (i.e. 1 msg received), return #bytes received,
> -  * else error code.
> +  * If everything went ok (i.e. 1 msg transferred), return #bytes
> +  * transferred, else error code.
>*/
>   return (ret == 1) ? count : ret;
>  }
> -EXPORT_SYMBOL(i2c_master_recv);
> +EXPORT_SYMBOL(i2c_transfer_buffer_flags);
>  
>  /* 
>   * the i2c address scanning function
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a0b57de91e21d3..ef1a8791c1ae24 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -63,10 +63,36 @@ struct property_entry;
>   * transmit an arbitrary number of messages without interruption.
>   * @count must be be less than 64k since msg.len is u16.
>   */
> -extern int i2c_master_send(const struct i2c_client *client, const char *buf

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

2017-09-21 Thread Jonathan Cameron 
On Thu, 21 Sep 2017 16:15:28 +0200
Wolfram Sang  wrote:

> > > > +/**
> > > > + * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync 
> > > > with i2c_msg
> > > > + * @msg: the message to be synced with
> > > > + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be 
> > > > NULL.
> > > > + */
> > > > +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
> > > > +{
> > > > +   if (!buf || buf == msg->buf)
> > > > +   return;
> > > > +
> > > > +   if (msg->flags & I2C_M_RD)
> > > > +   memcpy(msg->buf, buf, msg->len);
> > > > +
> > > > +   kfree(buf);  
> > 
> > Only free when you actually allocated it.  Seems to me like you need
> > to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.
> > 
> > Otherwise the logic to do this will be needed in every driver
> > which will get irritating fast.  
> 
> Well, I return early if (buf == msg->buf) which is only true for
> I2C_M_DMA_SAFE. If not, I allocated the buffer. Am I missing something?
> It would be very strange to call this function if the caller allocated
> the buffer manually.
> 
> Thanks for the review!

Doh missed that check and my comment was bonkers even if it hadn't been there.
I come back to the claim of insufficient caffeine.

You are quite correct.  Please ignore previous comment - the code is
fine as is. 

Jonathan
> 
>

Re: [RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

2017-09-21 Thread Jonathan Cameron 
On Wed, 20 Sep 2017 20:59:56 +0200
Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:

> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Makes sense as do the other drivers.

Feel free to add

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>

to all of them (though they hardly took a lot of reviewing given how simple
the patches were :)

> ---
>  drivers/i2c/i2c-dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 6f638bbc922db4..bbc7aadb4c899d 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client 
> *client,
>   res = PTR_ERR(rdwr_pa[i].buf);
>   break;
>   }
> + /* memdup_user allocates with GFP_KERNEL, so DMA is ok */
> + rdwr_pa[i].flags |= I2C_M_DMA_SAFE;
>  
>   /*
>* If the message length is received from the slave (similar

Re: [RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

2017-09-21 Thread Jonathan Cameron 
On Wed, 20 Sep 2017 16:56:48 -0300
Mauro Carvalho Chehab <mche...@s-opensource.com> wrote:

> Em Wed, 20 Sep 2017 20:59:53 +0200
> Wolfram Sang <wsa+rene...@sang-engineering.com> escreveu:
> 
> > Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>  
> 
> Documentation looks OK on my eyes. So:
> 
> Reviewed-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Really minor suggestion inline. I don't really care either way as
what you had is perfectly comprehensible. 

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>

> 
> > ---
> >  Documentation/i2c/DMA-considerations | 58 
> > 
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 Documentation/i2c/DMA-considerations
> > 
> > diff --git a/Documentation/i2c/DMA-considerations 
> > b/Documentation/i2c/DMA-considerations
> > new file mode 100644
> > index 00..5a63355c6a9b6f
> > --- /dev/null
> > +++ b/Documentation/i2c/DMA-considerations
> > @@ -0,0 +1,58 @@
> > +=
> > +Linux I2C and DMA
> > +=
> > +
> > +Given that I2C is a low-speed bus where largely small messages are 
> > transferred,

Slightly nicer as:

Given that i2c is a low-speed bus, over which the majority of messages 
transferred are small,

> > +it is not considered a prime user of DMA access. At this time of writing, 
> > only
> > +10% of I2C bus master drivers have DMA support implemented. And the vast
> > +majority of transactions are so small that setting up DMA for it will 
> > likely
> > +add more overhead than a plain PIO transfer.
> > +
> > +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA 
> > safe.
> > +It does not seem reasonable to apply additional burdens when the feature 
> > is so
> > +rarely used. However, it is recommended to use a DMA-safe buffer if your
> > +message size is likely applicable for DMA. Most drivers have this threshold
> > +around 8 bytes (as of today, this is mostly an educated guess, however). 
> > For
> > +any message of 16 byte or larger, it is probably a really good idea. Please
> > +note that other subsystems you use might add requirements. E.g., if your
> > +I2C bus master driver is using USB as a bridge, then you need to have DMA
> > +safe buffers always, because USB requires it.
> > +
> > +For clients, if you use a DMA safe buffer in i2c_msg, set the 
> > I2C_M_DMA_SAFE
> > +flag with it. Then, the I2C core and drivers know they can safely operate 
> > DMA
> > +on it. Note that using this flag is optional. I2C host drivers which are 
> > not
> > +updated to use this flag will work like before. And like before, they risk
> > +using an unsafe DMA buffer. To improve this situation, using 
> > I2C_M_DMA_SAFE in
> > +more and more clients and host drivers is the planned way forward. Note 
> > also
> > +that setting this flag makes only sense in kernel space. User space data is
> > +copied into kernel space anyhow. The I2C core makes sure the destination
> > +buffers in kernel space are always DMA capable.
> > +
> > +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers 
> > for i2c_smbus_xfer_emulated.
> > +
> > +Drivers wishing to implement safe DMA can use helper functions from the I2C
> > +core. One gives you a DMA-safe buffer for a given i2c_msg as long as a 
> > certain
> > +threshold is met::
> > +
> > +   dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
> > +
> > +If a buffer is returned, it is either msg->buf for the I2C_M_DMA_SAFE case 
> > or a
> > +bounce buffer. But you don't need to care about that detail, just use the
> > +returned buffer. If NULL is returned, the threshold was not met or a bounce
> > +buffer could not be allocated. Fall back to PIO in that case.
> > +
> > +In any case, a buffer obtained from above needs to be released. It ensures 
> > data
> > +is copied back to the message and a potentially used bounce buffer is 
> > freed::
> > +
> > +   i2c_release_dma_safe_msg_buf(msg, dma_buf);
> > +
> > +The bounce buffer handling from the core is generic and simple. It will 
> > always
> > +allocate a new bounce buffer. If you want a more sophisticated handling 
> > (e.g.
> > +reusing pre-allocated buffers), you are free to implement your own.
> > +
> > +Please also check the in-kernel documentation for details. The 
> > i2c-sh_mobile
> > +driver can be used as a reference example how to use the above helpers.
> > +
> > +Final note: If you plan to use DMA with I2C (or with anything else, 
> > actually)
> > +make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can 
> > help
> > +you find various issues which can be complex to debug otherwise.  
> 
> 
> 
> Thanks,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

2017-09-21 Thread Jonathan Cameron 
On Thu, 21 Sep 2017 14:59:22 +0100
Jonathan Cameron <jonathan.came...@huawei.com> wrote:

> On Wed, 20 Sep 2017 20:59:52 +0200
> Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:
> 
> > One helper checks if DMA is suitable and optionally creates a bounce
> > buffer, if not. The other function returns the bounce buffer and makes
> > sure the data is properly copied back to the message.
> > 
> > Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>  
> 
> One minor suggestion for wording. Otherwise looks good to me.
> 
> Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
>

Need more coffee. See below.
 
> > ---
> >  drivers/i2c/i2c-core-base.c | 45 
> > +
> >  include/linux/i2c.h |  3 +++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 56e46581b84bdb..925a22794d3ced 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -2241,6 +2241,51 @@ void i2c_put_adapter(struct i2c_adapter *adap)
> >  }
> >  EXPORT_SYMBOL(i2c_put_adapter);
> >  
> > +/**
> > + * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
> > + * @msg: the message to be checked
> > + * @threshold: the amount of byte from which using DMA makes sense  
> 
> the minimum number of bytes for which using DMA makes sense.
> 
> > + *
> > + * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with 
> > PIO.
> > + *
> > + *Or a valid pointer to be used with DMA. Note that it can either be
> > + *msg->buf or a bounce buffer. After use, release it by calling
> > + *i2c_release_dma_safe_msg_buf().
> > + *
> > + * This function must only be called from process context!
> > + */
> > +u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
> > +{
> > +   if (msg->len < threshold)
> > +   return NULL;
> > +
> > +   if (msg->flags & I2C_M_DMA_SAFE)
> > +   return msg->buf;
> > +
> > +   if (msg->flags & I2C_M_RD)
> > +   return kzalloc(msg->len, GFP_KERNEL);
> > +   else
> > +   return kmemdup(msg->buf, msg->len, GFP_KERNEL);
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
> > +
> > +/**
> > + * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with 
> > i2c_msg
> > + * @msg: the message to be synced with
> > + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
> > + */
> > +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
> > +{
> > +   if (!buf || buf == msg->buf)
> > +   return;
> > +
> > +   if (msg->flags & I2C_M_RD)
> > +   memcpy(msg->buf, buf, msg->len);
> > +
> > +   kfree(buf);

Only free when you actually allocated it.  Seems to me like you need
to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.

Otherwise the logic to do this will be needed in every driver
which will get irritating fast.


> > +}
> > +EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
> > +
> >  MODULE_AUTHOR("Simon G. Vogl <si...@tk.uni-linz.ac.at>");
> >  MODULE_DESCRIPTION("I2C-Bus main module");
> >  MODULE_LICENSE("GPL");
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index d501d3956f13f0..1e99342f180f45 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -767,6 +767,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct 
> > i2c_msg *msg)
> > return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
> >  }
> >  
> > +u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
> > +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
> > +
> >  int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short 
> > addr);
> >  /**
> >   * module_i2c_driver() - Helper macro for registering a modular I2C driver 
> >  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

2017-09-21 Thread Jonathan Cameron 
On Wed, 20 Sep 2017 20:59:52 +0200
Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:

> One helper checks if DMA is suitable and optionally creates a bounce
> buffer, if not. The other function returns the bounce buffer and makes
> sure the data is properly copied back to the message.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

One minor suggestion for wording. Otherwise looks good to me.

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>

> ---
>  drivers/i2c/i2c-core-base.c | 45 
> +
>  include/linux/i2c.h |  3 +++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 56e46581b84bdb..925a22794d3ced 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2241,6 +2241,51 @@ void i2c_put_adapter(struct i2c_adapter *adap)
>  }
>  EXPORT_SYMBOL(i2c_put_adapter);
>  
> +/**
> + * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
> + * @msg: the message to be checked
> + * @threshold: the amount of byte from which using DMA makes sense

the minimum number of bytes for which using DMA makes sense.

> + *
> + * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
> + *
> + *  Or a valid pointer to be used with DMA. Note that it can either be
> + *  msg->buf or a bounce buffer. After use, release it by calling
> + *  i2c_release_dma_safe_msg_buf().
> + *
> + * This function must only be called from process context!
> + */
> +u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
> +{
> + if (msg->len < threshold)
> + return NULL;
> +
> + if (msg->flags & I2C_M_DMA_SAFE)
> + return msg->buf;
> +
> + if (msg->flags & I2C_M_RD)
> + return kzalloc(msg->len, GFP_KERNEL);
> + else
> + return kmemdup(msg->buf, msg->len, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
> +
> +/**
> + * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with 
> i2c_msg
> + * @msg: the message to be synced with
> + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
> + */
> +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
> +{
> + if (!buf || buf == msg->buf)
> + return;
> +
> + if (msg->flags & I2C_M_RD)
> + memcpy(msg->buf, buf, msg->len);
> +
> + kfree(buf);
> +}
> +EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
> +
>  MODULE_AUTHOR("Simon G. Vogl <si...@tk.uni-linz.ac.at>");
>  MODULE_DESCRIPTION("I2C-Bus main module");
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index d501d3956f13f0..1e99342f180f45 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -767,6 +767,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct 
> i2c_msg *msg)
>   return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
>  }
>  
> +u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
> +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
> +
>  int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short 
> addr);
>  /**
>   * module_i2c_driver() - Helper macro for registering a modular I2C driver

Re: [RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it

2017-08-20 Thread Jonathan Cameron 
On Thu, 17 Aug 2017 16:14:43 +0200
Wolfram Sang  wrote:

> So, after revisiting old mail threads, taking part in a similar discussion on
> the USB list, and implementing a not-convincing solution before, here is what 
> I
> cooked up to document and ease DMA handling for I2C within Linux. Please have 
> a
> look at the documentation introduced in patch 3 for details.
> 
> While the previous versions tried to magically apply bounce buffers when
> needed, it became clear that detecting DMA safe buffers is too fragile. This
> approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
> outcome so far is very convincing IMO. The core additions are simple and easy
> to understand (makes me even think of inlining them again?). The driver 
> changes
> for the Renesas IP cores became easier to understand, too. While only a tad 
> for
> the i2c-sh_mobile driver, the situation became a LOT better for the i2c-rcar
> driver. No more DMA disabling for the whole transfer in case of unsafe 
> buffers,
> we are back to per-msg handling. And the code fix is now an easy to understand
> one line change. Yay!
> 
> Of course, we must now whitelist DMA safe buffers. An example for I2C_RDWR 
> case
> is in this series. It makes the i2ctransfer utility have DMA_SAFE buffers,
> which is nice for testing as i2cdump will (currently) not use DMA_SAFE 
> buffers.
> My plan is to add two new calls: i2c_master_{send|receive}_dma_safe which can
> be used if DMA_SAFE buffers are provided. So, drivers can simply switch to
> them. Also, the buffers used within i2c_smbus_xfer_emulated() need to be
> converted to be DMA_SAFE which will cover a huge bunch of use cases. The rest
> is then updating drivers which can be done when needed.
> 
> As these conversions are not done yet, this patch series has RFC status. But I
> already would like to get opinions on this approach, so I'll cc mailing lists
> of the heavier I2C users. Please let me know what you think.
> 
> All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W).
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
> renesas/topic/i2c-core-dma-rfc-v4
> 
> And big kudos to Renesas Electronics for funding this work, thank you very 
> much!

All looks good to me.  I should really set up some testing for this to see if it
gains us much on the various platforms I use, but that will 'a while' so don't 
wait
on me!  This is particularly true as none of them have dma support in the master
drivers yet.

Thanks for you work on this and cool than Renesas are funding it!

Jonathan

> 
> Regards,
> 
>Wolfram
> 
> Changes since v3:
>   * completely redesigned
> 
> Wolfram Sang (6):
>   i2c: add a message flag for DMA safe buffers
>   i2c: add helpers to ease DMA handling
>   i2c: add docs to clarify DMA handling
>   i2c: sh_mobile: use helper to decide if DMA is useful
>   i2c: rcar: skip DMA if buffer is not safe
>   i2c: dev: mark RDWR buffers as DMA_SAFE
> 
>  Documentation/i2c/DMA-considerations | 50 
> 
>  drivers/i2c/busses/i2c-rcar.c|  2 +-
>  drivers/i2c/busses/i2c-sh_mobile.c   |  8 --
>  drivers/i2c/i2c-core-base.c  | 45 
>  drivers/i2c/i2c-dev.c|  2 ++
>  include/linux/i2c.h  |  3 +++
>  include/uapi/linux/i2c.h |  3 +++
>  7 files changed, 110 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/i2c/DMA-considerations
>

Re: [PATCH v3 2/4] i2c: add docs to clarify DMA handling

2017-07-23 Thread Jonathan Cameron 
On Tue, 18 Jul 2017 12:23:37 +0200
Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:

> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
Is this material not perhaps better placed in the sphinx docs?
Up to you of course as your subsystem ;)

Text is good though.

Acked-by: Jonathan Cameron <jonathan.came...@huawei.com>
> ---
> Changes since v2:
> 
> * documentation updates. Hopefully better wording now
> 
>  Documentation/i2c/DMA-considerations | 38 
> 
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/i2c/DMA-considerations
> 
> diff --git a/Documentation/i2c/DMA-considerations 
> b/Documentation/i2c/DMA-considerations
> new file mode 100644
> index 00..e46c24d65c8556
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,38 @@
> +Linux I2C and DMA
> +-
> +
> +Given that I2C is a low-speed bus where largely small messages are 
> transferred,
> +it is not considered a prime user of DMA access. At this time of writing, 
> only
> +10% of I2C bus master drivers have DMA support implemented. And the vast
> +majority of transactions are so small that setting up DMA for it will likely
> +add more overhead than a plain PIO transfer.
> +
> +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA 
> safe.
> +It does not seem reasonable to apply additional burdens when the feature is 
> so
> +rarely used. However, it is recommended to use a DMA-safe buffer if your
> +message size is likely applicable for DMA. Most drivers have this threshold
> +around 8 bytes. As of today, this is mostly an educated guess, however.
> +
> +To support this scenario, drivers wishing to implement DMA can use helper
> +functions from the I2C core. One checks if a message is DMA capable in terms 
> of
> +size and memory type. It can optionally also create a bounce buffer:
> +
> + i2c_check_msg_for_dma(msg, threshold, _buf);
> +
> +The bounce buffer handling from the core is generic and simple. It will 
> always
> +allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
> +reusing pre-allocated buffers), you can leave the pointer to the bounce 
> buffer
> +empty and implement your own handling based on the return value of the above
> +function.
> +
> +The other helper function releases the bounce buffer. It ensures data is 
> copied
> +back to the message:
> +
> + i2c_release_dma_bounce_buf(msg, bounce_buf);
> +
> +Please check the in-kernel documentation for details. The i2c-sh_mobile 
> driver
> +can be used as a reference example.
> +
> +If you plan to use DMA with I2C (or with any other bus, actually) make sure 
> you
> +have CONFIG_DMA_API_DEBUG enabled during development. It can help you find
> +various issues which can be complex to debug otherwise.

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

2017-07-23 Thread Jonathan Cameron 
On Tue, 18 Jul 2017 12:23:36 +0200
Wolfram Sang  wrote:

> One helper checks if DMA is suitable and optionally creates a bounce
> buffer, if not. The other function returns the bounce buffer and makes
> sure the data is properly copied back to the message.
> 
> Signed-off-by: Wolfram Sang 
My knowledge of the exact requirements for dma on all platforms isn't
all that good.  Mostly it's based around the assumption that if one
forces a heap allocation and makes sure nothing else is in the
cacheline all is good.

I like the basic idea of this patch set a lot btw!

Jonathan
> ---
> Changes since v2:
> 
> * rebased to v4.13-rc1
> * helper functions are not inlined anymore but moved to i2c core
> * __must_check has been added to the buffer check helper
> * the release function has been renamed to contain 'dma' as well
> 
>  drivers/i2c/i2c-core-base.c | 68 
> +
>  include/linux/i2c.h |  5 
>  2 files changed, 73 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index c89dac7fd2e7b7..7326a9d2e4eb69 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -44,6 +45,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "i2c-core.h"
> @@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap)
>  }
>  EXPORT_SYMBOL(i2c_put_adapter);
>  
> +/**
> + * i2c_check_msg_for_dma() - check if a message is suitable for DMA
> + * @msg: the message to be checked
> + * @threshold: the amount of byte from which using DMA makes sense
> + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
> + *   ptr, if needed. The bounce buffer must be freed by the
> + *   caller using i2c_release_dma_bounce_buf().
> + *
> + * Return: -ERANGE if message is smaller than threshold
> + *  -EFAULT if message buffer is not DMA capable and no bounce buffer
> + *  was requested
> + *  -ENOMEM if a bounce buffer could not be created
> + *  0 if message is suitable for DMA
> + *
> + * The return value must be checked.
> + *
> + * Note: This function should only be called from process context! It uses
> + * helper functions which work on the 'current' task.
> + */
> +int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> + u8 **ptr_for_bounce_buf)
> +{
> + if (ptr_for_bounce_buf)
> + *ptr_for_bounce_buf = NULL;
> +
> + if (msg->len < threshold)
> + return -ERANGE;
> +
> + if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
> + pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr,
> +  ptr_for_bounce_buf ? ", trying bounce buffer" : "");
> + if (ptr_for_bounce_buf) {
> + if (msg->flags & I2C_M_RD)
> + *ptr_for_bounce_buf = kzalloc(msg->len, 
> GFP_KERNEL);
> + else
> + *ptr_for_bounce_buf = kmemdup(msg->buf, 
> msg->len,
> +   GFP_KERNEL);
> + if (!*ptr_for_bounce_buf)
> + return -ENOMEM;
> + } else {
> + return -EFAULT;
> + }
> + }
I'm trying to get my head around whether this is a sufficient set of conditions
for a dma safe buffer and I'm not sure it is.

We go to a lot of effort with SPI buffers to ensure they are in their own cache
lines.  Do we not need to do the same here?  The issue would be the
classic case of embedding a buffer inside a larger structure which is on
the stack.  Need the magic of __cacheline_aligned to force it into it's
own line for devices with dma-incoherent caches.
DMA-API-HOWTO.txt (not read that for a while at it is a rather good
at describing this stuff these days!)

So that one is easy enough to check by checking if it is cache line aligned,
but what do you do to know there is nothing much after it?

Not sure there is a way around this short of auditing i2c drivers to
change any that do this.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma);
> +
> +/**
> + * i2c_release_bounce_buf - copy data back from bounce buffer and release it
> + * @msg: the message to be copied back to
> + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
> + *   May be NULL.
> + */
> +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
> +{
> + if (!bounce_buf)
> + return;
> +
> + if (msg->flags & I2C_M_RD)
> + memcpy(msg->buf, bounce_buf, msg->len);
> +
> + kfree(bounce_buf);
> +}
> +EXPORT_SYMBOL_GPL(i2c_release_bounce_buf);
> +

Re: [PATCH 10/14] staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read

2017-07-15 Thread Jonathan Cameron 
On Fri, 14 Jul 2017 11:31:03 +0200
Arnd Bergmann  wrote:

> gcc-7 points out an older regression:
> 
> drivers/staging/iio/resolver/ad2s1210.c: In function 'ad2s1210_read_raw':
> drivers/staging/iio/resolver/ad2s1210.c:515:42: error: '<<' in boolean 
> context, did you mean '<' ? [-Werror=int-in-bool-context]
> 
> The original code had 'unsigned short' here, but incorrectly got
> converted to 'bool'. This reverts the regression and uses a normal
> type instead.
> 
> Fixes: 29148543c521 ("staging:iio:resolver:ad2s1210 minimal chan spec 
> conversion.")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Arnd Bergmann 
Thanks Arnd,

Applied to the fixes-togreg branch of iio.git.

Jonathan
> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c 
> b/drivers/staging/iio/resolver/ad2s1210.c
> index a6a8393d6664..3e00df74b18c 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -472,7 +472,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
>long m)
>  {
>   struct ad2s1210_state *st = iio_priv(indio_dev);
> - bool negative;
> + u16 negative;
>   int ret = 0;
>   u16 pos;
>   s16 vel;

Re: [PATCH 8/9] iio: gyro: mpu3050: stop double error reporting

2017-04-03 Thread Jonathan Cameron 
On 03/04/17 18:56, Linus Walleij wrote:
> On Mon, Apr 3, 2017 at 10:38 AM, Peter Rosin  wrote:
> 
>> i2c_mux_add_adapter already logs a message on failure.
>>
>> Signed-off-by: Peter Rosin 
> 
> Reviewed-by: Linus Walleij 
Applied to the togreg branch of iio.git.

Can't see any reason not to split this set up and apply through
the various trees so I've picked this one up.

Good little series.  We should do more of these in general!

Jonathan
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2 00/11] getting back -Wmaybe-uninitialized

2016-11-12 Thread Jonathan Cameron 
On 11/11/16 19:49, Arnd Bergmann wrote:
> On Friday, November 11, 2016 9:13:00 AM CET Linus Torvalds wrote:
>> On Thu, Nov 10, 2016 at 8:44 AM, Arnd Bergmann  wrote:
>>>
>>> Please merge these directly if you are happy with the result.
>>
>> I will take this.
> 
> Thanks a lot!
>  
>> I do see two warnings, but they both seem to be valid and recent,
>> though, so I have no issues with the spurious cases.
> 
> Ok, both of them should have my fixes coming your way already.
> 
>> Warning #1:
>>
>>   sound/soc/qcom/lpass-platform.c: In function ‘lpass_platform_pcmops_open’:
>>   sound/soc/qcom/lpass-platform.c:83:29: warning: ‘dma_ch’ may be used
>> uninitialized in this function [-Wmaybe-uninitialized]
>> drvdata->substream[dma_ch] = substream;
>> ~~~^~~
>>
>> and 'dma_ch' usage there really is crazy and wrong. Broken by
>> 022d00ee0b55 ("ASoC: lpass-platform: Fix broken pcm data usage")
> 
> Right, the patches crossed here, the bugfix patch that introduced
> this came into linux-next over the kernel summit, and the fix I
> sent on Tuesday made it into Mark Brown's tree on Wednesday but not
> before you pulled alsa tree. It should be fixed the next time you
> pull from the alsa tree, the commit is
> 
> 3b89e4b77ef9 ("ASoC: lpass-platform: initialize dma channel number")
>  
>> Warning #2 is not a real bug, but it's reasonable that gcc doesn't
>> know that storage_bytes (chip->read_size) has to be 2/4. Again,
>> introduced recently by commit 231147ee77f3 ("iio: maxim_thermocouple:
>> Align 16 bit big endian value of raw reads"), so you didn't see it.
> 
> This is the one I mentioned in the commit message as one that
> is fixed in linux-next and that should make it in soon.
> 
>>   drivers/iio/temperature/maxim_thermocouple.c: In function
>> ‘maxim_thermocouple_read_raw’:
>>   drivers/iio/temperature/maxim_thermocouple.c:141:5: warning: ‘ret’
>> may be used uninitialized in this function [-Wmaybe-uninitialized]
>> if (ret)
>>^
>>   drivers/iio/temperature/maxim_thermocouple.c:128:6: note: ‘ret’ was
>> declared here
>> int ret;
>> ^~~
>>
>> and I guess that code can just initialize 'ret' to '-EINVAL' or
>> something to just make the theoretical "somehow we had a wrong
>> chip->read_size" case error out cleanly.
> 
> Right, that was my conclusion too. I sent the bugfix on Oct 25
> for linux-next but it didn't make it in until this Monday, after
> you pulled the patch that introduced it on Oct 29.
> 
> The commit in staging-testing is
> 32cb7d27e65d ("iio: maxim_thermocouple: detect invalid storage size in 
> read()")
> 
> Greg and Jonathan, I see now that this is part of the 'iio-for-4.10b'
> branch, so I suspect you were not planning to send this before the
> merge window. Could you make sure this ends up in v4.9 so we get
> a clean build when -Wmaybe-uninitialized gets enabled again?
I'll queue this up and send a pull to Greg tomorrow.

Was highly doubtful that a false warning suppression (be it an
understandable one) was worth sending mid cycle, hence it was
taking the slow route.

Jonathan
> 
>   Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core

2016-04-10 Thread Jonathan Cameron 
On 03/04/16 12:51, Peter Rosin wrote:
> On 2016-04-03 12:51, Jonathan Cameron wrote:
>> On 03/04/16 09:52, Peter Rosin wrote:
>>> From: Peter Rosin <p...@axentia.se>
>>>
>>> Allocate an explicit i2c mux core to handle parent and child adapters
>>> etc. Update the select/deselect ops to be in terms of the i2c mux core
>>> instead of the child adapter.
>>>
>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> I'm mostly fine with this (though one unrelated change seems to have snuck
>> in).  However, I'm not set up to test it - hence other than fixing the change
>> you can have my ack, but ideal would be a tested by from someone with
>> relevant hardware...  However, it looks to be a fairly mechanical change so
>> if no one is currently setup to test it, then don't let it hold up the
>> series too long!
>>
>> Acked-by: Jonathan Cameron <ji...@kernel.org>
> 
> Thanks for your acks!
> 
>> Jonathan
>>> ---
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c |  2 +-
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  1 -
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  | 32 
>>> +-
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  3 ++-
>>>  4 files changed, 17 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
>>> index 2771106fd650..f62b8bd9ad7e 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
>>> @@ -183,7 +183,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client 
>>> *client)
>>> } else
>>> return 0; /* no secondary addr, which is OK */
>>> }
>>> -   st->mux_client = i2c_new_device(st->mux_adapter, );
>>> +   st->mux_client = i2c_new_device(st->muxc->adapter[0], );
>>> if (!st->mux_client)
>>> return -ENODEV;
>>> }
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> index d192953e9a38..0c2bded2b5b7 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> @@ -23,7 +23,6 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> -#include 
>>>  #include 
>>>  #include "inv_mpu_iio.h"
>>>  
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> index f581256d9d4c..0d429d788106 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> @@ -15,7 +15,6 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> -#include 
>>>  #include 
>>>  #include 
>>>  #include "inv_mpu_iio.h"
>>> @@ -52,10 +51,9 @@ static int inv_mpu6050_write_reg_unlocked(struct 
>>> i2c_client *client,
>>> return 0;
>>>  }
>>>  
>>> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void 
>>> *mux_priv,
>>> -u32 chan_id)
>>> +static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 
>>> chan_id)
>>>  {
>>> -   struct i2c_client *client = mux_priv;
>>> +   struct i2c_client *client = i2c_mux_priv(muxc);
>>> struct iio_dev *indio_dev = dev_get_drvdata(>dev);
> 
> Here, the existing code uses drv_get_drvdata to get from i2c_client to 
> iio_dev...
> 
>>> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>> int ret = 0;
>>> @@ -84,10 +82,9 @@ write_error:
>>> return ret;
>>>  }
>>>  
>>> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
>>> -  void *mux_priv, u32 chan_id)
>>> +static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 
>>> chan_id)
>>>  {
>>> -   struct i2c_client *client = mux_priv;
>>> +   struct i2c_client *client = i2c_mux_priv(muxc);
>>> struct iio_dev *indio_dev = dev_get_drvdata(>dev);
> 
> ...and here too...
> 
>>> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>  
>>> @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client

Re: [PATCH v6 19/24] i2c-mux: document i2c muxes and elaborate on parent-/mux-locked muxes

2016-04-03 Thread Jonathan Cameron 
On 03/04/16 09:52, Peter Rosin wrote:
> From: Peter Rosin 
> 
> Signed-off-by: Peter Rosin 
Very nice, one typo that I could see.

> ---
>  Documentation/i2c/i2c-topology | 370 
> +
>  MAINTAINERS|   1 +
>  2 files changed, 371 insertions(+)
>  create mode 100644 Documentation/i2c/i2c-topology
> 
> diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
> new file mode 100644
> index ..7a10edd0874f
> --- /dev/null
> +++ b/Documentation/i2c/i2c-topology
> @@ -0,0 +1,370 @@
> +I2C topology
> +
> +
> +There are a couple of reasons for building more complex i2c topologies
> +than a straight-forward i2c bus with one adapter and one or more devices.
> +
> +1. A mux may be needed on the bus to prevent address collisions.
> +
> +2. The bus may be accessible from some external bus master, and arbitration
> +   may be needed to determine if it is ok to access the bus.
> +
> +3. A device (particularly RF tuners) may want to avoid the digital noise
> +   from the i2c bus, at least most of the time, and sits behind a gate
> +   that has to be operated before the device can be accessed.
> +
> +Etc
> +
> +These constructs are represented as i2c adapter trees by Linux, where
> +each adapter has a parent adapter (except the root adapter) and zero or
> +more child adapters. The root adapter is the actual adapter that issues
> +i2c transfers, and all adapters with a parent are part of an "i2c-mux"
> +object (quoted, since it can also be an arbitrator or a gate).
> +
> +Depending of the particular mux driver, something happens when there is
> +an i2c transfer on one of its child adapters. The mux driver can
> +obviously operate a mux, but it can also do arbitration with an external
> +bus master or open a gate. The mux driver has two operations for this,
> +select and deselect. select is called before the transfer and (the
> +optional) deselect is called after the transfer.
> +
> +
> +Locking
> +===
> +
> +There are two variants of locking available to i2c muxes, they can be
> +mux-locked or parent-locked muxes. As is evident from below, it can be
> +useful to know if a mux is mux-locked or if it is parent-locked. The
> +following list was correct at the time of writing:
> +
> +In drivers/i2c/muxes/
> +i2c-arb-gpio-challengeParent-locked
> +i2c-mux-gpio  Normally parent-locked, mux-locked iff
> +  all involved gpio pins are controlled by the
> +  same i2c root adapter that they mux.
> +i2c-mux-pca9541   Parent-locked
> +i2c-mux-pca954x   Parent-locked
> +i2c-mux-pinctrl   Normally parent-locked, mux-locked iff
> +  all involved pinctrl devices are controlled
> +  by the same i2c root adapter that they mux.
> +i2c-mux-reg   Parent-locked
> +
> +In drivers/iio/
> +imu/inv_mpu6050/  Parent-locked
> +
> +In drivers/media/
> +dvb-frontends/m88ds3103   Parent-locked
> +dvb-frontends/rtl2830 Parent-locked
> +dvb-frontends/rtl2832 Parent-locked
> +dvb-frontends/si2168  Parent-locked
> +usb/cx231xx/  Parent-locked
> +
> +
> +Mux-locked muxes
> +
> +
> +Mux-locked muxes does not lock the entire parent adapter during the
> +full select-transfer-deselect transaction, only the muxes on the parent
> +adapter are locked. Mux-locked muxes are mostly interesting if the
> +select and/or deselect operations must use i2c transfers to complete
> +their tasks. Since the parent adapter is not fully locked during the
> +full transaction, unrelated i2c transfers may interleave the different
> +stages of the transaction. This has the benefit that the mux driver
> +may be easier and cleaner to implement, but it has some caveats.
> +
> +ML1. If you build a topology with a mux-locked mux being the parent
> + of a parent-locked mux, this might break the expectation from the
> + parent-locked mux that the root adapter is locked during the
> + transaction.
> +
> +ML2. It is not safe to build arbitrary topologies with two (or more)
> + mux-locked muxes that are not siblings, when there are address
> + collisions between the devices on the child adapters of these
> + non-sibling muxes.
> +
> + I.e. the select-transfer-deselect transaction targeting e.g. device
> + address 0x42 behind mux-one may be interleaved with a similar
> + operation targeting device address 0x42 behind mux-two. The
> + intension with such a topology would in this hypothetical example
> + be that mux-one and mux-two should not be selected simultaneously,
> + but mux-locked muxes do not guarantee that in all topologies.
> +
> +ML3. A mux-locked mux cannot be used by a driver for auto-closing
> + gates/muxes, i.e. something that closes automatically after a given
> + number (one, in most cases) of i2c

Re: [PATCH v6 20/24] iio: imu: inv_mpu6050: change the i2c gate to be mux-locked

2016-04-03 Thread Jonathan Cameron 
On 03/04/16 09:52, Peter Rosin wrote:
> From: Peter Rosin <p...@axentia.se>
> 
> The root i2c adapter lock is then no longer held by the i2c mux during
> accesses behind the i2c gate, and such accesses need to take that lock
> just like any other ordinary i2c accesses do.
> 
> So, declare the i2c gate mux-locked, and zap the code that makes the
> unlocked i2c accesses and just use ordinary regmap_write accesses.
> 
> This also happens to fix the deadlock described in
> http://patchwork.ozlabs.org/patch/584776/ authored by
> Adriana Reus <adriana.r...@intel.com> and submitted by
> Daniel Baluta <daniel.bal...@intel.com>
> 
> --8<--
> iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock
> 
> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
> I2C (in my setup it's an ak8975) are working at the same time.
> 
> Scenario:
> 
>   T1  T2
>  
> inv_mpu6050_read_fifo  aux sensor op (eg. ak8975_read_raw)
> | |
> mutex_lock(_dev->mlock)   i2c_transfer
> | |
> i2c transaction i2c adapter lock
> | |
> i2c adapter locki2c_mux_master_xfer
>   |
> inv_mpu6050_select_bypass
>   |
> mutex_lock(_dev->mlock)
> 
> When we operate on an mpu sensor the order of locking is mpu lock
> followed by the i2c adapter lock. However, when we operate the auxiliary
> sensor the order of locking is the other way around.
> 
> ...
> --8<--
> 
> The reason this patch fixes the deadlock is that T2 does not grab the
> i2c adapter lock until the very end (and grabs the newfangled i2c mux
> lock where it previously grabbed the i2c adapter lock).
> 
> Signed-off-by: Peter Rosin <p...@axentia.se>
This one obviously wants a ack from Adriana or Daniel in addition to mine.
I'm more than happy for these to go through the i2c tree btw.

Acked-by: Jonathan Cameron <ji...@kernel.org>
> ---
>  Documentation/i2c/i2c-topology|  2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 56 
> +++
>  2 files changed, 13 insertions(+), 45 deletions(-)
> 
> diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
> index 7a10edd0874f..346623a80bd1 100644
> --- a/Documentation/i2c/i2c-topology
> +++ b/Documentation/i2c/i2c-topology
> @@ -50,7 +50,7 @@ i2c-mux-pinctrl   Normally parent-locked, 
> mux-locked iff
>  i2c-mux-reg   Parent-locked
>  
>  In drivers/iio/
> -imu/inv_mpu6050/  Parent-locked
> +imu/inv_mpu6050/  Mux-locked
>  
>  In drivers/media/
>  dvb-frontends/m88ds3103   Parent-locked
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 0d429d788106..71ad31a275c9 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -24,45 +24,16 @@ static const struct regmap_config inv_mpu_regmap_config = 
> {
>   .val_bits = 8,
>  };
>  
> -/*
> - * The i2c read/write needs to happen in unlocked mode. As the parent
> - * adapter is common. If we use locked versions, it will fail as
> - * the mux adapter will lock the parent i2c adapter, while calling
> - * select/deselect functions.
> - */
> -static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
> -   u8 reg, u8 d)
> -{
> - int ret;
> - u8 buf[2] = {reg, d};
> - struct i2c_msg msg[1] = {
> - {
> - .addr = client->addr,
> - .flags = 0,
> - .len = sizeof(buf),
> - .buf = buf,
> - }
> - };
> -
> - ret = __i2c_transfer(client->adapter, msg, 1);
> - if (ret != 1)
> - return ret;
> -
> - return 0;
> -}
> -
>  static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>  {
> - struct i2c_client *client = i2c_mux_priv(muxc);
> - struct iio_dev *indio_dev = dev_get_drvdata(>dev);
> + struct iio_dev *indio_dev = i2c_mux_priv(muxc);
>   struct inv_mpu6050_state *st = iio_priv(indio_dev);
>   int ret = 0;
>  
>   /* Use the same mutex which was used everywhere to prot

Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core

2016-04-03 Thread Jonathan Cameron 
On 03/04/16 09:52, Peter Rosin wrote:
> From: Peter Rosin <p...@axentia.se>
> 
> Allocate an explicit i2c mux core to handle parent and child adapters
> etc. Update the select/deselect ops to be in terms of the i2c mux core
> instead of the child adapter.
> 
> Signed-off-by: Peter Rosin <p...@axentia.se>
I'm mostly fine with this (though one unrelated change seems to have snuck
in).  However, I'm not set up to test it - hence other than fixing the change
you can have my ack, but ideal would be a tested by from someone with
relevant hardware...  However, it looks to be a fairly mechanical change so
if no one is currently setup to test it, then don't let it hold up the
series too long!

Acked-by: Jonathan Cameron <ji...@kernel.org>

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c |  2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  1 -
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  | 32 
> +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  3 ++-
>  4 files changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> index 2771106fd650..f62b8bd9ad7e 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> @@ -183,7 +183,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client 
> *client)
>   } else
>   return 0; /* no secondary addr, which is OK */
>   }
> - st->mux_client = i2c_new_device(st->mux_adapter, );
> + st->mux_client = i2c_new_device(st->muxc->adapter[0], );
>   if (!st->mux_client)
>   return -ENODEV;
>   }
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index d192953e9a38..0c2bded2b5b7 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -23,7 +23,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include "inv_mpu_iio.h"
>  
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index f581256d9d4c..0d429d788106 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -15,7 +15,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include "inv_mpu_iio.h"
> @@ -52,10 +51,9 @@ static int inv_mpu6050_write_reg_unlocked(struct 
> i2c_client *client,
>   return 0;
>  }
>  
> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void 
> *mux_priv,
> -  u32 chan_id)
> +static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>  {
> - struct i2c_client *client = mux_priv;
> + struct i2c_client *client = i2c_mux_priv(muxc);
>   struct iio_dev *indio_dev = dev_get_drvdata(>dev);
>   struct inv_mpu6050_state *st = iio_priv(indio_dev);
>   int ret = 0;
> @@ -84,10 +82,9 @@ write_error:
>   return ret;
>  }
>  
> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
> -void *mux_priv, u32 chan_id)
> +static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 
> chan_id)
>  {
> - struct i2c_client *client = mux_priv;
> + struct i2c_client *client = i2c_mux_priv(muxc);
>   struct iio_dev *indio_dev = dev_get_drvdata(>dev);
>   struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  
> @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client *client,
>   return result;
>  
>   st = iio_priv(dev_get_drvdata(>dev));
> - st->mux_adapter = i2c_add_mux_adapter(client->adapter,
> -   >dev,
> -   client,
> -   0, 0, 0,
> -   inv_mpu6050_select_bypass,
> -   inv_mpu6050_deselect_bypass);
> - if (!st->mux_adapter) {
> - result = -ENODEV;
> + st->muxc = i2c_mux_one_adapter(client->adapter, >dev, 0, 0,
> +0, 0, 0,
> +inv_mpu6050_select_bypass,
> +inv_mpu6050_deselect_bypass);
> + if (IS_ERR(st->muxc)) {
> + result = PTR_ERR(st->muxc);
>   goto out_unreg_device;
>   }
> + st->muxc-&

Re: [PATCH v3 0/8] i2c mux cleanup and locking update

2016-03-05 Thread Jonathan Cameron 
On 02/03/16 17:29, Wolfram Sang wrote:
> On Fri, Jan 08, 2016 at 04:04:48PM +0100, Peter Rosin wrote:
>> From: Peter Rosin 
>>
>> Hi!
>>
>> [doing a v3 even if there is no "big picture" feedback yet, but
>>  previous versions has bugs that make them harder to test than
>>  needed, and testing is very much desired]
>>
>> I have a pair of boards with this i2c topology:
>>
>>GPIO ---|  -- BAT1
>> |  v /
>>I2C  -+--B---+ MUX
>>  |   \
>>EEPROM -- BAT2
>>
>>  (B denotes the boundary between the boards)
>>
>> The problem with this is that the GPIO controller sits on the same i2c bus
>> that it MUXes. For pca954x devices this is worked around by using unlocked
>> transfers when updating the MUX. I have no such luck as the GPIO is a general
>> purpose IO expander and the MUX is just a random bidirectional MUX, unaware
>> of the fact that it is muxing an i2c bus, and extending unlocked transfers
>> into the GPIO subsystem is too ugly to even think about. But the general hw
>> approach is sane in my opinion, with the number of connections between the
>> two boards minimized. To put is plainly, I need support for it.
>>
>> So, I observe that while it is needed to have the i2c bus locked during the
>> actual MUX update in order to avoid random garbage on the slave side, it
>> is not strictly a must to have it locked over the whole sequence of a full
>> select-transfer-deselect operation. The MUX itself needs to be locked, so
>> transfers to clients behind the mux are serialized, and the MUX needs to be
>> stable during all i2c traffic (otherwise individual mux slave segments
>> might see garbage).
>>
>> This series accomplishes this by adding code to i2c-mux-gpio and
>> i2c-mux-pinctrl that determines if all involved devices used to update the
>> mux are controlled by the same root i2c adapter that is muxed. When this
>> is the case, the select-transfer-deselect operations should be locked
>> individually to avoid the deadlock. The i2c bus *is* still locked
>> during muxing, since the muxing happens as part of i2c transfers. This
>> is true even if the MUX is updated with several transfers to the GPIO (at
>> least as long as *all* MUX changes are using the i2s master bus). A lock
>> is added to the mux so that transfers through the mux are serialized.
>>
>> Concerns:
>> - The locking is perhaps too complex?
>> - I worry about the priority inheritance aspect of the adapter lock. When
>>   the transfers behind the mux are divided into select-transfer-deselect all
>>   locked individually, low priority transfers get more chances to interfere
>>   with high priority transfers.
>> - When doing an i2c_transfer() in_atomic() context or with irqs_disabled(),
>>   there is a higher possibility that the mux is not returned to its idle
>>   state after a failed (-EAGAIN) transfer due to trylock.
>> - Is the detection of i2c-controlled gpios and pinctrls sane (i.e. the
>>   usage of the new i2c_root_adapter() function in 8/8)?
>>
>> To summarize the series, there's some i2c-mux infrastructure cleanup work
>> first (I think that part stands by itself as desireable regardless), the
>> locking changes are in the last three patches of the series, with the real
>> meat in 8/8.
>>
>> PS. needs a bunch of testing, I do not have access to all the involved hw
> 
> I want to let you know that I am currently thinking about this series.
> 
> There seems to be a second occasion where it could have helped AFAICT.
> http://patchwork.ozlabs.org/patch/584776/ (check my comments there from
> yesterday and today)
> 
> First of all, really thank you that you tried to find the proper
> solution and went all the way for it. It is easy to do a fire
> hack here, but you didn't.
> 
> I hope you understand, though, that I need to make a balance between
> features and complexity in my subsystem to have maintainable and stable
> code.
> 
> As I wrote in the mentioned thread already: "However, I am still
> undecided if that series should go upstream because it makes the mux
> code another magnitude more complex. And while this seems to be the
> second issue which could be fixed by that series, both issues seem to
> be corner cases, so I am not sure it is worth the complexity."
> 
> And for the cleanup series using struct mux_core. It is quite an
> intrusive change and, frankly, the savings look surprisingly low. I
> would have expected more, but you never find out until you do it. So, I
> am unsure here as well.
> 
> I am not decided and open for discussion. This is just where we are
> currently. All interested parties, I am looking forward to more
> thoughts.
> 
> Thanks,
Perhaps it's one to let sit into at least the next cycle (and get some testing
on those media devices if we can) but, whilst it is fiddly the gains seen in
individual drivers (like the example Peter put in response to the V4 series)
make

Re: Is IIO the appropriate subsystem for a thermal camera sensor?

2015-10-26 Thread Jonathan Cameron 


On 26 October 2015 08:21:07 GMT+00:00, Jean-Michel Hautbois 
 wrote:
>Hi Lucas,
>
>2015-10-26 8:53 GMT+01:00 Daniel Baluta :
>> Hi Lucas,
>>
>> Adding linux-iio and linux-media, I hope you don't mind.
>>
>> On 10/23/2015 06:58 PM, Lucas Magasweran wrote:
>>>
>>> Hi Daniel,
>>>
>>> My colleague, Roberto Cornetti, attended your IIO talk at LinuxCon
>>> recently and is using the IIO subsystem for an I2C IMU.
>>>
>>
>> Glad to hear that :)
>>
>>> My question is if IIO the appropriate subsystem for a thermal camera
>>> sensor. The driver needs to acquire frames over SPI and configure
>the
>>> sensor via I2C. It also has to respond to a GPIO interrupt to
>>> synchronize with the camera.
>>
>>
>> I am not sure exactly about this. My feeling is that this should go
>with the
>> v4l subsystem thus Cc-ing linux-media.
>
>This is definitely a V4L2 driver. Note however that AFAIK, no sensor
>is currently sending frames from SPI right now... Nothing impossible
>though :).

Absolutely agree. The confusion on this probably stems from thermopiles
 (which are kind of single pixel thermal cameras) and higher res thermal
 cameras.  If you think of it as the same an ambient light sensor vs an optical
 camera then the divisions become clearer.

There are very low pixel count thermal cameras that blur the boundaries though
 so I guess you may have one of those? 4x4 for example.  I still think these 
should
 be v4l.  Kind of dependent on whether the output is 2d or 1d to my mind.

Jonathan
>
>> Do you have any datasheet for this sensor?
>>
>> At some point [1] we considered introducing the fingerprint sensor as
>an IIO
>> device, but that didn't quite fit. So, we reconsidered using v4l.
>> I this is your case too :).
>>
>> Hope this helps.
>> Daniel.
>>
>>
>> [1] http://marc.info/?l=linux-kernel=141769805614596=2
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>linux-media" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] staging: iio: Drop owner assignment from i2c_driver

2015-07-11 Thread Jonathan Cameron 
On 10/07/15 07:34, Krzysztof Kozlowski wrote:
 i2c_driver does not need to set an owner because i2c_register_driver()
 will set it.
 
 Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
Applied to the togreg branch of iio.git

Thanks,

Jonathan
 
 ---
 
 The coccinelle script which generated the patch was sent here:
 http://www.spinics.net/lists/kernel/msg2029903.html
 ---
  drivers/staging/iio/addac/adt7316-i2c.c | 1 -
  drivers/staging/iio/light/isl29018.c| 1 -
  drivers/staging/iio/light/isl29028.c| 1 -
  3 files changed, 3 deletions(-)
 
 diff --git a/drivers/staging/iio/addac/adt7316-i2c.c 
 b/drivers/staging/iio/addac/adt7316-i2c.c
 index 75ddd4f801a3..78fe0b557280 100644
 --- a/drivers/staging/iio/addac/adt7316-i2c.c
 +++ b/drivers/staging/iio/addac/adt7316-i2c.c
 @@ -124,7 +124,6 @@ static struct i2c_driver adt7316_driver = {
   .driver = {
   .name = adt7316,
   .pm = ADT7316_PM_OPS,
 - .owner  = THIS_MODULE,
   },
   .probe = adt7316_i2c_probe,
   .id_table = adt7316_i2c_id,
 diff --git a/drivers/staging/iio/light/isl29018.c 
 b/drivers/staging/iio/light/isl29018.c
 index e646c5d24004..019ba5245c23 100644
 --- a/drivers/staging/iio/light/isl29018.c
 +++ b/drivers/staging/iio/light/isl29018.c
 @@ -838,7 +838,6 @@ static struct i2c_driver isl29018_driver = {
   .name = isl29018,
   .acpi_match_table = ACPI_PTR(isl29018_acpi_match),
   .pm = ISL29018_PM_OPS,
 - .owner = THIS_MODULE,
   .of_match_table = isl29018_of_match,
   },
   .probe   = isl29018_probe,
 diff --git a/drivers/staging/iio/light/isl29028.c 
 b/drivers/staging/iio/light/isl29028.c
 index e5b2fdc2334b..cd6f2727aa58 100644
 --- a/drivers/staging/iio/light/isl29028.c
 +++ b/drivers/staging/iio/light/isl29028.c
 @@ -547,7 +547,6 @@ static struct i2c_driver isl29028_driver = {
   .class  = I2C_CLASS_HWMON,
   .driver  = {
   .name = isl29028,
 - .owner = THIS_MODULE,
   .of_match_table = isl29028_of_match,
   },
   .probe   = isl29028_probe,
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/3] Introduce IIO interface for fingerprint sensors

2014-12-26 Thread Jonathan Cameron 
On 18/12/14 16:51, Lars-Peter Clausen wrote:
 Adding V4L folks to Cc for more input.
Thanks Lars - we definitely would need the v4l guys to agree to a driver like
this going in IIO. (not that I'm convinced it should!)
 
 On 12/08/2014 03:10 PM, Baluta, Teodora wrote:
 Hello,

 On Vi, 2014-12-05 at 02:15 +, Jonathan Cameron wrote:
 On 04/12/14 13:00, Teodora Baluta wrote:
 This patchset adds support for fingerprint sensors through the IIO 
 interface.
 This way userspace applications collect information in a uniform way. All
 processing would be done in the upper layers as suggested in [0].

 In order to test out this proposal, a minimal implementation for UPEK's
 TouchChip Fingerprint Sensor via USB is also available. Although there is 
 an
 existing implementation in userspace for USB fingerprint devices, 
 including this
 particular device, the driver represents a proof of concept of how 
 fingerprint
 sensors could be integrated in the IIO framework regardless of the used 
 bus. For
 lower power requirements, the SPI bus is preferred and a kernel driver
 implementation makes more sense.

 So why not v4l?  These are effectively image sensors..

 Well, here's why I don't think v4l would be the best option:

 - an image scanner could be implemented in the v4l subsystem, but it
 seems far more complicated for a simple fingerprint scanner - it usually
 has drivers for webcams, TVs or video streaming devices. The v4l
 subsystem (with all its support for colorspace, decoders, image
 compression, frame control) seems a bit of an overkill for a very
 straightforward fingerprint imaging sensor.
Whilst those are there, I would doubt the irrelevant bits would put much
burden on a fingerprint scanning driver.  Been a while since I did
anything in that area though so I could be wrong!

 - a fingerprint device could also send out a processed information, not
 just the image of a fingerprint. This means that the processing is done
 in hardware - the UPEK TouchStrip chipset in libfprint has this behavior
 (see [0]). So, the IIO framework would support a uniform way of handling
 fingerprint devices that either do processing in software or in
 hardware.
This is more interesting, but does that map well to IIO style
channels anyway?  If not we are going to end up with a whole new
interface which ever subsystem is used for the image side of things.

 The way I see it now, for processed fingerprint information, an IIO
 device could have an IIO_FINGERPRINT channel with a modifier and only
 the sensitivity threshold attribute set. We would also need two
 triggers: one for enrollment and one for the verification mode to
 control the device from a userspace application.
Sure - what you proposed would work.  The question is whether it is
the best way to do it.



 Thanks,
 Teodora

 [0] http://www.freedesktop.org/wiki/Software/fprint/libfprint/upekts/



 A sysfs trigger is enabled and the device starts scanning. As soon as an 
 image
 is available it is written in the character device /dev/iio:deviceX.

 Userspace applications will be able to calculate the expected image size 
 using
 the fingerprint attributes height, width and bit depth. Other attributes
 introduced for the fingerprint channel in IIO represent information that 
 aids in
 the fingerprint image processing. Besides these, the proposed interface 
 offers
 userspace a way to read a feedback after a scan (like the swipe was too 
 slow or
 too fast) through a modified fingerprint_status channel.

 [0] http://www.spinics.net/lists/linux-iio/msg11463.html

 Teodora Baluta (3):
iio: core: add support for fingerprint devices
iio: core: change channel's storagebits/realbits to u32
iio: fingerprint: add fingerprint sensor via USB

   Documentation/ABI/testing/sysfs-bus-iio |  51 +++
   drivers/iio/Kconfig |   1 +
   drivers/iio/Makefile|   1 +
   drivers/iio/fingerprint/Kconfig |  15 +
   drivers/iio/fingerprint/Makefile|   5 +
   drivers/iio/fingerprint/fp_tc.c | 162 +
   drivers/iio/fingerprint/fp_tc.h |  22 ++
   drivers/iio/fingerprint/fp_tc_usb.c | 618 
 
   drivers/iio/fingerprint/fp_tc_usb.h | 144 
   drivers/iio/industrialio-core.c |   9 +
   include/linux/iio/iio.h |  11 +-
   include/linux/iio/types.h   |  10 +
   12 files changed, 1047 insertions(+), 2 deletions(-)
   create mode 100644 drivers/iio/fingerprint/Kconfig
   create mode 100644 drivers/iio/fingerprint/Makefile
   create mode 100644 drivers/iio/fingerprint/fp_tc.c
   create mode 100644 drivers/iio/fingerprint/fp_tc.h
   create mode 100644 drivers/iio/fingerprint/fp_tc_usb.c
   create mode 100644 drivers/iio/fingerprint/fp_tc_usb.h



 N�r��y���b�X��ǧv�^�)޺{.n�+{��*��^n�r���z���h���G���h�(�階�ݢj���m�z�ޖ���f���h���~�mml==

 
 -- 
 To unsubscribe from this list: send the line unsubscribe linux-iio

Re: [PATCH] v4l: mt9p031/mt9t001: Use i2c_smbus_{read|write}_word_swapped()

2011-10-24 Thread Jonathan Cameron 
On 10/22/11 09:57, Laurent Pinchart wrote:
 The MT9P031 and MT9T001 sensors transfer 16-bit data on the I2C bus in
 swapped order. Let the I2C core handle byte order by using the
 i2c_smbus_{read|write}_word_swapped() functions.
 
 Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
Acked-by: Jonathan Cameron ji...@cam.ac.uk
 ---
  drivers/media/video/mt9p031.c |5 ++---
  drivers/media/video/mt9t001.c |5 ++---
  2 files changed, 4 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
 index 5cfa39f..aa01c4b 100644
 --- a/drivers/media/video/mt9p031.c
 +++ b/drivers/media/video/mt9p031.c
 @@ -131,13 +131,12 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev 
 *sd)
  
  static int mt9p031_read(struct i2c_client *client, u8 reg)
  {
 - s32 data = i2c_smbus_read_word_data(client, reg);
 - return data  0 ? data : be16_to_cpu(data);
 + return i2c_smbus_read_word_swapped(client, reg);
  }
  
  static int mt9p031_write(struct i2c_client *client, u8 reg, u16 data)
  {
 - return i2c_smbus_write_word_data(client, reg, cpu_to_be16(data));
 + return i2c_smbus_write_word_swapped(client, reg, data);
  }
  
  static int mt9p031_set_output_control(struct mt9p031 *mt9p031, u16 clear,
 diff --git a/drivers/media/video/mt9t001.c b/drivers/media/video/mt9t001.c
 index cac1416..2ea6a08 100644
 --- a/drivers/media/video/mt9t001.c
 +++ b/drivers/media/video/mt9t001.c
 @@ -132,13 +132,12 @@ static inline struct mt9t001 *to_mt9t001(struct 
 v4l2_subdev *sd)
  
  static int mt9t001_read(struct i2c_client *client, u8 reg)
  {
 - s32 data = i2c_smbus_read_word_data(client, reg);
 - return data  0 ? data : be16_to_cpu(data);
 + return i2c_smbus_read_word_swapped(client, reg);
  }
  
  static int mt9t001_write(struct i2c_client *client, u8 reg, u16 data)
  {
 - return i2c_smbus_write_word_data(client, reg, cpu_to_be16(data));
 + return i2c_smbus_write_word_swapped(client, reg, data);
  }
  
  static int mt9t001_set_output_control(struct mt9t001 *mt9t001, u16 clear,

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] v4l: use i2c_smbus_read_word_swapped

2011-10-21 Thread Jonathan Cameron 
Function ensures that error codes don't get mangled.
Dependant on:
i2c: boilerplate function for byte swapped  smbus_write/read_word_data
which is working it's way through the i2c tree.

Signed-off-by: Jonathan Cameron ji...@cam.ac.uk
---

In some cases the now largely pointless boiler plate functions
could be squished.

patch based on 3.1-rc10

 drivers/media/video/mt9m001.c |6 +++---
 drivers/media/video/mt9m111.c |7 +++
 drivers/media/video/mt9t031.c |5 ++---
 drivers/media/video/mt9v022.c |5 ++---
 drivers/media/video/mt9v032.c |8 
 5 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index 4da9cca..7493d9e 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -102,14 +102,14 @@ static struct mt9m001 *to_mt9m001(const struct i2c_client 
*client)
 
 static int reg_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
-   return data  0 ? data : swab16(data);
+   return i2c_smbus_read_word_swapped(client, reg);
+
 }
 
 static int reg_write(struct i2c_client *client, const u8 reg,
 const u16 data)
 {
-   return i2c_smbus_write_word_data(client, reg, swab16(data));
+   return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
 static int reg_set(struct i2c_client *client, const u8 reg,
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index a357aa8..41b3029 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -210,7 +210,7 @@ static int reg_page_map_set(struct i2c_client *client, 
const u16 reg)
if (page  2)
return -EINVAL;
 
-   ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
+   ret = i2c_smbus_write_word_swapped(client, MT9M111_PAGE_MAP, page);
if (!ret)
lastpage = page;
return ret;
@@ -222,7 +222,7 @@ static int mt9m111_reg_read(struct i2c_client *client, 
const u16 reg)
 
ret = reg_page_map_set(client, reg);
if (!ret)
-   ret = swab16(i2c_smbus_read_word_data(client, reg  0xff));
+   ret = i2c_smbus_read_word_swapped(client, reg  0xff);
 
dev_dbg(client-dev, read  reg.%03x - %04x\n, reg, ret);
return ret;
@@ -235,8 +235,7 @@ static int mt9m111_reg_write(struct i2c_client *client, 
const u16 reg,
 
ret = reg_page_map_set(client, reg);
if (!ret)
-   ret = i2c_smbus_write_word_data(client, reg  0xff,
-   swab16(data));
+   ret = i2c_smbus_write_word_swapped(client, reg  0xff, data);
dev_dbg(client-dev, write reg.%03x = %04x - %d\n, reg, data, ret);
return ret;
 }
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index 30547cc..e4ac238 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -81,14 +81,13 @@ static struct mt9t031 *to_mt9t031(const struct i2c_client 
*client)
 
 static int reg_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
-   return data  0 ? data : swab16(data);
+   return i2c_smbus_read_word_swapped(client, reg);
 }
 
 static int reg_write(struct i2c_client *client, const u8 reg,
 const u16 data)
 {
-   return i2c_smbus_write_word_data(client, reg, swab16(data));
+   return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
 static int reg_set(struct i2c_client *client, const u8 reg,
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 51b0fcc..d5272d9 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -116,14 +116,13 @@ static struct mt9v022 *to_mt9v022(const struct i2c_client 
*client)
 
 static int reg_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
-   return data  0 ? data : swab16(data);
+   return i2c_smbus_read_word_swapped(client, reg);
 }
 
 static int reg_write(struct i2c_client *client, const u8 reg,
 const u16 data)
 {
-   return i2c_smbus_write_word_data(client, reg, swab16(data));
+   return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
 static int reg_set(struct i2c_client *client, const u8 reg,
diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index c64e1dc..7906929 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -138,10 +138,10 @@ static struct mt9v032 *to_mt9v032(struct v4l2_subdev *sd)
 
 static int mt9v032_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
+   s32 data = i2c_smbus_read_word_swapped(client, reg);
dev_dbg(client-dev, %s: read 0x%04x from 0x%02x\n, __func__,
-   swab16(data), reg

[PATCH V2] v4l: use i2c_smbus_read_word_swapped

2011-10-21 Thread Jonathan Cameron 
Function ensures that error codes don't get mangled.
Dependant on:
i2c: boilerplate function for byte swapped  smbus_write/read_word_data
which is working it's way through the i2c tree.

Signed-off-by: Jonathan Cameron ji...@cam.ac.uk
Acked-by: Jean Delvare kh...@linux-fr.org
---

V2: Stray line removed as per Jean's comment.  Thanks Jean.

Dependency due to go upstream during next merge window.

 drivers/media/video/mt9m001.c |5 ++---
 drivers/media/video/mt9m111.c |7 +++
 drivers/media/video/mt9t031.c |5 ++---
 drivers/media/video/mt9v022.c |5 ++---
 drivers/media/video/mt9v032.c |8 
 5 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index 4da9cca..862a300 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -102,14 +102,13 @@ static struct mt9m001 *to_mt9m001(const struct i2c_client 
*client)
 
 static int reg_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
-   return data  0 ? data : swab16(data);
+   return i2c_smbus_read_word_swapped(client, reg);
 }
 
 static int reg_write(struct i2c_client *client, const u8 reg,
 const u16 data)
 {
-   return i2c_smbus_write_word_data(client, reg, swab16(data));
+   return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
 static int reg_set(struct i2c_client *client, const u8 reg,
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index a357aa8..41b3029 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -210,7 +210,7 @@ static int reg_page_map_set(struct i2c_client *client, 
const u16 reg)
if (page  2)
return -EINVAL;
 
-   ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
+   ret = i2c_smbus_write_word_swapped(client, MT9M111_PAGE_MAP, page);
if (!ret)
lastpage = page;
return ret;
@@ -222,7 +222,7 @@ static int mt9m111_reg_read(struct i2c_client *client, 
const u16 reg)
 
ret = reg_page_map_set(client, reg);
if (!ret)
-   ret = swab16(i2c_smbus_read_word_data(client, reg  0xff));
+   ret = i2c_smbus_read_word_swapped(client, reg  0xff);
 
dev_dbg(client-dev, read  reg.%03x - %04x\n, reg, ret);
return ret;
@@ -235,8 +235,7 @@ static int mt9m111_reg_write(struct i2c_client *client, 
const u16 reg,
 
ret = reg_page_map_set(client, reg);
if (!ret)
-   ret = i2c_smbus_write_word_data(client, reg  0xff,
-   swab16(data));
+   ret = i2c_smbus_write_word_swapped(client, reg  0xff, data);
dev_dbg(client-dev, write reg.%03x = %04x - %d\n, reg, data, ret);
return ret;
 }
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index 30547cc..e4ac238 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -81,14 +81,13 @@ static struct mt9t031 *to_mt9t031(const struct i2c_client 
*client)
 
 static int reg_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
-   return data  0 ? data : swab16(data);
+   return i2c_smbus_read_word_swapped(client, reg);
 }
 
 static int reg_write(struct i2c_client *client, const u8 reg,
 const u16 data)
 {
-   return i2c_smbus_write_word_data(client, reg, swab16(data));
+   return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
 static int reg_set(struct i2c_client *client, const u8 reg,
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 51b0fcc..d5272d9 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -116,14 +116,13 @@ static struct mt9v022 *to_mt9v022(const struct i2c_client 
*client)
 
 static int reg_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
-   return data  0 ? data : swab16(data);
+   return i2c_smbus_read_word_swapped(client, reg);
 }
 
 static int reg_write(struct i2c_client *client, const u8 reg,
 const u16 data)
 {
-   return i2c_smbus_write_word_data(client, reg, swab16(data));
+   return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
 static int reg_set(struct i2c_client *client, const u8 reg,
diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index c64e1dc..7906929 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -138,10 +138,10 @@ static struct mt9v032 *to_mt9v032(struct v4l2_subdev *sd)
 
 static int mt9v032_read(struct i2c_client *client, const u8 reg)
 {
-   s32 data = i2c_smbus_read_word_data(client, reg);
+   s32 data = i2c_smbus_read_word_swapped(client, reg);
dev_dbg(client-dev, %s: read 0x%04x from 0x%02x\n, __func__

Error routes through omap3isp ccdc.

2011-07-11 Thread Jonathan Cameron 
Hi All,

Came across this when my camera driver failed to load..

Following causes a kernel panic.

Set up link between cdcc and ccdc output to memory.
(at this point intitializing a capture gives an invalid
arguement error which is fine.)

Now set the format on ISP CCDC/0 to SGRBG10 752x480 (doubt what these
settings are actually matters).  Actually come to think of it, this
may only be relevant as otherwise, the format of my attempted stream
setup from userspace doesn't matter.

Blithely attempt to grab frames from userspace off the ccdc output.
(note it has no input.)

Null pointer exception occurs because:

pad = media_entity_remote_source(ccdc-pads[CCDC_PAD_SINK]);
in 
static void ccdc_configure(struct isp_ccdc_device *ccdc)

returns null and this is not checked.

Obvious local fix is to check the value of pad and allow ccdc_configure
to return an error + pass this up out of ccdc_set_stream.

Something like the following does the trick.
(not sure error code is right choice!)

Patch is against todays linux-next + a few unconnected things to make
that tree actually build for the beagle xm.

Grep isn't indicating any exactly matching cases to this problem, so
the rest of the tree may be fine.

 [PATCH] omap3isp: check if there is actually a source for ispccdc when
 setting up the link.

Without this if no source actually exists and the ccdc is configured
to output to memory, a read from userspace will cause a null pointer
exception.

Signed-off-by: Jonathan Cameron ji...@cam.ac.uk
---
 drivers/media/video/omap3isp/ispccdc.c |9 +++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispccdc.c 
b/drivers/media/video/omap3isp/ispccdc.c
index 6766247..2703a94 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1110,7 +1110,7 @@ static const u32 ccdc_sgbrg_pattern =
ISPCCDC_COLPTN_R_Ye   ISPCCDC_COLPTN_CP3PLC2_SHIFT |
ISPCCDC_COLPTN_Gr_Cy  ISPCCDC_COLPTN_CP3PLC3_SHIFT;
 
-static void ccdc_configure(struct isp_ccdc_device *ccdc)
+static int ccdc_configure(struct isp_ccdc_device *ccdc)
 {
struct isp_device *isp = to_isp_device(ccdc);
struct isp_parallel_platform_data *pdata = NULL;
@@ -1127,6 +1127,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
u32 ccdc_pattern;
 
pad = media_entity_remote_source(ccdc-pads[CCDC_PAD_SINK]);
+   if (pad == NULL)
+   return -ENODEV;
sensor = media_entity_to_v4l2_subdev(pad-entity);
if (ccdc-input == CCDC_INPUT_PARALLEL)
pdata = ((struct isp_v4l2_subdevs_group *)sensor-host_priv)
@@ -1257,6 +1259,7 @@ unlock:
spin_unlock_irqrestore(ccdc-lsc.req_lock, flags);
 
ccdc_apply_controls(ccdc);
+   return 0;
 }
 
 static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
@@ -1726,7 +1729,9 @@ static int ccdc_set_stream(struct v4l2_subdev *sd, int 
enable)
isp_reg_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_CFG,
ISPCCDC_CFG_VDLC);
 
-   ccdc_configure(ccdc);
+   ret = ccdc_configure(ccdc);
+   if (ret  0)
+   return ret;
 
/* TODO: Don't configure the video port if all of its output
 * links are inactive.
-- 
1.7.3.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Error routes through omap3isp ccdc.

2011-07-11 Thread Jonathan Cameron 
On 07/11/11 11:57, Laurent Pinchart wrote:
 On Monday 11 July 2011 12:54:42 Laurent Pinchart wrote:
 On Monday 11 July 2011 12:41:49 Jonathan Cameron wrote:
 
 [snip]
 
 I think we should try to fix it in ispvideo.c instead. You could add a
 check to isp_video_validate_pipeline() to make sure that the pipeline has
 a video source.
 
 And I forgot to mention, I can send a patch if you don't want to write it.
 
Given I can't quite see why the validate_pipeline code would ever want to break
on source pad being null (which I think is what it is currently doing),
I'll leave it to you.  Really don't know this code well enough!

Thanks.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: omap3isp: known causes of CCDC won't become idle!

2011-07-06 Thread Jonathan Cameron 
On 07/05/11 17:35, Jonathan Cameron wrote:
 On 07/05/11 16:22, Jonathan Cameron wrote:
 On 07/05/11 16:02, Laurent Pinchart wrote:
 On Tuesday 05 July 2011 16:38:07 Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 02:48:57PM +0100, Jonathan Cameron wrote:
 On 07/05/11 13:19, Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 12:22:06PM +0100, Jonathan Cameron wrote:
 Hi Laurent,

 I'm just trying to get an mt9v034 sensor working on a beagle xm.
 Everything more or less works, except that after a random number
 of frames of capture, I tend to get won't become idle messages
 and the vd0 and vd1 interrupts tend to turn up at same time.

 I was just wondering if there are any known issues with the ccdc
 driver / silicon that might explain this?

 I also note that it appears to be impossible to disable
 HS_VS_IRQarch/arm/mach-s3c2410/Kconfig:# cpu frequency scaling
 support

 despite the datasheet claiming this can be done.  Is this a known
 issue?

 The same interrupt may be used to produce an interrupt per horizontal
 sync but the driver doesn't use that. I remember of a case where the
 two sync signals had enough crosstalk to cause vertical sync interrupt
 per every horizontal sync. (It's been discussed on this list.) This
 might not be the case here, though: you should be flooded with HS_VS
 interrupts.

 As far as I can tell, the driver doesn't use either interrupt (except to
 pass it up as an event). Hence I was trying to mask it purely to cut
 down on the interrupt load.

 It does. This is the only way to detect the CCDC has finished processing a
 frame.

 We actually use the VD0 and VD1 interrupts for that, not the HS_VS 
 interrupt.
 On that note, the interrupt was being disabled, just not it's value in the
 register.  What I was actually seeing was it combined with one of the others.
 

 The VD* counters are counting and interrupts are produced (AFAIR) even
 if the CCDC is disabled.

 Oh goody...

 Once the CCDC starts receiving a frame, it becomes busy, and becomes
 idle only when it has received the full frame. For this reason it's
 important that the full frame is actually received by the CCDC,
 otherwise this is due to happen when the CCDC is being stopped at the
 end of the stream.

 Fair enough.  Is there any software reason why it might think it hasn't
 received the whole frame?  Obviously it could in theory be a hardware
 issue, but it's a bit odd that it can reliably do a certain number of
 frames before falling over.

 Others than those which Laurent already pointed out, one which crosses my
 mind is the vsync polarity. The Documentation/video4linux/omap3isp.txt does
 mention it. It _may_ have the effect that one line of input is missed by
 the VD* counters. Thus the VD* counters might never reach the expected
 value --- the last line of the frame.

 I would first try to increase vertical blanking to see if it helps.
 Have done. No luck as yet.  This sensor mt9v034 annoyingly starts live.
 Right now this means I get two frames with very short vblank (10% ratio, at 
 60fps,
 so sub 2 microseonds.)  Whilst the failure seems to be at a later time, I'd
 obviously like to get rid of these.
 Hmm. Via a bit of reordering of writes and a temporary disable, it now does 
 one frame,
 then about a 100ms pause, then continues with 50ms blanking periods, vs, 
 about 18ms of
 data transfer.  Still running into what looks like the same issue with the 
 ccdc getting
 wedged...
 
 Having introduced some debugging into the isr I get:
 
   55.123840] power on called
 [   55.135528] interrupt 2147483648
 [   55.139038] interrupt 2147483648
 [   55.142578] interrupt 2147483648
 [   55.145965] interrupt 2147483648
 [   55.149383] interrupt 2147483648
 [   55.152770] interrupt 2147483648
 [   55.156188] interrupt 2147483648
 [   55.159576] interrupt 2147483648
 [   55.162994] interrupt 2147483648
 [   55.181854] end of set power
 [   55.241821] get format called
 [   55.245025] get pad format
 [   55.248138] in
 [   55.249877] get format called
 [   55.252990] get pad format
 [   55.256195] on
 [   55.257904] s_stream called
 [   55.260833] set params called
 [   55.267944] stream enable attempted
 [   55.282257] interrupt 2147483648
 [   55.302429] interrupt 512
 [   55.312408] interrupt 256
 [   55.385864] interrupt 2147483648
 [   55.406005] interrupt 512
 [   55.415985] interrupt 256
 [   55.492401] interrupt 2147483648
 [   55.512603] interrupt 512
 [   55.522583] interrupt 256
 [   55.599029] interrupt 2147483648
 [   55.619201] interrupt 512
 [   55.629180] interrupt 256
 [   55.705627] interrupt 2147483648
 [   55.725738] interrupt 512
 [   55.735687] interrupt 256
 [   55.740417] omap3isp omap3isp: CCDC won't become idle!
 [   55.811645] interrupt 2147483648
 [   55.832000] interrupt 512
 [   55.842010] interrupt 256
 Repeat last 4 lines ad infinitum or if lucky till my program gives up
 and closes everything.  Sometimes that hangs the machine as well.
 
 So nothing obviously changing. Waveform of the vsync signal

omap3isp: known causes of CCDC won't become idle!

2011-07-05 Thread Jonathan Cameron 
Hi Laurent,

I'm just trying to get an mt9v034 sensor working on a beagle xm.
Everything more or less works, except that after a random number
of frames of capture, I tend to get won't become idle messages
and the vd0 and vd1 interrupts tend to turn up at same time.

I was just wondering if there are any known issues with the ccdc
driver / silicon that might explain this?

I also note that it appears to be impossible to disable HS_VS_IRQ
despite the datasheet claiming this can be done.  Is this a known
issue?

Thanks,

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: omap3isp: known causes of CCDC won't become idle!

2011-07-05 Thread Jonathan Cameron 
On 07/05/11 13:19, Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 12:22:06PM +0100, Jonathan Cameron wrote:
 Hi Laurent,

 I'm just trying to get an mt9v034 sensor working on a beagle xm.
 Everything more or less works, except that after a random number
 of frames of capture, I tend to get won't become idle messages
 and the vd0 and vd1 interrupts tend to turn up at same time.

 I was just wondering if there are any known issues with the ccdc
 driver / silicon that might explain this?

 I also note that it appears to be impossible to disable 
 HS_VS_IRQarch/arm/mach-s3c2410/Kconfig:# cpu frequency scaling support

 despite the datasheet claiming this can be done.  Is this a known
 issue?
 
 The same interrupt may be used to produce an interrupt per horizontal sync
 but the driver doesn't use that. I remember of a case where the two sync
 signals had enough crosstalk to cause vertical sync interrupt per every
 horizontal sync. (It's been discussed on this list.) This might not be the
 case here, though: you should be flooded with HS_VS interrupts.
As far as I can tell, the driver doesn't use either interrupt (except to pass
it up as an event). Hence I was trying to mask it purely to cut down on the
interrupt load.
 
 The VD* counters are counting and interrupts are produced (AFAIR) even if
 the CCDC is disabled.
Oh goody...
 
 Once the CCDC starts receiving a frame, it becomes busy, and becomes idle
 only when it has received the full frame. For this reason it's important
 that the full frame is actually received by the CCDC, otherwise this is due
 to happen when the CCDC is being stopped at the end of the stream.
Fair enough.  Is there any software reason why it might think it hasn't received
the whole frame?  Obviously it could in theory be a hardware issue, but it's
a bit odd that it can reliably do a certain number of frames before falling 
over.






--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: omap3isp: known causes of CCDC won't become idle!

2011-07-05 Thread Jonathan Cameron 
On 07/05/11 16:02, Laurent Pinchart wrote:
 On Tuesday 05 July 2011 16:38:07 Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 02:48:57PM +0100, Jonathan Cameron wrote:
 On 07/05/11 13:19, Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 12:22:06PM +0100, Jonathan Cameron wrote:
 Hi Laurent,

 I'm just trying to get an mt9v034 sensor working on a beagle xm.
 Everything more or less works, except that after a random number
 of frames of capture, I tend to get won't become idle messages
 and the vd0 and vd1 interrupts tend to turn up at same time.

 I was just wondering if there are any known issues with the ccdc
 driver / silicon that might explain this?

 I also note that it appears to be impossible to disable
 HS_VS_IRQarch/arm/mach-s3c2410/Kconfig:# cpu frequency scaling
 support

 despite the datasheet claiming this can be done.  Is this a known
 issue?

 The same interrupt may be used to produce an interrupt per horizontal
 sync but the driver doesn't use that. I remember of a case where the
 two sync signals had enough crosstalk to cause vertical sync interrupt
 per every horizontal sync. (It's been discussed on this list.) This
 might not be the case here, though: you should be flooded with HS_VS
 interrupts.

 As far as I can tell, the driver doesn't use either interrupt (except to
 pass it up as an event). Hence I was trying to mask it purely to cut
 down on the interrupt load.

 It does. This is the only way to detect the CCDC has finished processing a
 frame.
 
 We actually use the VD0 and VD1 interrupts for that, not the HS_VS interrupt.
 
 The VD* counters are counting and interrupts are produced (AFAIR) even
 if the CCDC is disabled.

 Oh goody...

 Once the CCDC starts receiving a frame, it becomes busy, and becomes
 idle only when it has received the full frame. For this reason it's
 important that the full frame is actually received by the CCDC,
 otherwise this is due to happen when the CCDC is being stopped at the
 end of the stream.

 Fair enough.  Is there any software reason why it might think it hasn't
 received the whole frame?  Obviously it could in theory be a hardware
 issue, but it's a bit odd that it can reliably do a certain number of
 frames before falling over.

 Others than those which Laurent already pointed out, one which crosses my
 mind is the vsync polarity. The Documentation/video4linux/omap3isp.txt does
 mention it. It _may_ have the effect that one line of input is missed by
 the VD* counters. Thus the VD* counters might never reach the expected
 value --- the last line of the frame.
 
 I would first try to increase vertical blanking to see if it helps.
Have done. No luck as yet.  This sensor mt9v034 annoyingly starts live.
Right now this means I get two frames with very short vblank (10% ratio, at 
60fps,
so sub 2 microseonds.)  Whilst the failure seems to be at a later time, I'd
obviously like to get rid of these.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: omap3isp: known causes of CCDC won't become idle!

2011-07-05 Thread Jonathan Cameron 
On 07/05/11 16:22, Jonathan Cameron wrote:
 On 07/05/11 16:02, Laurent Pinchart wrote:
 On Tuesday 05 July 2011 16:38:07 Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 02:48:57PM +0100, Jonathan Cameron wrote:
 On 07/05/11 13:19, Sakari Ailus wrote:
 On Tue, Jul 05, 2011 at 12:22:06PM +0100, Jonathan Cameron wrote:
 Hi Laurent,

 I'm just trying to get an mt9v034 sensor working on a beagle xm.
 Everything more or less works, except that after a random number
 of frames of capture, I tend to get won't become idle messages
 and the vd0 and vd1 interrupts tend to turn up at same time.

 I was just wondering if there are any known issues with the ccdc
 driver / silicon that might explain this?

 I also note that it appears to be impossible to disable
 HS_VS_IRQarch/arm/mach-s3c2410/Kconfig:# cpu frequency scaling
 support

 despite the datasheet claiming this can be done.  Is this a known
 issue?

 The same interrupt may be used to produce an interrupt per horizontal
 sync but the driver doesn't use that. I remember of a case where the
 two sync signals had enough crosstalk to cause vertical sync interrupt
 per every horizontal sync. (It's been discussed on this list.) This
 might not be the case here, though: you should be flooded with HS_VS
 interrupts.

 As far as I can tell, the driver doesn't use either interrupt (except to
 pass it up as an event). Hence I was trying to mask it purely to cut
 down on the interrupt load.

 It does. This is the only way to detect the CCDC has finished processing a
 frame.

 We actually use the VD0 and VD1 interrupts for that, not the HS_VS interrupt.
On that note, the interrupt was being disabled, just not it's value in the
register.  What I was actually seeing was it combined with one of the others.


 The VD* counters are counting and interrupts are produced (AFAIR) even
 if the CCDC is disabled.

 Oh goody...

 Once the CCDC starts receiving a frame, it becomes busy, and becomes
 idle only when it has received the full frame. For this reason it's
 important that the full frame is actually received by the CCDC,
 otherwise this is due to happen when the CCDC is being stopped at the
 end of the stream.

 Fair enough.  Is there any software reason why it might think it hasn't
 received the whole frame?  Obviously it could in theory be a hardware
 issue, but it's a bit odd that it can reliably do a certain number of
 frames before falling over.

 Others than those which Laurent already pointed out, one which crosses my
 mind is the vsync polarity. The Documentation/video4linux/omap3isp.txt does
 mention it. It _may_ have the effect that one line of input is missed by
 the VD* counters. Thus the VD* counters might never reach the expected
 value --- the last line of the frame.

 I would first try to increase vertical blanking to see if it helps.
 Have done. No luck as yet.  This sensor mt9v034 annoyingly starts live.
 Right now this means I get two frames with very short vblank (10% ratio, at 
 60fps,
 so sub 2 microseonds.)  Whilst the failure seems to be at a later time, I'd
 obviously like to get rid of these.
Hmm. Via a bit of reordering of writes and a temporary disable, it now does one 
frame,
then about a 100ms pause, then continues with 50ms blanking periods, vs, about 
18ms of
data transfer.  Still running into what looks like the same issue with the ccdc 
getting
wedged...

Having introduced some debugging into the isr I get:

  55.123840] power on called
[   55.135528] interrupt 2147483648
[   55.139038] interrupt 2147483648
[   55.142578] interrupt 2147483648
[   55.145965] interrupt 2147483648
[   55.149383] interrupt 2147483648
[   55.152770] interrupt 2147483648
[   55.156188] interrupt 2147483648
[   55.159576] interrupt 2147483648
[   55.162994] interrupt 2147483648
[   55.181854] end of set power
[   55.241821] get format called
[   55.245025] get pad format
[   55.248138] in
[   55.249877] get format called
[   55.252990] get pad format
[   55.256195] on
[   55.257904] s_stream called
[   55.260833] set params called
[   55.267944] stream enable attempted
[   55.282257] interrupt 2147483648
[   55.302429] interrupt 512
[   55.312408] interrupt 256
[   55.385864] interrupt 2147483648
[   55.406005] interrupt 512
[   55.415985] interrupt 256
[   55.492401] interrupt 2147483648
[   55.512603] interrupt 512
[   55.522583] interrupt 256
[   55.599029] interrupt 2147483648
[   55.619201] interrupt 512
[   55.629180] interrupt 256
[   55.705627] interrupt 2147483648
[   55.725738] interrupt 512
[   55.735687] interrupt 256
[   55.740417] omap3isp omap3isp: CCDC won't become idle!
[   55.811645] interrupt 2147483648
[   55.832000] interrupt 512
[   55.842010] interrupt 256
Repeat last 4 lines ad infinitum or if lucky till my program gives up
and closes everything.  Sometimes that hangs the machine as well.

So nothing obviously changing. Waveform of the vsync signal looks right to
me.

The other fun wedge that sometimes happens is both vd0 and vd1 occur together

Re: pxa_camera + ov9655: image shifted on first capture after reset

2010-04-16 Thread Jonathan Cameron 
On 04/16/10 09:25, Stefan Herbrechtsmeier wrote:
 Hi,
 
 I have updated my ov9655 driver to kernel 2.6.33 and
 did some test regarding the image shift problem on pxa.
 (http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/10773/focus=11810)
 
 
 - The image was shifted 32 pixels (64 bytes) to the right
  or rather the first 32 pixels belongs to the previous image.
 - The image was only shifted on the first capture after reset.
   It doesn't matter whether I previous change the resolution with v4l2-ctl.
 - On big images (1280 x 1024) the shift disappears after some images,
   but not on small images (320 x 240).
 
 It looks like the FIFO was not cleared at start capture.
 
Sounds reasonable.  Similar problem seen with ov7670 attached to pxa271.
I've never taken the time to try and track it down.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: exposing controls in sysfs

2010-04-06 Thread Jonathan Cameron 
On 04/06/10 15:32, Mauro Carvalho Chehab wrote:
 Hans Verkuil wrote:
 Hans Verkuil wrote:
 $ ls /sys/class/video4linux/video1/controls
 balance   mpeg_insert_navigation_packets
 mpeg_video_aspect
 brightnessmpeg_median_chroma_filter_maximum
 mpeg_video_b_frames
 chroma_agcmpeg_median_chroma_filter_minimum
 mpeg_video_bitrate
 chroma_gain   mpeg_median_filter_type
 mpeg_video_bitrate_mode
 contrast  mpeg_median_luma_filter_maximum
 mpeg_video_encoding
 hue   mpeg_median_luma_filter_minimum
 mpeg_video_gop_closure
 mpeg_audio_crcmpeg_spatial_chroma_filter_type
 mpeg_video_gop_size
 mpeg_audio_emphasis   mpeg_spatial_filter
 mpeg_video_mute
 mpeg_audio_encoding   mpeg_spatial_filter_mode
 mpeg_video_mute_yuv
 mpeg_audio_layer_ii_bitrate   mpeg_spatial_luma_filter_type
 mpeg_video_peak_bitrate
 mpeg_audio_mute   mpeg_stream_type
 mpeg_video_temporal_decimation
 mpeg_audio_sampling_frequency mpeg_stream_vbi_format
 mute
 mpeg_audio_stereo_modempeg_temporal_filter
 saturation
 mpeg_audio_stereo_mode_extension  mpeg_temporal_filter_mode
 volume

 It would be more intuitive if you group the classes with a few subdirs:

 /video/balance
 /video/brightness
 ...
 /mpeg_audio/crc
 /mpeg_audio/mute
 ...
 /audio/volume
 /audio/bass
 /audio/treble

 1) We don't have that information.
 2) It would make a simple scheme suddenly a lot more complicated (see
 Andy's comments)
 3) The main interface is always the application's GUI through ioctls, not
 sysfs.
 4) Remember that ivtv has an unusually large number of controls. Most
 drivers will just have the usual audio and video controls, perhaps 10 at
 most.
 
 Ok.
 
 I think we should just ditch this for the first implementation of the
 control framework. It can always be added later, but once added it is
 *much* harder to remove again. It's a nice proof-of-concept, though :-)
 
 I like the concept, especially if we can get rid of other similar sysfs 
 interfaces
 that got added on a few drivers (pvrusb2 and some non-gspca drivers have
 it, for sure). I think I saw some of the gspca patches also touching on sysfs.
 Having this unified into a common interface is a bonus.

Obviously it adds to the review burden, but perhaps we can state that the sysfs
interface is only an additional option (and personally I think I'd find it 
pretty
helpful for debugging if nothing else) and that all functionality there MUST be
available through the normal routes?  If any functionality only supported via
sysfs is seen as a bug, then we can point it out in reviews etc.

I agree with Mauro that it would be really handy to unify any interfaces that 
are
going to turn up there anyway.

Generally I'm personally in favor with the convenience of sysfs interfaces for 
quick
and dirty debugging purposes but perhaps this isn't the time to do it here.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: exposing controls in sysfs

2010-04-06 Thread Jonathan Cameron 
On 04/06/10 15:41, Mike Isely wrote:
 On Tue, 6 Apr 2010, Hans Verkuil wrote:
 
[...]
 

 One thing that might be useful is to prefix the name with the control class
 name. E.g. hue becomes user_hue and audio_crc becomes mpeg_audio_crc. It 
 would
 groups them better. Or one could make a controls/user and controls/mpeg
 directory. That might not be such a bad idea actually.
 
 I agree with grouping in concept, and using subdirectories is not a bad 
 thing.  Probably however you'd want to ensure that in the end all the 
 controls end up logically at the same depth in the tree.
 
 
[...]
 

 An in between solution would be to add _type files. So you would have 
 'hue' and 'hue_type'. 'cat hue_type' would give something like:

 int 0 255 1 128 0x Hue

 In other words 'type min max step flags name'.
 
 There was I thought at some point in the past a kernel policy that sysfs 
 controls were supposed to limit themselves to one value per node.
It's usually considered to be one 'conceptual' value per node, though
this falls fowl of that rule too.  So you could have one file with a list
of possible values, or even one for say hue_range 0...255 but people are
going to through a wobbly about antyhing with as much data in it as above.

The debate on this was actually pretty well covered in an lwn article the
other week. http://lwn.net/Articles/378884/

So the above hue type would probably need:

hue_type (int)
hue_range (0...255)
hue_step (1)
hue_flags (128)
hue_name (Hue)

Of those, hue_name doesn't in this case tell us anything and hue_step could
be suppressed as an obvious default.  It could be argued that parts of the
above could be considered a single 'conceptual' value but I don't think the
whole can be.  The reasoning behind this  (and it is definitely true with
your above example) is that sysfs should be human readable without needing
to reach for the documentation.
 

 And for menu controls like stream_type (hmm, that would become 
 stream_type_type...) you would get:

 menu 0 5 1 0 Stream Type

 MPEG-2 Program Stream

 MPEG-1 System Stream
 MPEG-2 DVD-compatible Stream
 MPEG-1 VCD-compatible Stream
 MPEG-2 SVCD-compatible Stream

 Note the empty line to denote the unsupported menu item (transport stream).

 This would give the same information with just a single extra file. Still not
 sure whether it is worth it though.
 
 Just remember that the more complex / subtle you make the node contents, 
 then the more parsing will be required for any program that tries to use 
 it.  I also think it's probably a bad idea for example to define a 
 format where the whitespace conveys additional information.  The case 
 where I've seen whitespace as part of the syntax actually work cleanly 
 is in Python.
 
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: adv7180 as SoC camera device

2010-02-22 Thread Jonathan Cameron 
On 02/22/10 16:16, Guennadi Liakhovetski wrote:
 On Mon, 22 Feb 2010, Rodolfo Giometti wrote:
 
 On Fri, Feb 19, 2010 at 08:36:38PM +0100, Guennadi Liakhovetski wrote:
 On Fri, 19 Feb 2010, Rodolfo Giometti wrote:

 Hello,

 on my pxa27x based board I have a adv7180 connected with the CIF
 interface. Due this fact I'm going to use the pxa_camera.c driver
 which in turn registers a soc_camera_host.

 In the latest kernel I found your driver for the ADV7180, but it
 registers the chip as a v4l sub device.

 I suppose these two interfaces are not compatible, aren't they?

 Congratulations! Thereby you're in a position to develop the first 
 v4l2-subdev / soc-camera universal driver;) The answer to this your 
 question is - they are... kinda. This means - yes, soc-camera is also 
 using the v4l2-subdev API, but - with a couple of additions. Basically, 
 there are two things you have to change in the adv7180 driver to make it 
 compatible with soc-camera - (1) add bus-configuration methods, even if 
 they don't do much (see .query_bus_param() and .set_bus_param() methods 
 from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
 API. The latter one requires some care - in principle, mediabus should be 
 the future API to negotiate parameters on the video bus between bridges 
 (in your case PXA CIF) and clients, but for you this means you also have 
 to migrate any other bridge drivers in the mainline to that API, and, if 
 they also interface to some other subdevices - those too, and if those can 
 also work with other bridges - those too...;) But, I think, that chain 
 will terminate quite soon, in fact, I cannot find any users of that driver 
 currently in the mainline, Richard?

 In this situation, should I write a new driver for the
 soc_camera_device? Which is The-Right-Thing(TM) to do? :)

 Please, have a look and try to convert the driver as described above. All 
 the APIs and a few examples are in the mainline, so, you should have 
 enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
 still unclear.

 Thanks for your quick answer! :)

 What I still don't understand is if should I move the driver form
 v4l2-subdev to a soc_camera device or trying to support both API...
 
 Both. It is just one (v4l2-subdev) API, but soc-camera is using some 
 extensions to it.
 
 It seems to me that the driver is not used by any machines into
 mainline
 
 That makes your task even easier - you do not have to convert any bridge 
 drivers to mediabus API.
Indeed.  Having time to do that is what is delaying the ov7670 conversion.
(that and having time in general!)  For info I did post some untested
patches for the ov7670 a while back that show the minimal changes I think
are needed under these circumstances.

 
 so if soc-camera is also using the v4l2-subdev API but with a
 couple of additions I suppose I can move it to soc_camera API...
 
 Not move, extend. But preserve an ability to function without soc-camera 
 additions too. I.e., the use of soc-camera extensions should be optional 
 in your driver. Look here 
 http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/11486/focus=11493
  
 for an example.
 
 Thanks
 Guennadi
 ---
 Guennadi Liakhovetski, Ph.D.
 Freelance Open-Source Software Developer
 http://www.open-technology.de/
 --
 To unsubscribe from this list: send the line unsubscribe linux-media in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PULL] soc-camera and mediabus

2009-12-14 Thread Jonathan Cameron 
Hi All,

 3) it would be interesting to patch the other sensor drivers to be compatible
with soc_camera (mt9v011/ov7670);
 
 Well, this could be done, yes, but does it make sense to do this blindly 
 without any hardware to test? I would rather add such conversions on a 
 one-by-one basis as need arises.
 
I'm working on the ov7670. I've added the media bus stuff to what I've
previously posted but I haven't tested as yet.  For reference, a quick and dirty
cut of the patch is below.  Some thought is needed on how to deal with the 
sections
that currently need to be different for the soc-camera and non soc camera uses.
(mainly the registration code in probe).

On another note, does anyone have an objection to making the set_bus_param
function optional?   At the moment we are adding null functions for those
devices that can't actually change anything which seems a little pointless.

Jonathan


diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
index 0e2184e..e57c3b5 100644
--- a/drivers/media/video/ov7670.c
+++ b/drivers/media/video/ov7670.c
@@ -18,6 +18,7 @@
 #include media/v4l2-device.h
 #include media/v4l2-chip-ident.h
 #include media/v4l2-i2c-drv.h
+#include media/soc_camera.h
 
 
 MODULE_AUTHOR(Jonathan Corbet cor...@lwn.net);
@@ -545,7 +546,7 @@ static struct ov7670_format_struct {
.bpp= 1
},
 };
-#define N_OV7670_FMTS ARRAY_SIZE(ov7670_formats)
+
 
 
 /*
@@ -668,52 +669,37 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int 
hstart, int hstop,
return ret;
 }
 
+static enum v4l2_mbus_pixelcode ov7670_codes[] = {
+   V4L2_MBUS_FMT_YUYV8_2X8,
+   V4L2_MBUS_FMT_RGB565_2X8_LE,
+};
 
-static int ov7670_enum_fmt(struct v4l2_subdev *sd, struct v4l2_fmtdesc *fmt)
+static int ov7670_enum_fmt(struct v4l2_subdev *sd, int index, enum 
v4l2_mbus_pixelcode *code)
 {
-   struct ov7670_format_struct *ofmt;
-
-   if (fmt-index = N_OV7670_FMTS)
+   if (index = ARRAY_SIZE(ov7670_codes))
return -EINVAL;
 
-   ofmt = ov7670_formats + fmt-index;
-   fmt-flags = 0;
-   strcpy(fmt-description, ofmt-desc);
-   fmt-pixelformat = ofmt-pixelformat;
+   *code = ov7670_codes[index];
return 0;
 }
 
 
 static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
-   struct v4l2_format *fmt,
-   struct ov7670_format_struct **ret_fmt,
-   struct ov7670_win_size **ret_wsize)
+  struct v4l2_mbus_framefmt *mf,
+  struct ov7670_format_struct **ret_fmt,
+  struct ov7670_win_size **ret_wsize)
 {
int index;
struct ov7670_win_size *wsize;
-   struct v4l2_pix_format *pix = fmt-fmt.pix;
 
-   for (index = 0; index  N_OV7670_FMTS; index++)
-   if (ov7670_formats[index].pixelformat == pix-pixelformat)
-   break;
-   if (index = N_OV7670_FMTS) {
-   /* default to first format */
-   index = 0;
-   pix-pixelformat = ov7670_formats[0].pixelformat;
-   }
-   if (ret_fmt != NULL)
-   *ret_fmt = ov7670_formats + index;
-   /*
-* Fields: the OV devices claim to be progressive.
-*/
-   pix-field = V4L2_FIELD_NONE;
+   mf-field = V4L2_FIELD_NONE;
/*
 * Round requested image size down to the nearest
 * we support, but not below the smallest.
 */
for (wsize = ov7670_win_sizes; wsize  ov7670_win_sizes + N_WIN_SIZES;
 wsize++)
-   if (pix-width = wsize-width  pix-height = wsize-height)
+   if ( mf-width = wsize-width  mf-height = wsize-height)
break;
if (wsize = ov7670_win_sizes + N_WIN_SIZES)
wsize--;   /* Take the smallest one */
@@ -722,22 +708,34 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
/*
 * Note the size we'll actually handle.
 */
-   pix-width = wsize-width;
-   pix-height = wsize-height;
-   pix-bytesperline = pix-width*ov7670_formats[index].bpp;
-   pix-sizeimage = pix-height*pix-bytesperline;
+   mf-width = wsize-width;
+   mf-height = wsize-height;
+   switch (mf-code) {
+   case V4L2_MBUS_FMT_RGB565_2X8_LE:
+   mf-colorspace = V4L2_COLORSPACE_SRGB;
+   if (ret_fmt != NULL)
+   *ret_fmt = ov7670_formats + 2;
+   break;
+   default:
+   mf-code = V4L2_MBUS_FMT_YUYV8_2X8;
+   case V4L2_MBUS_FMT_YUYV8_2X8:
+   mf-colorspace = V4L2_COLORSPACE_JPEG;
+   if (ret_fmt != NULL)
+   *ret_fmt = ov7670_formats;
+   break;
+   }
return 0;
 }
 
-static int ov7670_try_fmt(struct v4l2_subdev *sd, struct v4l2_format *fmt)
+static int ov7670_try_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt 
*mf)
 {
-   return

pxa-camera: build error 2.6.32-rc4

2009-10-12 Thread Jonathan Cameron 

Error was 

drivers/media/video/pxa_camera.c: In function 'pxa_camera_wakeup':
drivers/media/video/pxa_camera.c:683: error: 'TASK_NORMAL' undeclared (first 
use in this function)
drivers/media/video/pxa_camera.c:683: error: (Each undeclared identifier is 
reported only once
drivers/media/video/pxa_camera.c:683: error: for each function it appears in.)
  CC [M]  drivers/pcmcia/soc_common.o

Line in question is

wake_up(vb-done);
in pxa_camera_wakeup.

Looks like issue is lack of inclusion of sched.h.

Right now I'm having trouble tracking down why this became and issue since 
2.6.32-rc3.

Might be related to
Staging: comedi: Add include of linux/sched.h to fix build
which is down to removal of sched.h from poll.h
commmit a99bbaf5ee6bad1aca0c88ea65ec6e5373e86184
Looks like media/v4l2-dev.h and others include poll.h so I'm guessing
we were original getting it from there.

I'm happy to post a patch but was wondering if anyone else has seen this or has 
tracked down
exactly what changed, not to mention if this is a more general problem? (or for 
that matter
already fixed and I just missed it.)

Thanks

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pxa-camera: build error 2.6.32-rc4

2009-10-12 Thread Jonathan Cameron 
Jonathan Cameron wrote:
 Error was 
 
 drivers/media/video/pxa_camera.c: In function 'pxa_camera_wakeup':
 drivers/media/video/pxa_camera.c:683: error: 'TASK_NORMAL' undeclared (first 
 use in this function)
 drivers/media/video/pxa_camera.c:683: error: (Each undeclared identifier is 
 reported only once
 drivers/media/video/pxa_camera.c:683: error: for each function it appears in.)
   CC [M]  drivers/pcmcia/soc_common.o
 
 Line in question is
 
 wake_up(vb-done);
 in pxa_camera_wakeup.
 
 Looks like issue is lack of inclusion of sched.h.
 
 Right now I'm having trouble tracking down why this became and issue since 
 2.6.32-rc3.
 
 Might be related to
 Staging: comedi: Add include of linux/sched.h to fix build
 which is down to removal of sched.h from poll.h
 commmit a99bbaf5ee6bad1aca0c88ea65ec6e5373e86184
 Looks like media/v4l2-dev.h and others include poll.h so I'm guessing
 we were original getting it from there.
 
 I'm happy to post a patch but was wondering if anyone else has seen this or 
 has tracked down
 exactly what changed, not to mention if this is a more general problem? (or 
 for that matter
 already fixed and I just missed it.)

Just read lkml, Ingo Molnar has posted a whole set of patches (in reply to 
Linus' 2.6.32-rc4 thread)
dealing with this issue in a number of other places.  This one isn't there 
however.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: soc-camera: Handling hardware reset?

2009-09-02 Thread Jonathan Cameron 
Guennadi Liakhovetski wrote:
 On Tue, 1 Sep 2009, Jonathan Cameron wrote:
 
 Dear all,

 With an ov7670 I have been using soc_camera_link.reset to pass in board 
 specific
 hardware reset of the sensor. (Is this a correct usage? The reset must occur
 before the chip is used.)

 Unfortunately this function is called on every initialization of the camera
 (so on probe and before taking images). Basically any call to open()

 This would be fine if the v4l2_subdev_core_ops.init  was called after every
 call of this (ensuring valid state post reset). 

 Previously I was using the soc_camera_ops.init to call the core init function
 thus putting the register values back before capturing, but now it's gone 
 from
 the interface.

 What is the right way to do this?
 
 The idea is, that we're trying to save power, as long as noone is using 
 the camera, i.e., between the last close and the next open. But some 
 boards might not implement the power callback, so, to make the situation 
 equal for all, we also added a reset call on every first open. So, this is 
 exactly your case. Imagine, your camera driver has to work on a platform, 
 where power callbacks are implemented. So, in your .s_fmt() (or the new 
 .s_imgbus_fmt()) method, which is always called on the first open, you 
 have to be able to configure the chip after a fresh power-on.
In general that seems sensible.

I haven't looked at your imgbus patches yet (will do in a few days).

I've copied in those who might have an opinion on adding a rewrite
of all registers to s_fmt.  Current aim is to keep changes needed for
soc-camera support to an absolute minimum as I don't have access to
the other hardware that uses this driver.  So Jonathan / Hans,
would adding a call to ov7670_init inside s_fmt be alright with you?

The solution works, but seems like overkill to me, if soc-camera is
going to make a call to reset whilst other users don't then things
are going to get a little unpredictable.

 
 (isn't this a job for runtime-pm?...)
Probably, but in that case you would have relevant call back in place
to ensure that all relevant registers were in place (rather than putting
it in the fmt setting code). I'm guessing moving over to that might leave
a lot of broken drivers.

For reference, current patch (against v4l-next of yesterday) is below.

Thanks,

Jonathan

---

diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
index 0e2184e..3f80932 100644
--- a/drivers/media/video/ov7670.c
+++ b/drivers/media/video/ov7670.c
@@ -19,6 +19,7 @@
 #include media/v4l2-chip-ident.h
 #include media/v4l2-i2c-drv.h
 
+#include media/soc_camera.h
 
 MODULE_AUTHOR(Jonathan Corbet cor...@lwn.net);
 MODULE_DESCRIPTION(A low-level driver for OmniVision ov7670 sensors);
@@ -745,6 +746,10 @@ static int ov7670_s_fmt(struct v4l2_subdev *sd, struct 
v4l2_format *fmt)
struct ov7670_info *info = to_state(sd);
unsigned char com7, clkrc = 0;
 
+   ret = ov7670_init(sd, 0);
+   if (ret)
+   return ret;
+
ret = ov7670_try_fmt_internal(sd, fmt, ovfmt, wsize);
if (ret)
return ret;
@@ -1239,6 +1244,43 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 };
 
 /* --- */
+#ifdef CONFIG_SOC_CAMERA
+static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device *icd)
+{
+   struct soc_camera_link *icl = to_soc_camera_link(icd);
+
+   unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
+   SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+   SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
+
+   return soc_camera_apply_sensor_flags(icl, flags);
+}
+
+/* This device only supports one bus option */
+static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
+   unsigned long flags)
+{
+   return 0;
+}
+
+static struct soc_camera_ops ov7670_soc_ops = {
+   .set_bus_param = ov7670_soc_set_bus_param,
+   .query_bus_param = ov7670_soc_query_bus_param,
+};
+
+#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
+static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
+   {
+   SETFOURCC(YUYV),
+   .depth = 16,
+   .colorspace = V4L2_COLORSPACE_JPEG,
+   }, {
+   SETFOURCC(RGB565),
+   .depth = 16,
+   .colorspace = V4L2_COLORSPACE_SRGB,
+   },
+};
+#endif
 
 static int ov7670_probe(struct i2c_client *client,
const struct i2c_device_id *id)
@@ -1246,7 +1288,18 @@ static int ov7670_probe(struct i2c_client *client,
struct v4l2_subdev *sd;
struct ov7670_info *info;
int ret;
+#ifdef CONFIG_SOC_CAMERA
+   struct soc_camera_device *icd = client-dev.platform_data;
 
+   if (!icd) {
+   dev_err(client-dev, OV7670: missing soc-camera data!\n);
+   return -EINVAL

Re: soc-camera: status, roadmap

2009-07-03 Thread Jonathan Cameron 
Guennadi Liakhovetski wrote:
 On Wed, 10 Jun 2009, Guennadi Liakhovetski wrote:
 
 2. The above means, I'll have to maintain and update my patches for a 
 whole 2.6.31 development cycle. In this time I'll try to update and upload 
 them as a quilt patch series and announce it on the list a couple of 
 times.
 
 As promised, I just uploaded my current tree snapshot at 
 http://download.open-technology.de/soc-camera/20090617/
 This is nothing remarkable, just my current patch-stack for those working 
 with the soc-camera framework. It is still based on a linux-next snapshot 
 of 07.05.2009 history branch. The exact commit on which the stack is 
 based is, as usual, in -base. This is still based off 2.6.30-rc4, and 
 I expect to upgrade next time after 2.6.31-rc1.

Hi Guennadi, 

I notice you've posted newer version of these patches on your website. 20090701/
I was wondering what tree these are based on?
I can't seem to track down the base commit.

Thanks,

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OV7670: getting it working with soc-camera.

2009-06-19 Thread Jonathan Cameron 
Hi All,

Turns out that, in addition the below, one more soc-camera ops
function is still needed.

Init is required in my case to make a call to the subdev-ops-core_ops-init
function in order to put the register contents back after the reset callback
in soc_camera_open.

I'm guessing down the line it will make sense to call this directly in 
soc_camera
or are the circumstances where it should call additional init functions?




 Updated temporary patch to get ov7670 working with soc camera.
 
 ---
 Basically this is the original patch with the changes Guennadi
 suggested. Again this is only for info, not a formal patch submission.
 
 
 diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
 index 0e2184e..9bea804 100644
 --- a/drivers/media/video/ov7670.c
 +++ b/drivers/media/video/ov7670.c
 @@ -19,6 +19,12 @@
  #include media/v4l2-chip-ident.h
  #include media/v4l2-i2c-drv.h
  
 +#define OV7670_SOC
 +
 +
 +#ifdef OV7670_SOC
 +#include media/soc_camera.h
 +#endif /* OV7670_SOC */
  
  MODULE_AUTHOR(Jonathan Corbet cor...@lwn.net);
  MODULE_DESCRIPTION(A low-level driver for OmniVision ov7670 sensors);
 @@ -1239,13 +1245,58 @@ static const struct v4l2_subdev_ops ov7670_ops = {
  };
  
  /* --- */
 +#ifdef OV7670_SOC
 +
 +static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device 
 *icd)
 +{
 + struct soc_camera_link *icl = to_soc_camera_link(icd);
 +
 + unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
 + SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
 + SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
 +
 + return soc_camera_apply_sensor_flags(icl, flags);
 +}
 +/* This device only supports one bus option */
 +static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
 + unsigned long flags)
 +{
 + return 0;
 +}
 +
 +static struct soc_camera_ops ov7670_soc_ops = {
 + .set_bus_param = ov7670_soc_set_bus_param,
 + .query_bus_param = ov7670_soc_query_bus_param,
 +};
  
 +#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
 +static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
 + {
 + SETFOURCC(YUYV),
 + .depth = 16,
 + .colorspace = V4L2_COLORSPACE_JPEG,
 + }, {
 + SETFOURCC(RGB565),
 + .depth = 16,
 + .colorspace = V4L2_COLORSPACE_SRGB,
 + },
 +};
 +
 +#endif
  static int ov7670_probe(struct i2c_client *client,
   const struct i2c_device_id *id)
  {
   struct v4l2_subdev *sd;
   struct ov7670_info *info;
   int ret;
 +#ifdef OV7670_SOC
 + struct soc_camera_device *icd = client-dev.platform_data;
 + icd-ops = ov7670_soc_ops;
 + icd-rect_max.width = VGA_WIDTH;
 + icd-rect_max.height = VGA_HEIGHT;
 + icd-formats = ov7670_soc_fmt_lists;
 + icd-num_formats = ARRAY_SIZE(ov7670_soc_fmt_lists);
 +#endif
  
   info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
   if (info == NULL)
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OV7670: getting it working with soc-camera.

2009-06-18 Thread Jonathan Cameron 
Guennadi Liakhovetski wrote:
 On Wed, 17 Jun 2009, Jonathan Cameron wrote:
 
 This is purely for info of anyone else wanting to use the ov7670
 with Guennadi's recent work on converted soc-camera to v4l2-subdevs.

 It may not be completely minimal, but it's letting me take pictures ;)
 
 Cool, I like it! Not the pictures, but the fact that the required patch 
 turned out to be so small. Of course, you understand this is not what 
 we'll eventually commit, but, I think, this is a good start. In principle, 
 if a device has all parameters fixed, there's no merit in trying to set 
 them.
Yup, my intention is to slowly remove elements as they become unnecessary
(and push any that actually make sense to the mailing list).

 
 Couple of minor queries:

 Currently it is assumed that there is a means of telling the chip to
 use particular bus params.  In the case of this one it doesn't support
 anything other than 8 bit. Stuff may get added down the line, but
 in meantime does anyone mind if we make icd-ops-set_bus_param
 optional in soc-camera?
 
 struct soc_camera_ops will disappear completely anyway, and we don't know 
 yet what the v4l2-subdev counterpart will look like.
 
Sure, I'll wait and see whether this question is relevant down the line.

...

 Or for that matter why the address is right shifted by
 1 in:

 v4l_info(client, chip found @ 0x%02x (%s)\n,
   client-addr  1, client-adapter-name);

 Admittedly the data sheet uses an 'unusual' convention for the
 address (separate write and read address which correspond to
 a single address of 0x21 with the relevant write bit set as
 appropriate).
 
 That's exactly the reason, I think. Many (or most?) datasheets specify i2c 
 addresses which are a double of Linux i2c address. IIRC this is just a 
 Linux convention to use the shifted address.

Um. I'm not sure I agree with this.  The convention when specifying the
address in registration is to use correct one (without the write bit)
and based on a lot of non video chips I've come across is about 50 / 50
on how they document it (with many using a delightful and random mix of the two)
If you are going to have a registration scheme that requires the board code
to specify the address as 0x21 then to my mind having the driver declare
it as being on 0x42 seems rather odd and misleading.

This is particularly true here where the driver is using smbus calls as
that specification is very clear indeed on the fact that addresses are 7 bit.
Admittedly this chip uses the sccb bus protocol that just 'happens'
to bare a startling resemblance to smbus / i2c.

Still this isn't exactly a crucial element of the driver!
 
 As ever any comments welcome. Thanks to Guennadi Liakhovetski
 for his soc-camera work and Hans Verkuil for conversion of the
 ov7670 to soc-dev.

 Tested against a merge of todays v4l-next tree and Linus' current
 with the minor pxa-camera bug I posted earlier fixed and Guennadi's
 extensive patch set applied (this requires a few hand merges, but
 nothing too nasty).
 
 Good to know.
 
 A couple of comments:
 

...
 +#endif
 
 ...and this switching. All this should be done in struct soc_camera_link 
 .power() and .reset() methods in your platform code.
Ah, I'd missed those methods, thanks!

You are quite right about the i2c_device_table as well, not sure what
got into me there.

Thanks,

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OV7670: getting it working with soc-camera.

2009-06-18 Thread Jonathan Cameron 
Updated temporary patch to get ov7670 working with soc camera.

---
Basically this is the original patch with the changes Guennadi
suggested. Again this is only for info, not a formal patch submission.


diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
index 0e2184e..9bea804 100644
--- a/drivers/media/video/ov7670.c
+++ b/drivers/media/video/ov7670.c
@@ -19,6 +19,12 @@
 #include media/v4l2-chip-ident.h
 #include media/v4l2-i2c-drv.h
 
+#define OV7670_SOC
+
+
+#ifdef OV7670_SOC
+#include media/soc_camera.h
+#endif /* OV7670_SOC */
 
 MODULE_AUTHOR(Jonathan Corbet cor...@lwn.net);
 MODULE_DESCRIPTION(A low-level driver for OmniVision ov7670 sensors);
@@ -1239,13 +1245,58 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 };
 
 /* --- */
+#ifdef OV7670_SOC
+
+static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device *icd)
+{
+   struct soc_camera_link *icl = to_soc_camera_link(icd);
+
+   unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
+   SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+   SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
+
+   return soc_camera_apply_sensor_flags(icl, flags);
+}
+/* This device only supports one bus option */
+static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
+   unsigned long flags)
+{
+   return 0;
+}
+
+static struct soc_camera_ops ov7670_soc_ops = {
+   .set_bus_param = ov7670_soc_set_bus_param,
+   .query_bus_param = ov7670_soc_query_bus_param,
+};
 
+#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
+static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
+   {
+   SETFOURCC(YUYV),
+   .depth = 16,
+   .colorspace = V4L2_COLORSPACE_JPEG,
+   }, {
+   SETFOURCC(RGB565),
+   .depth = 16,
+   .colorspace = V4L2_COLORSPACE_SRGB,
+   },
+};
+
+#endif
 static int ov7670_probe(struct i2c_client *client,
const struct i2c_device_id *id)
 {
struct v4l2_subdev *sd;
struct ov7670_info *info;
int ret;
+#ifdef OV7670_SOC
+   struct soc_camera_device *icd = client-dev.platform_data;
+   icd-ops = ov7670_soc_ops;
+   icd-rect_max.width = VGA_WIDTH;
+   icd-rect_max.height = VGA_HEIGHT;
+   icd-formats = ov7670_soc_fmt_lists;
+   icd-num_formats = ARRAY_SIZE(ov7670_soc_fmt_lists);
+#endif
 
info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
if (info == NULL)
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] soc-camera: ov7670 merged multiple drivers and moved over to v4l2-subdev

2009-06-17 Thread Jonathan Cameron 
Hans Verkuil wrote:
 On Tuesday 16 June 2009 16:45:04 Jonathan Cameron wrote:
 Guennadi Liakhovetski wrote:
 On Mon, 15 Jun 2009, Jonathan Cameron wrote:
 From: Jonathan Cameron ji...@cam.ac.uk

 OV7670 soc-camera driver. Merge of drivers from Jonathan Corbet,
 Darius Augulis and Jonathan Cameron
 Could you please, describe in more detail how you merged them?
 Mostly by combining the various register sets and then adding pretty much
 all the functionality in each of them, testing pretty much everything.

 Note that a lot of what was in those drivers (usually labeled as
 untested) simply doesn't work and is based on 'magic' register sets
 provided by omnivision.

 However, I am not sure this is the best way to go. I think, a better
 approach would be to take a driver currently in the mainline, perhaps,
 the most feature-complete one if there are several of them there,
 That is more or less what I've done (it's based on Jonathan Corbet's
 driver). Darius' driver and mine have never been in mainline. Darius' was
 a complete rewrite based on doc's he has under NDA.  Mine was based on
 Jonathan Corbet's one with a few bits leveraged from a working tinyos
 driver for the platform I'm using (principally because Omnivision are
 ignoring both myself and the board supplier).

 convert
 it and its user(s) to v4l2-subdev, extend it with any features missing
 in it and present in other drivers, then switch users of all other
 ov7670 drivers over to this one,
 That's the problem. The only mainlined driver is specifically for an OLPC
 machine.  The driver is tied to specific i2c device and doesn't use
 anything anywhere near soc-camera or v4l2-subdev.
 
 Yes, it does. ov7670.c was converted to v4l2-subdev in 2.6.30.
Ah! thanks for the heads up on that.
Some how I missed that entirely.
 
 While it would be nice to get a single driver working
 for this hardware as well as more conventional soc-camera devices, it
 isn't going to happen without a lot of input from someone with an olpc. 
 The chip is interfaced through a Marvell 88alp101 'cafe' chip which does
 a whole host of random things alongside being video processor and taking
 a quick look at that would be written in a completely different fashion
 if it were done now (mfd with subdevices etc, v4l2-sudev)

 So basically in the ideal world it would happen exactly as you've
 suggested, but I doubt it'll happen any time soon and in the meantime
 there is no in kernel support for those of us using the chip on other
 platforms.

 *looks hopefully in the direction of Jonathan Corbet and other olpc
 owners*
 
 I have an olpc, but I still haven't had the time to setup a proper test 
 environment on it. However, I managed to clear my backlog of reviews in the 
 past few days so hopefully I can find some time this weekend to set it up.
Cool.
 
 and finally make it work with soc-camera. This
 way you get a series of smaller and reviewable patches, instead of a
 completely new driver, that reproduces a lot of existing code but has
 to be reviewed anew. How does this sound?
 Would be fine if the original driver (or anything terribly close to it)
 were useable on a platform I actually have without more or less being
 rewritten.

 I can back track the driver to be as close to that as possible and still
 functional, but I'm not entirely sure it will make the code any easier to
 review and you'll loose a lot the functionality lifted from Darius' as
 my original drivers.

 The original posting I made was as close as you can reasonably get to
 Jonathan's original driver.

 http://patchwork.kernel.org/patch/12192/

 At the time it wasn't really reviewed (beyond a few comments)
 as you were just commencing the soc-camera conversion and it made
 sense to wait for after that.

 I'm not really sure how we should proceed with this. I'm particularly
 loath to touch the olpc driver unless we have a reasonable number of
 people willing to test.
 
 The current ov7670.c driver in the 2.6.30 tree (and the v4l-dvb repository) 
 is already v4l2_subdev based and should be reusable in other platforms, 
 including soc-camera once that has also been converted to use v4l2_subdev.
Excellent, I'll look at moving the functionality patches based on this driver
and Darius' on to that then.  Sorry for repeating some of your work, I somehow
completely forgot your original email saying this was on it's way!

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

typo: v4l2_bound_align_image name mismatch.

2009-06-17 Thread Jonathan Cameron 
Just came across a build error with pxa_camera with Mauro's linux-next tree.

pxa-camera calls v4l2_bound_align_image whereas the function is called
v4l_bound_align_image.  

Cheers,

---
Jonathan Cameron
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

OV7670: getting it working with soc-camera.

2009-06-17 Thread Jonathan Cameron 
This is purely for info of anyone else wanting to use the ov7670
with Guennadi's recent work on converted soc-camera to v4l2-subdevs.

It may not be completely minimal, but it's letting me take pictures ;)

Couple of minor queries:

Currently it is assumed that there is a means of telling the chip to
use particular bus params.  In the case of this one it doesn't support
anything other than 8 bit. Stuff may get added down the line, but
in meantime does anyone mind if we make icd-ops-set_bus_param
optional in soc-camera?

Is there any reason (or advantage) in not specifying the i2c address
in the driver? Or for that matter why the address is right shifted by
1 in:

v4l_info(client, chip found @ 0x%02x (%s)\n,
 client-addr  1, client-adapter-name);

Admittedly the data sheet uses an 'unusual' convention for the
address (separate write and read address which correspond to
a single address of 0x21 with the relevant write bit set as
appropriate).

As ever any comments welcome. Thanks to Guennadi Liakhovetski
for his soc-camera work and Hans Verkuil for conversion of the
ov7670 to soc-dev.

Tested against a merge of todays v4l-next tree and Linus' current
with the minor pxa-camera bug I posted earlier fixed and Guennadi's
extensive patch set applied (this requires a few hand merges, but
nothing too nasty).

---

diff --git a/include/media/ov7670_soc.h b/include/media/ov7670_soc.h
new file mode 100644
index 000..2f264b2
--- /dev/null
+++ b/include/media/ov7670_soc.h
@@ -0,0 +1,21 @@
+/* ov7670_soc Camera
+ *
+ * Copyright (C) 2009 Jonathan Cameron ji...@cam.ac.uk
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __OV7670_SOC_H__
+#define __OV7670_SOC_H__
+
+#include media/soc_camera.h
+
+struct ov7670_soc_camera_info {
+   int gpio_pwr;
+   int gpio_reset;
+   struct soc_camera_link link;
+};
+
+#endif
diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
index 0e2184e..51d432e 100644
--- a/drivers/media/video/ov7670.c
+++ b/drivers/media/video/ov7670.c
@@ -19,7 +19,14 @@
 #include media/v4l2-chip-ident.h
 #include media/v4l2-i2c-drv.h
 
+#define OV7670_SOC
 
+
+#ifdef OV7670_SOC
+#include media/ov7670_soc.h
+#include media/soc_camera.h
+#include linux/gpio.h
+#endif /* OV7670_SOC */
 MODULE_AUTHOR(Jonathan Corbet cor...@lwn.net);
 MODULE_DESCRIPTION(A low-level driver for OmniVision ov7670 sensors);
 MODULE_LICENSE(GPL);
@@ -1239,19 +1246,94 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 };
 
 /* --- */
+#ifdef OV7670_SOC
+
+static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device *icd)
+{
+   struct soc_camera_link *icl = to_soc_camera_link(icd);
+
+   unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
+   SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+   SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
+
+   return soc_camera_apply_sensor_flags(icl, flags);
+}
+/* This device only supports one bus option */
+static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
+   unsigned long flags)
+{
+   return 0;
+}
+
+static struct soc_camera_ops ov7670_soc_ops = {
+   .set_bus_param = ov7670_soc_set_bus_param,
+   .query_bus_param = ov7670_soc_query_bus_param,
+};
+
+#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
+static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
+   {
+   SETFOURCC(YUYV),
+   .depth = 16,
+   .colorspace = V4L2_COLORSPACE_JPEG,
+   }, {
+   SETFOURCC(RGB565),
+   .depth = 16,
+   .colorspace = V4L2_COLORSPACE_SRGB,
+   },
+};
 
+#endif
 static int ov7670_probe(struct i2c_client *client,
const struct i2c_device_id *id)
 {
+#ifdef OV7670_SOC
+   struct soc_camera_device *icd = client-dev.platform_data;
+   struct soc_camera_link *icl;
+   struct ov7670_soc_camera_info *board_info;
+#endif
struct v4l2_subdev *sd;
struct ov7670_info *info;
+
int ret;
 
+#ifdef OV7670_SOC
+   icl = to_soc_camera_link(icd);
+   if (!icl)
+   return -EINVAL;
+   board_info = container_of(icl, struct ov7670_soc_camera_info, link);
+
+   gpio_request(board_info-gpio_reset, ov7670 soc reset);
+   gpio_request(board_info-gpio_pwr, ov7670 soc power);
+
+   /* reset high for normal mode */
+   gpio_direction_output(board_info-gpio_reset, 1);
+   /* power down normal mode. */
+   gpio_direction_output(board_info-gpio_pwr, 0);
+   /* perform a hard reset as per tinyos code */
+   gpio_set_value(board_info-gpio_pwr, 1);
+   gpio_set_value(board_info-gpio_reset, 1);
+   mdelay(2

Re: [PATCH] soc-camera: ov7670 merged multiple drivers and moved over to v4l2-subdev

2009-06-16 Thread Jonathan Cameron 
Guennadi Liakhovetski wrote:
 On Mon, 15 Jun 2009, Jonathan Cameron wrote:
 
 From: Jonathan Cameron ji...@cam.ac.uk

 OV7670 soc-camera driver. Merge of drivers from Jonathan Corbet,
 Darius Augulis and Jonathan Cameron
 
 Could you please, describe in more detail how you merged them?
Mostly by combining the various register sets and then adding pretty much
all the functionality in each of them, testing pretty much everything.

Note that a lot of what was in those drivers (usually labeled as untested)
simply doesn't work and is based on 'magic' register sets provided by
omnivision.
 
 However, I am not sure this is the best way to go. I think, a better 
 approach would be to take a driver currently in the mainline, perhaps, 
 the most feature-complete one if there are several of them there,
That is more or less what I've done (it's based on Jonathan Corbet's driver).
Darius' driver and mine have never been in mainline. Darius' was a complete
rewrite based on doc's he has under NDA.  Mine was based on Jonathan
Corbet's one with a few bits leveraged from a working tinyos driver for the
platform I'm using (principally because Omnivision are ignoring both myself
and the board supplier).
 
 convert 
 it and its user(s) to v4l2-subdev, extend it with any features missing in 
 it and present in other drivers, then switch users of all other ov7670 
 drivers over to this one,
That's the problem. The only mainlined driver is specifically for an OLPC
machine.  The driver is tied to specific i2c device and doesn't use anything
anywhere near soc-camera or v4l2-subdev.

While it would be nice to get a single driver working
for this hardware as well as more conventional soc-camera devices, it isn't
going to happen without a lot of input from someone with an olpc.  The chip
is interfaced through a Marvell 88alp101 'cafe' chip which does a whole host
of random things alongside being video processor and taking a quick look at
that would be written in a completely different fashion if it were done now
(mfd with subdevices etc, v4l2-sudev)

So basically in the ideal world it would happen exactly as you've suggested,
but I doubt it'll happen any time soon and in the meantime there is no in
kernel support for those of us using the chip on other platforms.

*looks hopefully in the direction of Jonathan Corbet and other olpc owners*

 and finally make it work with soc-camera. This 
 way you get a series of smaller and reviewable patches, instead of a 
 completely new driver, that reproduces a lot of existing code but has to 
 be reviewed anew. How does this sound?
Would be fine if the original driver (or anything terribly close to it)
were useable on a platform I actually have without more or less being rewritten.

I can back track the driver to be as close to that as possible and still
functional, but I'm not entirely sure it will make the code any easier to
review and you'll loose a lot the functionality lifted from Darius' as
my original drivers.

The original posting I made was as close as you can reasonably get to
Jonathan's original driver.

http://patchwork.kernel.org/patch/12192/

At the time it wasn't really reviewed (beyond a few comments)
as you were just commencing the soc-camera conversion and it made
sense to wait for after that.

I'm not really sure how we should proceed with this. I'm particularly
loath to touch the olpc driver unless we have a reasonable number of
people willing to test.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] soc-camera: ov7670 merged multiple drivers and moved over to v4l2-subdev

2009-06-16 Thread Jonathan Cameron 
Darius Augulis wrote:
 On 06/16/2009 05:45 PM, Jonathan Cameron wrote:
 Guennadi Liakhovetski wrote:
 On Mon, 15 Jun 2009, Jonathan Cameron wrote:

 From: Jonathan Cameron ji...@cam.ac.uk

 OV7670 soc-camera driver. Merge of drivers from Jonathan Corbet,
 Darius Augulis and Jonathan Cameron 
 Could you please, describe in more detail how you merged them? 
 Mostly by combining the various register sets and then adding pretty much
 all the functionality in each of them, testing pretty much everything.

 Note that a lot of what was in those drivers (usually labeled as
 untested)
 simply doesn't work and is based on 'magic' register sets provided by
 omnivision.

 However, I am not sure this is the best way to go. I think, a better
 approach would be to take a driver currently in the mainline, perhaps,
 the most feature-complete one if there are several of them there, 
 That is more or less what I've done (it's based on Jonathan Corbet's
 driver).
 Darius' driver and mine have never been in mainline. Darius' was a
 complete
 rewrite based on doc's he has under NDA. Mine was based on Jonathan
 Corbet's one with a few bits leveraged from a working tinyos driver
 for the
 platform I'm using (principally because Omnivision are ignoring both
 myself
 and the board supplier). 
 
 It's very difficult to write 'normal' driver for it.
 Omnivision does not provide useful documentation,
 only long constant arrays with few strange comments.
 Beside documentation is poor, there are lot of errors in register
 description.
 Many things are mistery, not documented and seems Omnivision does not
 have such documentation.
 I thing this sensor isn't perfect for embedded projects. It's 'designed'
 for webcams, where reliability and quality are not needed.
 With ov7720 similar situation...
Agreed.  Though random discussions with others suggest lots of these
chips turn up in things like pedestrian avoidance systems in cars
and similar.  (not generally running linux and tend to have fairly
fixed settings I guess).

Jonathan

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] soc-camera: ov7670 merged multiple drivers and moved over to v4l2-subdev

2009-06-15 Thread Jonathan Cameron 
From: Jonathan Cameron ji...@cam.ac.uk

OV7670 soc-camera driver. Merge of drivers from Jonathan Corbet,
Darius Augulis and Jonathan Cameron

Signed-off-by: Jonathan Cameron ji...@cam.ac.uk
---

This is my first cut at a merge of the various ov7670 drivers to work
with Guennadi Liakhovetski's work on moving soc-camera over to v4l2-subdev
framework.

Thanks to Darius Augulis for many of the register settings, though a few
sets marked as untested in his driver don't seem to work.

I'm not entirely happy with the mapping of various parameters onto
the standard v4l controls and would greatly appreciate any suggestions
about these.

There are still a lot of magic numbers in here but I've tried to identify
the purposes of as many as possible.

At the moment I've deliberately kept it separate in naming etc from the
in tree ov7670 driver as I don't want to go breaking that driver. Is there
anyone out there who has the hardware and would consider doing the relevant
board support code and testing?

All comments welcome.


 drivers/media/video/Kconfig  |6 +
 drivers/media/video/Makefile |1 +
 drivers/media/video/ov7670_soc.c | 1475 ++
 include/media/ov7670_soc.h   |   21 +
 4 files changed, 1503 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 9ff760c..4580d7a 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -746,6 +746,12 @@ config SOC_CAMERA_OV772X
help
  This is a ov772x camera driver
 
+config SOC_CAMERA_OV7670_SOC
+   tristate ov7670 soc camera support
+   depends on SOC_CAMERA  I2C
+   help
+ This is an ov7670 soc camera driver
+
 config MX1_VIDEO
bool
 
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 7aefac6..1249326 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -141,6 +141,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111)+= mt9m111.o
 obj-$(CONFIG_SOC_CAMERA_MT9T031)   += mt9t031.o
 obj-$(CONFIG_SOC_CAMERA_MT9V022)   += mt9v022.o
 obj-$(CONFIG_SOC_CAMERA_OV772X)+= ov772x.o
+obj-$(CONFIG_SOC_CAMERA_OV7670_SOC)+= ov7670_soc.o
 obj-$(CONFIG_SOC_CAMERA_PLATFORM)  += soc_camera_platform.o
 obj-$(CONFIG_SOC_CAMERA_TW9910)+= tw9910.o
 # soc-camera host drivers have to be linked after camera drivers
diff --git a/drivers/media/video/ov7670_soc.c b/drivers/media/video/ov7670_soc.c
new file mode 100644
index 000..5494427
--- /dev/null
+++ b/drivers/media/video/ov7670_soc.c
@@ -0,0 +1,1475 @@
+/*
+ * A V4L2 driver for OmniVision OV7670 cameras via soc interface
+ *
+ * Copyright 2006 One Laptop Per Child Association, Inc.  Written
+ * by Jonathan Corbet with substantial inspiration from Mark
+ * McClelland's ovcamchip code.
+ *
+ * Copyright 2006-7 Jonathan Corbet cor...@lwn.net
+ *
+ * Copyright 2008-9 Jonathan Cameron ji...@cam.ac.uk
+ *
+ * Copyright 2008 Darius Augulis augulis.dar...@gmail.com
+ *
+ * This file may be distributed under the terms of the GNU General
+ * Public License, version 2.
+ *
+ * Todo: Add control for auto saturation control
+ *   Inversion of sync signals etc.
+ *   Driver 2 had a qqvga mode, but register settings don't seem to
+ *   be right so I've removed it.
+ *
+ * Queries for review:
+ * 1) Here I'm using brightness controls for what are effectively shutter
+ * timings.  How should this be done?
+ */
+#include linux/init.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/videodev2.h
+#include media/v4l2-common.h
+#include media/v4l2-chip-ident.h
+#include linux/i2c.h
+#include linux/gpio.h
+#include linux/delay.h
+#include media/soc_camera.h
+#include media/ov7670_soc.h
+
+#define MAX_WIDTH  640
+#define MAX_HEIGHT 480
+
+/* Registers */
+#defineREG_GAIN0x00/* Gain lower 8 bits (rest in vref) */
+#define REG_BLUE   0x01/* blue gain */
+#define REG_RED0x02/* red gain */
+#define REG_VREF   0x03/* Pieces of GAIN, VSTART, VSTOP */
+#define REG_COM1   0x04/* Control 1 */
+#defineCOM1_CCIR656  0x40  /* CCIR656 enable */
+
+#define REG_AECHH  0x07/* AEC MS 5 bits */
+
+#define REG_COM2   0x09/* Control 2 */
+#defineCOM2_SSLEEP   0x10  /* Soft sleep mode */
+#define REG_PID0x0a/* Product ID MSB */
+#define REG_VER0x0b/* Product ID LSB */
+#define REG_COM3   0x0c/* Control 3 */
+#defineCOM3_SWAP 0x40/* Byte swap */
+#defineCOM3_SCALEEN  0x08/* Enable scaling */
+#defineCOM3_DCWEN0x04/* Enable 
downsamp/crop/window */
+#define REG_COM4   0x0d/* Control 4 */
+#define REG_COM5   0x0e/* All reserved */
+#define REG_COM6   0x0f/* Control 6 */
+#define REG_AECH   0x10/* More bits of AEC value

soc-camera: Why are exposure and gain handled via special cases?

2009-06-08 Thread Jonathan Cameron 
Hi All,

Whilst working on merging the various ov7670 drivers posted
recently, I came across the following in soc-camera:

static int soc_camera_g_ctrl(struct file *file, void *priv,
 struct v4l2_control *ctrl)
{
struct soc_camera_file *icf = file-private_data;
struct soc_camera_device *icd = icf-icd;
struct soc_camera_host *ici = to_soc_camera_host(icd-dev.parent);

WARN_ON(priv != file-private_data);

switch (ctrl-id) {
case V4L2_CID_GAIN:
if (icd-gain == (unsigned short)~0)
return -EINVAL;
ctrl-value = icd-gain;
return 0;
case V4L2_CID_EXPOSURE:
if (icd-exposure == (unsigned short)~0)
return -EINVAL;
ctrl-value = icd-exposure;
return 0;
}

return v4l2_device_call_until_err(ici-v4l2_dev, (__u32)icd, core, 
g_ctrl, ctrl);
}

Why are these two cases and only these two handled by soc-camera rather than 
being passed
on to the drivers?

Thanks,

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

OV7670 Soc-camera driver (anyone else want to merge versions?)

2009-05-27 Thread Jonathan Cameron 
Hi All,

The email is mainly in the interests of avoiding wasting time.

I'm currently updating the driver I posted alongside
Guennadi's soc-camera changes with the intention to also
merge features from the 3 drivers that are out there.

Unsurprisingly they all have different feature sets at the
moment and ideally we want to support everything the individual
drivers do.

Is anyone else doing or aiming to do the same?

Thanks,

---
Jonathan Cameron

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: ov7670 soc-camera driver

2009-03-16 Thread Jonathan Cameron 
Guennadi Liakhovetski wrote:
 On Sun, 15 Mar 2009, Hans Verkuil wrote:
 
 On Sunday 15 March 2009 19:50:39 Guennadi Liakhovetski wrote:
 On Sun, 15 Mar 2009, Jonathan Cameron wrote:
 From: Jonathan Cameron ji...@cam.ac.uk

 OV7670 driver for soc-camera interfaces.
 Much appreciated, thanks!

 ---
 There is already an ov7670 driver in tree, but it is very interface
 specific (olpc) and hence not much use for the crossbow IMB400 board
 which is plugged into an imote 2 pxa271 main board.
 
 [snip]
 
 Clearly this driver shares considerable portions of code with
 Jonathan Corbet's driver (in tree). It would be complex to combine
 the two drivers, but perhaps people feel this would be worthwhile?
 Now, there we go... Hans, time for v4l2-device?...

 This means, I will look through the driver, but we should really think
 properly whether to pull it in now, or just convert the OLPC driver and
 soc-camera to v4l2-device and thus enable re-use.
 I have already converted ov7670 to v4l2_subdev here:

 http://linuxtv.org/hg/~hverkuil/v4l-dvb-cafe2/

 I'm waiting for test feedback (Hi Corbet!) before I'll merge it.

 This situation is exactly why we need one single API for subdevices like 
 this. As soon as soc-camera is converted to v4l2-device it will just all 
 fall neatly into place.

 I don't think it is a good idea to merge a second ov7670 driver as that's 
 exactly what we are trying to avoid. Migrating soc-camera to the 
 v4l2-device/v4l2-subdev framework is the right approach. Otherwise this 
 issue will just crop up time and again.

 Although not good news for Jonathan, since his work will be delayed. 
 Jonathan, to get an idea what all of this is about you should read the 
 v4l2-framework.txt document in the master v4l-dvb repository 
 (linuxtv.org/hg/v4l-dvb). It will give you the background information on 
 the v4l2_subdev structure and associated API that we are migrating towards. 
 And soc-camera happens to a framework that hasn't been converted yet.
 
 Well, I don't think Jonathan's work will be quite in vain - it will 
 probably help having both drivers (soc-camera and v4l2-device) during the 
 porting for comparison and feature exchange. But I agree, that it wouldn't 
 be a right thing to merge this driver in the mainline now.
That's fair enough.  I'm having to maintain a couple of other drivers
out of tree under similar circumstances so one more doesn't make much 
difference.
Hardest bit of this driver was dealing with some board specific i2c quirks
anyway which I'd have had to do even if their was a suitable driver in tree.

For reference, any changes I make in the meantime will end up in 
http://git.kernel.org/?p=linux/kernel/git/jic23/imote2.git
 
 I am willing and planning to do (at least a part of) this conversion, the 
 only problem is that I don't have much free (as in beer:-)) time for it, 
Know that feeling, or I'd offer to help ;)  Happy to do some testing but
probably can't contribute any coding time.  Too much in the todo list right
now.

I'm not entirely clear how the subdev stuff will actually help, but I guess
that'll be come clear as the code progresses.
 and so far I don't see anyone outside queuing to finance this work:-) In 
 any case, looks like I'll have to pump up its priority and start working 
 on it asap. I only have to review the next version of PXA DMA conversion 
 patches, and then I'll declare a feature-freeze and just plunge into it. 
 Once started it will be finished some time:-)
Sounds, good.

Please keep me informed of progress (I'm not going to be that active
a reader of linux-media ;)

Thanks for the quick responses.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: ov7670 soc-camera driver

2009-03-16 Thread Jonathan Cameron 
Jonathan Corbet wrote:
 On Sun, 15 Mar 2009 17:10:01 +
 Jonathan Cameron ji...@cam.ac.uk wrote:
 
 The primary control on this chip related to shutter rate is actualy
 the frame rate. There are rather complex (and largerly undocumented)
 interactions between this setting and the auto brightness controls
 etc. Anyone have any suggestions on a better way of specifying this?
 
 Welcome to the world of the ov7670!  My conclusion, after working with
 this sensor, is that is consists of something like 150 analog tweakers
 disguised as digital registers.  Everything interacts with everything
 else, many of the settings are completely undocumented, and that's not
 to mention the weird multiplexor at 0x79.  It's hard to make this thing
 work if you don't have a blessed set of settings from OmniVision.
Hmm... And the grape vine / rumour says that they get most of their
'magic' values from customers who tweak the chips enough to get something
working.

Thanks for all the good work you put in.  Only other useful info was
the tinyos driver and that was a port of yours in the first place ;)

I'm particularly fond of the apparently obvious registers that won't
take a write unless something else is in a particular state.
 
 Clearly this driver shares considerable portions of code with
 Jonathan Corbet's driver (in tree). It would be complex to combine
 the two drivers, but perhaps people feel this would be worthwhile?
 
 I think it's necessary, really.  Having two drivers for the same device
 seems like a bad idea.  As Hans noted, he's already put quite a bit of
 work into generalizing the ov7670 driver; I think it would be best to
 work with him to get a driver that works for everybody.
That sounds like a good plan.  Now all we need is some time ;)

Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html