* Re: [PATCH] [trivial] i2c: Spelling s/acknowedge/acknowledge/
From: Wolfram Sang @ 2016-09-08 20:22 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Jiri Kosina, linux-i2c, linux-kernel
In-Reply-To: <1472636328-27229-1-git-send-email-geert+renesas@glider.be>
[-- Attachment #1: Type: text/plain, Size: 164 bytes --]
On Wed, Aug 31, 2016 at 11:38:48AM +0200, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v1] i2c: designware: save the preset value of DW_IC_SDA_HOLD
From: Wolfram Sang @ 2016-09-08 20:18 UTC (permalink / raw)
To: Zhuo-hao Lee
Cc: jarkko.nikula, andriy.shevchenko, mika.westerberg, linux-i2c,
linux-kernel
In-Reply-To: <1472283570-31471-1-git-send-email-zhuo-hao.lee@intel.com>
[-- Attachment #1: Type: text/plain, Size: 901 bytes --]
On Sat, Aug 27, 2016 at 03:39:30PM +0800, Zhuo-hao Lee wrote:
> There are several ways to set the SDA hold time for i2c controller,
> including: Device Tree, built-in device properties and ACPI. However,
> if the SDA hold time is not specified by above method, we should
> read the value, where it is preset by firmware, and save it to
> sda_hold_time. This is needed because when i2c controller enters
> runtime suspend, the DW_IC_SDA_HOLD value will be reset to chipset
> default value. And during runtime resume, i2c_dw_init will be called
> to reconfigure i2c controller. If sda_hold_time is zero, the chipset
> default hold time will be used, that will be too short for some
> platforms. Therefore, to have a better tolerance, the DW_IC_SDA_HOLD
> value should be kept by sda_hold_time.
>
> Signed-off-by: Zhuo-hao Lee <zhuo-hao.lee@intel.com>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation: i2c: slave-interface: add note for driver development
From: Wolfram Sang @ 2016-09-08 14:57 UTC (permalink / raw)
To: linux-i2c
In-Reply-To: <20160825172740.25390-1-wsa@the-dreams.de>
[-- Attachment #1: Type: text/plain, Size: 273 bytes --]
On Thu, Aug 25, 2016 at 07:27:40PM +0200, Wolfram Sang wrote:
> Make it clear that adding slave support shall not disable master
> functionality. We can have both, so we should.
>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: mux: demux-pinctrl: run properly with multiple instances
From: Wolfram Sang @ 2016-09-08 14:50 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, linux-renesas-soc, Simon Horman, Pantelis Antoniou
In-Reply-To: <20160823152803.2051-1-wsa+renesas@sang-engineering.com>
[-- Attachment #1: Type: text/plain, Size: 500 bytes --]
On Tue, Aug 23, 2016 at 05:28:03PM +0200, Wolfram Sang wrote:
> We can't use a static property for all the changesets, so we now create
> dynamic ones for each changeset.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Fixes: 50a5ba87690814 ("i2c: mux: demux-pinctrl: add driver")
Applied to for-current (and thus will be in 4.8), thanks!
> Simon: I think you can retest your demuxer-dts-series now.
Simon: you think we can get that into 4.9? Would be awesome.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: DT connectors, thoughts
From: David Gibson @ 2016-09-08 0:26 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, Frank Rowand, Pantelis Antoniou, Mark Brown,
Grant Likely, Mark Rutland, Matt Porter, Koen Kooi, Guenter Roeck,
Marek Vašut, Wolfram Sang, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
Pantelis Antoniou
In-Reply-To: <147329189889.27412.12053595745294007737@sboyd-linaro>
[-- Attachment #1: Type: text/plain, Size: 8916 bytes --]
On Wed, Sep 07, 2016 at 04:44:58PM -0700, Stephen Boyd wrote:
> Quoting David Gibson (2016-08-30 16:55:23)
> > On Mon, Aug 29, 2016 at 07:07:49PM -0700, Stephen Boyd wrote:
> > > Quoting David Gibson (2016-08-29 06:45:11)
> > > > So, combining those thoughts together, I'm thinking dtc format for
> > > > something connecting to two different widget sockets (pretty much the
> > > > worst case) would look something like:
> > > >
> > > > /plugin/ foo,widget-socket {
> > > > };
> > > >
> > > > /plugin/ foo,widget-socket {
> > > > realias {
> > > > i2c-b = "i2c";
> > > > intc-b = "intc";
> > > > mmio-b = "mmio";
> > > > };
> > > > };
> > > >
> > > > &i2c {
> > > > .. devices on the i2c from the first plug ..
> > > > };
> > > >
> > > > &i2c-b {
> > > > .. devices on the i2c from the second plug ..
> > > > };
> > > >
> > > > Obviously we'd also need to devise an encoding for this to compile
> > > > into, since the one I proposed previously won't work in this case.
> > > >
> > >
> > > I suppose we can distribute the realias nodes when we compile the plugin
> > > into overlay fragments. The socket matching is a little vague though.
> > > How would we know which socket to apply to when we have two identical
> > > looking sockets? I'm thinking we could put some of that information into
> > > the fragment itself.
> >
> > So, my assumption in this example was that the plugin could plug into
> > *any* two widget sockets. If it needs to connect to specific ones,
> > then pretty much by definition, the sockets aren't really of
> > indistinguishable type.
>
> Ah ok. It wasn't clear on how we target where the extension board
> connects into the full tree. It sounds like the decision of where to
> plug in the extension board falls to the overlay manager then?
Yes. The whole point of connectors is that they're interchangeable,
so neither the "plug" side nor the "socket" side should specify which
instance of the other part they're connected to.
So to resolve the whole tree in this case, extra information would be
needed. Ideally it would come from probing (even something ad-hoc
like an ID register), failing that it would need to be admin
configured.
> > > /{
> > > compatible = "foo,whirlgig-widget";
> > >
> > > fragment@0 { /* corresponds to i2c in example above */
> > > target-socket = "foo,widget-socket-a";
> > > target-alias = "i2c";
> > > __overlay__ {
> > > ....
> > > };
> > > };
> > >
> > > fragment@1 { /* corresponds to i2c-b in example above */
> > > target-socket = "foo,widget-socket-b";
> > > target-alias = "i2c";
> > > __overlay__ {
> > > ...
> > > };
> > > };
> > > };
> >
> > We don't need any new construct here. In this case the sockets aren't
> > 100% compatible, which we can notate with
> > compatible = "widget-socket-a", "widget-socket";
> > In the base board.
> >
> > Devices which can plug into any widget socket will use target-socket =
> > "widget-socket", those which require a specific one (including
> > requiring both) can specifically list "widget-socket-a" and/or
> > "widget-socket-b".
>
> Agreed. How do we express the realias node in the overlay format though?
> I was trying to come up with some way to make that work by
> redistributing the alias to the fragments but that doesn't work so
> well.
So, I'm thinking of the overlay dt info in 3 pieces
1) Global metadata
Not much here yet, but it seems like it could be useful.
2) Per-connector metadata
This specifies what sort of "socket" each "plug" on the overlay
needs to connect to, and any other information we need that's
specific to a particular "plug"
3) DT fragments
The actual device info applied once we've resolved all the aliases
and whatnot.
The realias information would need to be in part (2)
How we encode those pieces into the final overlay dt is pretty close
to an arbitrary choice.
> > > If we have two identical connectors maybe we'll have to enforce that the
> > > connectors have some more specific unique compatible string so that we
> > > can match up the right socket. But I don't see how we can require that
> > > the overlays know this detail if they only care about one socket and
> > > could go into either one of them. In that case we should have the loader
> > > ask the user which socket they connected this extension board to?
> > >
> > > I was also thinking it would be better to leave the gpio-map and
> > > interrupt-map properties at the connector level. For example:
> > >
> > > widget1 {
> > > compatible = "foo,widget-socket";
> > > interrupt-map-mask = <0xffffffff>;
> > > interrupt-map = <0 &intc 7 0>,
> > > <1 &intc 8 0>;
> > > };
> >
> > That could work - but we should (and implicitly, do) support either
> > way. Using subnodes might be useful for particularly complex irq or
> > gpio mappings.
>
> Sounds good.
>
> >
> > > and then we could put a label on the plugin/expansion syntax so we can
> > > reference the connector as a whole:
> > >
> > > /plugin/ connector: foo,widget-socket {
> > > compatible = "foo,whirlgig-widget";
> > > };
> > >
> > > &i2c {
> > > device@40 {
> > > interrupt-parent = <&connector>;
> > > interrupts = <1>;
> > > };
> > > };
> > >
> > > I also thought about making another alias inside the connector node to
> > > point to itself, but that fails when you get into the situation of two
> > > connectors and collisions, unless you rename them of course. It felt
> > > better to leave that choice to the overlay though.
> > >
> > > In conclusion, I see a few topics/patterns emerging:
> > >
> > > 1) Expose phandles through the connectors in some way that allows us to
> > > limit what the plugin/expansion boards can modify or use
> >
> > Yes, I definitely think we want that.
> >
> > > 2) Have some flexible syntax to remap cell sizes from the baseboard
> > > through the connector to provide a consistent connector size (i.e.
> > > remap interrupts and gpios from multiple sources, etc. into a fixed
> > > number of cells)
> >
> > I don't think we need any new constructs here. If there are
> > mismatches we can put dummy bridges with appropriate ranges properties
> > on one side or the other.
> >
> > The only thing I see that might want some help is that the connector
> > type should certainly imply a specific set of cell widths for all the
> > included buses. So possibly we should supply some stuff to help
> > enforce that.
>
> I'm specifically thinking that anything that has a #<name>-cells
> associated with it (#gpio-cells, #clock-cells, #power-domain-cells,
> etc.) would need to have an associated <name>-map property and
> associated parsing code so that we can make a consistent cell width for
> the connector. If we have existing ways to make this work, e.g.
> interrupt-map or ranges, then we don't need.
Yes, I agree. All the "interrupt tree like" things need interrupt
tree like nexus nodes, with a map and map-mask.
However, we also need some way of ensuring that the #address-cells and
#size-cells for any buses we expose match properly.
Or maybe we need to invent something like a cross between 'ranges' and
'interrupt-map' that can remap ranges on one address space to the
address space attached to a different node.
>
> >
> > > 3) Allow plugin/expansion boards to use multiple connectors from the
> > > baseboard in a consistent way
> >
> > Seems reasonable.
> >
> > > 4) Attempt to maintain almost all of the current overlay syntax with
> > > syntactic sugar
> >
> > I'm not really sure what you mean by that.
> >
>
> I mean that we're not really trying to change the general structure of
> the DT overlay syntax. At least from what I can tell we're planning to
> convert this /plugin/ format into a DT overlay that gets compiled into
> binary all inside DTC.
Roughly speaking, yes. However I think we should take the opportunity
to address design flaws in the current overlay encoding as they come
up (e.g. the fact that it uses phandles which *always* have to be
resolved - better to directly use an alias / symbol / label).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* [PATCH v1 2/2] i2c-mux-gpio: call i2c_add_reparented_mux_adapter
From: jim_baxter @ 2016-09-07 20:05 UTC (permalink / raw)
To: linux-i2c; +Cc: Peter Korsgaard, Wolfram Sang, Peter Rosin, linux-kernel
In-Reply-To: <1473278729-16329-1-git-send-email-jim_baxter@mentor.com>
From: Joshua Frkuska <joshua_frkuska@mentor.com>
This reparents the adapter created in i2c-mux to this module for
module unloading and chaining purposes.
Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
drivers/i2c/muxes/i2c-mux-gpio.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index e5cf26e..9f92535 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -234,7 +234,11 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
- ret = i2c_mux_add_adapter(muxc, nr, mux->data.values[i], class);
+ ret = i2c_mux_add_reparented_adapter(THIS_MODULE,
+ muxc,
+ nr,
+ mux->data.values[i],
+ class);
if (ret) {
dev_err(&pdev->dev, "Failed to add adapter %d\n", i);
goto add_adapter_failed;
--
1.9.1
^ permalink raw reply related
* [PATCH v1 1/2] i2c-mux: add i2c_mux_add_reparented_adapter api
From: jim_baxter @ 2016-09-07 20:05 UTC (permalink / raw)
To: linux-i2c; +Cc: Peter Korsgaard, Wolfram Sang, Peter Rosin, linux-kernel
In-Reply-To: <1473278729-16329-1-git-send-email-jim_baxter@mentor.com>
From: Joshua Frkuska <joshua_frkuska@mentor.com>
This new api allows the i2c adapter created by i2c-mux to be owned by
the module that calls this api. This allows module removal chaining to
be done at a higher level.
Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
drivers/i2c/i2c-mux.c | 14 ++++++++++++--
include/linux/i2c-mux.h | 15 +++++++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 8eee986..191816b 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -263,7 +263,8 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
}
EXPORT_SYMBOL_GPL(i2c_mux_alloc);
-int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
+int i2c_mux_add_reparented_adapter(struct module *owner,
+ struct i2c_mux_core *muxc,
u32 force_nr, u32 chan_id,
unsigned int class)
{
@@ -305,7 +306,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
/* Now fill out new adapter structure */
snprintf(priv->adap.name, sizeof(priv->adap.name),
"i2c-%d-mux (chan_id %d)", i2c_adapter_id(parent), chan_id);
- priv->adap.owner = THIS_MODULE;
+ priv->adap.owner = owner;
priv->adap.algo = &priv->algo;
priv->adap.algo_data = priv;
priv->adap.dev.parent = &parent->dev;
@@ -385,6 +386,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
muxc->adapter[muxc->num_adapters++] = &priv->adap;
return 0;
}
+EXPORT_SYMBOL_GPL(i2c_mux_add_reparented_adapter);
+
+int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
+ u32 force_nr, u32 chan_id,
+ unsigned int class)
+{
+ return i2c_mux_add_reparented_adapter(THIS_MODULE, muxc, force_nr,
+ chan_id, class);
+}
EXPORT_SYMBOL_GPL(i2c_mux_add_adapter);
void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index d4c1d12..d4f8610 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -64,6 +64,21 @@ struct i2c_adapter *i2c_root_adapter(struct device *dev);
* Called to create an i2c bus on a multiplexed bus segment.
* The chan_id parameter is passed to the select and deselect
* callback functions to perform hardware-specific mux control.
+ *
+ * Unlike the simple i2c_add_mux_adapter, this is passed in the caller's
+ * module reference in order to reparent the adapter from the perspective of
+ * the i2c hierarchy. This allows the caller to be reference locked while the
+ * i2c adapter is in use.
+ */
+int i2c_mux_add_reparented_adapter(struct module *owner,
+ struct i2c_mux_core *muxc,
+ u32 force_nr, u32 chan_id,
+ unsigned int class);
+
+/*
+ * Called to create an i2c bus on a multiplexed bus segment.
+ * The chan_id parameter is passed to the select and deselect
+ * callback functions to perform hardware-specific mux control.
*/
int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
u32 force_nr, u32 chan_id,
--
1.9.1
^ permalink raw reply related
* [PATCH v1 0/2] i2c: Stop i2c modules being unloaded while in use.
From: jim_baxter @ 2016-09-07 20:05 UTC (permalink / raw)
To: linux-i2c; +Cc: Peter Korsgaard, Wolfram Sang, Peter Rosin, linux-kernel
From: Jim Baxter <jim_baxter@mentor.com>
This patchset adds a new i2c_mux_add_reparented_adapter API to the i2c
that allows owning modules to use module_get/module_put and stop the
i2c bus module being removed whilst in use.
This was tested on an ARM i.MX6 Sabre board with the pca953x gpio module.
Joshua Frkuska (2):
i2c-mux: add i2c_mux_add_reparented_adapter api
i2c-mux-gpio: call i2c_add_reparented_mux_adapter
drivers/i2c/i2c-mux.c | 14 ++++++++++++--
drivers/i2c/muxes/i2c-mux-gpio.c | 6 +++++-
include/linux/i2c-mux.h | 15 +++++++++++++++
3 files changed, 32 insertions(+), 3 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH v6] i2c: imx: make bus recovery through pinctrl optional
From: Uwe Kleine-König @ 2016-09-07 18:09 UTC (permalink / raw)
To: Li Yang
Cc: Wolfram Sang, linux-i2c, linux-kernel, linux-gpio,
linux-arm-kernel, Gao Pan
In-Reply-To: <1473267796-10403-1-git-send-email-leoyang.li@nxp.com>
Hello,
On Wed, Sep 07, 2016 at 12:03:16PM -0500, Li Yang wrote:
> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
> is not always available for platforms with this controller such as
> ls1021a and ls1043a, and the device tree binding also mentioned this
> gpio based recovery mechanism as optional. The patch makes it really
> optional that the probe function won't bailout but just disable the
> bus recovery function when pinctrl is not usable.
>
> Signed-off-by: Li Yang <leoyang.li@nxp.com>
> Cc: Gao Pan <pandy.gao@nxp.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> v6:
> Update code comment
>
> v5:
> Revert the last minute change of recovery info initialization timing, it
> will cause problem if initialized after i2c_add_numbered_adapter()
>
> v4:
> Remove the use of IS_ERR_OR_NULL
> Move the condition judgement to i2c_imx_init_recovery_info()
> Change the timing of recovery initialization to be after bus registration
>
> v3:
> Rebased to Wolfram's for-next branch
> Added acked-by from Linus Walleij
> Update to use new nxp email addresses due to company merge
>
> drivers/i2c/busses/i2c-imx.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 1844bc9..f52ff47 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -989,6 +989,14 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
> {
> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>
> + /* If pinctrl is not available disable bus recovery */
> + if (IS_ERR(i2c_imx->pinctrl))
> + i2c_imx->pinctrl = NULL;
I still prefer to do this in i2c_imx_probe to have a single place that
assignes to i2c_imx->pinctrl.
> + if (!i2c_imx->pinctrl) {
> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> + return;
> + }
> +
> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
> PINCTRL_STATE_DEFAULT);
> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> @@ -1081,8 +1089,19 @@ static int i2c_imx_probe(struct platform_device *pdev)
> return ret;
> }
>
> + /*
> + * If pinctrl is available it might be possible to switch the
> + * SDA and SCL lines to their GPIO functions and do a recovery
> + * procedure in this mode. If there is anything wrong with
> + * pinctrl we cannot do this and simply not enable it.
> + */
> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> - if (IS_ERR(i2c_imx->pinctrl)) {
> + /*
> + * Bailout on critical errors, continue for other errors.
> + * Bus recovery will not be enabled if pinctrl is not available.
> + */
> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
> ret = PTR_ERR(i2c_imx->pinctrl);
> goto clk_disable;
I'd not call EPROBE_DEFER a critical error. Maybe only handle this and
also ignore ENOMEM? You argued that ENOMEM might affect the i2c driver,
too. I'd say it's worth to try that because not enough memory might also
happen because pinctrl_get asked for a too big chunk.
> }
> @@ -1125,6 +1144,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> i2c_imx, IMX_I2C_I2CR);
> imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR);
>
> + /* Init bus recovery info if supported */
> i2c_imx_init_recovery_info(i2c_imx, pdev);
>
> /* Add I2C adapter */
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH v6] i2c: imx: make bus recovery through pinctrl optional
From: Li Yang @ 2016-09-07 17:03 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, linux-kernel, linux-gpio,
linux-arm-kernel
Cc: Uwe Kleine-König, Gao Pan, Li Yang
Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
is not always available for platforms with this controller such as
ls1021a and ls1043a, and the device tree binding also mentioned this
gpio based recovery mechanism as optional. The patch makes it really
optional that the probe function won't bailout but just disable the
bus recovery function when pinctrl is not usable.
Signed-off-by: Li Yang <leoyang.li@nxp.com>
Cc: Gao Pan <pandy.gao@nxp.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
v6:
Update code comment
v5:
Revert the last minute change of recovery info initialization timing, it
will cause problem if initialized after i2c_add_numbered_adapter()
v4:
Remove the use of IS_ERR_OR_NULL
Move the condition judgement to i2c_imx_init_recovery_info()
Change the timing of recovery initialization to be after bus registration
v3:
Rebased to Wolfram's for-next branch
Added acked-by from Linus Walleij
Update to use new nxp email addresses due to company merge
drivers/i2c/busses/i2c-imx.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 1844bc9..f52ff47 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -989,6 +989,14 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
{
struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
+ /* If pinctrl is not available disable bus recovery */
+ if (IS_ERR(i2c_imx->pinctrl))
+ i2c_imx->pinctrl = NULL;
+ if (!i2c_imx->pinctrl) {
+ dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
+ return;
+ }
+
i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
PINCTRL_STATE_DEFAULT);
i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
@@ -1081,8 +1089,19 @@ static int i2c_imx_probe(struct platform_device *pdev)
return ret;
}
+ /*
+ * If pinctrl is available it might be possible to switch the
+ * SDA and SCL lines to their GPIO functions and do a recovery
+ * procedure in this mode. If there is anything wrong with
+ * pinctrl we cannot do this and simply not enable it.
+ */
i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
- if (IS_ERR(i2c_imx->pinctrl)) {
+ /*
+ * Bailout on critical errors, continue for other errors.
+ * Bus recovery will not be enabled if pinctrl is not available.
+ */
+ if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
+ PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
ret = PTR_ERR(i2c_imx->pinctrl);
goto clk_disable;
}
@@ -1125,6 +1144,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
i2c_imx, IMX_I2C_I2CR);
imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR);
+ /* Init bus recovery info if supported */
i2c_imx_init_recovery_info(i2c_imx, pdev);
/* Add I2C adapter */
--
1.9.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Leo Li @ 2016-09-07 16:40 UTC (permalink / raw)
To: Tracy Smith
Cc: Uwe Kleine-König, linux-i2c@vger.kernel.org, Wolfram Sang,
linux-arm-kernel@lists.infradead.org, linux-gpio, Li Yang,
Stefan Agner, Gao Pan, lkml
In-Reply-To: <CAChUvXMPDZB7=ZArQ5+6ae42N=Bsgv_OczGXnyhBjEdjt_yvsw@mail.gmail.com>
On Wed, Sep 7, 2016 at 5:07 AM, Tracy Smith <tlsmith3777@gmail.com> wrote:
> Hello, bus recovery is needed generally speaking because of potential
> protocol errors that might cause a failure condition hanging the bus.
>
> It happens frequently during bring-up of new I2C devices because firmware in
> I2C controllers fail to handle properly protocol errors.
>
> Can NXP add bus recovery for the LS1021A and LS1043A in a separate patch--
> unless there is no HW bus recovery mechanism?
>
> The concern is while fixing I.MX, NXP will fail to fix the driver bus
> recovery for the LS1021A and LS1043A and the bus will hang.
>
> If bus recovery is supported on the LS1021A and the LS1043A, a patch should
> be provided or added in this patch instead of simply disabling bus recovery.
> Request NXP to consider the patch if there is HW support for bus recovery.
I'm not the right person to answer if this is possible. I will
forward your request to the related developer.
Thanks,
Leo
^ permalink raw reply
* Re: [PATCH V2 9/9] i2c: tegra: Add pinctrl support
From: Linus Walleij @ 2016-09-07 14:17 UTC (permalink / raw)
To: Jon Hunter
Cc: Laxman Dewangan, Wolfram Sang, Stephen Warren, Thierry Reding,
Alexandre Courbot, linux-i2c@vger.kernel.org,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1472216945-11818-10-git-send-email-jonathanh@nvidia.com>
On Fri, Aug 26, 2016 at 3:09 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
> On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX
> (DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared
> with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is
> handled by a register in the DPAUX and so the Tegra DPAUX driver has
> been updated to register a pinctrl device for managing these pins.
>
> The pins for these particular I2C devices are bound to the I2C device
> prior to probing. However, these I2C devices are in a different power
> partition to the DPAUX devices that own the pins. Hence, it is desirable
> to place the pins in the 'idle' state and allow the DPAUX power
> partition to switch off, when these I2C devices is not in use.
> Therefore, add calls to place the I2C pins in the 'default' and 'idle'
> states when the I2C device is runtime resumed and suspended,
> respectively.
>
> Please note that the pinctrl functions that set the state of the pins
> check to see if the devices has pins associated and will return zero
> if they do not. Therefore, it is safe to call these pinctrl functions
> even for I2C devices that do not have any pins associated.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
This is exactly how I imagined these states being used,
so obviously:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable
From: Yadi Hu @ 2016-09-07 9:25 UTC (permalink / raw)
To: yadi.hu, wsa, jdelvare; +Cc: linux-i2c
In-Reply-To: <1473240337-5694-1-git-send-email-yadi.hu@windriver.com>
From: "Yadi.hu" <yadi.hu@windriver.com>
the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, isassigned an effective value.
it is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others. since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.
At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.
the new patch adds a check point on interrupt handler in case field
'pch_base_address' has not been initialed.
Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
---
drivers/i2c/busses/i2c-eg20t.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index 137125b..4ac8a49 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -152,6 +152,7 @@ struct i2c_algo_pch_data {
int pch_buff_mode_en;
u32 pch_event_flag;
bool pch_i2c_xfer_in_progress;
+ bool initflag;
};
/**
@@ -635,6 +636,8 @@ static irqreturn_t pch_i2c_handler(int irq, void *pData)
u32 mode;
for (i = 0, flag = 0; i < adap_info->ch_num; i++) {
+ if (!adap_info->pch_data[i].initflag)
+ continue;
p = adap_info->pch_data[i].pch_base_address;
mode = ioread32(p + PCH_I2CMOD);
mode &= BUFFER_MODE | EEPROM_SR_MODE;
@@ -783,6 +786,7 @@ static int pch_i2c_probe(struct pci_dev *pdev,
for (i = 0; i < adap_info->ch_num; i++) {
pch_adap = &adap_info->pch_data[i].pch_adapter;
adap_info->pch_i2c_suspended = false;
+ adap_info->pch_data[i].initflag = false;
adap_info->pch_data[i].p_adapter_info = adap_info;
@@ -806,6 +810,7 @@ static int pch_i2c_probe(struct pci_dev *pdev,
pch_pci_err(pdev, "i2c_add_adapter[ch:%d] FAILED\n", i);
goto err_add_adapter;
}
+ adap_info->pch_data[i].initflag = true;
}
pci_set_drvdata(pdev, adap_info);
--
2.9.3
^ permalink raw reply related
* RESEND:i2c-eg20t: fix race between i2c init and interrupt enable
From: Yadi Hu @ 2016-09-07 9:25 UTC (permalink / raw)
To: yadi.hu, wsa, jdelvare; +Cc: linux-i2c
the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, isassigned an effective value.
it is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others. since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.
At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.
Bad IO access at port 0x18 (return inl(port))
Call Trace:
[<c102c733>] warn_slowpath_common+0x73/0xa0
[<c13109f5>] ? bad_io_access+0x45/0x50
[<c13109f5>] ? bad_io_access+0x45/0x50
[<c102c804>] warn_slowpath_fmt+0x34/0x40
[<c13109f5>] bad_io_access+0x45/0x50
[<c1310b62>] ioread32+0x22/0x40
[<c14c60db>] pch_i2c_handler+0x3b/0x120
[<c1092f34>] handle_irq_event_percpu+0x64/0x330
[<c109323b>] handle_irq_event+0x3b/0x60
[<c1095b50>] ? unmask_irq+0x30/0x30
[<c1095ba0>] handle_fasteoi_irq+0x50/0xe0
<IRQ> [<c16e7e78>] ? do_IRQ+0x48/0xc0
[<c16e7f6f>] ? smp_apic_timer_interrupt+0x7f/0x17a
[<c16e7da9>] ? common_interrupt+0x29/0x30
[<c1008ebb>] ? mwait_idle+0x8b/0x2c0
[<c1009e8e>] ? cpu_idle+0x9e/0xc0
[<c16be408>] ? rest_init+0x6c/0x74
[<c19f770a>] ? start_kernel+0x311/0x318
[<c19f7231>] ? repair_env_string+0x51/0x51
[<c19f7078>] ? i386_start_kernel+0x78/0x7d
---[ end trace eb3a1028f468a140 ]---
i2c_eg20t 0000:05:0c.2: pch_i2c_handler :I2C-3 mode(0) is not supported
after testing last patch, replacing request_irq() location is not a
good idea,it might produce a little time windows ,in which the interrupts
occurring inside will be omitted.
the new patch adds a check point on interrupt handler in case field
'pch_base_address' has not been initialed.
Yadi Hu (1)
i2c-eg20t: fix race between i2c init and interrupt enable
drivers/i2c/busses/i2c-eg20t.c | 5 +++++
1 file changed, 5 insertions(+)
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Uwe Kleine-König @ 2016-09-07 5:55 UTC (permalink / raw)
To: Tracy Smith
Cc: Leo Li, Stefan Agner, Gao Pan, Wolfram Sang, lkml, Li Yang,
linux-gpio, linux-i2c@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAChUvXOGjTn6K5588UVgjbRWUY+weDtk2qbOU-NM4=kA2znh=Q@mail.gmail.com>
Hello,
On Tue, Sep 06, 2016 at 06:35:38PM -0500, Tracy Smith wrote:
> >The patch makes it really optional that
> >the probe function won't bailout but just disable the bus recovery function
> >when pinctrl is not available.
>
> in the case of the LS1043A and LS1021A, if the bus recovery function is
> disabled, how will bus recovery occur? Is there no option to recover the
> bus for the LS1021a and LS1043a?
Right. That's the situation that we had for all drivers some time ago.
Most of the time this is no problem.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* RE: [v11, 7/8] base: soc: introduce soc_device_match() interface
From: Y.B. Lu @ 2016-09-07 4:10 UTC (permalink / raw)
To: Arnd Bergmann, Ulf Hansson
Cc: linux-mmc, Scott Wood, linuxppc-dev@lists.ozlabs.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-clk,
linux-i2c@vger.kernel.org, iommu@lists.linux-foundation.org,
netdev@vger.kernel.org, Mark Rutland, Rob Herring, Russell King,
Jochen Friedrich, Joerg Roedel, Claudiu Manoil
In-Reply-To: <6515108.aCEls7dUuW@wuerfel>
Hi Anrd and Uffe,
Thank you for your comment.
Please see my comment inline.
Best regards,
Yangbo Lu
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, September 06, 2016 8:46 PM
> To: Ulf Hansson
> Cc: Y.B. Lu; linux-mmc; Scott Wood; linuxppc-dev@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-clk; linux-i2c@vger.kernel.org;
> iommu@lists.linux-foundation.org; netdev@vger.kernel.org; Mark Rutland;
> Rob Herring; Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil;
> Bhupesh Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B.
> Xie
> Subject: Re: [v11, 7/8] base: soc: introduce soc_device_match() interface
>
> On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote:
> > On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > We keep running into cases where device drivers want to know the
> > > exact version of the a SoC they are currently running on. In the
> > > past, this has usually been done through a vendor specific API that
> > > can be called by a driver, or by directly accessing some kind of
> > > version register that is not part of the device itself but that
> > > belongs to a global register area of the chip.
>
> Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to
> preserve authorship. If you use "git send-email" or "git format-patch",
> that should happen automatically if the author field is set right (if not,
> use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"'
> to fix it).
>
[Lu Yangbo-B47093] Oh, I'm sorry for my careless. Will correct it in next version.
> > > +
> > > +/*
> > > + * soc_device_match - identify the SoC in the machine
> > > + * @matches: zero-terminated array of possible matches
> >
> > Perhaps also express the constraint on the matching entries. As you
> > need at least one of the ->machine(), ->family(), ->revision() or
> > ->soc_id() callbacks implemented, right!?
>
> They are not callbacks, just strings. Having an empty entry indicates the
> end of the array, and this is not called.
>
> > > + *
> > > + * returns the first matching entry of the argument array, or NULL
> > > + * if none of them match.
> > > + *
> > > + * This function is meant as a helper in place of of_match_node()
> > > + * in cases where either no device tree is available or the
> > > + information
> > > + * in a device node is insufficient to identify a particular
> > > + variant
> > > + * by its compatible strings or other properties. For new devices,
> > > + * the DT binding should always provide unique compatible strings
> > > + * that allow the use of of_match_node() instead.
> > > + *
> > > + * The calling function can use the .data entry of the
> > > + * soc_device_attribute to pass a structure or function pointer for
> > > + * each entry.
> >
> > I don't get the use case behind this, could you elaborate?
> >
> > Perhaps we should postpone adding the .data entry until we actually
> > see a need for it?
>
> I think the interface is rather useless without a way to figure out which
> entry you got. Almost all users of of_match_node() actually use the
> returned ->data field, and I expect this to be the same here.
>
> > > + */
> > > +const struct soc_device_attribute *soc_device_match(
> > > + const struct soc_device_attribute *matches) {
> > > + struct device *dev;
> > > + int ret;
> > > +
> > > + for (ret = 0; ret == 0; matches++) {
> >
> > This loop looks a bit weird and unsafe.
>
> Ah, and I thought I was being clever ;-)
>
> > 1) Perhaps using a while loop makes this more readable?
> > 2) As this is an exported API, I guess validation of the ->matches
> > pointer needs to be done before accessing it.
>
> Sounds fine.
[Lu Yangbo-B47093] Ok, Will change this according to Uffe.
And actually there is issue with this for() when I verified it again this morning.
We will get matches++ rather than matches which is correct finally :)
>
> > > + if (!(matches->machine || matches->family ||
> > > + matches->revision || matches->soc_id))
> > > + return NULL;
> > > + dev = NULL;
> >
> > There's no need to use a struct device just to assign it to NULL.
> > Instead just provide the function below with NULL.
> >
> > > + ret = bus_for_each_dev(&soc_bus_type, dev, (void
> *)matches,
> > > + soc_device_match_one);
>
>
> I don't remember what led to this, I think you are right, we should just
> pass NULL as most other callers.
[Lu Yangbo-B47093] Will correct it. Thanks. :)
>
> Thanks for the review.
>
> ARnd
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Leo Li @ 2016-09-06 23:14 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Li Yang, Gao Pan, Wolfram Sang, lkml, linux-gpio,
linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20160906210735.6wl5x2mdhnhdnh7y@pengutronix.de>
On Tue, Sep 6, 2016 at 4:07 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Sep 06, 2016 at 03:06:41PM -0500, Leo Li wrote:
>> On Tue, Sep 6, 2016 at 1:58 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
>> >> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
>> >> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
>> >> is not always available for platforms with this controller such as ls1021a
>> >> and ls1043a, and the device tree binding also mentioned this gpio based
>> >> recovery mechanism as optional. The patch makes it really optional that
>> >> the probe function won't bailout but just disable the bus recovery function
>> >> when pinctrl is not available.
>> >>
>> >> Signed-off-by: Li Yang <leoyang.li@nxp.com>
>> >> Cc: Gao Pan <pandy.gao@nxp.com>
>> >> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> >> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> >> ---
>> >> v5:
>> >> Revert the last minute change of recovery info initialization timing, it
>> >> will cause problem if initialized after i2c_add_numbered_adapter()
>> >>
>> >> v4:
>> >> Remove the use of IS_ERR_OR_NULL
>> >> Move the condition judgement to i2c_imx_init_recovery_info()
>> >> Change the timing of recovery initialization to be after bus registration
>> >>
>> >> v3:
>> >> Rebased to Wolfram's for-next branch
>> >> Added acked-by from Linus Walleij
>> >> Update to use new nxp email addresses due to company merge
>> >>
>> >> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
>> >> 1 file changed, 14 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> >> index 1844bc9..7ae7992 100644
>> >> --- a/drivers/i2c/busses/i2c-imx.c
>> >> +++ b/drivers/i2c/busses/i2c-imx.c
>> >> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>> >> {
>> >> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>> >>
>> >> + /* if pinctrl is not supported on the system */
>> >> + if (IS_ERR(i2c_imx->pinctrl))
>> >> + i2c_imx->pinctrl = NULL;
>> >> +
>> >> + if (!i2c_imx->pinctrl) {
>> >> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>> >> + return;
>> >> + }
>> >> +
>> >> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
>> >> PINCTRL_STATE_DEFAULT);
>> >> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
>> >> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
>> >> return ret;
>> >> }
>> >>
>> >> + /* optional bus recovery feature through pinctrl */
>> >> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
>> >> - if (IS_ERR(i2c_imx->pinctrl)) {
>> >> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
>> >> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
>> >> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
>> >> ret = PTR_ERR(i2c_imx->pinctrl);
>> >> goto clk_disable;
>> >> }
>> >
>> > devm_pinctrl_get might return the following error-valued pointers:
>> > - -EINVAL
>> > - -ENOMEM
>> > - -ENODEV
>> > - -EPROBE_DEFER
>> >
>> > There are several error paths returning -EINVAL, one is when an invalid
>> > phandle is used. Do you really want to ignore that?
>> >
>> > IMO error handling is better done with inverse logic, that is continue
>> > on some explicit error, bail out on all unknown stuff. This tends to be
>> > more robust. Also the comment should be improved to not explain that for
>> > -ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
>> > anyone who can read C) but to explain why.
>>
>> What you said is true for normal error handling, but in this scenario
>> it is intentional to ignore all pinctrl related errors except critical
>> ones because failing to have pinctrl for an optional feature shouldn't
>> impact the function of normal i2c. We choose to catch -ENOMEM because
>> the error could also cause problem for i2c probe, and -EPROBE_DEFER
>> because it's possible that the pinctrl will be ready later and we want
>> to give it a chance. The i2c driver really don't care why the pinctrl
>> was not usable. I thought I added comment before the
>> devm_pinctrl_get() to explain that the pinctrl is optional. Hopefully
>> you are not suggesting to explain what -ENOMEM and -EPROBE_DEFER mean
>> in the comment. :)
>
> You wrote
>
> /* optional bus recovery feature through pinctrl */
>
> there. I'd expect to read something like:
>
> /*
> * If pinctrl is available it might be possible to switch the
> * SDA and SCL lines to their GPIO functions and do a recovery
> * procedure in this mode. If there is anything wrong with
> * pinctrl we cannot do this and simply skip it.
> */
Thanks. I will use this. This does looks more clear. And more verbose too. :)
>
>> > Also I'd put
>> >
>> > i2c_imx->pinctrl = NULL;
>> >
>> > directly after devm_pinctrl_get() which I consider the more obvious
>> > place for this.
>>
>> On the other hand, put it together with the actually judgement can
>> make it clear that it is catching both IS_ERR and NULL returns from
>> devm_pinctrl_get()?
>
> When you do it immediately, you have:
>
> if (i2c_imx->pinctrl)
> use_it();
> else
> ignore_it();
>
But you suggested the other way last time "Or maybe make
i2c_imx_init_recovery_info aware of this situation to keep the caller
simple?" :) Either way I think it is just personal preferences with
both subtle pros and cons. I don't think we should spend too much
time on this.
Regards,
Leo
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Leo Li @ 2016-09-06 22:40 UTC (permalink / raw)
To: Stefan Agner
Cc: Uwe Kleine-König, Gao Pan, Wolfram Sang, lkml, Li Yang,
linux-gpio, linux-i2c@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <87ca69fb85848a0a86b34a9a9037a57b@agner.ch>
On Tue, Sep 6, 2016 at 4:51 PM, Stefan Agner <stefan@agner.ch> wrote:
> On 2016-09-06 13:06, Leo Li wrote:
>> On Tue, Sep 6, 2016 at 1:58 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>>> On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
>>>> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
>>>> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
>>>> is not always available for platforms with this controller such as ls1021a
>>>> and ls1043a, and the device tree binding also mentioned this gpio based
>>>> recovery mechanism as optional. The patch makes it really optional that
>>>> the probe function won't bailout but just disable the bus recovery function
>>>> when pinctrl is not available.
>>>>
>>>> Signed-off-by: Li Yang <leoyang.li@nxp.com>
>>>> Cc: Gao Pan <pandy.gao@nxp.com>
>>>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>>> ---
>>>> v5:
>>>> Revert the last minute change of recovery info initialization timing, it
>>>> will cause problem if initialized after i2c_add_numbered_adapter()
>>>>
>>>> v4:
>>>> Remove the use of IS_ERR_OR_NULL
>>>> Move the condition judgement to i2c_imx_init_recovery_info()
>>>> Change the timing of recovery initialization to be after bus registration
>>>>
>>>> v3:
>>>> Rebased to Wolfram's for-next branch
>>>> Added acked-by from Linus Walleij
>>>> Update to use new nxp email addresses due to company merge
>>>>
>>>> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
>>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>>>> index 1844bc9..7ae7992 100644
>>>> --- a/drivers/i2c/busses/i2c-imx.c
>>>> +++ b/drivers/i2c/busses/i2c-imx.c
>>>> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>>>> {
>>>> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>>>>
>>>> + /* if pinctrl is not supported on the system */
>>>> + if (IS_ERR(i2c_imx->pinctrl))
>>>> + i2c_imx->pinctrl = NULL;
>>>> +
>>>> + if (!i2c_imx->pinctrl) {
>>>> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>>>> + return;
>>>> + }
>>>> +
>>>> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
>>>> PINCTRL_STATE_DEFAULT);
>>>> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
>>>> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
>>>> return ret;
>>>> }
>>>>
>>>> + /* optional bus recovery feature through pinctrl */
>>>> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
>>>> - if (IS_ERR(i2c_imx->pinctrl)) {
>>>> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
>>>> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
>>>> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
>>>> ret = PTR_ERR(i2c_imx->pinctrl);
>>>> goto clk_disable;
>>>> }
>>>
>>> devm_pinctrl_get might return the following error-valued pointers:
>>> - -EINVAL
>>> - -ENOMEM
>>> - -ENODEV
>>> - -EPROBE_DEFER
>>>
>>> There are several error paths returning -EINVAL, one is when an invalid
>>> phandle is used. Do you really want to ignore that?
>>>
>>> IMO error handling is better done with inverse logic, that is continue
>>> on some explicit error, bail out on all unknown stuff. This tends to be
>>> more robust. Also the comment should be improved to not explain that for
>>> -ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
>>> anyone who can read C) but to explain why.
>>
>> What you said is true for normal error handling, but in this scenario
>> it is intentional to ignore all pinctrl related errors except critical
>> ones because failing to have pinctrl for an optional feature shouldn't
>> impact the function of normal i2c. We choose to catch -ENOMEM because
>> the error could also cause problem for i2c probe, and -EPROBE_DEFER
>> because it's possible that the pinctrl will be ready later and we want
>> to give it a chance. The i2c driver really don't care why the pinctrl
>> was not usable. I thought I added comment before the
>
> I don't agree. E.g. -EINVAL would appear if you pass devm_pinctrl_get an
> invalid device. Currently you would silently ignore that, which is not
> what you want.
It is not silently ignored, there will be a message printed out saying
pinctrl is not available and bus recovery is not supported. On the
contrary, without this change the entire i2c driver fails to work
silently if pinctrl is somehow not working. And if the system is so
broken that the pointer to the i2c device is NULL, the probe of i2c
would have already failed before this point. We shouldn't count on an
optional function of the driver to catch fundamental issues like this.
>
> You want to get the pinctrl in any case expect there isn't one. And that
> is how you should formulate your if statement.
>
> /*
> * It is ok if no pinctrl device is available. We'll not be able to use
> the
> * bus recovery feature, but otherwise the driver works fine...
> */
> if (PTR_ERR(i2c_imx->pinctrl) != -ENODEV)
I agree that there could be other possibilities that the pinctrl
failed to work beside the reason I described in the commit
message(platform doesn't support pinctrl at all). But I don't think
any of them other than the -ENOMEM and -EPROBE_DEFER deserves a bail
out for the entire i2c driver.
Regards,
Leo
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Stefan Agner @ 2016-09-06 21:51 UTC (permalink / raw)
To: Leo Li
Cc: Uwe Kleine-König, Gao Pan, Wolfram Sang, lkml, Li Yang,
linux-gpio, linux-i2c, linux-arm-kernel
In-Reply-To: <CADRPPNQ+1qBNL5WA=Jo_keE7BPnoOs7a2T6CpTuciiF16qK6xQ@mail.gmail.com>
On 2016-09-06 13:06, Leo Li wrote:
> On Tue, Sep 6, 2016 at 1:58 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
>>> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
>>> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
>>> is not always available for platforms with this controller such as ls1021a
>>> and ls1043a, and the device tree binding also mentioned this gpio based
>>> recovery mechanism as optional. The patch makes it really optional that
>>> the probe function won't bailout but just disable the bus recovery function
>>> when pinctrl is not available.
>>>
>>> Signed-off-by: Li Yang <leoyang.li@nxp.com>
>>> Cc: Gao Pan <pandy.gao@nxp.com>
>>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>> v5:
>>> Revert the last minute change of recovery info initialization timing, it
>>> will cause problem if initialized after i2c_add_numbered_adapter()
>>>
>>> v4:
>>> Remove the use of IS_ERR_OR_NULL
>>> Move the condition judgement to i2c_imx_init_recovery_info()
>>> Change the timing of recovery initialization to be after bus registration
>>>
>>> v3:
>>> Rebased to Wolfram's for-next branch
>>> Added acked-by from Linus Walleij
>>> Update to use new nxp email addresses due to company merge
>>>
>>> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>>> index 1844bc9..7ae7992 100644
>>> --- a/drivers/i2c/busses/i2c-imx.c
>>> +++ b/drivers/i2c/busses/i2c-imx.c
>>> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>>> {
>>> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>>>
>>> + /* if pinctrl is not supported on the system */
>>> + if (IS_ERR(i2c_imx->pinctrl))
>>> + i2c_imx->pinctrl = NULL;
>>> +
>>> + if (!i2c_imx->pinctrl) {
>>> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>>> + return;
>>> + }
>>> +
>>> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
>>> PINCTRL_STATE_DEFAULT);
>>> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
>>> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
>>> return ret;
>>> }
>>>
>>> + /* optional bus recovery feature through pinctrl */
>>> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> - if (IS_ERR(i2c_imx->pinctrl)) {
>>> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
>>> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
>>> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
>>> ret = PTR_ERR(i2c_imx->pinctrl);
>>> goto clk_disable;
>>> }
>>
>> devm_pinctrl_get might return the following error-valued pointers:
>> - -EINVAL
>> - -ENOMEM
>> - -ENODEV
>> - -EPROBE_DEFER
>>
>> There are several error paths returning -EINVAL, one is when an invalid
>> phandle is used. Do you really want to ignore that?
>>
>> IMO error handling is better done with inverse logic, that is continue
>> on some explicit error, bail out on all unknown stuff. This tends to be
>> more robust. Also the comment should be improved to not explain that for
>> -ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
>> anyone who can read C) but to explain why.
>
> What you said is true for normal error handling, but in this scenario
> it is intentional to ignore all pinctrl related errors except critical
> ones because failing to have pinctrl for an optional feature shouldn't
> impact the function of normal i2c. We choose to catch -ENOMEM because
> the error could also cause problem for i2c probe, and -EPROBE_DEFER
> because it's possible that the pinctrl will be ready later and we want
> to give it a chance. The i2c driver really don't care why the pinctrl
> was not usable. I thought I added comment before the
I don't agree. E.g. -EINVAL would appear if you pass devm_pinctrl_get an
invalid device. Currently you would silently ignore that, which is not
what you want.
You want to get the pinctrl in any case expect there isn't one. And that
is how you should formulate your if statement.
/*
* It is ok if no pinctrl device is available. We'll not be able to use
the
* bus recovery feature, but otherwise the driver works fine...
*/
if (PTR_ERR(i2c_imx->pinctrl) != -ENODEV)
...
--
Stefan
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Uwe Kleine-König @ 2016-09-06 21:07 UTC (permalink / raw)
To: Leo Li
Cc: Li Yang, Gao Pan, Wolfram Sang, lkml, linux-gpio,
linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CADRPPNQ+1qBNL5WA=Jo_keE7BPnoOs7a2T6CpTuciiF16qK6xQ@mail.gmail.com>
On Tue, Sep 06, 2016 at 03:06:41PM -0500, Leo Li wrote:
> On Tue, Sep 6, 2016 at 1:58 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
> >> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
> >> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
> >> is not always available for platforms with this controller such as ls1021a
> >> and ls1043a, and the device tree binding also mentioned this gpio based
> >> recovery mechanism as optional. The patch makes it really optional that
> >> the probe function won't bailout but just disable the bus recovery function
> >> when pinctrl is not available.
> >>
> >> Signed-off-by: Li Yang <leoyang.li@nxp.com>
> >> Cc: Gao Pan <pandy.gao@nxp.com>
> >> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >> ---
> >> v5:
> >> Revert the last minute change of recovery info initialization timing, it
> >> will cause problem if initialized after i2c_add_numbered_adapter()
> >>
> >> v4:
> >> Remove the use of IS_ERR_OR_NULL
> >> Move the condition judgement to i2c_imx_init_recovery_info()
> >> Change the timing of recovery initialization to be after bus registration
> >>
> >> v3:
> >> Rebased to Wolfram's for-next branch
> >> Added acked-by from Linus Walleij
> >> Update to use new nxp email addresses due to company merge
> >>
> >> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
> >> 1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> >> index 1844bc9..7ae7992 100644
> >> --- a/drivers/i2c/busses/i2c-imx.c
> >> +++ b/drivers/i2c/busses/i2c-imx.c
> >> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
> >> {
> >> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
> >>
> >> + /* if pinctrl is not supported on the system */
> >> + if (IS_ERR(i2c_imx->pinctrl))
> >> + i2c_imx->pinctrl = NULL;
> >> +
> >> + if (!i2c_imx->pinctrl) {
> >> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> >> + return;
> >> + }
> >> +
> >> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
> >> PINCTRL_STATE_DEFAULT);
> >> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> >> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
> >> return ret;
> >> }
> >>
> >> + /* optional bus recovery feature through pinctrl */
> >> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> >> - if (IS_ERR(i2c_imx->pinctrl)) {
> >> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
> >> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
> >> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
> >> ret = PTR_ERR(i2c_imx->pinctrl);
> >> goto clk_disable;
> >> }
> >
> > devm_pinctrl_get might return the following error-valued pointers:
> > - -EINVAL
> > - -ENOMEM
> > - -ENODEV
> > - -EPROBE_DEFER
> >
> > There are several error paths returning -EINVAL, one is when an invalid
> > phandle is used. Do you really want to ignore that?
> >
> > IMO error handling is better done with inverse logic, that is continue
> > on some explicit error, bail out on all unknown stuff. This tends to be
> > more robust. Also the comment should be improved to not explain that for
> > -ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
> > anyone who can read C) but to explain why.
>
> What you said is true for normal error handling, but in this scenario
> it is intentional to ignore all pinctrl related errors except critical
> ones because failing to have pinctrl for an optional feature shouldn't
> impact the function of normal i2c. We choose to catch -ENOMEM because
> the error could also cause problem for i2c probe, and -EPROBE_DEFER
> because it's possible that the pinctrl will be ready later and we want
> to give it a chance. The i2c driver really don't care why the pinctrl
> was not usable. I thought I added comment before the
> devm_pinctrl_get() to explain that the pinctrl is optional. Hopefully
> you are not suggesting to explain what -ENOMEM and -EPROBE_DEFER mean
> in the comment. :)
You wrote
/* optional bus recovery feature through pinctrl */
there. I'd expect to read something like:
/*
* If pinctrl is available it might be possible to switch the
* SDA and SCL lines to their GPIO functions and do a recovery
* procedure in this mode. If there is anything wrong with
* pinctrl we cannot do this and simply skip it.
*/
> > Also I'd put
> >
> > i2c_imx->pinctrl = NULL;
> >
> > directly after devm_pinctrl_get() which I consider the more obvious
> > place for this.
>
> On the other hand, put it together with the actually judgement can
> make it clear that it is catching both IS_ERR and NULL returns from
> devm_pinctrl_get()?
When you do it immediately, you have:
if (i2c_imx->pinctrl)
use_it();
else
ignore_it();
which looks clean. And if later another piece of code is added that
works on the pinctrl value, you don't have to care about the order (or
set it to NULL at two places). Instead you can consider the value const
in the complete driver apart from the little snippet that assigns to it
in .probe().
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Leo Li @ 2016-09-06 20:06 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Li Yang, Gao Pan, Wolfram Sang, lkml, linux-gpio,
linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20160906185849.27pjhawadca5pwow@pengutronix.de>
On Tue, Sep 6, 2016 at 1:58 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
>> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
>> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
>> is not always available for platforms with this controller such as ls1021a
>> and ls1043a, and the device tree binding also mentioned this gpio based
>> recovery mechanism as optional. The patch makes it really optional that
>> the probe function won't bailout but just disable the bus recovery function
>> when pinctrl is not available.
>>
>> Signed-off-by: Li Yang <leoyang.li@nxp.com>
>> Cc: Gao Pan <pandy.gao@nxp.com>
>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> v5:
>> Revert the last minute change of recovery info initialization timing, it
>> will cause problem if initialized after i2c_add_numbered_adapter()
>>
>> v4:
>> Remove the use of IS_ERR_OR_NULL
>> Move the condition judgement to i2c_imx_init_recovery_info()
>> Change the timing of recovery initialization to be after bus registration
>>
>> v3:
>> Rebased to Wolfram's for-next branch
>> Added acked-by from Linus Walleij
>> Update to use new nxp email addresses due to company merge
>>
>> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> index 1844bc9..7ae7992 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>> {
>> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>>
>> + /* if pinctrl is not supported on the system */
>> + if (IS_ERR(i2c_imx->pinctrl))
>> + i2c_imx->pinctrl = NULL;
>> +
>> + if (!i2c_imx->pinctrl) {
>> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>> + return;
>> + }
>> +
>> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
>> PINCTRL_STATE_DEFAULT);
>> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
>> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> + /* optional bus recovery feature through pinctrl */
>> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
>> - if (IS_ERR(i2c_imx->pinctrl)) {
>> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
>> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
>> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
>> ret = PTR_ERR(i2c_imx->pinctrl);
>> goto clk_disable;
>> }
>
> devm_pinctrl_get might return the following error-valued pointers:
> - -EINVAL
> - -ENOMEM
> - -ENODEV
> - -EPROBE_DEFER
>
> There are several error paths returning -EINVAL, one is when an invalid
> phandle is used. Do you really want to ignore that?
>
> IMO error handling is better done with inverse logic, that is continue
> on some explicit error, bail out on all unknown stuff. This tends to be
> more robust. Also the comment should be improved to not explain that for
> -ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
> anyone who can read C) but to explain why.
What you said is true for normal error handling, but in this scenario
it is intentional to ignore all pinctrl related errors except critical
ones because failing to have pinctrl for an optional feature shouldn't
impact the function of normal i2c. We choose to catch -ENOMEM because
the error could also cause problem for i2c probe, and -EPROBE_DEFER
because it's possible that the pinctrl will be ready later and we want
to give it a chance. The i2c driver really don't care why the pinctrl
was not usable. I thought I added comment before the
devm_pinctrl_get() to explain that the pinctrl is optional. Hopefully
you are not suggesting to explain what -ENOMEM and -EPROBE_DEFER mean
in the comment. :)
>
> Also I'd put
>
> i2c_imx->pinctrl = NULL;
>
> directly after devm_pinctrl_get() which I consider the more obvious
> place for this.
On the other hand, put it together with the actually judgement can
make it clear that it is catching both IS_ERR and NULL returns from
devm_pinctrl_get()?
--
- Leo
^ permalink raw reply
* Re: [PATCH 1/3] i2c: Add class for LEGO MINDSTORMS sensors
From: Uwe Kleine-König @ 2016-09-06 19:01 UTC (permalink / raw)
To: David Lechner; +Cc: Wolfram Sang, linux-i2c, linux-kernel
In-Reply-To: <1473108014-30787-2-git-send-email-david@lechnology.com>
Hello David,
On Mon, Sep 05, 2016 at 03:40:12PM -0500, David Lechner wrote:
> that use I2C communications. These sensors have a well-know register
s/w/wn/
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional
From: Uwe Kleine-König @ 2016-09-06 18:58 UTC (permalink / raw)
To: Li Yang
Cc: Gao Pan, Wolfram Sang, linux-kernel, linux-gpio, linux-i2c,
linux-arm-kernel
In-Reply-To: <1471644322-22670-1-git-send-email-leoyang.li@nxp.com>
On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
> is not always available for platforms with this controller such as ls1021a
> and ls1043a, and the device tree binding also mentioned this gpio based
> recovery mechanism as optional. The patch makes it really optional that
> the probe function won't bailout but just disable the bus recovery function
> when pinctrl is not available.
>
> Signed-off-by: Li Yang <leoyang.li@nxp.com>
> Cc: Gao Pan <pandy.gao@nxp.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> v5:
> Revert the last minute change of recovery info initialization timing, it
> will cause problem if initialized after i2c_add_numbered_adapter()
>
> v4:
> Remove the use of IS_ERR_OR_NULL
> Move the condition judgement to i2c_imx_init_recovery_info()
> Change the timing of recovery initialization to be after bus registration
>
> v3:
> Rebased to Wolfram's for-next branch
> Added acked-by from Linus Walleij
> Update to use new nxp email addresses due to company merge
>
> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 1844bc9..7ae7992 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
> {
> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>
> + /* if pinctrl is not supported on the system */
> + if (IS_ERR(i2c_imx->pinctrl))
> + i2c_imx->pinctrl = NULL;
> +
> + if (!i2c_imx->pinctrl) {
> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> + return;
> + }
> +
> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
> PINCTRL_STATE_DEFAULT);
> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
> return ret;
> }
>
> + /* optional bus recovery feature through pinctrl */
> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> - if (IS_ERR(i2c_imx->pinctrl)) {
> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
> ret = PTR_ERR(i2c_imx->pinctrl);
> goto clk_disable;
> }
devm_pinctrl_get might return the following error-valued pointers:
- -EINVAL
- -ENOMEM
- -ENODEV
- -EPROBE_DEFER
There are several error paths returning -EINVAL, one is when an invalid
phandle is used. Do you really want to ignore that?
IMO error handling is better done with inverse logic, that is continue
on some explicit error, bail out on all unknown stuff. This tends to be
more robust. Also the comment should be improved to not explain that for
-ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
anyone who can read C) but to explain why.
Also I'd put
i2c_imx->pinctrl = NULL;
directly after devm_pinctrl_get() which I consider the more obvious
place for this.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [v11, 7/8] base: soc: introduce soc_device_match() interface
From: Arnd Bergmann @ 2016-09-06 12:46 UTC (permalink / raw)
To: Ulf Hansson
Cc: Yangbo Lu, linux-mmc, Scott Wood,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
netdev-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Rob Herring,
Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil,
Bhupesh
In-Reply-To: <CAPDyKFrvg3Qr5m3xXvywsCoVvaiF7ggHAj0Aw=WCOXg65Pt0PQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote:
> On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu-3arQi8VN3Tc@public.gmane.org> wrote:
> > We keep running into cases where device drivers want to know the exact
> > version of the a SoC they are currently running on. In the past, this has
> > usually been done through a vendor specific API that can be called by a
> > driver, or by directly accessing some kind of version register that is
> > not part of the device itself but that belongs to a global register area
> > of the chip.
Please add "From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>" as the first line, to
preserve authorship. If you use "git send-email" or "git format-patch",
that should happen automatically if the author field is set right
(if not, use 'git commit --amend --author="Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>"'
to fix it).
> > +
> > +/*
> > + * soc_device_match - identify the SoC in the machine
> > + * @matches: zero-terminated array of possible matches
>
> Perhaps also express the constraint on the matching entries. As you
> need at least one of the ->machine(), ->family(), ->revision() or
> ->soc_id() callbacks implemented, right!?
They are not callbacks, just strings. Having an empty entry indicates
the end of the array, and this is not called.
> > + *
> > + * returns the first matching entry of the argument array, or NULL
> > + * if none of them match.
> > + *
> > + * This function is meant as a helper in place of of_match_node()
> > + * in cases where either no device tree is available or the information
> > + * in a device node is insufficient to identify a particular variant
> > + * by its compatible strings or other properties. For new devices,
> > + * the DT binding should always provide unique compatible strings
> > + * that allow the use of of_match_node() instead.
> > + *
> > + * The calling function can use the .data entry of the
> > + * soc_device_attribute to pass a structure or function pointer for
> > + * each entry.
>
> I don't get the use case behind this, could you elaborate?
>
> Perhaps we should postpone adding the .data entry until we actually
> see a need for it?
I think the interface is rather useless without a way to figure
out which entry you got. Almost all users of of_match_node()
actually use the returned ->data field, and I expect this to
be the same here.
> > + */
> > +const struct soc_device_attribute *soc_device_match(
> > + const struct soc_device_attribute *matches)
> > +{
> > + struct device *dev;
> > + int ret;
> > +
> > + for (ret = 0; ret == 0; matches++) {
>
> This loop looks a bit weird and unsafe.
Ah, and I thought I was being clever ;-)
> 1) Perhaps using a while loop makes this more readable?
> 2) As this is an exported API, I guess validation of the ->matches
> pointer needs to be done before accessing it.
Sounds fine.
> > + if (!(matches->machine || matches->family ||
> > + matches->revision || matches->soc_id))
> > + return NULL;
> > + dev = NULL;
>
> There's no need to use a struct device just to assign it to NULL.
> Instead just provide the function below with NULL.
>
> > + ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
> > + soc_device_match_one);
I don't remember what led to this, I think you are right, we should
just pass NULL as most other callers.
Thanks for the review.
ARnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v11, 7/8] base: soc: introduce soc_device_match() interface
From: Ulf Hansson @ 2016-09-06 11:44 UTC (permalink / raw)
To: Yangbo Lu
Cc: linux-mmc, Scott Wood, Arnd Bergmann,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
netdev-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Rob Herring,
Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil,
Bhupesh Sharma <bhupe>
In-Reply-To: <1473150503-9550-8-git-send-email-yangbo.lu-3arQi8VN3Tc@public.gmane.org>
On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu-3arQi8VN3Tc@public.gmane.org> wrote:
> We keep running into cases where device drivers want to know the exact
> version of the a SoC they are currently running on. In the past, this has
> usually been done through a vendor specific API that can be called by a
> driver, or by directly accessing some kind of version register that is
> not part of the device itself but that belongs to a global register area
> of the chip.
>
> Common reasons for doing this include:
>
> - A machine is not using devicetree or similar for passing data about
> on-chip devices, but just announces their presence using boot-time
> platform devices, and the machine code itself does not care about the
> revision.
>
> - There is existing firmware or boot loaders with existing DT binaries
> with generic compatible strings that do not identify the particular
> revision of each device, but the driver knows which SoC revisions
> include which part.
>
> - A prerelease version of a chip has some quirks and we are using the same
> version of the bootloader and the DT blob on both the prerelease and the
> final version. An update of the DT binding seems inappropriate because
> that would involve maintaining multiple copies of the dts and/or
> bootloader.
>
> This patch introduces the soc_device_match() interface that is meant to
> work like of_match_node() but instead of identifying the version of a
> device, it identifies the SoC itself using a vendor-agnostic interface.
>
> Unlike of_match_node(), we do not do an exact string compare but instead
> use glob_match() to allow wildcards in strings.
Overall, this change make sense to me, although some minor comment below.
>
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Signed-off-by: Yangbo Lu <yangbo.lu-3arQi8VN3Tc@public.gmane.org>
> ---
> Changes for v11:
> - Added this patch for soc match
> ---
> drivers/base/Kconfig | 1 +
> drivers/base/soc.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/sys_soc.h | 3 +++
> 3 files changed, 65 insertions(+)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 98504ec..f1591ad2 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE
>
> config SOC_BUS
> bool
> + select GLOB
>
> source "drivers/base/regmap/Kconfig"
>
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 75b98aa..5c4e84a 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -14,6 +14,7 @@
> #include <linux/spinlock.h>
> #include <linux/sys_soc.h>
> #include <linux/err.h>
> +#include <linux/glob.h>
>
> static DEFINE_IDA(soc_ida);
>
> @@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void)
> bus_unregister(&soc_bus_type);
> }
> module_exit(soc_bus_unregister);
> +
> +static int soc_device_match_one(struct device *dev, void *arg)
> +{
> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> + const struct soc_device_attribute *match = arg;
> +
> + if (match->machine &&
> + !glob_match(match->machine, soc_dev->attr->machine))
> + return 0;
> +
> + if (match->family &&
> + !glob_match(match->family, soc_dev->attr->family))
> + return 0;
> +
> + if (match->revision &&
> + !glob_match(match->revision, soc_dev->attr->revision))
> + return 0;
> +
> + if (match->soc_id &&
> + !glob_match(match->soc_id, soc_dev->attr->soc_id))
> + return 0;
> +
> + return 1;
> +}
> +
> +/*
> + * soc_device_match - identify the SoC in the machine
> + * @matches: zero-terminated array of possible matches
Perhaps also express the constraint on the matching entries. As you
need at least one of the ->machine(), ->family(), ->revision() or
->soc_id() callbacks implemented, right!?
> + *
> + * returns the first matching entry of the argument array, or NULL
> + * if none of them match.
> + *
> + * This function is meant as a helper in place of of_match_node()
> + * in cases where either no device tree is available or the information
> + * in a device node is insufficient to identify a particular variant
> + * by its compatible strings or other properties. For new devices,
> + * the DT binding should always provide unique compatible strings
> + * that allow the use of of_match_node() instead.
> + *
> + * The calling function can use the .data entry of the
> + * soc_device_attribute to pass a structure or function pointer for
> + * each entry.
I don't get the use case behind this, could you elaborate?
Perhaps we should postpone adding the .data entry until we actually
see a need for it?
> + */
> +const struct soc_device_attribute *soc_device_match(
> + const struct soc_device_attribute *matches)
> +{
> + struct device *dev;
> + int ret;
> +
> + for (ret = 0; ret == 0; matches++) {
This loop looks a bit weird and unsafe.
1) Perhaps using a while loop makes this more readable?
2) As this is an exported API, I guess validation of the ->matches
pointer needs to be done before accessing it.
> + if (!(matches->machine || matches->family ||
> + matches->revision || matches->soc_id))
> + return NULL;
> + dev = NULL;
There's no need to use a struct device just to assign it to NULL.
Instead just provide the function below with NULL.
> + ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
> + soc_device_match_one);
> + }
> + return matches;
> +}
> +EXPORT_SYMBOL_GPL(soc_device_match);
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> index 2739ccb..9f5eb06 100644
> --- a/include/linux/sys_soc.h
> +++ b/include/linux/sys_soc.h
> @@ -13,6 +13,7 @@ struct soc_device_attribute {
> const char *family;
> const char *revision;
> const char *soc_id;
> + const void *data;
> };
>
> /**
> @@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
> */
> struct device *soc_device_to_device(struct soc_device *soc);
>
> +const struct soc_device_attribute *soc_device_match(
> + const struct soc_device_attribute *matches);
> #endif /* __SOC_BUS_H */
> --
> 2.1.0.27.g96db324
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox