Re: [PATCH] [media] ov5645: I2C address change

[Laurent Pinchart]

Hi Todor,

On Monday, 9 October 2017 19:18:17 EEST Todor Tomov wrote:
> On  9.10.2017 15:52, Laurent Pinchart wrote:
> > On Monday, 9 October 2017 12:34:26 EEST Sakari Ailus wrote:
> >> On Mon, Oct 09, 2017 at 11:36:01AM +0300, Todor Tomov wrote:
> >>> On  4.10.2017 13:47, Laurent Pinchart wrote:
>  CC'ing the I2C mainling list and the I2C maintainer.
>  
>  On Wednesday, 4 October 2017 13:30:08 EEST Sakari Ailus wrote:
> > On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
> >> As soon as the sensor is powered on, change the I2C address to the
> >> one specified in DT. This allows to use multiple physical sensors
> >> connected to the same I2C bus.
> >> 
> >> Signed-off-by: Todor Tomov 
> > 
> > The smiapp driver does something similar and I understand Laurent
> > might be interested in such functionality as well.
> > 
> > It'd be nice to handle this through the I²C framework instead and to
> > define how the information is specified through DT. That way it could
> > be made generic, to work with more devices than just this one.
> > 
> > What do you think?
> >>> 
> >>> Thank you for this suggestion.
> >>> 
> >>> The way I have done it is to put the new I2C address in the DT and the
> >>> driver programs the change using the original I2C address. The original
> >>> I2C address is hardcoded in the driver. So maybe we can extend the DT
> >>> binding and the I2C framework so that both addresses come from the DT
> >>> and avoid hiding the original I2C address in the driver. This sounds
> >>> good to me.
> >> 
> >> Agreed.
> >> 
> >> In this case the address is known but in general that's not the case it's
> >> not that simple. There are register compatible devices that have
> >> different addresses even if they're the same devices.
> >> 
> >> It might be a good idea to make this explicit.
> > 
> > Yes, in the general case we need to specify the original address in DT, as
> > the chip could have a non-fixed boot-up I2C address.
> > 
> > In many cases the value of the new I2C address doesn't matter much, as
> > long as it's unique on the bus. I was thinking about implementing a
> > dynamic allocator for I2C addresses, but after discussing it with Wolfram
> > we concluded that it would probably not be a good idea. There could be
> > other I2C devices on the bus that Linux isn't aware of, in which case the
> > dynamic allocator could create address conflicts. Specifying the new
> > address in DT is likely a better idea, even if it could feel a bit more
> > of system configuration information than a pure hardware description.
> > 
> >>> Then changing the address could be device specific and also this must be
> >>> done right after power on so that there are no address conflicts. So I
> >>> don't think that we can support this through the I2C framework only, the
> >>> drivers that we want to do that will have to be expanded with this
> >>> functionality. Or do you have any other idea?
> >> 
> >> Yes, how the address is changed is always hardware specific. This would
> >> be most conveniently done in driver's probe or PM runtime_resume
> >> functions.
> > 
> > This patch modifies client->addr directly, which I don't think is a good
> > idea. I'd prefer making the I2C core aware of the address change through
> > an explicit API call. This would allow catching I2C adress conflicts for
> > instance.
> > 
> >> It could be as simple as providing an adapter specific mutex to serialise
> >> address changes on the bus so that no two address changes are taking
> >> place at the same time. Which is essentially the impliementation you had,
> >> only the mutex would be for the I²C adapter, not the driver. An helper
> >> functions for acquiring and releasing the mutex.
> > 
> > Why do you need to serialize address changes ?
> 
> Correct me if I'm wrong, but if you power on more than one device with the
> same I2C address and issue a command to change it, then all devices will
> recognize this command as addressed to them. The only solution (which I know
> about) to avoid this is to serialize the power on and address change (as a
> whole!) for these devices.

Yes, that's correct. It can be even worse than that, sometimes only one of the 
two devices with the same address can be reconfigured, which means that 
powering that device requires powering up the other device and changing its 
address first, otherwise the second device can't be used as long as the first 
one is power on (this happened for real in a Nokia platform).

> I think it would be better to move the mutex out of the driver - to avoid
> all client drivers which will change I2C address to add a global variable
> mutex for this. We just have to find a better place for it :)

The biggest issue I see is that there's no C code that has knowledge of the 
whole platform. It's hard to describe this in DT in a generic way, board files 
were clearly useful for this kind 

Re: [PATCH] [media] ov5645: I2C address change

[Todor Tomov]

Hi Laurent :)

On  9.10.2017 15:52, Laurent Pinchart wrote:
> Hello,
> 
> On Monday, 9 October 2017 12:34:26 EEST Sakari Ailus wrote:
>> On Mon, Oct 09, 2017 at 11:36:01AM +0300, Todor Tomov wrote:
>>> On  4.10.2017 13:47, Laurent Pinchart wrote:
 CC'ing the I2C mainling list and the I2C maintainer.

 On Wednesday, 4 October 2017 13:30:08 EEST Sakari Ailus wrote:
> On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
>> As soon as the sensor is powered on, change the I2C address to the one
>> specified in DT. This allows to use multiple physical sensors
>> connected to the same I2C bus.
>>
>> Signed-off-by: Todor Tomov 
>
> The smiapp driver does something similar and I understand Laurent might
> be interested in such functionality as well.
>
> It'd be nice to handle this through the I²C framework instead and to
> define how the information is specified through DT. That way it could
> be made generic, to work with more devices than just this one.
>
> What do you think?
>>>
>>> Thank you for this suggestion.
>>>
>>> The way I have done it is to put the new I2C address in the DT and the
>>> driver programs the change using the original I2C address. The original
>>> I2C address is hardcoded in the driver. So maybe we can extend the DT
>>> binding and the I2C framework so that both addresses come from the DT and
>>> avoid hiding the original I2C address in the driver. This sounds good to
>>> me.
>>
>> Agreed.
>>
>> In this case the address is known but in general that's not the case it's
>> not that simple. There are register compatible devices that have different
>> addresses even if they're the same devices.
>>
>> It might be a good idea to make this explicit.
> 
> Yes, in the general case we need to specify the original address in DT, as 
> the 
> chip could have a non-fixed boot-up I2C address.
> 
> In many cases the value of the new I2C address doesn't matter much, as long 
> as 
> it's unique on the bus. I was thinking about implementing a dynamic allocator 
> for I2C addresses, but after discussing it with Wolfram we concluded that it 
> would probably not be a good idea. There could be other I2C devices on the 
> bus 
> that Linux isn't aware of, in which case the dynamic allocator could create 
> address conflicts. Specifying the new address in DT is likely a better idea, 
> even if it could feel a bit more of system configuration information than a 
> pure hardware description.
> 
>>> Then changing the address could be device specific and also this must be
>>> done right after power on so that there are no address conflicts. So I
>>> don't think that we can support this through the I2C framework only, the
>>> drivers that we want to do that will have to be expanded with this
>>> functionality. Or do you have any other idea?
>>
>> Yes, how the address is changed is always hardware specific. This would be
>> most conveniently done in driver's probe or PM runtime_resume functions.
> 
> This patch modifies client->addr directly, which I don't think is a good 
> idea. 
> I'd prefer making the I2C core aware of the address change through an 
> explicit 
> API call. This would allow catching I2C adress conflicts for instance.
> 
>> It could be as simple as providing an adapter specific mutex to serialise
>> address changes on the bus so that no two address changes are taking place
>> at the same time. Which is essentially the impliementation you had, only
>> the mutex would be for the I²C adapter, not the driver. An helper functions
>> for acquiring and releasing the mutex.
> 
> Why do you need to serialize address changes ?

Correct me if I'm wrong, but if you power on more than one device with the
same I2C address and issue a command to change it, then all devices will
recognize this command as addressed to them. The only solution (which I know
about) to avoid this is to serialize the power on and address change (as a 
whole!)
for these devices.

I think it would be better to move the mutex out of the driver - to avoid all
client drivers which will change I2C address to add a global variable mutex 
for this. We just have to find a better place for it :)

> 
>> I wonder what others think.
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH] [media] ov5645: I2C address change

[Laurent Pinchart]

Hello,

On Monday, 9 October 2017 12:34:26 EEST Sakari Ailus wrote:
> On Mon, Oct 09, 2017 at 11:36:01AM +0300, Todor Tomov wrote:
> > On  4.10.2017 13:47, Laurent Pinchart wrote:
> >> CC'ing the I2C mainling list and the I2C maintainer.
> >> 
> >> On Wednesday, 4 October 2017 13:30:08 EEST Sakari Ailus wrote:
> >>> On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
>  As soon as the sensor is powered on, change the I2C address to the one
>  specified in DT. This allows to use multiple physical sensors
>  connected to the same I2C bus.
>  
>  Signed-off-by: Todor Tomov 
> >>> 
> >>> The smiapp driver does something similar and I understand Laurent might
> >>> be interested in such functionality as well.
> >>> 
> >>> It'd be nice to handle this through the I²C framework instead and to
> >>> define how the information is specified through DT. That way it could
> >>> be made generic, to work with more devices than just this one.
> >>> 
> >>> What do you think?
> > 
> > Thank you for this suggestion.
> > 
> > The way I have done it is to put the new I2C address in the DT and the
> > driver programs the change using the original I2C address. The original
> > I2C address is hardcoded in the driver. So maybe we can extend the DT
> > binding and the I2C framework so that both addresses come from the DT and
> > avoid hiding the original I2C address in the driver. This sounds good to
> > me.
> 
> Agreed.
> 
> In this case the address is known but in general that's not the case it's
> not that simple. There are register compatible devices that have different
> addresses even if they're the same devices.
> 
> It might be a good idea to make this explicit.

Yes, in the general case we need to specify the original address in DT, as the 
chip could have a non-fixed boot-up I2C address.

In many cases the value of the new I2C address doesn't matter much, as long as 
it's unique on the bus. I was thinking about implementing a dynamic allocator 
for I2C addresses, but after discussing it with Wolfram we concluded that it 
would probably not be a good idea. There could be other I2C devices on the bus 
that Linux isn't aware of, in which case the dynamic allocator could create 
address conflicts. Specifying the new address in DT is likely a better idea, 
even if it could feel a bit more of system configuration information than a 
pure hardware description.

> > Then changing the address could be device specific and also this must be
> > done right after power on so that there are no address conflicts. So I
> > don't think that we can support this through the I2C framework only, the
> > drivers that we want to do that will have to be expanded with this
> > functionality. Or do you have any other idea?
> 
> Yes, how the address is changed is always hardware specific. This would be
> most conveniently done in driver's probe or PM runtime_resume functions.

This patch modifies client->addr directly, which I don't think is a good idea. 
I'd prefer making the I2C core aware of the address change through an explicit 
API call. This would allow catching I2C adress conflicts for instance.

> It could be as simple as providing an adapter specific mutex to serialise
> address changes on the bus so that no two address changes are taking place
> at the same time. Which is essentially the impliementation you had, only
> the mutex would be for the I²C adapter, not the driver. An helper functions
> for acquiring and releasing the mutex.

Why do you need to serialize address changes ?

> I wonder what others think.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] ov5645: I2C address change

[Sakari Ailus]

Hi Todor,

On Mon, Oct 09, 2017 at 11:36:01AM +0300, Todor Tomov wrote:
> Hi Sakari,
> 
> On  4.10.2017 13:47, Laurent Pinchart wrote:
> > CC'ing the I2C mainling list and the I2C maintainer.
> > 
> > On Wednesday, 4 October 2017 13:30:08 EEST Sakari Ailus wrote:
> >> Hi Todor,
> >>
> >> On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
> >>> As soon as the sensor is powered on, change the I2C address to the one
> >>> specified in DT. This allows to use multiple physical sensors connected
> >>> to the same I2C bus.
> >>>
> >>> Signed-off-by: Todor Tomov 
> >>
> >> The smiapp driver does something similar and I understand Laurent might be
> >> interested in such functionality as well.
> >>
> >> It'd be nice to handle this through the I²C framework instead and to define
> >> how the information is specified through DT. That way it could be made
> >> generic, to work with more devices than just this one.
> >>
> >> What do you think?
> 
> Thank you for this suggestion.
> 
> The way I have done it is to put the new I2C address in the DT and the driver
> programs the change using the original I2C address. The original I2C address
> is hardcoded in the driver. So maybe we can extend the DT binding and the I2C
> framework so that both addresses come from the DT and avoid hiding the
> original I2C address in the driver. This sounds good to me.

Agreed.

In this case the address is known but in general that's not the case it's
not that simple. There are register compatible devices that have different
addresses even if they're the same devices.

It might be a good idea to make this explicit.

> 
> Then changing the address could be device specific and also this must be done
> right after power on so that there are no address conflicts. So I don't think
> that we can support this through the I2C framework only, the drivers that we
> want to do that will have to be expanded with this functionality. Or do you
> have any other idea?

Yes, how the address is changed is always hardware specific. This would be
most conveniently done in driver's probe or PM runtime_resume functions.

It could be as simple as providing an adapter specific mutex to serialise
address changes on the bus so that no two address changes are taking place
at the same time. Which is essentially the impliementation you had, only
the mutex would be for the I²C adapter, not the driver. An helper functions
for acquiring and releasing the mutex.

I wonder what others think.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH] [media] ov5645: I2C address change

[Todor Tomov]

Hi Sakari,

On  4.10.2017 13:47, Laurent Pinchart wrote:
> CC'ing the I2C mainling list and the I2C maintainer.
> 
> On Wednesday, 4 October 2017 13:30:08 EEST Sakari Ailus wrote:
>> Hi Todor,
>>
>> On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
>>> As soon as the sensor is powered on, change the I2C address to the one
>>> specified in DT. This allows to use multiple physical sensors connected
>>> to the same I2C bus.
>>>
>>> Signed-off-by: Todor Tomov 
>>
>> The smiapp driver does something similar and I understand Laurent might be
>> interested in such functionality as well.
>>
>> It'd be nice to handle this through the I²C framework instead and to define
>> how the information is specified through DT. That way it could be made
>> generic, to work with more devices than just this one.
>>
>> What do you think?

