Re: Delay between stop condition and start condition

[Peter Rosin]

On 2018-10-17 14:16, Wolfram Sang wrote:
> 
>> Or, add an I2C gate driver (sort of like an I2C mux with only one child
>> bus) in the HDMI transmitter driver and implement the delay there. Then
>> move the monitor to this new gate/mux child bus.
> 
> That would actually be my preferred solution. Because it describes the
> HW setup best. It is the passthrough creating the problem, so it should
> be fixed in its driver. It probably could be a generic driver, or?

Don't know about the possibility of a generic driver, but one thing to
look out for is that if the "gate" is left open at all times, *other*
xfers on the bus might not have the required delay between stop and
start, which might lead to the monitor (or other clients on the other
side of the HDMI transmitter) seeing potentially nasty things on the
distorted bus...

Cheers,
Peter

Re: Delay between stop condition and start condition

[Peter Rosin]

On 2018-10-17 12:32, Wolfram Sang wrote:
> Hi Fabrizio, everyone,
> 
> Thanks for bringing this up!
> 
>> What's the best way to address this? I could pull in the HDMI
>> transmitter driver the code to read the EDID back from the monitor so
>> that I can fit device specific delays without impacting the generic
>> implementation of the EDID readback, but that would replicate some
>> code and the driver would not benefit from fixes made to the generic
>> implementation. I could change the RCar I2C driver in order to fit a
>> new DT parameter (i2c-delay-after-stop-us?), and the driver would put
>> in the desired delay after every stop condition, but isn't this
>> parameter something every I2C controller would benefit from (now that
>> we know we have a use case for it)? What are your thoughts and
>> recommendations?
> 
> No need for a property. The I2C standard has a clearly defined value for
> that which is named 'tbuf' and is in general the same value as the
> minimal low period of the SCL signal. So, it must be handled at the I2C
> bus master level.
> 
> Currently, we have no rule at what time drivers have to leave the
> master_xfer() callback. Some exit when STOP is still processed, some
> when STOP has been sent. I am not aware of a driver respecting tbuf. It
> seems worth recommending to respect tbuf.
> 
> I think this needs to be completely handled in the bus master driver. We
> have USB-to-I2C bridges which we can control only high level and the
> firmware of those need to respect tbuf. I don't see how the I2C core
> could support individual drivers here.
> 
> So, that's how I see this situation. Other comments? Other ideas?

From the looks of the picture, it seems that the SoC does respect 'tbuf',
but that the DDC pass-through in the HDMI transmitter then destroys the
signals such that the monitor has no chance to interpret stuff correctly.

Since this is not related to the specific bus driver in use, and since
it would be ugly to add some hook that the HDMI transmitter driver could
use, methinks that a sane way to describe that the bus driver needs to
slow down is to add some DT property that makes the I2C core apply a
quirk and thus force some minimum time between stop and start. Just like
Fabrizio suggested...

Either that, or add some hook in the generic edid reader code, so that it
can slow down on request.

Or, add an I2C gate driver (sort of like an I2C mux with only one child
bus) in the HDMI transmitter driver and implement the delay there. Then
move the monitor to this new gate/mux child bus.

Cheers,
Peter

Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or

The first sentence is a bit backwards, I'd rephrase like so:

Multiple times now we've had the request to access devices very late, when
interrupts are no longer available.

> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

accidentally

> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/i2c-core-base.c |  6 +-
>  include/linux/i2c.h | 10 +++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct 
> i2c_msg *msgs, int num)
>   /* Retry automatically on arbitration loss */
>   orig_jiffies = jiffies;
>   for (ret = 0, try = 0; try <= adap->retries; try++) {
> - ret = adap->algo->master_xfer(adap, msgs, num);
> + if ((in_atomic() || irqs_disabled()) && 
> adap->algo->master_xfer_irqless)
> + ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> + else
> + ret = adap->algo->master_xfer(adap, msgs, num);
> +
>   if (ret != -EAGAIN)
>   break;
>   if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info 
> const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts

"Same" (with capital 'S') to match the other entries. Also, should it
not be @master_xfer to help the tools do the right thing?

> + *   so e.g. PMICs can be accessed very late before shutdown

Trailing period.

I'm fine with this change, but should it not wait until there is a user?
(I think there is one in the wings, so that's a very weak objection...)

Acked-by: Peter Rosin 


>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info 
> const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the 
> PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>   /* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  processed, or a negative value on error */
>   int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  int num);
> + int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +struct i2c_msg *msgs, int num);
>   int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  unsigned short flags, char read_write,
>  u8 command, int size, union i2c_smbus_data *data);
> 

Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> If I2C transfers are executed in atomic contexts, trylock is used
> instead of lock. This behaviour was missing for SMBUS, although a lot of
> transfers are of SMBUS type, either emulated or direct. So, factor out
> the locking routine into a helper and use it for I2C and SMBUS.
> 
> Signed-off-by: Wolfram Sang 

Is it ok with static analyzers to "hide" the locking in helpers like
this? Will it not be harder for them to "see" what's going on? But I
don't think we have any annotations anyway, so...

Acked-by: Peter Rosin 


