* Re: [PATCH v2 5/8] v4l: Switch from V4L2 OF not V4L2 fwnode API
From: Sakari Ailus @ 2017-04-07 10:58 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, linux-acpi, devicetree
In-Reply-To: <14918382.izlyCngq8n@avalon>
Hi Laurent,
On Fri, Apr 07, 2017 at 01:32:54PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thursday 06 Apr 2017 16:12:07 Sakari Ailus wrote:
> > Switch users of the v4l2_of_ APIs to the more generic v4l2_fwnode_ APIs.
> >
> > Existing OF matching continues to be supported. omap3isp and smiapp
> > drivers are converted to fwnode matching as well.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Benoit Parrot <bparrot@ti.com> # i2c/ov2569.c,
> > am437x/am437x-vpfe.c and ti-vpe/cal.c ---
> > drivers/media/i2c/Kconfig | 9 ++++
> > drivers/media/i2c/adv7604.c | 7 +--
> > drivers/media/i2c/mt9v032.c | 7 +--
> > drivers/media/i2c/ov2659.c | 8 +--
> > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 7 +--
> > drivers/media/i2c/s5k5baf.c | 6 +--
> > drivers/media/i2c/smiapp/Kconfig | 1 +
> > drivers/media/i2c/smiapp/smiapp-core.c | 29 ++++++-----
> > drivers/media/i2c/tc358743.c | 11 ++--
> > drivers/media/i2c/tvp514x.c | 6 +--
> > drivers/media/i2c/tvp5150.c | 7 +--
> > drivers/media/i2c/tvp7002.c | 6 +--
> > drivers/media/platform/Kconfig | 3 ++
> > drivers/media/platform/am437x/Kconfig | 1 +
> > drivers/media/platform/am437x/am437x-vpfe.c | 8 +--
> > drivers/media/platform/atmel/Kconfig | 1 +
> > drivers/media/platform/atmel/atmel-isc.c | 8 +--
> > drivers/media/platform/exynos4-is/Kconfig | 2 +
> > drivers/media/platform/exynos4-is/media-dev.c | 6 +--
> > drivers/media/platform/exynos4-is/mipi-csis.c | 6 +--
> > drivers/media/platform/omap3isp/isp.c | 71 +++++++++++-----------
> > drivers/media/platform/pxa_camera.c | 7 +--
> > drivers/media/platform/rcar-vin/Kconfig | 1 +
> > drivers/media/platform/rcar-vin/rcar-core.c | 6 +--
> > drivers/media/platform/soc_camera/Kconfig | 1 +
> > drivers/media/platform/soc_camera/atmel-isi.c | 7 +--
> > drivers/media/platform/soc_camera/soc_camera.c | 3 +-
> > drivers/media/platform/ti-vpe/cal.c | 11 ++--
> > drivers/media/platform/xilinx/Kconfig | 1 +
> > drivers/media/platform/xilinx/xilinx-vipp.c | 59 +++++++++++----------
> > include/media/v4l2-fwnode.h | 4 +-
> > 31 files changed, 176 insertions(+), 134 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index cee1dae..6b2423a 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -210,6 +210,7 @@ config VIDEO_ADV7604
> > depends on GPIOLIB || COMPILE_TEST
> > select HDMI
> > select MEDIA_CEC_EDID
> > + select V4L2_FWNODE
>
> What happens when building the driver on a platform that includes neither OF
> nor ACPI support ?
You need either in practice, also for the V4L2 fwnode to be meaningful.
Do you have something in particular in mind?
>
> > ---help---
> > Support for the Analog Devices ADV7604 video decoder.
> >
>
> [snip]
>
> How have you checked that you haven't missed any entry in the Kconfig files ?
I made one Kconfig change per driver. :-)
>
> [snip]
>
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 084ecf4a..95850b9 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
>
> [snip]
>
> > @@ -2024,43 +2025,42 @@ enum isp_of_phy {
> > ISP_OF_PHY_CSIPHY2,
> > };
> >
> > -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> > - struct isp_async_subdev *isd)
> > +static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> > + struct isp_async_subdev *isd)
> > {
> > struct isp_bus_cfg *buscfg = &isd->bus;
> > - struct v4l2_of_endpoint vep;
> > + struct v4l2_fwnode_endpoint vfwn;
>
> vfwn is confusing to me, I think the variable name should show that it refers
> to an endpoint.
How about adding ep to tell it's an endpoint?
>
> > unsigned int i;
> > int ret;
> >
> > - ret = v4l2_of_parse_endpoint(node, &vep);
> > + ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
> > if (ret)
> > return ret;
> >
> > - dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> > - vep.base.port);
> > + dev_dbg(dev, "interface %u\n", vfwn.base.port);
>
> Is there no way to keep the node name in the error message ?
There's no generic fwnode means to do something similar currently, possibly
because I understand ACPI doesn't do that. One could check whether the node
is an OF node and then use the full_name field but I wonder if it's worth
it.
>
>
> [snip]
>
> > @@ -2094,18 +2094,17 @@ static int isp_of_parse_node(struct device *dev,
> > struct device_node *node, break;
> >
> > default:
> > - dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
> > - vep.base.port);
> > + dev_warn(dev, "invalid interface %u\n", vfwn.base.port);
>
> Ditto.
>
> > break;
> > }
> >
> > return 0;
> > }
> >
> > -static int isp_of_parse_nodes(struct device *dev,
> > - struct v4l2_async_notifier *notifier)
> > +static int isp_fwnodes_parse(struct device *dev,
> > + struct v4l2_async_notifier *notifier)
> > {
> > - struct device_node *node = NULL;
> > + struct fwnode_handle *fwn = NULL;
>
> As explained in the review of another patch from the same series, I wouldn't
> rename the variable.
Most pointers to struct fwnode_handle are actually called fwnode and some
fw_node. fwn is just shorter. :-)
There are also cases pointers to struct device_node and struct fwnode_handle
are needed in the same function.
>
> > notifier->subdevs = devm_kcalloc(
> > dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
>
> [snip]
>
> > @@ -2219,12 +2220,12 @@ static int isp_probe(struct platform_device *pdev)
> > if (IS_ERR(isp->syscon))
> > return PTR_ERR(isp->syscon);
> >
> > - ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
> > - &isp->syscon_offset);
> > + ret = of_property_read_u32_index(pdev->dev.of_node,
> > + "syscon", 1, &isp->syscon_offset);
>
> This change doesn't seem to be needed.
Oh, indeed. Will fix.
>
> > if (ret)
> > return ret;
> >
> > - ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
> > + ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
> > if (ret < 0)
> > return ret;
> >
>
> [snip]
>
> > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> > b/drivers/media/platform/xilinx/xilinx-vipp.c index feb3b2f..6a2721b 100644
> > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
>
> [snip]
>
> > @@ -103,9 +103,10 @@ static int xvip_graph_build_one(struct
> > xvip_composite_device *xdev, * the link.
> > */
> > if (link.local_port >= local->num_pads) {
> > - dev_err(xdev->dev, "invalid port number %u on %s\n",
> > - link.local_port, link.local_node->full_name);
> > - v4l2_of_put_link(&link);
> > + dev_err(xdev->dev, "invalid port number %u for %s\n",
> > + link.local_port,
> > + to_of_node(link.local_node)->full_name);
>
> This makes me believe that we're missing a fwnode_full_name() function.
It'd be nice to have that, I agree. What should it do on non-OF nodes?
Return a pointers to an empty string?
>
> > + v4l2_fwnode_put_link(&link);
> > ret = -EINVAL;
> > break;
> > }
>
> [snip]
>
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index a675d8a..bc9cf51 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -17,10 +17,10 @@
> > #ifndef _V4L2_FWNODE_H
> > #define _V4L2_FWNODE_H
> >
> > +#include <linux/errno.h>
> > +#include <linux/fwnode.h>
> > #include <linux/list.h>
> > #include <linux/types.h>
> > -#include <linux/errno.h>
> > -#include <linux/of_graph.h>
> >
> > #include <media/v4l2-mediabus.h>
>
> This probably belongs to another patch (at least the alphabetical sorting
> does).
Yes, I'll change it.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH v4 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs
From: Lee Jones @ 2017-04-07 10:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Paul Cercueil, Alexandre Courbot, Rob Herring, Mark Rutland,
Ralf Baechle, Boris Brezillon, Thierry Reding,
Bartlomiej Zolnierkiewicz, Maarten ter Huurne, Lars-Peter Clausen,
Paul Burton, James Hogan, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Linux MIPS, linux-mmc@vger.kernel.org
In-Reply-To: <CACRpkdbeudhXDE8KEgB_kZSAUUwS4Sd3=+GyR0YZ2-PQnEa9zw@mail.gmail.com>
On Fri, 07 Apr 2017, Linus Walleij wrote:
> On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:
>
> > This driver handles pin configuration and pin muxing for the
> > JZ4740 and JZ4780 SoCs from Ingenic.
> >
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> (...)
> > + select MFD_CORE
> (...)
> > +#include <linux/mfd/core.h>
>
> That's unorthodox. Still quite pretty!
> I would nee Lee Jones to say something about this, as it is
> essentially hijacking MFD into the pinctrl subsystem.
>
> > +static struct mfd_cell ingenic_pinctrl_mfd_cells[] = {
> > + {
> > + .id = 0,
> > + .name = "GPIOA",
> > + .of_compatible = "ingenic,gpio-bank-a",
> > + },
> > + {
> > + .id = 1,
> > + .name = "GPIOB",
> > + .of_compatible = "ingenic,gpio-bank-b",
> > + },
> > + {
> > + .id = 2,
> > + .name = "GPIOC",
> > + .of_compatible = "ingenic,gpio-bank-c",
> > + },
> > + {
> > + .id = 3,
> > + .name = "GPIOD",
> > + .of_compatible = "ingenic,gpio-bank-d",
> > + },
> > + {
> > + .id = 4,
> > + .name = "GPIOE",
> > + .of_compatible = "ingenic,gpio-bank-e",
> > + },
> > + {
> > + .id = 5,
> > + .name = "GPIOF",
> > + .of_compatible = "ingenic,gpio-bank-f",
> > + },
> > +};
> (...)
> > + err = devm_mfd_add_devices(dev, 0, ingenic_pinctrl_mfd_cells,
> > + ARRAY_SIZE(ingenic_pinctrl_mfd_cells), NULL, 0, NULL);
> > + if (err) {
> > + dev_err(dev, "Failed to add MFD devices\n");
> > + return err;
> > + }
>
> I guess the alternative would be to reimplement the MFD structure.
>
> Did you check the approach to use "simple-mfd" and just let the subnodes
> spawn as devices that way? I guess you did and this adds something
> necessary.
I'd like to hear what the OP has to say about why this is necessary.
However, as a first glimpse, I'm dead against exporting MFD
functionality to outside of the subsystem.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 2/8] v4l: fwnode: Support generic fwnode for parsing standardised properties
From: Laurent Pinchart @ 2017-04-07 10:54 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Sakari Ailus, linux-media, linux-acpi, devicetree
In-Reply-To: <20170407103633.GD4192@valkosipuli.retiisi.org.uk>
Hi Sakari,
On Friday 07 Apr 2017 13:36:34 Sakari Ailus wrote:
> On Fri, Apr 07, 2017 at 12:44:27PM +0300, Laurent Pinchart wrote:
> > On Thursday 06 Apr 2017 16:12:04 Sakari Ailus wrote:
> > > The fwnode_handle is a more generic way than OF device_node to describe
> > > firmware nodes. Instead of the OF API, use more generic fwnode API to
> > > obtain the same information.
> >
> > I would mention that this is a copy of v4l2-of.c with the OF API replaced
> > with the fwnode API.
>
> I'll add that to the description.
>
> > > As the V4L2 fwnode support will be required by a small minority of e.g.
> > > ACPI based systems (the same might actually go for OF), make this a
> > > module instead of embedding it in the videodev module.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >
> > > drivers/media/v4l2-core/Kconfig | 3 +
> > > drivers/media/v4l2-core/Makefile | 1 +
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 353 +++++++++++++++++++++++++++
> > > include/media/v4l2-fwnode.h | 104 ++++++++++
> > > 4 files changed, 461 insertions(+)
> > > create mode 100644 drivers/media/v4l2-core/v4l2-fwnode.c
> > > create mode 100644 include/media/v4l2-fwnode.h
[snip]
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > b/drivers/media/v4l2-core/v4l2-fwnode.c new file mode 100644
> > > index 0000000..4f69b11
> > > --- /dev/null
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -0,0 +1,353 @@
> > > +/*
> > > + * V4L2 fwnode binding parsing library
> > > + *
> > > + * Copyright (c) 2016 Intel Corporation.
> > > + * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > + *
> > > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > > + *
> > > + * Copyright (C) 2012 Renesas Electronics Corp.
> > > + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of version 2 of the GNU General Public License as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +#include <linux/acpi.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/property.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/string.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include <media/v4l2-fwnode.h>
> > > +
> > > +static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle
> > > *fwn,
> > > + struct v4l2_fwnode_endpoint
> > > *vfwn)
> > > +{
> > > + struct v4l2_fwnode_bus_mipi_csi2 *bus = &vfwn->bus.mipi_csi2;
> > > + bool have_clk_lane = false;
> > > + unsigned int flags = 0, lanes_used = 0;
> > > + unsigned int i;
> > > + u32 v;
> > > + int rval;
> >
> > I would have used "ret" instead of "rval" ;-)
>
> I know. But
>
> 1) there's no established convention in the file and
>
> 2) "rval" has the benefit is easier to look up; one doesn't find a plethora
> of "return something". Therefore it is better than "ret" for the purpose.
The solution to that is
/ret\>
(and, of course, switching to vim :-D)
[snip]
> > > +/*
> > > + * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> > > + * v4l2_fwnode_endpoint_alloc_parse()
> > > + * @fwn - the V4L2 fwnode the resources of which are to be released
> >
> > Mayeb "the V4L2 fwnode whose resources are to be released" ?
> >
> > > + *
> > > + * It is safe to call this function with NULL argument or on an
> >
> > s/on an/on a/
>
> Yes.
>
> > > + * V4L2 fwnode the parsing of which failed.
> >
> > "whose parsing failed" ?
>
> Any particular reason? Do you like "whose"? :-)
"of which" sounds dubious in this context, but please consult a native English
speaker in case of doubt.
[snip]
> > > +/**
> > > + * v4l2_fwnode_endpoint_parse_link() - parse a link between two
> > > endpoints
> > > + * @node: pointer to the fwnode at the local end of the link
> >
> > The parameter is called __fwn. I believe you should rename it to fwn,
> > otherwise the documentation will look weird.
> >
> > As explained before, you should mention that this is an endpoint fwnode
> > handle.
>
> Agreed.
>
> > > + * @link: pointer to the V4L2 fwnode link data structure
> > > + *
> > > + * Fill the link structure with the local and remote nodes and port
> > > numbers.
> > > + * The local_node and remote_node fields are set to point to the local
> > > and
> > > + * remote port's parent nodes respectively (the port parent node being
> > > the
> > > + * parent node of the port node if that node isn't a 'ports' node, or
> > > the
> > > + * grand-parent node of the port node otherwise).
> > > + *
> > > + * A reference is taken to both the local and remote nodes, the caller
> > > + * must use v4l2_fwnode_endpoint_put_link() to drop the references
> > > + * when done with the link.
> >
> > Just curious, is there a reason to wrap earlier than the 80 columns limit
> > ?
> >
> > * A reference is taken to both the local and remote nodes, the caller
> > must use
> > * v4l2_fwnode_endpoint_put_link() to drop the references when done with
> > the
> > * link.
> >
> > would work.
>
> It is like a speed limit, you can perfectly legally drive slower than 80
> characters per line, right? :-)
That reminds me of a particular road trip in California that should probably
not be mentioned here, but whose participants certainly remember :-)
> Albeit the conditions seem pretty good in this file, I can try pushing it
> closer to the limit.
>
> > > + * Return: 0 on success, or -ENOLINK if the remote fwnode can't be
> > > found.
> >
> > Here too you should mention remote endpoint.
>
> Oh yes.
>
> > > + */
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints
From: Leonard Crestez @ 2017-04-07 10:51 UTC (permalink / raw)
To: Mark Brown
Cc: Sascha Hauer, Liam Girdwood, Viresh Kumar, Rafael J. Wysocki,
Shawn Guo, Robin Gong, Anson Huang, Irina Tirdea, Rob Herring,
Mark Rutland, Fabio Estevam, Octavian Purdila,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170406185202.uixxcv3dgucrddgc-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
On Thu, 2017-04-06 at 19:52 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 10:49:55PM +0300, Leonard Crestez wrote:
> > On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:
> > > To repeat what I said previously the whole point of bypassing is to not
> > > do regulation and generally the constraints in the unregulated idle case
> > > are substantially more relaxed. This would break use cases relying on
> > > the existing behaviour which wouldn't expect to affect the parent
> > > voltage at all, either stopping things working or making them less
> > > efficient by needlessly regulating the voltage down which defeats the
> > > main point of bypassing.
> > So what you want is to prevent voltage changes unless strictly
> > required, even lowering? What I want is to get the minimum voltage in
> > the SOC because that's where power is being consumed.
> So your end goal here is to bypass a regulator which is forced into your
> system design by being integrated into the SoC which isn't able to
> regulate down to a low enough voltage for your use case? I guess one
> question is if you need to use the regulator at all?
Both the PMIC and the LDOs can provide the required voltage range. LDO
bypass mode is a design tradeoff: the PMIC provides more efficient
conversion but with more fluctuations.
My question was about how the regulator framework handles constraints
and consumer voltage requests. What I want is for the output to be set
to the minimum value that meets all constraints. Maybe other users
would prefer regulators to keep their output as much as possible?
This is relevant even if LDOs are enabled, it's still preferable to
have PMIC output as low as possible because then less of the voltage
reduction is performed in the LDO.
It currently seems to work how I expect but from your statement it's
not clear if it's entirely intentional.
> > It's not at all obvious that in bypass mode the output constraints of a
> > regulator need not be respected by the core. I expected the opposite,
> > this is something that should be documented.
> SubmittingPatches... Bear in mind that most regulators are fixed
> voltage in a given system so bypass would be very difficult to use if it
> tried to pass the constraints upstream.
> > But if the bypassed regulator has a downstream consumer then it's
> > requirements should definitely still be met in bypass mode, right? I
> > could set my maximum voltage directly from cpufreq in that case.
> What we're interpreting bypass mode as meaning is "stop regulating".
> There will still be some limits but we're expecting them to be enforced
> in the extremes of the constraints in the parent regulators.
> > Or should a bypassed regulator ignore all other requests? One of the
> > behaviors that this patch series relies on is that calling set_voltage
> > on a bypassed regulator propagates this request to the supply and picks
> > the minimum voltage there. An alternative implementation would be to
> Yes, the expectation is that if anything is being changed it won't have
> any effect until regulation is reenabled but we're not particularly
> expecting much activity on bypassed regulators.
OK, so it is up to consumers to ensure the supply voltage is acceptable
before allowing bypass. If they want to do something clever they should
register as a consumer to the supply as well.
> >
> > call set_voltage directly on the supply regulator by changing the
> > "{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
> > Would that be better?
> That's more what's expected here.
Ok, I will try that approach instead.
> > Both approaches work. Relying on propagation feels like it is the
> > "right way" to handle this, even if it's harder to get right and the
> > regulator core does not entirely support it. But it's possible that
> > this is based on a misunderstanding of what "bypass" is actually
> > supposed to do.
> Another option would be to add a regulator configuration which allowed
> the regulator to transparently go into bypass mode if the parent could
> do things directly, only enabling regulation if the parent couldn't
> support. That would mean you'd loose the supply cleanup function which
> is typically part of why there are LDOs in the system but it sounds like
> you're OK with that in at least your use case.
Dynamic enabling of bypass mode in the core seems very complex and not
terribly useful for my case. I pretty much want them always in bypass
or always enabled.
--
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: [PATCH v2 3/8] v4l: async: Add fwnode match support
From: Laurent Pinchart @ 2017-04-07 10:47 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Sakari Ailus, linux-media, linux-acpi, devicetree
In-Reply-To: <20170407104508.GF4192@valkosipuli.retiisi.org.uk>
Hi Sakari,
On Friday 07 Apr 2017 13:45:09 Sakari Ailus wrote:
> On Fri, Apr 07, 2017 at 01:04:47PM +0300, Laurent Pinchart wrote:
> > > @@ -58,6 +60,9 @@ struct v4l2_async_subdev {
> > > const struct device_node *node;
> > > } of;
> > > struct {
> > > + struct fwnode_handle *fwn;
> >
> > Shouldn't this be const ?
>
> I thought the same, but a lot of functions that operate on fwnode_handle
> take a non-const argument. I attempted changing that, but it starts a
> cascade of unavoidable changes elsewhere. That's not very well suitable for
> this patchset.
fwnode is young, we should try to fix it instead of propagating issues :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 3/8] v4l: async: Add fwnode match support
From: Sakari Ailus @ 2017-04-07 10:45 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, linux-acpi, devicetree
In-Reply-To: <2374089.j4OXu9zDtc@avalon>
Hi Laurent,
On Fri, Apr 07, 2017 at 01:04:47PM +0300, Laurent Pinchart wrote:
> > @@ -58,6 +60,9 @@ struct v4l2_async_subdev {
> > const struct device_node *node;
> > } of;
> > struct {
> > + struct fwnode_handle *fwn;
>
> Shouldn't this be const ?
I thought the same, but a lot of functions that operate on fwnode_handle
take a non-const argument. I attempted changing that, but it starts a
cascade of unavoidable changes elsewhere. That's not very well suitable for
this patchset.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH v2 3/8] v4l: async: Add fwnode match support
From: Sakari Ailus @ 2017-04-07 10:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3722368.bfpMvP62Mi@avalon>
Hi Laurent,
On Fri, Apr 07, 2017 at 12:49:02PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thursday 06 Apr 2017 16:12:05 Sakari Ailus wrote:
> > Add fwnode matching to complement OF node matching. And fwnode may also be
> > an OF node.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 12 ++++++++++++
> > include/media/v4l2-async.h | 5 +++++
> > include/media/v4l2-subdev.h | 3 +++
> > 3 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 96cc733..384ad5e 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -46,6 +46,11 @@ static bool match_of(struct v4l2_subdev *sd, struct
> > v4l2_async_subdev *asd) of_node_full_name(asd->match.of.node));
> > }
> >
> > +static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd)
> > +{
> > + return sd->fwnode == asd->match.fwnode.fwn;
> > +}
> > +
> > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd) {
> > if (!asd->match.custom.match)
> > @@ -80,6 +85,9 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> > v4l2_async_notifier * case V4L2_ASYNC_MATCH_OF:
> > match = match_of;
> > break;
> > + case V4L2_ASYNC_MATCH_FWNODE:
> > + match = match_fwnode;
> > + break;
> > default:
> > /* Cannot happen, unless someone breaks us */
> > WARN_ON(true);
> > @@ -158,6 +166,7 @@ int v4l2_async_notifier_register(struct v4l2_device
> > *v4l2_dev, case V4L2_ASYNC_MATCH_DEVNAME:
> > case V4L2_ASYNC_MATCH_I2C:
> > case V4L2_ASYNC_MATCH_OF:
> > + case V4L2_ASYNC_MATCH_FWNODE:
> > break;
> > default:
> > dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev :
> NULL,
> > @@ -282,6 +291,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > */
> > if (!sd->of_node && sd->dev)
> > sd->of_node = sd->dev->of_node;
> > + if (!sd->fwnode && sd->dev)
> > + sd->fwnode = sd->dev->of_node ?
> > + &sd->dev->of_node->fwnode : sd->dev->fwnode;
> >
> > mutex_lock(&list_lock);
> >
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index 8e2a236..8f552d2 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -32,6 +32,7 @@ struct v4l2_async_notifier;
> > * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
> > * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
> > * @V4L2_ASYNC_MATCH_OF: Match will use OF node
> > + * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
> > *
> > * This enum is used by the asyncrhronous sub-device logic to define the
> > * algorithm that will be used to match an asynchronous device.
> > @@ -41,6 +42,7 @@ enum v4l2_async_match_type {
> > V4L2_ASYNC_MATCH_DEVNAME,
> > V4L2_ASYNC_MATCH_I2C,
> > V4L2_ASYNC_MATCH_OF,
> > + V4L2_ASYNC_MATCH_FWNODE,
> > };
> >
> > /**
> > @@ -58,6 +60,9 @@ struct v4l2_async_subdev {
> > const struct device_node *node;
> > } of;
> > struct {
> > + struct fwnode_handle *fwn;
>
> I'd name this "node". The rationale is that code should be as independent as
> possible of whether we use device_node or fwnode_handle. Naming both variable
> "node" helps in that regard, and is in my opinion easier to read. This applies
> to the other patches in the series too.
What you're proposing doesn't really change that: you'll have to be aware of
which one you have anyway. Variables pointing to fwnode_handle are often
called fwn as well --- as node is used for struct device_node.
In other words, I prefer to keep it as it is.
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>
> > + } fwnode;
> > + struct {
> > const char *name;
> > } device_name;
> > struct {
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 0ab1c5d..5f1669c 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -788,6 +788,8 @@ struct v4l2_subdev_platform_data {
> > * @devnode: subdev device node
> > * @dev: pointer to the physical device, if any
> > * @of_node: The device_node of the subdev, usually the same as
> > dev->of_node. + * @fwnode: The fwnode_handle of the subdev, usually the
> > same as
> > + * either dev->of_node->fwnode or dev->fwnode (whichever is non-
> NULL).
> > * @async_list: Links this subdev to a global subdev_list or
> > @notifier->done * list.
> > * @asd: Pointer to respective &struct v4l2_async_subdev.
> > @@ -819,6 +821,7 @@ struct v4l2_subdev {
> > struct video_device *devnode;
> > struct device *dev;
> > struct device_node *of_node;
> > + struct fwnode_handle *fwnode;
> > struct list_head async_list;
> > struct v4l2_async_subdev *asd;
> > struct v4l2_async_notifier *notifier;
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
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: [PATCH v2 2/8] v4l: fwnode: Support generic fwnode for parsing standardised properties
From: Sakari Ailus @ 2017-04-07 10:36 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, linux-acpi, devicetree
In-Reply-To: <2366571.B7YdK6QUO2@avalon>
Hi Laurent,
On Fri, Apr 07, 2017 at 12:44:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thursday 06 Apr 2017 16:12:04 Sakari Ailus wrote:
> > The fwnode_handle is a more generic way than OF device_node to describe
> > firmware nodes. Instead of the OF API, use more generic fwnode API to
> > obtain the same information.
>
> I would mention that this is a copy of v4l2-of.c with the OF API replaced with
> the fwnode API.
I'll add that to the description.
>
> > As the V4L2 fwnode support will be required by a small minority of e.g.
> > ACPI based systems (the same might actually go for OF), make this a module
> > instead of embedding it in the videodev module.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/Kconfig | 3 +
> > drivers/media/v4l2-core/Makefile | 1 +
> > drivers/media/v4l2-core/v4l2-fwnode.c | 353 +++++++++++++++++++++++++++++++
> > include/media/v4l2-fwnode.h | 104 ++++++++++
> > 4 files changed, 461 insertions(+)
> > create mode 100644 drivers/media/v4l2-core/v4l2-fwnode.c
> > create mode 100644 include/media/v4l2-fwnode.h
> >
> > diff --git a/drivers/media/v4l2-core/Kconfig
> > b/drivers/media/v4l2-core/Kconfig index 6b1b78f..a35c336 100644
> > --- a/drivers/media/v4l2-core/Kconfig
> > +++ b/drivers/media/v4l2-core/Kconfig
> > @@ -55,6 +55,9 @@ config V4L2_FLASH_LED_CLASS
> >
> > When in doubt, say N.
> >
> > +config V4L2_FWNODE
> > + tristate
> > +
> > # Used by drivers that need Videobuf modules
> > config VIDEOBUF_GEN
> > tristate
> > diff --git a/drivers/media/v4l2-core/Makefile
> > b/drivers/media/v4l2-core/Makefile index 795a535..cf77a63 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -13,6 +13,7 @@ endif
> > ifeq ($(CONFIG_OF),y)
> > videodev-objs += v4l2-of.o
> > endif
> > +obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> > ifeq ($(CONFIG_TRACEPOINTS),y)
> > videodev-objs += vb2-trace.o v4l2-trace.o
> > endif
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c new file mode 100644
> > index 0000000..4f69b11
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -0,0 +1,353 @@
> > +/*
> > + * V4L2 fwnode binding parsing library
> > + *
> > + * Copyright (c) 2016 Intel Corporation.
> > + * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > + *
> > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > + *
> > + * Copyright (C) 2012 Renesas Electronics Corp.
> > + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + */
> > +#include <linux/acpi.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +
> > +#include <media/v4l2-fwnode.h>
> > +
> > +static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle *fwn,
> > + struct v4l2_fwnode_endpoint
> *vfwn)
> > +{
> > + struct v4l2_fwnode_bus_mipi_csi2 *bus = &vfwn->bus.mipi_csi2;
> > + bool have_clk_lane = false;
> > + unsigned int flags = 0, lanes_used = 0;
> > + unsigned int i;
> > + u32 v;
> > + int rval;
>
> I would have used "ret" instead of "rval" ;-)
I know. But
1) there's no established convention in the file and
2) "rval" has the benefit is easier to look up; one doesn't find a plethora
of "return something". Therefore it is better than "ret" for the purpose.
>
> > + rval = fwnode_property_read_u32_array(fwn, "data-lanes", NULL, 0);
> > + if (rval > 0) {
> > + u32 array[ARRAY_SIZE(bus->data_lanes)];
> > +
> > + bus->num_data_lanes =
> > + min_t(int, ARRAY_SIZE(bus->data_lanes), rval);
> > +
> > + fwnode_property_read_u32_array(
> > + fwn, "data-lanes", array, bus->num_data_lanes);
>
> Is there anything wrong with the usual way to wrap code ?
>
> fwnode_property_read_u32_array(fwn, "data-lanes", array,
> bus->num_data_lanes);
I'll change the occurrences of that whenever feasible.
>
> > +
> > + for (i = 0; i < bus->num_data_lanes; i++) {
> > + if (lanes_used & BIT(array[i]))
> > + pr_warn("duplicated lane %u in data-lanes\n",
> > + array[i]);
> > + lanes_used |= BIT(array[i]);
> > +
> > + bus->data_lanes[i] = array[i];
> > + }
> > + }
> > +
> > + rval = fwnode_property_read_u32_array(fwn, "lane-polarities", NULL,
> 0);
> > + if (rval > 0) {
> > + u32 array[ARRAY_SIZE(bus->lane_polarities)];
> > +
> > + if (rval < 1 + bus->num_data_lanes /* clock + data */) {
> > + pr_warn("too few lane-polarities entries (need %u, got
> %u)\n",
> > + 1 + bus->num_data_lanes, rval);
> > + return -EINVAL;
> > + }
> > +
> > + fwnode_property_read_u32_array(
> > + fwn, "lane-polarities", array, 1 + bus-
> >num_data_lanes);
>
> Ditto with
>
> fwnode_property_read_u32_array(fwn, "lane-polarities", array,
> 1 + bus->num_data_lanes);
>
> > + for (i = 0; i < 1 + bus->num_data_lanes; i++)
> > + bus->lane_polarities[i] = array[i];
> > + }
> > +
> > + if (!fwnode_property_read_u32(fwn, "clock-lanes", &v)) {
> > + if (lanes_used & BIT(v))
> > + pr_warn("duplicated lane %u in clock-lanes\n", v);
> > + lanes_used |= BIT(v);
> > +
> > + bus->clock_lane = v;
> > + have_clk_lane = true;
> > + }
> > +
> > + if (fwnode_property_present(fwn, "clock-noncontinuous"))
> > + flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> > + else if (have_clk_lane || bus->num_data_lanes > 0)
> > + flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> > +
> > + bus->flags = flags;
> > + vfwn->bus_type = V4L2_MBUS_CSI2;
> > +
> > + return 0;
> > +}
> > +
> > +static void v4l2_fwnode_endpoint_parse_parallel_bus(
> > + struct fwnode_handle *fwn, struct v4l2_fwnode_endpoint *vfwn)
> > +{
> > + struct v4l2_fwnode_bus_parallel *bus = &vfwn->bus.parallel;
> > + unsigned int flags = 0;
> > + u32 v;
> > +
> > + if (!fwnode_property_read_u32(fwn, "hsync-active", &v))
> > + flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH :
> > + V4L2_MBUS_HSYNC_ACTIVE_LOW;
> > +
> > + if (!fwnode_property_read_u32(fwn, "vsync-active", &v))
> > + flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH :
> > + V4L2_MBUS_VSYNC_ACTIVE_LOW;
> > +
> > + if (!fwnode_property_read_u32(fwn, "field-even-active", &v))
> > + flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH :
> > + V4L2_MBUS_FIELD_EVEN_LOW;
> > + if (flags)
> > + vfwn->bus_type = V4L2_MBUS_PARALLEL;
> > + else
> > + vfwn->bus_type = V4L2_MBUS_BT656;
> > +
> > + if (!fwnode_property_read_u32(fwn, "pclk-sample", &v))
> > + flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING :
> > + V4L2_MBUS_PCLK_SAMPLE_FALLING;
> > +
> > + if (!fwnode_property_read_u32(fwn, "data-active", &v))
> > + flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH :
> > + V4L2_MBUS_DATA_ACTIVE_LOW;
> > +
> > + if (fwnode_property_present(fwn, "slave-mode"))
> > + flags |= V4L2_MBUS_SLAVE;
> > + else
> > + flags |= V4L2_MBUS_MASTER;
> > +
> > + if (!fwnode_property_read_u32(fwn, "bus-width", &v))
> > + bus->bus_width = v;
> > +
> > + if (!fwnode_property_read_u32(fwn, "data-shift", &v))
> > + bus->data_shift = v;
> > +
> > + if (!fwnode_property_read_u32(fwn, "sync-on-green-active", &v))
> > + flags |= v ? V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH :
> > + V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW;
> > +
> > + bus->flags = flags;
> > +
> > +}
> > +
> > +/**
> > + * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> > + * @fwn: pointer to fwnode_handle
>
> I would say "the endpoint's fwnode handle", or "pointer to the endpoint's
> fwnode handle". Otherwise which fwnode handle should be passed won't be clear.
I prefer the latter.
>
> > + * @vfwn: pointer to the V4L2 fwnode data structure
> > + *
> > + * All properties are optional. If none are found, we don't set any flags.
> > + * This means the port has a static configuration and no properties have
> > + * to be specified explicitly.
> > + * If any properties that identify the bus as parallel are found and
> > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we
> > recognise
> > + * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> > + * The caller should hold a reference to @node.
>
> s/@node/@fwn/
>
> By the way, have you tried to compile the documentation ?
I have, but I may have made changes since that. I'll test that again before
submitting the next version.
>
> > + *
> > + * NOTE: This function does not parse properties the size of which is
> > + * variable without a low fixed limit. Please use
> > + * v4l2_fwnode_endpoint_alloc_parse() in new drivers instead.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwn,
> > + struct v4l2_fwnode_endpoint *vfwn)
>
> Is there a specific reason not to keep the original parameter name "endpoint"
> ? vfwn seems to stand for v4l2_fwnode, not v4l2_fwnode_endpoint. Same comment
> for all other locations in this patch.
I'm ok with keeping endpoint.
>
> > +{
> > + int rval;
> > +
> > + fwnode_graph_parse_endpoint(fwn, &vfwn->base);
> > +
> > + /* Zero fields from bus_type to until the end */
> > + memset(&vfwn->bus_type, 0, sizeof(*vfwn) -
> > + offsetof(typeof(*vfwn), bus_type));
> > +
> > + rval = v4l2_fwnode_endpoint_parse_csi_bus(fwn, vfwn);
> > + if (rval)
> > + return rval;
> > + /*
> > + * Parse the parallel video bus properties only if none
> > + * of the MIPI CSI-2 specific properties were found.
> > + */
> > + if (vfwn->bus.mipi_csi2.flags == 0)
> > + v4l2_fwnode_endpoint_parse_parallel_bus(fwn, vfwn);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_fwnode_endpoint_parse);
>
> According to Mauro in http://www.mail-archive.com/linux-media@vger.kernel.org/msg110809.html you should use EXPORT_SYMBOL_GPL.
EXPORT_SYMBOL() was used by v4l2-of.c; I'll switch here.
>
> > +
> > +/*
> > + * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> > + * v4l2_fwnode_endpoint_alloc_parse()
> > + * @fwn - the V4L2 fwnode the resources of which are to be released
>
> Mayeb "the V4L2 fwnode whose resources are to be released" ?
>
> > + *
> > + * It is safe to call this function with NULL argument or on an
>
> s/on an/on a/
Yes.
>
> > + * V4L2 fwnode the parsing of which failed.
>
> "whose parsing failed" ?
Any particular reason? Do you like "whose"? :-)
>
> > + */
> > +void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vfwn)
> > +{
> > + if (IS_ERR_OR_NULL(vfwn))
> > + return;
> > +
> > + kfree(vfwn->link_frequencies);
> > + kfree(vfwn);
> > +}
> > +EXPORT_SYMBOL(v4l2_fwnode_endpoint_free);
> > +
> > +/**
> > + * v4l2_fwnode_endpoint_alloc_parse() - parse all fwnode node properties
> > + * @node: pointer to fwnode_handle
>
> s/@node/@fwn/
>
> Here too, I would say "the endpoint's fwnode handle", or "pointer to the
> endpoint's fwnode handle".
I'll use the latter.
>
> > + *
> > + * All properties are optional. If none are found, we don't set any flags.
> > + * This means the port has a static configuration and no properties have
> > + * to be specified explicitly.
> > + * If any properties that identify the bus as parallel are found and
> > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we
> > recognise
> > + * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> > + * The caller should hold a reference to @node.
>
> s/@node/@fwn/
Will fix.
>
> > + *
> > + * v4l2_fwnode_endpoint_alloc_parse() has two important differences to
> > + * v4l2_fwnode_endpoint_parse():
> > + *
> > + * 1. It also parses variable size data and
>
> Nitpicking, a valid sentence doesn't end with "and$". You could write
>
> 1. It also parses variable size data.
> 2. The memory it has allocated to store the variable size data must
> be freed using v4l2_fwnode_endpoint_free() when no longer needed.
I prefer this one, will change.
>
> or
>
> 1. it also parses variable size data; and
> 2. the memory it has allocated to store the variable size data must
> be freed using v4l2_fwnode_endpoint_free() when no longer needed.
>
> > + *
> > + * 2. The memory it has allocated to store the variable size data must
> > + * be freed using v4l2_fwnode_endpoint_free() when no longer needed.
> > + *
> > + * Return: Pointer to v4l2_fwnode_endpoint if successful, on error a
> > + * negative error code.
>
> The function returns an error pointer on error.
Yes, I'll improve the documentation.
>
> > + */
> > +struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> > + struct fwnode_handle *fwn)
> > +{
> > + struct v4l2_fwnode_endpoint *vfwn;
> > + int rval;
>
> ret ? :-)
>
> > +
> > + vfwn = kzalloc(sizeof(*vfwn), GFP_KERNEL);
> > + if (!vfwn)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + rval = v4l2_fwnode_endpoint_parse(fwn, vfwn);
> > + if (rval < 0)
> > + goto out_err;
> > +
> > + rval = fwnode_property_read_u64_array(fwn, "link-frequencies",
> > + NULL, 0);
> > +
>
> No need for a blank line.
Indeed.
>
> > + if (rval < 0)
> > + goto out_err;
> > +
> > + vfwn->link_frequencies =
> > + kmalloc_array(rval, sizeof(*vfwn->link_frequencies),
> > + GFP_KERNEL);
> > + if (!vfwn->link_frequencies) {
> > + rval = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vfwn->nr_of_link_frequencies = rval;
> > +
> > + rval = fwnode_property_read_u64_array(
> > + fwn, "link-frequencies", vfwn->link_frequencies,
> > + vfwn->nr_of_link_frequencies);
>
> rval = fwnode_property_read_u64_array(fwn, "link-frequencies",
> vfwn->link_frequencies,
> vfwn->nr_of_link_frequencies);
>
> > + if (rval < 0)
> > + goto out_err;
> > +
> > + return vfwn;
> > +
> > +out_err:
> > + v4l2_fwnode_endpoint_free(vfwn);
> > + return ERR_PTR(rval);
> > +}
> > +EXPORT_SYMBOL(v4l2_fwnode_endpoint_alloc_parse);
> > +
> > +/**
> > + * v4l2_fwnode_endpoint_parse_link() - parse a link between two endpoints
> > + * @node: pointer to the fwnode at the local end of the link
>
> The parameter is called __fwn. I believe you should rename it to fwn,
> otherwise the documentation will look weird.
>
> As explained before, you should mention that this is an endpoint fwnode
> handle.
Agreed.
>
> > + * @link: pointer to the V4L2 fwnode link data structure
> > + *
> > + * Fill the link structure with the local and remote nodes and port
> > numbers.
> > + * The local_node and remote_node fields are set to point to the local and
> > + * remote port's parent nodes respectively (the port parent node being the
> > + * parent node of the port node if that node isn't a 'ports' node, or the
> > + * grand-parent node of the port node otherwise).
> > + *
> > + * A reference is taken to both the local and remote nodes, the caller
> > + * must use v4l2_fwnode_endpoint_put_link() to drop the references
> > + * when done with the link.
>
> Just curious, is there a reason to wrap earlier than the 80 columns limit ?
>
> * A reference is taken to both the local and remote nodes, the caller must
> use
> * v4l2_fwnode_endpoint_put_link() to drop the references when done with the
> * link.
>
> would work.
It is like a speed limit, you can perfectly legally drive slower than 80
characters per line, right? :-)
Albeit the conditions seem pretty good in this file, I can try pushing it
closer to the limit.
>
> > + * Return: 0 on success, or -ENOLINK if the remote fwnode can't be found.
>
> Here too you should mention remote endpoint.
Oh yes.
>
> > + */
> > +int v4l2_fwnode_parse_link(struct fwnode_handle *__fwn,
> > + struct v4l2_fwnode_link *link)
> > +{
> > + struct fwnode_handle *fwn;
> > + const char *port_prop = is_of_node(__fwn) ? "reg" : "port";
> > +
> > + memset(link, 0, sizeof(*link));
> > +
> > + fwn = fwnode_get_parent(__fwn);
> > + fwnode_property_read_u32(fwn, port_prop, &link->local_port);
> > + fwn = fwnode_get_next_parent(fwn);
> > + if (is_of_node(fwn)) {
> > + if (of_node_cmp(to_of_node(fwn)->name, "ports") == 0)
> > + fwn = fwnode_get_next_parent(fwn);
> > + } else {
> > + /* The "ports" node is always there in ACPI. */
> > + fwn = fwnode_get_next_parent(fwn);
> > + }
>
> To avoid duplicating the fwnode_get_next_parent(fwn) call, you could write
>
> if (!is_of_node(fwn) ||
> of_node_cmp(to_of_node(fwn)->name, "ports") == 0) {
> /* The "ports" node is always there in ACPI. */
> fwn = fwnode_get_next_parent(fwn);
> }
Will change.
>
> > + link->local_node = fwn;
> > +
> > + fwn = fwnode_graph_get_remote_endpoint(fwn);
> > + if (!fwn) {
> > + fwnode_handle_put(fwn);
> > + return -ENOLINK;
> > + }
> > +
> > + fwn = fwnode_get_parent(fwn);
> > + fwnode_property_read_u32(fwn, port_prop, &link->remote_port);
> > + fwn = fwnode_get_next_parent(fwn);
> > + if (is_of_node(fwn)) {
> > + if (of_node_cmp(to_of_node(fwn)->name, "ports") == 0)
> > + fwn = fwnode_get_next_parent(fwn);
> > + } else {
> > + /* The "ports" node is always there in ACPI. */
> > + fwn = fwnode_get_next_parent(fwn);
> > + }
>
> Same comment here.
>
> > + link->remote_node = fwn;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_fwnode_parse_link);
> > +
> > +/**
> > + * v4l2_fwnode_put_link() - drop references to nodes in a link
> > + * @link: pointer to the V4L2 fwnode link data structure
> > + *
> > + * Drop references to the local and remote nodes in the link. This
> > + * function must be called on every link parsed with
> > + * v4l2_fwnode_parse_link().
> > + */
> > +void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> > +{
> > + fwnode_handle_put(link->local_node);
> > + fwnode_handle_put(link->remote_node);
> > +}
> > +EXPORT_SYMBOL(v4l2_fwnode_put_link);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> > +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > new file mode 100644
> > index 0000000..a675d8a
> > --- /dev/null
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -0,0 +1,104 @@
> > +/*
> > + * V4L2 fwnode binding parsing library
> > + *
> > + * Copyright (c) 2016 Intel Corporation.
> > + * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > + *
> > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > + *
> > + * Copyright (C) 2012 Renesas Electronics Corp.
> > + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef _V4L2_FWNODE_H
> > +#define _V4L2_FWNODE_H
> > +
> > +#include <linux/list.h>
> > +#include <linux/types.h>
> > +#include <linux/errno.h>
> > +#include <linux/of_graph.h>
>
> Alphabetical order please.
Yes.
>
> > +#include <media/v4l2-mediabus.h>
> > +
> > +struct fwnode_handle;
> > +
> > +/**
> > + * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
> > + * @flags: media bus (V4L2_MBUS_*) flags
> > + * @data_lanes: an array of physical data lane indexes
> > + * @clock_lane: physical lane index of the clock lane
> > + * @num_data_lanes: number of data lanes
> > + * @lane_polarities: polarity of the lanes. The order is the same of
> > + * the physical lanes.
> > + */
> > +struct v4l2_fwnode_bus_mipi_csi2 {
> > + unsigned int flags;
> > + unsigned char data_lanes[4];
> > + unsigned char clock_lane;
> > + unsigned short num_data_lanes;
> > + bool lane_polarities[5];
> > +};
> > +
> > +/**
> > + * struct v4l2_fwnode_bus_parallel - parallel data bus data structure
> > + * @flags: media bus (V4L2_MBUS_*) flags
> > + * @bus_width: bus width in bits
> > + * @data_shift: data shift in bits
> > + */
> > +struct v4l2_fwnode_bus_parallel {
> > + unsigned int flags;
> > + unsigned char bus_width;
> > + unsigned char data_shift;
> > +};
> > +
> > +/**
> > + * struct v4l2_fwnode_endpoint - the endpoint data structure
> > + * @base: fwnode endpoint of the v4l2_fwnode
> > + * @bus_type: bus type
> > + * @bus: bus configuration data structure
> > + * @link_frequencies: array of supported link frequencies
> > + * @nr_of_link_frequencies: number of elements in link_frequenccies array
> > + */
> > +struct v4l2_fwnode_endpoint {
> > + struct fwnode_endpoint base;
> > + /*
> > + * Fields below this line will be zeroed by
> > + * v4l2_fwnode_parse_endpoint()
> > + */
> > + enum v4l2_mbus_type bus_type;
> > + union {
> > + struct v4l2_fwnode_bus_parallel parallel;
> > + struct v4l2_fwnode_bus_mipi_csi2 mipi_csi2;
> > + } bus;
> > + u64 *link_frequencies;
> > + unsigned int nr_of_link_frequencies;
> > +};
> > +
> > +/**
> > + * struct v4l2_fwnode_link - a link between two endpoints
> > + * @local_node: pointer to device_node of this endpoint
> > + * @local_port: identifier of the port this endpoint belongs to
> > + * @remote_node: pointer to device_node of the remote endpoint
> > + * @remote_port: identifier of the port the remote endpoint belongs to
> > + */
> > +struct v4l2_fwnode_link {
> > + struct fwnode_handle *local_node;
> > + unsigned int local_port;
> > + struct fwnode_handle *remote_node;
> > + unsigned int remote_port;
> > +};
> > +
> > +int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwn,
> > + struct v4l2_fwnode_endpoint *vfwn);
> > +struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> > + struct fwnode_handle *fwn);
> > +void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vfwn);
> > +int v4l2_fwnode_parse_link(struct fwnode_handle *fwn,
> > + struct v4l2_fwnode_link *link);
> > +void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> > +
> > +#endif /* _V4L2_FWNODE_H */
>
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH v2 5/8] v4l: Switch from V4L2 OF not V4L2 fwnode API
From: Laurent Pinchart @ 2017-04-07 10:32 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-6-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:07 Sakari Ailus wrote:
> Switch users of the v4l2_of_ APIs to the more generic v4l2_fwnode_ APIs.
>
> Existing OF matching continues to be supported. omap3isp and smiapp
> drivers are converted to fwnode matching as well.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Benoit Parrot <bparrot@ti.com> # i2c/ov2569.c,
> am437x/am437x-vpfe.c and ti-vpe/cal.c ---
> drivers/media/i2c/Kconfig | 9 ++++
> drivers/media/i2c/adv7604.c | 7 +--
> drivers/media/i2c/mt9v032.c | 7 +--
> drivers/media/i2c/ov2659.c | 8 +--
> drivers/media/i2c/s5c73m3/s5c73m3-core.c | 7 +--
> drivers/media/i2c/s5k5baf.c | 6 +--
> drivers/media/i2c/smiapp/Kconfig | 1 +
> drivers/media/i2c/smiapp/smiapp-core.c | 29 ++++++-----
> drivers/media/i2c/tc358743.c | 11 ++--
> drivers/media/i2c/tvp514x.c | 6 +--
> drivers/media/i2c/tvp5150.c | 7 +--
> drivers/media/i2c/tvp7002.c | 6 +--
> drivers/media/platform/Kconfig | 3 ++
> drivers/media/platform/am437x/Kconfig | 1 +
> drivers/media/platform/am437x/am437x-vpfe.c | 8 +--
> drivers/media/platform/atmel/Kconfig | 1 +
> drivers/media/platform/atmel/atmel-isc.c | 8 +--
> drivers/media/platform/exynos4-is/Kconfig | 2 +
> drivers/media/platform/exynos4-is/media-dev.c | 6 +--
> drivers/media/platform/exynos4-is/mipi-csis.c | 6 +--
> drivers/media/platform/omap3isp/isp.c | 71 +++++++++++-----------
> drivers/media/platform/pxa_camera.c | 7 +--
> drivers/media/platform/rcar-vin/Kconfig | 1 +
> drivers/media/platform/rcar-vin/rcar-core.c | 6 +--
> drivers/media/platform/soc_camera/Kconfig | 1 +
> drivers/media/platform/soc_camera/atmel-isi.c | 7 +--
> drivers/media/platform/soc_camera/soc_camera.c | 3 +-
> drivers/media/platform/ti-vpe/cal.c | 11 ++--
> drivers/media/platform/xilinx/Kconfig | 1 +
> drivers/media/platform/xilinx/xilinx-vipp.c | 59 +++++++++++----------
> include/media/v4l2-fwnode.h | 4 +-
> 31 files changed, 176 insertions(+), 134 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index cee1dae..6b2423a 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -210,6 +210,7 @@ config VIDEO_ADV7604
> depends on GPIOLIB || COMPILE_TEST
> select HDMI
> select MEDIA_CEC_EDID
> + select V4L2_FWNODE
What happens when building the driver on a platform that includes neither OF
nor ACPI support ?
> ---help---
> Support for the Analog Devices ADV7604 video decoder.
>
[snip]
How have you checked that you haven't missed any entry in the Kconfig files ?
[snip]
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 084ecf4a..95850b9 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
[snip]
> @@ -2024,43 +2025,42 @@ enum isp_of_phy {
> ISP_OF_PHY_CSIPHY2,
> };
>
> -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> - struct isp_async_subdev *isd)
> +static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> + struct isp_async_subdev *isd)
> {
> struct isp_bus_cfg *buscfg = &isd->bus;
> - struct v4l2_of_endpoint vep;
> + struct v4l2_fwnode_endpoint vfwn;
vfwn is confusing to me, I think the variable name should show that it refers
to an endpoint.
> unsigned int i;
> int ret;
>
> - ret = v4l2_of_parse_endpoint(node, &vep);
> + ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
> if (ret)
> return ret;
>
> - dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> - vep.base.port);
> + dev_dbg(dev, "interface %u\n", vfwn.base.port);
Is there no way to keep the node name in the error message ?
[snip]
> @@ -2094,18 +2094,17 @@ static int isp_of_parse_node(struct device *dev,
> struct device_node *node, break;
>
> default:
> - dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
> - vep.base.port);
> + dev_warn(dev, "invalid interface %u\n", vfwn.base.port);
Ditto.
> break;
> }
>
> return 0;
> }
>
> -static int isp_of_parse_nodes(struct device *dev,
> - struct v4l2_async_notifier *notifier)
> +static int isp_fwnodes_parse(struct device *dev,
> + struct v4l2_async_notifier *notifier)
> {
> - struct device_node *node = NULL;
> + struct fwnode_handle *fwn = NULL;
As explained in the review of another patch from the same series, I wouldn't
rename the variable.
> notifier->subdevs = devm_kcalloc(
> dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
[snip]
> @@ -2219,12 +2220,12 @@ static int isp_probe(struct platform_device *pdev)
> if (IS_ERR(isp->syscon))
> return PTR_ERR(isp->syscon);
>
> - ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
> - &isp->syscon_offset);
> + ret = of_property_read_u32_index(pdev->dev.of_node,
> + "syscon", 1, &isp->syscon_offset);
This change doesn't seem to be needed.
> if (ret)
> return ret;
>
> - ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
> + ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
> if (ret < 0)
> return ret;
>
[snip]
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c index feb3b2f..6a2721b 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
[snip]
> @@ -103,9 +103,10 @@ static int xvip_graph_build_one(struct
> xvip_composite_device *xdev, * the link.
> */
> if (link.local_port >= local->num_pads) {
> - dev_err(xdev->dev, "invalid port number %u on %s\n",
> - link.local_port, link.local_node->full_name);
> - v4l2_of_put_link(&link);
> + dev_err(xdev->dev, "invalid port number %u for %s\n",
> + link.local_port,
> + to_of_node(link.local_node)->full_name);
This makes me believe that we're missing a fwnode_full_name() function.
> + v4l2_fwnode_put_link(&link);
> ret = -EINVAL;
> break;
> }
[snip]
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index a675d8a..bc9cf51 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -17,10 +17,10 @@
> #ifndef _V4L2_FWNODE_H
> #define _V4L2_FWNODE_H
>
> +#include <linux/errno.h>
> +#include <linux/fwnode.h>
> #include <linux/list.h>
> #include <linux/types.h>
> -#include <linux/errno.h>
> -#include <linux/of_graph.h>
>
> #include <media/v4l2-mediabus.h>
This probably belongs to another patch (at least the alphabetical sorting
does).
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH V3 4/4] pwm: tegra: Add support to configure pin state in suspends/resume
From: Jon Hunter @ 2017-04-07 10:27 UTC (permalink / raw)
To: Laxman Dewangan, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491557642-15940-5-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 07/04/17 10:34, Laxman Dewangan wrote:
> In some of NVIDIA Tegra's platform, PWM controller is used to
> control the PWM controlled regulators. PWM signal is connected to
> the VID pin of the regulator where duty cycle of PWM signal decide
> the voltage level of the regulator output.
>
> When system enters suspend, some PWM client/slave regulator devices
> require the PWM output to be tristated.
>
> Add support to configure the pin state via pinctrl frameworks in
> suspend and active state of the system.
>
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> ---
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
> - Use API pinctrl_pm_select_*()
>
> Changes from V2:
> - Use returns of pinctrl_pm_select_*()
> - Rephrase commit message.
> ---
> drivers/pwm/pwm-tegra.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 21518be..9c7f180 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -29,6 +29,7 @@
> #include <linux/of_device.h>
> #include <linux/pwm.h>
> #include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> #include <linux/slab.h>
> #include <linux/reset.h>
>
> @@ -255,6 +256,18 @@ static int tegra_pwm_remove(struct platform_device *pdev)
> return pwmchip_remove(&pc->chip);
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_pwm_suspend(struct device *dev)
> +{
> + return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int tegra_pwm_resume(struct device *dev)
> +{
> + return pinctrl_pm_select_default_state(dev);
> +}
> +#endif
> +
> static const struct tegra_pwm_soc tegra20_pwm_soc = {
> .num_channels = 4,
> };
> @@ -271,10 +284,15 @@ static const struct of_device_id tegra_pwm_of_match[] = {
>
> MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
>
> +static const struct dev_pm_ops tegra_pwm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> +};
> +
> static struct platform_driver tegra_pwm_driver = {
> .driver = {
> .name = "tegra-pwm",
> .of_match_table = tegra_pwm_of_match,
> + .pm = &tegra_pwm_pm_ops,
> },
> .probe = tegra_pwm_probe,
> .remove = tegra_pwm_remove,
Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high
From: Linus Walleij @ 2017-04-07 10:25 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Alexandre Courbot, Rob Herring, Mark Rutland, Frank Rowand,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <1491485752-28030-1-git-send-email-ldewangan@nvidia.com>
On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Currently, the GPIO interface is said to Open Drain if it is Single
> Ended and active LOW. Similarly, it is said as Open Source if it is
> Single Ended and active HIGH.
>
> The active HIGH/LOW is used in the interface for setting the pin
> state to HIGH or LOW when enabling/disabling the interface.
>
> In Open Drain interface, pin is set to HIGH by putting pin in
> high impedance and LOW by driving to the LOW.
>
> In Open Source interface, pin is set to HIGH by driving pin to
> HIGH and set to LOW by putting pin in high impedance.
>
> With above, the Open Drain/Source is unrelated to the active LOW/HIGH
> in interface. There is interface where the enable/disable of interface
> is ether active LOW or HIGH but it is Open Drain type.
>
> Hence decouple the Open Drain with Single Ended + Active LOW and
> Open Source with Single Ended + Active HIGH.
>
> Adding different flag for the Open Drain/Open Source which is valid
> only when Single ended flag is enabled.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Patch applied.
Good that you found this and fixed it before someone git hurt.
Sorry for screwing it up, my fault.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH V3 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume
From: Jon Hunter @ 2017-04-07 10:25 UTC (permalink / raw)
To: Laxman Dewangan, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491557642-15940-4-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 07/04/17 10:34, Laxman Dewangan wrote:
> In some of NVIDIA Tegra's platform, PWM controller is used to
> control the PWM controlled regulators. PWM signal is connected to
> the VID pin of the regulator where duty cycle of PWM signal decide
> the voltage level of the regulator output.
>
> When system enters suspend, some PWM client/slave regulator devices
> require the PWM output to be tristated.
>
> Add DT binding details to provide the pin configuration state
> from PWM and pinctrl DT node in suspend and active state of
> the system.
>
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> ---
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
>
> Changes from V2:
> - Fix the commit message and details
> ---
> .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 45 ++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> index b4e7377..c57e11b 100644
> --- a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> @@ -19,6 +19,19 @@ Required properties:
> - reset-names: Must include the following entries:
> - pwm
>
> +Optional properties:
> +============================
> +In some of the interface like PWM based regulator device, it is required
> +to configure the pins differently in different states, especially in suspend
> +state of the system. The configuration of pin is provided via the pinctrl
> +DT node as detailed in the pinctrl DT binding document
> + Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +The PWM node will have following optional properties.
> +pinctrl-names: Pin state names. Must be "default" and "sleep".
> +pinctrl-0: phandle for the default/active state of pin configurations.
> +pinctrl-1: phandle for the sleep state of pin configurations.
> +
> Example:
>
> pwm: pwm@7000a000 {
> @@ -29,3 +42,35 @@ Example:
> resets = <&tegra_car 17>;
> reset-names = "pwm";
> };
> +
> +
> +Example with the pin configuration for suspend and resume:
> +=========================================================
> +Suppose pin PE7 (On Tegra210) interfaced with the regulator device and
> +it requires PWM output to be tristated when system enters suspend.
> +Following will be DT binding to achieve this:
> +
> +#include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +
> + pinmux@700008d4 {
> + pwm_active_state: pwm_active_state {
> + pe7 {
> + nvidia,pins = "pe7";
> + nvidia,tristate = <TEGRA_PIN_DISABLE>;
> + };
> + };
> +
> + pwm_sleep_state: pwm_sleep_state {
> + pe7 {
> + nvidia,pins = "pe7";
> + nvidia,tristate = <TEGRA_PIN_ENABLE>;
> + };
> + };
> + };
> +
> + pwm@7000a000 {
> + /* Mandatory PWM properties */
Maybe these are mandatory for the platform, but given these are
optional, its a bit confusing.
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&pwm_active_state>;
> + pinctrl-1 = <&pwm_sleep_state>;
> + };
However, fine with me so ...
Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v2 1/8] v4l: flash led class: Use fwnode_handle instead of device_node in init
From: Sakari Ailus @ 2017-04-07 10:20 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3129720.GhXEqohgpp@avalon>
Hi Laurent,
Thank you for the review!
On Fri, Apr 07, 2017 at 11:49:56AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thursday 06 Apr 2017 16:12:03 Sakari Ailus wrote:
> > Pass the more generic fwnode_handle to the init function than the
> > device_node.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> > drivers/leds/leds-aat1290.c | 5 +++--
> > drivers/leds/leds-max77693.c | 5 +++--
> > drivers/media/v4l2-core/v4l2-flash-led-class.c | 11 ++++++-----
> > include/media/v4l2-flash-led-class.h | 4 ++--
> > 4 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> > index def3cf9..a21e192 100644
> > --- a/drivers/leds/leds-aat1290.c
> > +++ b/drivers/leds/leds-aat1290.c
> > @@ -503,8 +503,9 @@ static int aat1290_led_probe(struct platform_device
> > *pdev) aat1290_init_v4l2_flash_config(led, &led_cfg, &v4l2_sd_cfg);
> >
> > /* Create V4L2 Flash subdev. */
> > - led->v4l2_flash = v4l2_flash_init(dev, sub_node, fled_cdev, NULL,
> > - &v4l2_flash_ops, &v4l2_sd_cfg);
> > + led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
> > + fled_cdev, NULL, &v4l2_flash_ops,
> > + &v4l2_sd_cfg);
> > if (IS_ERR(led->v4l2_flash)) {
> > ret = PTR_ERR(led->v4l2_flash);
> > goto error_v4l2_flash_init;
> > diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
> > index 1eb58ef..2d3062d 100644
> > --- a/drivers/leds/leds-max77693.c
> > +++ b/drivers/leds/leds-max77693.c
> > @@ -930,8 +930,9 @@ static int max77693_register_led(struct max77693_sub_led
> > *sub_led, max77693_init_v4l2_flash_config(sub_led, led_cfg, &v4l2_sd_cfg);
> >
> > /* Register in the V4L2 subsystem. */
> > - sub_led->v4l2_flash = v4l2_flash_init(dev, sub_node, fled_cdev, NULL,
> > - &v4l2_flash_ops, &v4l2_sd_cfg);
> > + sub_led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
> > + fled_cdev, NULL,
> &v4l2_flash_ops,
> > + &v4l2_sd_cfg);
> > if (IS_ERR(sub_led->v4l2_flash)) {
> > ret = PTR_ERR(sub_led->v4l2_flash);
> > goto err_v4l2_flash_init;
> > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > b/drivers/media/v4l2-core/v4l2-flash-led-class.c index 794e563..f430c89
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/of.h>
>
> I think you can drop linux/of.h.
Correct. Will fix.
>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/types.h>
> > #include <media/v4l2-flash-led-class.h>
> > @@ -612,7 +613,7 @@ static const struct v4l2_subdev_internal_ops
> > v4l2_flash_subdev_internal_ops = { static const struct v4l2_subdev_ops
> > v4l2_flash_subdev_ops;
> >
> > struct v4l2_flash *v4l2_flash_init(
> > - struct device *dev, struct device_node *of_node,
> > + struct device *dev, struct fwnode_handle *fwn,
> > struct led_classdev_flash *fled_cdev,
> > struct led_classdev_flash *iled_cdev,
> > const struct v4l2_flash_ops *ops,
> > @@ -638,7 +639,7 @@ struct v4l2_flash *v4l2_flash_init(
> > v4l2_flash->iled_cdev = iled_cdev;
> > v4l2_flash->ops = ops;
> > sd->dev = dev;
> > - sd->of_node = of_node ? of_node : led_cdev->dev->of_node;
> > + sd->fwnode = fwn ? fwn : dev_fwnode(led_cdev->dev);
>
> v4l2_subdev will only have a fwnode field in patch 3/8.
I think I've rearranged the set after writing the patch and missed the
dependency. I'll move it third unless there are other dependencies.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
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: [PATCH 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume
From: Linus Walleij @ 2017-04-07 10:19 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Thierry Reding, Jon Hunter, Rob Herring, Mark Rutland,
linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <58E6405E.5020402-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Apr 6, 2017 at 3:19 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On Thursday 06 April 2017 06:33 PM, Thierry Reding wrote:
>> On Thu, Apr 06, 2017 at 09:57:09AM +0100, Jon Hunter wrote:
>>> On 05/04/17 15:13, Laxman Dewangan wrote:
>>>>
>>>> +state of the system. The configuration of pin is provided via the
>>>> pinctrl
>>>> +DT node as detailed in the pinctrl DT binding document
>>>> + Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>>>> +
>>>> +The PWM node will have following optional properties.
>>>> +pinctrl-names: Pin state names. Must be "suspend" and "resume".
>>>
>>> Why not just use the pre-defined names here? There is a pre-defined name
>>> for "default", "idle" and "sleep" and then you can use the following
>>> APIs and avoid the lookup of the state ...
>>>
>>> pinctrl_pm_select_default_state()
>>> pinctrl_pm_select_idle_state()
>>> pinctrl_pm_select_sleep_state()
>>>
>>> Note for i2c [0][1], I used "default" as the active/on state (which I
>>> know is not that descriptive) and then used 'idle' as the suspended
>>> state. This way we don't need any custom names.
>>
>> Agreed, I think that's how these states are meant to be used.
>
> I did quick grep for the pinctrl_pm_select_* functions in the code tree and
> found usage of these APIs in some of the places.
> I am taking the reference of i2c-st, i2c-nomadic and
> extcon/extcon-usb-gpio.c drivers and from this the interpretation is
>
> default state: When interface active and transfer need to be done in IO
> interface.
> idle state: Active state of the system but interface is not active, put in
> non-active state of the interface.
> sleep state: When system entering into suspend and IO interface is going to
> be inactive.
>
> So in PWM case, we will need the "default" and "sleep" state.
>
> In suspend(), set the "sleep" state and in resume, set the "default" state.
>
> + Linus W as I refereed his st/nomadik driver for reference.
It is actually documented:
include/linux/pinctrl/pinctrl-state.h
/*
* Standard pin control state definitions
*/
/**
* @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put
* into as default, usually this means the pins are up and ready to
* be used by the device driver. This state is commonly used by
* hogs to configure muxing and pins at boot, and also as a state
* to go into when returning from sleep and idle in
* .pm_runtime_resume() or ordinary .resume() for example.
* @PINCTRL_STATE_INIT: normally the pinctrl will be set to "default"
* before the driver's probe() function is called. There are some
* drivers where that is not appropriate becausing doing so would
* glitch the pins. In those cases you can add an "init" pinctrl
* which is the state of the pins before drive probe. After probe
* if the pins are still in "init" state they'll be moved to
* "default".
* @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into
* when the pins are idle. This is a state where the system is relaxed
* but not fully sleeping - some power may be on but clocks gated for
* example. Could typically be set from a pm_runtime_suspend() or
* pm_runtime_idle() operation.
* @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into
* when the pins are sleeping. This is a state where the system is in
* its lowest sleep state. Could typically be set from an
* ordinary .suspend() function.
*/
#define PINCTRL_STATE_DEFAULT "default"
#define PINCTRL_STATE_INIT "init"
#define PINCTRL_STATE_IDLE "idle"
#define PINCTRL_STATE_SLEEP "sleep"
Yours,
Linus Walleij
--
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: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high
From: Laxman Dewangan @ 2017-04-07 10:14 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Rob Herring, Mark Rutland, Frank Rowand,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CACRpkdaK1ut-1=NtqqL15QuQV+P2n8XidNqVspOfu+7x=XMvog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Friday 07 April 2017 03:55 PM, Linus Walleij wrote:
> On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Currently, the GPIO interface is said to Open Drain if it is Single
>> Ended and active LOW. Similarly, it is said as Open Source if it is
>> Single Ended and active HIGH.
>>
>> The active HIGH/LOW is used in the interface for setting the pin
>> state to HIGH or LOW when enabling/disabling the interface.
>>
>> In Open Drain interface, pin is set to HIGH by putting pin in
>> high impedance and LOW by driving to the LOW.
>>
>> In Open Source interface, pin is set to HIGH by driving pin to
>> HIGH and set to LOW by putting pin in high impedance.
>>
>> With above, the Open Drain/Source is unrelated to the active LOW/HIGH
>> in interface. There is interface where the enable/disable of interface
>> is ether active LOW or HIGH but it is Open Drain type.
>>
>> Hence decouple the Open Drain with Single Ended + Active LOW and
>> Open Source with Single Ended + Active HIGH.
>>
>> Adding different flag for the Open Drain/Open Source which is valid
>> only when Single ended flag is enabled.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Patch applied.
>
> Good that you found this and fixed it before someone git hurt.
Thanks you very much.
Now we are going to use the descriptor based GPIO APIs as this is very
powerful and passing this info from DT is very useful.
--
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: [RFC] Documentation: pinctrl: Add "pinmux" property
From: Linus Walleij @ 2017-04-07 10:11 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Geert Uytterhoeven, Rob Herring, Mark Rutland,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux-Renesas,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1491474955-25605-1-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
On Thu, Apr 6, 2017 at 12:35 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
> Document "pinmux" property as part of generic pin controller
> documentation.
> Fix 2 minor typos in documentation while at there.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
Patch applied. This is documenting what is already the case
and is as such very helpful.
> Sending this one as RFC to collect feedback.
I just applied it. It is already much better than what we have.
If more adjustments are needed, people can send additional patches.
> If the change is not rejected is it
> worth adding to pin controller core helper functions to parse the newly
> documented property, as this commit
> <https://patchwork.kernel.org/patch/9411231/>
> did for "pinctrl-pin-array" one?
>
> All drivers using "pinmux" exhibit the same behavior which is fine as long as
> "pinmux" only accepts a list of u32 parameters.
>
> ...
> pins = of_find_property(node, "pinmux", NULL);
> ...
> npins = pins->length / sizeof(u32);
> ...
> of_property_read_u32_index(node, "pinmux",
> i, &pinfunc);
> ...
It makes sense. Just make sure to move users over to use the helpers.
Yours,
Linus Walleij
--
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: [PATCH v2 4/8] v4l: async: Provide interoperability between OF and fwnode matching
From: Laurent Pinchart @ 2017-04-07 10:07 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491484330-12040-5-git-send-email-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:06 Sakari Ailus wrote:
> OF and fwnode support are separated in V4L2 and individual drivers may
> implement one of them. Sub-devices do not match with a notifier
> expecting sub-devices with fwnodes, nor the other way around.
Shouldn't we instead convert all drivers to fwnode matching ? What's missing
after the mass conversion in patch 5/8 ?
> Fix this by checking for sub-device's of_node field in fwnode match and
> fwnode field in OF match.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 26 +++++++++++++++++++++++---
> include/media/v4l2-async.h | 2 +-
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 384ad5e..7f5d804 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -14,6 +14,7 @@
> #include <linux/list.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -40,15 +41,34 @@ static bool match_devname(struct v4l2_subdev *sd,
> return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
> }
>
> +static bool fwnode_cmp(struct fwnode_handle *one,
> + struct fwnode_handle *theother)
> +{
> + if (!one || !theother)
> + return false;
> +
> + if (one->type != theother->type)
> + return false;
> +
> + if (is_of_node(one))
> + return !of_node_cmp(of_node_full_name(to_of_node(one)),
> + of_node_full_name(to_of_node(theother)));
> + else
> + return one == theother;
> +}
> +
> static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> {
> - return !of_node_cmp(of_node_full_name(sd->of_node),
> - of_node_full_name(asd->match.of.node));
> + return fwnode_cmp(sd->of_node ?
> + of_fwnode_handle(sd->of_node) : sd->fwnode,
> + of_fwnode_handle(asd->match.of.node));
> }
>
> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> {
> - return sd->fwnode == asd->match.fwnode.fwn;
> + return fwnode_cmp(sd->of_node ?
> + of_fwnode_handle(sd->of_node) : sd->fwnode,
> + asd->match.fwnode.fwn);
> }
>
> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8f552d2..df8b682 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -57,7 +57,7 @@ struct v4l2_async_subdev {
> enum v4l2_async_match_type match_type;
> union {
> struct {
> - const struct device_node *node;
> + struct device_node *node;
That seems to be a bit of a hack :-( I'd rather make everything const and cast
to non-const pointers explicitly where the API requires us to. Or, better, add
a to_of_node_const() function.
> } of;
> struct {
> struct fwnode_handle *fwn;
--
Regards,
Laurent Pinchart
--
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: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
From: Linus Walleij @ 2017-04-07 10:07 UTC (permalink / raw)
To: Han, Nandor (GE Healthcare)
Cc: Alexandre Courbot, Rob Herring, Mark Rutland,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Malinen, Semi (GE Healthcare)
In-Reply-To: <HE1P101MB01883B516828F26921267A4FE60A0@HE1P101MB0188.NAMP101.PROD.OUTLOOK.COM>
On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
<nandor.han@ge.com> wrote:
> [Me]
>> > + /* bring the chip out of reset */
>> > + reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
>> > + if (IS_ERR(reset_gpio))
>> > + dev_warn(&spi->dev, "could not get reset-gpios\n");
>> > + else if (reset_gpio)
>> > + gpiod_put(reset_gpio);
>>
>> I don't think you should put it, other than in the remove()
>> function and in that case you need to have it in the
>> state container.
>
> Can you please be more explicit here.
>
> Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> I don't see how this could be done in remove() :)
If you issue gpiod_put() you release the GPIO hande so something else
can go in and grab the GPIO and assert the reset.
This is not what you want to make possible: you want to hold this gpiod handle
as long as the driver is running. devm_gpiod_get_optional() will do the
trick if you don't want to put the handle under explicit control.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 3/8] v4l: async: Add fwnode match support
From: Laurent Pinchart @ 2017-04-07 10:04 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-4-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
One more small comment below.
On Thursday 06 Apr 2017 16:12:05 Sakari Ailus wrote:
> Add fwnode matching to complement OF node matching. And fwnode may also be
> an OF node.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 12 ++++++++++++
> include/media/v4l2-async.h | 5 +++++
> include/media/v4l2-subdev.h | 3 +++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 96cc733..384ad5e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -46,6 +46,11 @@ static bool match_of(struct v4l2_subdev *sd, struct
> v4l2_async_subdev *asd) of_node_full_name(asd->match.of.node));
> }
>
> +static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> +{
> + return sd->fwnode == asd->match.fwnode.fwn;
> +}
> +
> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
> if (!asd->match.custom.match)
> @@ -80,6 +85,9 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier * case V4L2_ASYNC_MATCH_OF:
> match = match_of;
> break;
> + case V4L2_ASYNC_MATCH_FWNODE:
> + match = match_fwnode;
> + break;
> default:
> /* Cannot happen, unless someone breaks us */
> WARN_ON(true);
> @@ -158,6 +166,7 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev, case V4L2_ASYNC_MATCH_DEVNAME:
> case V4L2_ASYNC_MATCH_I2C:
> case V4L2_ASYNC_MATCH_OF:
> + case V4L2_ASYNC_MATCH_FWNODE:
> break;
> default:
> dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev :
NULL,
> @@ -282,6 +291,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> */
> if (!sd->of_node && sd->dev)
> sd->of_node = sd->dev->of_node;
> + if (!sd->fwnode && sd->dev)
> + sd->fwnode = sd->dev->of_node ?
> + &sd->dev->of_node->fwnode : sd->dev->fwnode;
>
> mutex_lock(&list_lock);
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8e2a236..8f552d2 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -32,6 +32,7 @@ struct v4l2_async_notifier;
> * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
> * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
> * @V4L2_ASYNC_MATCH_OF: Match will use OF node
> + * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
> *
> * This enum is used by the asyncrhronous sub-device logic to define the
> * algorithm that will be used to match an asynchronous device.
> @@ -41,6 +42,7 @@ enum v4l2_async_match_type {
> V4L2_ASYNC_MATCH_DEVNAME,
> V4L2_ASYNC_MATCH_I2C,
> V4L2_ASYNC_MATCH_OF,
> + V4L2_ASYNC_MATCH_FWNODE,
> };
>
> /**
> @@ -58,6 +60,9 @@ struct v4l2_async_subdev {
> const struct device_node *node;
> } of;
> struct {
> + struct fwnode_handle *fwn;
Shouldn't this be const ?
> + } fwnode;
> + struct {
> const char *name;
> } device_name;
> struct {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 0ab1c5d..5f1669c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -788,6 +788,8 @@ struct v4l2_subdev_platform_data {
> * @devnode: subdev device node
> * @dev: pointer to the physical device, if any
> * @of_node: The device_node of the subdev, usually the same as
> dev->of_node.
> + * @fwnode: The fwnode_handle of the subdev, usually the same as
> + * either dev->of_node->fwnode or dev->fwnode (whichever is non-
NULL).
> * @async_list: Links this subdev to a global subdev_list or
> @notifier->done * list.
> * @asd: Pointer to respective &struct v4l2_async_subdev.
> @@ -819,6 +821,7 @@ struct v4l2_subdev {
> struct video_device *devnode;
> struct device *dev;
> struct device_node *of_node;
> + struct fwnode_handle *fwnode;
> struct list_head async_list;
> struct v4l2_async_subdev *asd;
> struct v4l2_async_notifier *notifier;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 6/8] v4l: media/drv-intf/soc_mediabus.h: include dependent header file
From: Laurent Pinchart @ 2017-04-07 10:01 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491484330-12040-7-git-send-email-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:08 Sakari Ailus wrote:
> media/drv-intf/soc_mediabus.h does depend on struct v4l2_mbus_config which
> is defined in media/v4l2-mediabus.h. Include it.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Was this provided indirectly before, through v4l2-of.h perhaps ? If so,
shouldn't this patch be moved before 5/8 ? Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
> include/media/drv-intf/soc_mediabus.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/media/drv-intf/soc_mediabus.h
> b/include/media/drv-intf/soc_mediabus.h index 2ff7737..0449788 100644
> --- a/include/media/drv-intf/soc_mediabus.h
> +++ b/include/media/drv-intf/soc_mediabus.h
> @@ -14,6 +14,8 @@
> #include <linux/videodev2.h>
> #include <linux/v4l2-mediabus.h>
>
> +#include <media/v4l2-mediabus.h>
> +
> /**
> * enum soc_mbus_packing - data packing types on the media-bus
> * @SOC_MBUS_PACKING_NONE: no packing, bit-for-bit transfer to RAM, one
--
Regards,
Laurent Pinchart
--
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: [PATCH v2 7/8] docs-rst: media: Switch documentation to V4L2 fwnode API
From: Laurent Pinchart @ 2017-04-07 9:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491484330-12040-8-git-send-email-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:09 Sakari Ailus wrote:
> Instead of including the V4L2 OF header in ReST documentation, use the
> V4L2 fwnode header instead.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> Documentation/media/kapi/v4l2-core.rst | 2 +-
> Documentation/media/kapi/v4l2-fwnode.rst | 3 +++
> Documentation/media/kapi/v4l2-of.rst | 3 ---
> 3 files changed, 4 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/media/kapi/v4l2-fwnode.rst
> delete mode 100644 Documentation/media/kapi/v4l2-of.rst
>
> diff --git a/Documentation/media/kapi/v4l2-core.rst
> b/Documentation/media/kapi/v4l2-core.rst index e967715..1bc8a14 100644
> --- a/Documentation/media/kapi/v4l2-core.rst
> +++ b/Documentation/media/kapi/v4l2-core.rst
> @@ -19,7 +19,7 @@ Video2Linux devices
> v4l2-mc
> v4l2-mediabus
> v4l2-mem2mem
> - v4l2-of
> + v4l2-fwnode
I wonder whether we should keep this alphabetically sorted.
Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> v4l2-rect
> v4l2-tuner
> v4l2-common
> diff --git a/Documentation/media/kapi/v4l2-fwnode.rst
> b/Documentation/media/kapi/v4l2-fwnode.rst new file mode 100644
> index 0000000..6c8bccd
> --- /dev/null
> +++ b/Documentation/media/kapi/v4l2-fwnode.rst
> @@ -0,0 +1,3 @@
> +V4L2 fwnode kAPI
> +^^^^^^^^^^^^^^^^
> +.. kernel-doc:: include/media/v4l2-fwnode.h
> diff --git a/Documentation/media/kapi/v4l2-of.rst
> b/Documentation/media/kapi/v4l2-of.rst deleted file mode 100644
> index 1ddf76b..0000000
> --- a/Documentation/media/kapi/v4l2-of.rst
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -V4L2 Open Firmware kAPI
> -^^^^^^^^^^^^^^^^^^^^^^^
> -.. kernel-doc:: include/media/v4l2-of.h
--
Regards,
Laurent Pinchart
--
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: [PATCH v2 8/8] v4l: Remove V4L2 OF framework in favour of V4L2 fwnode framework
From: Laurent Pinchart @ 2017-04-07 9:58 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-9-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:10 Sakari Ailus wrote:
> All drivers have been converted from V4L2 OF to V4L2 fwnode. The V4L2 OF
> framework is now unused. Remove it.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/v4l2-core/Makefile | 3 -
> drivers/media/v4l2-core/v4l2-of.c | 327 -----------------------------------
> include/media/v4l2-of.h | 128 ---------------
> 3 files changed, 458 deletions(-)
> delete mode 100644 drivers/media/v4l2-core/v4l2-of.c
> delete mode 100644 include/media/v4l2-of.h
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 0/3] Add Mediatek CIRQ interrupt controller
From: Youlin Pei @ 2017-04-07 9:54 UTC (permalink / raw)
To: Marc Zyngier
Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper,
Mark Rutland, Russell King, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
hongkun.cao-NuS5LvNUpcJWk0Htik3J/w,
yong.wu-NuS5LvNUpcJWk0Htik3J/w, erin.lo-NuS5LvNUpcJWk0Htik3J/w
In-Reply-To: <771954fe-c994-96be-76ab-ece75383d058-5wv7dgnIgG8@public.gmane.org>
On Fri, 2017-04-07 at 09:52 +0100, Marc Zyngier wrote:
> On 07/04/17 09:06, Youlin Pei wrote:
> > In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to
> > works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC.
> >
> > The CIRQ controller is integrated in between MCUSYS and interrupt sources
> > as the second level interrupt controller. The external interrupts which
> > outside MCUSYS will feed through CIRQ then bypass to GIC.
> >
> > In normal mode(where MCUSYS is active), CIRQ is disabled and interrupts
> > will directly issue to MCUSYS. When MCUSYS enters sleep mode, where GIC
> > is power downed. CIRQ will be enabled and monitor all edge trigger
> > interrupts(only edge trigger interrupts will be lost in this scenario).
> > When an edge interrupt is triggered, CIRQ will record the status and
> > generated a pulse signal to GIC when flush command is executed.
> >
> > With CIRQ, MCUSYS can be completely turned off to improve the system
> > power consumption without losing interrupts.
> >
> > change in v4:
> > 1. add some comment to explain CIRQ suspend callback.
> > 2. rebase on 4.11
>
> Hi Youlin,
>
> I'm happy to take the first two patches through the irq tree. How do we
> deal with the third one? It seems to me that it'd be better routed via
> armsoc.
>
> Let me know what you and Matthias want to do.
Hi Marc,
Thanks for your review. I think that driver and dtsi can merged
separately.
Hi Matthias,
Could you help to review the dtsi patch?
Thanks a lot!
>
> Thanks,
>
> M.
--
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: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs
From: Richard Fitzgerald @ 2017-04-07 9:54 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Rob Herring, Thomas Gleixner, Jason Cooper,
Lee Jones, Mark Brown,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CACRpkdatoJOg1U218Q-NteRdz6B+w_yr1PWvnfa1P1EgGm7zug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, 2017-04-07 at 11:11 +0200, Linus Walleij wrote:
> On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> <rf-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>
> > This adds support for the GPIOs on Cirrus Logic Madera class codecs.
>
> A bit terse commit message, could you elaborate a bit on their
> specifics?
>
Sure.
> > .../devicetree/bindings/gpio/gpio-madera.txt | 24 +++
>
> Again should probably be a separate patch. Again, I don't care much
> as long as the DT people are happy.
>
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
> > @@ -0,0 +1,24 @@
> > +Cirrus Logic Madera class audio codecs gpio driver
> > +
> > +This is a subnode of the parent mfd node.
> > +
> > +See also the core bindings for the parent MFD driver:
> > +See Documentation/devicetree/bindings/mfd/madera.txt
> > +
> > +Required properties:
> > + - compatible : must be "cirrus,madera-gpio"
> > + - gpio-controller : Indicates this device is a GPIO controller.
> > + - #gpio-cells : Must be 2. The first cell is the pin number. The second cell
> > + is reserved for future use and must be zero
> > +
> > +Example:
> > +
> > +codec: cs47l85@0 {
> > + compatible = "cirrus,cs47l85";
> > +
> > + gpio {
> > + compatible = "cirrus,madera-gpio";
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + }
>
> Maybe you want to use the gpio-line-names = ; property in the example
> to show how nice it is to name the lines?
>
I'll take a look at that.
> > +config GPIO_MADERA
> > + tristate "Cirrus Logic Madera class codecs"
> > + depends on MFD_MADERA
> > + help
> > + Support for GPIOs on Cirrus Logic Madera class codecs.
>
> I wonder if you should not depend on the pin controller instead.
> It seems closer and also likely to act as a back-end for the
> GPIOs.
>
> > +static int madera_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
> > + struct madera *madera = madera_gpio->madera;
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(madera->regmap,
> > + MADERA_GPIO1_CTRL_1 + (2 * offset), &val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (val & MADERA_GP1_LVL_MASK)
> > + return 1;
> > + else
> > + return 0;
>
> Just do this:
>
> return !!(val & MADERA_GP1_LVL_MASK);
>
Ok. Personally I like the clarity of the more verbose version rather
than the !! but I can change it.
> > +static struct gpio_chip template_chip = {
> > + .label = "madera",
> > + .owner = THIS_MODULE,
> > + .direction_input = madera_gpio_direction_in,
> > + .get = madera_gpio_get,
> > + .direction_output = madera_gpio_direction_out,
> > + .set = madera_gpio_set,
> > + .can_sleep = true,
> > +};
>
> - Implement .get_direction()
>
Ok
> Also consider implementing:
>
> - request/free/set_config looking like this:
>
> .request = gpiochip_generic_request,
> .free = gpiochip_generic_free,
> .set_config = gpiochip_generic_config,
>
> If you also implement the corresponding
> .pin_config_set in struct pinconf_ops and
> .gpio_request_enable() and .gpio_disable_free()
> in struct pinmux_ops, you get a pin control back-end
> that will mux in the pins to GPIO mode if they are wrong
> set, and also set up debounce and/or open drain for the
> GPIO line using the standard GPIO callbacks with pin
> control as a back-end.
>
> If you also specify "strict" in struct pinmux_ops you block
> the collisions between users of GPIO and other functions
> in the pin control driver.
>
> (Please go back and look at your pin control driver
> for this.)
>
I'll take a look at these things.
> Example driver using pin control as GPIO back-end:
> drivers/pinctrl/intel/pinctrl-intel.c
>
> Other than this it looks fine.
>
> Yours,
> Linus Walleij
--
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: [PATCH 3/3] ARM: dts: ARTPEC-6: Add pinctrl configuration
From: Linus Walleij @ 2017-04-07 9:51 UTC (permalink / raw)
To: Jesper Nilsson
Cc: Jesper Nilsson, Lars Persson, Niklas Cassel, Rob Herring,
Mark Rutland, Russell King, linux-arm-kernel-VrBV9hrLPhE,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170330113349.GG29118-VrBV9hrLPhE@public.gmane.org>
On Thu, Mar 30, 2017 at 1:33 PM, Jesper Nilsson <jesper.nilsson-VrBV9hrLPhE@public.gmane.org> wrote:
> Enable the pinctrl driver for ARTPEC-6 in the artpec6.dtsi
> with all main pinmux functions.
> Add pinctrl information to the relevant (uart) nodes.
>
> Signed-off-by: Jesper Nilsson <jesper.nilsson-VrBV9hrLPhE@public.gmane.org>
Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Please get this applied through the ARM SoC tree.
Yours,
Linus Walleij
--
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