Thank you for this suggestion.

The way I have done it is to put the new I2C address in the DT and the driver
programs the change using the original I2C address. The original I2C address
is hardcoded in the driver. So maybe we can extend the DT binding and the I2C
framework so that both addresses come from the DT and avoid hiding the
original I2C address in the driver. This sounds good to me.

Then changing the address could be device specific and also this must be done
right after power on so that there are no address conflicts. So I don't think
that we can support this through the I2C framework only, the drivers that we
want to do that will have to be expanded with this functionality. Or do you
have any other idea?

Best regards,
Todor 

>>
>> Cc Laurent.
>>
>>> ---
>>>
>>>  drivers/media/i2c/ov5645.c | 42 
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
>>> index d28845f..8541109 100644
>>> --- a/drivers/media/i2c/ov5645.c
>>> +++ b/drivers/media/i2c/ov5645.c
>>> @@ -33,6 +33,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -42,6 +43,8 @@
>>>  #include 
>>>  #include 
>>>
>>> +static DEFINE_MUTEX(ov5645_lock);
>>> +
>>>  #define OV5645_VOLTAGE_ANALOG   280
>>>  #define OV5645_VOLTAGE_DIGITAL_CORE 150
>>>  #define OV5645_VOLTAGE_DIGITAL_IO   180
>>> @@ -590,6 +593,31 @@ static void ov5645_regulators_disable(struct ov5645
>>> *ov5645)
>>> dev_err(ov5645->dev, "io regulator disable failed\n");
>>>  }
>>>
>>> +static int ov5645_write_reg_to(struct ov5645 *ov5645, u16 reg, u8 val,
>>> +  u16 i2c_addr)
>>> +{
>>> +   u8 regbuf[3] = {
>>> +   reg >> 8,
>>> +   reg & 0xff,
>>> +   val
>>> +   };
>>> +   struct i2c_msg msgs = {
>>> +   .addr = i2c_addr,
>>> +   .flags = 0,
>>> +   .len = 3,
>>> +   .buf = regbuf
>>> +   };
>>> +   int ret;
>>> +
>>> +   ret = i2c_transfer(ov5645->i2c_client->adapter, , 1);
>>> +   if (ret < 0)
>>> +   dev_err(ov5645->dev,
>>> +   "%s: write reg error %d on addr 0x%x: reg=0x%x, 
>>> val=0x%x\n",
>>> +   __func__, ret, i2c_addr, reg, val);
>>> +
>>> +   return ret;
>>> +}
>>> +
>>>  static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
>>>  {
>>> u8 regbuf[3];
>>> @@ -729,10 +757,24 @@ static int ov5645_s_power(struct v4l2_subdev *sd,
>>> int on)
>>>  */
>>> if (ov5645->power_count == !on) {
>>> if (on) {
>>> +   mutex_lock(_lock);
>>> +
>>> ret = ov5645_set_power_on(ov5645);
>>> if (ret < 0)
>>> goto exit;
>>>
>>> +   ret = ov5645_write_reg_to(ov5645, 0x3100,
>>> +   ov5645->i2c_client->addr, 0x78);
>>> +   if (ret < 0) {
>>> +   dev_err(ov5645->dev,
>>> +   "could not change i2c address\n");
>>> +   ov5645_set_power_off(ov5645);
>>> +   mutex_unlock(_lock);
>>> +   goto exit;
>>> +   }
>>> +
>>> +   mutex_unlock(_lock);
>>> +
>>> ret = ov5645_set_register_array(ov5645,
>>> ov5645_global_init_setting,
>>> ARRAY_SIZE(ov5645_global_init_setting));
> 

Re: [PATCH] [media] ov5645: I2C address change

[Laurent Pinchart]

CC'ing the I2C mainling list and the I2C maintainer.

On Wednesday, 4 October 2017 13:30:08 EEST Sakari Ailus wrote:
> Hi Todor,
> 
> On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
> > As soon as the sensor is powered on, change the I2C address to the one
> > specified in DT. This allows to use multiple physical sensors connected
> > to the same I2C bus.
> > 
> > Signed-off-by: Todor Tomov 
> 
> The smiapp driver does something similar and I understand Laurent might be
> interested in such functionality as well.
> 
> It'd be nice to handle this through the I²C framework instead and to define
> how the information is specified through DT. That way it could be made
> generic, to work with more devices than just this one.
> 
> What do you think?
> 
> Cc Laurent.
> 
> > ---
> > 
> >  drivers/media/i2c/ov5645.c | 42 
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index d28845f..8541109 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -42,6 +43,8 @@
> >  #include 
> >  #include 
> > 
> > +static DEFINE_MUTEX(ov5645_lock);
> > +
> >  #define OV5645_VOLTAGE_ANALOG   280
> >  #define OV5645_VOLTAGE_DIGITAL_CORE 150
> >  #define OV5645_VOLTAGE_DIGITAL_IO   180
> > @@ -590,6 +593,31 @@ static void ov5645_regulators_disable(struct ov5645
> > *ov5645)
> > dev_err(ov5645->dev, "io regulator disable failed\n");
> >  }
> > 
> > +static int ov5645_write_reg_to(struct ov5645 *ov5645, u16 reg, u8 val,
> > +  u16 i2c_addr)
> > +{
> > +   u8 regbuf[3] = {
> > +   reg >> 8,
> > +   reg & 0xff,
> > +   val
> > +   };
> > +   struct i2c_msg msgs = {
> > +   .addr = i2c_addr,
> > +   .flags = 0,
> > +   .len = 3,
> > +   .buf = regbuf
> > +   };
> > +   int ret;
> > +
> > +   ret = i2c_transfer(ov5645->i2c_client->adapter, , 1);
> > +   if (ret < 0)
> > +   dev_err(ov5645->dev,
> > +   "%s: write reg error %d on addr 0x%x: reg=0x%x, 
> > val=0x%x\n",
> > +   __func__, ret, i2c_addr, reg, val);
> > +
> > +   return ret;
> > +}
> > +
> >  static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
> >  {
> > u8 regbuf[3];
> > @@ -729,10 +757,24 @@ static int ov5645_s_power(struct v4l2_subdev *sd,
> > int on)
> >  */
> > if (ov5645->power_count == !on) {
> > if (on) {
> > +   mutex_lock(_lock);
> > +
> > ret = ov5645_set_power_on(ov5645);
> > if (ret < 0)
> > goto exit;
> > 
> > +   ret = ov5645_write_reg_to(ov5645, 0x3100,
> > +   ov5645->i2c_client->addr, 0x78);
> > +   if (ret < 0) {
> > +   dev_err(ov5645->dev,
> > +   "could not change i2c address\n");
> > +   ov5645_set_power_off(ov5645);
> > +   mutex_unlock(_lock);
> > +   goto exit;
> > +   }
> > +
> > +   mutex_unlock(_lock);
> > +
> > ret = ov5645_set_register_array(ov5645,
> > ov5645_global_init_setting,
> > ARRAY_SIZE(ov5645_global_init_setting));

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] ov5645: I2C address change

[Sakari Ailus]

Hi Todor,

On Mon, Oct 02, 2017 at 04:28:45PM +0300, Todor Tomov wrote:
> As soon as the sensor is powered on, change the I2C address to the one
> specified in DT. This allows to use multiple physical sensors connected
> to the same I2C bus.
> 
> Signed-off-by: Todor Tomov 

The smiapp driver does something similar and I understand Laurent might be
interested in such functionality as well.

It'd be nice to handle this through the I²C framework instead and to define
how the information is specified through DT. That way it could be made
generic, to work with more devices than just this one.

What do you think?

Cc Laurent.

> ---
>  drivers/media/i2c/ov5645.c | 42 ++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index d28845f..8541109 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -42,6 +43,8 @@
>  #include 
>  #include 
>  
> +static DEFINE_MUTEX(ov5645_lock);
> +
>  #define OV5645_VOLTAGE_ANALOG   280
>  #define OV5645_VOLTAGE_DIGITAL_CORE 150
>  #define OV5645_VOLTAGE_DIGITAL_IO   180
> @@ -590,6 +593,31 @@ static void ov5645_regulators_disable(struct ov5645 
> *ov5645)
>   dev_err(ov5645->dev, "io regulator disable failed\n");
>  }
>  
> +static int ov5645_write_reg_to(struct ov5645 *ov5645, u16 reg, u8 val,
> +u16 i2c_addr)
> +{
> + u8 regbuf[3] = {
> + reg >> 8,
> + reg & 0xff,
> + val
> + };
> + struct i2c_msg msgs = {
> + .addr = i2c_addr,
> + .flags = 0,
> + .len = 3,
> + .buf = regbuf
> + };
> + int ret;
> +
> + ret = i2c_transfer(ov5645->i2c_client->adapter, , 1);
> + if (ret < 0)
> + dev_err(ov5645->dev,
> + "%s: write reg error %d on addr 0x%x: reg=0x%x, 
> val=0x%x\n",
> + __func__, ret, i2c_addr, reg, val);
> +
> + return ret;
> +}
> +
>  static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
>  {
>   u8 regbuf[3];
> @@ -729,10 +757,24 @@ static int ov5645_s_power(struct v4l2_subdev *sd, int 
> on)
>*/
>   if (ov5645->power_count == !on) {
>   if (on) {
> + mutex_lock(_lock);
> +
>   ret = ov5645_set_power_on(ov5645);
>   if (ret < 0)
>   goto exit;
>  
> + ret = ov5645_write_reg_to(ov5645, 0x3100,
> + ov5645->i2c_client->addr, 0x78);
> + if (ret < 0) {
> + dev_err(ov5645->dev,
> + "could not change i2c address\n");
> + ov5645_set_power_off(ov5645);
> + mutex_unlock(_lock);
> + goto exit;
> + }
> +
> + mutex_unlock(_lock);
> +
>   ret = ov5645_set_register_array(ov5645,
>   ov5645_global_init_setting,
>   ARRAY_SIZE(ov5645_global_init_setting));
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH] [media] ov5645: I2C address change (fwd)

[Todor Tomov]

Hello,

On  4.10.2017 12:06, Julia Lawall wrote:
> Hello,
> 
> It seems that an unlock is missing on line 764.

Yes, this is true. I'll add an unlock there. Thank you for noticing this.

Best regards,
Todor

> 
> julia
> 
> -- Forwarded message --
> Date: Wed, 4 Oct 2017 05:59:09 +0800
> From: kbuild test robot <fengguang...@intel.com>
> To: kbu...@01.org
> Cc: Julia Lawall <julia.law...@lip6.fr>
> Subject: Re: [PATCH] [media] ov5645: I2C address change
> 
> CC: kbuild-...@01.org
> In-Reply-To: <1506950925-13924-1-git-send-email-todor.to...@linaro.org>
> TO: Todor Tomov <todor.to...@linaro.org>
> CC: mche...@kernel.org, sakari.ai...@linux.intel.com, hansv...@cisco.com, 
> linux-media@vger.kernel.org, linux-ker...@vger.kernel.org, Todor Tomov 
> <todor.to...@linaro.org>
> CC: linux-media@vger.kernel.org, linux-ker...@vger.kernel.org, Todor Tomov 
> <todor.to...@linaro.org>
> 
> Hi Todor,
> 
> [auto build test WARNING on linuxtv-media/master]
> [also build test WARNING on v4.14-rc3 next-20170929]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Todor-Tomov/ov5645-I2C-address-change/20171003-234231
> base:   git://linuxtv.org/media_tree.git master
> :: branch date: 6 hours ago
> :: commit date: 6 hours ago
> 
>>> drivers/media/i2c/ov5645.c:806:1-7: preceding lock on line 760
> 
> # 
> https://github.com/0day-ci/linux/commit/c222075023642217170e2ef95f48efef079f9bcd
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout c222075023642217170e2ef95f48efef079f9bcd
> vim +806 drivers/media/i2c/ov5645.c
> 
> 9cae9722 Todor Tomov 2017-04-11  747
> 9cae9722 Todor Tomov 2017-04-11  748  static int ov5645_s_power(struct 
> v4l2_subdev *sd, int on)
> 9cae9722 Todor Tomov 2017-04-11  749  {
> 9cae9722 Todor Tomov 2017-04-11  750  struct ov5645 *ov5645 = 
> to_ov5645(sd);
> 9cae9722 Todor Tomov 2017-04-11  751  int ret = 0;
> 9cae9722 Todor Tomov 2017-04-11  752
> 9cae9722 Todor Tomov 2017-04-11  753  mutex_lock(>power_lock);
> 9cae9722 Todor Tomov 2017-04-11  754
> 9cae9722 Todor Tomov 2017-04-11  755  /* If the power count is 
> modified from 0 to != 0 or from != 0 to 0,
> 9cae9722 Todor Tomov 2017-04-11  756   * update the power state.
> 9cae9722 Todor Tomov 2017-04-11  757   */
> 9cae9722 Todor Tomov 2017-04-11  758  if (ov5645->power_count == !on) 
> {
> 9cae9722 Todor Tomov 2017-04-11  759  if (on) {
> c2220750 Todor Tomov 2017-10-02 @760  
> mutex_lock(_lock);
> c2220750 Todor Tomov 2017-10-02  761
> 9cae9722 Todor Tomov 2017-04-11  762  ret = 
> ov5645_set_power_on(ov5645);
> 9cae9722 Todor Tomov 2017-04-11  763  if (ret < 0)
> 9cae9722 Todor Tomov 2017-04-11  764  goto 
> exit;
> 9cae9722 Todor Tomov 2017-04-11  765
> c2220750 Todor Tomov 2017-10-02  766  ret = 
> ov5645_write_reg_to(ov5645, 0x3100,
> c2220750 Todor Tomov 2017-10-02  767  
> ov5645->i2c_client->addr, 0x78);
> c2220750 Todor Tomov 2017-10-02  768  if (ret < 0) {
> c2220750 Todor Tomov 2017-10-02  769  
> dev_err(ov5645->dev,
> c2220750 Todor Tomov 2017-10-02  770  
> "could not change i2c address\n");
> c2220750 Todor Tomov 2017-10-02  771  
> ov5645_set_power_off(ov5645);
> c2220750 Todor Tomov 2017-10-02  772  
> mutex_unlock(_lock);
> c2220750 Todor Tomov 2017-10-02  773  goto 
> exit;
> c2220750 Todor Tomov 2017-10-02  774  }
> c2220750 Todor Tomov 2017-10-02  775
> c2220750 Todor Tomov 2017-10-02  776  
> mutex_unlock(_lock);
> c2220750 Todor Tomov 2017-10-02  777
> 9cae9722 Todor Tomov 2017-04-11  778  ret = 
> ov5645_set_register_array(ov5645,
> 9cae9722 Todor Tomov 2017-04-11  779  
> ov5645_global_init_setting,
> 9cae9722 Todor Tomov 2017-04-11  780  
> ARRAY_SIZE(ov5645_global_init_setting));
> 9cae9722 Todor Tomov 2017-04-11  781  if (ret < 0) {
> 9cae9722 Todor Tomov 2017-04-11  782  
> dev_err(ov5645->dev,
> 9cae9722 Todor Tomov 2017-04-11 

Re: [PATCH] [media] ov5645: I2C address change (fwd)

[Julia Lawall]

Hello,

It seems that an unlock is missing on line 764.

julia

-- Forwarded message --
Date: Wed, 4 Oct 2017 05:59:09 +0800
From: kbuild test robot <fengguang...@intel.com>
To: kbu...@01.org
Cc: Julia Lawall <julia.law...@lip6.fr>
Subject: Re: [PATCH] [media] ov5645: I2C address change

CC: kbuild-...@01.org
In-Reply-To: <1506950925-13924-1-git-send-email-todor.to...@linaro.org>
TO: Todor Tomov <todor.to...@linaro.org>
CC: mche...@kernel.org, sakari.ai...@linux.intel.com, hansv...@cisco.com, 
linux-media@vger.kernel.org, linux-ker...@vger.kernel.org, Todor Tomov 
<todor.to...@linaro.org>
CC: linux-media@vger.kernel.org, linux-ker...@vger.kernel.org, Todor Tomov 
<todor.to...@linaro.org>

Hi Todor,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.14-rc3 next-20170929]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Todor-Tomov/ov5645-I2C-address-change/20171003-234231
base:   git://linuxtv.org/media_tree.git master
:: branch date: 6 hours ago
:: commit date: 6 hours ago

>> drivers/media/i2c/ov5645.c:806:1-7: preceding lock on line 760

# 
https://github.com/0day-ci/linux/commit/c222075023642217170e2ef95f48efef079f9bcd
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c222075023642217170e2ef95f48efef079f9bcd
vim +806 drivers/media/i2c/ov5645.c

9cae9722 Todor Tomov 2017-04-11  747
9cae9722 Todor Tomov 2017-04-11  748  static int ov5645_s_power(struct 
v4l2_subdev *sd, int on)
9cae9722 Todor Tomov 2017-04-11  749  {
9cae9722 Todor Tomov 2017-04-11  750struct ov5645 *ov5645 = to_ov5645(sd);
9cae9722 Todor Tomov 2017-04-11  751int ret = 0;
9cae9722 Todor Tomov 2017-04-11  752
9cae9722 Todor Tomov 2017-04-11  753mutex_lock(>power_lock);
9cae9722 Todor Tomov 2017-04-11  754
9cae9722 Todor Tomov 2017-04-11  755/* If the power count is modified from 
0 to != 0 or from != 0 to 0,
9cae9722 Todor Tomov 2017-04-11  756 * update the power state.
9cae9722 Todor Tomov 2017-04-11  757 */
9cae9722 Todor Tomov 2017-04-11  758if (ov5645->power_count == !on) {
9cae9722 Todor Tomov 2017-04-11  759if (on) {
c2220750 Todor Tomov 2017-10-02 @760
mutex_lock(_lock);
c2220750 Todor Tomov 2017-10-02  761
9cae9722 Todor Tomov 2017-04-11  762ret = 
ov5645_set_power_on(ov5645);
9cae9722 Todor Tomov 2017-04-11  763if (ret < 0)
9cae9722 Todor Tomov 2017-04-11  764goto exit;
9cae9722 Todor Tomov 2017-04-11  765
c2220750 Todor Tomov 2017-10-02  766ret = 
ov5645_write_reg_to(ov5645, 0x3100,
c2220750 Todor Tomov 2017-10-02  767
ov5645->i2c_client->addr, 0x78);
c2220750 Todor Tomov 2017-10-02  768if (ret < 0) {
c2220750 Todor Tomov 2017-10-02  769
dev_err(ov5645->dev,
c2220750 Todor Tomov 2017-10-02  770"could 
not change i2c address\n");
c2220750 Todor Tomov 2017-10-02  771
ov5645_set_power_off(ov5645);
c2220750 Todor Tomov 2017-10-02  772
mutex_unlock(_lock);
c2220750 Todor Tomov 2017-10-02  773goto exit;
c2220750 Todor Tomov 2017-10-02  774}
c2220750 Todor Tomov 2017-10-02  775
c2220750 Todor Tomov 2017-10-02  776
mutex_unlock(_lock);
c2220750 Todor Tomov 2017-10-02  777
9cae9722 Todor Tomov 2017-04-11  778ret = 
ov5645_set_register_array(ov5645,
9cae9722 Todor Tomov 2017-04-11  779
ov5645_global_init_setting,
9cae9722 Todor Tomov 2017-04-11  780
ARRAY_SIZE(ov5645_global_init_setting));
9cae9722 Todor Tomov 2017-04-11  781if (ret < 0) {
9cae9722 Todor Tomov 2017-04-11  782
dev_err(ov5645->dev,
9cae9722 Todor Tomov 2017-04-11  783"could 
not set init registers\n");
9cae9722 Todor Tomov 2017-04-11  784
ov5645_set_power_off(ov5645);
9cae9722 Todor Tomov 2017-04-11  785goto exit;
9cae9722 Todor Tomov 2017-04-11  786}
9cae9722 Todor Tomov 2017-04-11  787
9cae9722 Todor Tomov 2017-04-11  788ret = 
ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
9cae9722 Todor Tomov 2017-04-11  789   
OV5645_SYSTEM_CTRL0_STOP);
9cae9722 Todor Tomov 2017-04-11  790if (ret < 0) {
9cae9722 Todor Tomov 2017-04-11  791
ov5645_set_power_off(ov5645);
9cae9722 Todor Tomov 2017-04-11  792goto exi

[PATCH] [media] ov5645: I2C address change

[Todor Tomov]

As soon as the sensor is powered on, change the I2C address to the one
specified in DT. This allows to use multiple physical sensors connected
to the same I2C bus.

Signed-off-by: Todor Tomov 
---
 drivers/media/i2c/ov5645.c | 42 ++
 1 file changed, 42 insertions(+)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index d28845f..8541109 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -42,6 +43,8 @@
 #include 
 #include 
 
+static DEFINE_MUTEX(ov5645_lock);
+
 #define OV5645_VOLTAGE_ANALOG   280
 #define OV5645_VOLTAGE_DIGITAL_CORE 150
 #define OV5645_VOLTAGE_DIGITAL_IO   180
@@ -590,6 +593,31 @@ static void ov5645_regulators_disable(struct ov5645 
*ov5645)
dev_err(ov5645->dev, "io regulator disable failed\n");
 }
 
+static int ov5645_write_reg_to(struct ov5645 *ov5645, u16 reg, u8 val,
+  u16 i2c_addr)
+{
+   u8 regbuf[3] = {
+   reg >> 8,
+   reg & 0xff,
+   val
+   };
+   struct i2c_msg msgs = {
+   .addr = i2c_addr,
+   .flags = 0,
+   .len = 3,
+   .buf = regbuf
+   };
+   int ret;
+
+   ret = i2c_transfer(ov5645->i2c_client->adapter, , 1);
+   if (ret < 0)
+   dev_err(ov5645->dev,
+   "%s: write reg error %d on addr 0x%x: reg=0x%x, 
val=0x%x\n",
+   __func__, ret, i2c_addr, reg, val);
+
+   return ret;
+}
+
 static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
 {
u8 regbuf[3];
@@ -729,10 +757,24 @@ static int ov5645_s_power(struct v4l2_subdev *sd, int on)
 */
if (ov5645->power_count == !on) {
if (on) {
+   mutex_lock(_lock);
+
ret = ov5645_set_power_on(ov5645);
if (ret < 0)
goto exit;
 
+   ret = ov5645_write_reg_to(ov5645, 0x3100,
+   ov5645->i2c_client->addr, 0x78);
+   if (ret < 0) {
+   dev_err(ov5645->dev,
+   "could not change i2c address\n");
+   ov5645_set_power_off(ov5645);
+   mutex_unlock(_lock);
+   goto exit;
+   }
+
+   mutex_unlock(_lock);
+
ret = ov5645_set_register_array(ov5645,
ov5645_global_init_setting,
ARRAY_SIZE(ov5645_global_init_setting));
-- 
2.7.4