> ---
>  drivers/i2c/i2c-core-base.c  | 11 +++
>  drivers/i2c/i2c-core-smbus.c |  7 ++-
>  drivers/i2c/i2c-core.h   | 12 
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 799776c6d421..904b4d2ebefa 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct 
> i2c_msg *msgs, int num)
>*one (discarding status on the second message) or errno
>*(discarding status on the first one).
>*/
> - if (in_atomic() || irqs_disabled()) {
> - ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
> - if (!ret)
> - /* I2C activity is ongoing. */
> - return -EAGAIN;
> - } else {
> - i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> - }
> + ret = __i2c_lock_bus_helper(adap);
> + if (ret)
> + return ret;
>  
>   ret = __i2c_transfer(adap, msgs, num);
>   i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 9cd66cabb84f..dbb46edb8e02 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -20,6 +20,8 @@
>  #include 
>  #include 
>  
> +#include "i2c-core.h"
> +
>  #define CREATE_TRACE_POINTS
>  #include 
>  
> @@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
>  {
>   s32 res;
>  
> - i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
> + res = __i2c_lock_bus_helper(adapter);
> + if (res)
> + return res;
> +
>   res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
>  command, protocol, data);
>   i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 37576f50fe20..6e98aa811980 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,18 @@ extern int __i2c_first_dynamic_bus_num;
>  
>  int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>  
> +static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> +{
> + int ret = 0;
> +
> + if (in_atomic() || irqs_disabled())
> + ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
> + else
> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> +
> + return ret;
> +}
> +
>  #ifdef CONFIG_ACPI
>  const struct acpi_device_id *
>  i2c_acpi_match_device(const struct acpi_device_id *matches,
> 

Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> Using the common kernel pattern to bail out at the beginning if some
> conditions are not met, we can save a level of indentation. No
> functional change.
> 
> Signed-off-by: Wolfram Sang 

Yes please!

Acked-by: Peter Rosin 

Cheers,
Peter

Re: [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output

[Peter Rosin]

On 2018-09-20 18:14, Wolfram Sang wrote:
> The usefulness of this debug output is questionable. It covers only
> direct i2c_transfer calls and no SMBUS calls, neither direct nor
> emulated ones. And the latter one is largely used in the kernel, so a
> lot of stuff is missed. Also, we have a proper tracing mechanism for all
> these kinds of transfers in place for years now. Remove this old one.
> 
> Signed-off-by: Wolfram Sang 

This old one fires before locking and even if locking fails for the
atomic/irq case. You might want to mention that in the commit message?
But I certainly agree with the usefulness statement...

Acked-by: Peter Rosin 

> ---
>  drivers/i2c/i2c-core-base.c | 10 --
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9ee9a15e7134..c2b352c46fae 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct 
> i2c_msg *msgs, int num)
>*/
>  
>   if (adap->algo->master_xfer) {
> -#ifdef DEBUG
> - for (ret = 0; ret < num; ret++) {
> - dev_dbg(>dev,
> - "master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
> - ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
> - msgs[ret].addr, msgs[ret].len,
> - (msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
> - }
> -#endif
> -
>   if (in_atomic() || irqs_disabled()) {
>   ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
>   if (!ret)
> 

Re: [PATCH] i2c: recovery: make pin init look like STOP

[Peter Rosin]

On 2018-07-16 12:37, Geert Uytterhoeven wrote:
> On Thu, Jul 12, 2018 at 7:49 PM Wolfram Sang
>  wrote:
>> When we we initialize the pins, make sure it looks like STOP by dividing
>> the delay into halves. It shouldn't matter because SDA is expected to be
>> held low by a device, but for super-safety, let's do it.
>>
>> Signed-off-by: Wolfram Sang 
>> ---
>>  drivers/i2c/i2c-core-base.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 51cbb0c158f2..e57231ccb32a 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>> bri->prepare_recovery(adap);
>>
>> bri->set_scl(adap, scl);
>> +   ndelay(RECOVERY_NDELAY / 2);
> 
> Any change someone changes RECOVERY_NDELAY to 1, leading to a
> zero delay here? Is that an issue?

No!

Above this, there is this line:

#define RECOVERY_NDELAY 5000

Cheers,
Peter

>> if (bri->set_sda)
>> -   bri->set_sda(adap, 1);
>> -   ndelay(RECOVERY_NDELAY);
>> +   bri->set_sda(adap, scl);
>> +   ndelay(RECOVERY_NDELAY / 2);
>>
>> /*
>>  * By this time SCL is high, as we need to give 9 falling-rising 
>> edges
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 

Re: [PATCH] i2c: recovery: make pin init look like STOP

[Peter Rosin]

On 2018-07-12 19:49, Wolfram Sang wrote:
> When we we initialize the pins, make sure it looks like STOP by dividing
> the delay into halves. It shouldn't matter because SDA is expected to be
> held low by a device, but for super-safety, let's do it.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/i2c-core-base.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 51cbb0c158f2..e57231ccb32a 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>   bri->prepare_recovery(adap);
>  
>   bri->set_scl(adap, scl);

For me, it would be more natural to have

bri->set_scl(adap, 1);

> + ndelay(RECOVERY_NDELAY / 2);
>   if (bri->set_sda)
> - bri->set_sda(adap, 1);
> - ndelay(RECOVERY_NDELAY);
> + bri->set_sda(adap, scl);

instead of changing this "1" to "scl"? Same-same, but it looks odd
to use scl as argument to sda (at least without that comment about
sda following scl that is present inside the loop below).

At the same time, your version make the code inside the loop the
same as this initializing code. Oh well, your call...

Either way

Reviewed-by: Peter Rosin 

Cheers,
Peter

> + ndelay(RECOVERY_NDELAY / 2);
>  
>   /*
>* By this time SCL is high, as we need to give 9 falling-rising edges
> 

Re: [PATCH v2] i2c: recovery: rename variable for easier understanding

[Peter Rosin]

On 2018-07-11 00:27, Wolfram Sang wrote:
> While refactoring the routine before, it occured to me that this will
> make the code much easier to understand.

Acked-by: Peter Rosin 
 
> Signed-off-by: Wolfram Sang 
> ---
> 
> Change since V1:
> * rebased to the new recovery refactoring
> 
> Branch is here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
> renesas/i2c/recovery/write-byte-fix
> 
>  drivers/i2c/i2c-core-base.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 59f8dfc5be36..57538d72f2e5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -185,12 +185,12 @@ static int i2c_generic_bus_free(struct i2c_adapter 
> *adap)
>  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  {
>   struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> - int i = 0, val = 1, ret;
> + int i = 0, scl = 1, ret;
>  
>   if (bri->prepare_recovery)
>   bri->prepare_recovery(adap);
>  
> - bri->set_scl(adap, val);
> + bri->set_scl(adap, scl);
>   if (bri->set_sda)
>   bri->set_sda(adap, 1);
>   ndelay(RECOVERY_NDELAY);
> @@ -199,7 +199,7 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>* By this time SCL is high, as we need to give 9 falling-rising edges
>*/
>   while (i++ < RECOVERY_CLK_CNT * 2) {
> - if (val) {
> + if (scl) {
>   /* SCL shouldn't be low here */
>   if (!bri->get_scl(adap)) {
>   dev_err(>dev,
> @@ -209,8 +209,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>   }
>   }
>  
> - val = !val;
> - bri->set_scl(adap, val);
> + scl = !scl;
> + bri->set_scl(adap, scl);
>  
>   /*
>* If we can set SDA, we will always create STOP here to ensure
> @@ -220,10 +220,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>*/
>   ndelay(RECOVERY_NDELAY / 2);
>   if (bri->set_sda)
> - bri->set_sda(adap, val);
> + bri->set_sda(adap, scl);
>   ndelay(RECOVERY_NDELAY / 2);
>  
> - if (val) {
> + if (scl) {
>   ret = i2c_generic_bus_free(adap);
>   if (ret == 0)
>   break;
> 

Re: [PATCH v2 3/3] i2c: recovery: refactor recovery function

[Peter Rosin]

On 2018-07-10 23:42, Wolfram Sang wrote:
> After exiting the while loop, we checked if recovery was successful and
> sent a STOP to the clients. Meanwhile however, we send a STOP after
> every pulse, so it is not needed after the loop. If we move the check
> for a free bus to the end of the while loop, we can shorten and simplify
> the logic. It is still ensured that at least one STOP will be sent to
> the wire even if SDA was not stuck low.

Well, there will be no STOP if ->set_sda isn't implemented, but that case
is also handled equivalently after this patch AFAICT, so

Reviewed-by: Peter Rosin 

> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/i2c-core-base.c | 24 ++--
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 871a9731894f..c7995efd58ea 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -191,9 +191,6 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>   ret = -EBUSY;
>   break;
>   }
> - /* Break if SDA is high */
> - if (bri->get_sda && bri->get_sda(adap))
> - break;
>   }
>  
>   val = !val;
> @@ -209,22 +206,13 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>   if (bri->set_sda)
>   bri->set_sda(adap, val);
>   ndelay(RECOVERY_NDELAY / 2);
> - }
> -
> - /* check if recovery actually succeeded */
> - if (bri->get_sda && !bri->get_sda(adap))
> - ret = -EBUSY;
>  
> - /* If all went well, send STOP for a sane bus state. */
> - if (ret == 0 && bri->set_sda) {
> - bri->set_scl(adap, 0);
> - ndelay(RECOVERY_NDELAY / 2);
> - bri->set_sda(adap, 0);
> - ndelay(RECOVERY_NDELAY / 2);
> - bri->set_scl(adap, 1);
> - ndelay(RECOVERY_NDELAY / 2);
> - bri->set_sda(adap, 1);
> - ndelay(RECOVERY_NDELAY / 2);
> + /* Break if SDA is high */
> + if (val && bri->get_sda) {
> + ret = bri->get_sda(adap) ? 0 : -EBUSY;
> + if (ret == 0)
> + break;
> + }
>   }
>  
>   if (bri->unprepare_recovery)
> 

Re: [PATCH v2 2/3] i2c: recovery: require either get_sda or set_sda

[Peter Rosin]

On 2018-07-10 23:42, Wolfram Sang wrote:
> For bus recovery, we either need to bail out early if we can read SDA or
> we need to send STOP after every pulse. Otherwise recovery might be
> misinterpreted as an unwanted write. So, require one of those SDA
> handling functions to avoid this problem.

Assuming that all users fulfill the stricter requirement...

Acked-by: Peter Rosin 

> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/i2c-core-base.c |  7 ++-
>  include/linux/i2c.h | 12 ++--
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 301285c54603..871a9731894f 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -202,7 +202,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>   /*
>* If we can set SDA, we will always create STOP here to ensure
>* the additional pulses will do no harm. This is achieved by
> -  * letting SDA follow SCL half a cycle later.
> +  * letting SDA follow SCL half a cycle later. Check the
> +  * 'incomplete_write_byte' fault injector for details.
>*/
>   ndelay(RECOVERY_NDELAY / 2);
>   if (bri->set_sda)
> @@ -274,6 +275,10 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>   err_str = "no {get|set}_scl() found";
>   goto err;
>   }
> + if (!bri->set_sda && !bri->get_sda) {
> + err_str = "either get_sda() or set_sda() needed";
> + goto err;
> + }
>   }
>  
>   return;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 465afb092fa7..9d1818c56775 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -581,12 +581,12 @@ struct i2c_timings {
>   *  recovery. Populated internally for generic GPIO recovery.
>   * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL 
> recovery.
>   *  Populated internally for generic GPIO recovery.
> - * @get_sda: This gets current value of SDA line. Optional for generic SCL
> - *  recovery. Populated internally, if sda_gpio is a valid GPIO, for 
> generic
> - *  GPIO recovery.
> - * @set_sda: This sets/clears the SDA line. Optional for generic SCL 
> recovery.
> - *   Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
> - *   recovery.
> + * @get_sda: This gets current value of SDA line. This or set_sda() is 
> mandatory
> + *   for generic SCL recovery. Populated internally, if sda_gpio is a valid
> + *   GPIO, for generic GPIO recovery.
> + * @set_sda: This sets/clears the SDA line. This or get_sda() is mandatory 
> for
> + *   generic SCL recovery. Populated internally, if sda_gpio is a valid GPIO,
> + *   for generic GPIO recovery.
>   * @prepare_recovery: This will be called before starting recovery. Platform 
> may
>   *   configure padmux here for SDA/SCL line or something else they want.
>   * @unprepare_recovery: This will be called after completing recovery. 
> Platform
> 

Re: [PATCH v2 1/3] i2c: recovery: if possible send STOP with recovery pulses

[Peter Rosin]

On 2018-07-10 23:42, Wolfram Sang wrote:
> I2C clients may misunderstand recovery pulses if they can't read SDA to
> bail out early. In the worst case, as a write operation. To avoid that
> and if we can write SDA, try to send STOP to avoid the
> misinterpretation.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/i2c-core-base.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 31d16ada6e7d..301285c54603 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -198,7 +198,16 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  
>   val = !val;
>   bri->set_scl(adap, val);
> - ndelay(RECOVERY_NDELAY);
> +
> + /*
> +  * If we can set SDA, we will always create STOP here to ensure
> +  * the additional pulses will do no harm. This is achieved by
> +  * letting SDA follow SCL half a cycle later.
> +  */
> + ndelay(RECOVERY_NDELAY / 2);
> + if (bri->set_sda)
> + bri->set_sda(adap, val);
> + ndelay(RECOVERY_NDELAY / 2);
>   }
>  
>   /* check if recovery actually succeeded */
> 
Hmmm, should not the ndelay before the loop also be split up in two like
this, with one ndelay(... / 2) on either side of the (possible) set_sda.
Not that it should matter, since SDA is presumably stuck low. But what if
it isn't?

I would also change the while (...) to

for (i = 0; i < RECOVERY_CLK_CNT * 2; i++)

but both these "issues" are perhaps not related to this patch...

Either way,

Reviewed-by: Peter Rosin 

Cheers,
Peter

Re: [PATCH] i2c: mux: improve error message for failed symlink

[Peter Rosin]

On 2018-05-21 09:34, Wolfram Sang wrote:
> Trivial, but still: the failed symlink is not *for* the channel but a
> link *to* the channel.
> 
> Signed-off-by: Wolfram Sang 

Applied to i2c-mux/for-next.

Cheers,
Peter

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Peter Rosin]

On 2018-05-22 21:13, Wolfram Sang wrote:
> Hi Peter,
> 
>> Hmm, now that I have slept on it, I find this a bit odd. For muxes, all
>> channels and the parent are always present. Here, that is not the case.
>> And don't get me wrong, I see why that is the case, but that doesn't
>> mean that I like it. It would be so much nicer and less disruptive if
>> the client devices were not unbound and rebound (which I think they are,
>> right?) on every master switch.
> 
> Yes, we rebind on every master switch. In the first iteration of this
> driver, I tried the on-the-fly approach. It turned out to be very flaky
> because it was stretching the driver model too much. There is no support
> for multiple parents and no support for switching the parent at runtime.
> When trying to do that, you find out that e.g. the whole relationship
> tree needs to be rebuilt, say to keep PM hierarchy consistent. And even,
> just for I2C, on-the-fly switching is not really supported. Some Renesas
> R-Car SoCs have two different I2C IP cores which can be muxed to the
> same pins. One has DMA, the other slave functionality. I don't see a way
> to combine both into one "virtual" master. This is why I came to the
> current approach. The use case that the customers decide on the feature
> set they want to use after booting was said to be good enough.
> 
>> In some cases I think it might be possible to make the switch automatic
>> and seamless, e.g. if there are two masters and one of them is faster
>> but has some glitch(es), and the other is slower but complete (or at
>> least complete enough).
> 
> That might work for some cases, yet I'd favor a generic solution.
> 
>> What do you think?
> 
> Given my above experiences, I'd just drop the channel symlink and keep
> the driver as is :)

Ok, I'm dropping this patch.

> But thanks for thinking about it!

No problem.

Cheers,
Peter

Re: [PATCH v2 1/2] i2c: mux: demux-pinctrl: use proper parent device for demux adapter

[Peter Rosin]

On 2018-05-21 09:29, Wolfram Sang wrote:
> Due to a typo, the wrong parent device was assigned to the newly created
> demuxing adapter device. It got connected to the demuxing platform
> device but not to the selected parent I2C adapter device. Fix it to get
> a proper parent-child relationship of the demuxed busses.

Applied to i2c-mux/for-next.

Cheers,
Peter

Re: [PATCH v2 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Peter Rosin]

On 2018-05-21 09:29, Wolfram Sang wrote:
> Similar to mux devices, create special symlinks to connect the demuxed
> bus with the demux device.

Hmm, now that I have slept on it, I find this a bit odd. For muxes, all
channels and the parent are always present. Here, that is not the case.
And don't get me wrong, I see why that is the case, but that doesn't
mean that I like it. It would be so much nicer and less disruptive if
the client devices were not unbound and rebound (which I think they are,
right?) on every master switch.

In some cases I think it might be possible to make the switch automatic
and seamless, e.g. if there are two masters and one of them is faster
but has some glitch(es), and the other is slower but complete (or at
least complete enough).

The demuxer could then switch to the slower master automatically, on
a transaction-by-transaction basis, in order to avoid the glitch(es).
Yes, it would have to work a bit differently etc etc, but I don't see
any reason why it can't be done. But you probably know more than I on
that part of the I2C code...

Anyway, since this is ABI (and should be documented) I think it would
be good if it could accommodate the automatic master switch case too.
And for that to work, the "channel-0" name is wrong. I think channel-N
should be reserved for the actual masters, in the order given in the
devicetree (I know that you currently can't establish these links since
you unbind the inactive masters, but that unbind can probably not happen
for the automatic switch case?). There could then be some other symlink,
e.g. "current" or "master" or something like that, that reflects the
present situation (your "channel-0").

What do you think?

Cheers,
Peter

> Signed-off-by: Wolfram Sang 
> ---
> 
> Changes since v1:
> * check sysfs_create_link and print WARN if something fails
> 
>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c 
> b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 035032e20327..13d1461703f3 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -116,6 +116,13 @@ static int i2c_demux_activate_master(struct 
> i2c_demux_pinctrl_priv *priv, u32 ne
>   if (ret < 0)
>   goto err_with_put;
>  
> + WARN(sysfs_create_link(>cur_adap.dev.kobj, >dev->kobj,
> +"demux_device"),
> +  "can't create symlink to mux device\n");
> + WARN(sysfs_create_link(>dev->kobj, >cur_adap.dev.kobj,
> +"channel-0"),
> +  "can't create symlink to channel 0\n");
> +
>   return 0;
>  
>   err_with_put:
> @@ -135,6 +142,9 @@ static int i2c_demux_deactivate_master(struct 
> i2c_demux_pinctrl_priv *priv)
>   if (cur < 0)
>   return 0;
>  
> + sysfs_remove_link(>dev->kobj, "channel-0");
> + sysfs_remove_link(>cur_adap.dev.kobj, "demux_device");
> +
>   i2c_del_adapter(>cur_adap);
>   i2c_put_adapter(priv->chan[cur].parent_adap);
>  
> 

Re: [PATCH 1/2] i2c: mux: demux-pinctrl: use proper parent device for demux adapter

[Peter Rosin]

On 2018-05-20 08:45, Wolfram Sang wrote:
> On Sat, May 19, 2018 at 11:40:04PM +0200, Peter Rosin wrote:
>> On 2018-04-30 13:55, Wolfram Sang wrote:
>>> Due to a typo, the wrong parent device was assigned to the newly created
>>> demuxing adapter device. It got connected to the demuxing platform
>>> device but not to the selected parent I2C adapter device. Fix it to get
>>> a proper parent-child relationship of the demuxed busses, needed also for
>>> proper PM.
>>>
>>
>> Should this one have a Fixes: tag? Should it go to current or next? Is
>> a backport to stable good enough?
> 
> A Fixes tag is probably apropriate. I don't think it should go to
> stable, though, because it will break a scheme a user might be using.
> This can't be avoided for a kernel upgrade here, but IMO we shouldn't
> enforce it for a stable update. Especially, given this is not a real
> bug, but more something improper. For the same reason, I'd think -next
> is good enough.
> 
> Thanks, will resend!

Hmm, in that case, I think a Fixes tag is best left out, because I suspect
the net effect is mostly a bunch of mails from the stable trees as the
automatic patch-picker finds this patch. So, I plan to go with this patch.

Ok?

Cheers,
Peter

Re: [PATCH] i2c: mux: demux-pinctrl: disable PM user interface

[Peter Rosin]

On 2018-04-30 14:08, Wolfram Sang wrote:
> The demux device is only a logical device with no children. So, no
> RuntimePM is needed, let's disable the sysfs interface for it.
> 
> Signed-off-by: Wolfram Sang 
> ---
> 
> I think it is proper to just disable the interface without enabling RPM at 
> all.
> USB and PCIE core do this as well. Still, adding PM and Rafael to CC in case 
> we
> all got it wrong.

Sounds ok to me. I'm not fluent with PM stuff, but I trust your judgement.

Applied to i2c-mux/for-next.

Cheers,
Peter

>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c 
> b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 61440a9507e4..d5e7d4aa6ee1 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -262,6 +263,8 @@ static int i2c_demux_pinctrl_probe(struct platform_device 
> *pdev)
>  
>   platform_set_drvdata(pdev, priv);
>  
> + pm_runtime_no_callbacks(>dev);
> +
>   /* switch to first parent as active master */
>   i2c_demux_activate_master(priv, 0);
>  
> 

Re: [PATCH 2/2] i2c: mux: demux-pinctrl: add symlinks to the demux device in sysfs

[Peter Rosin]

On 2018-04-30 13:55, Wolfram Sang wrote:
> Similar to mux devices, create special symlinks to connect the demuxed
> bus with the demux device.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c 
> b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 035032e20327..d5e7d4aa6ee1 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -116,6 +116,11 @@ static int i2c_demux_activate_master(struct 
> i2c_demux_pinctrl_priv *priv, u32 ne
>   if (ret < 0)
>   goto err_with_put;
>  
> + sysfs_create_link(>cur_adap.dev.kobj, >dev->kobj,
> +"demux_device");
> + sysfs_create_link(>dev->kobj, >cur_adap.dev.kobj,
> +"channel-0");
> +

>From sysfs.h:
int __must_check sysfs_create_link(...);

Cheers,
Peter

>   return 0;
>  
>   err_with_put:
> @@ -135,6 +140,9 @@ static int i2c_demux_deactivate_master(struct 
> i2c_demux_pinctrl_priv *priv)
>   if (cur < 0)
>   return 0;
>  
> + sysfs_remove_link(>dev->kobj, "channel-0");
> + sysfs_remove_link(>cur_adap.dev.kobj, "demux_device");
> +
>   i2c_del_adapter(>cur_adap);
>   i2c_put_adapter(priv->chan[cur].parent_adap);
>  
> 

Re: [PATCH 1/2] i2c: mux: demux-pinctrl: use proper parent device for demux adapter

[Peter Rosin]

On 2018-04-30 13:55, Wolfram Sang wrote:
> Due to a typo, the wrong parent device was assigned to the newly created
> demuxing adapter device. It got connected to the demuxing platform
> device but not to the selected parent I2C adapter device. Fix it to get
> a proper parent-child relationship of the demuxed busses, needed also for
> proper PM.
> 

Should this one have a Fixes: tag? Should it go to current or next? Is
a backport to stable good enough?

Cheers,
Peter

> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c 
> b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 428de4c97fb2..035032e20327 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -106,7 +106,7 @@ static int i2c_demux_activate_master(struct 
> i2c_demux_pinctrl_priv *priv, u32 ne
>   priv->cur_adap.owner = THIS_MODULE;
>   priv->cur_adap.algo = >algo;
>   priv->cur_adap.algo_data = priv;
> - priv->cur_adap.dev.parent = priv->dev;
> + priv->cur_adap.dev.parent = >dev;
>   priv->cur_adap.class = adap->class;
>   priv->cur_adap.retries = adap->retries;
>   priv->cur_adap.timeout = adap->timeout;
> 

Re: [PATCH 0/2] i2c: mux: demux-pinctrl: improve device relationships

[Peter Rosin]

On 2018-05-17 16:11, Wolfram Sang wrote:
> On Mon, Apr 30, 2018 at 01:55:42PM +0200, Wolfram Sang wrote:
>> While researching some PM behaviour within I2C, I found out that the 
>> i2c-demux
>> driver does not play well with that due to broken relationship with other
>> devices. Patch 1 ensures the right parent-child relationship. Patch 2 makes 
>> the
>> connection between the demux device and the demuxed bus similar to that we 
>> have
>> in the mux core.
>>
>> Tested on a Renesas Lager board (R-Car H2).
> 
> Peter, I'd think these demux patches (and the single one later) should
> go via your tree. Let me know if you prefer that I pick them up.

Agreed, sorry for being a bit slow. I'll add some comments to the individual
patches...

Cheers,
Peter

[PATCH v3 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner device or keep just providing an
of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

There is an interaction with the rockchip lvds driver, since that
driver peeks into somewhat private parts of the bridge struct in
order to find out things about the remote bridge. When there are
now two ways to get to the remote bridge, the rockchip lvds driver
has to adapt. That said, the correct thing to do for the rockchip
lvds driver is to use some other way than DT to find things out
about the remote bridge, but that is orthogonal to this patch.

Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 ++-
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
 include/drm/drm_bridge.h | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3872f5379998 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if (bridge->of_node == np) {
+   if ((bridge->odev && bridge->odev->of_node == np) ||
+   bridge->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..557e0079c98d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else
+   else if (lvds->bridge->of_node)
remote = lvds->bridge->of_node;
+   else
+   remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
/* default set it as output rgb */
lvds->output = DISPLAY_OUTPUT_RGB;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7c17977c3537 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @odev: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *odev;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-- 
2.11.0

[PATCH v3 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russell King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russell.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 25 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. Jyri, are
you able to test? That said, I might have missed some subtlety.

Oh, and the reason I'm pushing this is of course so that the issue
noted by Russell in [1] is addressed which in turn means that the
tda998x bridge driver can be patched according to that series without
objection (hopefully) and then used from the atmel-hlcdc driver (and
other drivers that are not componentized).

Changes since v2https://lkml.org/lkml/2018/5/4/443

- Russell King spells his name this way. Sorry!
- Add review tag from Andrzej Hajda (patches 1, 25 and 26).
- Add ack tag from Daniel Vetter (patches 1, 24, 25 and 26).
- Mention the interaction with the rockchip_lvds driver in the
  commit message for patch 1.
- Change the comment for the new @link member in patch 26, so that a
  symmetric relationchip between cunsumer/supplier isn't implied.

Changes since v1https://lkml.org/lkml/2018/4/26/1018

- rename .owner to .odev to not get mixed up with the module owner.
- added patches for new recent drivers thc63lvd1024 and cdns-dsi
- fix for problem in the rockchip_lvds driver reported by 0day
- added a WARN in drm_bridge_add if there is no .odev owner device

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (26):
  drm/bridge: allow optionally specifying an owner .odev device
  drm/bridge: adv7511: provide an owner .odev device
  drm/bridge/analogix: core: specify the owner .odev of the bridge
  drm/bridge: analogix-anx78xx: provide an owner .odev device
  drm/bridge: cdns-dsi: provide an owner .odev device
  drm/bridge: vga-dac: provide an owner .odev device
  drm/bridge: lvds-encoder: provide an owner .odev device
  drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
device
  drm/bridge: nxp-ptn3460: provide an owner .odev device
  drm/bridge: panel: provide an owner .odev device
  drm/bridge: ps8622: provide an owner .odev device
  drm/bridge: sii902x: provide an owner .odev device
  drm/bridge: sii9234: provide an owner .odev device
  drm/bridge: sii8620: provide an owner .odev device
  drm/bridge: synopsys: provide an owner .odev device for the bridges
  drm/bridge: tc358767: provide an owner .odev device
  drm/bridge: thc63lvd1024: provide an owner .odev device
  drm/bridge: ti-tfp410: provide an owner .odev device
  drm/exynos: mic: provide an owner .odev device for the bridge
  drm/mediatek: hdmi: provide an owner .odev device for the bridge
  drm/msm: specify the owner .odev of the bridges
  drm/rcar-du: lvds: provide an owner .odev device for the bridge
  drm/sti: provide an owner .odev device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the owner .odev to be filled in on
drm_bridge_add/attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/cdns-dsi.c  |  2 +-
 drivers/gpu/drm/bridge/dumb-vga-dac.c  |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c  |  2 +-
 .../drm/bridge/megachips-stdp-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c   |  2 +-
 drivers/gpu/drm/bridge/panel.c |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c |  2 +-
 drivers/gpu/drm/bridge/sii902x.c   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c   |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c  |  4 +---
 drivers/gpu/drm/bridge/tc358767.c  |  2 +-
 drivers/gpu/drm/bridge/thc63lvd1024.c  |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c |  2 +-
 drivers/gpu/drm/drm_bridge.c   | 26 +-
 drivers/gpu/drm/exynos/exynos_drm_mic.c|  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c|  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c  |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c|  2 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c   |  2 +-
 drivers/gpu/drm/sti

[PATCH v3 22/26] drm/rcar-du: lvds: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..efda02f55c95 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
 
+   lvds->bridge.odev = >dev;
lvds->bridge.driver_private = lvds;
lvds->bridge.funcs = _lvds_bridge_ops;
-   lvds->bridge.of_node = pdev->dev.of_node;
 
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lvds->mmio = devm_ioremap_resource(>dev, mem);
-- 
2.11.0

[PATCH v3 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Peter Rosin]

The .odev owner device will be handy to have around.

Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index df084db33494..78d186b6831b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+   if (WARN_ON(!bridge->odev))
+   return;
+
mutex_lock(_lock);
list_add_tail(>list, _list);
mutex_unlock(_lock);
@@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (!encoder || !bridge)
return -EINVAL;
 
+   if (WARN_ON(!bridge->odev))
+   return -EINVAL;
+
if (previous && (!previous->dev || previous->encoder != encoder))
return -EINVAL;
 
-- 
2.11.0

[PATCH v3 24/26] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +--
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 include/drm/drm_bridge.h | 4 
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3872f5379998..df084db33494 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if ((bridge->odev && bridge->odev->of_node == np) ||
-   bridge->of_node == np) {
+   if (bridge->odev->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 557e0079c98d..e77d4c909582 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else if (lvds->bridge->of_node)
-   remote = lvds->bridge->of_node;
else
remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7c17977c3537..b656e505d11e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-#ifdef CONFIG_OF
-   struct device_node *of_node;
-#endif
struct list_head list;
const struct drm_bridge_timings *timings;
 
-- 
2.11.0

[PATCH v3 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++
 include/drm/drm_bridge.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 78d186b6831b..0259f0a3ff27 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "drm_crtc_internal.h"
@@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   if (encoder->dev->dev != bridge->odev) {
+   bridge->link = device_link_add(encoder->dev->dev,
+  bridge->odev, 0);
+   if (!bridge->link) {
+   dev_err(bridge->odev, "failed to link bridge to %s\n",
+   dev_name(encoder->dev->dev));
+   return -EINVAL;
+   }
+   }
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
+
bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b656e505d11e..bd1265c5a0bc 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: device link between the drm consumer and the bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
struct drm_bridge *next;
struct list_head list;
const struct drm_bridge_timings *timings;
+   struct device_link *link;
 
const struct drm_bridge_funcs *funcs;
void *driver_private;
-- 
2.11.0

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-15 12:22, Daniel Vetter wrote:
> On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <p...@axentia.se> wrote:
>> On 2018-05-14 18:28, Daniel Vetter wrote:
>>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>>>> down along with the bridge. Thus, there will no longer linger any
>>>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>>>> non-existent bridge supplier.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>>>>>  include/drm/drm_bridge.h |  2 ++
>>>>>>  2 files changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include 
>>>>>>
>>>>>>  #include 
>>>>>> +#include 
>>>>>>  #include 
>>>>>>
>>>>>>  #include "drm_crtc_internal.h"
>>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>>>>>> struct drm_bridge *bridge,
>>>>>>if (bridge->dev)
>>>>>>return -EBUSY;
>>>>>>
>>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>>
>>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>>> silently as noop, as it seems to be a correct behavior.
>>>>
>>>> It's kind-of a silly corner-case though, so perfectly understandable
>>>> that it isn't handled.
>>>>
>>>>>> +  bridge->link = device_link_add(encoder->dev->dev,
>>>>>> + bridge->odev, 0);
>>>>>> +  if (!bridge->link) {
>>>>>> +  dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>>>> +  dev_name(encoder->dev->dev));
>>>>>> +  return -EINVAL;
>>>>>> +  }
>>>>>> +  }
>>>>>> +
>>>>>>bridge->dev = encoder->dev;
>>>>>>bridge->encoder = encoder;
>>>>>>
>>>>>>if (bridge->funcs->attach) {
>>>>>>ret = bridge->funcs->attach(bridge);
>>>>>>if (ret < 0) {
>>>>>> +  if (bridge->link)
>>>>>> +  device_link_del(bridge->link);
>>>>>> +  bridge->link = NULL;
>>>>>>bridge->dev = NULL;
>>>>>>bridge->encoder = NULL;
>>>>>>return ret;
>>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>>>if (bridge->funcs->detach)
>>>>>>bridge->funcs->detach(bridge);
>>>>>>
>>>>>> +  if (bridge->link)
>>>>>> +  device_link_del(bridge->link);
>>>>>> +  bridge->link = NULL;
>>>>>> +
>>>>>>bridge->dev = NULL;
>>>>>>  }
>>>>>>
>>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>>>> index b656e505d11e..804189c63a4c 100644
>>>>>> --- a/include/drm/drm_bridge.h
>>>>>> +++ b/include/drm/drm_bridge.h
>>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>>>   * @list: to keep track of all added bridges
>>>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>>>   * be NULL)
>>>>>> + * @link: drm consumer <-> bridge supplier
>>>>>
>>>>> Nitpick: "<->" suggests symmetry, maybe "d

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-14 18:28, Daniel Vetter wrote:
> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>> down along with the bridge. Thus, there will no longer linger any
>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>> non-existent bridge supplier.
>>>>
>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>>>  include/drm/drm_bridge.h |  2 ++
>>>>  2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include 
>>>>  
>>>>  #include 
>>>> +#include 
>>>>  #include 
>>>>  
>>>>  #include "drm_crtc_internal.h"
>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>>>> struct drm_bridge *bridge,
>>>>if (bridge->dev)
>>>>return -EBUSY;
>>>>  
>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>
>>> I wonder why device_link_add does not handle this case (self dependency)
>>> silently as noop, as it seems to be a correct behavior.
>>
>> It's kind-of a silly corner-case though, so perfectly understandable
>> that it isn't handled.
>>
>>>> +  bridge->link = device_link_add(encoder->dev->dev,
>>>> + bridge->odev, 0);
>>>> +  if (!bridge->link) {
>>>> +  dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>> +  dev_name(encoder->dev->dev));
>>>> +  return -EINVAL;
>>>> +  }
>>>> +  }
>>>> +
>>>>bridge->dev = encoder->dev;
>>>>bridge->encoder = encoder;
>>>>  
>>>>if (bridge->funcs->attach) {
>>>>ret = bridge->funcs->attach(bridge);
>>>>if (ret < 0) {
>>>> +  if (bridge->link)
>>>> +  device_link_del(bridge->link);
>>>> +  bridge->link = NULL;
>>>>bridge->dev = NULL;
>>>>bridge->encoder = NULL;
>>>>return ret;
>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>if (bridge->funcs->detach)
>>>>bridge->funcs->detach(bridge);
>>>>  
>>>> +  if (bridge->link)
>>>> +  device_link_del(bridge->link);
>>>> +  bridge->link = NULL;
>>>> +
>>>>bridge->dev = NULL;
>>>>  }
>>>>  
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index b656e505d11e..804189c63a4c 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>   * @list: to keep track of all added bridges
>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>   * be NULL)
>>>> + * @link: drm consumer <-> bridge supplier
>>>
>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>> to the bridge" would be better.
>>
>> I meant "<->" to indicate that the link is bidirectional, not that the
>> relationship is in any way symmetric. I wasn't aware of any implication
>> of a symmetric relationship when using "<->", do you have a reference?
>> But I guess the different arrow notations in math are somewhat overloaded
>> and that someone at some point must have used "<->" to indicate a
>> symmetric relationship...
> 
> Yeah I agree with Andrzej here, for me <-> implies a symmetric
> relationship. Spelling it out like Andrzej suggested sounds like the
> better idea.
> -Daniel

Ok, I guess that means I have to do a v3 after all. Or can this
trivial documentation update be done by the committer? I hate to
spam everyone with another volley...

Or perhaps I should squash patches 2-23 that are all rather similar
and mechanic? I separated them to allow for easier review from
individual driver maintainers, but that didn't seem to happen
anyway...

Cheers,
Peter

> 
>>
>>> Anyway:
>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>
>> Thanks!
>>
>> Cheers,
>> Peter
>>
>>>  --
>>> Regards
>>> Andrzej
>>>
>>>>   * @funcs: control functions
>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>   */
>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>struct drm_bridge *next;
>>>>struct list_head list;
>>>>const struct drm_bridge_timings *timings;
>>>> +  struct device_link *link;
>>>>  
>>>>const struct drm_bridge_funcs *funcs;
>>>>void *driver_private;
>>>
>>>
>>
> 

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-10 10:10, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>  include/drm/drm_bridge.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include 
>>  
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>  if (bridge->dev)
>>  return -EBUSY;
>>  
>> +if (encoder->dev->dev != bridge->odev) {
> 
> I wonder why device_link_add does not handle this case (self dependency)
> silently as noop, as it seems to be a correct behavior.

It's kind-of a silly corner-case though, so perfectly understandable
that it isn't handled.

>> +bridge->link = device_link_add(encoder->dev->dev,
>> +   bridge->odev, 0);
>> +if (!bridge->link) {
>> +dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +dev_name(encoder->dev->dev));
>> +return -EINVAL;
>> +}
>> +}
>> +
>>  bridge->dev = encoder->dev;
>>  bridge->encoder = encoder;
>>  
>>  if (bridge->funcs->attach) {
>>  ret = bridge->funcs->attach(bridge);
>>  if (ret < 0) {
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>>  bridge->dev = NULL;
>>  bridge->encoder = NULL;
>>  return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  if (bridge->funcs->detach)
>>  bridge->funcs->detach(bridge);
>>  
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>> +
>>  bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
> 
> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> to the bridge" would be better.

I meant "<->" to indicate that the link is bidirectional, not that the
relationship is in any way symmetric. I wasn't aware of any implication
of a symmetric relationship when using "<->", do you have a reference?
But I guess the different arrow notations in math are somewhat overloaded
and that someone at some point must have used "<->" to indicate a
symmetric relationship...

> Anyway:
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

Thanks!

Cheers,
Peter

>  --
> Regards
> Andrzej
> 
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  struct drm_bridge *next;
>>  struct list_head list;
>>  const struct drm_bridge_timings *timings;
>> +struct device_link *link;
>>  
>>  const struct drm_bridge_funcs *funcs;
>>  void *driver_private;
> 
> 

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

On 2018-05-09 17:53, Peter Rosin wrote:
> On 2018-05-09 17:08, Andrzej Hajda wrote:
>> On 04.05.2018 15:51, Peter Rosin wrote:
>>> Bridge drivers can now (temporarily, in a transition phase) select if
>>> they want to provide a full owner device or keep just providing an
>>> of_node.
>>>
>>> By providing a full owner device, the bridge drivers no longer need
>>> to provide an of_node since that node is available via the owner
>>> device.
>>>
>>> When all bridge drivers provide an owner device, that will become
>>> mandatory and the .of_node member will be removed.
>>>
>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 3 ++-
>>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
>>
>> What is the reason to put rockchip here? Shouldn't be in separate patch?
> 
> Because the rockchip driver peeks into the bridge struct and all the
> changes in this patch relate to making the new .odev member optional in
> the transition phase, when the bridge can have either a new-style odev
> or an old style of_node.
> 
> I guess this rockchip change could be patch 2, but it has to come first
> after this patch and it makes no sense on its own. Hence, one patch.
> 
> This rockchip_lvds interaction is also present in patch 24/26
> drm/bridge: remove the .of_node member
> 
> I can split them if you want, but I personally don't see the point.

I had a second look, and maybe the series should start with a patch like
this instead, so that the rockchip_lvds.c hunks can be removed from
patch 1/26 and 24/26. That would perhaps be slightly cleaner?

On the other hand, it's orthogonal and this series can be left as is
(with the benefit of me not having to do another iteration and you all
not having another bunch of messages to sift through). The below
patch could easily be (adjusted and) applied later instead. Or not,
since the right fix is to do this with the newfangled static image
format mechanism from Jacopo Mondi, and it might be easier to just do
it right.

State your preference.

Cheers,
Peter

>From dee27b36a36acd271459d1489336b56132097425 Mon Sep 17 00:00:00 2001
From: Peter Rosin <p...@axentia.se>
Date: Wed, 9 May 2018 23:58:24 +0200
Subject: [PATCH] drm/rockchip: lvds: do not dig into the DT of remote bridges

The driver is trying to find the needed "data-mapping" for
interacting with the following bridge in the graphics chain.
But, doing so is bad since it is done w/o regard of the
compatible of the remote bridge, so the value of "data-mapping"
might not mean what this driver assumes. It is also pointless
since no bridge has any documented "data-mapping" DT property
and no dts file show any undocumented use.

Just remove the inappropriate code.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..fa3f4bf9712f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else
-   remote = lvds->bridge->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
/* default set it as output rgb */
lvds->output = DISPLAY_OUTPUT_RGB;
-- 
2.11.0

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

On 2018-05-09 17:08, Andrzej Hajda wrote:
> On 04.05.2018 15:51, Peter Rosin wrote:
>> Bridge drivers can now (temporarily, in a transition phase) select if
>> they want to provide a full owner device or keep just providing an
>> of_node.
>>
>> By providing a full owner device, the bridge drivers no longer need
>> to provide an of_node since that node is available via the owner
>> device.
>>
>> When all bridge drivers provide an owner device, that will become
>> mandatory and the .of_node member will be removed.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 ++-
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
> 
> What is the reason to put rockchip here? Shouldn't be in separate patch?

Because the rockchip driver peeks into the bridge struct and all the
changes in this patch relate to making the new .odev member optional in
the transition phase, when the bridge can have either a new-style odev
or an old style of_node.

I guess this rockchip change could be patch 2, but it has to come first
after this patch and it makes no sense on its own. Hence, one patch.

This rockchip_lvds interaction is also present in patch 24/26
drm/bridge: remove the .of_node member

I can split them if you want, but I personally don't see the point.

>>  include/drm/drm_bridge.h | 2 ++
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 1638bfe9627c..3872f5379998 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
>> *np)
>>  mutex_lock(_lock);
>>  
>>  list_for_each_entry(bridge, _list, list) {
>> -if (bridge->of_node == np) {
>> +if ((bridge->odev && bridge->odev->of_node == np) ||
>> +bridge->of_node == np) {
>>  mutex_unlock(_lock);
>>  return bridge;
>>  }
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
>> b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 4bd94b167d2c..557e0079c98d 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> @@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, 
>> struct device *master,
>>  }
>>  if (lvds->panel)
>>  remote = lvds->panel->dev->of_node;
>> -else
>> +else if (lvds->bridge->of_node)
>>  remote = lvds->bridge->of_node;
>> +else
>> +remote = lvds->bridge->odev->of_node;
> 
> I guess odev should be NULL here, or have I missed something.

s/should/could/ ???

Assuming that was what you meant and answering accordingly...

No, .odev cannot be NULL in that else branch. drm_of_find_panel_or_bridge
either found a panel or a bridge (or it failed). If it found a bridge
(which is the relevant branch for this question) that bridge would have
to have either an of_node (in the transition phase) or a valid .odev.
Otherwise the bridge could not have been found by
drm_of_find_panel_or_bridge.

*time passes*

Ahh, yes, .odev is always NULL here so you probably did mean "should".
But after patches 2-23 when bridges start populating .odev *instead*
of .of_node, .odev will not remain NULL. But as I said above, while
.odev is NULL, .of_node will never be NULL and vice versa (or
drm_of_find_panel_or_bridge could not have found the bridge) so there
is no risk of a NULL dereference.

Cheers,
Peter

> 
> Regards
> Andrzej
> 
>>  if (of_property_read_string(dev->of_node, "rockchip,output", ))
>>  /* default set it as output rgb */
>>  lvds->output = DISPLAY_OUTPUT_RGB;
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..7c17977c3537 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -254,6 +254,7 @@ struct drm_bridge_timings {
>>  
>>  /**
>>   * struct drm_bridge - central DRM bridge control structure
>> + * @odev: device that owns the bridge
>>   * @dev: DRM device this bridge belongs to
>>   * @encoder: encoder to which this bridge is connected
>>   * @next: the next bridge in the encoder chain
>> @@ -265,6 +266,7 @@ struct drm_bridge_timings {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> +struct device *odev;
>>  struct drm_device *dev;
>>  struct drm_encoder *encoder;
>>  struct drm_bridge *next;
> 
> 

Re: [PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-05-07 15:56, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:51:46PM +0200, Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 25 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. Jyri, are
>> you able to test? That said, I might have missed some subtlety.
>>
>> Oh and the reason I'm pushing this is of course so that the issue
>> noted by Russel in [1] is addressed which in turn means that the
>> tda998x bridge driver can be patched according to that series without
>> objection (hopefully) and then used from the atmel-hlcdc driver (and
>> other drivers that are not componentized).
>>
>> Changes since v1https://lkml.org/lkml/2018/4/26/1018
>>
>> - rename .owner to .odev to not get mixed up with the module owner.
>> - added patches for new recent drivers thc63lvd1024 and cdns-dsi
>> - fix for problem in the rockchip_lvds driver reported by 0day
>> - added a WARN in drm_bridge_add if there is no .odev owner device
>>
>> I did *not*:
>> - add any ack from Daniel since he suggested "pdev", and I ended up
>>   with "odev" in the rename since I disliked "pdev" about as much
>>   as "owner".
> 
> As long as it's not owner, I'm fine :-) Ack on the idea still holds.
> 
>> - add any port id. The current .of_node (that this series removes)
>>   does not identify the port, so that problem seems orthogonal
>>   to me.
> 
> Hm, from my cursory DT/of code reading last week I thought the port is
> used to lookup the right node, but there's no port thing on the target for
> a phandle? At least that's how current drm_of_find_panel_or_bridge seems
> to work ...

drm_of_find_panel_or_bridge calls of_graph_get_remote_node and that
function looks up the main remote device node, i.e. not the remote
port or endpoint node but the parent node. So, bridges using .of_node
have stored their main device node in the .of_node member. I.e. the
same value as of_node in struct device for all current cases.

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>> [1] https://lkml.org/lkml/2018/4/23/769
>> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
>>
>> Peter Rosin (26):
>>   drm/bridge: allow optionally specifying an owner .odev device
>>   drm/bridge: adv7511: provide an owner .odev device
>>   drm/bridge/analogix: core: specify the owner .odev of the bridge
>>   drm/bridge: analogix-anx78xx: provide an owner .odev device
>>   drm/bridge: cdns-dsi: provide an owner .odev device
>>   drm/bridge: vga-dac: provide an owner .odev device
>>   drm/bridge: lvds-encoder: provide an owner .odev device
>>   drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
>> device
>>   drm/bridge: nxp-ptn3460: provide an owner .odev device
>>   drm/bridge: panel: provide an owner .odev device
>>   drm/bridge: ps8622: provide an owner .odev device
>>   drm/bridge: sii902x: provide an owner .odev device
>>   drm/bridge: sii9234: provide an owner .odev device
>>   drm/bridge: sii8620: provide an owner .odev device
>>   drm/bridge: synopsys: provide an owner .odev device for the bridges
>>   drm/bridge: tc358767: provide an owner .odev device
>>   drm/bridge: thc63lvd1024: provide an owner .odev device
>>   drm/bridge: ti-tfp410: provide an owner .odev device
>>   drm/exynos: mic: provide an owner .odev device for the bridge
>>   drm/mediatek: hdmi: provide an owner .odev device for the bridge
>>   drm/msm: specify the owner .odev of the bridges
>>   drm/rcar-du: lvds: provide an owner .odev device for the bridge
>>   drm/sti: provide an owner .odev device for the bridges
>>   drm/bridge: remove the .of_node member
>>   drm/bridge: require the owner .odev to be filled in on
>> drm_bridge_add/attach
>>   drm/bridge: establish a link between the bridge supplier and consumer
>>
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>>  drivers/gpu/drm/bridge/cdns-dsi.c  |  

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-07 14:59, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded.

The step is very small IMHO. Just a device-link, which is very easy to
remove once whatever other solution is ready.

>   Do we need to go this way?

If the drivers expect the parts to be there, and there is no other safety
net in place if they are not, what is the (short-term) alternative?

> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.

This patch does not contribute to any late init and any such delay is not
affected by this. At all.

> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

That is a much bigger question than this patch/series. Conflating the
two is not fair IMHO. You could run this very same argument for every
driver that gets added, since any additional driver will just make it
harder to make everything dynamic. Should we stop development right
away?

Besides, as long as the drm devices are in fact acting as big static
drivers (built from smaller parts), this should be considered a bug-fix
that will prevent dereference of stale pointers.

Or will some other solution appear and magically make all bridges and
drm drivers capable of dynamic reconfiguration in the next few weeks?
Yeah, right...

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>  include/drm/drm_bridge.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include 
>>  
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>  if (bridge->dev)
>>  return -EBUSY;
>>  
>> +if (encoder->dev->dev != bridge->odev) {
>> +bridge->link = device_link_add(encoder->dev->dev,
>> +   bridge->odev, 0);
>> +if (!bridge->link) {
>> +dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +dev_name(encoder->dev->dev));
>> +return -EINVAL;
>> +}
>> +}
>> +
>>  bridge->dev = encoder->dev;
>>  bridge->encoder = encoder;
>>  
>>  if (bridge->funcs->attach) {
>>  ret = bridge->funcs->attach(bridge);
>>  if (ret < 0) {
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>>  bridge->dev = NULL;
>>  bridge->encoder = NULL;
>>  return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  if (bridge->funcs->detach)
>>  bridge->funcs->detach(bridge);
>>  
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>> +
>>  bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  struct drm_bridge *next;
>>  struct list_head list;
>>  const struct drm_bridge_timings *timings;
>> +struct device_link *link;
>>  
>>  const struct drm_bridge_funcs *funcs;
>>  void *driver_private;
> 
> 

[PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner device or keep just providing an
of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 ++-
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
 include/drm/drm_bridge.h | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3872f5379998 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if (bridge->of_node == np) {
+   if ((bridge->odev && bridge->odev->of_node == np) ||
+   bridge->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..557e0079c98d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else
+   else if (lvds->bridge->of_node)
remote = lvds->bridge->of_node;
+   else
+   remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
/* default set it as output rgb */
lvds->output = DISPLAY_OUTPUT_RGB;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7c17977c3537 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @odev: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *odev;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-- 
2.11.0

[PATCH v2 22/26] drm/rcar-du: lvds: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..efda02f55c95 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
 
+   lvds->bridge.odev = >dev;
lvds->bridge.driver_private = lvds;
lvds->bridge.funcs = _lvds_bridge_ops;
-   lvds->bridge.of_node = pdev->dev.of_node;
 
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lvds->mmio = devm_ioremap_resource(>dev, mem);
-- 
2.11.0

[PATCH v2 24/26] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +--
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 include/drm/drm_bridge.h | 4 
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3872f5379998..df084db33494 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if ((bridge->odev && bridge->odev->of_node == np) ||
-   bridge->of_node == np) {
+   if (bridge->odev->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 557e0079c98d..e77d4c909582 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else if (lvds->bridge->of_node)
-   remote = lvds->bridge->of_node;
else
remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7c17977c3537..b656e505d11e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-#ifdef CONFIG_OF
-   struct device_node *of_node;
-#endif
struct list_head list;
const struct drm_bridge_timings *timings;
 
-- 
2.11.0

[PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Peter Rosin]

The .odev owner device will be handy to have around.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index df084db33494..78d186b6831b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+   if (WARN_ON(!bridge->odev))
+   return;
+
mutex_lock(_lock);
list_add_tail(>list, _list);
mutex_unlock(_lock);
@@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (!encoder || !bridge)
return -EINVAL;
 
+   if (WARN_ON(!bridge->odev))
+   return -EINVAL;
+
if (previous && (!previous->dev || previous->encoder != encoder))
return -EINVAL;
 
-- 
2.11.0

[PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++
 include/drm/drm_bridge.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 78d186b6831b..0259f0a3ff27 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "drm_crtc_internal.h"
@@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   if (encoder->dev->dev != bridge->odev) {
+   bridge->link = device_link_add(encoder->dev->dev,
+  bridge->odev, 0);
+   if (!bridge->link) {
+   dev_err(bridge->odev, "failed to link bridge to %s\n",
+   dev_name(encoder->dev->dev));
+   return -EINVAL;
+   }
+   }
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
+
bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b656e505d11e..804189c63a4c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
struct drm_bridge *next;
struct list_head list;
const struct drm_bridge_timings *timings;
+   struct device_link *link;
 
const struct drm_bridge_funcs *funcs;
void *driver_private;
-- 
2.11.0

[PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russel King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russel.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 25 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. Jyri, are
you able to test? That said, I might have missed some subtlety.

Oh and the reason I'm pushing this is of course so that the issue
noted by Russel in [1] is addressed which in turn means that the
tda998x bridge driver can be patched according to that series without
objection (hopefully) and then used from the atmel-hlcdc driver (and
other drivers that are not componentized).

Changes since v1https://lkml.org/lkml/2018/4/26/1018

- rename .owner to .odev to not get mixed up with the module owner.
- added patches for new recent drivers thc63lvd1024 and cdns-dsi
- fix for problem in the rockchip_lvds driver reported by 0day
- added a WARN in drm_bridge_add if there is no .odev owner device

I did *not*:
- add any ack from Daniel since he suggested "pdev", and I ended up
  with "odev" in the rename since I disliked "pdev" about as much
  as "owner".
- add any port id. The current .of_node (that this series removes)
  does not identify the port, so that problem seems orthogonal
  to me.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (26):
  drm/bridge: allow optionally specifying an owner .odev device
  drm/bridge: adv7511: provide an owner .odev device
  drm/bridge/analogix: core: specify the owner .odev of the bridge
  drm/bridge: analogix-anx78xx: provide an owner .odev device
  drm/bridge: cdns-dsi: provide an owner .odev device
  drm/bridge: vga-dac: provide an owner .odev device
  drm/bridge: lvds-encoder: provide an owner .odev device
  drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
device
  drm/bridge: nxp-ptn3460: provide an owner .odev device
  drm/bridge: panel: provide an owner .odev device
  drm/bridge: ps8622: provide an owner .odev device
  drm/bridge: sii902x: provide an owner .odev device
  drm/bridge: sii9234: provide an owner .odev device
  drm/bridge: sii8620: provide an owner .odev device
  drm/bridge: synopsys: provide an owner .odev device for the bridges
  drm/bridge: tc358767: provide an owner .odev device
  drm/bridge: thc63lvd1024: provide an owner .odev device
  drm/bridge: ti-tfp410: provide an owner .odev device
  drm/exynos: mic: provide an owner .odev device for the bridge
  drm/mediatek: hdmi: provide an owner .odev device for the bridge
  drm/msm: specify the owner .odev of the bridges
  drm/rcar-du: lvds: provide an owner .odev device for the bridge
  drm/sti: provide an owner .odev device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the owner .odev to be filled in on
drm_bridge_add/attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/cdns-dsi.c  |  2 +-
 drivers/gpu/drm/bridge/dumb-vga-dac.c  |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c  |  2 +-
 .../drm/bridge/megachips-stdp-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c   |  2 +-
 drivers/gpu/drm/bridge/panel.c |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c |  2 +-
 drivers/gpu/drm/bridge/sii902x.c   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c   |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c  |  4 +---
 drivers/gpu/drm/bridge/tc358767.c  |  2 +-
 drivers/gpu/drm/bridge/thc63lvd1024.c  |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c |  2 +-
 drivers/gpu/drm/drm_bridge.c   | 26 +-
 drivers/gpu/drm/exynos/exynos_drm_mic.c|  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c|  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c  |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c|  2 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c   |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c  |  2 +-
 drivers/gpu/drm/sti/sti_hda.c  |  1 +
 drivers/gpu/drm

Re: [PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-04-30 17:32, Daniel Vetter wrote:
> On Fri, Apr 27, 2018 at 12:31:39AM +0200, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
> 
> Minus the ->owner bikeshed I brought up in the previous patch I agree with
> this approach as the best way to move forward for now.
> 
> Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>

Thanks, let's see if Laurent is also on-board...

> One small suggestion below, for merging I'd say pls get Jyri's
> review/tested-by too, since you're both working on the same problem it
> seems.

Yes, I too would be very happy to see a tested-by from someone.

> Aside: Do you want commit rights to drm-misc to be able to push work like
> this?

If that makes life easier, sure.

Cheers,
Peter

Re: [PATCH 23/24] drm/bridge: require the .owner to be filled in on drm_bridge_attach

[Peter Rosin]

On 2018-04-30 17:24, Daniel Vetter wrote:
> On Fri, Apr 27, 2018 at 12:31:38AM +0200, Peter Rosin wrote:
>> The .owner will be handy to have around.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 9f023bd84d56..a038da696802 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -115,6 +115,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>  if (!encoder || !bridge)
>>  return -EINVAL;
>>  
>> +if (WARN_ON(!bridge->owner))
>> +return -EINVAL;
> 
> I think conceptually this is checked at the wrong place, and I think also 
> misnamed
> a bit. The ->owner is essentially the struct device (and its associated
> driver) that provides the drm_bridge. As such it should be filled out
> already at drm_bridge_add() time, and I think the check should be in
> there. For driver-internal bridges it might make sense to also check this
> here, not sure. Or just require all bridges get added.

The reason for the position is that while I originally had the WARN in
drm_bridge_add, I found that quite a few bridges never call drm_bridge_add.
So I moved it. Other options are to start requiring all bridge suppliers to
call drm_bridge_add or to have the WARN in both function. Too me, it would
make sense to require all bridge suppliers to call drm_bridge_add, as that
enables other init stuff later, when needed. But that is a hairy patch to
get right, and is probably best left as a separate series.

> Wrt the name, I think we should call this pdev or something. ->owner
> usually means the module owner. I think in other subsystems ->dev is used,
> but in drm we use ->dev for the drm_device pointer, so totally different
> thing. pdev = physical device is the best I came up with. Better
> suggestions very much welcome.

pdev is about as problematic as owner. To me it reads "platform device".
And dev for a drm_device is also somewhat problematic, and I think that
drm would have been better, but dev for drm_device is probably quite
common. But one way to go is to rename the current dev to drm, so that
dev is freed up for the owner/supplier device. But that is a tedious
patch to write (I don't do the cocci thing).

Other suggestions I can think of: odev for owner device, sdev for supplier
device or just plain supplier.

Cheers,
Peter

> -Daniel
> 
>> +
>>  if (previous && (!previous->dev || previous->encoder != encoder))
>>  return -EINVAL;
>>  
>> -- 
>> 2.11.0
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 22/24] drm/bridge: remove the .of_node member

[Peter Rosin]

On 2018-04-28 10:09, kbuild test robot wrote:
> Hi Peter,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on v4.17-rc2]
> [also build test ERROR on next-20180426]
> [cannot apply to drm/drm-next robclark/msm-next 
> drm-exynos/exynos-drm/for-next]
> [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/Peter-Rosin/device-link-bridge-supplier-drm-device/20180428-135229
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/gpu//drm/rockchip/rockchip_lvds.c: In function 
> 'rockchip_lvds_bind':
>>> drivers/gpu//drm/rockchip/rockchip_lvds.c:381:24: error: 'struct 
>>> drm_bridge' has no member named 'of_node'
>   remote = lvds->bridge->of_node;
>^~
> 
> vim +381 drivers/gpu//drm/rockchip/rockchip_lvds.c

Ugh.

So, patch 1/24 needs to be amended with this

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..3f33034b3f58 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else
+   else if (lvds->bridge->of_node)
remote = lvds->bridge->of_node;
+   else
+   remote = lvds->bridge->owner->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
/* default set it as output rgb */
lvds->output = DISPLAY_OUTPUT_RGB;


and patch 22/24 with this

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 3f33034b3f58..8c82fa647536 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else if (lvds->bridge->of_node)
-   remote = lvds->bridge->of_node;
else
remote = lvds->bridge->owner->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))


But that is of course just a stop-gap. The real fix is to adapt to the
"drm: bridge: Add support for static image formats​" series from Jacopo.
But that's orthogonal.

Cheers,
Peter

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 09:37, Peter Rosin wrote:
> On 2018-04-27 09:11, Andrzej Hajda wrote:
>> Hi Peter,
>>
>> On 27.04.2018 00:31, Peter Rosin wrote:
>>> Hi!
>>>
>>> It was noted by Russel King [1] that bridges (not using components)
>>> might disappear unexpectedly if the owner of the bridge was unbound.
>>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>>> came up with using device links to resolve the panel issue, which
>>> was also my (independent) reaction to the note from Russel.
>>>
>>> This series builds up to the addition of that link in the last
>>> patch, but in my opinion the other 23 patches do have merit on their
>>> own.
>>>
>>> The last patch needs testing, while the others look trivial. That
>>> said, I might have missed some subtlety.
>>
>> of_node is used as an identifier of the bridge in the kernel. If you
>> replace it with device pointer there will be potential problem with
>> devices having two or more bridges, how do you differentiate bridges if
>> the owner is the same? If I remember correctly current bridge code does
>> not allow to have multiple bridges in one device, but that should be
>> quite easy to fix if necessary. After this change it will become more
>> difficult.
> 
> I don't see how it will be more difficult?
> 
>> Anyway I remember discussion that in DT world bridge should be
>> identified rather by of_graph port node, not by parent node as it is
>> now. If you want to translate this relation to device owner, you should
>> add also port number to have full identification of the bridge, ie pair
>> (owner, port_number) would be equivalent of port node.
> 
> You even state the trivial solution here, just add the port/endpoint ID
> when/if it is needed. So, what is the significant difference?

Or, since this is apparently a rare requirement, you could make the owners
that do need it fix it themselves. E.g. by embedding the struct drm_bridge
in another struct that contains the needed ID, and use container_of to get
to that containing struct with the ID.

Cheers,
Peter

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 09:11, Andrzej Hajda wrote:
> Hi Peter,
> 
> On 27.04.2018 00:31, Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 23 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. That
>> said, I might have missed some subtlety.
> 
> of_node is used as an identifier of the bridge in the kernel. If you
> replace it with device pointer there will be potential problem with
> devices having two or more bridges, how do you differentiate bridges if
> the owner is the same? If I remember correctly current bridge code does
> not allow to have multiple bridges in one device, but that should be
> quite easy to fix if necessary. After this change it will become more
> difficult.

I don't see how it will be more difficult?

> Anyway I remember discussion that in DT world bridge should be
> identified rather by of_graph port node, not by parent node as it is
> now. If you want to translate this relation to device owner, you should
> add also port number to have full identification of the bridge, ie pair
> (owner, port_number) would be equivalent of port node.

You even state the trivial solution here, just add the port/endpoint ID
when/if it is needed. So, what is the significant difference?

Cheers,
Peter

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 01:18, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Friday, 27 April 2018 02:09:14 EEST Peter Rosin wrote:
>> On 2018-04-27 00:40, Laurent Pinchart wrote:
>>> On Friday, 27 April 2018 01:31:15 EEST Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> It was noted by Russel King [1] that bridges (not using components)
>>>> might disappear unexpectedly if the owner of the bridge was unbound.
>>>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>>>> came up with using device links to resolve the panel issue, which
>>>> was also my (independent) reaction to the note from Russel.
>>>>
>>>> This series builds up to the addition of that link in the last
>>>> patch, but in my opinion the other 23 patches do have merit on their
>>>> own.
>>>>
>>>> The last patch needs testing, while the others look trivial. That
>>>> said, I might have missed some subtlety.
>>>
>>> I don't think this is the way to go. We have a real lifetime management
>>> problem with bridges, and device links are just trying to hide the problem
>>> under the carpet. They will further corner us by making a real fix much
>>> more difficult to implement. I'll try to comment further in the next few
>>> days on what I think a better solution would be, but in a nutshell I
>>> believe that drm_bridge objects need to be refcounted, with a .release()
>>> operation to free the bridge resources when the reference count drops to
>>> zero. This shouldn't be difficult to implement and I'm willing to help.
>>
>> Ok, sp 24/24 is dead, and maybe 23/24 too.
> 
> Not necessarily, maybe I'll get proven wrong :-) Let's discuss, but I first 
> need some sleep.

Ok, I'll add my view...

I don't see how a refcount is going to resolve anything. Let's take some
device that allocs a bridge that is later attached to some encoder. In doing
so it adds hooks to some of the drm_bridge_funcs. If you add a refcount to
the bridge itself then yes, the bridge object might remain but the code
backing the hook functions will go away the moment the owner device goes
away. So, you'd have to refcount the owner device itself to prevent it
from going away. But, you can't stop a device from going away IIUC, you can
only bring other stuff down with it in an orderly fashion.

Components, that some bridges use, takes care of bringing the whole cluster
down *before* any device goes away, so that is obviously a solution. But
that solution is not in place everywhere.

Now, my device-link is pretty light-weight. And it should only matter if
the owner goes away before the consuming drm device has brought down the
encoder properly. If that ever happens, we're in trouble. So, the link can
at worst only substitute one problem with another, and at best it prevents
the fireworks.

Sure, there's the reprobe problem if the bridge's owner device shows up
again, but that's pretty minor compared to a hard crash. And there was a
patch for that, so in the end that may be a non-issue.

If some other solution is found, removing the device-link is trivial. It is
localized to bridge attach/detach and nothing else is affected (in terms of
code).

Cheers,
Peter

>> But how do you feel about dropping .of_node in favour of .owner? That gets
>> rid of ugly #ifdefs...
> 
> I'll review that part and reply, I have nothing against it on principle at 
> the 
> moment. The more generic the code is, the better in my opinion. We just need 
> to make sure that the OF node we're interested in will always be .owner-
>> of_node in all cases.
> 
>> I also have the nagging feeling that .driver_private serves very little real
>> purpose if there is a .owner so that you can do
>>
>>  dev_get_drvdata(bridge->owner)
>>
>> for the cases where the container_of macro is not appropriate.
> 
> I'll review that too, it's a good point.
> 

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 00:40, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patches.
> 
> On Friday, 27 April 2018 01:31:15 EEST Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 23 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. That
>> said, I might have missed some subtlety.
> 
> I don't think this is the way to go. We have a real lifetime management 
> problem with bridges, and device links are just trying to hide the problem 
> under the carpet. They will further corner us by making a real fix much more 
> difficult to implement. I'll try to comment further in the next few days on 
> what I think a better solution would be, but in a nutshell I believe that 
> drm_bridge objects need to be refcounted, with a .release() operation to free 
> the bridge resources when the reference count drops to zero. This shouldn't 
> be 
> difficult to implement and I'm willing to help.

Ok, sp 24/24 is dead, and maybe 23/24 too. But how do you feel about dropping
.of_node in favour of .owner? That gets rid of ugly #ifdefs...

I also have the nagging feeling that .driver_private serves very little real
purpose if there is a .owner so that you can do

dev_get_drvdata(bridge->owner)

for the cases where the container_of macro is not appropriate.

Cheers,
Peter

[PATCH 01/24] drm/bridge: allow optionally specifying an .owner device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner or keep just providing an of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 ++-
 include/drm/drm_bridge.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..67147673fdeb 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if (bridge->of_node == np) {
+   if ((bridge->owner && bridge->owner->of_node == np) ||
+   bridge->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..c28a75ad0ae2 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *owner;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-- 
2.11.0

[PATCH 20/24] drm/rcar-du: lvds: provide an .owner device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..5984c70b5590 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
 
+   lvds->bridge.owner = >dev;
lvds->bridge.driver_private = lvds;
lvds->bridge.funcs = _lvds_bridge_ops;
-   lvds->bridge.of_node = pdev->dev.of_node;
 
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lvds->mmio = devm_ioremap_resource(>dev, mem);
-- 
2.11.0

[PATCH 22/24] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +--
 include/drm/drm_bridge.h | 4 
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 67147673fdeb..9f023bd84d56 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if ((bridge->owner && bridge->owner->of_node == np) ||
-   bridge->of_node == np) {
+   if (bridge->owner->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c28a75ad0ae2..3bc659f3e7d2 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-#ifdef CONFIG_OF
-   struct device_node *of_node;
-#endif
struct list_head list;
const struct drm_bridge_timings *timings;
 
-- 
2.11.0

[PATCH 23/24] drm/bridge: require the .owner to be filled in on drm_bridge_attach

[Peter Rosin]

The .owner will be handy to have around.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 9f023bd84d56..a038da696802 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -115,6 +115,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (!encoder || !bridge)
return -EINVAL;
 
+   if (WARN_ON(!bridge->owner))
+   return -EINVAL;
+
if (previous && (!previous->dev || previous->encoder != encoder))
return -EINVAL;
 
-- 
2.11.0

[PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++
 include/drm/drm_bridge.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index a038da696802..f0c79043ec43 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "drm_crtc_internal.h"
@@ -124,12 +125,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   if (encoder->dev->dev != bridge->owner) {
+   bridge->link = device_link_add(encoder->dev->dev,
+  bridge->owner, 0);
+   if (!bridge->link) {
+   dev_err(bridge->owner, "failed to link bridge to %s\n",
+   dev_name(encoder->dev->dev));
+   return -EINVAL;
+   }
+   }
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -156,6 +170,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
+
bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3bc659f3e7d2..9a386559a41a 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
struct drm_bridge *next;
struct list_head list;
const struct drm_bridge_timings *timings;
+   struct device_link *link;
 
const struct drm_bridge_funcs *funcs;
void *driver_private;
-- 
2.11.0

[PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russel King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russel.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 23 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. That
said, I might have missed some subtlety.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (24):
  drm/bridge: allow optionally specifying an .owner device
  drm/bridge: adv7511: provide an .owner device
  drm/bridge/analogix: core: specify the .owner of the bridge
  drm/bridge: analogix-anx78xx: provide an .owner device
  drm/bridge: vga-dac: provide an .owner device
  drm/bridge: lvds-encoder: provide an .owner device
  drm/bridge: megachips-stdp-ge-b850v3-fw: provide an .owner device
  drm/bridge: nxp-ptn3460: provide an .owner device
  drm/bridge: panel: provide an .owner device
  drm/bridge: ps8622: provide an .owner device
  drm/bridge: sii902x: provide an .owner device
  drm/bridge: sii9234: provide an .owner device
  drm/bridge: sii8620: provide an .owner device
  drm/bridge: synopsys: provide an .owner device for the bridges
  drm/bridge: tc358767: provide an .owner device
  drm/bridge: ti-tfp410: provide an .owner device
  drm/exynos: mic: provide an .owner device for the bridge
  drm/mediatek: hdmi: provide an .owner device for the bridge
  drm/msm: specify the .owner of the bridges
  drm/rcar-du: lvds: provide an .owner device for the bridge
  drm/sti: provide an .owner device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the .owner to be filled in on drm_bridge_attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/dumb-vga-dac.c  |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c  |  2 +-
 .../drm/bridge/megachips-stdp-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c   |  2 +-
 drivers/gpu/drm/bridge/panel.c |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c |  2 +-
 drivers/gpu/drm/bridge/sii902x.c   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c   |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c  |  4 +---
 drivers/gpu/drm/bridge/tc358767.c  |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c |  2 +-
 drivers/gpu/drm/drm_bridge.c   | 23 +-
 drivers/gpu/drm/exynos/exynos_drm_mic.c|  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c|  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c  |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c|  2 +-
 drivers/gpu/drm/sti/sti_dvo.c  |  2 +-
 drivers/gpu/drm/sti/sti_hda.c  |  1 +
 drivers/gpu/drm/sti/sti_hdmi.c |  1 +
 include/drm/drm_bridge.h   |  8 
 27 files changed, 51 insertions(+), 33 deletions(-)

-- 
2.11.0

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Peter Rosin]

On 2018-04-23 09:28, jacopo mondi wrote:
> Hi Peter,
>thanks for looking into this
> 
> On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
>> On 2018-04-19 11:31, Jacopo Mondi wrote:
>>> With the introduction of static input image format enumeration in DRM
>>> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
>>> from both panel or bridge, to set the desired LVDS mode.
>>>
>>> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
>>> format, as it is only defined for drm connectors, but use the newly
>>> introduced _LE version of LVDS mbus image formats.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> ---
>>>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 
>>> +
>>>  1 file changed, 44 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
>>> b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>>> index 3d2d3bb..2fa875f 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>>> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge 
>>> *bridge,
>>> return true;
>>>  }
>>>
>>> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
>>> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
>>> + unsigned int *bus_fmt)
>>>  {
>>> struct drm_display_info *info = >connector.display_info;
>>> -   enum rcar_lvds_mode mode;
>>> -
>>> -   /*
>>> -* There is no API yet to retrieve LVDS mode from a bridge, only panels
>>> -* are supported.
>>> -*/
>>> -   if (!lvds->panel)
>>> -   return;
>>>
>>> if (!info->num_bus_formats || !info->bus_formats) {
>>> dev_err(lvds->dev, "no LVDS bus format reported\n");
>>> -   return;
>>> +   return -EINVAL;
>>> +   }
>>> +
>>> +   *bus_fmt = info->bus_formats[0];
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
>>> +  unsigned int *bus_fmt)
>>> +{
>>> +   if (!lvds->next_bridge->num_bus_formats ||
>>> +   !lvds->next_bridge->bus_formats) {
>>> +   dev_err(lvds->dev, "no LVDS bus format reported\n");
>>> +   return -EINVAL;
>>> }
>>>
>>> -   switch (info->bus_formats[0]) {
>>> +   *bus_fmt = lvds->next_bridge->bus_formats[0];
>>
>> What makes the first reported format the best choice?
> 
> It already was the selection 'policy' in place in this driver before
> introducing bridge formats. As you can see from the switch I have here
> removed, the first format was selected even when only the format
> reported by the connector was inspected.

Well, *if* some bridge/panel do support more than one format, and your
driver depends on it being the first reported format, then I can easily
see that some other driver also requires its expected format to be first.
Then we might end up in a war over what format should be reported as the
first so that this multi-input bridge/panel could be used by both drivers.

> And, anyway, as DRM lacks a format negotiation API, there is no way to
> tell a bridge/panel "use this format instead of this other one" (which
> makes me wonders why more formats can be reported, but the
> bus_formats[] helpers for connectors allow that, so I thought it made
> sense to do the same for bridges).

Since there is no way to negotiate, I would assume that the other end
really does support all reported formats (in some automagical way). To
me, the only sensible approach is to loop over the formats and see if
*any* of them fits, and assume that something else deals with the
details.

>>
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
>>> +{
>>> +   unsigned int bus_fmt;
>>> +   int ret;
>>> +
>>> +   if (lvds->panel)
>>> +   ret = rcar_lvds_get_lvds_mode_from_connector(lvds, _fmt);
>>> +   else
>>> +   ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, _fmt);
>>
>> What if no bridge reports any format, shouldn't the connector be examined
>> then?
> 
> Th

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> With the introduction of static input image format enumeration in DRM
> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> from both panel or bridge, to set the desired LVDS mode.
> 
> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> format, as it is only defined for drm connectors, but use the newly
> introduced _LE version of LVDS mbus image formats.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 
> +
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
> b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3d2d3bb..2fa875f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge 
> *bridge,
>   return true;
>  }
>  
> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
> +   unsigned int *bus_fmt)
>  {
>   struct drm_display_info *info = >connector.display_info;
> - enum rcar_lvds_mode mode;
> -
> - /*
> -  * There is no API yet to retrieve LVDS mode from a bridge, only panels
> -  * are supported.
> -  */
> - if (!lvds->panel)
> - return;
>  
>   if (!info->num_bus_formats || !info->bus_formats) {
>   dev_err(lvds->dev, "no LVDS bus format reported\n");
> - return;
> + return -EINVAL;
> + }
> +
> + *bus_fmt = info->bus_formats[0];
> +
> + return 0;
> +}
> +
> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
> +unsigned int *bus_fmt)
> +{
> + if (!lvds->next_bridge->num_bus_formats ||
> + !lvds->next_bridge->bus_formats) {
> + dev_err(lvds->dev, "no LVDS bus format reported\n");
> + return -EINVAL;
>   }
>  
> - switch (info->bus_formats[0]) {
> + *bus_fmt = lvds->next_bridge->bus_formats[0];

What makes the first reported format the best choice?

> +
> + return 0;
> +}
> +
> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> +{
> + unsigned int bus_fmt;
> + int ret;
> +
> + if (lvds->panel)
> + ret = rcar_lvds_get_lvds_mode_from_connector(lvds, _fmt);
> + else
> + ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, _fmt);

What if no bridge reports any format, shouldn't the connector be examined
then?

> + if (ret)
> + return;
> +
> + switch (bus_fmt) {
> + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> + lvds->mode |= RCAR_LVDS_MODE_MIRROR;
>   case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
>   case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> - mode = RCAR_LVDS_MODE_JEIDA;
> + lvds->mode = RCAR_LVDS_MODE_JEIDA;

This is b0rken, first the mirror bit is ORed into some unknown preexisting
value, then the code falls through (without any fall through comment, btw)
and forcibly sets the mode, thus discarding the mirror bit which was
carefully ORed in.

>   break;
> +
> + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> + lvds->mode |= RCAR_LVDS_MODE_MIRROR;
>   case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> - mode = RCAR_LVDS_MODE_VESA;
> + lvds->mode = RCAR_LVDS_MODE_VESA;

Dito.

Cheers,
Peter

>   break;
>   default:
>   dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> - info->bus_formats[0]);
> - return;
> + bus_fmt);
>   }
> -
> - if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> - mode |= RCAR_LVDS_MODE_MIRROR;
> -
> - lvds->mode = mode;
>  }
>  
>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> 

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> modes, selectable by means of an external pin.
> 
> Add support for configurable LVDS input mapping modes, using the newly
> introduced support for bridge input image formats.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 
> +++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index 48527f8..a3071a1 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -10,9 +10,15 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  
> +enum thc63_lvds_mapping_mode {
> + THC63_LVDS_MAP_MODE2,
> + THC63_LVDS_MAP_MODE1,
> +};
> +
>  enum thc63_ports {
>   THC63_LVDS_IN0,
>   THC63_LVDS_IN1,
> @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>   return 0;
>  }
>  
> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> +{
> + u32 bus_fmt;
> + u32 map;
> + int ret;
> +
> + ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
> + if (ret) {
> + dev_err(thc63->dev,
> + "Unable to parse property \"thine,map\": %d\n", ret);
> + return ret;
> + }
> +
> + switch (map) {
> + case THC63_LVDS_MAP_MODE1:
> + bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> + break;
> + case THC63_LVDS_MAP_MODE2:
> + bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;

Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
or rgb101010/1x7x5, no?

Cheers,
Peter

> + break;
> + default:
> + dev_err(thc63->dev,
> + "Invalid value for property \"thine,map\": %u\n", map);
> + return -EINVAL;
> + }
> +
> + drm_bridge_set_bus_formats(>bridge, _fmt, 1);
> +
> + return 0;
> +}
> +
>  static int thc63_gpio_init(struct thc63_dev *thc63)
>  {
>   thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
>   if (ret)
>   return ret;
>  
> + ret = thc63_set_bus_fmt(thc63);
> + if (ret)
> + return ret;
> +
>   thc63->bridge.driver_private = thc63;
>   thc63->bridge.of_node = pdev->dev.of_node;
>   thc63->bridge.funcs = _bridge_func;
> 

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> modes, selectable by means of an external pin.
> 
> Describe the LVDS mode map through a newly defined mandatory property in
> device tree bindings.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt  | 3 
> +++
>  1 file changed, 3 insertions(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> index 37f0c04..0937595 100644
> --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -12,6 +12,8 @@ Required properties:
>  - compatible: Shall be "thine,thc63lvd1024"
>  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
>PPL and digital circuitry
> +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be <1>
> +  for mapping mode 1, <0> for mapping mode 2

Since the MAP pin is an input pin, I would expect there to be an optional gpio
specifier like thine,map-gpios so that the driver can set it according to
the value given in thine,map in case the HW has a line from some gpio output
to the MAP pin (instead of hardwired hi/low which seem to be your thinking).

Cheers,
Peter

>  
>  Optional properties:
>  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> @@ -36,6 +38,7 @@ Example:
>  
>   vcc-supply = <_lvds_vcc>;
>   powerdown-gpios = < 15 GPIO_ACTIVE_LOW>;
> + thine,map = <1>;
>  
>   ports {
>   #address-cells = <1>;
> 

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> Add support for storing image format information in DRM bridges with
> associated helper function.
> 
> This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> is for connectors.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/drm_bridge.c | 30 ++
>  include/drm/drm_bridge.h |  8 
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe..e2ad098 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  }
>  
>  /**
> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> + * @bridge: the bridge to set image formats in
> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> + * @num_formats: number of elements in the @formats array
> + *
> + * Store a list of supported image formats in a bridge.
> + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h 
> for
> + * a full list of available formats.
> + */
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *formats,
> +unsigned int num_formats)
> +{
> + u32 *fmts;
> +
> + if (!formats || !num_formats)
> + return -EINVAL;

I see no compelling reason to forbid restoring the number of reported
input formats to zero? I can't think of a use right now of course, but it
seems a bit odd all the same.

Cheers,
Peter

> +
> + fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> + if (!fmts)
> + return -ENOMEM;
> +
> + kfree(bridge->bus_formats);
> + bridge->bus_formats = fmts;
> + bridge->num_bus_formats = num_formats;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> +
> +/**
>   * DOC: bridge callbacks
>   *
>   * The _bridge_funcs ops are populated by the bridge driver. The DRM
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec..6b3648c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
>   * @of_node: device node pointer to the bridge
> + * @bus_formats: wire image formats. Array of @num_bus_formats 
> MEDIA_BUS_FMT\_
> + * elements
> + * @num_bus_formats: size of @bus_formats array
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> @@ -271,6 +274,9 @@ struct drm_bridge {
>  #ifdef CONFIG_OF
>   struct device_node *of_node;
>  #endif
> + const u32 *bus_formats;
> + unsigned int num_bus_formats;
> +
>   struct list_head list;
>   const struct drm_bridge_timings *timings;
>  
> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>   struct drm_display_mode *adjusted_mode);
>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>  void drm_bridge_enable(struct drm_bridge *bridge);
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> +unsigned int num_fmts);
>  
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> 

Re: [RFC v2 1/6] mux: include compiler.h from mux/consumer.h

[Peter Rosin]

Hi Ulrich,

This patch looks good to me. Let me know if I should feed this
through the mux subsystem or if it will take some other route.

In case someone else ends up taking it:
Acked-by: Peter Rosin <p...@axentia.se>

Cheers,
Peter

On 2017-07-17 17:24, Ulrich Hecht wrote:
> Required for __must_check.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+rene...@gmail.com>
> ---
>  include/linux/mux/consumer.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 5577e1b..ea96d4c 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -13,6 +13,8 @@
>  #ifndef _LINUX_MUX_CONSUMER_H
>  #define _LINUX_MUX_CONSUMER_H
>  
> +#include 
> +
>  struct device;
>  struct mux_control;
>  
> 

Re: [RFC v2 3/6] serdev: add multiplexer support

[Peter Rosin]

On 2017-07-17 17:24, Ulrich Hecht wrote:
> Adds an interface for slave device multiplexing using the mux subsystem.
> 
> Signed-off-by: Ulrich Hecht 
> ---
>  drivers/tty/serdev/Kconfig  |  3 +++
>  drivers/tty/serdev/Makefile |  1 +
>  drivers/tty/serdev/core.c   | 14 +-
>  drivers/tty/serdev/mux.c| 66 
> +
>  include/linux/serdev.h  | 16 ---
>  5 files changed, 96 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/tty/serdev/mux.c
> 
> diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
> index cdc6b82..9096b71 100644
> --- a/drivers/tty/serdev/Kconfig
> +++ b/drivers/tty/serdev/Kconfig
> @@ -13,4 +13,7 @@ config SERIAL_DEV_CTRL_TTYPORT
>   depends on TTY
>   depends on SERIAL_DEV_BUS != m
>  
> +config SERIAL_DEV_MUX
> + bool "Serial device multiplexer support"

Here, you should

select MULTIPLEXER

And, instead of adding a config option, you might want to consider
making the mux optional instead. See

https://lkml.org/lkml/2017/7/14/655

> +
>  endif
> diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
> index 0cbdb94..d713381 100644
> --- a/drivers/tty/serdev/Makefile
> +++ b/drivers/tty/serdev/Makefile
> @@ -1,5 +1,6 @@
>  serdev-objs := core.o
>  
>  obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
> +obj-$(CONFIG_SERIAL_DEV_MUX) += mux.o
>  
>  obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 1fbaa4c..92c5bfa 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -305,7 +305,8 @@ struct serdev_device *serdev_device_alloc(struct 
> serdev_controller *ctrl)
>   return NULL;
>  
>   serdev->ctrl = ctrl;
> - ctrl->serdev = serdev;
> + if (!ctrl->serdev)
> + ctrl->serdev = serdev;
>   device_initialize(>dev);
>   serdev->dev.parent = >dev;
>   serdev->dev.bus = _bus_type;
> @@ -368,6 +369,8 @@ static int of_serdev_register_devices(struct 
> serdev_controller *ctrl)
>   struct serdev_device *serdev = NULL;
>   int err;
>   bool found = false;
> + int nr = 0;
> + u32 reg;
>  
>   for_each_available_child_of_node(ctrl->dev.of_node, node) {
>   if (!of_get_property(node, "compatible", NULL))
> @@ -380,6 +383,10 @@ static int of_serdev_register_devices(struct 
> serdev_controller *ctrl)
>   continue;
>  
>   serdev->dev.of_node = node;
> + serdev->nr = nr++;
> +
> + if (!of_property_read_u32(node, "reg", ))
> + serdev->mux_addr = reg;
>  
>   err = serdev_device_add(serdev);
>   if (err) {
> @@ -414,6 +421,11 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>   if (ret)
>   return ret;
>  
> +#ifdef CONFIG_SERIAL_DEV_MUX
> + if (of_serdev_register_mux(ctrl) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +#endif
> +
>   ret = of_serdev_register_devices(ctrl);
>   if (ret)
>   goto out_dev_del;
> diff --git a/drivers/tty/serdev/mux.c b/drivers/tty/serdev/mux.c
> new file mode 100644
> index 000..fc9405b
> --- /dev/null
> +++ b/drivers/tty/serdev/mux.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (C) 2017 Ulrich Hecht
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +int serdev_device_mux_select(struct serdev_device *serdev)
> +{
> + struct mux_control *mux = serdev->ctrl->mux;
> + int ret;
> +
> + if (!mux)
> + return -EINVAL;
> +
> + ret = mux_control_select(mux, serdev->mux_addr);
> + serdev->ctrl->mux_do_not_deselect = ret < 0;
> +
> + serdev->ctrl->serdev = serdev;
> +
> + return ret;
> +}
> +
> +int serdev_device_mux_deselect(struct serdev_device *serdev)
> +{
> + struct mux_control *mux = serdev->ctrl->mux;
> +
> + if (!mux)
> + return -EINVAL;
> +
> + if (!serdev->ctrl->mux_do_not_deselect)
> + return mux_control_deselect(mux);
> + else
> + return 0;
> +}
> +
> +int of_serdev_register_mux(struct serdev_controller *ctrl)
> +{
> + struct mux_control *mux;
> + struct device *dev = >dev;
> +
> + mux = devm_mux_control_get(dev, NULL);
> +
> + if (IS_ERR(mux)) {
> + if (PTR_ERR(mux) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get control mux\n");
> +  

Re: [RFC 3/4] max9260: add driver for i2c over GMSL passthrough

[Peter Rosin]

On 2017-06-14 16:38, Ulrich Hecht wrote:
> This driver implements tunnelling of i2c requests over GMSL via a
> MAX9260 deserializer. It provides an i2c adapter that can be used
> to reach devices on the far side of the link.
> 
> Signed-off-by: Ulrich Hecht 
> ---
>  drivers/media/i2c/Kconfig   |   6 +
>  drivers/media/i2c/Makefile  |   1 +
>  drivers/media/i2c/max9260.c | 294 
> 
>  3 files changed, 301 insertions(+)
>  create mode 100644 drivers/media/i2c/max9260.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 7c23b7a..743f8ee 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -400,6 +400,12 @@ config VIDEO_VPX3220
> To compile this driver as a module, choose M here: the
> module will be called vpx3220.
>  
> +config VIDEO_MAX9260
> + tristate "Maxim MAX9260 GMSL deserializer support"
> + depends on I2C
> + ---help---
> +   This driver supports the Maxim MAX9260 GMSL deserializer.
> +
>  comment "Video and audio decoders"
>  
>  config VIDEO_SAA717X
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 62323ec..9b2fd13 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -86,3 +86,4 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
>  obj-$(CONFIG_VIDEO_ML86V7667)+= ml86v7667.o
>  obj-$(CONFIG_VIDEO_OV2659)   += ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> +obj-$(CONFIG_VIDEO_MAX9260)  += max9260.o
> diff --git a/drivers/media/i2c/max9260.c b/drivers/media/i2c/max9260.c
> new file mode 100644
> index 000..2030eb0
> --- /dev/null
> +++ b/drivers/media/i2c/max9260.c
> @@ -0,0 +1,294 @@
> +/*
> + * Maxim MAX9260 GMSL Deserializer Driver
> + *
> + * Copyright (C) 2017 Ulrich Hecht
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SYNC 0x79
> +#define ACK  0xc3
> +
> +#define RX_FINISHED  0
> +#define RX_FRAME_ERROR   1
> +#define RX_EXPECT_ACK2
> +#define RX_EXPECT_ACK_DATA   3
> +#define RX_EXPECT_DATA   4
> +
> +struct max9260_device {
> + struct serdev_device *serdev;
> + u8 *rx_buf;
> + int rx_len;
> + int rx_state;
> + wait_queue_head_t rx_wq;
> + struct i2c_adapter adap;
> +};
> +
> +static void wait_for_transaction(struct max9260_device *dev)
> +{
> + wait_event_interruptible_timeout(dev->rx_wq,
> + dev->rx_state <= RX_FRAME_ERROR,
> + HZ/2);
> +}
> +
> +static void transact(struct max9260_device *dev,
> +  int expect,
> +  u8 *request, int len)
> +{
> + serdev_device_mux_select(dev->serdev);

You don't check the return value here...

> +
> + serdev_device_set_baudrate(dev->serdev, 115200);
> + serdev_device_set_parity(dev->serdev, 1, 0);
> +
> + dev->rx_state = expect;
> + serdev_device_write_buf(dev->serdev, request, len);
> +
> + wait_for_transaction(dev);
> +
> + serdev_device_mux_deselect(dev->serdev);

...and unconditionally deselect the mux here. I.e. a potential
unlock of an unlocked mutex...

Cheers,
peda

> +}
> +
> +static int max9260_read_reg(struct max9260_device *dev, int reg)
> +{
> + u8 request[] = { 0x79, 0x91, reg, 1 };
> + u8 rx;
> +
> + dev->rx_len = 1;
> + dev->rx_buf = 
> +
> + transact(dev, RX_EXPECT_ACK_DATA, request, 4);
> +
> + if (dev->rx_state == RX_FINISHED)
> + return rx;
> +
> + return -1;
> +}
> +
> +static int max9260_setup(struct max9260_device *dev)
> +{
> + int ret;
> +
> + ret = max9260_read_reg(dev, 0x1e);
> +
> + if (ret != 0x02) {
> + dev_err(>serdev->dev,
> + "device does not identify as MAX9260\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void max9260_uart_write_wakeup(struct serdev_device *serdev)
> +{
> +}
> +
> +static int max9260_uart_receive_buf(struct serdev_device *serdev,
> + const u8 *data, size_t count)
> +{
> + struct max9260_device *dev = serdev_device_get_drvdata(serdev);
> + int accepted;
> +
> + switch (dev->rx_state) {
> + case RX_FINISHED:
> + dev_dbg(>serdev->dev, "excess data ignored\n");
> + return count;
> +
> + case RX_EXPECT_ACK:
> + case RX_EXPECT_ACK_DATA:
> + if (data[0] != ACK) {
> + dev_dbg(>serdev->dev, "frame error");
> + dev->rx_state = RX_FRAME_ERROR;
> + wake_up_interruptible(>rx_wq);
> +   

Re: [RFC 0/4] serdev GPIO-based multiplexing support

[Peter Rosin]

On 2017-06-14 16:38, Ulrich Hecht wrote:
> Hi!
> 
> This is an attempt to add multiplexer support to serdev, specifically
> GPIO-based multiplexing.
> 
> Our use case is the Renesas Blanche V2H board with several MAX9260 GMSL
> deserializers attached to one serial port. A sample driver that implements
> i2c passthrough over the GMSL link is part of this series. This device
> wants to be talked to with even parity, so a patch implementing parity
> control in serdev is included as well.
> 
> The board-specific part of this series depends on the "pinctrl: sh-pfc:
> r8a7792: Add SCIF1 pin groups" patch.
> 
> Please tell me if this is a suitable way to implement this functionality
> in serdev, or how to improve it. Thank you.

When I look at patch 2/4, I can't help but think that you should perhaps
consider the new mux framework available in linux-next [1]. But as the
author of that, I'm maybe biased...

You then support other means of controlling the mux automatically (i.e.
you get free support for non-gpio muxes). You also get support for
sharing the mux controller should the same gpio pins control muxes
for unrelated functions (which happened for my hw).

However, I see that you request the gpios when you select the mux, and
free them when you deselect it. That is not how the gpio mux in the
mux framework operates; it instead keeps the gpios requested and either
leaves the gpios as-is on deselect or sets a specific idle value. But
that is perhaps ok for this use-case too?

Cheers,
peda

[1] https://lkml.org/lkml/2017/5/14/160

> CU
> Uli
> 
> 
> Ulrich Hecht (4):
>   serdev: add method to set parity
>   serdev: add GPIO-based multiplexer support
>   max9260: add driver for i2c over GMSL passthrough
>   ARM: dts: blanche: add SCIF1 and MAX9260 deserializer
> 
>  arch/arm/boot/dts/r8a7792-blanche.dts |  45 ++
>  drivers/media/i2c/Kconfig |   6 +
>  drivers/media/i2c/Makefile|   1 +
>  drivers/media/i2c/max9260.c   | 294 
> ++
>  drivers/tty/serdev/Kconfig|   3 +
>  drivers/tty/serdev/Makefile   |   1 +
>  drivers/tty/serdev/core.c |  55 ++-
>  drivers/tty/serdev/mux-gpio.c |  80 +
>  drivers/tty/serdev/serdev-ttyport.c   |  17 ++
>  include/linux/serdev.h|  30 +++-
>  10 files changed, 528 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/media/i2c/max9260.c
>  create mode 100644 drivers/tty/serdev/mux-gpio.c
> 

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Peter Rosin]

On 2017-06-01 04:19, jmondi wrote:
>> But why do you need to identify them? What problem are you trying to solve?
>>
> I want to be able to walk all children devices of an i2c-adapter,
> not including the mux channel i2c-adapters. I'm sure I can work around it.
> While doing that I stumbled upon this and thought it was wrong.

Hey, I'm not saying that the current (ab?)use of dev->parent for the
i2c hierarchy is "right". But fixing things is simply just more involved
than what you proposed.

Some random thoughts on this topic:

- I'm not sure it is sane to rearrange the device hierarchy the way the
  i2c mux code does, and the i2c hierarchy is available by other means
  as parent in struct i2c_mux_core. The trouble is getting to that struct
  from outside the owner driver. Maybe make i2c_mux_alloc create a
  separate device? Then all i2c muxes could store their copy of struct
  i2c_mux_core in a way that could be easily found by generic code.
- Is it ok to add children to unsuspecting(?) devices? (I.e. the parent
  adapter of an i2c mux gets extra children.)
- I think the current scheme prevents the parent adapter from going away
  before the mux child adapter during tare-down, which is a good thing,
  but I think that can be solved with these new device links that I read
  about?
- The i2c hierarchy also needs to be visible in sysfs, which it is with
  the current scheme (but not with your patch) so some kind of new
  i2c-parent attribute is needed for i2c mux child adapters.
- Changes in this area feel subtle, and needs testing...

Cheers,
peda

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Peter Rosin]

On 2017-05-31 09:51, jmondi wrote:
> Hi Peter,
> 
> On Tue, May 30, 2017 at 01:04:08PM +0200, Peter Rosin wrote:
>> On 2017-05-30 10:54, Jacopo Mondi wrote:
>>> I2c-mux channels are created as mux siblings while they should be
>>> children of the mux itself. Fix it.
>>
>> Has this received any testing at all?
>>
> 
> Yes, but on our specific use case, that apparently does not makes use of
> i2c_parent_is_i2c_adapter()

Try e.g. adding a client device with some address to the root i2c
adapter, and then add another client device with the same address
to one of the mux child adapters. Do that with and without your
patch. I suspect it will be allowed with your patch even though it
shouldn't.

>> I think it will break various users of i2c_parent_is_i2c_adapter()
>> that expect the current situation that a i2c mux child adapter is
>> direct child of the parent i2c adapter. I.e. with no intermediate
>> device node.
> 
> Oh, I know see..
> 
> So when walking the devices sitting on an i2c-adapter we should expect to
> see mux children adapters as well there. Do you think is there a way to
> easily identify them?

Well, you can use the method from i2c_parent_is_i2c_adapter() and
write a function like so (untested):

struct i2c_adapter *i2c_is_i2c_adapter(struct device *dev)
{
if (dev->type != _adapter_type)
return NULL;

return to_i2c_adapter(dev);
}

But why do you need to identify them? What problem are you trying to solve?

Also, I forgot to mention this in my first reply, but note that the
device implementing the i2c-mux need not be a child of the i2c adapter.
I.e. in your example, the device I'm talking about is gmsl-deserializer@0.
This "MUX" device could be e.g. a platform device situated in some other
completely random place in the device tree. Oh well, perhaps not random,
but I hope you get what I mean...

Cheers,
peda

> Thanks
>j
>>
>> Cheers,
>> peda
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> Suggested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>> ---
>>>
>>> Hello,
>>>while inspecting child nodes of an i2c adapter it has been noted that
>>> child devices of an i2c-mux are listed as children of the i2c adapter 
>>> itself,
>>> and not of the i2c-mux.
>>>
>>> The hierarchy of devices looked like
>>>
>>> -- i2c-04
>>> --- eeprom@57
>>> --- video_receiver@70
>>> --- video_receiver@34
>>> --- gmsl-deserializer@0 <-- MUX
>>> --- gmsl-deserializer@0/i2c@0   <-- MUX CHANNEL
>>>
>>> It now looks like
>>>
>>> -- i2c-04
>>> --- eeprom@57
>>> --- video_receiver@70
>>> --- video_receiver@34
>>> --- gmsl-deserializer@0
>>>  gmsl-deserializer@0/i2c@0
>>>
>>> v1 -> v2:
>>> - change commit message as suggested by Geert
>>>
>>>  drivers/i2c/i2c-mux.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
>>> index 83768e8..37b7804 100644
>>> --- a/drivers/i2c/i2c-mux.c
>>> +++ b/drivers/i2c/i2c-mux.c
>>> @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>> priv->adap.owner = THIS_MODULE;
>>> priv->adap.algo = >algo;
>>> priv->adap.algo_data = priv;
>>> -   priv->adap.dev.parent = >dev;
>>> +   priv->adap.dev.parent = muxc->dev;
>>> priv->adap.retries = parent->retries;
>>> priv->adap.timeout = parent->timeout;
>>> priv->adap.quirks = parent->quirks;
>>> --
>>> 2.7.4
>>>
>>

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Peter Rosin]

On 2017-05-30 10:54, Jacopo Mondi wrote:
> I2c-mux channels are created as mux siblings while they should be
> children of the mux itself. Fix it.

Has this received any testing at all?

I think it will break various users of i2c_parent_is_i2c_adapter()
that expect the current situation that a i2c mux child adapter is
direct child of the parent i2c adapter. I.e. with no intermediate
device node.

Cheers,
peda

> Signed-off-by: Jacopo Mondi 
> Suggested-by: Laurent Pinchart 
> ---
> 
> Hello,
>while inspecting child nodes of an i2c adapter it has been noted that
> child devices of an i2c-mux are listed as children of the i2c adapter itself,
> and not of the i2c-mux.
> 
> The hierarchy of devices looked like
> 
> -- i2c-04
> --- eeprom@57
> --- video_receiver@70
> --- video_receiver@34
> --- gmsl-deserializer@0   <-- MUX
> --- gmsl-deserializer@0/i2c@0 <-- MUX CHANNEL
> 
> It now looks like
> 
> -- i2c-04
> --- eeprom@57
> --- video_receiver@70
> --- video_receiver@34
> --- gmsl-deserializer@0
>  gmsl-deserializer@0/i2c@0
> 
> v1 -> v2:
> - change commit message as suggested by Geert
> 
>  drivers/i2c/i2c-mux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 83768e8..37b7804 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>   priv->adap.owner = THIS_MODULE;
>   priv->adap.algo = >algo;
>   priv->adap.algo_data = priv;
> - priv->adap.dev.parent = >dev;
> + priv->adap.dev.parent = muxc->dev;
>   priv->adap.retries = parent->retries;
>   priv->adap.timeout = parent->timeout;
>   priv->adap.quirks = parent->quirks;
> --
> 2.7.4
> 

Re: [PATCH] i2c: mux: refer to i2c-mux.txt

[Peter Rosin]

Hi!

On 2016-06-08 08:21, Simon Horman wrote:
> Correct references to i2c-mux.txt which was previously mux.txt.
> 
> Also correct the spelling of relevant.
> 
> Signed-off-by: Simon Horman <horms+rene...@verge.net.au>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-arb-gpio-challenge.txt | 4 ++--
>  Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt  | 3 ++-
>  Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt   | 6 +++---
>  Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt| 4 ++--
>  Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt| 6 +++---
>  5 files changed, 12 insertions(+), 11 deletions(-)
> 

Acked-by: Peter Rosin <p...@axentia.se>

Thanks,
Peter

RE: [PATCH] i2c: mux: demux-pinctrl: Update docs to new sysfs-attributes

[Peter Rosin]

Wolfram Sang wrote:
> Update the docs according to the recent code changes, too.
> 
> Fixes: c0c508a418f9da ("i2c: mux: demux-pinctrl: Clean up sysfs attributes")
> Cc: Ben Hutchings 
> Signed-off-by: Wolfram Sang 

Wolfram, in case it wasn't obvious, I just wanted to clarify that I'm
perfectly happy with you handling everything about demux-pinctrl.
If that suits you?

Cheers,
Peter