Re: [PATCH] i2c: mux: demux-pinctrl: use proper email address for ABI requests
On 2019-06-20 22:01, Wolfram Sang wrote: > Use my commercial address, not my community one. > > Signed-off-by: Wolfram Sang > --- > > Peter, I'd assume this goes via your tree unless you let me know > otherwise. Thanks! > > Documentation/ABI/testing/sysfs-platform-i2c-demux-pinctrl | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-platform-i2c-demux-pinctrl > b/Documentation/ABI/testing/sysfs-platform-i2c-demux-pinctrl > index 3c3514815cd5..d6e0c3bb6bbf 100644 > --- a/Documentation/ABI/testing/sysfs-platform-i2c-demux-pinctrl > +++ b/Documentation/ABI/testing/sysfs-platform-i2c-demux-pinctrl > @@ -1,7 +1,8 @@ > What:/sys/devices/platform//available_masters > Date:January 2016 > KernelVersion: 4.6 > -Contact: Wolfram Sang > +Contact: Wolfram Sang > + Surplus newline... I removed that, and added the patch to the i2c-mux/for-next branch. Cheers, Peter > Description: > Reading the file will give you a list of masters which can be > selected for a demultiplexed bus. The format is > @@ -12,7 +13,7 @@ Description: > What:/sys/devices/platform//current_master > Date:January 2016 > KernelVersion: 4.6 > -Contact: Wolfram Sang > +Contact: Wolfram Sang > Description: > This file selects/shows the active I2C master for a > demultiplexed > bus. It uses the value from the file > 'available_masters'. >
Re: [PATCH v8 3/3] mfd: da9063: occupy second I2C address, too
On 2019-05-16 23:13, Wolfram Sang wrote: > Even though we don't use it yet, we should mark the second I2C address > this device is listening to as used. > > Not yet for upstream until all dependencies are merged! > > Signed-off-by: Wolfram Sang Reviewed-by: Peter Rosin Cheers, Peter
Re: [PATCH v8 2/3] i2c: core: add device-managed version of i2c_new_dummy
On 2019-05-16 23:13, Wolfram Sang wrote: > From: Heiner Kallweit > > i2c_new_dummy is typically called from the probe function of the > driver for the primary i2c client. It requires calls to > i2c_unregister_device in the error path of the probe function and > in the remove function. > This can be simplified by introducing a device-managed version. > > Note the changed error case return value type: i2c_new_dummy returns > NULL whilst devm_i2c_new_dummy_device returns an ERR_PTR. > > Signed-off-by: Heiner Kallweit > [wsa: rename new functions and fix minor kdoc issues] > Signed-off-by: Wolfram Sang Reviewed-by: Peter Rosin Cheers, Peter
Re: [PATCH v8 1/3] i2c: core: improve return value handling of i2c_new_device and i2c_new_dummy
On 2019-05-16 23:13, Wolfram Sang wrote: > From: Heiner Kallweit > > Currently i2c_new_device and i2c_new_dummy return just NULL in error > case although they have more error details internally. Therefore move > the functionality into new functions returning detailed errors and > add wrappers for compatibility with the current API. > > This allows to use these functions with detailed error codes within > the i2c core or for API extensions. > > Signed-off-by: Heiner Kallweit > [wsa: rename new functions and fix minor kdoc issues] > Signed-off-by: Wolfram Sang The only nit I can find is that you could perhaps sweep the patches with sed 's/i2c /I2C /'. But that is indeed a nit, so if you're in a hurry... Reviewed-by: Peter Rosin Cheers, Peter
Re: [PATCH] i2c: core: ratelimit 'transfer when suspended' errors
On 2019-04-25 10:23, Wolfram Sang wrote:
> There are two problems with WARN_ON() here. One: It is not ratelimited.
> Two: We don't see which adapter was used when trying to transfer
> something when already suspended. Implement a custom ratelimit once per
> adapter and use dev_WARN there. This fixes both issues. Drawback is that
> we don't see if multiple drivers are trying to transfer with the same
> adapter while suspended. They need to be discovered one after the other
> now. This is better than a high CPU load because a really broken driver
> might try to resend endlessly.
>
> Signed-off-by: Wolfram Sang
> ---
>
> So, Simon had a point with his review comment back then, and I think we now
> found a properly balanced way. Tested with a Renesas Lager board (R-Car H2). I
> decided against dev_WARN_ONCE because that seems too coarse grained for my
> taste (once per system) and the custom implementation is small.
>
> drivers/i2c/i2c-core-base.c | 5 -
> include/linux/i2c.h | 3 ++-
Perhaps unrelated to this patch, but I expected a similar change in
i2c-core-smbus.c:__i2c_smbus_xfer(), but it has no suspended check at
all. At first I thought that perhaps no driver that actually did
suspend had an .smbus_xfer implementation, but upon verifying that,
I found that i2c-zx2967.c does.
Am I missing something, or do we need to add an -ESHUTDOWN test
to i2c-core-smbus.c:__i2c_smbus_xfer()?
Cheers,
Peter
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 4e6300dc2c4e..f8e85983cb04 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1867,8 +1867,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>
> if (WARN_ON(!msgs || num < 1))
> return -EINVAL;
> - if (WARN_ON(test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags)))
> + if (test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags)) {
> + if (!test_and_set_bit(I2C_ALF_SUSPEND_REPORTED,
> &adap->locked_flags))
> + dev_WARN(&adap->dev, "Transfer while suspended\n");
> return -ESHUTDOWN;
> + }
>
> if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
> return -EOPNOTSUPP;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 03755d4c9229..be27062f8ed1 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -694,7 +694,8 @@ struct i2c_adapter {
> int retries;
> struct device dev; /* the adapter device */
> unsigned long locked_flags; /* owned by the I2C core */
> -#define I2C_ALF_IS_SUSPENDED 0
> +#define I2C_ALF_IS_SUSPENDED 0
> +#define I2C_ALF_SUSPEND_REPORTED 1
>
> int nr;
> char name[48];
>
Re: [PATCH v7 2/3] i2c: core: add device-managed version of i2c_new_dummy
On 2019-03-16 17:43, Wolfram Sang wrote:
>
>> I personally really dislike the proposed name. It is akin to the abomination
>> where some sort of abbreviation of the types of variables are included also
>> in the variable names. It's useless clutter, at least to me.
>
> In general, I agree with you...
>
>> I can see that you do not want to change the i2c_new_{device,dummy} names
>> since they are kind of widespread and I can also see that you want to
>> match the devm_ name with the non-devm_ name. So, I see *why* this naming
>> is as it is, but I just don't like it.
>
> ... yet, there is another reason which is consistency. i2c_new_{device,dummy}
> return NULL if something went wrong, and if the devm_* variants return
> an ERRPTR, this is super confusing and error prone IMO. And the
> difference between 'i2c_new_dummy' and 'i2c_register_dummy' doesn't make
> that more clear, I think.
>
> I would love to convert all of those functions to return an errptr at
> some time. However, they are so widespread that this is difficult. I
> actually had a look to convert the users with coccinelle, but handling
> the error cases is so diverse that I don't think this is a way forward.
Ok, it's your call obviously, and doing a rename away from the _ptrerr
suffix when the old name is no longer in use is easier than changing
the return value convention. You just have to wait a couple of releases
so that stragglers that are slow to upstream don't get caught by the
changed semantics when it eventually happens. However, my point is that
these conversions tend to drag out, and in the meantime we are stuck
with whatever names we chose.
Perhaps add a rule to checkpatch to avoid new instances of the now
inferior i2c_new_{device,dummy}?
Anyway, what happens for the callers that ignore the return value of
these functions? Is that always a leak or are there cases when it is ok?
__must_check?? (not applicable for devm_... of course)
Cheers,
Peter
Re: [PATCH v7 2/3] i2c: core: add device-managed version of i2c_new_dummy
On 2019-03-15 22:08, Wolfram Sang wrote: > >>> I personally really dislike the proposed name. It is akin to the abomination >>> where some sort of abbreviation of the types of variables are included also >>> in the variable names. It's useless clutter, at least to me. >> >> >> I hate to be jumping in with just a 'me-too' - but I also had a similar >> disliking to the _errptr suffix on the function name here. > > As I said to Peter, I am not exactly happy with the naming. I just > couldn't come up with something better. I am totally open for > suggestions here. Let's give ourselves a few days. Maybe inspiration > will hit one of us somehow somewhen. I can't seem to find what you said to me anywhere, it's not in my inbox and I don't see any (other) reply from you on patchwork for this patch either. Perhaps a resend is in order? Cheers, Peter
Re: [PATCH v7 2/3] i2c: core: add device-managed version of i2c_new_dummy
Hi!
A comment from the peanut gallery...
On 2019-03-13 17:55, Wolfram Sang wrote:
> From: Heiner Kallweit
>
> i2c_new_dummy is typically called from the probe function of the
> driver for the primary i2c client. It requires calls to
> i2c_unregister_device in the error path of the probe function and
> in the remove function.
> This can be simplified by introducing a device-managed version.
>
> Note the changed error case return value type:
> i2c_new_dummy returns NULL whilst devm_new_i2c_dummy returns an ERR_PTR.
>
> Signed-off-by: Heiner Kallweit
> [wsa: rename new functions to 'errptr' style and fix minor kdoc issues]
> Signed-off-by: Wolfram Sang
> ---
> Documentation/driver-model/devres.txt | 3 +++
> drivers/i2c/i2c-core-base.c | 44
> +++
> include/linux/i2c.h | 3 +++
> 3 files changed, 50 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt
> b/Documentation/driver-model/devres.txt
> index b277cafce71e..e36dc8041857 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -266,6 +266,9 @@ GPIO
>devm_gpio_request_one()
>devm_gpio_free()
>
> +I2C
> + devm_i2c_new_dummy_errptr()
> +
TL;DR Can we call the new functions
i2c_register_device
i2c_register_dummy
devm_i2c_register_dummy
or
i2c_add_device
i2c_add_dummy
devm_i2c_add_dummy
or something? Please?
I personally really dislike the proposed name. It is akin to the abomination
where some sort of abbreviation of the types of variables are included also
in the variable names. It's useless clutter, at least to me.
I can see that you do not want to change the i2c_new_{device,dummy} names
since they are kind of widespread and I can also see that you want to
match the devm_ name with the non-devm_ name. So, I see *why* this naming
is as it is, but I just don't like it.
I had a look at a couple of the i2c_new_dummy call sites, and some of them
can be easily updated to simply pass on the PTRERR instead of hardcoding
INVAL (or something) on NULL, some of them ignore the return value (and are
thus not able to call i2c_unregister_device correctly) and some are
different in other ways. In short, it seems that there are plenty of good
opportunities to update lots of call sites to the new interfaces.
However, after most of the maintained uses have been updated we are bound
to have a tail of unmaintained code that will use the old interface. At
some point, someone might convert the tail of call sites using the old
NULL-returning functions. At that point we are stuck with a bunch of ugly
hysterical foo_errptr names. And again it will be a daunting series to
change them all at once.
Cheers,
Peter
Re: [RFC PATCH v2 0/7] i2c: core: introduce atomic transfers
On 2019-03-04 23:48, Wolfram Sang wrote: > Hi Peda, > >> The way I read this series, you are not giving atomic transfers priority. The > > You are reading correctly. I could have made more clear that the issue > pointed out by Russell is not handled by this series but discussion > about it is welcome / needed to decide if we can take this series as is > or if we need to redesign it. But here we are anyhow :) > >> only thing that happens is that if an xfer happens in atomic/irq context, >> trylock is used instead of an ordinary (unconditional) lock (this is just >> like it is already). If a mux is sitting in between the client device and >> the root adapter, the trylock operation will percolate to the root. Sure, >> there are more trylock ops that may fail and abort the xfer, but if >> everything is uncontended, then things should proceed in orderly fashion. >> Also, sure, the mux may need additional resources that are no longer >> available if the machine is half way down (or worse). But I don't see any >> fundamental *locking* issue with muxes that is different from the case >> without a mux. > > Good, that was my conclusion as well. The series, as is, doesn't change > the locking behaviour, so that will work exactly as before. Or, it will > not work in the case described by Russell. Like before. > >> That said, if you then want to introduce xfers that want to circumvent the >> locking, then parent-locked muxes are easier since the actual muxing >> operation >> is performed as an unlocked xfer (if one is needed) while the client device >> has grabbed the adapter lock "from the outside". Sure, there is a list of >> locks going up through the adapter tree to handle, but that can probably be >> handled in one place. I.e. the locking must have been avoided prior to the >> actual muxing operation, but the code to do so can be in one place. The > > That was my gut feeling, too... > >> mux-locked case is where the trouble is, since the muxing operation is done >> as a normal xfer and needs to be classified as a special xfer that just like >> the original client xfer also needs to break through any existing locks in >> the adapter tree. And those muxing xfers might come from anywhere, e.g. >> >> - IO-expander controlling a gpio/pinctrl mux >> - dedicated I2C mux (e.g. the LTC4306) >> - regmap device >> - etc, who knows what muxing options will evolve? >> >> So, any scheme that require a white-list will work poorly for mux-locked >> muxes, unless you can add some new grip/pinctrl/regmap flags to s/grip/gpio/ of course >> gpios/pins/registers so that the particular accesses can be white-listed. >> Adding those flags seem rather invasive? > > ... and sadly, this too. We would need the same kind of flag which I > described in my first paragraph of the original posting where I wanted > the flag to detect "unauthorized" uses of late I2C transfers. And this > is gonna be invasive. And I am not sure it is worth the effort. > > I wonder what a reasonable effort is? Simply ignore the lock from the > "current" adapter and hope for the best that there is no mux or at > least no mux which needs interrupts / a lock attached to it? Just wanted to add a note that the underlying problem is similar to why I introduced the mux-locked concept. There is no simple way to identify *exactly* which xfers that need to be unlocked. Going only by call site is not enough, since the same call in different context may need to be muxed (in my case) or irq-less (in this case). If someone comes up with a solution for that, all muxes can be converted to the parent-locked scheme and we can get rid of a bunch of complexity. I just don't see how though, all ideas I have come up with I have immediately discarded as way too invasive, ugly and/or error prone. >> But of course, you need to actually do something about the added FIXME in >> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer >> just like it does for ->master_xfer, no? > > Yes. The idea of having two seperate SMBus controllers in one SoC which > would need demuxing is amusing, but still, it exists for I2C and you are > right. Right, I didn't actually think all that far... :-) Cheers, Peter
Re: [RFC PATCH v2 0/7] i2c: core: introduce atomic transfers
On 2019-03-02 14:47, Wolfram Sang wrote: > So, finally, here is the second RFC for supporting I2C transfers in atomic > contexts (i.e. very late). This will need some text because I tried some > things > on the way but had to discard them. However, I think it is important to have > that documented. > > One thing I really wanted to have is a kind of whitelist for devices which are > allowed to use atomic transfers. So we could identify the "unauthorized" ones > as buggy. To be useful, this should not add new API calls for transfers, > otherwise things would have become way more complicated for I2C users like > regmap. So, I tried e.g. to flag clients and provide that information > throughout the i2c tree (think muxes here). In the end, I concluded that this > is not an I2C specific problem, so it can't have an I2C specifc solution. > Imagine a GPIO which is needed to reboot (drivers/power/reset/gpio-restart.c). > This is the device which needs to whitelisted but the driver doesn't even know > if the GPIO is behind I2C or not. So, if we want this, it should probably be > handled on 'struct device' level. Including all the hierarchy. Postponed. > > So, this RFC v2 is much more similar to v1 than I expected. Main changes: > > * cleaned up 'struct i2c_adapter' a bit before adding the new stuff > * added an atomic callback for SMBus, too. Only build-tested so far. But spent > a few braincells of getting the SMBus logic readable because we could have > an I2C fallback just for the atomic case > * add a WARN for atomic transfers with no atomic transfer handler > * added support for the i2c-demuxer, so I could test the series. Support > for I2C muxes is missing because of the locking issue (see later) which > may mean a redesign anyhow > * imported the omap support into this series to have another user. I didn't > pick up the patch for imx from Stefan because it is bigger and probably > needs seperate review first > * I converted the tegra-bpmp driver which already had handling for the atomic > case*. I did not convert the pxa driver which has a polling-only mode, too. > This also seems like a bigger task and its current behaviour shouldn't be > affected by this series. *only build tested > * added a HACK to allow the i2c-gpio driver atomic transfers. This will only > work if accessing the GPIO can be done in atomic contexts, too, so this is > for testing only > > For the regular cases this series works well on my Renesas Lager board* > which needs an I2C access to the PMIC to reboot the board. *if I use the > i2c-gpio driver, the i2c-sh_mobile is not converted yet. > > However, during the last review, Russell King brought up an interesting corner > case. What if we want to reboot because of a panic and the bus is not in a > consistent state? To create this situation, I recently created the > 'inject-panic' > fault injector [1] which is merged into i2c/for-next meanwhile. > > With this fault injector and 'reboot after panic' settings, I can create > the problem Russell described: a) the bus is in an inconsistent state because > the driver was interrupted (SCL/SDA both low) and b) the lock for this driver > is taken, so trylock fails. > > I think b) is an interesting question: shall we give atomic transfers priority > and ignore the lock? Do we need a seperate one then (SMP is turned off > already, > or?)? If so, that would probably mean way more complicated mux-locking code > (Peter?)? And what if some mux in the path needs interrupts? And how academic > is all that? Because someone putting the reboot functionality behind muxed I2C > is kind of asking for problems :) > > That being said: this is an issue I think it is worth tackling. However, this > issue is not introduced by this series. It is already there. It might just > become more visible. The way I read this series, you are not giving atomic transfers priority. The only thing that happens is that if an xfer happens in atomic/irq context, trylock is used instead of an ordinary (unconditional) lock (this is just like it is already). If a mux is sitting in between the client device and the root adapter, the trylock operation will percolate to the root. Sure, there are more trylock ops that may fail and abort the xfer, but if everything is uncontended, then things should proceed in orderly fashion. Also, sure, the mux may need additional resources that are no longer available if the machine is half way down (or worse). But I don't see any fundamental *locking* issue with muxes that is different from the case without a mux. That said, if you then want to introduce xfers that want to circumvent the locking, then parent-locked muxes are easier since the actual muxing operation is performed as an unlocked xfer (if one is needed) while the client device has grabbed the adapter lock "from the outside". Sure, there is a list of locks going up through the adapter tree to handle, but that can probably be handled in one place. I.e. the l
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
On 2019-02-19 14:13, Wolfram Sang wrote: > > + ret = gpiod_direction_output(priv->scl, 1); This may overwrite the error code returned by request_irq(). >>> >>> Yeah. What do you think about this, is this too dense? >>> >>> ret = gpiod_direction_output(priv->scl, 1) ?: ret; >> >> That may also overwrite the error code, of course. Isn't it >> usually best to return the first error? I have no clue if that >> guideline does not apply here, though... > > I am neither entirely sure here. My take was that the above was the more > severe error. Because if setting to output fails, the GPIO I2C bus will > be broken. If the former stuff fails, well, the injection didn't work or > was interrupted. > > However, the GPIO was set to output before the injector. So, if setting > it back fails, then the system likely has more severe problems anyhow? One way to end up with that is if there is an irq attached to the gpio pin. If you disable the irq, you are (sometimes) allowed to change the gpio to output, but not if the irq is active. So, if some other part of the driver "steals" the gpio by activating an irq while the injector is doing its thing, it is possible to end up with this. But that seems like a gigantic corner case. > I am open to any better solution. However, let's not forget, this is > debug code aimed to be used by devs. > You are right, please ignore me. Cheers, Peter
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
On 2019-02-18 21:41, Wolfram Sang wrote: > Hi Geert, >>> + >>> + free_irq(irq, priv); >>> + output: >>> + ret = gpiod_direction_output(priv->scl, 1); >> >> This may overwrite the error code returned by request_irq(). > > Yeah. What do you think about this, is this too dense? > > ret = gpiod_direction_output(priv->scl, 1) ?: ret; That may also overwrite the error code, of course. Isn't it usually best to return the first error? I have no clue if that guideline does not apply here, though... Cheers, Peter
Re: [PATCH 01/10] i2c: add suspended flag and accessors for i2c adapters
On 2018-12-21 15:50, Wolfram Sang wrote: > I think this might be as simple as adding: if (WARN_ON(adap->dev.parent->power.is_suspended)) return -ESHUTDOWN; > > Peter, I think this should work for muxes, too, or? The i2c_transfer() > call to the mux will not be rejected, but it will be later when we reach > the root adapter. And then the error code will be pushed down the tree > until we arrive at the mux again. So, the rejection will not happen at > the earliest time, but it will happen. Is my understanding correct? Yes, I agree with that analysis. All mux actions eventually end up with an __i2c_transfer() call on the relevant root adapter. Hmm, but not *all* calls. How about SMBus adapters? Should there not be a similar WARN_ON in __i2c_smbus_xfer? But maybe that's not applicable for some reason? Just asking... Cheers, Peter
Re: [PATCH] dt-bindings: i2c: sh_mobile: Add r8a774c0 support
On 2018-12-13 21:19, Fabrizio Castro wrote: > Document RZ/G2E (R8A774C0) SoC bindings. > > Signed-off-by: Fabrizio Castro > --- > Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt > b/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt > index bc876b7..3dff89f 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt > @@ -8,6 +8,7 @@ Required properties: > - "renesas,iic-r8a7744" (RZ/G1N) > - "renesas,iic-r8a7745" (RZ/G1E) > - "renesas,iic-r8a774a1" (RZ/G2M) > + - "renesas,iic-r8a774c0" (RZ/G2E) > - "renesas,iic-r8a7790" (R-Car H2) > - "renesas,iic-r8a7791" (R-Car M2-W) > - "renesas,iic-r8a7792" (R-Car V2H) > @@ -32,7 +33,7 @@ Required properties: > When compatible with "renesas,rmobile-iic" it should > be the last compatibility string listed. > > - The r8a77990 (R-Car E3) controller is not > + The r8a77990 (R-Car E3) and RZ/G2E controllers are not The references to these two controllers do not follow the same style. I'd either delete r8a77990 and get "R-Car E3 and RZ/G2E" or add r8a774c0 in front of RZ/G2E (bracketed), with a slight preference for the latter. Cheers, Peter > considered compatible with "renesas,rcar-gen3-iic" > or "renesas,rmobile-iic" due to the absence of > automatic transmission registers. >
Re: [RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters
On 2018-12-10 22:02, Wolfram Sang wrote:
> Some drivers open code handling of suspended adapters. It should be
> handled by the core, though, to ensure generic handling. This patch adds
> the flag and an accessor function.
>
> Signed-off-by: Wolfram Sang
> ---
> drivers/i2c/i2c-core-base.c | 1 +
> include/linux/i2c.h | 9 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 28460f6a60cc..9f89e258c8ff 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1232,6 +1232,7 @@ static int i2c_register_adapter(struct i2c_adapter
> *adap)
> if (!adap->lock_ops)
> adap->lock_ops = &i2c_adapter_lock_ops;
>
> + adap->is_suspended = false;
> rt_mutex_init(&adap->bus_lock);
> rt_mutex_init(&adap->mux_lock);
> mutex_init(&adap->userspace_clients_lock);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..9852038ee3a7 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -680,6 +680,7 @@ struct i2c_adapter {
> int timeout;/* in jiffies */
> int retries;
> struct device dev; /* the adapter device */
> + unsigned int is_suspended:1;/* owned by the I2C core */
When more stuff is added to this bit field (which always happens at
some point) updates to all members of the bit field will have to use
the same root-adapter-locking, or we will suffer from RMW-races. So
this feels like an invitation for future disaster. Maybe a comment
about that to remind our future selves? Or perhaps the bit field
should be avoided altogether?
Cheers,
Peter
>
> int nr;
> char name[48];
> @@ -762,6 +763,14 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int
> flags)
> adapter->lock_ops->unlock_bus(adapter, flags);
> }
>
> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap,
> + bool suspended)
> +{
> + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> + adap->is_suspended = suspended;
> + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +}
> +
> /*flags for the client struct: */
> #define I2C_CLIENT_PEC 0x04/* Use Packet Error Checking */
> #define I2C_CLIENT_TEN 0x10/* we have a ten bit chip
> address */
>
Re: [RFC/RFT 10/10] i2c: rcar: add suspend/resume support
On 2018-12-10 22:03, Wolfram Sang wrote:
> Because the adapter will be set up before every transaction anyhow, we
> just need to mark it as adapted to the I2C core.
mark it as "suspended", not "adapted"?
Cheers,
Peter
>
> Signed-off-by: Hiromitsu Yamasaki
> Signed-off-by: Wolfram Sang
> ---
> drivers/i2c/busses/i2c-rcar.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 254e6219e538..30cff066e0da 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -1017,10 +1017,35 @@ static int rcar_i2c_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int rcar_i2c_suspend(struct device *dev)
> +{
> + struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
> +
> + i2c_mark_adapter_suspended(&priv->adap, true);
> + return 0;
> +}
> +
> +static int rcar_i2c_resume(struct device *dev)
> +{
> + struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
> +
> + i2c_mark_adapter_suspended(&priv->adap, false);
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rcar_i2c_pm_ops, rcar_i2c_suspend, rcar_i2c_resume);
> +
> +#define DEV_PM_OPS (&rcar_i2c_pm_ops)
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP */
> +
> static struct platform_driver rcar_i2c_driver = {
> .driver = {
> .name = "i2c-rcar",
> .of_match_table = rcar_i2c_dt_ids,
> + .pm = DEV_PM_OPS,
> },
> .probe = rcar_i2c_probe,
> .remove = rcar_i2c_remove,
>
Re: [PATCH v3] drm/bridge/sii902x: Add missing dependency on I2C_MUX
On 2018-11-19 14:26, Fabrizio Castro wrote:
> kbuild test robot reports:
>
>>> ERROR: "i2c_mux_add_adapter" [drivers/gpu/drm/bridge/sii902x.ko]
> undefined!
>>> ERROR: "i2c_mux_alloc" [drivers/gpu/drm/bridge/sii902x.ko]
> undefined!
>>> ERROR: "i2c_mux_del_adapters" [drivers/gpu/drm/bridge/sii902x.ko]
> undefined!
Hi!
My preference would be to not wrap the quoted text. To hell with
warnings about long lines for cases like this. However, I'm not
going to require a new iteration for that detail, but keep that
in mind for the next patch or if you end up resending for some
unrelated reason.
>
> Quite obviously the driver depends on I2C_MUX, but adding a "depends on"
> introduces a recursive dependency, therefore this patch selects I2C_MUX
> instead.
This driver will not be the first to select I2C_MUX. So, even if it
it is a bit untidy to select stuff that is also selectable by the
user...
Acked-by: Peter Rosin
>
> Fixes: 21d808405fe4 ("drm/bridge/sii902x: Fix EDID readback")
> Signed-off-by: Fabrizio Castro
> Link: https://lists.01.org/pipermail/kbuild-all/2018-November/054924.html
> ---
> v2->v3:
> * Changed the title
>
> v1->v2:
> * Added "Fixes" tag
>
> drivers/gpu/drm/bridge/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 9eeb8ef..2fee47b 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -95,6 +95,7 @@ config DRM_SII902X
> depends on OF
> select DRM_KMS_HELPER
> select REGMAP_I2C
> + select I2C_MUX
> ---help---
> Silicon Image sii902x bridge chip driver.
>
>
Re: [PATCH v3] drm/bridge/sii902x: Fix EDID readback
On 2018-11-06 12:52, Fabrizio Castro wrote: > While adding SiI9022A support to the iwg23s board, it came > up that when the HDMI transmitter is in pass through mode the > device is not compliant with the I2C specification anymore, > as it requires a far bigger tbuf, due to a delay the HDMI > transmitter is adding when relaying the STOP condition on the > monitor i2c side of things. > > When not providing an appropriate delay after the STOP condition > the i2c bus would get stuck. Also, any other traffic on the bus > while talking to the monitor may cause the transaction to fail > or even cause issues with the i2c bus as well. > > I2c-gates seemed to reach consent as a possible way to address > these issues, and as such this patch is implementing a solution > based on that. Since others are clearly relying on the current > implementation of the driver, this patch won't require any DT > changes. > > Since we don't want any interference during the DDC Bus > Request/Grant procedure and while talking to the monitor, we > have to use the adapter locking primitives rather than the > i2c-mux locking primitives. > > Signed-off-by: Fabrizio Castro Reviewed-by: Peter Rosin Cheers, Peter > > --- > v2->v3: > * Incorporated comments from Boris Brezillon and Peter Rosin > > v1->v2: > * Incorporated comments from Peter Rosin > > drivers/gpu/drm/bridge/sii902x.c | 247 > --- > 1 file changed, 178 insertions(+), 69 deletions(-)
Re: [PATCH v3] drm/bridge/sii902x: Fix EDID readback
On 2018-11-06 12:52, Fabrizio Castro wrote: > While adding SiI9022A support to the iwg23s board, it came > up that when the HDMI transmitter is in pass through mode the > device is not compliant with the I2C specification anymore, > as it requires a far bigger tbuf, due to a delay the HDMI > transmitter is adding when relaying the STOP condition on the > monitor i2c side of things. > > When not providing an appropriate delay after the STOP condition > the i2c bus would get stuck. Also, any other traffic on the bus > while talking to the monitor may cause the transaction to fail > or even cause issues with the i2c bus as well. > > I2c-gates seemed to reach consent as a possible way to address > these issues, and as such this patch is implementing a solution > based on that. Since others are clearly relying on the current > implementation of the driver, this patch won't require any DT > changes. > > Since we don't want any interference during the DDC Bus > Request/Grant procedure and while talking to the monitor, we > have to use the adapter locking primitives rather than the > i2c-mux locking primitives. > > Signed-off-by: Fabrizio Castro Reviewed-by: Peter Rosin
Re: [PATCH v2] drm/bridge/sii902x: Fix EDID readback
Hi!
A handful of nitpicks inline...
On 2018-11-05 14:56, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
>
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
>
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
>
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
>
> Signed-off-by: Fabrizio Castro
>
> ---
> v1->v2:
> * Incorporated comments from Peter Rosin
>
> drivers/gpu/drm/bridge/sii902x.c | 256
> ---
> 1 file changed, 188 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..dd4318b 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
> * Copyright (C) 2016 Atmel
> * Bo Shen
> *
> @@ -21,6 +23,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -86,8 +89,49 @@ struct sii902x {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct gpio_desc *reset_gpio;
> + struct i2c_mux_core *i2cmux;
> };
>
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> + union i2c_smbus_data data;
> + int ret;
> +
> + ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> +I2C_SMBUS_READ, reg, I2C_SMBUS_BYTE_DATA, &data);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = data.byte;
> + return 0;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> + union i2c_smbus_data data;
> +
> + data.byte = val;
> +
> + return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> + I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE_DATA,
> + &data);
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8
> mask,
> + u8 val)
> +{
> + int ret;
> + u8 status;
> +
> + ret = sii902x_read_unlocked(i2c, reg, &status);
> + if (ret)
> + return ret;
> + status &= ~mask;
> + status |= val & mask;
> + return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> {
> return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +179,11 @@ static const struct drm_connector_funcs
> sii902x_connector_funcs = {
> static int sii902x_get_modes(struct drm_connector *connector)
> {
> struct sii902x *sii902x = connector_to_sii902x(connector);
> - struct regmap *regmap = sii902x->regmap;
> u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> - struct device *dev = &sii902x->i2c->dev;
> - unsigned long timeout;
> - unsigned int retries;
> - unsigned int status;
> struct edid *edid;
> - int num = 0;
> - int ret;
> -
> - ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> - SII902X_SYS_CTRL_DDC_BUS_REQ,
> - SII902X_SYS_CTRL_DDC_BUS_REQ);
> - if (ret)
> - return ret;
> -
> - timeout = jiffies +
> - msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> - do {
> - ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> - if (ret)
> - return ret;
> - } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> - time_before(jiffies, timeout));
> -
> - if (!(status & S
Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
>
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
>
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
>
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
>
> Signed-off-by: Fabrizio Castro
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
>
> Thank you guys
>
> drivers/gpu/drm/bridge/sii902x.c | 264
> +--
> 1 file changed, 196 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
> * Copyright (C) 2016 Atmel
> * Bo Shen
> *
> @@ -21,6 +23,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -86,8 +89,68 @@ struct sii902x {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct gpio_desc *reset_gpio;
> + struct i2c_mux_core *i2cmux;
> };
>
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> + struct i2c_msg xfer[] = {
> + {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = ®,
> + }, {
> + .addr = i2c->addr,
> + .flags = I2C_M_RD,
> + .len = 1,
> + .buf = val,
> + }
> + };
> + unsigned char xfers = ARRAY_SIZE(xfer);
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> + if (ret < 0)
> + return ret;
> + } while (ret != xfers && --retries);
> + return ret == xfers ? 0 : -1;
New in 4.19 __i2c_smbus_xfer, and I think it helps here? (untested code)
union i2c_smbus_data data;
int ret;
ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
I2C_SMBUS_READ, reg,
I2C_SMBUS_BYTE_DATA, &data);
if (ret < 0)
return ret;
*val = data.byte;
return 0;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> + u8 data[2] = {reg, val};
> + struct i2c_msg xfer = {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = sizeof(data),
> + .buf = data,
> + };
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> + if (ret < 0)
> + return ret;
> + } while (!ret && --retries);
> + return !ret ? -1 : 0;
union i2c_smbus_data data;
data.byte = val;
return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
I2C_SMBUS_WRITE, reg,
I2C_SMBUS_BYTE_DATA, &data);
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8
> mask,
> + u8 val)
> +{
> + int ret;
> + u8 status;
> +
> + ret = sii902x_read_unlocked(i2c, reg, &status);
> + if (ret)
> + return ret;
> + status &= ~mask;
> + status |= val & mask;
> + return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>
Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
On 2018-11-02 17:16, Fabrizio Castro wrote: >>> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the >>> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is >>> also doing rmw accesses to that register in other parts of the driver. I >>> think you need to either add comment as to why that is safe (maybe other >>> things make it impossible for the two rmw accesses to cross?), or add the >>> missing coordination. >>> >> >> The other two places where SII902X_SYS_CTRL_DATA is being handled are >> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is >> any chance of the modes being probed while the bridge gets enabled/disabled, >> but now that you make me think about it user space may trigger the readback >> of the EDID at the most inconvenient times. Anyway, this is a good point, and >> since I don't think I am supposed to access regmap's lock from outside the >> APIs, >> I could switch to the unlocked version of update_bits from within >> sii902x_bridge_disable >> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do >> you think? >> > > The bridge enable/disable callbacks deal with different bits of register > SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA after > sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select > gets triggered > so basically even if we managed to get in the way of the regmap rmw (in the > sense that > for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call > into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the > write operation the value of register SII902X_SYS_CTRL_DATA is the same as > the one > read by regmap, as "select xfer deselect" won't alter the final value of > SII902X_SYS_CTRL_DATA > and nobody can get in between. > What do you think I should do? Do you think I can leave this alone and maybe > add some > comments or do you think I should explicitly protect access to > SII902X_SYS_CTRL_DATA? If the things you write about the bits are true (haven't looked closely), I wouldn't add any regmap coordination. But if I was the maintainer, I'd like a comment about this so that the next contributor has a better chance to understand the subtlety. But I'm not the maintainer, so this is not my call, but by adding the comment you also clarify the situation for the maintainer, which is something that is likely to be appreciated. Cheers, Peter
Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
>
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
>
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
>
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
>
> Signed-off-by: Fabrizio Castro
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
>
> Thank you guys
>
> drivers/gpu/drm/bridge/sii902x.c | 264
> +--
> 1 file changed, 196 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
> * Copyright (C) 2016 Atmel
> * Bo Shen
> *
> @@ -21,6 +23,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -86,8 +89,68 @@ struct sii902x {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct gpio_desc *reset_gpio;
> + struct i2c_mux_core *i2cmux;
> };
>
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> + struct i2c_msg xfer[] = {
> + {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = ®,
> + }, {
> + .addr = i2c->addr,
> + .flags = I2C_M_RD,
> + .len = 1,
> + .buf = val,
> + }
> + };
> + unsigned char xfers = ARRAY_SIZE(xfer);
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> + if (ret < 0)
> + return ret;
> + } while (ret != xfers && --retries);
> + return ret == xfers ? 0 : -1;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> + u8 data[2] = {reg, val};
> + struct i2c_msg xfer = {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = sizeof(data),
> + .buf = data,
> + };
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> + if (ret < 0)
> + return ret;
> + } while (!ret && --retries);
> + return !ret ? -1 : 0;
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8
> mask,
> + u8 val)
> +{
> + int ret;
> + u8 status;
> +
> + ret = sii902x_read_unlocked(i2c, reg, &status);
> + if (ret)
> + return ret;
> + status &= ~mask;
> + status |= val & mask;
> + return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> {
> return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +198,11 @@ static const struct drm_connector_funcs
> sii902x_connector_funcs = {
> static int sii902x_get_modes(struct drm_connector *connector)
> {
> struct sii902x *sii902x = connector_to_sii902x(connector);
> - struct regmap *regmap = sii902x->regmap;
> u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> - struct device *dev = &sii902x->i2c->dev;
> - unsigned long timeout;
> - unsigned int retries;
> - unsigned int status;
> struct edid *edid;
> - int num = 0;
> - int ret;
> -
> - ret = regma
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-11-01 18:32, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > snip > >>> >>> To further detail the problem, the system is vulnerable from before the >>> last write >>> performed by sii902x_i2c_bypass_select to after we confirm we need the >>> switch >>> to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount >>> of time >>> we could keep the parent adapter locked for, I guess I am stuck with a >>> parent-locking >>> architecture, aren't I? >> >> If that problem description is correct, then yes, I think the *only* solution >> is to combine the three parts "open bypass mode", "edid xfer" and "close >> bypass >> mode", and to keep the i2c adapter locked during the procedure so that other >> xfers do not creep in and crap thing up from the side. And one way to combine >> the three parts is to use a parent-locked i2c gate. And since you need to >> keep >> the i2c adapter locked over the whole procedure, you need to use unlocked >> xfers >> (as you have already discovered). But how do you know that this problem >> description is accurate? > > I basically observed what was going on on the bus (with a logic analyser) > while generating > traffic on the parent adapter > >> Why is it not ok for unrelated xfers to creep in >> between opening the bypass mode and the edid xfer, and how do you know that >> this is not ok? > > Because those transfers would come with no extra delay between STOP and START > conditions while the HDMI transmitter is in passthrough mode Yeah yeah, now I get it. It's not the edid eeprom that's bad, it's the passthrough mode in the hdmi transmitter that's broken. Your problem description follows naturally. >> >>> But I guess I could release it when it's not actually needed, >> >> How would you figure out when it's not needed? > > The moment you tell the HDMI transmitter you are going to talk to the monitor > nothing else can > flow on the bus, up until you gracefully close the pass through session, > which means I wouldn't > really need to hold the parent lock during the entire duration of the select > callback, I would need > to hold the parent lock only from before the last write as that's when we > tell the HDMI transmitter > to activate the passthrough mode, but I guess it would make the driver hard > to maintain (as in > others would need to understand this properly before making any changes), > wouldn't it? Yes, that would just complicate things and would probably not have all that much benefit. I don't think I'd go there... >> >>> or is this going to be a pain to maintain? Shall I just keep going with the >>> parent-locking >>> but using bare i2c transactions only within select and deselect and leave >>> regmap >>> to deal with everything else? >> >> That's a possibility. Take care to not mess up any cached state in regmap >> though. > > The original version of the driver wasn't using any caching, so I guess I > would need to fallback > exactly to the same implementation. > > So, what should I do? Should I keep the parent-locking, the unlocked flavours > of rd/wr/rmw from > within select/deselect, and put back regmap based rd/wr/rmw with no caching > for everything else? I think that sounds like a reasonable compromise, but be careful since you still might need the two implementations to interact, e.g. the two rmw variants might still need a common lock so that they don't trample on each others toes. At least if there are accesses to the same register (SII902X_SYS_CTRL_DATA in this case if I read it right). Cheers, Peter
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-11-01 17:04, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > >>> The "mux-locked" implementation was the one I first tried and I discovered >>> it doesn't work for me, as other traffic on the parent adapter can get in >>> the >>> way. What we need for this device is no other traffic on the bus during the >>> "select transaction deselect" procedure, that's why I went with the parent >>> locking. Also this device needs a delay between stop and start conditions >>> while addressing the monitor. >> >> Ok, I thought the problem was that a delay was needed between the STOP >> of the command opening the gate and the START of the edid eeprom xfer, and >> that everything else worked normally. Too bad this wasn't the actual problem. >> >> Hmm. If it is indeed true that other xfers must never creep into the "select >> xfer deselect" procedure then you are indeed stuck with a parent-locking. >> But is that really the case? Could it be that the extra delay between >> STOP-START is only needed after the absolutely last xfer before the >> command closing the gate? >> >> If that problem description is correct, it should be possible to go back to >> a mux-locked gate, but then in your deselect implementation grab the i2c >> adapter >> lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before >> the >> 30 us delay, then open code the command to close the gate with an unlocked >> i2c >> access, and finally release the i2c bus lock. That way you have ensured >> silence >> on the bus for the required time before closing the gate. You would still >> need >> to bypass regmap, but only in this one place (but maybe you should bypass >> regmap for opening the gate too, for symmetry). > > To further detail the problem, the system is vulnerable from before the last > write > performed by sii902x_i2c_bypass_select to after we confirm we need the switch > to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of > time > we could keep the parent adapter locked for, I guess I am stuck with a > parent-locking > architecture, aren't I? If that problem description is correct, then yes, I think the *only* solution is to combine the three parts "open bypass mode", "edid xfer" and "close bypass mode", and to keep the i2c adapter locked during the procedure so that other xfers do not creep in and crap thing up from the side. And one way to combine the three parts is to use a parent-locked i2c gate. And since you need to keep the i2c adapter locked over the whole procedure, you need to use unlocked xfers (as you have already discovered). But how do you know that this problem description is accurate? Why is it not ok for unrelated xfers to creep in between opening the bypass mode and the edid xfer, and how do you know that this is not ok? > But I guess I could release it when it's not actually needed, How would you figure out when it's not needed? > or is this going to be a pain to maintain? Shall I just keep going with the > parent-locking > but using bare i2c transactions only within select and deselect and leave > regmap > to deal with everything else? That's a possibility. Take care to not mess up any cached state in regmap though. The registers you touch from select/deselect should probably be volatile or something like that? Cheers, Peter
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-11-01 12:04, Fabrizio Castro wrote:
> Hello Peter,
>
> Thank you for your feedback!
>
> snip
>
>>> Yeah, there is some issue with locking here, and that's coming down
>>> from the fact that when we call into drm_get_edid, we implicitly call
>>> i2c_transfer which ultimately locks the i2c adapter, and then calls
>>> into our sii902x_i2c_bypass_select, which in turn calls into the regmap
>>> functions and therefore we try and lock the same i2c adapter again,
>>> driving the system into a deadlock.
>>> Having the option of using "unlocked" flavours of reads and writes
>>> is what we need here, but looking at drivers/base/regmap/regmap-i2c.c
>>> I couldn't find anything suitable for my case, maybe Mark could advise
>>> on this one? I am sure I overlooked something here, is there a better
>>> way to address this?
>>
>> This recursive locking problem surfaces when an i2c mux is operated
>> by writing commands on the same i2c bus that is muxed. When this
>> happens for a typical i2c mux chip like nxp,pca9548 this can be kept
>> local to that driver. However, when there are interactions with other
>> parts of the kernel (e.g. if the i2c-mux is operated by gpio pins,
>> and those gpio pins in turn are operated by accesses to the i2c bus
>> that is muxed) this locked vs. unlocked thing would spread like
>> wildfire.
>>
>> Since I did not like that wildfire, I came up with the "mux-locked"
>> scheme. It is not appropriate everywhere, but in this case I think it
>> is a perfect fit. By using it, you should be able to dodge all locking
>> issues and it should be possible to keep the regmap usage as-is and the
>> patch would in all likelihood be much less intrusive.
>>
>> For some background on "mux-locked" vs. "parent-locked" (which is what
>> you have used in this RFC patch), see Documentation/i2c/i2c-topology.
>
> The "mux-locked" implementation was the one I first tried and I discovered
> it doesn't work for me, as other traffic on the parent adapter can get in the
> way. What we need for this device is no other traffic on the bus during the
> "select transaction deselect" procedure, that's why I went with the parent
> locking. Also this device needs a delay between stop and start conditions
> while addressing the monitor.
Ok, I thought the problem was that a delay was needed between the STOP
of the command opening the gate and the START of the edid eeprom xfer, and
that everything else worked normally. Too bad this wasn't the actual problem.
Hmm. If it is indeed true that other xfers must never creep into the "select
xfer deselect" procedure then you are indeed stuck with a parent-locking.
But is that really the case? Could it be that the extra delay between
STOP-START is only needed after the absolutely last xfer before the
command closing the gate?
If that problem description is correct, it should be possible to go back to
a mux-locked gate, but then in your deselect implementation grab the i2c adapter
lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the
30 us delay, then open code the command to close the gate with an unlocked i2c
access, and finally release the i2c bus lock. That way you have ensured silence
on the bus for the required time before closing the gate. You would still need
to bypass regmap, but only in this one place (but maybe you should bypass
regmap for opening the gate too, for symmetry).
Cheers,
Peter
>>
>> There are a couple of more comments below, inline.
>>
>
> snip
>
>
> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32
> chan_id)
> +{
> + struct sii902x *sii902x = i2c_mux_priv(mux);
> + struct device *dev = &sii902x->i2c->dev;
> + unsigned long timeout;
> + u8 status;
> + int ret;
> +
> + ret = __sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> + SII902X_SYS_CTRL_DDC_BUS_REQ,
> + SII902X_SYS_CTRL_DDC_BUS_REQ);
> +
> + timeout = jiffies +
> +
> msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> + do {
> + ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +&status);
> + if (ret)
> + return ret;
> + } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +time_before(jiffies, timeout));
> +
> + if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> + dev_err(dev, "Failed to acquire the i2c bus\n");
> + return -ETIMEDOUT;
> + }
> +
> + ret = __sii902x_write(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> status);
> + if (ret)
> + return ret;
>>
>> Why do I not see a delay here? I thought the whole point of adding the i2c
>> gate
>> was that it enabled the introduction of a dela
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-10-31 17:55, Fabrizio Castro wrote: > Hello Linus, > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback >> >> Hi Fabrizio, >> >> thanks for your patch! > > Thank you for your feedback! > >> >> On Wed, Oct 31, 2018 at 1:58 PM Fabrizio Castro >> wrote: >> >>> While adding SiI9022A support to the iwg23s board it came up >>> that when the HDMI transmitter is in pass through mode the >>> device is not compliant with the I2C specification anymore, >>> as it requires a far bigger tbuf due to a delay the HDMI >>> transmitter is adding when relaying the STOP condition on the >>> monitor i2c side of things. When not providing an appropriate >>> delay after the STOP condition the i2c bus would get stuck. >>> Also, any other traffic on the bus while talking to the monitor >>> may cause the transaction to fail or even cause issues with the >>> i2c bus as well. >>> >>> I2c-gates seemed to reach consent as a possible way to address >>> these issues, and as such this patch is implementing a solution >>> based on that. Since others are clearly relying on the current >>> implementation of the driver, this patch won't require any DT >>> changes. >>> >>> Since we don't want any interference during the DDC Bus >>> Request/Grant procedure and while talking to the monitor, we have >>> to use the adapter locking primitives rather than the i2c-mux >>> locking primitives, but in order to achieve that I had to get >>> rid of regmap. >> >> Why did you have to remove regmap? It is perfectly possible >> to override any reading/writing operations locally if you don't >> want to use the regmap i2c variants. >> >> The code in your patch does not make it evident to me exactly >> where the problem is with regmap, also I bet the regmap >> maintainer would love to hear about it so we can attempt to >> fix it there instead of locally in this driver. >> >> AFAICT there is some locked vs unlocked business and I >> strongly hope there is some way to simply pass that down >> into whatever functions regmap end up calling so we don't >> have to change all call sites. > > Yeah, there is some issue with locking here, and that's coming down > from the fact that when we call into drm_get_edid, we implicitly call > i2c_transfer which ultimately locks the i2c adapter, and then calls > into our sii902x_i2c_bypass_select, which in turn calls into the regmap > functions and therefore we try and lock the same i2c adapter again, > driving the system into a deadlock. > Having the option of using "unlocked" flavours of reads and writes > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c > I couldn't find anything suitable for my case, maybe Mark could advise > on this one? I am sure I overlooked something here, is there a better > way to address this? This recursive locking problem surfaces when an i2c mux is operated by writing commands on the same i2c bus that is muxed. When this happens for a typical i2c mux chip like nxp,pca9548 this can be kept local to that driver. However, when there are interactions with other parts of the kernel (e.g. if the i2c-mux is operated by gpio pins, and those gpio pins in turn are operated by accesses to the i2c bus that is muxed) this locked vs. unlocked thing would spread like wildfire. Since I did not like that wildfire, I came up with the "mux-locked" scheme. It is not appropriate everywhere, but in this case I think it is a perfect fit. By using it, you should be able to dodge all locking issues and it should be possible to keep the regmap usage as-is and the patch would in all likelihood be much less intrusive. For some background on "mux-locked" vs. "parent-locked" (which is what you have used in this RFC patch), see Documentation/i2c/i2c-topology. There are a couple of more comments below, inline. >> >> (So please include Mark Brown on CC in future iterations.) >> >>> Signed-off-by: Fabrizio Castro >>> --- >>> Dear All, >>> >>> this is a follow up to: >>> https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg32072.html >>> >>> Comments very welcome! >> >> At the very least I think the patch needs to be split in two, >> one dealing with the locking business and one dealing with >> the buffer size. As it looks now it is very hard to review. > > The change with the buffer size comes down from sii902x_bulk_write > implementation, > which btw is the only function that doesn't resembles its regmap equivalent, > in terms > of parameters. > >> >>> >>> Thanks, >>> Fab >>> >>> drivers/gpu/drm/bridge/sii902x.c | 404 >>> ++- >>> 1 file changed, 274 insertions(+), 130 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/sii902x.c >>> b/drivers/gpu/drm/bridge/sii902x.c >>> index e59a135..137a05a 100644 >>> --- a/drivers/gpu/drm/bridge/sii902x.c >>> +++ b/drivers/gpu/drm/bridge/sii902x.c >>> @@ -21,9 +21,9 @@ >>> */ >>> >>> #include >>> +#include >>> #include >>> #include >>> -#include >>> >>> #include >>> #include
Re: Delay between stop condition and start condition
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
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
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
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
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
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(&adap->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
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
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 1/2] i2c: recovery: add get_bus_free callback
On 2018-07-11 00:24, Wolfram Sang wrote:
> Some IP cores have an internal 'bus free' logic which may be more
> advanced than just checking if SDA is high. Add a separate callback to
> get this status. Filling it is optional.
>
> Signed-off-by: Wolfram Sang
> ---
> drivers/i2c/i2c-core-base.c | 27 +++
> include/linux/i2c.h | 3 +++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index c7995efd58ea..59f8dfc5be36 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -158,6 +158,22 @@ static void set_sda_gpio_value(struct i2c_adapter *adap,
> int val)
> gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val);
> }
>
> +static int i2c_generic_bus_free(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + int ret = -EOPNOTSUPP;
> +
> + if (bri->get_bus_free)
> + ret = bri->get_bus_free(adap);
> + else if (bri->get_sda)
> + ret = bri->get_sda(adap);
> +
> + if (ret < 0)
> + return ret;
> +
> + return ret ? 0 : -EBUSY;
> +}
Hmmm, the name i2c_generic_bus_free suggests that scl should also be checked,
because the *bus* isn't really free unless both lines are high? No? Or, maybe
just rename to i2c_generic_sda_free (or something)?
But that's of course just a nit...
More importantly, isn't a ->get_bus_free implementation a sufficient requirement
for recovery? I.e. even if both ->get_sda and ->set_sda are missing?
Cheers,
Peter
> +
> /*
> * We are generating clock pulses. ndelay() determines durating of clk
> pulses.
> * We will generate clock with rate 100 KHz and so duration of both clock
> levels
> @@ -169,7 +185,7 @@ static void set_sda_gpio_value(struct i2c_adapter *adap,
> int val)
> 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 = 0;
> + int i = 0, val = 1, ret;
>
> if (bri->prepare_recovery)
> bri->prepare_recovery(adap);
> @@ -207,14 +223,17 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> bri->set_sda(adap, val);
> ndelay(RECOVERY_NDELAY / 2);
>
> - /* Break if SDA is high */
> - if (val && bri->get_sda) {
> - ret = bri->get_sda(adap) ? 0 : -EBUSY;
> + if (val) {
> + ret = i2c_generic_bus_free(adap);
> if (ret == 0)
> break;
> }
> }
>
> + /* If we can't check bus status, assume recovery worked */
> + if (ret == -EOPNOTSUPP)
> + ret = 0;
> +
> if (bri->unprepare_recovery)
> bri->unprepare_recovery(adap);
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 9d1818c56775..60e224428a85 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -587,6 +587,8 @@ struct i2c_timings {
> * @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.
> + * @get_bus_free: Returns the bus free state as seen from the IP core in
> case it
> + * has a more complex internal logic than just reading SDA. Optional.
> * @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
> @@ -601,6 +603,7 @@ struct i2c_bus_recovery_info {
> void (*set_scl)(struct i2c_adapter *adap, int val);
> int (*get_sda)(struct i2c_adapter *adap);
> void (*set_sda)(struct i2c_adapter *adap, int val);
> + int (*get_bus_free)(struct i2c_adapter *adap);
>
> void (*prepare_recovery)(struct i2c_adapter *adap);
> void (*unprepare_recovery)(struct i2c_adapter *adap);
>
Re: [PATCH v2] i2c: recovery: rename variable for easier understanding
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(&adap->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
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
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
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
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
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
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
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(&priv->cur_adap.dev.kobj, &priv->dev->kobj, > +"demux_device"), > + "can't create symlink to mux device\n"); > + WARN(sysfs_create_link(&priv->dev->kobj, &priv->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(&priv->dev->kobj, "channel-0"); > + sysfs_remove_link(&priv->cur_adap.dev.kobj, "demux_device"); > + > i2c_del_adapter(&priv->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
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
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(&pdev->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
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(&priv->cur_adap.dev.kobj, &priv->dev->kobj, > +"demux_device"); > + sysfs_create_link(&priv->dev->kobj, &priv->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(&priv->dev->kobj, "channel-0"); > + sysfs_remove_link(&priv->cur_adap.dev.kobj, "demux_device"); > + > i2c_del_adapter(&priv->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
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 = &priv->algo; > priv->cur_adap.algo_data = priv; > - priv->cur_adap.dev.parent = priv->dev; > + priv->cur_adap.dev.parent = &adap->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
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
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
Acked-by: Daniel Vetter
Signed-off-by: Peter Rosin
---
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(&bridge_lock);
list_for_each_entry(bridge, &bridge_list, list) {
- if (bridge->of_node == np) {
+ if ((bridge->odev && bridge->odev->of_node == np) ||
+ bridge->of_node == np) {
mutex_unlock(&bridge_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", &name))
/* 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
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/g
[PATCH v3 22/26] drm/rcar-du: lvds: provide an owner .odev device for the bridge
The .of_node member is going away. Signed-off-by: Peter Rosin --- 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 = &pdev->dev; lvds->bridge.driver_private = lvds; lvds->bridge.funcs = &rcar_lvds_bridge_ops; - lvds->bridge.of_node = pdev->dev.of_node; mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); lvds->mmio = devm_ioremap_resource(&pdev->dev, mem); -- 2.11.0
[PATCH v3 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach
The .odev owner device will be handy to have around.
Reviewed-by: Andrzej Hajda
Acked-by: Daniel Vetter
Signed-off-by: Peter Rosin
---
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(&bridge_lock);
list_add_tail(&bridge->list, &bridge_list);
mutex_unlock(&bridge_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
It is unused.
Acked-by: Daniel Vetter
Signed-off-by: Peter Rosin
---
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(&bridge_lock);
list_for_each_entry(bridge, &bridge_list, list) {
- if ((bridge->odev && bridge->odev->of_node == np) ||
- bridge->of_node == np) {
+ if (bridge->odev->of_node == np) {
mutex_unlock(&bridge_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", &name))
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
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
Acked-by: Daniel Vetter
Signed-off-by: Peter Rosin
---
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
On 2018-05-15 12:22, Daniel Vetter wrote:
> On Mon, May 14, 2018 at 10:40 PM, Peter Rosin 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
>>>>>> ---
>>>>>> 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
>>>>> t
Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
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
>>>> ---
>>>> 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
>>
>> 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
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
>> ---
>> 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
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
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 >>> --- >>> 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 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 --- 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", &name)) /* 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
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
>> ---
>> 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(&bridge_lock);
>>
>> list_for_each_entry(bridge, &bridge_list, list) {
>> -if (bridge->of_node == np) {
>> +if ((bridge->odev && bridge->odev->of_node == np) ||
>> +bridge->of_node == np) {
>> mutex_unlock(&bridge_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", &name))
>> /* 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
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-d
Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
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
>> ---
>> 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
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
---
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(&bridge_lock);
list_for_each_entry(bridge, &bridge_list, list) {
- if (bridge->of_node == np) {
+ if ((bridge->odev && bridge->odev->of_node == np) ||
+ bridge->of_node == np) {
mutex_unlock(&bridge_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", &name))
/* 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
The .of_node member is going away. Signed-off-by: Peter Rosin --- 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 = &pdev->dev; lvds->bridge.driver_private = lvds; lvds->bridge.funcs = &rcar_lvds_bridge_ops; - lvds->bridge.of_node = pdev->dev.of_node; mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); lvds->mmio = devm_ioremap_resource(&pdev->dev, mem); -- 2.11.0
[PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach
The .odev owner device will be handy to have around.
Signed-off-by: Peter Rosin
---
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(&bridge_lock);
list_add_tail(&bridge->list, &bridge_list);
mutex_unlock(&bridge_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 24/26] drm/bridge: remove the .of_node member
It is unused.
Signed-off-by: Peter Rosin
---
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(&bridge_lock);
list_for_each_entry(bridge, &bridge_list, list) {
- if ((bridge->odev && bridge->odev->of_node == np) ||
- bridge->of_node == np) {
+ if (bridge->odev->of_node == np) {
mutex_unlock(&bridge_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", &name))
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 26/26] drm/bridge: establish a link between the bridge supplier and consumer
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
---
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
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/gp
Re: [PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer
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 > > 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 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
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 >> --- >> 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
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", &name)) /* 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", &name)) 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
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
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
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
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
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
---
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(&bridge_lock);
list_for_each_entry(bridge, &bridge_list, list) {
- if (bridge->of_node == np) {
+ if ((bridge->owner && bridge->owner->of_node == np) ||
+ bridge->of_node == np) {
mutex_unlock(&bridge_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
The .of_node member is going away. Signed-off-by: Peter Rosin --- 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 = &pdev->dev; lvds->bridge.driver_private = lvds; lvds->bridge.funcs = &rcar_lvds_bridge_ops; - lvds->bridge.of_node = pdev->dev.of_node; mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); lvds->mmio = devm_ioremap_resource(&pdev->dev, mem); -- 2.11.0
[PATCH 22/24] drm/bridge: remove the .of_node member
It is unused.
Signed-off-by: Peter Rosin
---
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(&bridge_lock);
list_for_each_entry(bridge, &bridge_list, list) {
- if ((bridge->owner && bridge->owner->of_node == np) ||
- bridge->of_node == np) {
+ if (bridge->owner->of_node == np) {
mutex_unlock(&bridge_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
The .owner will be handy to have around. Signed-off-by: Peter Rosin --- 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
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
---
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
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
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
>>> ---
>>> 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 = &lvds->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, &bus_fmt);
>>> + else
>>> + ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
>>
>> What if no bridge reports any format, shouldn't the connector be examined
>> then?
>
Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support
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 = &lvds->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, &bus_fmt);
> + else
> + ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_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
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", &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(&thc63->bridge, &bus_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 = &thc63_bridge_func;
>
Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property
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 = <&gpio4 15 GPIO_ACTIVE_LOW>;
> + thine,map = <1>;
>
> ports {
> #address-cells = <1>;
>
Re: [PATCH 1/8] drm: bridge: Add support for static image formats
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 &drm_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
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 Cheers, Peter On 2017-07-17 17:24, Ulrich Hecht wrote: > Required for __must_check. > > Signed-off-by: Ulrich Hecht > --- > 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
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(&serdev->dev);
> serdev->dev.parent = &ctrl->dev;
> serdev->dev.bus = &serdev_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 = &ctrl->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
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(&dev->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(&dev->serdev->dev, "excess data ignored\n");
> + return count;
> +
> + case RX_EXPECT_ACK:
> + case RX_EXPECT_ACK_DATA:
> + if (data[0] != ACK) {
> + dev_dbg(&dev->serdev->dev, "frame error");
> + dev->rx_state = RX_FRAME_ERROR;
> + wake_up_interruptible(&dev->rx_wq);
> +
Re: [RFC 0/4] serdev GPIO-based multiplexing support
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
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
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 != &i2c_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
>>> 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 = &priv->algo;
>>> priv->adap.algo_data = priv;
>>> - priv->adap.dev.parent = &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
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 = &priv->algo; > priv->adap.algo_data = priv; > - priv->adap.dev.parent = &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
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 > --- > 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 Thanks, Peter
RE: [PATCH] i2c: mux: demux-pinctrl: Update docs to new sysfs-attributes
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
