* default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support)
From: Pavel Machek @ 2017-04-07 11:57 UTC (permalink / raw)
To: Felix Brack
Cc: Jacek Anaszewski, rpurdie-Fm38FmjxZ/leoWH0uzbU5w,
mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0,
linux-leds-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, robh-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <ec012568-f540-4e87-d8c4-4065fbcd1c2a-GovowT2ENgg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]
Hi!
> >> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
> >> be documented.
> >>
> >> Plus... is it useful to have default-state? We already have default
> >> trigger. If we keep the value by default (on PC, we do something like
> >> that) this patch should not be neccessary?
> >
> > Thanks for the heads-up. Dropping the patch for now.
>
> No, please do not drop the patch.
>
> > I guess that pwm0/1 got propagated to v2 by an omission.
>
> Yes, I agree. However the two strings do not break anything and behave
> analog to the 'on' or 'keep' string. Though this code could be removed
> if absolutely necessary. An alternative would be to add a description
> for the strings. Just to be clear: these strings have nothing to with
> the exposition of device specific registers to the DT.
Well, they should be removed. No need to keep surprises in code.
> > Regarding default-on: Felix, do you have any use case that require
> > default-on set to "keep"?
>
> This patch is not about 'default-on' which is a value that could be
> assigned to the property 'linux,default-trigger' (according to DT
> bindings documentation file 'common.txt').
> My patch does not introduce anything new with the'keep' state, it rather
> completes the existing bindings according to the description in
> Documentation/devicetree/bindings/leds/common.txt which states:
Hmm, that was introduce by 1d1a77ddc8acfa3d506f1958e09a12085e71fc69
. I don't quite like the patch -- it duplicates default-trigger
functionality without good reason.
IMO we should
1) keep the led state by default
2) deprecate the default-state property
3) use default-triggers to emulate the functionality if someone really
needs it.
> One of my use cases is to turn a LED on by U-Boot. This LED must remain
> on until eventually, under certain conditions, some userland code
> changes it's state.
> Setting 'default-state' to 'keep' is how you can sort of tell the
> kernel, or better the driver, 'not to initialize the LED' which would
> turn it off.
Yes, that's reasonable. Perhaps 'keep' should be the default.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH] iio: stm32 trigger: fix sampling_frequency read
From: Fabrice Gasnier @ 2017-04-07 11:53 UTC (permalink / raw)
To: jic23-DgEjT+Ai2ygdnm+yROfE0A, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
alexandre.torgue-qxv4g6HH51o, lars-Qo5EllUWu/uELgA04lAiVw,
knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
fabrice.gasnier-qxv4g6HH51o,
benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A,
benjamin.gaignard-qxv4g6HH51o
When prescaler (PSC) is 0, it means div factor is 1: counter clock
frequency is equal to input clk / (PSC + 1).
When reload value is 8 for example, counter counts 9 cycles, from 0 to 8.
This is handled in frequency write routine, by writing respectively:
- prescaler - 1 to PSC
- reload value - 1 to ARR
This fix does the opposite when reading the frequency from PSC and ARR:
- prescaler is PSC + 1
- reload value is ARR + 1
Thus, PSC may be 0, depending on requested sampling frequency (div 1).
In this case, reading freq wrongly reports 0, instead of computing and
reporting correct value.
Remove test on !psc and !arr.
Small test on stm32f4 (example on tim1_trgo), before this fix:
$ cd /sys/bus/iio/devices/triggerX
$ echo 10000 > sampling_frequency
$ cat sampling_frequency
0
After this fix:
$ echo 10000 > sampling_frequency
$ cat sampling_frequency
10000
Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
---
drivers/iio/trigger/stm32-timer-trigger.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
index 994b96d..5ee362b 100644
--- a/drivers/iio/trigger/stm32-timer-trigger.c
+++ b/drivers/iio/trigger/stm32-timer-trigger.c
@@ -152,10 +152,10 @@ static ssize_t stm32_tt_read_frequency(struct device *dev,
regmap_read(priv->regmap, TIM_PSC, &psc);
regmap_read(priv->regmap, TIM_ARR, &arr);
- if (psc && arr && (cr1 & TIM_CR1_CEN)) {
+ if (cr1 & TIM_CR1_CEN) {
freq = (unsigned long long)clk_get_rate(priv->clk);
- do_div(freq, psc);
- do_div(freq, arr);
+ do_div(freq, psc + 1);
+ do_div(freq, arr + 1);
}
return sprintf(buf, "%d\n", (unsigned int)freq);
--
1.9.1
--
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 related
* Re: [PATCH v2] ARM: dts: imx: add Gateworks Ventana GW5903 support
From: Shawn Guo @ 2017-04-07 11:30 UTC (permalink / raw)
To: Tim Harvey
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491254301-14574-1-git-send-email-tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
On Mon, Apr 03, 2017 at 02:18:21PM -0700, Tim Harvey wrote:
> The Gateworks Ventana GW5903 is a single-board computer based on the NXP
> IMX6 SoC with the following features:
> * IMX6 DualLite Soc (supports IMX6S,IMX6DL,IMX6Q)
> * 4GiB DDR3 DRAM
> * 32GB eMMC
> * 1x microSD connector
> * Gateworks System Controller:
> - hardware watchdog
> - hardware monitor
> - pushbutton controller
> - EEPROM storage
> - power control
> * JTAG programmable
> * Inertial Module
> * uBlox EMMY-W1 (bluetooth/nfc/802.11ac)
>
> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
Applied, thanks.
--
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 4/8] regulator: core: Check enabling bypass respects constraints
From: Mark Brown @ 2017-04-07 11:22 UTC (permalink / raw)
To: Leonard Crestez
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,
linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <1491562312.9365.41.camel@nxp.com>
[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]
On Fri, Apr 07, 2017 at 01:51:52PM +0300, Leonard Crestez wrote:
> It currently seems to work how I expect but from your statement it's
> not clear if it's entirely intentional.
The current behaviour of bypassed regulators is intentional.
> > > 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.
No, that's not expected either - one or the other is fine but both isn't
expected. What you're looking for here is something logically different
to bypass even if the hardware implementation ends up the same.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs
From: Mark Brown @ 2017-04-07 11:14 UTC (permalink / raw)
To: Linus Walleij
Cc: Charles Keepax, Richard Fitzgerald, Alexandre Courbot,
Rob Herring, Thomas Gleixner, Jason Cooper, Lee Jones,
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: <CACRpkdZLfL+dxMN-uaRp57u4yW4cLfLFoZVkNWFZL4eECOyR1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]
On Fri, Apr 07, 2017 at 11:12:53AM +0200, Linus Walleij wrote:
> On Fri, Apr 7, 2017 at 10:48 AM, Charles Keepax
> > On Fri, Apr 07, 2017 at 10:30:12AM +0200, Linus Walleij wrote:
> >> > Get included in all subdrivers I suppose?
> >> > So you are broadcasting 8800+ lines into every subdriver across the
> >> > entire kernel.
> >> > Just the time spent in the preprocessor parsing this will affect compilation
> >> > time.
> >> Or maybe this is a necessary sacrifice to get the regmap cache
> >> centralized in MFD. I don't know. I feel stupid.
> >> I guess I should focus on "my" subsystems...
> > This only gets included in files that are part of this driver, it
> > shouldn't affect compilation time for anyone not building the
> > madera driver and even then it should only affect compilation
> > times for the 10 or so C files that make up the driver. Also I
> > don't really see any other way to specify the registers for the
> > device.
> No when using regmap cache this seems necessary.
> I was just wrong.
Right. The other thing to bear in mind with things like this is that
often if it's the chip vendor they've got mechanisms to generate the
register definitions directly from the chip design so if we take the
complete thing with minimal edits it's a great way of ensuring we've got
information about the chip in the upstream kernel to support future
development.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 5/8] v4l: Switch from V4L2 OF not V4L2 fwnode API
From: Laurent Pinchart @ 2017-04-07 11:09 UTC (permalink / raw)
To: Sakari Ailus
Cc: Sakari Ailus, linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170407105805.GG4192-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
Hi Sakari,
On Friday 07 Apr 2017 13:58:06 Sakari Ailus wrote:
> On Fri, Apr 07, 2017 at 01:32:54PM +0300, Laurent Pinchart wrote:
> > 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > Acked-by: Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org> # 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?
I will obviously need either OF or ACPI to use the fwnode API, but some
drivers still support platform data (either on non-OF embedded systems, or
when the I2C device is part of a PCI card for instance). Compile-testing is
also a use case I'm concerned about.
[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?
I'd name is vep or 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.
My ACPI knowledge is limited, but don't ACPI nodes have 4 character names that
can be combined in a string to create a full path ?
[snip]
> > > -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.
When both are needed in the same function it certainly makes sense to use more
detailed names. My point is that, in a function that exclusively processes
fw*, I find calling variables fwn or vfwep more confusing that calling them
node or endpoint.
> > > notifier->subdevs = devm_kcalloc(
> > >
> > > dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
[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?
See above.
> > > + v4l2_fwnode_put_link(&link);
> > >
> > > ret = -EINVAL;
> > > break;
> > >
> > > }
> >
> > [snip]
--
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 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
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