Re: The RZ/A1 pin controller driver will be broken for 4.13
Hi Geert, Chris,
On Wed, Aug 30, 2017 at 10:26:30AM +0200, Geert Uytterhoeven wrote:
> CC Timur, LinusW, gpio
>
> On Tue, Aug 29, 2017 at 8:56 PM, Chris Brandt
> wrote:
> > Just FYI,
> >
> > After pulling Geert's new renesas-drivers-2017-08-29-v4.13-rc7, I tried
> > testing RZ/A1 and it hangs during boot.
> >
> > Basically...
> >
> > [ 110.329579] pinctrl-rza1 fcfe3000.pin-controller: Parsed gpiochip
> > gpio-0-0 with 6 pins
> > [ 112.970056] pinctrl-rza1 fcfe3000.pin-controller: Parsed gpiochip
> > gpio-1-1 with 16 pins
> > (hangs here forever)
> >
> >
> > After some debug, I found that this commit broke the new
> > drivers/pinctrl/pinctrl-rza1.c driver.
> >
> > 108d23e322a2 ("gpiolib: request the gpio before querying its direction")
> >
> > Looks like it was added for -rc5.
> >
> > If I revert that one patch, RZ/A1 boots fine.
> >
> > Since we're at -rc7, I probably won't have time to fix it before the
> > release.
>
> It's not in v4.13-rc5, only in -next, for v4.14.
> So v4.13 will be fine, and we still have plenty of time to fix the issue.
After some investigations, it turns out some register settings
performed during pin_reset() are invalid and hang the system BUT only
when performed on Port_9.
Specifically, setting PMC and PIPC registers to 0 (default initial state)
on Port_9, hangs the system.
There is no mention (none that I've found) in the processor manual of
specific procedures to be performed when resetting that port to its
initial state, so I guess this may either be an indication of
something else going wrong (like writing to some invalid memory
address) or it's an HW issue not yet documented.
Timur's patch does trigger a "reset" on all Ports during gpiochip
registration , while before that patch made into -next branch, reset
was only triggered when actually requesting a gpio or when performing
muxing on a pin.
Apparently, we never multiplexed any pin on Port_9 nor tested gpio
functionalities on that port before.
This:
@@ -487,8 +491,10 @@ static void rza1_pin_reset(struct rza1_port *port,
unsigned int pin)
rza1_set_bit(port, RZA1_PBDC_REG, pin, 0);
rza1_set_bit(port, RZA1_PM_REG, pin, 1);
- rza1_set_bit(port, RZA1_PMC_REG, pin, 0);
- rza1_set_bit(port, RZA1_PIPC_REG, pin, 0);
+ if (port->id != 9) {
+ rza1_set_bit(port, RZA1_PMC_REG, pin, 0);
+ rza1_set_bit(port, RZA1_PIPC_REG, pin, 0);
+ }
spin_unlock_irqrestore(&port->lock, irqflags);
}
Fixes the issue, but I'm not that satisfied with this as I would like
to know if the same issue happens on other processors of the RZ/A1 family
other than RZ/A1H before proposing this as a proper fix.
Chris, do you have RZ/A1* devices where to test this?
Thanks
j
>
> > Poor RZ/A1 pinctrl driver...it took so long to get into mainline, and
> > then it only worked for 1 release.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH 3/4] arm: dts: gr-peach: Add ETHER pin group
Hi Simon,
On Wed, Aug 30, 2017 at 09:25:42AM +0200, Simon Horman wrote:
> On Thu, Aug 24, 2017 at 02:53:59PM +0200, jmondi wrote:
> > Thanks Geert,
> >
> > On Thu, Aug 24, 2017 at 01:56:16PM +0200, Geert Uytterhoeven wrote:
> > > Hi Jacopo,
> > >
> >
> > [snip]
> >
> > > > I haven't find any mention in device tree bindings documentation of a
> > > > "reset-gpio" property for sh_eth, in the code examples I've seen in
> > > > u-boot and mbed, the interface is reset before any actual
> > > > configuration is performed. I feel like that should be the place where
> > > > that gpio is requested and cycled...
> > >
> > > Documentation/devicetree/bindings/net/mdio.txt says
> > >
> > > These are generic properties that can apply to any MDIO bus.
> > >
> >
> > I have now used mdio defined generic properties
> >
> > ðer {
> > pinctrl-names = "default";
> > pinctrl-0 = <ðer_pins>;
> >
> > status = "okay";
> >
> > reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
> > reset-delay-us = <5>;
> >
> > renesas,no-ether-link;
> > phy-handle = <&phy0>;
> > phy0: ethernet-phy@0 {
> >reg = <0>;
> > };
> > };
> >
> > I see the gpio being cycled, but same results as before: device gets
> > probed, address set, but I cannot ping, device gets probed, address
> > gets set, but I cannot ping
> >
> > --- target ---
> > [0.00] libphy: sh_mii: probed
> > [0.00] sh-eth e8203000.ethernet eth0: Base address at 0xe8203000,
> > 68:14:97:20:97:00, IRQ 22.
> >
> > # ifconfig eth0 192.168.100.50
> > [0.00] Generic PHY e8203000.ethernet-:00: attached PHY
> > driver [Generic PHY] (mii_bus:phy_addr=e8203000.ethernet-f)
> >
> > --- host ---
> > $ping 192.168.100.50
> > PING 192.168.100.50 (192.168.100.50) 56(84) bytes of data.
> > >From 192.168.100.18 icmp_seq=1 Destination Host Unreachable
> > >From 192.168.100.18 icmp_seq=1 Destination Host Unreachable
> > >From 192.168.100.18 icmp_seq=1 Destination Host Unreachable
> >
> >
> > Let's leave this out of the DTS series I've just sent for now, ok?
>
> I'm a little confused by this. Am I right in thinking that you don't want
> this patch applied at this time and may resubmit it or an updated version
> later? With that understanding I have marked it as "Rejected" for now. Feel
> free to resubmit when you are ready.
Yes please, you got me right here.
Even if pix muxing is performed "correctly", and I can set ip address
and probe the interface, not traffic can be exchanged on it.
For this reason, let's not enable it at this time
Is this fine?
Re: The RZ/A1 pin controller driver will be broken for 4.13
Hi Chris,
thanks for bisecting this down to the problem
On Tue, Aug 29, 2017 at 06:56:53PM +, Chris Brandt wrote:
> Just FYI,
>
> After pulling Geert's new renesas-drivers-2017-08-29-v4.13-rc7, I tried
> testing RZ/A1 and it hangs during boot.
>
> Basically...
>
> [ 110.329579] pinctrl-rza1 fcfe3000.pin-controller: Parsed gpiochip gpio-0-0
> with 6 pins
> [ 112.970056] pinctrl-rza1 fcfe3000.pin-controller: Parsed gpiochip gpio-1-1
> with 16 pins
> (hangs here forever)
>
>
> After some debug, I found that this commit broke the new
> drivers/pinctrl/pinctrl-rza1.c driver.
>
> 108d23e322a2 ("gpiolib: request the gpio before querying its direction")
>
> Looks like it was added for -rc5.
>
> If I revert that one patch, RZ/A1 boots fine.
>
> Since we're at -rc7, I probably won't have time to fix it before the
> release.
I tried -rc1 on a Peach, not -rc5 :(
I will have a look into that and see how bad this is
>
> Poor RZ/A1 pinctrl driver...it took so long to get into mainline, and
> then it only worked for 1 release.
>
Yeah, that's definitely bad karma for this platform
Thanks
j
> :(
>
>
> Chris
>
Re: [PATCH 3/4] arm: dts: gr-peach: Add ETHER pin group
Thanks Geert,
On Thu, Aug 24, 2017 at 01:56:16PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
[snip]
> > I haven't find any mention in device tree bindings documentation of a
> > "reset-gpio" property for sh_eth, in the code examples I've seen in
> > u-boot and mbed, the interface is reset before any actual
> > configuration is performed. I feel like that should be the place where
> > that gpio is requested and cycled...
>
> Documentation/devicetree/bindings/net/mdio.txt says
>
> These are generic properties that can apply to any MDIO bus.
>
I have now used mdio defined generic properties
ðer {
pinctrl-names = "default";
pinctrl-0 = <ðer_pins>;
status = "okay";
reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
reset-delay-us = <5>;
renesas,no-ether-link;
phy-handle = <&phy0>;
phy0: ethernet-phy@0 {
reg = <0>;
};
};
I see the gpio being cycled, but same results as before: device gets
probed, address set, but I cannot ping, device gets probed, address
gets set, but I cannot ping
--- target ---
[0.00] libphy: sh_mii: probed
[0.00] sh-eth e8203000.ethernet eth0: Base address at 0xe8203000,
68:14:97:20:97:00, IRQ 22.
# ifconfig eth0 192.168.100.50
[0.00] Generic PHY e8203000.ethernet-:00: attached PHY driver
[Generic PHY] (mii_bus:phy_addr=e8203000.ethernet-f)
--- host ---
$ping 192.168.100.50
PING 192.168.100.50 (192.168.100.50) 56(84) bytes of data.
>From 192.168.100.18 icmp_seq=1 Destination Host Unreachable
>From 192.168.100.18 icmp_seq=1 Destination Host Unreachable
>From 192.168.100.18 icmp_seq=1 Destination Host Unreachable
Let's leave this out of the DTS series I've just sent for now, ok?
Thanks
j
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH 3/4] arm: dts: gr-peach: Add ETHER pin group
Hi Geert,
On Thu, Aug 24, 2017 at 11:48:14AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Aug 24, 2017 at 10:48 AM, Jacopo Mondi
> wrote:
> > Add pin configuration subnode for ETHER pin group.
> > The interface can be configured and probed, but no traffic can be
> > transmitted or received.
> >
> > Signed-off-by: Jacopo Mondi
> >
> > ---
> > When in u-boot console I can ping a connected host, after the
> > system has booted I can configure an ip address on the interface but
> > cannot exchange any traffic.
> > I have compared the pin configuration procedure with the u-boot
> > implemented one and some sketches from mbed operating system libraries,
> > the configured pins are correct and registers values seems to match.
> > Not sure if this patch should be sent for inclusion but sending it out
> > anyway for you to judge this.
> >
> > Thanks
> >j
> > ---
> > arch/arm/boot/dts/r7s72100-gr-peach.dts | 35
> > +
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > b/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > index bcfa644..5ce558f 100644
> > --- a/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > +++ b/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > @@ -58,6 +58,28 @@
> > /* P6_2 as RxD2; P6_3 as TxD2 */
> > pinmux = , ;
> > };
> > +
> > + ether_pins: ether {
> > + /* Ethernet on Ports 1,3,5,10 */
> > + pinmux = , /* P1_14 = ET_COL */
> > +, /* P3_0 = ET_TXCLK */
> > +, /* P3_3 = ET_MDIO */
> > +, /* P3_4 = ET_RXCLK */
> > +, /* P3_5 = ET_RXER */
> > +, /* P3_6 = ET_RXDV */
> > +, /* P5_9 = ET_MDC*/
> > +, /* P10_1 = ET_TXER */
> > +, /* P10_2 = ET_TXEN */
> > +, /* P10_3 = ET_CRS */
> > +, /* P10_4 = ET_TXD0 */
> > +, /* P10_5 = ET_TXD1 */
> > +, /* P10_6 = ET_TXD2 */
> > +, /* P10_7 = ET_TXD3 */
> > +, /* P10_8 = ET_RXD0 */
> > +, /* P10_9 = ET_RXD1 */
> > +,/* P10_10 = ET_RXD2 */
> > +;/* P10_11 = ET_RXD3 */
>
> All OK, but do you need P4_2, which is used for ET_nRST?
I tried requesting the GPIO in the "ether" subnode and define it as
"active low", so that it is kept high during regular operations.
I have verified through register writes dump it is cycled just after
the gpio is requested, and this should reset the interface before the
actual sh_eth driver kicks in.
I haven't find any mention in device tree bindings documentation of a
"reset-gpio" property for sh_eth, in the code examples I've seen in
u-boot and mbed, the interface is reset before any actual
configuration is performed. I feel like that should be the place where
that gpio is requested and cycled...
Thanks
j
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v1 03/12] media: i2c: mt9m111: Skip chid identification
Hi Laurent,
On Tue, Jun 20, 2017 at 10:48:21AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Monday 19 Jun 2017 19:04:40 Jacopo Mondi wrote:
> > Reads of chip identification code (both on registers 0x00 and 0xff)
> > always return 0x00.
>
> This shouldn't be the case. It might mean that your I2C master controller
> doesn't handle reads correctly. I don't think this patch is correct.
Indeed this patch was not meant for inclusion (as it is part of an RFC series)
but I still fail to read the chip identifier even when using a tool to
sniff the bus or talk directly to the chip (I'm using bus pirate
for this purpose).
What puzzles me is that I get back reads from some registers that match the
defaults reported by the data sheet, while other registers return
meaningful values but different from what I have indicated as defaults
in the sensor manual, while chip identifier reads always back as 0x00.
I am pasting some examples down here of some interactions with the
chip using bus pirate serial control console [1]
I wonder if I am working with a different chip revision from the one
the driver and the datasheet have as reference, but it really seems
unlikely to me..
Thanks
j
[1]
** Read register 0x106: expected value 0x700E -> got it!
I2C>[0x90+0xf0+0x00+0x1]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0xF0 ACK
WRITE: 0x00 ACK
WRITE: 0x01 ACK
I2C STOP BIT
I2C>[0x90+0x06+[0x91+rr]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0x06 ACK
I2C START BIT
WRITE: 0x91 ACK
READ: 0x70
READ: ACK 0x0E
NACK
I2C STOP BIT
** Read register 0x108: expected value 0x0080 -> got 0xc800
I2C>[0x90+0xf0+0x00+0x1]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0xF0 ACK
WRITE: 0x00 ACK
WRITE: 0x01 ACK
I2C STOP BIT
I2C>[0x90+0x08+[0x91+rr]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0x08 ACK
I2C START BIT
WRITE: 0x91 ACK
READ: 0xC8
READ: ACK 0x00
NACK
I2C STOP BIT
** Read Chip ID: expected value 0x143a -> got 0x
I2C>[0x90+0xf0+0x00+0x0]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0xF0 ACK
WRITE: 0x00 ACK
WRITE: 0x00 ACK
I2C STOP BIT
I2C>[0x90+0x00+[0x91+rr]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0x00 ACK
I2C START BIT
WRITE: 0x91 ACK
READ: 0x00
READ: ACK 0x00
NACK
I2C STOP BIT
>
> > Skip chip identification to have the device complete probing.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/i2c/mt9m111.c | 19 ---
> > 1 file changed, 19 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 72e71b7..8e86d51 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -890,31 +890,12 @@ static struct v4l2_subdev_ops mt9m111_subdev_ops = {
> > static int mt9m111_video_probe(struct i2c_client *client)
> > {
> > struct mt9m111 *mt9m111 = to_mt9m111(client);
> > - s32 data;
> > int ret;
> >
> > ret = mt9m111_s_power(&mt9m111->subdev, 1);
> > if (ret < 0)
> > return ret;
> >
> > - data = reg_read(CHIP_VERSION);
> > -
> > - switch (data) {
> > - case 0x143a: /* MT9M111 or MT9M131 */
> > - dev_info(&client->dev,
> > - "Detected a MT9M111/MT9M131 chip ID %x\n", data);
> > - break;
> > - case 0x148c: /* MT9M112 */
> > - dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n",
> data);
> > - break;
> > - default:
> > - dev_err(&client->dev,
> > - "No MT9M111/MT9M112/MT9M131 chip detected register
> read %x\n",
> > - data);
> > - ret = -ENODEV;
> > - goto done;
> > - }
> > -
> > ret = mt9m111_init(mt9m111);
> > if (ret)
> > goto done;
>
> --
> Regards,
>
> Laurent Pinchart
>
Re: [PATCH v6 2/8] dt-bindings: pinctrl: Add RZ/A1 bindings doc
Hi Rob, On Thu, Jun 22, 2017 at 04:09:17PM -0500, Rob Herring wrote: > On Thu, Jun 22, 2017 at 04:54:30PM +0200, Jacopo Mondi wrote: > > Add device tree bindings documentation for Renesas RZ/A1 gpio and pin > > controller. > > > > Signed-off-by: Jacopo Mondi > > --- > > .../bindings/pinctrl/renesas,rza1-pinctrl.txt | 221 > > + > > 1 file changed, 221 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt > > Please add acks when posting new versions. It seems there are 2 more > lines here, but I have no idea what changed. I'm assuming it was minor > enough to retain my ack. I have dropped your Acked-by in v4, as compared to the previous versions bindings changed "significantly". Quoting the v4 cover letter: "The device tree bindings changed significantly, and as anticipated I have not incorporated Rob's ack as I would like him to have a look there again." Now that I look at bindings again, I guess I could have kept it because all of the changes are related to pin controller subsystem specificities and are not that "significant" in the end.. I'll list them here for your reference - use "pinmux" in place of "renesas,pins" property - use generic properties in place of pin mux flags - a bit of shuffling on generic properties in v4->v6 transition If none of these is relevant to you, we can add you Acked-by back when sending pull-request (right Geert?) Thanks j > > Rob
Re: [PATCH v1 09/12] media: rcar: vin: Install notifier for digital input
Hi Niklas,
On Mon, Jun 19, 2017 at 09:51:15PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2017-06-19 19:04:46 +0200, Jacopo Mondi wrote:
> > Install async notifier for digital input on Gen3 when no other CSI-2
> > input has been found connected.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 23 +--
> > 1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 78ca232..6e5d84a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -866,17 +866,28 @@ static int rvin_group_graph_init(struct rvin_dev *vin)
> > }
> >
> > i = 0;
> > - for_each_set_bit(bit, &bitmap, RVIN_CSI_MAX) {
> > - subdevs[i++] = &vin->group->csi[bit].asd;
> > + for_each_set_bit(bit, &bitmap, RVIN_INPUT_MAX) {
> > + if (bit < RVIN_CSI_MAX)
> > + subdevs[i++] = &vin->group->csi[bit].asd;
> > + else if (bit == RVIN_PARALLEL_IN) {
> > + subdevs[0] = &vin->digital.asd;
> > + vin->notifier.num_subdevs = 1;
> > + }
> > }
> >
> > vin_dbg(vin, "Claimed %d subdevices for group\n", count);
> >
> > - vin->notifier.num_subdevs = count;
> > vin->notifier.subdevs = subdevs;
> > - vin->notifier.bound = rvin_group_notify_bound;
> > - vin->notifier.unbind = rvin_group_notify_unbind;
> > - vin->notifier.complete = rvin_group_notify_complete;
> > + if (!vin->notifier.num_subdevs) {
> > + vin->notifier.num_subdevs = count;
> > + vin->notifier.bound = rvin_group_notify_bound;
> > + vin->notifier.unbind = rvin_group_notify_unbind;
> > + vin->notifier.complete = rvin_group_notify_complete;
> > + } else {
> > + vin->notifier.bound = rvin_digital_notify_bound;
> > + vin->notifier.unbind = rvin_digital_notify_unbind;
> > + vin->notifier.complete = rvin_digital_notify_complete;
> > + }
>
> Hum, if there is a digital subdevice you ignore to look for CSI-2
> devices? This feels wrong, it probably works (for now) since all CSI-2
> subdevs are found by VIN0 since it's probed first and the digital input
> is only valid for VIN4 and VIN5 right? I hope to remedy this before the
> VIN Gen3 code is ready for upstream and the goal is that each VIN should
> look for the subdevices that effects it self. With that change this will
> break :-(
This is something I probably did not get: I thought parallel input and
CSI-2 input where mutually exclusive. In particular, looking at chsel
tables (from Table 26.14 on), where routing between CSI inputs and VIN
channel is described, I did not get how parallel input fit in that
picture. My best assumption now is that when a channel is NC it means
it can be used for digital input?
>
> >
> > mutex_unlock(&vin->group->lock);
> >
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund
Re: [PATCH v1 05/12] media: rcar: vin: Prepare to parse Gen3 digital input
Hi Niklas,
thanks for review
On Mon, Jun 19, 2017 at 09:39:46PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2017-06-19 19:04:42 +0200, Jacopo Mondi wrote:
> > Support parsing digital input on configurable port id and reg
> > properties. Also make the function return -ENOENT when no subdevice is
> > found.
> > This change is needed to support parsing digital input on Gen3.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 22 --
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 175f138..ef61bcc 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct
> > v4l2_async_notifier *notifier,
> > return 0;
> > }
> >
> > -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> > +static int rvin_digital_graph_parse(struct rvin_dev *vin,
> > + unsigned int port,
> > + unsigned int reg)
> > {
> > struct device_node *ep, *np;
> > int ret;
> > @@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev
> > *vin)
> > vin->digital.asd.match.fwnode.fwnode = NULL;
> > vin->digital.subdev = NULL;
> >
> > - /*
> > -* Port 0 id 0 is local digital input, try to get it.
> > -* Not all instances can or will have this, that is OK
> > -*/
> > - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> > + ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg);
>
> I don't think it's necessary to supply the port and reg from the caller.
> If the DT proposed in rcar_vin.txt is used.
>
> > if (!ep)
> > - return 0;
> > + return -ENOENT;
>
> I'm not saying this is wrong, but why do you change this? I can't see a
> clear advantage in doing so. And if do it I think it should be done in a
> separate patch explaining why.
For both questions, as I parse entries in port@2 and
rvin_digital_notify_bound was intended to use port@0 by default I made
port and reg parameters. I use -ENOENT to distinguish the case where
no endpoint has been found
>
> >
> > np = of_graph_get_remote_port_parent(ep);
> > if (!np) {
> > @@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev
> > *vin)
> > if (ret)
> > return ret;
> >
> > - ret = rvin_digital_graph_parse(vin);
> > - if (ret)
> > + /*
> > +* Port 0 id 0 is local digital input, try to get it.
> > +* Not all instances can or will have this, that is OK
> > +*/
> > + ret = rvin_digital_graph_parse(vin, 0, 0);
> > + if (ret && ret != -ENOENT)
> > return ret;
> >
> > - if (!vin->digital.asd.match.fwnode.fwnode) {
> > + if (ret == -ENOENT) {
> > vin_dbg(vin, "No digital subdevice found\n");
> > return -ENODEV;
> > }
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Dong,
On Tue, Jun 13, 2017 at 02:25:08PM +0800, Dong Aisheng wrote:
> On Mon, Jun 12, 2017 at 5:44 PM, jmondi wrote:
> > Fair enough :)
> >
> > I'll try to keep this short: I don't like "output-enable", and at the
> > same time I don't think "output-high" and "output-low" fit well for
> > this purpose, as they electrically means something different from what
> > our (and IMX) use case is: enabling/disabling input/output
> > buffers internal to pin controller/gpio block HW and not driving a value
> > there.
> >
> > This seems clear to me from the "GPIO mode pitfalls" section of
> > pinctrl.txt documentation examples and from the fact that generic bindings
> > did not expose an "output" flag because if you drive an output line, you
> > reasonably either drive it high or low.
> >
> > Unfortunately I cannot convince myself that the same reasons apply
> > to the input use case. Enabling input on a pin implies the pinctrl/gpio
> > driver
> > has to enable any input buffer required to use that pin as a properly
> > working input line, and enabling an input buffer implies being able to sense
> > the line value from there, so I don't see that much use for
> > "input-buffer-enable"
> > alone.
> >
> > So, even if bindings could look a bit weird as there won't be a direct
> > matching between properties names used to enable input/output buffers,
> > my vote is to add "output-buffer-enable" only, and keep using the
> > already there "input-enable" properties for the input use case.
> >
>
> Yes, it may be a bit weird.
> I'm not pad internal details expert and can't tell much difference between
> output-enable and output-buffer-enable.
> I just feel a bit confuse if only using output-buffer-enable.
Yes it is, and I actually like your proposal, I was just trying to
make sure I was not confusing the property semantic with its
real-world effect.
If no one as different opinions on this, I can send a patch later to
add output-enable only, or since you have almost done it down here you
can do the same resusing what you have proposed below.
>
> If enable both input and output, it becomes:
> pinctrl_xxx: gpios_xxx_grp {
> pins = <
> ULP1_PAD_PTD0__PTD0
> >;
> input-enable;
> output-buffer-enable;
> bias-pull-up;
> };
>
> How about still use output-enable in pairs to input-enable but explain more
> in comments?
> Aslo update 'input-enable' comment to 'enable input buffer'.
> e.g.
> diff --git a/drivers/pinctrl/pinconf-generic.c
> b/drivers/pinctrl/pinconf-generic.c
> index 720a19f..96c83a4 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -172,6 +172,7 @@ static const struct pinconf_generic_params dt_params[] = {
> { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
> { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
> { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
> + { "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
> { "output-high", PIN_CONFIG_OUTPUT, 1, },
> { "output-low", PIN_CONFIG_OUTPUT, 0, },
> { "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
> diff --git a/include/linux/pinctrl/pinconf-generic.h
> b/include/linux/pinctrl/pinconf-generic.h
> index 7620eb1..d30f4fe 100644
> --- a/include/linux/pinctrl/pinconf-generic.h
> +++ b/include/linux/pinctrl/pinconf-generic.h
> @@ -59,9 +59,9 @@
> * which means it will wait for signals to settle when reading inputs.
> The
> * argument gives the debounce time in usecs. Setting the
> * argument to zero turns debouncing off.
> - * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does not
> - * affect the pin's ability to drive output. 1 enables input, 0 disables
> - * input.
> + * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer. Note
> that this does
> + * not affect the pin's ability to drive output.
> + * 1 enables input, 0 disables input.
I would not mention the "input buffer" here, as enabling input implies enabling
the buffer if you want to read values from there. Actually I guess
there may be platforms where buffer enabling may be implicit, so I
would leave this out and let drivers handle it internally.
> * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> * schmitt-trigger mode. If the schmitt-trigger has adjustable
> hysteresis
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Linus, On Sun, Jun 11, 2017 at 11:45:49PM +0200, Linus Walleij wrote: > On Fri, Jun 9, 2017 at 9:50 AM, jmondi wrote: > > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote: > > >> > I see three options here: > >> > > >> > 1) Add "output-buffer-enable" and "input-buffer-enable" > >> > we end up with > >> > "output-high" > >> > "output-low" > >> > "input-enable" > >> > "output-buffer-enable" > >> > "input-buffer-enable" > >> > > >> > 2) Add "output-buffer-enable" only > >> > we end up with > >> > "output-high" > >> > "output-low" > >> > "input-enable" > >> > "output-buffer-enable" > >> > > >> > Binding may be confusing as in one case we use "output-buffer-enable" > >> > while in the other "input-enable" > >> > > >> > 3) Add "output-enable" only > >> > "output-high" > >> > "output-low" > >> > "input-enable" > >> > "output-enable" > >> > > >> > As you, I don't like "output-enable" that much but it pairs better with > >> > "input-enable". > >> > > >> > I'll let you and DT people decide on this, as it's really an ABI > >> > definition > >> > problem and you have better judgment there. > >> > > >> > >> What's the final decision of this? > > > > I admit a was buying a bit of time and post-poned the gentle ping for > > any final word on this. But since you're asking I'll second your > > question :) > > I suspect it is time to quote > Documentation/process/management-style.rst > (Torvalds): > > 1) Decisions > > Everybody thinks managers make decisions, and that decision-making is > important. The bigger and more painful the decision, the bigger the > manager must be to make it. That's very deep and obvious, but it's not > actually true. > > The name of the game is to **avoid** having to make a decision. In > particular, if somebody tells you "choose (a) or (b), we really need you > to decide on this", you're in trouble as a manager. The people you > manage had better know the details better than you, so if they come to > you for a technical decision, you're screwed. You're clearly not > competent to make that decision for them. > > (It goes on, it's the best part of the entire Documentation/* dir in my > opinion, please take the time to read it in full.) > > So: what do you guys, using this feature, and Andy, who raised serious > concerns, think is the right binding? That is what *I* need to know. Fair enough :) I'll try to keep this short: I don't like "output-enable", and at the same time I don't think "output-high" and "output-low" fit well for this purpose, as they electrically means something different from what our (and IMX) use case is: enabling/disabling input/output buffers internal to pin controller/gpio block HW and not driving a value there. This seems clear to me from the "GPIO mode pitfalls" section of pinctrl.txt documentation examples and from the fact that generic bindings did not expose an "output" flag because if you drive an output line, you reasonably either drive it high or low. Unfortunately I cannot convince myself that the same reasons apply to the input use case. Enabling input on a pin implies the pinctrl/gpio driver has to enable any input buffer required to use that pin as a properly working input line, and enabling an input buffer implies being able to sense the line value from there, so I don't see that much use for "input-buffer-enable" alone. So, even if bindings could look a bit weird as there won't be a direct matching between properties names used to enable input/output buffers, my vote is to add "output-buffer-enable" only, and keep using the already there "input-enable" properties for the input use case. Thanks j > > Yours, > Linus Walleij
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Dong, On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote: > Hi Linus & j, > > >> > >> I just want to know if "output-enable" is the right name? > >> "output-buffer-enable"? > > > > Great! Thanks! > > > > On naming: if we need "output-buffer-enable" should we add > > "input-buffer-enable" as well? > > > > Currently we are using "input-enable" to pair with "output-enable", > > but as you said, just "output-enable" when "output-high" and > > "output-low" are there already seems a bit confusing. > > At the same time "input-buffer-enable" seems to actually be just > > electrically equivalent to "input-enable", so adding it is a bit of a > > waste as well. > > > > I see three options here: > > > > 1) Add "output-buffer-enable" and "input-buffer-enable" > > we end up with > > "output-high" > > "output-low" > > "input-enable" > > "output-buffer-enable" > > "input-buffer-enable" > > > > 2) Add "output-buffer-enable" only > > we end up with > > "output-high" > > "output-low" > > "input-enable" > > "output-buffer-enable" > > > > Binding may be confusing as in one case we use "output-buffer-enable" > > while in the other "input-enable" > > > > 3) Add "output-enable" only > > "output-high" > > "output-low" > > "input-enable" > > "output-enable" > > > > As you, I don't like "output-enable" that much but it pairs better with > > "input-enable". > > > > I'll let you and DT people decide on this, as it's really an ABI definition > > problem and you have better judgment there. > > > > What's the final decision of this? I admit a was buying a bit of time and post-poned the gentle ping for any final word on this. But since you're asking I'll second your question :) > > I saw the following revert patch in pinctrl-next but did not see a successive > patch to add output-enable back? > Still waiting to have a feedback on which properties to add, that's why I have not sent anything yet. Thanks j > IMX7ULP pinctrl driver is pending on this because it needs use both > input-enable and output-enable if we want to make them generic property. > > commit b4d2ea2af95cb77e2f320e24da526280d4aa2f6b > Author: Linus Walleij > Date: Mon May 8 10:48:21 2017 +0200 > > Revert "pinctrl: generic: Add bi-directional and output-enable" > > This reverts commit 8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0. > > It turns out that applying these generic properties was > premature: the properties used in the driver using this > are of unclear electrical nature and the subject need to > be discussed. > > Signed-off-by: Linus Walleij > > Regards > Dong Aisheng > > >> > >> > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked > >> > in some previous email. > >> > >> I'm just overloaded. I sent that revert to Torvalds today. > > > > Thank you. Didn't want to put pressure ;) > >> > >> > I can send another version of that patch with > >> > only 'output-enable' if you wish. > >> > >> That's what we want. > >> > >> > Once we reach consesus, I can then send v6 of our pin controller driver > >> > based on that. > >> > >> OK sounds like a plan. > >> > >> Sorry for the mess, I'm just trying to get this right :/ > > > > Not a mess, and thanks for your effort in maintaining all of this > > > > Thanks > >j > >> > >> Yours, > >> Linus Walleij
Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment
On Wed, May 31, 2017 at 10:19:07AM +0200, Peter Rosin wrote:
> On 2017-05-31 09:51, jmondi wrote:
> > Hi Peter,
> >
> > On Tue, May 30, 2017 at 01:04:08PM +0200, Peter Rosin wrote:
> >> On 2017-05-30 10:54, Jacopo Mondi wrote:
> >>> I2c-mux channels are created as mux siblings while they should be
> >>> children of the mux itself. Fix it.
> >>
> >> Has this received any testing at all?
> >>
> >
> > Yes, but on our specific use case, that apparently does not makes use of
> > i2c_parent_is_i2c_adapter()
>
> Try e.g. adding a client device with some address to the root i2c
> adapter, and then add another client device with the same address
> to one of the mux child adapters. Do that with and without your
> patch. I suspect it will be allowed with your patch even though it
> shouldn't.
>
Oh I see what's you're point here.
I cannot access the test board and try, but yes I see what you mean.
> >> I think it will break various users of i2c_parent_is_i2c_adapter()
> >> that expect the current situation that a i2c mux child adapter is
> >> direct child of the parent i2c adapter. I.e. with no intermediate
> >> device node.
> >
> > Oh, I know see..
> >
> > So when walking the devices sitting on an i2c-adapter we should expect to
> > see mux children adapters as well there. Do you think is there a way to
> > easily identify them?
>
> Well, you can use the method from i2c_parent_is_i2c_adapter() and
> write a function like so (untested):
>
> struct i2c_adapter *i2c_is_i2c_adapter(struct device *dev)
> {
> if (dev->type != &i2c_adapter_type)
> return NULL;
>
> return to_i2c_adapter(dev);
> }
>
> But why do you need to identify them? What problem are you trying to solve?
>
I want to be able to walk all children devices of an i2c-adapter,
not including the mux channel i2c-adapters. I'm sure I can work around it.
While doing that I stumbled upon this and thought it was wrong.
> Also, I forgot to mention this in my first reply, but note that the
> device implementing the i2c-mux need not be a child of the i2c adapter.
> I.e. in your example, the device I'm talking about is gmsl-deserializer@0.
> This "MUX" device could be e.g. a platform device situated in some other
> completely random place in the device tree. Oh well, perhaps not random,
> but I hope you get what I mean...
Yes, I guess so :)
Please drop this patch then, and thanks for your explanation
Thanks
j
>
> Cheers,
> peda
>
> > Thanks
> >j
> >>
> >> Cheers,
> >> peda
> >>
> >>> Signed-off-by: Jacopo Mondi
> >>> Suggested-by: Laurent Pinchart
> >>> ---
> >>>
> >>> Hello,
> >>>while inspecting child nodes of an i2c adapter it has been noted that
> >>> child devices of an i2c-mux are listed as children of the i2c adapter
> >>> itself,
> >>> and not of the i2c-mux.
> >>>
> >>> The hierarchy of devices looked like
> >>>
> >>> -- i2c-04
> >>> --- eeprom@57
> >>> --- video_receiver@70
> >>> --- video_receiver@34
> >>> --- gmsl-deserializer@0 <-- MUX
> >>> --- gmsl-deserializer@0/i2c@0 <-- MUX CHANNEL
> >>>
> >>> It now looks like
> >>>
> >>> -- i2c-04
> >>> --- eeprom@57
> >>> --- video_receiver@70
> >>> --- video_receiver@34
> >>> --- gmsl-deserializer@0
> >>> gmsl-deserializer@0/i2c@0
> >>>
> >>> v1 -> v2:
> >>> - change commit message as suggested by Geert
> >>>
> >>> drivers/i2c/i2c-mux.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> >>> index 83768e8..37b7804 100644
> >>> --- a/drivers/i2c/i2c-mux.c
> >>> +++ b/drivers/i2c/i2c-mux.c
> >>> @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> >>> priv->adap.owner = THIS_MODULE;
> >>> priv->adap.algo = &priv->algo;
> >>> priv->adap.algo_data = priv;
> >>> - priv->adap.dev.parent = &parent->dev;
> >>> + priv->adap.dev.parent = muxc->dev;
> >>> priv->adap.retries = parent->retries;
> >>> priv->adap.timeout = parent->timeout;
> >>> priv->adap.quirks = parent->quirks;
> >>> --
> >>> 2.7.4
> >>>
> >>
>
Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment
Hi Peter, On Tue, May 30, 2017 at 01:04:08PM +0200, Peter Rosin wrote: > On 2017-05-30 10:54, Jacopo Mondi wrote: > > I2c-mux channels are created as mux siblings while they should be > > children of the mux itself. Fix it. > > Has this received any testing at all? > Yes, but on our specific use case, that apparently does not makes use of i2c_parent_is_i2c_adapter() > I think it will break various users of i2c_parent_is_i2c_adapter() > that expect the current situation that a i2c mux child adapter is > direct child of the parent i2c adapter. I.e. with no intermediate > device node. Oh, I know see.. So when walking the devices sitting on an i2c-adapter we should expect to see mux children adapters as well there. Do you think is there a way to easily identify them? Thanks j > > Cheers, > peda > > > Signed-off-by: Jacopo Mondi > > Suggested-by: Laurent Pinchart > > --- > > > > Hello, > >while inspecting child nodes of an i2c adapter it has been noted that > > child devices of an i2c-mux are listed as children of the i2c adapter > > itself, > > and not of the i2c-mux. > > > > The hierarchy of devices looked like > > > > -- i2c-04 > > --- eeprom@57 > > --- video_receiver@70 > > --- video_receiver@34 > > --- gmsl-deserializer@0 <-- MUX > > --- gmsl-deserializer@0/i2c@0 <-- MUX CHANNEL > > > > It now looks like > > > > -- i2c-04 > > --- eeprom@57 > > --- video_receiver@70 > > --- video_receiver@34 > > --- gmsl-deserializer@0 > > gmsl-deserializer@0/i2c@0 > > > > v1 -> v2: > > - change commit message as suggested by Geert > > > > drivers/i2c/i2c-mux.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c > > index 83768e8..37b7804 100644 > > --- a/drivers/i2c/i2c-mux.c > > +++ b/drivers/i2c/i2c-mux.c > > @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc, > > priv->adap.owner = THIS_MODULE; > > priv->adap.algo = &priv->algo; > > priv->adap.algo_data = priv; > > - priv->adap.dev.parent = &parent->dev; > > + priv->adap.dev.parent = muxc->dev; > > priv->adap.retries = parent->retries; > > priv->adap.timeout = parent->timeout; > > priv->adap.quirks = parent->quirks; > > -- > > 2.7.4 > > >
Re: [PATCH] i2c: i2c-mux: Fix channel parent node assignment
Hi Geert,
On Tue, May 30, 2017 at 10:31:21AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Tue, May 30, 2017 at 10:13 AM, Jacopo Mondi
> wrote:
> > I2c-mux channels are created with the mux parent device as their own
> > parent, while they should be siblings of the mux itself.
>
> Isn't it the other way around? Cfr. the example and the patch.
Yeah, I actually didn't know the exact meaning of "siblings" then.
So:
I2c-mux channels are created as mux siblings while they should be
children of the mux itself. Fix it.
Any better? ;)
Thanks
j
>
> > Fix this.
> >
> > Signed-off-by: Jacopo Mondi
> > Suggested-by: Laurent Pinchart
> >
> > ---
> >
> > Hello,
> >while inspecting child nodes of an i2c adapter it has been noted that
> > child devices of an i2c-mux are listed as children of the i2c adapter
> > itself,
> > and not of the i2c-mux.
> >
> > The hierarchy of devices looked like
> >
> > -- i2c-04
> > --- eeprom@57
> > --- video_receiver@70
> > --- video_receiver@34
> > --- gmsl-deserializer@0 <-- MUX
> > --- gmsl-deserializer@0/i2c@0 <-- MUX CHANNEL
>
> = sibling of mux
>
> >
> > It now looks like
> >
> > -- i2c-04
> > --- eeprom@57
> > --- video_receiver@70
> > --- video_receiver@34
> > --- gmsl-deserializer@0
> > gmsl-deserializer@0/i2c@0
>
> = child of mux
>
> >
> > drivers/i2c/i2c-mux.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> > index 83768e8..37b7804 100644
> > --- a/drivers/i2c/i2c-mux.c
> > +++ b/drivers/i2c/i2c-mux.c
> > @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> > priv->adap.owner = THIS_MODULE;
> > priv->adap.algo = &priv->algo;
> > priv->adap.algo_data = priv;
> > - priv->adap.dev.parent = &parent->dev;
>
> = sibling of mux
>
> > + priv->adap.dev.parent = muxc->dev;
>
> = child of mux
>
> > priv->adap.retries = parent->retries;
> > priv->adap.timeout = parent->timeout;
> > priv->adap.quirks = parent->quirks;
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Linus, On Mon, May 29, 2017 at 10:45:44AM +0200, Linus Walleij wrote: > On Tue, May 23, 2017 at 8:37 PM, jmondi wrote: > > >> I did not follow too much. > >> But it seems IMX7ULP/Vybrid to be also a fan of generic > >> output-enable/input-enable > >> property. > >> > >> See: > >> Figure 5-2. GPIO PAD in Page 241 > >> http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf > >> > >> It has separate register bits to control input buffer enable and > >> output buffer enable > >> and we need set it property for GPIO function. > > > > As it seems we have another user for 'output-enable' here, what if we just > > add that one to the generic bindings properties list, and we keep > > 'bi-directional' (which seems to be the most debated property we have > > added) out of generic properties? > > > > We can handle 'bi-directional' pins with static tables in our pin > > controller driver and not have it anywhere in DT. > > This sounds like a viable approach. > > I just want to know if "output-enable" is the right name? > "output-buffer-enable"? Great! Thanks! On naming: if we need "output-buffer-enable" should we add "input-buffer-enable" as well? Currently we are using "input-enable" to pair with "output-enable", but as you said, just "output-enable" when "output-high" and "output-low" are there already seems a bit confusing. At the same time "input-buffer-enable" seems to actually be just electrically equivalent to "input-enable", so adding it is a bit of a waste as well. I see three options here: 1) Add "output-buffer-enable" and "input-buffer-enable" we end up with "output-high" "output-low" "input-enable" "output-buffer-enable" "input-buffer-enable" 2) Add "output-buffer-enable" only we end up with "output-high" "output-low" "input-enable" "output-buffer-enable" Binding may be confusing as in one case we use "output-buffer-enable" while in the other "input-enable" 3) Add "output-enable" only "output-high" "output-low" "input-enable" "output-enable" As you, I don't like "output-enable" that much but it pairs better with "input-enable". I'll let you and DT people decide on this, as it's really an ABI definition problem and you have better judgment there. > > > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked > > in some previous email. > > I'm just overloaded. I sent that revert to Torvalds today. Thank you. Didn't want to put pressure ;) > > > I can send another version of that patch with > > only 'output-enable' if you wish. > > That's what we want. > > > Once we reach consesus, I can then send v6 of our pin controller driver > > based on that. > > OK sounds like a plan. > > Sorry for the mess, I'm just trying to get this right :/ Not a mess, and thanks for your effort in maintaining all of this Thanks j > > Yours, > Linus Walleij
Re: [PATCH] arm: dts: r7s72100: Add support for GR-Peach
Hello Simon,
On Tue, May 23, 2017 at 09:25:40PM +0200, Geert Uytterhoeven wrote:
> On Tue, May 23, 2017 at 9:20 PM, Jacopo Mondi
> wrote:
> > Add device tree source for Renesas GR-Peach board.
> > GR-Peach is an RZ/A1H based board with 10MB of on-chip SRAM and 8MB
> > QSPI flash storage.
> > Add support for the board, and create a 2MB partition to use as rootfs.
> >
> > Signed-off-by: Jacopo Mondi
>
> Reviewed-by: Geert Uytterhoeven
I forgot to change subject to "PATCH v2".
Could you please take this one, or would you like me to resend with
Geert's reviewed-by ?
Thanks
j
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH] arm: dts: r7s72100: Add support for GR-Peach
Hi Geert,
On Tue, May 23, 2017 at 12:08:30PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Tue, May 23, 2017 at 11:47 AM, Jacopo Mondi
> wrote:
> > Add device tree source for Renesas GR-Peach board.
> > GR-Peach is an RZ/A1H based board with 10MB of on-chip SRAM and 8MB
> > QSPI flash storage.
> > Add support for the board, and create a 2MB partition to use as rootfs.
> >
> > Signed-off-by: Jacopo Mondi
>
> Thanks for your patch!
>
> > --- a/Documentation/devicetree/bindings/arm/shmobile.txt
> > +++ b/Documentation/devicetree/bindings/arm/shmobile.txt
> > @@ -51,6 +51,8 @@ Boards:
> > compatible = "renesas,bockw", "renesas,r8a7778"
> >- Genmai (RTK772100BC0BR)
> > compatible = "renesas,genmai", "renesas,r7s72100"
> > + - GR-Peach (RTK772100BC0BR)
>
> That part number is Genmai?
Oh, I thought that was the SoC part number, and being both based on
RZ/A1H, I used the same.
Where can I find the correct part number?
>
> > +compatible = "renesas,grpeach", "renesas,r7s72100"
>
> "renesas,gr-peach"?
Let's use gr-peach in place of grpeach everywhere then.
>
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -716,6 +716,7 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
> > dtb-$(CONFIG_ARCH_SHMOBILE_MULTI) += \
> > emev2-kzm9d.dtb \
> > r7s72100-genmai.dtb \
> > + r7s72100-grpeach.dtb \
>
> r7s72100-gr-peach.dtb?
>
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/r7s72100-grpeach.dts
>
> r7s72100-gr-peach.dts
>
> > @@ -0,0 +1,65 @@
> > +/*
> > + * Device Tree Source for the GRPEACH board
>
> GR-PEACH
>
> > +/ {
> > + model = "GRPEACH";
>
> GR-PEACH
>
> > + compatible = "renesas,grpeach", "renesas,r7s72100";
>
> renesas,gr-peach
>
> > +
> > + aliases {
> > + serial2 = &scif2;
>
> The first serial port is supposed to be called serial0, unless labeled
> otherwise.
I assumed it had to match the SCIF number. Will change.
>
> > + };
> > +
> > + chosen {
> > + bootargs = "console=ttySC2,115200 ignore_loglevel rw
> > root=/dev/mtdblock0";
>
> bootargs = "ignore_loglevel rw root=/dev/mtdblock0";
> stdout-path = "serial0:115200n8";
>
> > + memory {
>
> memory@2000
>
> (please run "make dtbs W=1")
>
> > + device_type = "memory";
> > + reg = <0x2000 0x00A0>; /* 10Mb @ 0x2000 */
>
> reg = <0x2000 0x00a0>;
>
> No need for the comment, esp. since it says 10 megabits instead of
> 10 mebibyte ;-)
Was I the only one that had to go on Wikipedia to find out what a
mebibyte is :p ?
I'll drop comments here and in the rest of the patch.
>
> > + qspi@1800 {
>
> flash@1800?
>
> > + compatible = "mtd-rom";
> > + probe-type = "map_rom";
> > + reg = <0x1800 0x0080>; /* 8 MB*/
>
> No need for the comment (megabyte vs. mebibyte)
>
> > + bank-width = <4>;
> > + device-width = <1>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + rootfs@1860 {
>
> rootfs@60?
>
> > + label = "rootfs";
> > + reg = <0x0060 0x0020>; /* 2MB @ 0x1860
> > */
>
> Do we need the comment?
>
> > +&extal_clk {
> > + clock-frequency = <1333>;
>
> My schematics have one more digit of precision: <1000>
Thanks, I'll change this.
Do we need to change that on Genmai as well while at there?
Some parts of its documentation report a frequency of 13.33Mhz, but X1
is listed in the BOM as
X1 XIN oscillator (socket mounted) SG-8002DC-13.MHz (Epson)
Thanks
j
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Linus, On Tue, May 23, 2017 at 06:08:31PM +0800, Dong Aisheng wrote: > On Tue, May 9, 2017 at 5:19 AM, Linus Walleij > wrote: > > On Mon, May 8, 2017 at 7:25 PM, jmondi wrote: > > > >> From my perspective these flags are configurations internal to the pin > >> controller hardware used to enable/disable input buffers when a pin needs > >> to > >> perform in both direction. > >> The level of detail I can provide on this is the logical diagram we have > >> pointed > >> you to already. > >> > >> As I assume you are trying to get this answer from us in order to > >> avoid duplicating things in pin controller sub-system, and I > >> understand this, but my question here was "can we have those flags as part > >> of the pinmux property argument list, as that property description > >> seems to allow us to do that, instead of making them generic pin > >> configuration properties and upset other developers?" > > > > Pinmux with all it's magic flags baked into one is not any better > > or any more readable. The solution is already very pretty except > > for these two flags which I am sure we can agree on a way forward > > for. > > > > What we choose between is not this or another less transparent > > pin configuration mechanism, the mechanism (whether magic bits > > to pinmux or reasonable properties) does not matter. > > > > There is a strong preference to use the generic bindings. > > > > So the discussion is whether to use: > > > > bi-directional; > > output-enable; > > > > Or some already defined config flags. > > > > If these are too idiomatic to be used by others, they should anyways > > look similar, like: > > > > renesas,bi-directional; > > renesas,output-enable; > > > > Like the Qualcomm weirdness found in > > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > > > > qcom,pull-up-strength = <..>; > > > > Check how they use > > #define PMIC_GPIO_CONF_PULL_UP (PIN_CONFIG_END + 1) > > > > Etc. > > > >> Anyway, I still fail to see why those configuration flags, only > >> affecting the way the pin controller hardware enables/disables > >> its internal buffers and its internal operations have to be > >> described in term of their externally visible electrically characteristics. > > > > To me internal vs external is not what matters. What matters is > > if this is likely to pop up in more platforms, and then the property > > should be generic. > > > > The generic pin config definitions are likely to be picked up by other > > standards and even be inspiration to hardware engineers so that > > is why it matters so much. > > > >> To me, what already exists are pin configuration properties generic to > >> the whole pin controller subsystem, and I understand you don't want to > >> see duplication there. > >> > >> At the same time, to me, those flags are settings the pin controller > >> wants to have specified by software to overcome its hw design flaws, > >> and are intended to configure its internal buffers in a way it cannot > >> do by itself for some very specific operation modes (they are listed > >> in the hw reference manual, it's not something you can chose to > >> configure or not, if you want a pin working in i2c mode, you HAVE to > >> pass those flags to pin controller). > > > > Sounds like a case for > > > > renesas,bi-directional; > > renesas,output-enable; > > > > following the Qualcomm pattern in that case. > > > > But let's see if something else comes out of this discussion. > > > > I did not follow too much. > But it seems IMX7ULP/Vybrid to be also a fan of generic > output-enable/input-enable > property. > > See: > Figure 5-2. GPIO PAD in Page 241 > http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf > > It has separate register bits to control input buffer enable and > output buffer enable > and we need set it property for GPIO function. As it seems we have another user for 'output-enable' here, what if we just add that one to the generic bindings properties list, and we keep 'bi-directional' (which seems to be the most debated property we have added) out of generic properties? We can handle 'bi-directional' pins with static tables in our pin controller driver and not have it anywhere in DT. I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked in some previous email. I can send another version of that patch with only 'output-enable' if you wish. Once we reach consesus, I can then send v6 of our pin controller driver based on that. Thanks j [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0 > > Regards > Dong Aisheng
Re: [RFC v2 0/2] SH CEU camera driver
Hi Chris,
On Thu, Apr 27, 2017 at 06:03:27PM +, Chris Brandt wrote:
> Hi Jacopo,
>
> On Thursday, April 27, 2017, jmondi wrote:
> > We planned to use the following setup to test the CEU driver with an
> > actual HW platform
> >
> > GR-Peach + GR-Peach Audio Camera Shield + OV7670 evaluation module
> > https://www.digikey.com/product-detail/en/renesas-electronics-
> > america/YGRPEACHAUDIOCAMERASHIELD/YGRPEACHAUDIOCAMERASHIELD-ND/5800301
> > www.elecfreaks.com/estore/ov7670-camera-module.html
>
> Oh ya, I forgot about that thing.
>
>
> > This implies running mainline on Peach or backporting CEU driver to your
> > BSP if we're not able to do so. A desirable collateral benefit in both
> > cases, I guess...
>
> You should probably be able to run mainline on it.
>
I'm now trying to boot v4.11 (non-XIP) on GR-Peach.
I've been able to write u-boot from your BSP directly on the SPI
flash, and I'm now trying to use it to boot a kernel image from
on-chip SRAM.
I've spent quite some time on it without any appreciable result, so I
am now considering trying to make a XIP kernel out of mainline and
boot from QSPI.
I've seen you have applied lot of patches for XIP support on top of
your v3.14 BSP, can you summarize what I need to apply on v4.11 to
have the same?
Also FYI, I am now trying to boot a uImage with device tree appended,
loaded on the on-chip RAM with J-Link to address 0x2050, and
compiled with 20008000 [1] as load address (32Kb below on-chip RAM
address start)
This way I -should- be able to load my 3MB kernel image in RAM and
execute it from there.
I always get back a mute console unfortunately, so I'm starting to
wonder if I shouldn't go for a XIP kernel as well...
=> bootm 0x2050
## Booting kernel from Legacy Image at 2050 ...
Image Name: Linux
Created: 2017-05-15 14:29:05 UTC
Image Type: ARM Linux Kernel Image (uncompressed)
Data Size:3009736 Bytes = 2.9 MiB
Load Address: 20008000
Entry Point: 20008000
Verifying Checksum ... OK
Loading Kernel Image ... OK
Starting kernel ...
Thanks
j
[1] I tried to use 0x20208000 as well, to avoid writing to memory
pages for data retention.
[2] I am using r7s72100-grpeach.dts from your BSP, with the following applied:
memory {
device_type = "memory";
- reg = <0x0800 0x0200>; /* 32MB @ 0x0x0800 */
+ reg = <0x2000 0x00A0>; /* 10Mb @ 0x0x2000 */
};
[3] I have used uImage with appended DTB and uImage with separate dtb as well.
No luck.
PS I have removed renesas list not to spam to everyone. If relevant, I will
re-send and add them back.
> Go here for u-boot build instructions: http://elinux.org/RZ-A/Boards/GR-PEACH
>
> Remember, that board only has 8MB of SPI flash, so you should download uImage
> and DTB to RAM using Jlink, the use the SPI flash for a rootfs. I so have USB
> host working for mainline, but it requires an out-of-tree patch from me (I'm
> not sure on what upstream path I should take for RZ/A1 USB yet).
>
>
>
> Of course if you change your mind, this board:
>
> https://www.digikey.com/product-detail/en/renesas-electronics-america/YLCDRZA1H/YLCDRZA1H-ND/6173788
> has a camera, 1280x800 LCD, 64MB of SDRAM and a Segger JLINK built into the
> board. But...no serial port. You have to plug in a USB-to-UART PMOD adapter
> (something like this
> http://store.digilentinc.com/pmod-usbuart-usb-to-uart-interface/).
> Once I get everything working, I'll be pushing to our github repos and
> posting instructions on eLinux.org.
> I might test your new driver using this board (will require an out-of-tree
> fbdev driver)
>
> I'm going to start phasing out the 3.14 BSP for a 4.9 BSP here in a couple
> months, so I'll probably backport this new driver to 4.9, but that's about it.
>
>
> Chris
>
Re: [PATCH 1/4] sh: sh7722: Remove nonexistent GPIO_PTQ7 to fix pinctrl registration
Hi Geert,
On Tue, May 09, 2017 at 04:11:54PM +0200, Geert Uytterhoeven wrote:
> On sh7722/Migo-R, pinctrl registration fails with:
>
> sh-pfc pfc-sh7722: pin 0 already registered
> sh-pfc pfc-sh7722: error during pin registration
> sh-pfc pfc-sh7722: could not register: -22
> sh-pfc: probe of pfc-sh7722 failed with error -22
>
> pinmux_pins[] is initialized through PINMUX_GPIO(), using designated
> array initializers, where the GPIO_* enums serve as indices.
> As GPIO_PTQ7 is defined in the enum, but never used, pinmux_pins[]
> contains a (zero-filled) hole. Hence this entry is treated as pin zero,
> which was registered before, and initialization fails.
>
> According to the datasheet, port PTQ7 does not exist. Hence remove
> GPIO_PTQ7 from the enum to fix this.
>
> Fixes: 8d7b5b0af7e070b9 ("sh: Add sh7722 pinmux code")
> Reported-by: Magnus Damm
> Signed-off-by: Geert Uytterhoeven
Tested-by: Jacopo Mondi
Thank you
j
> ---
> Tested on sh7722/Migo-R.
> ---
> arch/sh/include/cpu-sh4/cpu/sh7722.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/sh/include/cpu-sh4/cpu/sh7722.h
> b/arch/sh/include/cpu-sh4/cpu/sh7722.h
> index 3bb74e534d0f8ca4..78961ab78a5a9c83 100644
> --- a/arch/sh/include/cpu-sh4/cpu/sh7722.h
> +++ b/arch/sh/include/cpu-sh4/cpu/sh7722.h
> @@ -67,7 +67,7 @@ enum {
> GPIO_PTN3, GPIO_PTN2, GPIO_PTN1, GPIO_PTN0,
>
> /* PTQ */
> - GPIO_PTQ7, GPIO_PTQ6, GPIO_PTQ5, GPIO_PTQ4,
> + GPIO_PTQ6, GPIO_PTQ5, GPIO_PTQ4,
> GPIO_PTQ3, GPIO_PTQ2, GPIO_PTQ1, GPIO_PTQ0,
>
> /* PTR */
> --
> 2.7.4
>
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Andy,
On Mon, May 08, 2017 at 08:47:17PM +0300, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 8:25 PM, jmondi wrote:
> > Andy,
> >
> > On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
> >> On Mon, May 8, 2017 at 7:01 PM, jmondi wrote:
> >> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >> >> wrote:
> >> >>
> >> >> > Linus, for me it looks like better to revert that change, until we
> >> >> > will have clear picture why existing configuration parameters can't
> >> >> > work.
> >> >>
> >> >> Yeah I'll revert the binding for fixes.
> >>
> >> > As it seems we won't be able to proceed with the currently proposed
> >> > solution,
> >> > would that be acceptable now that we use the "pinmux" property to add
> >> > flags as BIDIR
> >>
> >> Can you explain what does this *electrically* mean?
> >
> > I really don't know what to add to what Chris said in his 2 previous
> > replies to the same question, and I don't know if I can even get this
> > information as the most detailed drawing I can provide is what you
> > have seen already at page 2696 Fig. 54.1 of the following document.
> >
> > https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c
>
> I didn't see before this document. (I had downloaded what Chris
> referred to, which has less than 1200 pages).
>
> The figure you pointed to is really nice and explains it, thank you.
Oh sorry, I thought you had seen this already :)
>
> So, BiDi in this hardware is just helper to enable Input
> simultaneously when you enable output.
>
> This makes me wonder what prevents you to do this in two steps in software?
> So, basically in terms of pin control framework you define this pin
> configuration as
>
> 1. PIN_CONFIG_INPUT_ENABLE:
> 2. PIN_CONFIG_OUTPUT:
>
> (or wise versa)
>
That could be doable, as when we're collecting generic pin
configuration to apply to the pin I can simply check if both of them
are enabled.
That would feel un-natural in dts anyway, for someone that is not that
into the pin controller sub system details.
If I would have to do something like this, not knowing all the
reasonable pre-conditions we've been discussing about
pins {
pinmux = < .. >;
input-enable;
output-high; /* or output-low, we can ignore the argument here */
}
In place of
pins {
pinmux = < .. >;
renesas,bi-directional;
}
And the hardware manual speaks of "bi-directional" everywhere, I would
be wondering what those guys implementing this were thinking :)
> > From my perspective these flags are configurations internal to the pin
> > controller hardware used to enable/disable input buffers when a pin needs to
> > perform in both direction.
>
> > The level of detail I can provide on this is the logical diagram we have
> > pointed
> > you to already.
> >
> > As I assume you are trying to get this answer from us in order to
> > avoid duplicating things in pin controller sub-system, and I
> > understand this, but my question here was "can we have those flags as part
> > of the pinmux property argument list, as that property description
> > seems to allow us to do that, instead of making them generic pin
> > configuration properties and upset other developers?"
>
> I guess Linus is better than me could answer to this.
>
> > Anyway, I still fail to see why those configuration flags, only
> > affecting the way the pin controller hardware enables/disables
> > its internal buffers and its internal operations have to be
> > described in term of their externally visible electrically characteristics.
> >
> >> Second question, what makes it differ to what already exists?
> >
> > To me, what already exists are pin configuration properties generic to
> > the whole pin controller subsystem, and I understand you don't want to
> > see duplication there.
> >
> > At the same time, to me, those flags are settings the pin controller
> > wants to have specified by software to overcome its hw design flaws,
> > and are intended to configure its internal buffers in a way it cannot
> > do by itself for some very specific operation modes (they are listed
> > in the hw reference manual, it's not something you can chose to
> > configure or not, if you want a pin working in i2c mode, you HAVE to
>
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Andy, On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote: > On Mon, May 8, 2017 at 7:01 PM, jmondi wrote: > > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote: > >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko > >> wrote: > >> > >> > Linus, for me it looks like better to revert that change, until we > >> > will have clear picture why existing configuration parameters can't > >> > work. > >> > >> Yeah I'll revert the binding for fixes. > > > As it seems we won't be able to proceed with the currently proposed > > solution, > > would that be acceptable now that we use the "pinmux" property to add > > flags as BIDIR > > Can you explain what does this *electrically* mean? I really don't know what to add to what Chris said in his 2 previous replies to the same question, and I don't know if I can even get this information as the most detailed drawing I can provide is what you have seen already at page 2696 Fig. 54.1 of the following document. https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c >From my perspective these flags are configurations internal to the pin controller hardware used to enable/disable input buffers when a pin needs to perform in both direction. The level of detail I can provide on this is the logical diagram we have pointed you to already. As I assume you are trying to get this answer from us in order to avoid duplicating things in pin controller sub-system, and I understand this, but my question here was "can we have those flags as part of the pinmux property argument list, as that property description seems to allow us to do that, instead of making them generic pin configuration properties and upset other developers?" Anyway, I still fail to see why those configuration flags, only affecting the way the pin controller hardware enables/disables its internal buffers and its internal operations have to be described in term of their externally visible electrically characteristics. > Second question, what makes it differ to what already exists? To me, what already exists are pin configuration properties generic to the whole pin controller subsystem, and I understand you don't want to see duplication there. At the same time, to me, those flags are settings the pin controller wants to have specified by software to overcome its hw design flaws, and are intended to configure its internal buffers in a way it cannot do by itself for some very specific operation modes (they are listed in the hw reference manual, it's not something you can chose to configure or not, if you want a pin working in i2c mode, you HAVE to pass those flags to pin controller). Thanks j Edit: I see Chris have now replied as well so I leave the SWIO part out, as his answer is already better than what I can give you. > > > and SWIO_[INPUT|OUTPUT] directly there? > > Ditto. > > > This was my original proposal, rejected because we were using the "pins" > > property at the time. > > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
Hi Linus, On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote: > On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko > wrote: > > > Linus, for me it looks like better to revert that change, until we > > will have clear picture why existing configuration parameters can't > > work. > > Yeah I'll revert the binding for fixes. > As it seems we won't be able to proceed with the currently proposed solution, would that be acceptable now that we use the "pinmux" property to add flags as BIDIR and SWIO_[INPUT|OUTPUT] directly there? This was my original proposal, rejected because we were using the "pins" property at the time. Quoting to the description of "pinmux": "Each individual pin controller driver bindings documentation shall specify how those values (pin IDs and pin multiplexing configuration) are defined and assembled together" Do you think the "flags" we have failed to describe as generic pin configuration properties, fit the definition of "pin multiplexing configuration" to be assembled with pin IDs? As a reference this was the proposed bindings in v3: https://www.spinics.net/lists/linux-renesas-soc/msg12792.html Have a look at Pin multiplexing sub-nodes examples 2 and 3, with "pinmux" in place of "renesas,pins" property. Thanks j > Yours, > Linus Walleij
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Laurent,
On Wed, May 03, 2017 at 06:06:19PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wednesday 03 May 2017 11:52:36 jmondi wrote:
> > On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> >
> > [snip]
> >
> > >> +/**
> > >> + * Collect formats supported by CEU and sensor subdevice
> > >> + */
> > >> +static int sh_ceu_init_active_formats(struct sh_ceu_dev *pcdev)
> > >> +{
> > >> +struct v4l2_subdev *sensor = pcdev->sensor;
> > >> +struct sh_ceu_fmt *active_fmts;
> > >> +unsigned int n_active_fmts;
> > >> +struct sh_ceu_fmt *fmt;
> > >> +unsigned int i;
> > >> +
> > >> +struct v4l2_subdev_mbus_code_enum mbus_code = {
> > >> +.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > >> +.index = 0,
> > >> +};
> > >> +
> > >> +/* Count how may formats are supported by CEU and sensor
> > >> subdevice */
> > >> +n_active_fmts = 0;
> > >> +while (!v4l2_subdev_call(sensor, pad, enum_mbus_code,
> > >> + NULL, &mbus_code)) {
> > >> +mbus_code.index++;
> > >> +
> > >> +fmt = get_ceu_fmt_from_mbus(mbus_code.code);
> > >
> > > This is not correct. You will get one one pixel format per media bus
> > > format supported by the sensor. The CEU supports
> > >
> > > 1. converting packed YUV 4:2:2 (YUYV) to YUV 4:2:2 planar (CAMCR.JPG = 00)
> > > or capturing raw data (CAMCR.JPG = 01)
> > >
> > > 2. outputting YUV 4:2:2 planar to memory (CDOCR.CDS = 1) or downsampling
> > > it to YUV 4:2:0 planar (CDOCR.CDS = 0)
> > >
> > > 3. swapping bytes, words and long words at the output (CDOCR.COLS,
> > > CDOCR.COWS and CDOCR.COBS fields)
> > >
> > > This effectively allows to
> > >
> > > - capture the raw data produced by the sensor
> > > - capture YUV images produced by the sensor in packed YUV 4:2:2 format,
> > > from any Y/U/V order to any Y/U/V order
> > > - capture YUV images produced by the sensor in planar YUV 4:2:2 or planar
> > > YUV 4:2:0, from any Y/U/V order to any Y/U/V order
> > >
> > > The list of formats you create needs to reflect that. This means that
> > >
> > > - if the sensor can produce a packed YUV 4:2:2 format, the CEU will be
> > > able to output YUYV, UYVY, YVYU, VYUY, NV12, NV21, NV16 and NV16
> > >
> > > - for every non-YUV format produced by the sensor, the CEU will be able to
> > > output raw data
> > >
> > > The former is easy. You just need to list the formats produced by the
> > > sensor, and if one of them is packed YUV 4:2:2, flag that in the
> > > sh_ceu_dev structure, and allow all the above output YUV formats. You
> > > don't need to create an instance-specific list of those formats in the
> > > sh_ceu_dev structure, as they will be either all available or all
> > > non-available.
> > >
> > > The latter is a bit more complex, you need a list of media bus code to
> > > pixel format in the driver. I'd recommend counting the non-YUV packed
> > > formats produced by the sensor, allocating an array of sh_ceu_fmt
> > > pointers with that number of entries, and make each entry point to the
> > > corresponding entry in the global sh_ceu_fmts array. The global
> > > sh_ceu_fmts needs to be extended with common sensor formats (raw Bayer
> > > and JPEG come to mind).
> > >
> > > If all sensors currently used with the CEU can produce packed YUV, you
> > > could implement YUV support only to start with, leaving raw capture
> > > support for later.
> >
> > Ok, thanks for explaining.
> >
> > I have a proposal here, as the original driver only supported "image
> > fetch mode" (ie. It accepts data in YUYV with components ordering arbitrary
> > swapped) as a first step we may want to replicate this, ignoring data
> > synch fetch mode (Chris, you have a driver for this you are already
> > using in your BSP so I guess it's less urgent to support it, right?).
> >
> > Also, regarding output memory format I am sure we can produce planar formats
> > as NV12/21 and NV16/61, but I'm not sure I have found a way to output
>
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Laurent,
On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
[snip]
> > +/**
> > + * Collect formats supported by CEU and sensor subdevice
> > + */
> > +static int sh_ceu_init_active_formats(struct sh_ceu_dev *pcdev)
> > +{
> > + struct v4l2_subdev *sensor = pcdev->sensor;
> > + struct sh_ceu_fmt *active_fmts;
> > + unsigned int n_active_fmts;
> > + struct sh_ceu_fmt *fmt;
> > + unsigned int i;
> > +
> > + struct v4l2_subdev_mbus_code_enum mbus_code = {
> > + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > + .index = 0,
> > + };
> > +
> > + /* Count how may formats are supported by CEU and sensor subdevice */
> > + n_active_fmts = 0;
> > + while (!v4l2_subdev_call(sensor, pad, enum_mbus_code,
> > +NULL, &mbus_code)) {
> > + mbus_code.index++;
> > +
> > + fmt = get_ceu_fmt_from_mbus(mbus_code.code);
>
> This is not correct. You will get one one pixel format per media bus format
> supported by the sensor. The CEU supports
>
> 1. converting packed YUV 4:2:2 (YUYV) to YUV 4:2:2 planar (CAMCR.JPG = 00) or
> capturing raw data (CAMCR.JPG = 01)
>
> 2. outputting YUV 4:2:2 planar to memory (CDOCR.CDS = 1) or downsampling it to
> YUV 4:2:0 planar (CDOCR.CDS = 0)
>
> 3. swapping bytes, words and long words at the output (CDOCR.COLS, CDOCR.COWS
> and CDOCR.COBS fields)
>
> This effectively allows to
>
> - capture the raw data produced by the sensor
> - capture YUV images produced by the sensor in packed YUV 4:2:2 format, from
> any Y/U/V order to any Y/U/V order
> - capture YUV images produced by the sensor in planar YUV 4:2:2 or planar YUV
> 4:2:0, from any Y/U/V order to any Y/U/V order
>
> The list of formats you create needs to reflect that. This means that
>
> - if the sensor can produce a packed YUV 4:2:2 format, the CEU will be able to
> output YUYV, UYVY, YVYU, VYUY, NV12, NV21, NV16 and NV16
>
> - for every non-YUV format produced by the sensor, the CEU will be able to
> output raw data
>
> The former is easy. You just need to list the formats produced by the sensor,
> and if one of them is packed YUV 4:2:2, flag that in the sh_ceu_dev structure,
> and allow all the above output YUV formats. You don't need to create an
> instance-specific list of those formats in the sh_ceu_dev structure, as they
> will be either all available or all non-available.
>
> The latter is a bit more complex, you need a list of media bus code to pixel
> format in the driver. I'd recommend counting the non-YUV packed formats
> produced by the sensor, allocating an array of sh_ceu_fmt pointers with that
> number of entries, and make each entry point to the corresponding entry in the
> global sh_ceu_fmts array. The global sh_ceu_fmts needs to be extended with
> common sensor formats (raw Bayer and JPEG come to mind).
>
> If all sensors currently used with the CEU can produce packed YUV, you could
> implement YUV support only to start with, leaving raw capture support for
> later.
Ok, thanks for explaining.
I have a proposal here, as the original driver only supported "image
fetch mode" (ie. It accepts data in YUYV with components ordering arbitrary
swapped) as a first step we may want to replicate this, ignoring data
synch fetch mode (Chris, you have a driver for this you are already
using in your BSP so I guess it's less urgent to support it, right?).
Also, regarding output memory format I am sure we can produce planar formats
as NV12/21 and NV16/61, but I'm not sure I have found a way to output
packed YUVY (and its permutations) to memory, if not considering it a
binary format, as thus using data synch fetch mode.
So, as a first step my proposal is the following one:
Accept any YUYV bus format from sensor, and support planar ones as memory output
formats with down-sampling support (NV12/21 and NV16/61 then).
The way I am building the supported format list is indeed wrong, and
as you suggested I should just try to make sure the sensor can output
a YUV422 bus format. From there I can output planar memory formats.
One last thing, I am not that sure how I can produce NV21/61 from
bus formats that do not already present the CbCr components in the
desired order (which is Cr then Cb for NV61/21)
Eg. Can I produce NV21 from YUYV though byte/word/long word swapping
(CDOCR.COLS/CDOCR.COWS/CDOCR.COBS) ? Won't I be swapping the luminance
components as well (see figure 46.45 in RZ/A1H TRM).
If I cannot do that, I should restrict swapped planar format
availability based on the ability of the sensor to produce
chrominance components in the desired order
(Eg. If the sensor does not support producing YVYU I cannot produce
NV21 or NV61). Is this ok?
Thanks
j
>
> > + if (!fmt)
> > + continue;
> > +
> > + fmt->active = 1;
> > + n_active_fmts++;
> > + }
> > +
> > + if (n_active_fmts == 0)
> > + return -ENXIO;
> > +
> > + pcdev->n_active_fmts
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Laurent,
On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
>
[snip]
> > + pcdev = devm_kzalloc(&pdev->dev, sizeof(*pcdev), GFP_KERNEL);
>
> devm_kzalloc() is harmful here. You're embedding a struct video_device instead
> struct sh_ceu_dev, and the video device can outlive the platform driver
> remove() call if the device node is kept open by userspace when the device is
> removed. You should use kzalloc() and implement a .release callback for the
> video_device to kfree() the memory.
>
Does this apply to video_unregister_device() as well?
Should I keep it registered after platform driver removal?
Other drivers (eg atmel-isc and pxa_camera) unregister the video
device in their sensor unbind callbacks.
As video device has pointer to fops embedded, if we unregister it at
sensor unbind time, does the ipotetical application having an open
reference to the device node lose the ability to properly close it?
Thanks
j
> Note that devm_ioremap_resource() and devm_request_irq() are fine as the MMIO
> and interrupts must not be used after the remove() function returns.
>
> > + if (!pcdev) {
> > + dev_err(&pdev->dev, "Could not allocate pcdev\n");
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_init(&pcdev->mlock);
> > + INIT_LIST_HEAD(&pcdev->capture);
> > + spin_lock_init(&pcdev->lock);
> > + init_completion(&pcdev->complete);
> > +
> > + pcdev->pdata = pdev->dev.platform_data;
> > + if (pdev->dev.of_node && !pcdev->pdata) {
>
> If there's an OF node there's no platform data, you can this code this as
>
> if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
> ...
> } else if (pcdev->pdata) {
> ...
> } else {
> ...
> }
>
> The IS_ENABLED(CONFIG_OF) will make sure the code is not compiled in for non-
> OF platforms.
>
> > + /* TODO: implement OF parsing */
> > + } else if (pcdev->pdata && !pdev->dev.of_node) {
> > + /* TODO: implement per-device bus flags */
>
> Is this needed ? I don't expect any new feature to be implemented for non-OF
> platforms.
>
> > + pcdev->max_width = pcdev->pdata->max_width;
> > + pcdev->max_height = pcdev->pdata->max_height;
> > + pcdev->flags = pcdev->pdata->flags;
> > + pcdev->asd.match_type = V4L2_ASYNC_MATCH_I2C;
> > + pcdev->asd.match.i2c.adapter_id =
> > + pcdev->pdata->sensor_i2c_adapter_id;
> > + pcdev->asd.match.i2c.address = pcdev->pdata-
> >sensor_i2c_address;
> > + } else {
> > + dev_err(&pdev->dev, "CEU platform data not set.\n");
> > + return -EINVAL;
> > + }
> > +
> > + pcdev->field = V4L2_FIELD_NONE;
> > +
> > + if (!pcdev->max_width)
> > + pcdev->max_width = 2560;
> > + if (!pcdev->max_height)
> > + pcdev->max_height = 1920;
> > +
> > + base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + pcdev->irq = irq;
> > + pcdev->base = base;
> > + pcdev->video_limit = 0; /* only enabled if second resource exists */
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (res) {
> > + err = dma_declare_coherent_memory(&pdev->dev, res->start,
> > + res->start,
> > + resource_size(res),
> > + DMA_MEMORY_MAP |
> > + DMA_MEMORY_EXCLUSIVE);
> > + if (!err) {
> > + dev_err(&pdev->dev, "Unable to declare CEU memory.
> \n");
> > + return -ENXIO;
> > + }
> > +
> > + pcdev->video_limit = resource_size(res);
>
> You can get rid of this, we now have CMA that can be used unconditionally to
> allocate physically contiguous memory. Don't forget to remove the
> corresponding resource from SH board code that instantiate CEU devices.
>
> > + }
> > +
> > + /* request irq */
>
> I'm not sure the comment is really valuable :-)
>
> > + err = devm_request_irq(&pdev->dev, pcdev->irq, sh_ceu_irq,
> > + 0, dev_name(&pdev->dev), pcdev);
> > + if (err) {
> > + dev_err(&pdev->dev, "Unable to register CEU interrupt.\n");
> > + goto exit_release_mem;
> > + }
> > +
> > + pm_suspend_ignore_children(&pdev->dev, true);
> > + pm_runtime_enable(&pdev->dev);
> > + pm_runtime_resume(&pdev->dev);
> > +
> > + dev_set_drvdata(&pdev->dev, pcdev);
> > + err = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
> > + if (err)
> > + goto exit_free_clk;
> > +
> > + pcdev->asds[0] = &pcdev->asd;
> > + pcdev->notifier.v4l2_dev = &pcdev->v4l2_dev;
> > + pcdev->notifier.subdevs = pcdev->asds;
> > + pcdev->notifier.num_subdevs = 1;
> > + pcdev->notifier.bound = sh_ceu_sensor_bound;
> > + pcdev->notifier.unbind = s
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Laurent, thanks for the extensive review On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > This is a partial review, as some of the comments will result in large changes > that would make review of some of the current code pointless. There is however > enough comments to keep you busy for a bit :-) > Indeed > By the way, you might want to keep your development history in a private git > branch, in order to bisect issues when you'll finally be able to test the > driver on real hardware. Otherwise we'll end up at v7, the driver won't work, > and you will have no idea why. > As many of your comments are on existing code that I blindly took from the existing driver, I decided to start fresh with a new driver from scratch. Next patches I will send will be based on your comments on this series, and I take the opportunity to ask some questions on some parts of code that were already in the driver and I'm failing to fully understand. To speed things up, I'm going to reply/make questions on code as I'm rewriting it, so let's start from the bottom of the driver. > On Thursday 27 Apr 2017 10:42:27 Jacopo Mondi wrote: > > Add driver for SH Mobile CEU driver, based on > > drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c > > > > The original driver was based on soc-camera framework. > > > > Signed-off-by: Jacopo Mondi > > --- > > drivers/media/platform/Kconfig |9 + > > drivers/media/platform/Makefile|2 + > > drivers/media/platform/sh_ceu_camera.c | 1597 + > > 3 files changed, 1608 insertions(+) > > create mode 100644 drivers/media/platform/sh_ceu_camera.c > > > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > > index c9106e1..afb10fd 100644 > > --- a/drivers/media/platform/Kconfig > > +++ b/drivers/media/platform/Kconfig > > @@ -114,6 +114,15 @@ config VIDEO_S3C_CAMIF > > To compile this driver as a module, choose M here: the module > > will be called s3c-camif. > > > > +config VIDEO_SH_MOBILE_CEU > > + tristate "SuperH Mobile CEU Interface driver" > > Maybe this should be renamed to include the RZ family ? > I have renamed config symbol in RENESAS_CEU and transformed all instances of "sh_ceu" in driver's code in "ceu_camera" or just "ceu" when appropriate (eg. "ceu_device" or "ceu_buffer") > > + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA && HAVE_CLK > > You don't use the clock API directly, I think you can drop HAVE_CLK. > > > + depends on ARCH_SHMOBILE || COMPILE_TEST > > + depends on HAS_DMA > > This is already included above. > > > + select VIDEOBUF2_DMA_CONTIG > > + ---help--- > > + This is a v4l2 driver for the SuperH Mobile CEU Interface > > + > > source "drivers/media/platform/soc_camera/Kconfig" > > source "drivers/media/platform/exynos4-is/Kconfig" > > source "drivers/media/platform/am437x/Kconfig" > > diff --git a/drivers/media/platform/Makefile > > b/drivers/media/platform/Makefile index 349ddf6..a2b9aa6 100644 > > --- a/drivers/media/platform/Makefile > > +++ b/drivers/media/platform/Makefile > > @@ -25,6 +25,8 @@ obj-$(CONFIG_VIDEO_CODA) += coda/ > > > > obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o > > > > +obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_ceu_camera.o > > + This will be renamed in "ceu_camera.c" as well > > obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)+= m2m-deinterlace.o > > > > obj-$(CONFIG_VIDEO_S3C_CAMIF) += s3c-camif/ > > diff --git a/drivers/media/platform/sh_ceu_camera.c > > b/drivers/media/platform/sh_ceu_camera.c new file mode 100644 > > index 000..07fe0e7 > > --- /dev/null > > +++ b/drivers/media/platform/sh_ceu_camera.c > > @@ -0,0 +1,1597 @@ > > +/* > > + * V4L2 Driver for SuperH Mobile CEU interface > > + * > > + * Copyright (C) 2017 Jacopo Mondi > > + * > > + * Based on soc-camera driver "soc_camera/sh_mobile_ceu_camera.c" > > + * > > + * Copyright (C) 2008 Magnus Damm > > + * > > + * Based on V4L2 Driver for PXA camera host - "pxa_camera.c", > > + * > > + * Copyright (C) 2006, Sascha Hauer, Pengutronix > > + * Copyright (C) 2008, Guennadi Liakhovetski > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > Please sort headers alphabetically, it makes locating duplicated easier. > yup > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +
Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties
Hi Linus,
On Fri, Apr 28, 2017 at 10:16:22AM +0200, Linus Walleij wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> wrote:
>
> > Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> > unpack generic properties and their arguments
> >
> > Signed-off-by: Jacopo Mondi
>
> (...)
>
> /*
> * Helpful configuration macro to be used in tables etc.
>
> Then this should say "macros" rather than "macro".
>
> > -#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
> > +#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))
>
> Also adding some extra parantheses I see.
>
> > +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
> > +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)
>
> But why.
>
> I have these two static inlines just below your new macros:
>
> static inline enum pin_config_param pinconf_to_config_param(unsigned
> long config)
> {
> return (enum pin_config_param) (config & 0xffUL);
> }
>
> static inline u32 pinconf_to_config_argument(unsigned long config)
> {
> return (u32) ((config >> 8) & 0xffUL);
> }
>
> Why can't you use this in your code instead of macros?
>
> We generally prefer static inlines over macros because they are easier
> to read.
>
Right. I haven't noticed them.
I'll drop this patch, sorry for noise
> Yours,
> Linus Walleij
Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller
Hi Geert, Simon,
On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> wrote:
> >this is 5th round of gpio/pincontroller for RZ/A1 devices.
> >
> > I have updated the pin controller driver to use the newly introduced
> > "pinctrl_enable()" function.
> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > the pin controller does not start.
> >
> > I have incorporated your comments on the device tree bindings documentation,
> > and added to pinctrl-generic.h header file two macros to unpack generic
> > properties and their arguments.
> >
> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
>
> Thanks for the update!
>
> > Jacopo Mondi (10):
> > pinctrl: generic: Add bi-directional and output-enable
>
> Already applied by LinusW.
>
> > pinctrl: generic: Add macros to unpack properties
>
> LinusW: do you want me to queue this together with the driver for v4.13,
> or will you take this single patch for v4.12?
>
> > pinctrl: Renesas RZ/A1 pin and gpio controller
> > dt-bindings: pinctrl: Add RZ/A1 bindings doc
>
> Will queue in sh-pfc-for-v4.13.
>
> > arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> > arm: dts: r7s72100: Add pin controller node
> > arm: dts: genmai: Add SCIF2 pin group
> > arm: dts: genmai: Add RIIC2 pin group
> > arm: dts: genmai: Add user led device nodes
> > arm: dts: genmai: Add ethernet pin group
>
> These are for Simon.
>
> Does applying the DTS changes before the driver introduce regressions?
> If no, Simon can queue them for v4.13.
> If yes, they'll have to wait for v4.14.
>
I tried reverting patch [03/10] which introduces the driver, and I
lose serial output on Genmai. So I guess the dts patches have to be
taken after the driver has been merged.
One question: why can't they be taken at the same time? (eg. for
v4.13?)
Thanks
j
> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [RFC v2 0/2] SH CEU camera driver
Hi Chris, On Thu, Apr 27, 2017 at 01:10:15PM +, Chris Brandt wrote: > Hi Jacopo, > > > The driver is -not tested- for lack of available platforms, just > > compiled in. > > For HW, it would be good if you use a OV7670 camera module for testing. > > Like this: > https://www.amazon.com/OV7670-Compatible-Arduino-Atomic-Market/dp/B00TKXAGZM/ref=sr_1_2?ie=UTF8 > > That's what we use for the RSK board. There is probably a header on the > GENMAI board as well (but you might have to solder the header down). > > You can just use jumper wires to hook it up. > Like these: > https://www.amazon.com/eBoot-Prototype-Solderless-Breadboard-Adhesive/dp/B06XKLRYRM/ref=sr_1_2 > > > There is also this REA (Renesas America) board (YLCDRZA1H) that comes with a > OV7670 module attached. > https://www.renesas.com/en-us/products/software-tools/boards-and-kits/renesas-demonstration-kits/ylcdrza1h.html > > Better board pictures here: > > https://www.digikey.com/product-detail/en/renesas-electronics-america/YLCDRZA1H/YLCDRZA1H-ND/6173788 > > I might be able to get you one of these boards if the GENMAI board doesn't > work out. > We just ported u-boot and kernel (3.14) to it this week. > > > NOTE: The way the RZ/A1 RSK board is laid out, you cannot use the LCD and CEU > at the same time. So for testing, I had to save each image as a JPEG and view > it off board. > > Thanks for the info. We planned to use the following setup to test the CEU driver with an actual HW platform GR-Peach + GR-Peach Audio Camera Shield + OV7670 evaluation module https://www.digikey.com/product-detail/en/renesas-electronics-america/YGRPEACHAUDIOCAMERASHIELD/YGRPEACHAUDIOCAMERASHIELD-ND/5800301 www.elecfreaks.com/estore/ov7670-camera-module.html This implies running mainline on Peach or backporting CEU driver to your BSP if we're not able to do so. A desirable collateral benefit in both cases, I guess... Thanks j > > helpers. > > That's fine for me. At the moment, the only users of the CEU just need > capture and are not using those other features. > > > > I'm particularly interested in feedbacks in size/bus_parameters > > configuration functions. While I have used most of the original driver > > code for functions dealing with hardware configuration, those two have > > changed quite a lot, as the number of FIXMEs and TODOs there show. > > > NOTE: Since the existing CEU driver only seemed to work in "image capture > mode" (meaning it split the Y and CbCr into different buffers), I wrote a > very simple driver to give to customers (non-V4L2) because they really wanted > to use "Data synchronous fetch mode" and just keep the YUV data exactly the > same as it comes out of the cameras. So, I would focus on that mode first. > Also, I can share my simple driver code with you as reference (I put some > extra comments in there). The CEU registers can be tricky to set up because > descriptions in the hardware manual are not always clear. > > > NOTE: "Data enable fetch mode" is broken in the RZ/A1H and RZ/A1M...and not > one really seems to want it anyway, so don't worry about that mode. > > > Chris >
Re: [PATCH v4 2/9] pinctrl: Renesas RZ/A1 pin and gpio controller
Hi Geert,
On Wed, Apr 26, 2017 at 02:21:34PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Wed, Apr 5, 2017 at 4:07 PM, Jacopo Mondi
> wrote:
> > Add combined gpio and pin controller driver for Renesas RZ/A1
> > r7s72100 SoC.
> >
> > Signed-off-by: Jacopo Mondi
>
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinctrl-rza1.c
>
> > +/*
> > + * Keep this up-to-date with pinconf-generic.h: it performs packing of
> > + * pin conf flags and argument during pinconf_generic_parse_dt_config();
> > + * we simply discard pinconf argument here
> > + */
> > +#define PIN_CONF_UNPACK(pinconf) ((pinconf) & 0xffUL)
>
> Perhaps this should be moved to pinconf-generic.h, to make sure it stays
> up-to-date?
>
Not sure, I'm discarding the argument of the configuration flag with
this macro...
I would keep this internal to this driver, or make two of them, one to
retrieve the flag, and one to retrieve argument..
> > +static inline int rza1_get_bit(struct rza1_port *port, unsigned int reg,
>
> I'd use "unsigned int" as the return type.
> It doesn't matter much as register values are 16-bit, but people might copy
> from this driver when writing their own.
>
> > + unsigned int bit)
> > +{
> > + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
> > +
> > + return ioread16(mem) & BIT(bit);
> > +}
>
> > +static inline int rza1_pin_get_direction(struct rza1_port *port,
> > +unsigned int pin)
> > +{
> > + unsigned long irqflags;
> > + int input;
> > +
> > + spin_lock_irqsave(&port->lock, irqflags);
> > + input = rza1_get_bit(port, RZA1_PM_REG, pin);
> > + spin_unlock_irqrestore(&port->lock, irqflags);
> > +
> > + return input;
>
> return !!input;
>
> gpio_chip.get_direction() should return 0, 1, or a negative error value.
>
> > +}
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v4 3/9] dt-bindings: pinctrl: Add RZ/A1 bindings doc
Hi Rob,
On Mon, Apr 10, 2017 at 01:12:15PM -0500, Rob Herring wrote:
> On Wed, Apr 05, 2017 at 04:07:21PM +0200, Jacopo Mondi wrote:
> > Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
> > controller.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > .../bindings/pinctrl/renesas,rza1-pinctrl.txt | 218
> > +
> > 1 file changed, 218 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
> > b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
> > new file mode 100644
> > index 000..46584ef
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
> > @@ -0,0 +1,218 @@
> > +Renesas RZ/A1 combined Pin and GPIO controller
> > +
> > +The Renesas SoCs of RZ/A1 family feature a combined Pin and GPIO
> > controller,
> > +named "Ports" in the hardware reference manual.
> > +Pin multiplexing and GPIO configuration is performed on a per-pin basis
> > +writing configuration values to per-port register sets.
> > +Each "port" features up to 16 pins, each of them configurable for GPIO
> > +function (port mode) or in alternate function mode.
> > +Up to 8 different alternate function modes exist for each single pin.
> > +
> > +Pin controller node
> > +---
> > +
> > +Required properties:
> > + - compatible
> > +this shall be "renesas,r7s72100-ports".
> > +
> > + - reg
> > +address base and length of the memory area where pin controller
> > +hardware is mapped to.
> > +
> > +Example:
> > +Pin controller node for RZ/A1H SoC (r7s72100)
> > +
> > +pinctrl: pin-controller@fcfe3000 {
> > + compatible = "renesas,r7s72100-ports";
> > +
> > + reg = <0xfcfe3000 0x4230>;
> > +};
> > +
> > +Sub-nodes
> > +-
> > +
> > +The child nodes of the pin controller node describe a pin multiplexing
> > +function or a gpio controller alternatively.
> > +
> > +- Pin multiplexing sub-nodes:
> > + A pin multiplexing sub-node describes how to configure a set of
> > + (or a single) pin in some desired alternate function mode.
> > + A single sub-node may define several pin configurations.
> > + Some alternate functions require special pin configuration flags to be
> > + supplied along with the alternate function configuration number.
> > + When hardware reference manual specifies a pin function to be either
> > + "bi-directional" or "software IO driven", use the generic properties from
> > + header file to instruct the
> > + pin controller to perform the desired pin configuration operations.
> > + Please refer to pinctrl-bindings.txt to get to know more on generic
> > + pin properties usage.
> > +
> > + The allowed generic formats for a pin multiplexing sub-node are the
> > + following ones:
> > +
> > + node-1 {
> > + pinmux = , , ... ;
> > + GENERIC_PINCONFIG;
>
> What's GENERIC_PINCONFIG? I see this in some other binding docs, but not
> used anywhere. If this is a boolean property then get rid of the all
> caps. If this is a define, then don't use complex defines that expand to
> dts source.
GENERIC_PINCONF is a wildcard that identifies "generic" pin
configuration properties the pin controller framework defines.
Have a look at "enum pin_config_param" in
Thanks
j
Re: [PATCH v5 1/4] Documentation: dt-bindings: iio: Add max9611 ADC
Hi Jonathan,
On Sat, Apr 08, 2017 at 04:59:39PM +0100, Jonathan Cameron wrote:
> On 06/04/17 15:43, Geert Uytterhoeven wrote:
>
> > On Thu, Apr 6, 2017 at 4:20 PM, Jacopo Mondi
> > wrote:
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/max9611.txt
> >> @@ -0,0 +1,27 @@
> >> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
> >> +
> >> +Maxim max9611/max9612 is an high-side current sense amplifier with
> >> integrated
> >> +12-bits ADC communicating over I2c bus.
> >> +The device node for this driver shall be a child of a I2c controller.
> >> +
> >> +Required properties
> >> + - compatible: Should be "maxim,max9611" or "maxim,max9612"
> >> + - reg: The 7-bits long I2c address of the device
> >> + - shunt-resistor-micro-homs: Value, in micro Ohms, of the current sense
> >> shunt
> >
> > s/homs/ohms/
> I'll fix the title as Rob requested and this.
>
Uh! I missed that title thing! Sorry about this, and thank you
Jonathan for doing that!
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with it.
>
> Thanks for your hard work on this one!
>
Thank you all for your effort in reviewing this
Thanks
j
> Jonathan
> >
> >> + resistor
> >
> > Gr{oetje,eeting}s,
> >
> > Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > ge...@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like
> > that.
> > -- Linus Torvalds
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
Re: [PATCH] arm64: dts: r8a7795: salvator-x: Add current sense amplifiers
Hi Geert, Simon, On Tue, Mar 28, 2017 at 02:16:38PM +0200, Simon Horman wrote: > On Tue, Mar 28, 2017 at 11:44:42AM +0200, Geert Uytterhoeven wrote: > > Add device nodes for two Maxim max961x current sense amplifiers > > sensing the VDD_0.8V and DVFS_0.8V lines. > > > > Based on a patch for r8a7796-salvator-x.dts by Jacopo Mondi. > > > > Signed-off-by: Geert Uytterhoeven > > --- > > Pending acceptance of the maxim,max9611 DT bindings. > > I have marked this as deferred accordingly. Just to point out that in v5 I just sent out, bindings are different and this patch needs an update. Thanks j
