* Re: [PATCH RFC 0/5] *** SPI Slave mode support ***
From: Mark Brown @ 2017-04-25 10:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jiada Wang, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Fabio Estevam, linux-spi,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <CAMuHMdXa_O6RsqUciiNWQ0Zp6dniS47AUmPN9UWAWitP0csx=Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]
On Mon, Apr 24, 2017 at 12:55:21PM +0200, Geert Uytterhoeven wrote:
> On Fri, Apr 14, 2017 at 7:39 AM, Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> > Our use case is to use spidev as an interface to communicate with external
> > SPI master devices.
> > meanwhile the SPI bus controller can also act as master device to send data
> > to other
> > SPI slave devices on the board.
> That sounds a bit hackish to me. SPI was never meant to be a multi-master bus.
> While it can be done, you will need external synchronization (signals) to
> avoid conflicts between the SPI masters.
> > I found in your implementation, SPI bus controller is limited to either work
> > in master mode or
> > slave mode, is there any reasoning to not configure SPI mode based on SPI
> > devices use case?
> If you really need both master and slave support, you can use 2 subnodes
> in DT, the first representing the master, the second the slave.
> Mark, what's your opinion about this?
That sounds like a mess... we *could* put the slave flag on the device
rather than the controller I guess but there's also going to need to be
something representing whatever avoids collisions on the bus somewhere.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 10/18] pinctrl: madera: Add driver for Cirrus Logic Madera codecs
From: Richard Fitzgerald @ 2017-04-25 10:26 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, alsa-devel@alsa-project.org, Jason Cooper,
devicetree@vger.kernel.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-kernel@vger.kernel.org, Rob Herring,
linux-gpio@vger.kernel.org, Mark Brown, Thomas Gleixner,
Lee Jones
In-Reply-To: <CACRpkdYtuu4mFf9bjz-z_kNhbgmuw7TQdQqBc66iWfFP5wp_fw@mail.gmail.com>
On Tue, 2017-04-25 at 11:41 +0200, Linus Walleij wrote:
> On Mon, Apr 24, 2017 at 6:08 PM, Richard Fitzgerald
> <rf@opensource.wolfsonmicro.com> wrote:
>
> > These codecs have a variable number of I/O lines each of which
> > is individually selectable to a wide range of possible functions.
> >
> > The functionality is slightly different from the traditional muxed
> > GPIO since most of the functions can be mapped to any pin (and even
> > the same function to multiple pins). Most pins have a dedicated
> > "alternate" function that is only available on that pin. The
> > alternate functions are usually a group of signals, though it is
> > not always necessary to enable the full group, depending on the
> > alternate function and how it is to be used. The mapping between
> > alternate functions and GPIO pins varies between codecs depending
> > on the number of alternate functions and available pins.
> >
> > Note on the Kconfig options:
> > The formula "default y if..." is used for PINCTRL_MADERA so that its
> > select options will be processed, allowing us to group selects for
> > pinctrl into the pinctrl Kconfig where they logically belong instead
> > of accumulating under the parent MFD Kconfig.
> >
> > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> (...)
>
> > Changes since V1:
>
> This is starting to look good!
>
> In fact, kind of finished. Except:
>
> > + if (pdata) {
> > + ret = pinctrl_register_mappings(pdata->gpio_configs,
> > + pdata->n_gpio_configs);
> > + if (ret) {
> > + dev_err(priv->dev,
> > + "Failed to register pdata mappings (%d)\n",
> > + ret);
> > + return ret;
> > + }
> > + }
>
> What is this? It looks like boardfile support which we do not like to
> encourage. Why are the mappings not exclusively set up from
> device tree or similar?
>
We want to work on systems that don't use DT for configuration, so we
offer pdata as a fallback across all the drivers. In that case the
configuration is defined as part of the parent pdata struct and this
applies it.
> Yours,
> Linus Walleij
^ permalink raw reply
* RE: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
From: Ryan Chen @ 2017-04-25 9:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Brendan Higgins
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Joel Stanley, Vladimir Zapolskiy,
Kachalov Anton, Cédric Le Goater,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Kernel Mailing List, OpenBMC Maillist
In-Reply-To: <1493112875.25766.268.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 18710 bytes --]
Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?
About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it.
If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.
-----Original Message-----
From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
Sent: Tuesday, April 25, 2017 5:35 PM
To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins <brendanhiggins@google.com>
Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
> Hello All,
> ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.Â
> For example, if i2c bus is use on "high speed" and "single slave and
> master" and i2c bus is too long. It need drive SDA or SCL less lunacy.
> It would enable it.
> Otherwise, don’t enable it. especially in multi-master.
> It can’t be enable.
That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").
Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ? Does it force to a specific speed (ignoring the
divisor) or we can still play with the clock high/low counts ?
Cheers,
Ben.
> Â Â
>
>
> Best Regards,
> Ryan
>
> 信驊科技股份有é™å…¬å¸
> ASPEED Technology Inc.
> 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City
> 30077, Taiwan
> Tel: 886-3-578-9568Â #857
> Fax: 886-3-578-9586
> ************* Email Confidentiality Notice ********************
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged
> and/or other confidential information. If you have received it in
> error, please notify the sender by reply e-mail and immediately delete
> the e-mail and any attachments without copying or disclosing the
> contents. Thank you.
>
>
> -----Original Message-----
> From: Brendan Higgins [mailto:brendanhiggins@google.com]
> Sent: Tuesday, April 25, 2017 4:32 PM
> To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org
> >; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutro
> nix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyng
> ier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@m
> leia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod
> .org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
>
> Adding Ryan.
>
> On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.
> crashing.org> wrote:
> > On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > > > +struct aspeed_i2c_bus {
> > > > > +     struct i2c_adapter              adap;
> > > > > +     struct device                   *dev;
> > > > > +     void __iomem                    *base;
> > > > > +Â Â Â Â Â /* Synchronizes I/O mem access to base. */
> > > > > +     spinlock_t                      lock;
> > > >
> > > > I am not entirely convinced we need that lock. The i2c core will
> > > > take a mutex protecting all operations on the bus. So we only
> > > > need to synchronize between our "xfer" code and our interrupt
> > > > handler.
> > >
> > > You are right if both having slave and master active at the same
> > > time was not possible; however, it is.
> >
> > Right, I somewhat forgot about the slave case.
> >
> > Â ...
> >
> > > > Some of those error states probably also warrant a reset of the
> > > > controller, I think aspeed does that in the SDK.
> > >
> > > For timeout and cmd_err, I do not see any argument against it; it
> > > sounds like we are in a very messed up, very unknown state, so
> > > full reset is probably the best last resort.
> >
> > Yup.
> >
> > > For SDA staying pulled down, I
> > > think we can say with reasonable confidence that some device on
> > > our bus is behaving very badly and I am not convinced that
> > > resetting the controller is likely to do anything to help;
> >
> > Right. Hammering with STOPs and pray ...
>
> I think sending recovery mode sends stops as a part of the recovery
> algorithm it executes.
>
> >
> > > Â that being said, I really
> > > do not have any good ideas to address that. So maybe praying and
> > > resetting the controller is *the most reasonable thing to do.* I
> > > would like to know what you think we should do in that case.
> >
> > Well, there's a (small ?) chance that it's a controller bug
> > asserting the line so ... but there's little we can do if not.
>
> True.
>
> >
> > > While I was thinking about this I also realized that the SDA line
> > > check after recovery happens in the else branch, but SCL line
> > > check does not happen after we attempt to STOP if SCL is hung. If
> > > we decide to make special note SDA being hung by a device that
> > > won't let go, we might want to make a special note that SCL is
> > > hung by a device that won't let go. Just a thought.
> >
> > Maybe. Or just "unrecoverable error"... hopefully these don't happen
> > too often ... We had cases of a TPM misbehaving like that.
>
> Yeah, definitely should print something out.
>
> >
> > > > > +out:
> > >
> > > ...
> > > > What about I2C_M_NOSTART ?
> > > >
> > > > Not that I've ever seen it used... ;-)
> > >
> > > Right now I am not doing any of the protocol mangling options, but
> > > I can add them in if you think it is important for initial
> > > support.
> >
> > No, not important, we can add that later if it ever becomes useful.
> >
> > Â ...
> >
> > > > In general, you always ACK all interrupts first. Then you handle
> > > > the bits you have harvested.
> > > >
> > >
> > > The documentation says to ACK the interrupt after handling in the
> > > RX
> > > case:
> > >
> > > <<<
> > > S/W needs to clear this status bit to allow next data receiving.
> > > > > >
> > >
> > > I will double check with Ryan to make sure TX works the same way.
> > >
> > > > > +Â Â Â Â Â if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > > > +Â Â Â Â Â Â Â Â Â (!bus->msgs && bus->master_state !=
> > > > > ASPEED_I2C_MASTER_STOP)) {
> > >
> > > ...
> > > >
> > > > I would set master_state to "RECOVERY" (new state ?) and ensure
> > > > those things are caught if they happen outside of a recovery.
> >
> > I replied privately ... as long as we ack before we start a new
> > command we should be ok but we shouldn't ack after.
> >
> > Your latest patch still does that. It will do things like start a
> > STOP command *then* ack the status bits. I'm pretty sure that's
> > bogus.
> >
> > That way it's a lot simpler to simply move the
> >
> > Â Â Â Â Â Â Â Â writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> >
> > To either right after the readl of the status reg at the beginning
> > of aspeed_i2c_master_irq().
> >
> > I would be very surprised if that didn't work properly and wasn't
> > much safer than what you are currently doing.
>
> I think I tried your way and it worked. In anycase, Ryan will be able
> to clarify for us.
>
> >
> > > Let me know if you still think we need a "RECOVERY" state.
> >
> > The way you just switch to stop state and store the error for later
> > should work I think.
> >
> > > >
> > > > > +Â Â Â Â Â if (bus->master_state == ASPEED_I2C_MASTER_START) {
> > >
> > > ...
> > > >
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev_dbg(bus->dev,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "no slave present at %02x",
> > > > > msg-
> > > > > > addr);
> > > > >
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bus->cmd_err = -EIO;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â do_stop(bus);
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto out_no_complete;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â } else {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (msg->flags & I2C_M_RD)
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bus->master_state =
> > > > > ASPEED_I2C_MASTER_RX;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â else
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bus->master_state =
> > > > > ASPEED_I2C_MASTER_TX_FIRST;
> > > >
> > > > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> > > > to handle this here ? A quick look at the TX_FIRST case makes me
> > > > think we are ok there but I'm not sure about the RX case.
> > >
> > > I did not think that there is an SMBUS_QUICK RX. Could you point
> > > me to an example?
> >
> > Not so much an RX, it's more like you are sending a 1-bit data in
> > the place of the Rd/Wr bit. So you have a read with a lenght of 0, I
> > don't think in that case you should set ASPEED_I2CD_M_RX_CMD in
> > __aspeed_i2c_do_start
>
> Forget what I said, I was just not thinking about the fact that SMBus
> emulation causes the data bit to be encoded as the R/W flag. I see
> what you are saying; you are correct.
>
> >
> > > > I'm not sure the RX case is tight also. What completion does the
> > > > HW give you for the address cycle ? Won't you get that before it
> > > > has received the first character ? IE. You fall through to the
> > > > read case of the state machine with the read potentially not
> > > > complete yet no ?
> > >
> > > ...
> > > > > +Â Â Â Â Â case ASPEED_I2C_MASTER_RX:
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev_err(bus->dev, "master failed to
> > > > > RX");
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto out_complete;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â }
> > > >
> > > > See my comment above for a bog standard i2c_read. Aren't you
> > > > getting the completion for the address before the read is even
> > > > started ?
> > >
> > > In practice no, but it is probably best to be safe :-)
> >
> > Yup :)
> > > >
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > > > +
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â recv_byte = aspeed_i2c_read(bus,
> > > > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â msg->buf[bus->buf_index++] = recv_byte;
> > > > > +
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â if (msg->flags & I2C_M_RECV_LEN &&
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â msg->len = recv_byte +
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ((msg->flags &
> > > > > I2C_CLIENT_PEC) ? 2 : 1);
> > >
> > > ...
> > > > > +Â Â Â Â Â return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â | ((clk_low <<
> > > > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â | (base_clk &
> > > > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > > > +}
> > > >
> > > > As I think I mentioned earlier, the AST2500 has a slightly
> > > > different register layout which support larger values for high
> > > > and low, thus allowing a finer granularity.
> > >
> > > I am developing against the 2500.
> >
> > Yes but we'd like the driver to work with both :-)
>
> Right, I thought you were making an assertion about the 2500, if you
> are making an assertion about the 2400, I do not know and do not have
> one handy.
>
> >
> > > > BTW. In case you haven't, I would suggest you copy/paste the
> > > > above in a userspace app and run it for all frequency divisors
> > > > and see if your results match the aspeed table :)
> > >
> > > Good call.
> >
> > If you end up doing that, can you shoot it my way ? I can take care
> > of making sure it's all good for the 2400.
>
> Will do.
>
> >
> > > > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct platform_device *pdev) {
> > > > > +Â Â Â Â Â u32 clk_freq, divisor;
> > > > > +Â Â Â Â Â struct clk *pclk;
> > > > > +Â Â Â Â Â int ret;
> > > > > +
> > > > > +Â Â Â Â Â pclk = devm_clk_get(&pdev->dev, NULL);
> > > > > +Â Â Â Â Â if (IS_ERR(pclk)) {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â dev_err(&pdev->dev, "clk_get failed\n");
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â return PTR_ERR(pclk);
> > > > > +Â Â Â Â Â }
> > > > > +Â Â Â Â Â ret = of_property_read_u32(pdev->dev.of_node,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "clock-frequency",
> > > > > &clk_freq);
> > > >
> > > > See my previous comment about calling that 'bus-frequency'
> > > > rather
> > > > than 'clock-frequency'.
> > > >
> > > > > +Â Â Â Â Â if (ret < 0) {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â dev_err(&pdev->dev,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "Could not read clock-frequency
> > > > > property\n");
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â clk_freq = 100000;
> > > > > +Â Â Â Â Â }
> > > > > +Â Â Â Â Â divisor = clk_get_rate(pclk) / clk_freq;
> > > > > +Â Â Â Â Â /* We just need the clock rate, we don't actually use
> > > > > the
> > > > > clk object. */
> > > > > +Â Â Â Â Â devm_clk_put(&pdev->dev, pclk);
> > > > > +
> > > > > +Â Â Â Â Â /* Set AC Timing */
> > > > > +Â Â Â Â Â if (clk_freq / 1000 > 1000) {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2
> > > > > C_FU
> > > > > N_CTRL_REG) |
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2CD_M_HIGH_SPEED_EN |
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2CD_SDA_DRIVE_1T_EN,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2C_FUN_CTRL_REG);
> > > > > +
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â aspeed_i2c_write(bus, 0x3,
> > > > > ASPEED_I2C_AC_TIMING_REG2);
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2C_AC_TIMING_REG1);
> > > >
> > > > I already discussed by doubts about the above. I can try to
> > > > scope it with the EVB if you don't get to it. For now I'd rather
> > > > take the code out.
> > > >
> > > > We should ask aspeed from what frequency the "1T" stuff is
> > > > useful.
> > >
> > > Will do, I will try to rope Ryan in on the next review; it will be
> > > good for him to get used to working with upstream anyway.
> >
> > Yup. However, for the sake of getting something upstream (and in
> > OpenBMC 4.10 kernel) asap, I would suggest just dropping support for
> > those fast speeds for now, we can add them back later.
>
> Alright, that's fine. Still, Ryan, could you provide some context on
> this?
>
> >
> > > >
> > > > > +Â Â Â Â Â } else {
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2C_AC_TIMING_REG1);
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ASPEED_I2C_AC_TIMING_REG2);
> > > > > +Â Â Â Â Â }
> > >
> > > ...
> > > > > +Â Â Â Â Â spin_lock_init(&bus->lock);
> > > > > +Â Â Â Â Â init_completion(&bus->cmd_complete);
> > > > > +Â Â Â Â Â bus->adap.owner = THIS_MODULE;
> > > > > +Â Â Â Â Â bus->adap.retries = 0;
> > > > > +Â Â Â Â Â bus->adap.timeout = 5 * HZ;
> > > > > +Â Â Â Â Â bus->adap.algo = &aspeed_i2c_algo;
> > > > > +Â Â Â Â Â bus->adap.algo_data = bus;
> > > > > +Â Â Â Â Â bus->adap.dev.parent = &pdev->dev;
> > > > > +Â Â Â Â Â bus->adap.dev.of_node = pdev->dev.of_node;
> > > > > +Â Â Â Â Â snprintf(bus->adap.name, sizeof(bus->adap.name),
> > > > > "Aspeed
> > > > > i2c");
> > > >
> > > > Another trivial one, should we put some kind of bus number in
> > > > that string ?
> > >
> > > Whoops, looks like I missed this one; I will get to it in the next
> > > revision.
> >
> > Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
>
> Yep, I will get it in v8.
>
> >
> > > >
> > > > > +Â Â Â Â Â bus->dev = &pdev->dev;
> > > > > +
> > > > > +Â Â Â Â Â /* reset device: disable master & slave functions */
> > > > > +Â Â Â Â Â aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> > >
> > > ...
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > devicetree"
> > > in
> > > the body of a message to majordomo@vger.kernel.org More majordomo
> > > info at  http://vger.kernel.org/majordomo-info.html
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i
^ permalink raw reply
* Re: [PATCH v2 13/18] dt-bindings: gpio: Add bindings for GPIO on Cirrus Logic Madera codecs
From: Linus Walleij @ 2017-04-25 9:42 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: Alexandre Courbot, alsa-devel@alsa-project.org, Jason Cooper,
devicetree@vger.kernel.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-kernel@vger.kernel.org, Rob Herring,
linux-gpio@vger.kernel.org, Mark Brown, Thomas Gleixner,
Lee Jones
In-Reply-To: <1493050124-5970-14-git-send-email-rf@opensource.wolfsonmicro.com>
On Mon, Apr 24, 2017 at 6:08 PM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:
> The Cirrus Logic Madera codecs have a range of GPIO pins that can be
> used as single-bit logic input or output. These are presented as a
> standard GPIO binding.
>
> The second cell in a GPIO binding is currently reserved for future use
> as chip-specific flags.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
> Changes from V1:
> - moved out of main gpio patch into a separate patch
> - added gpio-line-names as an optional property
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 10/18] pinctrl: madera: Add driver for Cirrus Logic Madera codecs
From: Linus Walleij @ 2017-04-25 9:41 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: Lee Jones, Mark Brown, Alexandre Courbot, Rob Herring,
Thomas Gleixner, Jason Cooper, alsa-devel@alsa-project.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1493050124-5970-11-git-send-email-rf@opensource.wolfsonmicro.com>
On Mon, Apr 24, 2017 at 6:08 PM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:
> These codecs have a variable number of I/O lines each of which
> is individually selectable to a wide range of possible functions.
>
> The functionality is slightly different from the traditional muxed
> GPIO since most of the functions can be mapped to any pin (and even
> the same function to multiple pins). Most pins have a dedicated
> "alternate" function that is only available on that pin. The
> alternate functions are usually a group of signals, though it is
> not always necessary to enable the full group, depending on the
> alternate function and how it is to be used. The mapping between
> alternate functions and GPIO pins varies between codecs depending
> on the number of alternate functions and available pins.
>
> Note on the Kconfig options:
> The formula "default y if..." is used for PINCTRL_MADERA so that its
> select options will be processed, allowing us to group selects for
> pinctrl into the pinctrl Kconfig where they logically belong instead
> of accumulating under the parent MFD Kconfig.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
(...)
> Changes since V1:
This is starting to look good!
In fact, kind of finished. Except:
> + if (pdata) {
> + ret = pinctrl_register_mappings(pdata->gpio_configs,
> + pdata->n_gpio_configs);
> + if (ret) {
> + dev_err(priv->dev,
> + "Failed to register pdata mappings (%d)\n",
> + ret);
> + return ret;
> + }
> + }
What is this? It looks like boardfile support which we do not like to
encourage. Why are the mappings not exclusively set up from
device tree or similar?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.
From: Sergei Shtylyov @ 2017-04-25 9:41 UTC (permalink / raw)
To: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
netdev, Rob Herring, Mark Rutland, devicetree
Cc: linux-arm-kernel, linux-kernel, bcm-kernel-feedback-list, Ray Jui,
Scott Branden, Jon Mason
In-Reply-To: <20170424215022.30382-3-eric@anholt.net>
On 4/25/2017 12:50 AM, Eric Anholt wrote:
> Cygnus has a single amac controller connected to the B53 switch with 2
> PHYs. On the BCM911360_EP platform, those two PHYs are connected to
> the external ethernet jacks.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> arch/arm/boot/dts/bcm-cygnus.dtsi | 60 ++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/bcm911360_entphn.dts | 8 +++++
> 2 files changed, 68 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 009f1346b817..318899df9972 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
[...]
> @@ -295,6 +345,16 @@
> status = "disabled";
> };
>
> + eth0: enet@18042000 {
Oh, and this one should be named "ethernet", according to the DT spec.
> + compatible = "brcm,amac";
> + reg = <0x18042000 0x1000>,
> + <0x18110000 0x1000>;
> + reg-names = "amac_base", "idm_base";
> + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> + max-speed = <1000>;
> + status = "disabled";
> + };
> +
> nand: nand@18046000 {
> compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
> reg = <0x18046000 0x600>, <0xf8105408 0x600>,
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.
From: Sergei Shtylyov @ 2017-04-25 9:40 UTC (permalink / raw)
To: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
netdev, Rob Herring, Mark Rutland, devicetree
Cc: linux-arm-kernel, linux-kernel, bcm-kernel-feedback-list, Ray Jui,
Scott Branden, Jon Mason
In-Reply-To: <20170424215022.30382-3-eric@anholt.net>
Hello.
On 4/25/2017 12:50 AM, Eric Anholt wrote:
> Cygnus has a single amac controller connected to the B53 switch with 2
> PHYs. On the BCM911360_EP platform, those two PHYs are connected to
> the external ethernet jacks.
My spell checker trips on "amac" and "ethernet" -- perhaps they need
capitalization?
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> arch/arm/boot/dts/bcm-cygnus.dtsi | 60 ++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/bcm911360_entphn.dts | 8 +++++
> 2 files changed, 68 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 009f1346b817..318899df9972 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
> @@ -142,6 +142,56 @@
> interrupts = <0>;
> };
>
> + mdio: mdio@18002000 {
> + compatible = "brcm,iproc-mdio";
> + reg = <0x18002000 0x8>;
> + #size-cells = <1>;
> + #address-cells = <0>;
> +
> + gphy0: eth-gphy@0 {
The node anmes must be generic, the DT spec has standardized
"ethernet-phy" name for this case.
> + reg = <0>;
> + max-speed = <1000>;
> + };
> +
> + gphy1: eth-gphy@1 {
> + reg = <1>;
> + max-speed = <1000>;
> + };
> + };
[...]
> @@ -295,6 +345,16 @@
> status = "disabled";
> };
>
> + eth0: enet@18042000 {
> + compatible = "brcm,amac";
> + reg = <0x18042000 0x1000>,
> + <0x18110000 0x1000>;
> + reg-names = "amac_base", "idm_base";
I don't think "_base" suffixes are necessary here.
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] net: dsa: b53: Add compatible strings for the Cygnus-family BCM11360.
From: Sergei Shtylyov @ 2017-04-25 9:35 UTC (permalink / raw)
To: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
netdev, Rob Herring, Mark Rutland, devicetree
Cc: Scott Branden, Jon Mason, Ray Jui, linux-kernel,
bcm-kernel-feedback-list, linux-arm-kernel
In-Reply-To: <20170424215022.30382-2-eric@anholt.net>
Hello!
On 4/25/2017 12:50 AM, Eric Anholt wrote:
> Cygnus is a small family of SoCs, of which we currently have
> devicetree for BCM11360 and BCM58300. The 11360's B53 is mostly the
> same as 58xx, just requiring a tiny bit of setup that was previously
> missing.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> Documentation/devicetree/bindings/net/dsa/b53.txt | 3 +++
> drivers/net/dsa/b53/b53_srab.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/b53.txt b/Documentation/devicetree/bindings/net/dsa/b53.txt
> index d6c6e41648d4..49c93d3c0839 100644
> --- a/Documentation/devicetree/bindings/net/dsa/b53.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/b53.txt
> @@ -29,6 +29,9 @@ Required properties:
> "brcm,bcm58625-srab"
> "brcm,bcm88312-srab" and the mandatory "brcm,nsp-srab string
>
> + For the BCM11360 SoC, must be:
> + "brcm,bcm11360-srab" and the mandatory "brcm,cygnus-srab string
Missing closing quote here and above?
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v2 11/18] dt-bindings: pinctrl: Add bindings for Cirrus Logic Madera codecs
From: Linus Walleij @ 2017-04-25 9:35 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: Alexandre Courbot, alsa-devel@alsa-project.org, Jason Cooper,
devicetree@vger.kernel.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-kernel@vger.kernel.org, Rob Herring,
linux-gpio@vger.kernel.org, Mark Brown, Thomas Gleixner,
Lee Jones
In-Reply-To: <1493050124-5970-12-git-send-email-rf@opensource.wolfsonmicro.com>
On Mon, Apr 24, 2017 at 6:08 PM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:
> This is the binding description of the pinctrl driver for Cirru Logic
> Madera codecs. The binding uses the generic pinctrl binding so the main
> purpose here is to describe the device-specific names for groups and
> functions.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
> Changes since V1:
> - split out from main pinctrl patch into a separate patch
> - fixed typo in pinctrl-names property description
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
From: Benjamin Herrenschmidt @ 2017-04-25 9:34 UTC (permalink / raw)
To: Ryan Chen, Brendan Higgins
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Joel Stanley, Vladimir Zapolskiy,
Kachalov Anton, Cédric Le Goater, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, Linux Kernel Mailing List,
OpenBMC Maillist
In-Reply-To: <7d991733d3ea4591b74baf93300f0224@TWMBX01.aspeed.com>
On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
> Hello All,
> ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.
> For example, if i2c bus is use on "high speed" and
> "single slave and master" and i2c bus is too long. It need drive SDA
> or SCL less lunacy. It would enable it.
> Otherwise, don’t enable it. especially in multi-master.
> It can’t be enable.
That smells like a specific enough use case that we should probably
cover with a device-tree property, something like an empty
"sda-extra-drive" property (empty properties are typically used
for booleans, their presence means "true").
Thanks Ryan. Can you shed some light on the meaning of the high-speed
bit as well please ? Does it force to a specific speed (ignoring the
divisor) or we can still play with the clock high/low counts ?
Cheers,
Ben.
>
>
>
> Best Regards,
> Ryan
>
> 信驊科技股份有限公司
> ASPEED Technology Inc.
> 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City
> 30077, Taiwan
> Tel: 886-3-578-9568 #857
> Fax: 886-3-578-9586
> ************* Email Confidentiality Notice ********************
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged
> and/or other confidential information. If you have received it in
> error, please notify the sender by reply e-mail and immediately
> delete the e-mail and any attachments without copying or disclosing
> the contents. Thank you.
>
>
> -----Original Message-----
> From: Brendan Higgins [mailto:brendanhiggins@google.com]
> Sent: Tuesday, April 25, 2017 4:32 PM
> To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org
> >; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutro
> nix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyng
> ier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@m
> leia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod
> .org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
>
> Adding Ryan.
>
> On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.
> crashing.org> wrote:
> > On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > > > +struct aspeed_i2c_bus {
> > > > > + struct i2c_adapter adap;
> > > > > + struct device *dev;
> > > > > + void __iomem *base;
> > > > > + /* Synchronizes I/O mem access to base. */
> > > > > + spinlock_t lock;
> > > >
> > > > I am not entirely convinced we need that lock. The i2c core
> > > > will
> > > > take a mutex protecting all operations on the bus. So we only
> > > > need
> > > > to synchronize between our "xfer" code and our interrupt
> > > > handler.
> > >
> > > You are right if both having slave and master active at the same
> > > time
> > > was not possible; however, it is.
> >
> > Right, I somewhat forgot about the slave case.
> >
> > ...
> >
> > > > Some of those error states probably also warrant a reset of
> > > > the
> > > > controller, I think aspeed does that in the SDK.
> > >
> > > For timeout and cmd_err, I do not see any argument against it;
> > > it
> > > sounds like we are in a very messed up, very unknown state, so
> > > full
> > > reset is probably the best last resort.
> >
> > Yup.
> >
> > > For SDA staying pulled down, I
> > > think we can say with reasonable confidence that some device on
> > > our
> > > bus is behaving very badly and I am not convinced that resetting
> > > the
> > > controller is likely to do anything to help;
> >
> > Right. Hammering with STOPs and pray ...
>
> I think sending recovery mode sends stops as a part of the recovery
> algorithm it executes.
>
> >
> > > that being said, I really
> > > do not have any good ideas to address that. So maybe praying and
> > > resetting the controller is *the most reasonable thing to do.* I
> > > would like to know what you think we should do in that case.
> >
> > Well, there's a (small ?) chance that it's a controller bug
> > asserting
> > the line so ... but there's little we can do if not.
>
> True.
>
> >
> > > While I was thinking about this I also realized that the SDA
> > > line
> > > check after recovery happens in the else branch, but SCL line
> > > check
> > > does not happen after we attempt to STOP if SCL is hung. If we
> > > decide
> > > to make special note SDA being hung by a device that won't let
> > > go, we
> > > might want to make a special note that SCL is hung by a device
> > > that
> > > won't let go. Just a thought.
> >
> > Maybe. Or just "unrecoverable error"... hopefully these don't
> > happen
> > too often ... We had cases of a TPM misbehaving like that.
>
> Yeah, definitely should print something out.
>
> >
> > > > > +out:
> > >
> > > ...
> > > > What about I2C_M_NOSTART ?
> > > >
> > > > Not that I've ever seen it used... ;-)
> > >
> > > Right now I am not doing any of the protocol mangling options,
> > > but I
> > > can add them in if you think it is important for initial support.
> >
> > No, not important, we can add that later if it ever becomes useful.
> >
> > ...
> >
> > > > In general, you always ACK all interrupts first. Then you
> > > > handle
> > > > the bits you have harvested.
> > > >
> > >
> > > The documentation says to ACK the interrupt after handling in the
> > > RX
> > > case:
> > >
> > > <<<
> > > S/W needs to clear this status bit to allow next data receiving.
> > > > > >
> > >
> > > I will double check with Ryan to make sure TX works the same way.
> > >
> > > > > + if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > > > + (!bus->msgs && bus->master_state !=
> > > > > ASPEED_I2C_MASTER_STOP)) {
> > >
> > > ...
> > > >
> > > > I would set master_state to "RECOVERY" (new state ?) and
> > > > ensure
> > > > those things are caught if they happen outside of a recovery.
> >
> > I replied privately ... as long as we ack before we start a new
> > command we should be ok but we shouldn't ack after.
> >
> > Your latest patch still does that. It will do things like start a
> > STOP
> > command *then* ack the status bits. I'm pretty sure that's bogus.
> >
> > That way it's a lot simpler to simply move the
> >
> > writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> >
> > To either right after the readl of the status reg at the beginning
> > of
> > aspeed_i2c_master_irq().
> >
> > I would be very surprised if that didn't work properly and wasn't
> > much
> > safer than what you are currently doing.
>
> I think I tried your way and it worked. In anycase, Ryan will be able
> to clarify for us.
>
> >
> > > Let me know if you still think we need a "RECOVERY" state.
> >
> > The way you just switch to stop state and store the error for
> > later
> > should work I think.
> >
> > > >
> > > > > + if (bus->master_state == ASPEED_I2C_MASTER_START) {
> > >
> > > ...
> > > >
> > > > > + dev_dbg(bus->dev,
> > > > > + "no slave present at %02x",
> > > > > msg-
> > > > > > addr);
> > > > >
> > > > > + status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > > > + bus->cmd_err = -EIO;
> > > > > + do_stop(bus);
> > > > > + goto out_no_complete;
> > > > > + } else {
> > > > > + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > > > + if (msg->flags & I2C_M_RD)
> > > > > + bus->master_state =
> > > > > ASPEED_I2C_MASTER_RX;
> > > > > + else
> > > > > + bus->master_state =
> > > > > ASPEED_I2C_MASTER_TX_FIRST;
> > > >
> > > > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> > > > to
> > > > handle this here ? A quick look at the TX_FIRST case makes me
> > > > think
> > > > we are ok there but I'm not sure about the RX case.
> > >
> > > I did not think that there is an SMBUS_QUICK RX. Could you point
> > > me
> > > to an example?
> >
> > Not so much an RX, it's more like you are sending a 1-bit data in
> > the
> > place of the Rd/Wr bit. So you have a read with a lenght of 0, I
> > don't
> > think in that case you should set ASPEED_I2CD_M_RX_CMD in
> > __aspeed_i2c_do_start
>
> Forget what I said, I was just not thinking about the fact that SMBus
> emulation causes the data bit to be encoded as the R/W flag. I see
> what you are saying; you are correct.
>
> >
> > > > I'm not sure the RX case is tight also. What completion does
> > > > the HW
> > > > give you for the address cycle ? Won't you get that before it
> > > > has
> > > > received the first character ? IE. You fall through to the
> > > > read
> > > > case of the state machine with the read potentially not
> > > > complete
> > > > yet no ?
> > >
> > > ...
> > > > > + case ASPEED_I2C_MASTER_RX:
> > > > > + if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > > > + dev_err(bus->dev, "master failed to
> > > > > RX");
> > > > > + goto out_complete;
> > > > > + }
> > > >
> > > > See my comment above for a bog standard i2c_read. Aren't you
> > > > getting the completion for the address before the read is even
> > > > started ?
> > >
> > > In practice no, but it is probably best to be safe :-)
> >
> > Yup :)
> > > >
> > > > > + status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > > > +
> > > > > + recv_byte = aspeed_i2c_read(bus,
> > > > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > > > + msg->buf[bus->buf_index++] = recv_byte;
> > > > > +
> > > > > + if (msg->flags & I2C_M_RECV_LEN &&
> > > > > + recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > > > + msg->len = recv_byte +
> > > > > + ((msg->flags &
> > > > > I2C_CLIENT_PEC) ? 2 : 1);
> > >
> > > ...
> > > > > + return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > > > + & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > > > + | ((clk_low <<
> > > > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > > > + & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > > > + | (base_clk &
> > > > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > > > +}
> > > >
> > > > As I think I mentioned earlier, the AST2500 has a slightly
> > > > different register layout which support larger values for high
> > > > and
> > > > low, thus allowing a finer granularity.
> > >
> > > I am developing against the 2500.
> >
> > Yes but we'd like the driver to work with both :-)
>
> Right, I thought you were making an assertion about the 2500, if you
> are making an assertion about the 2400, I do not know and do not have
> one handy.
>
> >
> > > > BTW. In case you haven't, I would suggest you copy/paste the
> > > > above
> > > > in a userspace app and run it for all frequency divisors and
> > > > see if
> > > > your results match the aspeed table :)
> > >
> > > Good call.
> >
> > If you end up doing that, can you shoot it my way ? I can take care
> > of
> > making sure it's all good for the 2400.
>
> Will do.
>
> >
> > > > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > > > + struct platform_device *pdev) {
> > > > > + u32 clk_freq, divisor;
> > > > > + struct clk *pclk;
> > > > > + int ret;
> > > > > +
> > > > > + pclk = devm_clk_get(&pdev->dev, NULL);
> > > > > + if (IS_ERR(pclk)) {
> > > > > + dev_err(&pdev->dev, "clk_get failed\n");
> > > > > + return PTR_ERR(pclk);
> > > > > + }
> > > > > + ret = of_property_read_u32(pdev->dev.of_node,
> > > > > + "clock-frequency",
> > > > > &clk_freq);
> > > >
> > > > See my previous comment about calling that 'bus-frequency'
> > > > rather
> > > > than 'clock-frequency'.
> > > >
> > > > > + if (ret < 0) {
> > > > > + dev_err(&pdev->dev,
> > > > > + "Could not read clock-frequency
> > > > > property\n");
> > > > > + clk_freq = 100000;
> > > > > + }
> > > > > + divisor = clk_get_rate(pclk) / clk_freq;
> > > > > + /* We just need the clock rate, we don't actually use
> > > > > the
> > > > > clk object. */
> > > > > + devm_clk_put(&pdev->dev, pclk);
> > > > > +
> > > > > + /* Set AC Timing */
> > > > > + if (clk_freq / 1000 > 1000) {
> > > > > + aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > > > + ASPEED_I2
> > > > > C_FU
> > > > > N_CTRL_REG) |
> > > > > + ASPEED_I2CD_M_HIGH_SPEED_EN |
> > > > > + ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> > > > > + ASPEED_I2CD_SDA_DRIVE_1T_EN,
> > > > > + ASPEED_I2C_FUN_CTRL_REG);
> > > > > +
> > > > > + aspeed_i2c_write(bus, 0x3,
> > > > > ASPEED_I2C_AC_TIMING_REG2);
> > > > > + aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > + ASPEED_I2C_AC_TIMING_REG1);
> > > >
> > > > I already discussed by doubts about the above. I can try to
> > > > scope
> > > > it with the EVB if you don't get to it. For now I'd rather take
> > > > the
> > > > code out.
> > > >
> > > > We should ask aspeed from what frequency the "1T" stuff is
> > > > useful.
> > >
> > > Will do, I will try to rope Ryan in on the next review; it will
> > > be
> > > good for him to get used to working with upstream anyway.
> >
> > Yup. However, for the sake of getting something upstream (and in
> > OpenBMC 4.10 kernel) asap, I would suggest just dropping support
> > for
> > those fast speeds for now, we can add them back later.
>
> Alright, that's fine. Still, Ryan, could you provide some context on
> this?
>
> >
> > > >
> > > > > + } else {
> > > > > + aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > + ASPEED_I2C_AC_TIMING_REG1);
> > > > > + aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > > > + ASPEED_I2C_AC_TIMING_REG2);
> > > > > + }
> > >
> > > ...
> > > > > + spin_lock_init(&bus->lock);
> > > > > + init_completion(&bus->cmd_complete);
> > > > > + bus->adap.owner = THIS_MODULE;
> > > > > + bus->adap.retries = 0;
> > > > > + bus->adap.timeout = 5 * HZ;
> > > > > + bus->adap.algo = &aspeed_i2c_algo;
> > > > > + bus->adap.algo_data = bus;
> > > > > + bus->adap.dev.parent = &pdev->dev;
> > > > > + bus->adap.dev.of_node = pdev->dev.of_node;
> > > > > + snprintf(bus->adap.name, sizeof(bus->adap.name),
> > > > > "Aspeed
> > > > > i2c");
> > > >
> > > > Another trivial one, should we put some kind of bus number in
> > > > that
> > > > string ?
> > >
> > > Whoops, looks like I missed this one; I will get to it in the
> > > next
> > > revision.
> >
> > Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
>
> Yep, I will get it in v8.
>
> >
> > > >
> > > > > + bus->dev = &pdev->dev;
> > > > > +
> > > > > + /* reset device: disable master & slave functions */
> > > > > + aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> > >
> > > ...
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > devicetree"
> > > in
> > > the body of a message to majordomo@vger.kernel.org More
> > > majordomo
> > > info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 09/10] arm64: allwinner: a64: enable AXP803 regulators for Pine64
From: Icenowy Zheng @ 2017-04-25 9:28 UTC (permalink / raw)
To: Andre Przywara, Thomas Gleixner, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, Lee Jones, Liam Girdwood
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <8c7864f1-a0d7-b87f-f9d6-5201cc773609-5wv7dgnIgG8@public.gmane.org>
于 2017年4月25日 GMT+08:00 下午5:24:13, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> 写到:
>Hi,
>
>On 24/04/17 17:01, Icenowy Zheng wrote:
>> Add support of AXP803 regulators in the Pine64 device tree, in order
>to
>> enable many future functionalities, e.g. Wi-Fi.
>
>In general that's quite some code to just achieve some device power
>plane switching, but that's another discussion, I guess ;-)
>
>To me this patch here like a lot of churn to be duplicated in each
>board's DT.
>Can't we provide some sane defaults, either in axp803.dtsi or in an
>extra file (allwinner-ref-axp803.dtsi?), and only overwrite them in a
>board's DT if a board deviates?
For critical regulators you're right, however, peripherals' power seems to vary a lot, e.g. Wi-Fi and HDMI.
The DLDO1 for HDMI and DLDO4 for Wi-Fi design is only adopted by Pine64; according to BPi M64 schematics, Wi-Fi is DLDO2 and Wi-Fi IO voltage is DLDO4 (on Pine64 it's ELDO1 which is forced to be 1.8v because it's connected to CPVDD); on Orange Pi Win {,Plus} Wi-Fi is ALDO1, Wi-Fi IO is DLDO4 and HDMI is DLDO3.
>Because it seems like many boards are actually based on some Allwinner
>reference design and use very similar, if not identical settings.
>
>Also one thing below ...
>
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> ---
>> .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 109
>+++++++++++++++++++++
>> 1 file changed, 109 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>> index 3e1b44292534..abc1879e91f2 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>> @@ -106,6 +106,115 @@
>> };
>> };
>>
>> +#include "axp803.dtsi"
>> +
>> +®_aldo1 {
>> + regulator-min-microvolt = <2800000>;
>> + regulator-max-microvolt = <2800000>;
>> + regulator-name = "vcc-csi";
>> +};
>> +
>> +®_aldo2 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "vcc-pl";
>> +};
>> +
>> +®_aldo3 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <2700000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "vcc-pll-avcc";
>> +};
>> +
>> +®_dc1sw {
>> + regulator-name = "vcc-phy";
>> +};
>> +
>> +®_dcdc1 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "vcc-3v3";
>> +};
>> +
>> +®_dcdc2 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <1000000>;
>> + regulator-max-microvolt = <1300000>;
>> + regulator-name = "vdd-cpux";
>> +};
>> +
>> +/* DCDC3 is polyphased with DCDC2 */
>> +
>> +®_dcdc5 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <1500000>;
>> + regulator-max-microvolt = <1500000>;
>
>On the Pine64 there is DDR3L DRAM, running at 1.35V.
>
>Cheers,
>Andre.
>
>
>> + regulator-name = "vcc-dram";
>> +};
>> +
>> +®_dcdc6 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <1100000>;
>> + regulator-max-microvolt = <1100000>;
>> + regulator-name = "vdd-sys";
>> +};
>> +
>> +®_dldo1 {
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "vcc-hdmi";
>> +};
>> +
>> +®_dldo2 {
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "vcc-mipi";
>> +};
>> +
>> +®_dldo3 {
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "avdd-csi";
>> +};
>> +
>> +®_dldo4 {
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-name = "vcc-wifi";
>> +};
>> +
>> +®_eldo1 {
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-name = "cpvdd";
>> +};
>> +
>> +®_eldo3 {
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-name = "vdd-1v8-csi";
>> +};
>> +
>> +®_fldo1 {
>> + regulator-min-microvolt = <1200000>;
>> + regulator-max-microvolt = <1200000>;
>> + regulator-name = "vcc-1v2-hsic";
>> +};
>> +
>> +®_fldo2 {
>> + regulator-always-on;
>> + regulator-min-microvolt = <1100000>;
>> + regulator-max-microvolt = <1100000>;
>> + regulator-name = "vdd-cpus";
>> +};
>> +
>> +®_rtc_ldo {
>> + regulator-name = "vcc-rtc";
>> +};
>> +
>> &uart0 {
>> pinctrl-names = "default";
>> pinctrl-0 = <&uart0_pins_a>;
>>
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v4 09/10] arm64: allwinner: a64: enable AXP803 regulators for Pine64
From: Andre Przywara @ 2017-04-25 9:24 UTC (permalink / raw)
To: icenowy-h8G6r0blFSE, Thomas Gleixner, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, Lee Jones, Liam Girdwood
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170424160103.9447-10-icenowy-h8G6r0blFSE@public.gmane.org>
Hi,
On 24/04/17 17:01, Icenowy Zheng wrote:
> Add support of AXP803 regulators in the Pine64 device tree, in order to
> enable many future functionalities, e.g. Wi-Fi.
In general that's quite some code to just achieve some device power
plane switching, but that's another discussion, I guess ;-)
To me this patch here like a lot of churn to be duplicated in each
board's DT.
Can't we provide some sane defaults, either in axp803.dtsi or in an
extra file (allwinner-ref-axp803.dtsi?), and only overwrite them in a
board's DT if a board deviates?
Because it seems like many boards are actually based on some Allwinner
reference design and use very similar, if not identical settings.
Also one thing below ...
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 109 +++++++++++++++++++++
> 1 file changed, 109 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> index 3e1b44292534..abc1879e91f2 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> @@ -106,6 +106,115 @@
> };
> };
>
> +#include "axp803.dtsi"
> +
> +®_aldo1 {
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + regulator-name = "vcc-csi";
> +};
> +
> +®_aldo2 {
> + regulator-always-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-pl";
> +};
> +
> +®_aldo3 {
> + regulator-always-on;
> + regulator-min-microvolt = <2700000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-pll-avcc";
> +};
> +
> +®_dc1sw {
> + regulator-name = "vcc-phy";
> +};
> +
> +®_dcdc1 {
> + regulator-always-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-3v3";
> +};
> +
> +®_dcdc2 {
> + regulator-always-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-name = "vdd-cpux";
> +};
> +
> +/* DCDC3 is polyphased with DCDC2 */
> +
> +®_dcdc5 {
> + regulator-always-on;
> + regulator-min-microvolt = <1500000>;
> + regulator-max-microvolt = <1500000>;
On the Pine64 there is DDR3L DRAM, running at 1.35V.
Cheers,
Andre.
> + regulator-name = "vcc-dram";
> +};
> +
> +®_dcdc6 {
> + regulator-always-on;
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-name = "vdd-sys";
> +};
> +
> +®_dldo1 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-hdmi";
> +};
> +
> +®_dldo2 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-mipi";
> +};
> +
> +®_dldo3 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "avdd-csi";
> +};
> +
> +®_dldo4 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-wifi";
> +};
> +
> +®_eldo1 {
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-name = "cpvdd";
> +};
> +
> +®_eldo3 {
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-name = "vdd-1v8-csi";
> +};
> +
> +®_fldo1 {
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-name = "vcc-1v2-hsic";
> +};
> +
> +®_fldo2 {
> + regulator-always-on;
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-name = "vdd-cpus";
> +};
> +
> +®_rtc_ldo {
> + regulator-name = "vcc-rtc";
> +};
> +
> &uart0 {
> pinctrl-names = "default";
> pinctrl-0 = <&uart0_pins_a>;
>
^ permalink raw reply
* Re: [PATCH v3 0/7] V4L2 fwnode support
From: Hans Verkuil @ 2017-04-25 8:51 UTC (permalink / raw)
To: Sakari Ailus, linux-media; +Cc: linux-acpi, devicetree, laurent.pinchart
In-Reply-To: <1491829376-14791-1-git-send-email-sakari.ailus@linux.intel.com>
On 10/04/17 15:02, Sakari Ailus wrote:
> Hello everyone,
>
> This patchset adds support for fwnode to V4L2. Besides OF, also ACPI based
> systems can be supported this way. By using V4L2 fwnode, the individual
> drivers do not need to be aware of the underlying firmware implementation.
> The patchset also removes specific V4L2 OF support and converts the
> affected drivers to use V4L2 fwnode.
Successfully tested with my Atmel sama5d3 board + ov2640 sensor.
Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
>
> The patchset depends on another patchset here:
>
> <URL:http://www.spinics.net/lists/linux-acpi/msg72973.html>
>
> A git tree with the dependencies can be found here:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi-merge>
>
> v1 of the set can be found here:
>
> <URL:http://www.spinics.net/lists/linux-media/msg111073.html>
>
> and v2 here:
>
> <URL:http://www.spinics.net/lists/linux-media/msg114110.html>
>
> changes since v2:
>
> - Use EXPORT_SYMBOL_GPL() instead of EXPORT_SYMBOL().
>
> - Alphabetically order the topics under V4L2 core kAPI documentation.
>
> - Prefer "fwnode" variable name for struct fwnode_handle pointers instead
> of "fwn". Similarly, use "vep" for struct v4l2_fwnode_endpoint instead
> of "vfwn".
>
> - Convert existing users of OF matching to fwnode matching.
>
> - Remove OF matching support as well as compatibility between OF and
> fwnode matching.
>
> - Use of_node_cmp() to determine whether two nodes match in case both of
> them are OF nodes. There is thus no functional difference between
> existing OF matching in v1.
>
> - Continue to use struct device_node.full_name on fwnodes that are known
> to be OF nodes instead of omitting such debug information. Drivers that
> can actually use fwnode need a new interface to provide this in fwnode
> framework. This is out of scope of the patchset.
>
> - Remove linux/of.h header inclusion in
> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>
> - Improved line wrapping primarily in
> drivers/media/v4l2-core/v4l2-fwnode.c.
>
> - Rewrap KernelDoc documentation for V4L2 fwnode API up to 80 characters
> per line (new patch).
>
> - Fix KernelDoc documentation, there were a few locations where the
> argument had been changed but the documentation was not updated
> accordingly.
>
> - Fix punctuation and wording in V4L2 fwnode documentation.
>
> - Drop patch "v4l: media/drv-intf/soc_mediabus.h: include dependent header
> file". It is no longer needed.
>
> - Fix obtaining port parent in v4l2_fwnode_parse_link() on ACPI.
>
> - Include newly OF-supported atmel-isi to V4L2 OF -> fwnode conversion.
>
> - Add that the v4l2-fwnode.c has origins in v4l2-of.c to the commit
> message and the file header.
>
> changes since v1:
>
> - Use existing dev_fwnode() instead of device_fwnode_handle() added by the
> ACPI graph patchset,
>
> - Fix too long line of ^'s in ReST documentation and
>
> - Drop the patch rearranging the header files. It'd better go in
> separately, if at all.
>
> Sakari Ailus (7):
> v4l: fwnode: Support generic fwnode for parsing standardised
> properties
> v4l: async: Add fwnode match support
> v4l: flash led class: Use fwnode_handle instead of device_node in init
> v4l: Switch from V4L2 OF not V4L2 fwnode API
> docs-rst: media: Sort topic list alphabetically
> docs-rst: media: Switch documentation to V4L2 fwnode API
> v4l: Remove V4L2 OF framework in favour of V4L2 fwnode framework
>
> Documentation/media/kapi/v4l2-core.rst | 20 +-
> Documentation/media/kapi/v4l2-fwnode.rst | 3 +
> Documentation/media/kapi/v4l2-of.rst | 3 -
> drivers/leds/leds-aat1290.c | 5 +-
> drivers/leds/leds-max77693.c | 5 +-
> drivers/media/i2c/Kconfig | 9 +
> drivers/media/i2c/adv7604.c | 7 +-
> drivers/media/i2c/mt9v032.c | 7 +-
> drivers/media/i2c/ov2659.c | 8 +-
> drivers/media/i2c/s5c73m3/s5c73m3-core.c | 7 +-
> drivers/media/i2c/s5k5baf.c | 6 +-
> drivers/media/i2c/smiapp/Kconfig | 1 +
> drivers/media/i2c/smiapp/smiapp-core.c | 29 ++-
> drivers/media/i2c/tc358743.c | 11 +-
> drivers/media/i2c/tvp514x.c | 6 +-
> drivers/media/i2c/tvp5150.c | 7 +-
> drivers/media/i2c/tvp7002.c | 6 +-
> drivers/media/platform/Kconfig | 3 +
> drivers/media/platform/am437x/Kconfig | 1 +
> drivers/media/platform/am437x/am437x-vpfe.c | 15 +-
> drivers/media/platform/atmel/Kconfig | 1 +
> drivers/media/platform/atmel/atmel-isc.c | 13 +-
> drivers/media/platform/exynos4-is/Kconfig | 2 +
> drivers/media/platform/exynos4-is/media-dev.c | 13 +-
> drivers/media/platform/exynos4-is/mipi-csis.c | 6 +-
> drivers/media/platform/omap3isp/isp.c | 49 ++--
> drivers/media/platform/pxa_camera.c | 11 +-
> drivers/media/platform/rcar-vin/Kconfig | 1 +
> drivers/media/platform/rcar-vin/rcar-core.c | 23 +-
> drivers/media/platform/soc_camera/Kconfig | 1 +
> drivers/media/platform/soc_camera/atmel-isi.c | 7 +-
> drivers/media/platform/soc_camera/soc_camera.c | 7 +-
> drivers/media/platform/ti-vpe/cal.c | 15 +-
> drivers/media/platform/xilinx/Kconfig | 1 +
> drivers/media/platform/xilinx/xilinx-vipp.c | 63 +++--
> drivers/media/v4l2-core/Kconfig | 3 +
> drivers/media/v4l2-core/Makefile | 4 +-
> drivers/media/v4l2-core/v4l2-async.c | 21 +-
> drivers/media/v4l2-core/v4l2-flash-led-class.c | 12 +-
> drivers/media/v4l2-core/v4l2-fwnode.c | 342 +++++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-of.c | 327 -----------------------
> include/media/v4l2-async.h | 8 +-
> include/media/v4l2-flash-led-class.h | 4 +-
> include/media/v4l2-fwnode.h | 104 ++++++++
> include/media/v4l2-of.h | 128 ---------
> include/media/v4l2-subdev.h | 3 +
> 46 files changed, 690 insertions(+), 638 deletions(-)
> create mode 100644 Documentation/media/kapi/v4l2-fwnode.rst
> delete mode 100644 Documentation/media/kapi/v4l2-of.rst
> create mode 100644 drivers/media/v4l2-core/v4l2-fwnode.c
> delete mode 100644 drivers/media/v4l2-core/v4l2-of.c
> create mode 100644 include/media/v4l2-fwnode.h
> delete mode 100644 include/media/v4l2-of.h
>
^ permalink raw reply
* Re: [PATCH v3 0/4] of: remove *phandle properties from expanded device tree
From: Frank Rowand @ 2017-04-25 8:51 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493110049-30165-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 04/25/17 01:47, frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
>
> Remove "phandle" and "linux,phandle" properties from the internal
> device tree. The phandle will still be in the struct device_node
> phandle field.
>
> This is to resolve the issue found by Stephen Boyd [1] when he changed
> the type of struct property.value from void * to const void *. As
> a result of the type change, the overlay code had compile errors
> where the resolver updates phandle values.
>
> [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
>
> Patch 1 is the phandle related changes.
>
> Patches 2 - 4 are minor fixups for issues that became visible
> while implementing patch 1.
>
> Changes from v1:
^^^^^^^^
Changes from v2
> - patch 1: Remove check in __of_add_phandle_sysfs() that would not
> add a sysfs entry if IS_ENABLED(CONFIG_PPC_PSERIES)
>
> Changes from v1:
> - Remove phandle properties in of_attach_node(), before attaching
> the node to the live tree.
> - Add the phandle sysfs entry for the node in of_attach_node().
> - When creating an overlay changeset, duplicate the node phandle in
> __of_node_dup().
>
> Frank Rowand (4):
> of: remove *phandle properties from expanded device tree
> of: make __of_attach_node() static
> of: be consistent in form of file mode
> of: detect invalid phandle in overlay
>
> drivers/of/base.c | 50 +++++++++++++++++++++++++++++++++++++++----
> drivers/of/dynamic.c | 56 ++++++++++++++++++++++++++++++++++++-------------
> drivers/of/fdt.c | 40 +++++++++++++++++++++--------------
> drivers/of/of_private.h | 2 +-
> drivers/of/overlay.c | 8 ++++---
> drivers/of/resolver.c | 23 +-------------------
> include/linux/of.h | 1 +
> 7 files changed, 120 insertions(+), 60 deletions(-)
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
From: Ryan Chen @ 2017-04-25 8:50 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Joel Stanley, Vladimir Zapolskiy,
Kachalov Anton, Cédric Le Goater, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, Linux Kernel Mailing List,
OpenBMC Maillist
In-Reply-To: <CAFd5g44ioW0PdFVOhY-ep8TXBNg6qC7E5yjOVf3MEdY9bPgEDA@mail.gmail.com>
Hello All,
ASPEED_I2CD_M_SDA_DRIVE_1T_EN, ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.
For example, if i2c bus is use on "high speed" and "single slave and master" and i2c bus is too long. It need drive SDA or SCL less lunacy. It would enable it.
Otherwise, don’t enable it. especially in multi-master. It can’t be enable.
Best Regards,
Ryan
信驊科技股份有限公司
ASPEED Technology Inc.
2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 30077, Taiwan
Tel: 886-3-578-9568 #857
Fax: 886-3-578-9586
************* Email Confidentiality Notice ********************
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
-----Original Message-----
From: Brendan Higgins [mailto:brendanhiggins@google.com]
Sent: Tuesday, April 25, 2017 4:32 PM
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
Adding Ryan.
On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
>> > > +struct aspeed_i2c_bus {
>> > > + struct i2c_adapter adap;
>> > > + struct device *dev;
>> > > + void __iomem *base;
>> > > + /* Synchronizes I/O mem access to base. */
>> > > + spinlock_t lock;
>> >
>> > I am not entirely convinced we need that lock. The i2c core will
>> > take a mutex protecting all operations on the bus. So we only need
>> > to synchronize between our "xfer" code and our interrupt handler.
>>
>> You are right if both having slave and master active at the same time
>> was not possible; however, it is.
>
> Right, I somewhat forgot about the slave case.
>
> ...
>
>> > Some of those error states probably also warrant a reset of the
>> > controller, I think aspeed does that in the SDK.
>>
>> For timeout and cmd_err, I do not see any argument against it; it
>> sounds like we are in a very messed up, very unknown state, so full
>> reset is probably the best last resort.
>
> Yup.
>
>> For SDA staying pulled down, I
>> think we can say with reasonable confidence that some device on our
>> bus is behaving very badly and I am not convinced that resetting the
>> controller is likely to do anything to help;
>
> Right. Hammering with STOPs and pray ...
I think sending recovery mode sends stops as a part of the recovery algorithm it executes.
>
>> that being said, I really
>> do not have any good ideas to address that. So maybe praying and
>> resetting the controller is *the most reasonable thing to do.* I
>> would like to know what you think we should do in that case.
>
> Well, there's a (small ?) chance that it's a controller bug asserting
> the line so ... but there's little we can do if not.
True.
>
>> While I was thinking about this I also realized that the SDA line
>> check after recovery happens in the else branch, but SCL line check
>> does not happen after we attempt to STOP if SCL is hung. If we decide
>> to make special note SDA being hung by a device that won't let go, we
>> might want to make a special note that SCL is hung by a device that
>> won't let go. Just a thought.
>
> Maybe. Or just "unrecoverable error"... hopefully these don't happen
> too often ... We had cases of a TPM misbehaving like that.
Yeah, definitely should print something out.
>
>> > > +out:
>>
>> ...
>> > What about I2C_M_NOSTART ?
>> >
>> > Not that I've ever seen it used... ;-)
>>
>> Right now I am not doing any of the protocol mangling options, but I
>> can add them in if you think it is important for initial support.
>
> No, not important, we can add that later if it ever becomes useful.
>
> ...
>
>> > In general, you always ACK all interrupts first. Then you handle
>> > the bits you have harvested.
>> >
>>
>> The documentation says to ACK the interrupt after handling in the RX
>> case:
>>
>> <<<
>> S/W needs to clear this status bit to allow next data receiving.
>> > > >
>>
>> I will double check with Ryan to make sure TX works the same way.
>>
>> > > + if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> > > + (!bus->msgs && bus->master_state !=
>> > > ASPEED_I2C_MASTER_STOP)) {
>>
>> ...
>> >
>> > I would set master_state to "RECOVERY" (new state ?) and ensure
>> > those things are caught if they happen outside of a recovery.
>
> I replied privately ... as long as we ack before we start a new
> command we should be ok but we shouldn't ack after.
>
> Your latest patch still does that. It will do things like start a STOP
> command *then* ack the status bits. I'm pretty sure that's bogus.
>
> That way it's a lot simpler to simply move the
>
> writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> To either right after the readl of the status reg at the beginning of
> aspeed_i2c_master_irq().
>
> I would be very surprised if that didn't work properly and wasn't much
> safer than what you are currently doing.
I think I tried your way and it worked. In anycase, Ryan will be able to clarify for us.
>
>> Let me know if you still think we need a "RECOVERY" state.
>
> The way you just switch to stop state and store the error for later
> should work I think.
>
>> >
>> > > + if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>
>> ...
>> >
>> > > + dev_dbg(bus->dev,
>> > > + "no slave present at %02x", msg-
>> > > >addr);
>> > > + status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> > > + bus->cmd_err = -EIO;
>> > > + do_stop(bus);
>> > > + goto out_no_complete;
>> > > + } else {
>> > > + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> > > + if (msg->flags & I2C_M_RD)
>> > > + bus->master_state =
>> > > ASPEED_I2C_MASTER_RX;
>> > > + else
>> > > + bus->master_state =
>> > > ASPEED_I2C_MASTER_TX_FIRST;
>> >
>> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need to
>> > handle this here ? A quick look at the TX_FIRST case makes me think
>> > we are ok there but I'm not sure about the RX case.
>>
>> I did not think that there is an SMBUS_QUICK RX. Could you point me
>> to an example?
>
> Not so much an RX, it's more like you are sending a 1-bit data in the
> place of the Rd/Wr bit. So you have a read with a lenght of 0, I don't
> think in that case you should set ASPEED_I2CD_M_RX_CMD in
> __aspeed_i2c_do_start
Forget what I said, I was just not thinking about the fact that SMBus emulation causes the data bit to be encoded as the R/W flag. I see what you are saying; you are correct.
>
>> > I'm not sure the RX case is tight also. What completion does the HW
>> > give you for the address cycle ? Won't you get that before it has
>> > received the first character ? IE. You fall through to the read
>> > case of the state machine with the read potentially not complete
>> > yet no ?
>>
>> ...
>> > > + case ASPEED_I2C_MASTER_RX:
>> > > + if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> > > + dev_err(bus->dev, "master failed to RX");
>> > > + goto out_complete;
>> > > + }
>> >
>> > See my comment above for a bog standard i2c_read. Aren't you
>> > getting the completion for the address before the read is even
>> > started ?
>>
>> In practice no, but it is probably best to be safe :-)
>
> Yup :)
>> >
>> > > + status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> > > +
>> > > + recv_byte = aspeed_i2c_read(bus,
>> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> > > + msg->buf[bus->buf_index++] = recv_byte;
>> > > +
>> > > + if (msg->flags & I2C_M_RECV_LEN &&
>> > > + recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> > > + msg->len = recv_byte +
>> > > + ((msg->flags &
>> > > I2C_CLIENT_PEC) ? 2 : 1);
>>
>> ...
>> > > + return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> > > + & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> > > + | ((clk_low <<
>> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> > > + & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> > > + | (base_clk &
>> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> > > +}
>> >
>> > As I think I mentioned earlier, the AST2500 has a slightly
>> > different register layout which support larger values for high and
>> > low, thus allowing a finer granularity.
>>
>> I am developing against the 2500.
>
> Yes but we'd like the driver to work with both :-)
Right, I thought you were making an assertion about the 2500, if you are making an assertion about the 2400, I do not know and do not have one handy.
>
>> > BTW. In case you haven't, I would suggest you copy/paste the above
>> > in a userspace app and run it for all frequency divisors and see if
>> > your results match the aspeed table :)
>>
>> Good call.
>
> If you end up doing that, can you shoot it my way ? I can take care of
> making sure it's all good for the 2400.
Will do.
>
>> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> > > + struct platform_device *pdev) {
>> > > + u32 clk_freq, divisor;
>> > > + struct clk *pclk;
>> > > + int ret;
>> > > +
>> > > + pclk = devm_clk_get(&pdev->dev, NULL);
>> > > + if (IS_ERR(pclk)) {
>> > > + dev_err(&pdev->dev, "clk_get failed\n");
>> > > + return PTR_ERR(pclk);
>> > > + }
>> > > + ret = of_property_read_u32(pdev->dev.of_node,
>> > > + "clock-frequency", &clk_freq);
>> >
>> > See my previous comment about calling that 'bus-frequency' rather
>> > than 'clock-frequency'.
>> >
>> > > + if (ret < 0) {
>> > > + dev_err(&pdev->dev,
>> > > + "Could not read clock-frequency
>> > > property\n");
>> > > + clk_freq = 100000;
>> > > + }
>> > > + divisor = clk_get_rate(pclk) / clk_freq;
>> > > + /* We just need the clock rate, we don't actually use the
>> > > clk object. */
>> > > + devm_clk_put(&pdev->dev, pclk);
>> > > +
>> > > + /* Set AC Timing */
>> > > + if (clk_freq / 1000 > 1000) {
>> > > + aspeed_i2c_write(bus, aspeed_i2c_read(bus,
>> > > + ASPEED_I2C_FU
>> > > N_CTRL_REG) |
>> > > + ASPEED_I2CD_M_HIGH_SPEED_EN |
>> > > + ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
>> > > + ASPEED_I2CD_SDA_DRIVE_1T_EN,
>> > > + ASPEED_I2C_FUN_CTRL_REG);
>> > > +
>> > > + aspeed_i2c_write(bus, 0x3,
>> > > ASPEED_I2C_AC_TIMING_REG2);
>> > > + aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > + ASPEED_I2C_AC_TIMING_REG1);
>> >
>> > I already discussed by doubts about the above. I can try to scope
>> > it with the EVB if you don't get to it. For now I'd rather take the
>> > code out.
>> >
>> > We should ask aspeed from what frequency the "1T" stuff is useful.
>>
>> Will do, I will try to rope Ryan in on the next review; it will be
>> good for him to get used to working with upstream anyway.
>
> Yup. However, for the sake of getting something upstream (and in
> OpenBMC 4.10 kernel) asap, I would suggest just dropping support for
> those fast speeds for now, we can add them back later.
Alright, that's fine. Still, Ryan, could you provide some context on this?
>
>> >
>> > > + } else {
>> > > + aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > + ASPEED_I2C_AC_TIMING_REG1);
>> > > + aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> > > + ASPEED_I2C_AC_TIMING_REG2);
>> > > + }
>>
>> ...
>> > > + spin_lock_init(&bus->lock);
>> > > + init_completion(&bus->cmd_complete);
>> > > + bus->adap.owner = THIS_MODULE;
>> > > + bus->adap.retries = 0;
>> > > + bus->adap.timeout = 5 * HZ;
>> > > + bus->adap.algo = &aspeed_i2c_algo;
>> > > + bus->adap.algo_data = bus;
>> > > + bus->adap.dev.parent = &pdev->dev;
>> > > + bus->adap.dev.of_node = pdev->dev.of_node;
>> > > + snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed
>> > > i2c");
>> >
>> > Another trivial one, should we put some kind of bus number in that
>> > string ?
>>
>> Whoops, looks like I missed this one; I will get to it in the next
>> revision.
>
> Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
Yep, I will get it in v8.
>
>> >
>> > > + bus->dev = &pdev->dev;
>> > > +
>> > > + /* reset device: disable master & slave functions */
>> > > + aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
>>
>> ...
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree"
>> in
>> the body of a message to majordomo@vger.kernel.org More majordomo
>> info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* 33027 devicetree
From: arywhite007-/E1597aS9LQAvxtiuMwx3w @ 2017-04-25 8:50 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: 09469566543158.zip --]
[-- Type: application/zip, Size: 2600 bytes --]
^ permalink raw reply
* [PATCH v3 4/4] of: detect invalid phandle in overlay
From: frowand.list @ 2017-04-25 8:47 UTC (permalink / raw)
To: Rob Herring, stephen.boyd; +Cc: devicetree, linux-kernel
In-Reply-To: <1493110049-30165-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
Overlays are not allowed to modify phandle values of previously existing
nodes because there is no information available to allow fixup up
properties that use the previously existing phandle.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index ca0b85f5deb1..20ab49d2f7a4 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -130,6 +130,10 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
/* NOTE: Multiple mods of created nodes not supported */
tchild = of_get_child_by_name(target, cname);
if (tchild != NULL) {
+ /* new overlay phandle value conflicts with existing value */
+ if (child->phandle)
+ return -EINVAL;
+
/* apply overlay recursively */
ret = of_overlay_apply_one(ov, tchild, child);
of_node_put(tchild);
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v3 3/4] of: be consistent in form of file mode
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-04-25 8:47 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493110049-30165-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
checkpatch whined about using S_IRUGO instead of octal equivalent
when adding phandle sysfs code, so used octal in that patch.
Change other instances of the S_* constants in the same file to
the octal form.
Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
drivers/of/base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 41473b1e2445..9fe42346925b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -168,7 +168,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
sysfs_bin_attr_init(&pp->attr);
pp->attr.attr.name = safe_name(&np->kobj, pp->name);
- pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
+ pp->attr.attr.mode = secure ? 0400 : 0444;
pp->attr.size = secure ? 0 : pp->length;
pp->attr.read = of_node_property_read;
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v3 2/4] of: make __of_attach_node() static
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-04-25 8:47 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493110049-30165-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
__of_attach_node() is not used outside of drivers/of/dynamic.c. Make
it static and remove it from drivers/of/of_private.h.
Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
drivers/of/dynamic.c | 2 +-
drivers/of/of_private.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 59545b8fbf46..0b9cf6d5914c 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -216,7 +216,7 @@ int of_property_notify(int action, struct device_node *np,
return of_reconfig_notify(action, &pr);
}
-void __of_attach_node(struct device_node *np)
+static void __of_attach_node(struct device_node *np)
{
np->child = NULL;
np->sibling = np->parent->child;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 33f11a5384f3..b1ff70e1fdda 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -79,7 +79,6 @@ extern int __of_update_property(struct device_node *np,
extern void __of_update_property_sysfs(struct device_node *np,
struct property *newprop, struct property *oldprop);
-extern void __of_attach_node(struct device_node *np);
extern int __of_attach_node_sysfs(struct device_node *np);
extern void __of_detach_node(struct device_node *np);
extern void __of_detach_node_sysfs(struct device_node *np);
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v3 1/4] of: remove *phandle properties from expanded device tree
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-04-25 8:47 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493110049-30165-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
Remove "phandle", "linux,phandle", and "ibm,phandle" properties from
the internal device tree. The phandle will still be in the struct
device_node phandle field.
This is to resolve the issue found by Stephen Boyd [1] when he changed
the type of struct property.value from void * to const void *. As
a result of the type change, the overlay code had compile errors
where the resolver updates phandle values.
[1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
- Add sysfs infrastructure to report np->phandle, as if it was a property.
- Do not create "phandle" "ibm,phandle", and "linux,phandle" properties
in the expanded device tree.
- Remove phandle properties in of_attach_node(), for nodes dynamically
attached to the live tree. Add the phandle sysfs entry for these nodes.
- When creating an overlay changeset, duplicate the node phandle in
__of_node_dup().
- Remove no longer needed checks to exclude "phandle" and "linux,phandle"
properties in several locations.
- A side effect of these changes is that the obsolete "linux,phandle" and
"ibm,phandle" properties will no longer appear in /proc/device-tree (they
will appear as "phandle").
Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
drivers/of/base.c | 48 ++++++++++++++++++++++++++++++++++++++++---
drivers/of/dynamic.c | 54 +++++++++++++++++++++++++++++++++++++------------
drivers/of/fdt.c | 40 +++++++++++++++++++++---------------
drivers/of/of_private.h | 1 +
drivers/of/overlay.c | 4 +---
drivers/of/resolver.c | 23 +--------------------
include/linux/of.h | 1 +
7 files changed, 114 insertions(+), 57 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index d7c4629a3a2d..41473b1e2445 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -116,6 +116,19 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
}
+static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t offset, size_t count)
+{
+ phandle phandle;
+ struct device_node *np;
+
+ np = container_of(bin_attr, struct device_node, attr_phandle);
+ phandle = cpu_to_be32(np->phandle);
+ return memory_read_from_buffer(buf, count, &offset, &phandle,
+ sizeof(phandle));
+}
+
/* always return newly allocated name, caller must free after use */
static const char *safe_name(struct kobject *kobj, const char *orig_name)
{
@@ -164,6 +177,35 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
return rc;
}
+/*
+ * In the imported device tree (fdt), phandle is a property. In the
+ * internal data structure it is instead stored in the struct device_node.
+ * Make phandle visible in sysfs as if it was a property.
+ */
+int __of_add_phandle_sysfs(struct device_node *np)
+{
+ int rc;
+
+ if (!IS_ENABLED(CONFIG_SYSFS))
+ return 0;
+
+ if (!of_kset || !of_node_is_attached(np))
+ return 0;
+
+ if (!np->phandle || np->phandle == 0xffffffff)
+ return 0;
+
+ sysfs_bin_attr_init(&pp->attr);
+ np->attr_phandle.attr.name = "phandle";
+ np->attr_phandle.attr.mode = 0444;
+ np->attr_phandle.size = sizeof(np->phandle);
+ np->attr_phandle.read = of_node_phandle_read;
+
+ rc = sysfs_create_bin_file(&np->kobj, &np->attr_phandle);
+ WARN(rc, "error adding attribute phandle to node %s\n", np->full_name);
+ return rc;
+}
+
int __of_attach_node_sysfs(struct device_node *np)
{
const char *name;
@@ -193,6 +235,8 @@ int __of_attach_node_sysfs(struct device_node *np)
if (rc)
return rc;
+ __of_add_phandle_sysfs(np);
+
for_each_property_of_node(np, pp)
__of_add_property_sysfs(np, pp);
@@ -2097,9 +2141,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
int id, len;
/* Skip those we do not want to proceed */
- if (!strcmp(pp->name, "name") ||
- !strcmp(pp->name, "phandle") ||
- !strcmp(pp->name, "linux,phandle"))
+ if (!strcmp(pp->name, "name"))
continue;
np = of_find_node_by_path(pp->value);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 888fdbc09992..59545b8fbf46 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -218,19 +218,6 @@ int of_property_notify(int action, struct device_node *np,
void __of_attach_node(struct device_node *np)
{
- const __be32 *phandle;
- int sz;
-
- np->name = __of_get_property(np, "name", NULL) ? : "<NULL>";
- np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>";
-
- phandle = __of_get_property(np, "phandle", &sz);
- if (!phandle)
- phandle = __of_get_property(np, "linux,phandle", &sz);
- if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle)
- phandle = __of_get_property(np, "ibm,phandle", &sz);
- np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0;
-
np->child = NULL;
np->sibling = np->parent->child;
np->parent->child = np;
@@ -243,11 +230,47 @@ void __of_attach_node(struct device_node *np)
int of_attach_node(struct device_node *np)
{
struct of_reconfig_data rd;
+ struct property *prev;
+ struct property *prop;
+ struct property *save_next;
unsigned long flags;
+ const __be32 *phandle;
+ int sz;
memset(&rd, 0, sizeof(rd));
rd.dn = np;
+ np->name = __of_get_property(np, "name", NULL) ? : "<NULL>";
+ np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>";
+
+ phandle = __of_get_property(np, "phandle", &sz);
+ if (!phandle)
+ phandle = __of_get_property(np, "linux,phandle", &sz);
+ if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle)
+ phandle = __of_get_property(np, "ibm,phandle", &sz);
+ np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0;
+
+ /* remove phandle properties from node */
+ prev = NULL;
+ for (prop = np->properties; prop != NULL; ) {
+ save_next = prop->next;
+ if (!strcmp(prop->name, "phandle") ||
+ !strcmp(prop->name, "ibm,phandle") ||
+ !strcmp(prop->name, "linux,phandle")) {
+ if (prev)
+ prev->next = prop->next;
+ else
+ np->properties = prop->next;
+ prop->next = np->deadprops;
+ np->deadprops = prop;
+ } else {
+ prev = prop;
+ }
+ prop = save_next;
+ }
+
+ __of_add_phandle_sysfs(np);
+
mutex_lock(&of_mutex);
raw_spin_lock_irqsave(&devtree_lock, flags);
__of_attach_node(np);
@@ -429,6 +452,7 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
/* Iterate over and duplicate all properties */
if (np) {
struct property *pp, *new_pp;
+ node->phandle = np->phandle;
for_each_property_of_node(np, pp) {
new_pp = __of_prop_dup(pp, GFP_KERNEL);
if (!new_pp)
@@ -441,6 +465,10 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
}
}
}
+
+ node->name = __of_get_property(node, "name", NULL) ? : "<NULL>";
+ node->type = __of_get_property(node, "device_type", NULL) ? : "<NULL>";
+
return node;
err_prop:
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..270f31b0c259 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -181,6 +181,8 @@ static void populate_properties(const void *blob,
const __be32 *val;
const char *pname;
u32 sz;
+ int prop_is_phandle = 0;
+ int prop_is_ibm_phandle = 0;
val = fdt_getprop_by_offset(blob, cur, &pname, &sz);
if (!val) {
@@ -196,11 +198,6 @@ static void populate_properties(const void *blob,
if (!strcmp(pname, "name"))
has_name = true;
- pp = unflatten_dt_alloc(mem, sizeof(struct property),
- __alignof__(struct property));
- if (dryrun)
- continue;
-
/* We accept flattened tree phandles either in
* ePAPR-style "phandle" properties, or the
* legacy "linux,phandle" properties. If both
@@ -208,23 +205,34 @@ static void populate_properties(const void *blob,
* will get weird. Don't do that.
*/
if (!strcmp(pname, "phandle") ||
- !strcmp(pname, "linux,phandle")) {
- if (!np->phandle)
- np->phandle = be32_to_cpup(val);
- }
+ !strcmp(pname, "linux,phandle"))
+ prop_is_phandle = 1;
/* And we process the "ibm,phandle" property
* used in pSeries dynamic device tree
* stuff
*/
- if (!strcmp(pname, "ibm,phandle"))
- np->phandle = be32_to_cpup(val);
+ if (!strcmp(pname, "ibm,phandle")) {
+ prop_is_phandle = 1;
+ prop_is_ibm_phandle = 1;
+ }
+
+ if (!prop_is_phandle)
+ pp = unflatten_dt_alloc(mem, sizeof(struct property),
+ __alignof__(struct property));
- pp->name = (char *)pname;
- pp->length = sz;
- pp->value = (__be32 *)val;
- *pprev = pp;
- pprev = &pp->next;
+ if (dryrun)
+ continue;
+
+ if (!prop_is_phandle) {
+ pp->name = (char *)pname;
+ pp->length = sz;
+ pp->value = (__be32 *)val;
+ *pprev = pp;
+ pprev = &pp->next;
+ } else if (prop_is_ibm_phandle || !np->phandle) {
+ np->phandle = be32_to_cpup(val);
+ }
}
/* With version 0x10 we may not have the name property,
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 18bbb4517e25..33f11a5384f3 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -47,6 +47,7 @@ extern int of_property_notify(int action, struct device_node *np,
extern void of_node_release(struct kobject *kobj);
extern int __of_changeset_apply(struct of_changeset *ocs);
extern int __of_changeset_revert(struct of_changeset *ocs);
+extern int __of_add_phandle_sysfs(struct device_node *np);
#else /* CONFIG_OF_DYNAMIC */
static inline int of_property_notify(int action, struct device_node *np,
struct property *prop, struct property *old_prop)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7827786718d8..ca0b85f5deb1 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -101,9 +101,7 @@ static int of_overlay_apply_single_property(struct of_overlay *ov,
tprop = of_find_property(target, prop->name, NULL);
/* special properties are not meant to be updated (silent NOP) */
- if (of_prop_cmp(prop->name, "name") == 0 ||
- of_prop_cmp(prop->name, "phandle") == 0 ||
- of_prop_cmp(prop->name, "linux,phandle") == 0)
+ if (!of_prop_cmp(prop->name, "name"))
return 0;
propn = __of_prop_dup(prop, GFP_KERNEL);
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 7ae9863cb0a4..624cbaeae0a4 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -71,30 +71,11 @@ static void adjust_overlay_phandles(struct device_node *overlay,
int phandle_delta)
{
struct device_node *child;
- struct property *prop;
- phandle phandle;
/* adjust node's phandle in node */
if (overlay->phandle != 0 && overlay->phandle != OF_PHANDLE_ILLEGAL)
overlay->phandle += phandle_delta;
- /* copy adjusted phandle into *phandle properties */
- for_each_property_of_node(overlay, prop) {
-
- if (of_prop_cmp(prop->name, "phandle") &&
- of_prop_cmp(prop->name, "linux,phandle"))
- continue;
-
- if (prop->length < 4)
- continue;
-
- phandle = be32_to_cpup(prop->value);
- if (phandle == OF_PHANDLE_ILLEGAL)
- continue;
-
- *(uint32_t *)prop->value = cpu_to_be32(overlay->phandle);
- }
-
for_each_child_of_node(overlay, child)
adjust_overlay_phandles(child, phandle_delta);
}
@@ -197,9 +178,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
for_each_property_of_node(local_fixups, prop_fix) {
/* skip properties added automatically */
- if (!of_prop_cmp(prop_fix->name, "name") ||
- !of_prop_cmp(prop_fix->name, "phandle") ||
- !of_prop_cmp(prop_fix->name, "linux,phandle"))
+ if (!of_prop_cmp(prop_fix->name, "name"))
continue;
if ((prop_fix->length % 4) != 0 || prop_fix->length == 0)
diff --git a/include/linux/of.h b/include/linux/of.h
index 21e6323de0f3..4e86e05853f3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -61,6 +61,7 @@ struct device_node {
struct kobject kobj;
unsigned long _flags;
void *data;
+ struct bin_attribute attr_phandle;
#if defined(CONFIG_SPARC)
const char *path_component_name;
unsigned int unique_id;
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v3 0/4] of: remove *phandle properties from expanded device tree
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-04-25 8:47 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
Remove "phandle" and "linux,phandle" properties from the internal
device tree. The phandle will still be in the struct device_node
phandle field.
This is to resolve the issue found by Stephen Boyd [1] when he changed
the type of struct property.value from void * to const void *. As
a result of the type change, the overlay code had compile errors
where the resolver updates phandle values.
[1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
Patch 1 is the phandle related changes.
Patches 2 - 4 are minor fixups for issues that became visible
while implementing patch 1.
Changes from v1:
- patch 1: Remove check in __of_add_phandle_sysfs() that would not
add a sysfs entry if IS_ENABLED(CONFIG_PPC_PSERIES)
Changes from v1:
- Remove phandle properties in of_attach_node(), before attaching
the node to the live tree.
- Add the phandle sysfs entry for the node in of_attach_node().
- When creating an overlay changeset, duplicate the node phandle in
__of_node_dup().
Frank Rowand (4):
of: remove *phandle properties from expanded device tree
of: make __of_attach_node() static
of: be consistent in form of file mode
of: detect invalid phandle in overlay
drivers/of/base.c | 50 +++++++++++++++++++++++++++++++++++++++----
drivers/of/dynamic.c | 56 ++++++++++++++++++++++++++++++++++++-------------
drivers/of/fdt.c | 40 +++++++++++++++++++++--------------
drivers/of/of_private.h | 2 +-
drivers/of/overlay.c | 8 ++++---
drivers/of/resolver.c | 23 +-------------------
include/linux/of.h | 1 +
7 files changed, 120 insertions(+), 60 deletions(-)
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Re: [PATCH v4 07/10] mfd: axp20x: add axp20x-regulator cell for AXP803
From: Icenowy Zheng @ 2017-04-25 8:37 UTC (permalink / raw)
To: lee.jones-QSEj5FYQhm4dnm+yROfE0A
Cc: Thomas Gleixner, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
Liam Girdwood, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170425075717.l4x3qektdgt6kaiq@dell>
于 2017年4月25日 GMT+08:00 下午3:57:17, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 写到:
>On Tue, 25 Apr 2017, Icenowy Zheng wrote:
>
>> As axp20x-regulator now supports AXP803, add a cell for it.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>> Changes in v4:
>> - Added a trailing comma for new cell, for easier further cell
>addition.
>> Changes in v3:
>> - Make the new cell one-liner.
>>
>> drivers/mfd/axp20x.c | 3 ++-
>> drivers/regulator/axp20x-regulator.c | 6 +++---
>
>These 2 changes are orthogonal, thus there is no reason to send them
>bundled into a single patch. Doing so complicates things greatly.
>Please resubmit the two changes separately, so that they may be
>absorbed by our respective subsystems.
Yes. As Chen-Yu said, I wrongly squashed the regulator renaming patch here...
It should be squashed into the regulator patch.
Sorry for it.
>
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 1dc6235778eb..917b6ddc4f15 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -848,7 +848,8 @@ static struct mfd_cell axp803_cells[] = {
>> .name = "axp20x-pek",
>> .num_resources = ARRAY_SIZE(axp803_pek_resources),
>> .resources = axp803_pek_resources,
>> - }
>> + },
>> + { .name = "axp20x-regulator" },
>> };
>>
>> static struct mfd_cell axp806_cells[] = {
>> diff --git a/drivers/regulator/axp20x-regulator.c
>b/drivers/regulator/axp20x-regulator.c
>> index 9356ec8a9a1f..e2608fe770b9 100644
>> --- a/drivers/regulator/axp20x-regulator.c
>> +++ b/drivers/regulator/axp20x-regulator.c
>> @@ -311,13 +311,13 @@ static const struct regulator_desc
>axp803_regulators[] = {
>> AXP803_FLDO1_V_OUT, 0x0f, AXP22X_PWR_OUT_CTRL3, BIT(2)),
>> AXP_DESC(AXP803, FLDO2, "fldo2", "fldoin", 700, 1450, 50,
>> AXP803_FLDO2_V_OUT, 0x0f, AXP22X_PWR_OUT_CTRL3, BIT(3)),
>> - AXP_DESC_IO(AXP803, LDO_IO0, "ldo_io0", "ips", 700, 3300, 100,
>> + AXP_DESC_IO(AXP803, LDO_IO0, "ldo-io0", "ips", 700, 3300, 100,
>> AXP22X_LDO_IO0_V_OUT, 0x1f, AXP20X_GPIO0_CTRL, 0x07,
>> AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
>> - AXP_DESC_IO(AXP803, LDO_IO1, "ldo_io1", "ips", 700, 3300, 100,
>> + AXP_DESC_IO(AXP803, LDO_IO1, "ldo-io1", "ips", 700, 3300, 100,
>> AXP22X_LDO_IO1_V_OUT, 0x1f, AXP20X_GPIO1_CTRL, 0x07,
>> AXP22X_IO_ENABLED, AXP22X_IO_DISABLED),
>> - AXP_DESC_FIXED(AXP803, RTC_LDO, "rtc_ldo", "ips", 3000),
>> + AXP_DESC_FIXED(AXP803, RTC_LDO, "rtc-ldo", "ips", 3000),
>> };
>>
>> static const struct regulator_linear_range axp806_dcdca_ranges[] = {
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v7 5/5] i2c: aspeed: added slave support for Aspeed I2C driver
From: Brendan Higgins @ 2017-04-25 8:35 UTC (permalink / raw)
To: Wolfram Sang, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Joel Stanley, Vladimir Zapolskiy,
Kachalov Anton, Cédric Le Goater, Benjamin Herrenschmidt
Cc: linux-i2c, devicetree, Linux Kernel Mailing List,
OpenBMC Maillist, Brendan Higgins, Ryan Chen
In-Reply-To: <20170424181818.2754-6-brendanhiggins@google.com>
Adding Ryan (sorry to everyone else for the spam).
^ permalink raw reply
* Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
From: Brendan Higgins @ 2017-04-25 8:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Joel Stanley, Vladimir Zapolskiy,
Kachalov Anton, Cédric Le Goater,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List,
OpenBMC Maillist, Ryan Chen
In-Reply-To: <1493086747.25766.264.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Adding Ryan.
On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
>> > > +struct aspeed_i2c_bus {
>> > > + struct i2c_adapter adap;
>> > > + struct device *dev;
>> > > + void __iomem *base;
>> > > + /* Synchronizes I/O mem access to base. */
>> > > + spinlock_t lock;
>> >
>> > I am not entirely convinced we need that lock. The i2c core will
>> > take a mutex protecting all operations on the bus. So we only need
>> > to synchronize between our "xfer" code and our interrupt handler.
>>
>> You are right if both having slave and master active at the same time
>> was not possible; however, it is.
>
> Right, I somewhat forgot about the slave case.
>
> ...
>
>> > Some of those error states probably also warrant a reset of the
>> > controller,
>> > I think aspeed does that in the SDK.
>>
>> For timeout and cmd_err, I do not see any argument against it; it
>> sounds like we are in a very messed up, very unknown state, so full
>> reset is probably the best last resort.
>
> Yup.
>
>> For SDA staying pulled down, I
>> think we can say with reasonable confidence that some device on our
>> bus is behaving very badly and I am not convinced that resetting the
>> controller is likely to do anything to help;
>
> Right. Hammering with STOPs and pray ...
I think sending recovery mode sends stops as a part of the recovery
algorithm it executes.
>
>> that being said, I really
>> do not have any good ideas to address that. So maybe praying and
>> resetting the controller is *the most reasonable thing to do.* I
>> would like to know what you think we should do in that case.
>
> Well, there's a (small ?) chance that it's a controller bug asserting
> the line so ... but there's little we can do if not.
True.
>
>> While I was thinking about this I also realized that the SDA line
>> check after recovery happens in the else branch, but SCL line check
>> does not happen after we attempt to STOP if SCL is hung. If we decide
>> to make special note SDA being hung by a device that won't let go, we
>> might want to make a special note that SCL is hung by a device that
>> won't let go. Just a thought.
>
> Maybe. Or just "unrecoverable error"... hopefully these don't happen
> too often ... We had cases of a TPM misbehaving like that.
Yeah, definitely should print something out.
>
>> > > +out:
>>
>> ...
>> > What about I2C_M_NOSTART ?
>> >
>> > Not that I've ever seen it used... ;-)
>>
>> Right now I am not doing any of the protocol mangling options, but I
>> can add them in if you think it is important for initial support.
>
> No, not important, we can add that later if it ever becomes useful.
>
> ...
>
>> > In general, you always ACK all interrupts first. Then you handle
>> > the bits you have harvested.
>> >
>>
>> The documentation says to ACK the interrupt after handling in the RX
>> case:
>>
>> <<<
>> S/W needs to clear this status bit to allow next data receiving.
>> > > >
>>
>> I will double check with Ryan to make sure TX works the same way.
>>
>> > > + if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> > > + (!bus->msgs && bus->master_state !=
>> > > ASPEED_I2C_MASTER_STOP)) {
>>
>> ...
>> >
>> > I would set master_state to "RECOVERY" (new state ?) and ensure
>> > those things are caught if they happen outside of a recovery.
>
> I replied privately ... as long as we ack before we start a new command
> we should be ok but we shouldn't ack after.
>
> Your latest patch still does that. It will do things like start a STOP
> command *then* ack the status bits. I'm pretty sure that's bogus.
>
> That way it's a lot simpler to simply move the
>
> writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> To either right after the readl of the status reg at the beginning of
> aspeed_i2c_master_irq().
>
> I would be very surprised if that didn't work properly and wasn't much
> safer than what you are currently doing.
I think I tried your way and it worked. In anycase, Ryan will be able
to clarify for us.
>
>> Let me know if you still think we need a "RECOVERY" state.
>
> The way you just switch to stop state and store the error for later
> should work I think.
>
>> >
>> > > + if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>
>> ...
>> >
>> > > + dev_dbg(bus->dev,
>> > > + "no slave present at %02x", msg-
>> > > >addr);
>> > > + status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> > > + bus->cmd_err = -EIO;
>> > > + do_stop(bus);
>> > > + goto out_no_complete;
>> > > + } else {
>> > > + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> > > + if (msg->flags & I2C_M_RD)
>> > > + bus->master_state =
>> > > ASPEED_I2C_MASTER_RX;
>> > > + else
>> > > + bus->master_state =
>> > > ASPEED_I2C_MASTER_TX_FIRST;
>> >
>> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
>> > to handle this here ? A quick look at the TX_FIRST case makes
>> > me think we are ok there but I'm not sure about the RX case.
>>
>> I did not think that there is an SMBUS_QUICK RX. Could you point me
>> to an example?
>
> Not so much an RX, it's more like you are sending a 1-bit data in
> the place of the Rd/Wr bit. So you have a read with a lenght of 0,
> I don't think in that case you should set ASPEED_I2CD_M_RX_CMD in
> __aspeed_i2c_do_start
Forget what I said, I was just not thinking about the fact that SMBus
emulation causes the data bit to be encoded as the R/W flag. I see
what you are saying; you are correct.
>
>> > I'm not sure the RX case is tight also. What completion does the
>> > HW give you for the address cycle ? Won't you get that before it
>> > has received the first character ? IE. You fall through to
>> > the read case of the state machine with the read potentially
>> > not complete yet no ?
>>
>> ...
>> > > + case ASPEED_I2C_MASTER_RX:
>> > > + if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> > > + dev_err(bus->dev, "master failed to RX");
>> > > + goto out_complete;
>> > > + }
>> >
>> > See my comment above for a bog standard i2c_read. Aren't you
>> > getting
>> > the completion for the address before the read is even started ?
>>
>> In practice no, but it is probably best to be safe :-)
>
> Yup :)
>> >
>> > > + status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> > > +
>> > > + recv_byte = aspeed_i2c_read(bus,
>> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> > > + msg->buf[bus->buf_index++] = recv_byte;
>> > > +
>> > > + if (msg->flags & I2C_M_RECV_LEN &&
>> > > + recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> > > + msg->len = recv_byte +
>> > > + ((msg->flags &
>> > > I2C_CLIENT_PEC) ? 2 : 1);
>>
>> ...
>> > > + return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> > > + & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> > > + | ((clk_low <<
>> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> > > + & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> > > + | (base_clk &
>> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> > > +}
>> >
>> > As I think I mentioned earlier, the AST2500 has a slightly
>> > different
>> > register layout which support larger values for high and low, thus
>> > allowing a finer granularity.
>>
>> I am developing against the 2500.
>
> Yes but we'd like the driver to work with both :-)
Right, I thought you were making an assertion about the 2500, if you
are making an assertion about the 2400, I do not know and do not have
one handy.
>
>> > BTW. In case you haven't, I would suggest you copy/paste the above
>> > in
>> > a userspace app and run it for all frequency divisors and see if
>> > your
>> > results match the aspeed table :)
>>
>> Good call.
>
> If you end up doing that, can you shoot it my way ? I can take care
> of making sure it's all good for the 2400.
Will do.
>
>> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> > > + struct platform_device *pdev)
>> > > +{
>> > > + u32 clk_freq, divisor;
>> > > + struct clk *pclk;
>> > > + int ret;
>> > > +
>> > > + pclk = devm_clk_get(&pdev->dev, NULL);
>> > > + if (IS_ERR(pclk)) {
>> > > + dev_err(&pdev->dev, "clk_get failed\n");
>> > > + return PTR_ERR(pclk);
>> > > + }
>> > > + ret = of_property_read_u32(pdev->dev.of_node,
>> > > + "clock-frequency", &clk_freq);
>> >
>> > See my previous comment about calling that 'bus-frequency' rather
>> > than 'clock-frequency'.
>> >
>> > > + if (ret < 0) {
>> > > + dev_err(&pdev->dev,
>> > > + "Could not read clock-frequency
>> > > property\n");
>> > > + clk_freq = 100000;
>> > > + }
>> > > + divisor = clk_get_rate(pclk) / clk_freq;
>> > > + /* We just need the clock rate, we don't actually use the
>> > > clk object. */
>> > > + devm_clk_put(&pdev->dev, pclk);
>> > > +
>> > > + /* Set AC Timing */
>> > > + if (clk_freq / 1000 > 1000) {
>> > > + aspeed_i2c_write(bus, aspeed_i2c_read(bus,
>> > > + ASPEED_I2C_FU
>> > > N_CTRL_REG) |
>> > > + ASPEED_I2CD_M_HIGH_SPEED_EN |
>> > > + ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
>> > > + ASPEED_I2CD_SDA_DRIVE_1T_EN,
>> > > + ASPEED_I2C_FUN_CTRL_REG);
>> > > +
>> > > + aspeed_i2c_write(bus, 0x3,
>> > > ASPEED_I2C_AC_TIMING_REG2);
>> > > + aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > + ASPEED_I2C_AC_TIMING_REG1);
>> >
>> > I already discussed by doubts about the above. I can try to scope
>> > it with the EVB if you don't get to it. For now I'd rather take the
>> > code out.
>> >
>> > We should ask aspeed from what frequency the "1T" stuff is useful.
>>
>> Will do, I will try to rope Ryan in on the next review; it will be
>> good for him to get used to working with upstream anyway.
>
> Yup. However, for the sake of getting something upstream (and in
> OpenBMC 4.10 kernel) asap, I would suggest just dropping support
> for those fast speeds for now, we can add them back later.
Alright, that's fine. Still, Ryan, could you provide some context on this?
>
>> >
>> > > + } else {
>> > > + aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > + ASPEED_I2C_AC_TIMING_REG1);
>> > > + aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> > > + ASPEED_I2C_AC_TIMING_REG2);
>> > > + }
>>
>> ...
>> > > + spin_lock_init(&bus->lock);
>> > > + init_completion(&bus->cmd_complete);
>> > > + bus->adap.owner = THIS_MODULE;
>> > > + bus->adap.retries = 0;
>> > > + bus->adap.timeout = 5 * HZ;
>> > > + bus->adap.algo = &aspeed_i2c_algo;
>> > > + bus->adap.algo_data = bus;
>> > > + bus->adap.dev.parent = &pdev->dev;
>> > > + bus->adap.dev.of_node = pdev->dev.of_node;
>> > > + snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed
>> > > i2c");
>> >
>> > Another trivial one, should we put some kind of bus number
>> > in that string ?
>>
>> Whoops, looks like I missed this one; I will get to it in the next
>> revision.
>
> Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
Yep, I will get it in v8.
>
>> >
>> > > + bus->dev = &pdev->dev;
>> > > +
>> > > + /* reset device: disable master & slave functions */
>> > > + aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
>>
>> ...
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree"
>> in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v9 3/3] backlight arcxcnn add support for ArcticSand devices
From: Lee Jones @ 2017-04-25 8:19 UTC (permalink / raw)
To: Olimpiu Dejeu
Cc: robh-DgEjT+Ai2ygdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
jingoohan1-Re5JQEeQqe8AvxtiuMwx3w, bdodge-eV7fy4qpoLhpLGFMi4vTTA,
joe-6d6DIl74uiNBDgjK7y7TUQ, medasaro-eV7fy4qpoLhpLGFMi4vTTA,
daniel.thompson-QSEj5FYQhm4dnm+yROfE0A,
lkp-ral2JQCrhuEAvxtiuMwx3w, fengguang.wu-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <1490115528-11044-3-git-send-email-olimpiu-eV7fy4qpoLhpLGFMi4vTTA@public.gmane.org>
On Tue, 21 Mar 2017, Olimpiu Dejeu wrote:
> backlight: Add support for Arctic Sand LED backlight driver chips
> This driver provides support for the Arctic Sand arc2c0608 chip,
> and provides a framework to support future devices.
> Reviewed-by: Daniel Thompson <daniel.thompson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Olimpiu Dejeu <olimpiu-eV7fy4qpoLhpLGFMi4vTTA@public.gmane.org>
> ---
> v8 => v9:
> - Addressing kbuild test robot WARNING: PTR_ERR_OR_ZERO can be used
> v7 => v8:
> - Version updated to match other patch in set. No other changes.
> v6 => v7:
> - Addressing issues brought up by Daniel Thompson
> v5 => v6:
> - Addressing issues brought up by Daniel Thompson
> v4 => v5:
> - Code style changes per Joe Perches and Jingoo Han
> v3 => v4:
> - Code style changes per Joe Perches and Jingoo Han
> v2 => v3:
> - Renamed variables to comply with conventions on naming
> - Corrected device name in arcxcnn.h
> v1 => v2:
> - Removed "magic numbers" to initialize registers
> - Cleaned up device tree bindings
> - Fixed code style to address comments and pass "checkpatch"
> - Removed unneeded debug and testing code
>
>
> drivers/video/backlight/Kconfig | 7 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/arcxcnn_bl.c | 419 +++++++++++++++++++++++++++++++++++
> 3 files changed, 427 insertions(+)
> create mode 100644 drivers/video/backlight/arcxcnn_bl.c
Applied this (and the correct (v9) version of [PATCH 2/3] too).
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 5ffa4b4..4e1d2ad 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -460,6 +460,13 @@ config BACKLIGHT_BD6107
> help
> If you have a Rohm BD6107 say Y to enable the backlight driver.
>
> +config BACKLIGHT_ARCXCNN
> + tristate "Backlight driver for the Arctic Sands ARCxCnnnn family"
> + depends on I2C
> + help
> + If you have an ARCxCnnnn family backlight say Y to enable
> + the backlight driver.
> +
> endif # BACKLIGHT_CLASS_DEVICE
>
> endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 16ec534..8905129 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o
> obj-$(CONFIG_BACKLIGHT_TOSA) += tosa_bl.o
> obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
> obj-$(CONFIG_BACKLIGHT_WM831X) += wm831x_bl.o
> +obj-$(CONFIG_BACKLIGHT_ARCXCNN) += arcxcnn_bl.o
> diff --git a/drivers/video/backlight/arcxcnn_bl.c b/drivers/video/backlight/arcxcnn_bl.c
> new file mode 100644
> index 0000000..e01b1b0
> --- /dev/null
> +++ b/drivers/video/backlight/arcxcnn_bl.c
> @@ -0,0 +1,419 @@
> +/*
> + * Backlight driver for ArcticSand ARC_X_C_0N_0N Devices
> + *
> + * Copyright 2016 ArcticSand, Inc.
> + * Author : Brian Dodge <bdodge-eV7fy4qpoLhpLGFMi4vTTA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +enum arcxcnn_chip_id {
> + ARC2C0608
> +};
> +
> +/**
> + * struct arcxcnn_platform_data
> + * @name : Backlight driver name (NULL will use default)
> + * @initial_brightness : initial value of backlight brightness
> + * @leden : initial LED string enables, upper bit is global on/off
> + * @led_config_0 : fading speed (period between intensity steps)
> + * @led_config_1 : misc settings, see datasheet
> + * @dim_freq : pwm dimming frequency if in pwm mode
> + * @comp_config : misc config, see datasheet
> + * @filter_config : RC/PWM filter config, see datasheet
> + * @trim_config : full scale current trim, see datasheet
> + */
> +struct arcxcnn_platform_data {
> + const char *name;
> + u16 initial_brightness;
> + u8 leden;
> + u8 led_config_0;
> + u8 led_config_1;
> + u8 dim_freq;
> + u8 comp_config;
> + u8 filter_config;
> + u8 trim_config;
> +};
> +
> +#define ARCXCNN_CMD 0x00 /* Command Register */
> +#define ARCXCNN_CMD_STDBY 0x80 /* I2C Standby */
> +#define ARCXCNN_CMD_RESET 0x40 /* Reset */
> +#define ARCXCNN_CMD_BOOST 0x10 /* Boost */
> +#define ARCXCNN_CMD_OVP_MASK 0x0C /* --- Over Voltage Threshold */
> +#define ARCXCNN_CMD_OVP_XXV 0x0C /* <rsvrd> Over Voltage Threshold */
> +#define ARCXCNN_CMD_OVP_20V 0x08 /* 20v Over Voltage Threshold */
> +#define ARCXCNN_CMD_OVP_24V 0x04 /* 24v Over Voltage Threshold */
> +#define ARCXCNN_CMD_OVP_31V 0x00 /* 31.4v Over Voltage Threshold */
> +#define ARCXCNN_CMD_EXT_COMP 0x01 /* part (0) or full (1) ext. comp */
> +
> +#define ARCXCNN_CONFIG 0x01 /* Configuration */
> +#define ARCXCNN_STATUS1 0x02 /* Status 1 */
> +#define ARCXCNN_STATUS2 0x03 /* Status 2 */
> +#define ARCXCNN_FADECTRL 0x04 /* Fading Control */
> +#define ARCXCNN_ILED_CONFIG 0x05 /* ILED Configuration */
> +#define ARCXCNN_ILED_DIM_PWM 0x00 /* config dim mode pwm */
> +#define ARCXCNN_ILED_DIM_INT 0x04 /* config dim mode internal */
> +#define ARCXCNN_LEDEN 0x06 /* LED Enable Register */
> +#define ARCXCNN_LEDEN_ISETEXT 0x80 /* Full-scale current set extern */
> +#define ARCXCNN_LEDEN_MASK 0x3F /* LED string enables mask */
> +#define ARCXCNN_LEDEN_BITS 0x06 /* Bits of LED string enables */
> +#define ARCXCNN_LEDEN_LED1 0x01
> +#define ARCXCNN_LEDEN_LED2 0x02
> +#define ARCXCNN_LEDEN_LED3 0x04
> +#define ARCXCNN_LEDEN_LED4 0x08
> +#define ARCXCNN_LEDEN_LED5 0x10
> +#define ARCXCNN_LEDEN_LED6 0x20
> +
> +#define ARCXCNN_WLED_ISET_LSB 0x07 /* LED ISET LSB (in upper nibble) */
> +#define ARCXCNN_WLED_ISET_LSB_SHIFT 0x04 /* ISET LSB Left Shift */
> +#define ARCXCNN_WLED_ISET_MSB 0x08 /* LED ISET MSB (8 bits) */
> +
> +#define ARCXCNN_DIMFREQ 0x09
> +#define ARCXCNN_COMP_CONFIG 0x0A
> +#define ARCXCNN_FILT_CONFIG 0x0B
> +#define ARCXCNN_IMAXTUNE 0x0C
> +#define ARCXCNN_ID_MSB 0x1E
> +#define ARCXCNN_ID_LSB 0x1F
> +
> +#define MAX_BRIGHTNESS 4095
> +#define INIT_BRIGHT 60
> +
> +struct arcxcnn {
> + struct i2c_client *client;
> + struct backlight_device *bl;
> + struct device *dev;
> + struct arcxcnn_platform_data *pdata;
> +};
> +
> +static int arcxcnn_update_field(struct arcxcnn *lp, u8 reg, u8 mask, u8 data)
> +{
> + int ret;
> + u8 tmp;
> +
> + ret = i2c_smbus_read_byte_data(lp->client, reg);
> + if (ret < 0) {
> + dev_err(lp->dev, "failed to read 0x%.2x\n", reg);
> + return ret;
> + }
> +
> + tmp = (u8)ret;
> + tmp &= ~mask;
> + tmp |= data & mask;
> +
> + return i2c_smbus_write_byte_data(lp->client, reg, tmp);
> +}
> +
> +static int arcxcnn_set_brightness(struct arcxcnn *lp, u32 brightness)
> +{
> + int ret;
> + u8 val;
> +
> + /* lower nibble of brightness goes in upper nibble of LSB register */
> + val = (brightness & 0xF) << ARCXCNN_WLED_ISET_LSB_SHIFT;
> + ret = i2c_smbus_write_byte_data(lp->client,
> + ARCXCNN_WLED_ISET_LSB, val);
> + if (ret < 0)
> + return ret;
> +
> + /* remaining 8 bits of brightness go in MSB register */
> + val = (brightness >> 4);
> + return i2c_smbus_write_byte_data(lp->client,
> + ARCXCNN_WLED_ISET_MSB, val);
> +}
> +
> +static int arcxcnn_bl_update_status(struct backlight_device *bl)
> +{
> + struct arcxcnn *lp = bl_get_data(bl);
> + u32 brightness = bl->props.brightness;
> + int ret;
> +
> + if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> + brightness = 0;
> +
> + ret = arcxcnn_set_brightness(lp, brightness);
> + if (ret)
> + return ret;
> +
> + /* set power-on/off/save modes */
> + return arcxcnn_update_field(lp, ARCXCNN_CMD, ARCXCNN_CMD_STDBY,
> + (bl->props.power == 0) ? 0 : ARCXCNN_CMD_STDBY);
> +}
> +
> +static const struct backlight_ops arcxcnn_bl_ops = {
> + .options = BL_CORE_SUSPENDRESUME,
> + .update_status = arcxcnn_bl_update_status,
> +};
> +
> +static int arcxcnn_backlight_register(struct arcxcnn *lp)
> +{
> + struct backlight_properties *props;
> + const char *name = lp->pdata->name ? : "arctic_bl";
> +
> + props = devm_kzalloc(lp->dev, sizeof(*props), GFP_KERNEL);
> + if (!props)
> + return -ENOMEM;
> +
> + props->type = BACKLIGHT_PLATFORM;
> + props->max_brightness = MAX_BRIGHTNESS;
> +
> + if (lp->pdata->initial_brightness > props->max_brightness)
> + lp->pdata->initial_brightness = props->max_brightness;
> +
> + props->brightness = lp->pdata->initial_brightness;
> +
> + lp->bl = devm_backlight_device_register(lp->dev, name, lp->dev, lp,
> + &arcxcnn_bl_ops, props);
> + return PTR_ERR_OR_ZERO(lp->bl);
> +}
> +
> +static void arcxcnn_parse_dt(struct arcxcnn *lp)
> +{
> + struct device *dev = lp->dev;
> + struct device_node *node = dev->of_node;
> + u32 prog_val, num_entry, entry, sources[ARCXCNN_LEDEN_BITS];
> + int ret;
> +
> + /* device tree entry isn't required, defaults are OK */
> + if (!node)
> + return;
> +
> + ret = of_property_read_string(node, "label", &lp->pdata->name);
> + if (ret < 0)
> + lp->pdata->name = NULL;
> +
> + ret = of_property_read_u32(node, "default-brightness", &prog_val);
> + if (ret == 0)
> + lp->pdata->initial_brightness = prog_val;
> +
> + ret = of_property_read_u32(node, "arc,led-config-0", &prog_val);
> + if (ret == 0)
> + lp->pdata->led_config_0 = (u8)prog_val;
> +
> + ret = of_property_read_u32(node, "arc,led-config-1", &prog_val);
> + if (ret == 0)
> + lp->pdata->led_config_1 = (u8)prog_val;
> +
> + ret = of_property_read_u32(node, "arc,dim-freq", &prog_val);
> + if (ret == 0)
> + lp->pdata->dim_freq = (u8)prog_val;
> +
> + ret = of_property_read_u32(node, "arc,comp-config", &prog_val);
> + if (ret == 0)
> + lp->pdata->comp_config = (u8)prog_val;
> +
> + ret = of_property_read_u32(node, "arc,filter-config", &prog_val);
> + if (ret == 0)
> + lp->pdata->filter_config = (u8)prog_val;
> +
> + ret = of_property_read_u32(node, "arc,trim-config", &prog_val);
> + if (ret == 0)
> + lp->pdata->trim_config = (u8)prog_val;
> +
> + ret = of_property_count_u32_elems(node, "led-sources");
> + if (ret < 0) {
> + lp->pdata->leden = ARCXCNN_LEDEN_MASK; /* all on is default */
> + } else {
> + num_entry = ret;
> + if (num_entry > ARCXCNN_LEDEN_BITS)
> + num_entry = ARCXCNN_LEDEN_BITS;
> +
> + ret = of_property_read_u32_array(node, "led-sources", sources,
> + num_entry);
> + if (ret < 0) {
> + dev_err(dev, "led-sources node is invalid.\n");
> + return;
> + }
> +
> + lp->pdata->leden = 0;
> +
> + /* for each enable in source, set bit in led enable */
> + for (entry = 0; entry < num_entry; entry++) {
> + u8 onbit = 1 << sources[entry];
> +
> + lp->pdata->leden |= onbit;
> + }
> + }
> +}
> +
> +static int arcxcnn_probe(struct i2c_client *cl, const struct i2c_device_id *id)
> +{
> + struct arcxcnn *lp;
> + int ret;
> +
> + if (!i2c_check_functionality(cl->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EIO;
> +
> + lp = devm_kzalloc(&cl->dev, sizeof(*lp), GFP_KERNEL);
> + if (!lp)
> + return -ENOMEM;
> +
> + lp->client = cl;
> + lp->dev = &cl->dev;
> + lp->pdata = dev_get_platdata(&cl->dev);
> +
> + /* reset the device */
> + ret = i2c_smbus_write_byte_data(lp->client,
> + ARCXCNN_CMD, ARCXCNN_CMD_RESET);
> + if (ret)
> + goto probe_err;
> +
> + if (!lp->pdata) {
> + lp->pdata = devm_kzalloc(lp->dev,
> + sizeof(*lp->pdata), GFP_KERNEL);
> + if (!lp->pdata)
> + return -ENOMEM;
> +
> + /* Setup defaults based on power-on defaults */
> + lp->pdata->name = NULL;
> + lp->pdata->initial_brightness = INIT_BRIGHT;
> + lp->pdata->leden = ARCXCNN_LEDEN_MASK;
> +
> + lp->pdata->led_config_0 = i2c_smbus_read_byte_data(
> + lp->client, ARCXCNN_FADECTRL);
> +
> + lp->pdata->led_config_1 = i2c_smbus_read_byte_data(
> + lp->client, ARCXCNN_ILED_CONFIG);
> + /* insure dim mode is not default pwm */
> + lp->pdata->led_config_1 |= ARCXCNN_ILED_DIM_INT;
> +
> + lp->pdata->dim_freq = i2c_smbus_read_byte_data(
> + lp->client, ARCXCNN_DIMFREQ);
> +
> + lp->pdata->comp_config = i2c_smbus_read_byte_data(
> + lp->client, ARCXCNN_COMP_CONFIG);
> +
> + lp->pdata->filter_config = i2c_smbus_read_byte_data(
> + lp->client, ARCXCNN_FILT_CONFIG);
> +
> + lp->pdata->trim_config = i2c_smbus_read_byte_data(
> + lp->client, ARCXCNN_IMAXTUNE);
> +
> + if (IS_ENABLED(CONFIG_OF))
> + arcxcnn_parse_dt(lp);
> + }
> +
> + i2c_set_clientdata(cl, lp);
> +
> + /* constrain settings to what is possible */
> + if (lp->pdata->initial_brightness > MAX_BRIGHTNESS)
> + lp->pdata->initial_brightness = MAX_BRIGHTNESS;
> +
> + /* set initial brightness */
> + ret = arcxcnn_set_brightness(lp, lp->pdata->initial_brightness);
> + if (ret)
> + goto probe_err;
> +
> + /* set other register values directly */
> + ret = i2c_smbus_write_byte_data(lp->client, ARCXCNN_FADECTRL,
> + lp->pdata->led_config_0);
> + if (ret)
> + goto probe_err;
> +
> + ret = i2c_smbus_write_byte_data(lp->client, ARCXCNN_ILED_CONFIG,
> + lp->pdata->led_config_1);
> + if (ret)
> + goto probe_err;
> +
> + ret = i2c_smbus_write_byte_data(lp->client, ARCXCNN_DIMFREQ,
> + lp->pdata->dim_freq);
> + if (ret)
> + goto probe_err;
> +
> + ret = i2c_smbus_write_byte_data(lp->client, ARCXCNN_COMP_CONFIG,
> + lp->pdata->comp_config);
> + if (ret)
> + goto probe_err;
> +
> + ret = i2c_smbus_write_byte_data(lp->client, ARCXCNN_FILT_CONFIG,
> + lp->pdata->filter_config);
> + if (ret)
> + goto probe_err;
> +
> + ret = i2c_smbus_write_byte_data(lp->client, ARCXCNN_IMAXTUNE,
> + lp->pdata->trim_config);
> + if (ret)
> + goto probe_err;
> +
> + /* set initial LED Enables */
> + arcxcnn_update_field(lp, ARCXCNN_LEDEN,
> + ARCXCNN_LEDEN_MASK, lp->pdata->leden);
> +
> + ret = arcxcnn_backlight_register(lp);
> + if (ret)
> + goto probe_register_err;
> +
> + backlight_update_status(lp->bl);
> +
> + return 0;
> +
> +probe_register_err:
> + dev_err(lp->dev,
> + "failed to register backlight.\n");
> +
> +probe_err:
> + dev_err(lp->dev,
> + "failure ret: %d\n", ret);
> + return ret;
> +}
> +
> +static int arcxcnn_remove(struct i2c_client *cl)
> +{
> + struct arcxcnn *lp = i2c_get_clientdata(cl);
> +
> + /* disable all strings (ignore errors) */
> + i2c_smbus_write_byte_data(lp->client,
> + ARCXCNN_LEDEN, 0x00);
> + /* reset the device (ignore errors) */
> + i2c_smbus_write_byte_data(lp->client,
> + ARCXCNN_CMD, ARCXCNN_CMD_RESET);
> +
> + lp->bl->props.brightness = 0;
> +
> + backlight_update_status(lp->bl);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id arcxcnn_dt_ids[] = {
> + { .compatible = "arc,arc2c0608" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, arcxcnn_dt_ids);
> +
> +static const struct i2c_device_id arcxcnn_ids[] = {
> + {"arc2c0608", ARC2C0608},
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, arcxcnn_ids);
> +
> +static struct i2c_driver arcxcnn_driver = {
> + .driver = {
> + .name = "arcxcnn_bl",
> + .of_match_table = of_match_ptr(arcxcnn_dt_ids),
> + },
> + .probe = arcxcnn_probe,
> + .remove = arcxcnn_remove,
> + .id_table = arcxcnn_ids,
> +};
> +module_i2c_driver(arcxcnn_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Brian Dodge <bdodge-eV7fy4qpoLhpLGFMi4vTTA@public.gmane.org>");
> +MODULE_DESCRIPTION("ARCXCNN Backlight driver");
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox