* Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG
From: Pavel Machek @ 2017-04-11 17:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Jacek Anaszewski, Rob Herring, Richard Purdie, linux-kernel,
linux-leds, linux-arm-msm, Mark Rutland, devicetree
In-Reply-To: <20170410191948.GE15143@minitux>
[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]
Hi!
> > > How do we do with patterns that are implementable by the LP5xx but are
> > > not with the LPG? Should we reject those or should we do some sort of
> > > best-effort approach in the kernel?
> >
> > Lets say you get series of
> >
> > (red, green, blue, delta_t )
> >
> > points, meaning "in delta_t msec, change color to red, green,
> > blue. Lets ignore other channels for now. delta_t of 0 would be step
> > change. Would such interface work for you?
>
> So I presume this would be input to the RGB trigger that we discussed.
> But in my current device I have 6 LEDs, that are not in any RGB-like
> configuration. So we would need to come up with an interface that looks
> to be the same in both single-LED and RGB-LED setups.
Ok.
> This should be sufficient to describe a subset of the patterns I've seen
> so far in products.
>
> But let's consider the standard use case for an RGB LED on an Android
> phone; continuously blinking (pulsing based on patterns) as you have
> some notifications waiting. In this case you want the LED hardware to do
> all the work, so that you can deep-idle the CPU. So we would need to
> introduce a "repeat pattern"-command.
I'd say have additional parameter with number of repetitions. Yes. In
your case you can do 1 and infinity, LP5XX can do 1-255 or infinity.
> Then consider the fact that you want your patterns to have decent
> resolution, but you have a limited amount of storage. So we either have
> to be able to detect palindromes or have a way to represent this.
I'm not sure how common hardware support for palindromes is going to
be. I'd say "detect", but...
> > Simple compiler from this to LP5XX code should not be hard to
> > do.
>
> It sounds fairly straight forward to convert a pattern to instructions,
> but we do have an extremely limited amount of storage so it must be a
> quite good implementation for people to be able to use it for anything
> real.
>
> We could implement some optimization steps where we try to detect slopes
> and generate ramp-instructions instead of set-pwm + wait instructions,
> use some variables to handle ramp up/down and we could probably generate
> some jump instructions to implement loops.
Actually it is easier than that. Hardware can do slopes itself. If we
see change with non-zero delta_t, we issue slope, otherwise we issue
set_value.
Here's example "compiler": https://gitlab.com/tui/tui/blob/master/ofone/notcc.py
Here's example "program": https://gitlab.com/tui/tui/blob/master/ofone/tests.notcc/primes.nc
> But do we really want this logic in the kernel, for each LED chip
> supporting patterns?
I'd say so, yes. It should be, dunno, 200? 500? lines of code for
LP5XX? Sounds acceptable.
Otherwise we'd have to have led-chip-dependend part in userspace. That
would be ok... but we'd _still_ need led-chip-dependend part in the
kernel... and driver spread between kernel and userland is difficult.
The code needs to be created, anyway, so lets put it in kernel.
> > AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> >
>
> So out of the three examples I've looked at we're skipping one and we're
> abstracting away most functionality from another.
Well. We don't need to _skip_ AS3676, but its pattern engine is
basically useless for anything involving different PWM levels.
And abstracting away most of LP5XX functionality... well, you can
compute prime numbers on that chip (see example above), but you better
should not. And patterns we'll pretty much expose all the functionality.
> I'm sorry for being pessimistic about this, but while I can see the
> theoretical benefit of providing a uniform interface for this to user
> space I see three very different pieces of hardware that would be used
> in three different ways in products.
Three different pieces of hardware, at least two of them used in
phones to provide blinking leds... I'd say common interface is the
right thing to do.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: Re: [PATCH v2 03/11] dt-bindings: add device tree binding for X-Powers AXP803 PMIC
From: Icenowy Zheng @ 2017-04-11 18:08 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel, linux-sunxi, Rob Herring, linux-kernel,
Liam Girdwood, devicetree, Maxime Ripard, Chen-Yu Tsai
2017年4月12日 上午12:29于 Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>写道：
>
> On Tue, 11 Apr 2017, Chen-Yu Tsai wrote:
>
> > Hi,
> >
> > On Tue, Apr 11, 2017 at 11:00 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> > >
> > >
> > > On Tue, 11 Apr 2017, Lee Jones wrote:
> > >
> > >> On Sat, 08 Apr 2017, Icenowy Zheng wrote:
> > >>
> > >>> AXP803 is a PMIC produced by Shenzhen X-Powers, with either I2C or RSB
> > >>> bus.
> > >>>
> > >>> Add a compatible for it.
> > >>>
> > >>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> > >>> Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> > >>> ---
> > >>> Changes in v2:
> > >>> - Place AXP803 before AXP806/809.
> > >>> - Added Chen-Yu's ACK.
> > >>>
> > >>> Documentation/devicetree/bindings/mfd/axp20x.txt | 5 +++--
> > >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>
> > >>
> > >> For my own reference:
> > >> Acked-for-MFD-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >
> > >
> > > Excuse me... Who will apply this patch?
>
> Excuse you? Are you trying to be polite, or rude?
>
> I'm guessing due to the lagging '...', that it's the latter?
Oh sorry, I mean the former...
>
> > > I think this patch should be applied by you as it's in bindings/mfd
> > > directory, however, if I'm wrong, please point out ;-)
> >
> > We need the DT maintainers to ack it first. :)
>
> We don't really need the DT Maintainers to get involved here.
>
> What I do need is either clear direction as to how this
> patch-set should be applied, or an indication of which patches are
> dependant on others and which are orthogonal and can be applied
> independently.
This patch is independent, although the regulator binding patch
depends on it.
The real driver patches will need the binding patches to be merged
first.
And the dt patches depend on also the binding patches.
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
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: [linux-sunxi] Re: [PATCH v2 03/11] dt-bindings: add device tree binding for X-Powers AXP803 PMIC
From: Icenowy Zheng @ 2017-04-11 18:08 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel, linux-sunxi, Rob Herring, linux-kernel,
Liam Girdwood, devicetree, Maxime Ripard, Chen-Yu Tsai
2017年4月12日 上午12:29于 Lee Jones <lee.jones@linaro.org>写道：
>
> On Tue, 11 Apr 2017, Chen-Yu Tsai wrote:
>
> > Hi,
> >
> > On Tue, Apr 11, 2017 at 11:00 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> > >
> > >
> > > On Tue, 11 Apr 2017, Lee Jones wrote:
> > >
> > >> On Sat, 08 Apr 2017, Icenowy Zheng wrote:
> > >>
> > >>> AXP803 is a PMIC produced by Shenzhen X-Powers, with either I2C or RSB
> > >>> bus.
> > >>>
> > >>> Add a compatible for it.
> > >>>
> > >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > >>> Acked-by: Chen-Yu Tsai <wens@csie.org>
> > >>> ---
> > >>> Changes in v2:
> > >>> - Place AXP803 before AXP806/809.
> > >>> - Added Chen-Yu's ACK.
> > >>>
> > >>> Documentation/devicetree/bindings/mfd/axp20x.txt | 5 +++--
> > >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>
> > >>
> > >> For my own reference:
> > >> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> > >
> > >
> > > Excuse me... Who will apply this patch?
>
> Excuse you? Are you trying to be polite, or rude?
>
> I'm guessing due to the lagging '...', that it's the latter?
Oh sorry, I mean the former...
>
> > > I think this patch should be applied by you as it's in bindings/mfd
> > > directory, however, if I'm wrong, please point out ;-)
> >
> > We need the DT maintainers to ack it first. :)
>
> We don't really need the DT Maintainers to get involved here.
>
> What I do need is either clear direction as to how this
> patch-set should be applied, or an indication of which patches are
> dependant on others and which are orthogonal and can be applied
> independently.
This patch is independent, although the regulator binding patch
depends on it.
The real driver patches will need the binding patches to be merged
first.
And the dt patches depend on also the binding patches.
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/2] mtd: spi-nor: add driver for STM32 quad spi flash controller
From: Marek Vasut @ 2017-04-11 18:31 UTC (permalink / raw)
To: Ludovic BARRE, Cyrille Pitchen
Cc: David Woodhouse, Brian Norris, Boris Brezillon,
Richard Weinberger, Alexandre Torgue, Rob Herring,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <f9b4b8b0-4987-7f67-df3b-d32d6c130818-qxv4g6HH51o@public.gmane.org>
On 04/10/2017 06:52 PM, Ludovic BARRE wrote:
> hi Marek
>
> tomorrow, I send a v3 with your/Rob reviews.
Super, thanks! I'll be pretty busy till Friday, so please keep in mind
the final review might take a bit.
> BR
>
> Ludo
>
>
> On 04/10/2017 06:15 PM, Marek Vasut wrote:
>> On 04/10/2017 11:08 AM, Ludovic BARRE wrote:
>>> On 04/07/2017 01:55 AM, Marek Vasut wrote:
>>>> On 03/31/2017 07:02 PM, Ludovic Barre wrote:
>>>>> From: Ludovic Barre <ludovic.barre-qxv4g6HH51o@public.gmane.org>
>>>>>
>>>>> The quadspi is a specialized communication interface targeting single,
>>>>> dual or quad SPI Flash memories.
>>>>>
>>>>> It can operate in any of the following modes:
>>>>> -indirect mode: all the operations are performed using the quadspi
>>>>> registers
>>>>> -read memory-mapped mode: the external Flash memory is mapped to the
>>>>> microcontroller address space and is seen by the system as if it
>>>>> was
>>>>> an internal memory
>>>>>
>>>>> Signed-off-by: Ludovic Barre <ludovic.barre-qxv4g6HH51o@public.gmane.org>
>>>>> ---
>>>>> drivers/mtd/spi-nor/Kconfig | 7 +
>>>>> drivers/mtd/spi-nor/Makefile | 1 +
>>>>> drivers/mtd/spi-nor/stm32-quadspi.c | 690
>>>>> ++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 698 insertions(+)
>>>>> create mode 100644 drivers/mtd/spi-nor/stm32-quadspi.c
>>>>>
>>>> [...]
>>>>
>>>>> +struct stm32_qspi_flash {
>>>>> + struct spi_nor nor;
>>>>> + u32 cs;
>>>>> + u32 fsize;
>>>>> + u32 presc;
>>>>> + struct stm32_qspi *qspi;
>>>>> +};
>>>> [...]
>>>>
>>>>> +struct stm32_qspi_cmd {
>>>>> + struct {
>>>>> + u8 addr_width;
>>>>> + u8 dummy;
>>>>> + u8 data;
>>>>> + } conf;
>>>> Is there any benefit in having this structure here or could you just
>>>> make the struct stm32_qspi_cmd flat ?
>>> no benefit, it was just to regroup, so I can do a flat structure
>> Well, as you like, but I think it does make sense to just make it flat.
>>
>>>>> + u8 opcode;
>>>>> + u32 framemode;
>>>>> + u32 qspimode;
>>>>> + u32 addr;
>>>>> + size_t len;
>>>>> + void *buf;
>>>>> +};
>>>> [...]
>>>>
>>>>> +static ssize_t stm32_qspi_read(struct spi_nor *nor, loff_t from,
>>>>> size_t len,
>>>>> + u_char *buf)
>>>>> +{
>>>>> + struct stm32_qspi_flash *flash = nor->priv;
>>>>> + struct stm32_qspi *qspi = flash->qspi;
>>>>> + struct stm32_qspi_cmd cmd;
>>>>> + int err;
>>>>> +
>>>>> + dev_dbg(qspi->dev, "read(%#.2x): buf:%p from:%#.8x len:%#x\n",
>>>>> + nor->read_opcode, buf, (u32)from, len);
>>>>> +
>>>>> + memset(&cmd, 0, sizeof(cmd));
>>>>> + cmd.opcode = nor->read_opcode;
>>>>> + cmd.conf.addr_width = nor->addr_width;
>>>>> + cmd.addr = (u32)from;
>>>> loff_t (from) can be 64bit ... how do we handle this ?
>>> I'm surprise by the question,
>>> the SPI NOR device uses 3 Bytes or 4 bytes address mode.
>>> So, the stm32 qspi controller has a 32 bit register for NOR address.
>>> On the other hand the framework and other drivers used this variable
>>> (from) like
>>> a 32 bits.
>> Hmmm, (rhetorical question) then why do we even use loff_t in the
>> framework ?
>>
>> Anyway, this is no problem then.
> In fact, the loff_t 64 bit come from mtd interface
> (needed to address biggest device constraint) but not needed for spi-nor
> devices.
>>>>> + cmd.conf.data = 1;
>>>>> + cmd.conf.dummy = nor->read_dummy;
>>>>> + cmd.len = len;
>>>>> + cmd.buf = buf;
>>>>> + cmd.qspimode = qspi->read_mode;
>>>>> +
>>>>> + stm32_qspi_set_framemode(nor, &cmd, true);
>>>>> + err = stm32_qspi_send(flash, &cmd);
>>>>> +
>>>>> + return err ? err : len;
>>>>> +}
>>>> [...]
>>>>
>>>>> +static irqreturn_t stm32_qspi_irq(int irq, void *dev_id)
>>>>> +{
>>>>> + struct stm32_qspi *qspi = (struct stm32_qspi *)dev_id;
>>>>> + u32 cr, sr, fcr = 0;
>>>>> +
>>>>> + cr = readl_relaxed(qspi->io_base + QUADSPI_CR);
>>>>> + sr = readl_relaxed(qspi->io_base + QUADSPI_SR);
>>>>> +
>>>>> + if ((cr & CR_TCIE) && (sr & SR_TCF)) {
>>>>> + /* tx complete */
>>>>> + fcr |= FCR_CTCF;
>>>>> + complete(&qspi->cmd_completion);
>>>>> + } else {
>>>>> + dev_info(qspi->dev, "spurious interrupt\n");
>>>> You probably want to ratelimit this one ...
>>> yes it's better if there is an issue.
>> Yep
>>
>>>>> + }
>>>>> +
>>>>> + writel_relaxed(fcr, qspi->io_base + QUADSPI_FCR);
>>>>> +
>>>>> + return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static int stm32_qspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>>> +{
>>>>> + struct stm32_qspi_flash *flash = nor->priv;
>>>>> + struct stm32_qspi *qspi = flash->qspi;
>>>>> +
>>>>> + mutex_lock(&qspi->lock);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static void stm32_qspi_unprep(struct spi_nor *nor, enum spi_nor_ops
>>>>> ops)
>>>>> +{
>>>>> + struct stm32_qspi_flash *flash = nor->priv;
>>>>> + struct stm32_qspi *qspi = flash->qspi;
>>>>> +
>>>>> + mutex_unlock(&qspi->lock);
>>>>> +}
>>>>> +
>>>>> +static int stm32_qspi_flash_setup(struct stm32_qspi *qspi,
>>>>> + struct device_node *np)
>>>>> +{
>>>>> + u32 width, flash_read, presc, cs_num, max_rate = 0;
>>>>> + struct stm32_qspi_flash *flash;
>>>>> + struct mtd_info *mtd;
>>>>> + int ret;
>>>>> +
>>>>> + of_property_read_u32(np, "reg", &cs_num);
>>>>> + if (cs_num >= STM32_MAX_NORCHIP)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + of_property_read_u32(np, "spi-max-frequency", &max_rate);
>>>>> + if (!max_rate)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + presc = DIV_ROUND_UP(qspi->clk_rate, max_rate) - 1;
>>>>> +
>>>>> + if (of_property_read_u32(np, "spi-rx-bus-width", &width))
>>>>> + width = 1;
>>>>> +
>>>>> + if (width == 4)
>>>>> + flash_read = SPI_NOR_QUAD;
>>>>> + else if (width == 2)
>>>>> + flash_read = SPI_NOR_DUAL;
>>>>> + else if (width == 1)
>>>>> + flash_read = SPI_NOR_NORMAL;
>>>>> + else
>>>>> + return -EINVAL;
>>>>> +
>>>>> + flash = &qspi->flash[cs_num];
>>>>> + flash->qspi = qspi;
>>>>> + flash->cs = cs_num;
>>>>> + flash->presc = presc;
>>>>> +
>>>>> + flash->nor.dev = qspi->dev;
>>>>> + spi_nor_set_flash_node(&flash->nor, np);
>>>>> + flash->nor.priv = flash;
>>>>> + mtd = &flash->nor.mtd;
>>>>> + mtd->priv = &flash->nor;
>>>>> +
>>>>> + flash->nor.read = stm32_qspi_read;
>>>>> + flash->nor.write = stm32_qspi_write;
>>>>> + flash->nor.erase = stm32_qspi_erase;
>>>>> + flash->nor.read_reg = stm32_qspi_read_reg;
>>>>> + flash->nor.write_reg = stm32_qspi_write_reg;
>>>>> + flash->nor.prepare = stm32_qspi_prep;
>>>>> + flash->nor.unprepare = stm32_qspi_unprep;
>>>>> +
>>>>> + writel_relaxed(LPTR_DFT_TIMEOUT, qspi->io_base + QUADSPI_LPTR);
>>>>> +
>>>>> + writel_relaxed(CR_PRESC(presc) | CR_FTHRES(3) | CR_TCEN |
>>>>> CR_SSHIFT
>>>>> + | CR_EN, qspi->io_base + QUADSPI_CR);
>>>>> +
>>>>> + /*
>>>>> + * in stm32 qspi controller, QUADSPI_DCR register has a fsize
>>>>> field
>>>>> + * which define the size of nor flash.
>>>>> + * if fsize is NULL, the controller can't sent spi-nor command.
>>>>> + * set a temporary value just to discover the nor flash with
>>>>> + * "spi_nor_scan". After, the right value (mtd->size) can be set.
>>>>> + */
>>>> Is 25 the smallest value ? Use a macro for this ...
>>> 25 is an arbitrary choice, I will define a smallest value
>> Cool, thanks!
>>
>
--
Best regards,
Marek Vasut
--
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 06/16] regulator: madera-ldo1: LDO1 driver for Cirrus Logic Madera codecs
From: Rob Herring @ 2017-04-11 19:20 UTC (permalink / raw)
To: Mark Brown
Cc: Richard Fitzgerald, Linus Walleij, Alexandre Courbot,
Thomas Gleixner, Jason Cooper, Lee Jones, Linux-ALSA,
patches-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170410181136.btpvcat2ijwiebvm-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
On Mon, Apr 10, 2017 at 1:11 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Apr 10, 2017 at 12:49:17PM -0500, Rob Herring wrote:
>
>> Same question as Mark. Should this share bindings with arizona? The
>> arizona one looks a bit strange, so not sure we'd want to just copy it.
>
> In what way? Other than the -gpios stuff (which would just be a trivial
> thing if we wanted to change it)?
That's mainly it. It's not clear which node wlf,ldoena goes in either.
In the parent? ldo1?
Rob
--
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: [GIT PULL] PCI: Support for configurable PCI endpoint
From: Bjorn Helgaas @ 2017-04-11 19:34 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: devicetree, Joao Pinto, linux-doc, linux-pci, nsekhar,
linux-kernel, hch, Bjorn Helgaas, linux-omap, linux-arm-kernel
In-Reply-To: <20170410154328.GB13170@bhelgaas-glaptop.roam.corp.google.com>
On Mon, Apr 10, 2017 at 10:43:28AM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 05, 2017 at 02:22:20PM +0530, Kishon Vijay Abraham I wrote:
> > Hi Bjorn,
> >
> > Please find the pull request for PCI endpoint support below. I've
> > also included all the history here.
>
> Thanks, I applied these (with v7 of the first patch) to pci/host-designware
> for v4.12.
Ok, sorry, I screwed this up. I think my branch actually had v5, not
v6. But I *think* I fixed it. Here's the diff from my branch to your
git tree. Apparently you haven't pushed the v7 patch there, so I
*think* the diff below is the diff between v6 and v7 of that first
patch.
$ git diff pci/host-designware a5c85ba45c96
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index caa7be10e473..9ae9e59b2a74 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -83,7 +83,6 @@ struct pci_epc *pci_epc_get(const char *epc_name)
goto err;
}
- class_dev_iter_exit(&iter);
get_device(&epc->dev);
return epc;
}
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 6877d6a5bcc9..92db7dcd911c 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -40,10 +40,8 @@ static struct device_type pci_epf_type;
*/
void pci_epf_linkup(struct pci_epf *epf)
{
- if (!epf->driver) {
+ if (!epf->driver)
dev_WARN(&epf->dev, "epf device not bound to driver\n");
- return;
- }
epf->driver->ops->linkup(epf);
}
@@ -59,10 +57,8 @@ EXPORT_SYMBOL_GPL(pci_epf_linkup);
*/
void pci_epf_unbind(struct pci_epf *epf)
{
- if (!epf->driver) {
+ if (!epf->driver)
dev_WARN(&epf->dev, "epf device not bound to driver\n");
- return;
- }
epf->driver->ops->unbind(epf);
module_put(epf->driver->owner);
@@ -78,10 +74,8 @@ EXPORT_SYMBOL_GPL(pci_epf_unbind);
*/
int pci_epf_bind(struct pci_epf *epf)
{
- if (!epf->driver) {
+ if (!epf->driver)
dev_WARN(&epf->dev, "epf device not bound to driver\n");
- return -EINVAL;
- }
if (!try_module_get(epf->driver->owner))
return -EAGAIN;
@@ -233,7 +227,7 @@ struct pci_epf *pci_epf_create(const char *name)
epf->name = kstrdup(func_name, GFP_KERNEL);
if (!epf->name) {
ret = -ENOMEM;
- goto free_func_name;
+ goto free_epf;
}
dev = &epf->dev;
@@ -255,8 +249,6 @@ struct pci_epf *pci_epf_create(const char *name)
put_dev:
put_device(dev);
kfree(epf->name);
-
-free_func_name:
kfree(func_name);
free_epf:
^ permalink raw reply related
* Re: [PATCH v2 1/2] clk: imx7d: fix USDHC NAND clock
From: Dong Aisheng @ 2017-04-11 19:59 UTC (permalink / raw)
To: Stefan Agner
Cc: shawnguo-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ, aisheng.dong-3arQi8VN3Tc,
fabio.estevam-3arQi8VN3Tc, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170410210015.1620-1-stefan-XLVq0VzYD2Y@public.gmane.org>
On Mon, Apr 10, 2017 at 02:00:14PM -0700, Stefan Agner wrote:
> The USDHC NAND root clock is not gated by any CCM clock gate. Remove
> the bogus gate definition.
>
> Signed-off-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
Acked-by: Dong Aisheng <aisheng.dong-3arQi8VN3Tc@public.gmane.org>
Regards
Dong Aisheng
> ---
> drivers/clk/imx/clk-imx7d.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
> index 562055129ed8..93b03640da9b 100644
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -724,7 +724,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
> clks[IMX7D_MAIN_AXI_ROOT_DIV] = imx_clk_divider2("axi_post_div", "axi_pre_div", base + 0x8800, 0, 6);
> clks[IMX7D_DISP_AXI_ROOT_DIV] = imx_clk_divider2("disp_axi_post_div", "disp_axi_pre_div", base + 0x8880, 0, 6);
> clks[IMX7D_ENET_AXI_ROOT_DIV] = imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base + 0x8900, 0, 6);
> - clks[IMX7D_NAND_USDHC_BUS_ROOT_DIV] = imx_clk_divider2("nand_usdhc_post_div", "nand_usdhc_pre_div", base + 0x8980, 0, 6);
> + clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] = imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base + 0x8980, 0, 6);
> clks[IMX7D_AHB_CHANNEL_ROOT_DIV] = imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk", "ahb_root_clk", base + 0x9080, 0, 2);
> clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div", "dram_cg", base + 0x9880, 0, 3);
> @@ -798,7 +798,6 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
> clks[IMX7D_ENET_AXI_ROOT_CLK] = imx_clk_gate4("enet_axi_root_clk", "enet_axi_post_div", base + 0x4060, 0);
> clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk", "axi_post_div", base + 0x4110, 0);
> clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk", "ahb_root_clk", base + 0x4120, 0);
> - clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] = imx_clk_gate4("nand_usdhc_root_clk", "nand_usdhc_post_div", base + 0x4130, 0);
> clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate4("dram_root_clk", "dram_post_div", base + 0x4130, 0);
> clks[IMX7D_DRAM_PHYM_ROOT_CLK] = imx_clk_gate4("dram_phym_root_clk", "dram_phym_cg", base + 0x4130, 0);
> clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] = imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base + 0x4130, 0);
> --
> 2.12.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" 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 v2 2/2] ARM: dts: imx7: add USDHC NAND and IPG clock to SDHC instances
From: Dong Aisheng @ 2017-04-11 20:00 UTC (permalink / raw)
To: Stefan Agner
Cc: shawnguo, kernel, sboyd, aisheng.dong, fabio.estevam, robh+dt,
mark.rutland, linux-arm-kernel, devicetree, linux-clk,
linux-kernel
In-Reply-To: <20170410210015.1620-2-stefan@agner.ch>
On Mon, Apr 10, 2017 at 02:00:15PM -0700, Stefan Agner wrote:
> The USDHC instances need the USDHC NAND and IPG clock in order to
> operate. Reference them properly by replacing the dummy clocks with
> the actual clocks.
>
> Note that both clocks are currently implicitly enabled since they
> are part of the i.MX 7 clock drivers init_on list. This might
> change in the future.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Acked-by: Dong Aisheng <aisheng.dong@nxp.com>
Regards
Dong Aisheng
> ---
> This patch depends on "clk: imx7d: add the missing ipg_root_clk"
> which adds the IPG clock.
>
> --
> Stefan
>
> arch/arm/boot/dts/imx7s.dtsi | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index c4f12fd2e044..843eb379e1ea 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -934,8 +934,8 @@
> compatible = "fsl,imx7d-usdhc", "fsl,imx6sl-usdhc";
> reg = <0x30b40000 0x10000>;
> interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>;
> - clocks = <&clks IMX7D_CLK_DUMMY>,
> - <&clks IMX7D_CLK_DUMMY>,
> + clocks = <&clks IMX7D_IPG_ROOT_CLK>,
> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> <&clks IMX7D_USDHC1_ROOT_CLK>;
> clock-names = "ipg", "ahb", "per";
> bus-width = <4>;
> @@ -946,8 +946,8 @@
> compatible = "fsl,imx7d-usdhc", "fsl,imx6sl-usdhc";
> reg = <0x30b50000 0x10000>;
> interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
> - clocks = <&clks IMX7D_CLK_DUMMY>,
> - <&clks IMX7D_CLK_DUMMY>,
> + clocks = <&clks IMX7D_IPG_ROOT_CLK>,
> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> <&clks IMX7D_USDHC2_ROOT_CLK>;
> clock-names = "ipg", "ahb", "per";
> bus-width = <4>;
> @@ -958,8 +958,8 @@
> compatible = "fsl,imx7d-usdhc", "fsl,imx6sl-usdhc";
> reg = <0x30b60000 0x10000>;
> interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> - clocks = <&clks IMX7D_CLK_DUMMY>,
> - <&clks IMX7D_CLK_DUMMY>,
> + clocks = <&clks IMX7D_IPG_ROOT_CLK>,
> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> <&clks IMX7D_USDHC3_ROOT_CLK>;
> clock-names = "ipg", "ahb", "per";
> bus-width = <4>;
> --
> 2.12.1
>
^ permalink raw reply
* Re: [PATCH 06/16] regulator: madera-ldo1: LDO1 driver for Cirrus Logic Madera codecs
From: Mark Brown @ 2017-04-11 20:03 UTC (permalink / raw)
To: Rob Herring
Cc: Richard Fitzgerald, Linus Walleij, Alexandre Courbot,
Thomas Gleixner, Jason Cooper, Lee Jones, Linux-ALSA,
patches-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqLYi8txm2xb5emGvbC0P2cvtW2wXLdA=2qCO-wt_4JXXA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 691 bytes --]
On Tue, Apr 11, 2017 at 02:20:31PM -0500, Rob Herring wrote:
> On Mon, Apr 10, 2017 at 1:11 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > In what way? Other than the -gpios stuff (which would just be a trivial
> > thing if we wanted to change it)?
> That's mainly it.
I don't see that as meaningful or a good reason to duplicate the driver
or the binding, it'd be better to just add the new property and
deprecate the old.
> It's not clear which node wlf,ldoena goes in either.
> In the parent? ldo1?
Parent since it's looked up in device context, this is also something
that it seems better to fix in one binding rather than duplicate.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] leds: cpcap: new driver
From: Pavel Machek @ 2017-04-11 20:19 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Tony Lindgren, Richard Purdie, Jacek Anaszewski, Rob Herring,
Mark Rutland, linux-leds-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170307171926.wscaogsrdmxc2opz@earth>
[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]
Hi!
> On Mon, Mar 06, 2017 at 11:11:47PM +0100, Pavel Machek wrote:
> > > Motorola CPCAP is a PMIC found in multiple smartphones.
> > > This driver adds support for the chip's LED controllers.
> > > It has explicit support for all controllers used by the
> > > Droid 4. Since no datasheets are available the other
> > > available controllers are not supported until somebody
> > > verified, that the register layout matches.
> >
> > This of course leads me to two questions:
> >
> > 1) Where can I get Droid 4?
>
> I got a used one on Ebay for 42€ incl. shipping & customs. The trick
> is clicking the worldwide option, since they are pretty expensive in
> EU (they only exist with US LTE modem, so they were not sold here
> officially).
>
> > 2) How well is it supported?
>
> UART + WLAN works with mainline master branch. As written by Tony
> we have a couple of patches ready for 4.12. Big open tasks are the
> cameras and the modems. Cameras are handled via co-processor in the
> stock system (that's about all I know about them so far) and modems are
> connected via USB + GPIOs (and for the 2G/3G modem an additional UART).
> LTE modem support seems simple (USB-CDC based), but does not work
> in EU and 2G/3G looks like much work. Modem voice support will be
> simpler than on N900, though (data goes directly to the audio codec).
> Speaking about audio codec: I'm currently working on that.
Thanks for all the information. It looks like a nice and powerful
machine, but I'm looking for a phone, first. (I'd actually like
something smaller than n900, preferably 3 times smaller... but... as
long as it calls it will have to do :-) ).
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: Document STM32 I2S bindings
From: Mark Brown @ 2017-04-11 20:19 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree@vger.kernel.org, Linux-ALSA,
olivier moysan, Alexandre Torgue, Arnaud POULIQUEN, Takashi Iwai,
Liam Girdwood, Maxime Coquelin,
linux-arm-kernel@lists.infradead.org, Benjamin Gaignard
In-Reply-To: <CAL_Jsq+bsh8Byow4poT7cdpLY_skb7QxbzkAmj4q7Cf6Z88GWA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1535 bytes --]
On Tue, Apr 11, 2017 at 11:02:57AM -0500, Rob Herring wrote:
> On Tue, Apr 11, 2017 at 9:32 AM, Mark Brown <broonie@kernel.org> wrote:
> > The graph card is blocked on your review...
> Maybe if there were more reviewers it would move faster. I don't know
> ASoC that well.
The times I've looked at it recently it's been stuck in DT style issues
rather than anything substantially ASoC related, as far as I can tell
the binding is essentially empty from an ASoC point of view and
inherited from the of_graph binding. There's bits in that are a bit
random like specifically listing the CPU DAIs and only them but that
just looks like one of these random DT things that's predetermined. I
really can't see anything at all in there to review from an ASoC point
of view, I've applied the changes that don't seem blocked on the
binding. If there's something you're looking for then please say...
We never seem to make any progress on the generic changes in drivers/of at
the start of the series either...
> > I'm also not clear how
> > without something like this we'd be able to identify a specific DAI
> > within a device if we don't have a way of identifying them.
> Isn't that what the graph does? "dais" points to a list of ports which
> are the specific DAIs whether there are multiple ones in a single
> device or multiple devices with a single DAI each.
But the ports can still have indexes AFAICT (the examples show port@0,
port@1 and so on) so we still need to define what those indexes mean
which is what this is doing?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] leds: cpcap: new driver
From: Pavel Machek @ 2017-04-11 20:20 UTC (permalink / raw)
To: Tony Lindgren
Cc: Sebastian Reichel, Richard Purdie, Jacek Anaszewski, Rob Herring,
Mark Rutland, linux-leds, devicetree, linux-kernel
In-Reply-To: <20170307162640.GS20572@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]
On Tue 2017-03-07 08:26:40, Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [170307 03:57]:
> > 2 cores and more importantly 1GB of RAM. Nice. OTOH, I'm avoiding N950
> > due to its crazy "security" system and this one is even worse.
>
> Yeah this security crap sucks big time in general. It just slows
> down Linux development on usable devices making most of them throw
> away bricks that nobody bothers to work on.
>
> > Anyway, good luck, and thanks for the pointers. I will get Droid 4 if
> > it is easy. But I guess I'll stick to N900 for now. I want something
> > that can do voice calls.
>
> Yeah.. For a security free solution, let's hope pyra will be able to
> make voice calls too :)
>
> Meanwhile, using droid 4 with a lapdock as a low power laptop is
> probably few weeks away, which means I can use it :p
Turning phones into laptops, nice ;-). I think I'll keep the thinkpad
for a while, but yes, interesting machine.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] devicetree: Document the max31760 device binding.
From: Guenter Roeck @ 2017-04-11 20:27 UTC (permalink / raw)
To: John Muir
Cc: Rob Herring, Jean Delvare, Jonathan Corbet, Pawel Moll,
Ian Campbell, Kumar Gala, devicetree, linux-hwmon, linux-doc,
Anatol Pomazau, Mark Segal
In-Reply-To: <996F9836-F468-4FB5-A16E-2E1FAE8752AD@jmuir.com>
On Tue, Apr 11, 2017 at 06:47:28AM -0700, John Muir wrote:
> > On Apr 10, 2017, at 8:42 AM, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Apr 04, 2017 at 12:20:34PM -0700, John Muir wrote:
> >> +MAX31760 fan controller
> >> +-----------------------
> >> +
> >> +This device supports I2C only. Many properties of this device are configurable
> >> +thorugh the hwmon interface. See also Documentation/hwmon/max31760.
> >
> > I really think we need to describe the fans as separate nodes and
> > preferably with a common binding. This is the second fan controller
> > binding recently[1].
> >
> > Features of the "hwmon interface" are not relevant to the binding.
> > Bindings describe h/w.
>
> It seems to me that referring to the hwmon interface is only helpful. You are suggesting removing those sentences? If so, can I add a link to the data sheet?
>
Devicetree properties are supposed to be operating system independent.
Any mention of how access to the device is implemented on a given
operating system is out of scope for this document.
Guenter
> >
> >> +Optional node properties:
> >> +- maxim,fan1-enabled - 1 to enable, 0 to disable. Default: 1.
> >> +- maxim,fan2-enabled - 1 to enable, 0 to disable. Default: 1.
> >> +- maxim,fan1-label - String: Hwmon fan1_label.
> >> +- maxim,fan2-label - String: Hwmon fan2_label.
> >
> > Perhaps 2 fan sub nodes. reg for fan number, status for enabled, and
> > label for label.
>
> OK.
>
> Right now a fan’s number of pulses and the PWM frequency are configured using the hwmon sysfs interface (which defines standard controls for those), but as those are characteristics of the hardware, should they also be configured via the device tree binding?
>
> >> +- maxim,pwm-zero-fan-can-fail - 0: Fan failure detection disabled when PWM is
> >> + ramping to 0%.
> >> + 1: Fan failure detection enabled for all PWM
> >> + values.
> >> + Default: 0.
> >
> > All these can be boolean…
>
> OK. The only issue I see is when the default is ‘true’ in the device, but I’ll try to avoid that. Sometimes I wish that you could set a boolean to false in DTS files.
>
> >
> >> +- maxim,temp1-label - String: Hwmon temp1_label.
> >> +- maxim,temp2-label - String: Hwmon temp2_label.
> >> +- maxim,temp2-ideality - Set ideality factor for the remote temperature
> >> + sensor. Integer with range 0 to 63,
> >> + representing a multiplication factor of 0.9844
> >> + to 1.0489. Default: 24 (1.0080).
> >
> > No maxim,temp1-ideality?
> No - the device only lets you set the ideality of the ‘external' temperature sensor. I guess if there is an ideality for the internal temperature sensor, it would be hard-wired as a characteristic of the part that was used.
>
> > Not sure what to do with these, but perhaps
> > also as sub-nodes. Surely we have some bindings already for devices with
> > multiple temp sensors. Don't invent something custom here.
>
> I’ll look into it.
>
> What is the best way to distinguish between ‘fan’ and ‘temp’ sub-nodes? Do I require a ‘compatible’ string?
>
> Thanks!
>
> John.
>
>
^ permalink raw reply
* Re: [PATCH v5 04/10] drm/stm: Add STM32 LTDC driver
From: Eric Anholt @ 2017-04-11 20:45 UTC (permalink / raw)
To: Yannick Fertre, Alexandre TORGUE, Thierry Reding, David Airlie,
Maxime Coquelin, Russell King, Mark Rutland, Rob Herring,
Arnd Bergmann, Benjamin Gaignard
Cc: devicetree, kernel, Philippe Cornu, dri-devel, Fabien Dessenne,
Mickael Reulier, Vincent Abriou, Gabriel FERNANDEZ,
linux-arm-kernel
In-Reply-To: <1490694293-18358-5-git-send-email-yannick.fertre@st.com>
[-- Attachment #1.1: Type: text/plain, Size: 8894 bytes --]
Yannick Fertre <yannick.fertre@st.com> writes:
> This controller provides output signals to interface directly a variety
> of LCD and TFT panels. These output signals are: RGB signals
> (up to 24bpp), vertical & horizontal synchronisations, data enable and
> the pixel clock.
>
> Signed-off-by: Yannick Fertre <yannick.fertre@st.com>
> ---
> drivers/gpu/drm/Kconfig | 3 +-
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/stm/Kconfig | 16 +
> drivers/gpu/drm/stm/Makefile | 7 +
> drivers/gpu/drm/stm/drv.c | 221 ++++++++
> drivers/gpu/drm/stm/ltdc.c | 1210 ++++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/stm/ltdc.h | 40 ++
> 7 files changed, 1497 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/stm/Kconfig
> create mode 100644 drivers/gpu/drm/stm/Makefile
> create mode 100644 drivers/gpu/drm/stm/drv.c
> create mode 100644 drivers/gpu/drm/stm/ltdc.c
> create mode 100644 drivers/gpu/drm/stm/ltdc.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 78d7fc0..dd5762a 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -203,7 +203,6 @@ config DRM_VGEM
> as used by Mesa's software renderer for enhanced performance.
> If M is selected the module will be called vgem.
>
> -
Stray whitespace change.
With this removed, the driver is:
Reviewed-by: Eric Anholt <eric@anholt.net>
Apologies for the delay in the second review offered. The remainder of
my comments are little cleanups, all of which I think are optional and
fine to do after the code lands.
You should probably update MAINTAINERS for your new driver. If you'd
like to maintain this driver in the drm-misc small drivers collection
(https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html),
send a follow-up patch to the list to add the MAINTAINERS entry, and I
can get that and patches 1-4 merged. Once you have a few more patches
in, we can add you to the drm-misc committers crew so you can merge
directly after getting review.
I'll also take this moment to plug something: Please feel welcome to
review other people's driver patches on the list. You've built
something nice here, and probably learned a lot of lessons along the way
that you could share with others. (I just found out about
of_reset_control in reviewing your code, and I wish I had known about it
back when I was landing vc4!)
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> new file mode 100644
> index 0000000..922f021
> --- /dev/null
> +++ b/drivers/gpu/drm/stm/ltdc.c
> +static void ltdc_crtc_disable(struct drm_crtc *crtc)
> +{
> + struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> + struct drm_pending_vblank_event *event = crtc->state->event;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + if (!crtc->enabled) {
> + DRM_DEBUG_DRIVER("already disabled\n");
> + return;
> + }
I think this crtc->enabled is a given for the disable() being called.
> +
> + drm_crtc_vblank_off(crtc);
> +
> + /* disable LTDC */
> + reg_clear(ldev->regs, LTDC_GCR, GCR_LTDCEN);
> +
> + /* disable IRQ */
> + reg_clear(ldev->regs, LTDC_IER, IER_RRIE | IER_FUIE | IER_TERRIE);
> +
> + /* immediately commit disable of layers before switching off LTDC */
> + reg_set(ldev->regs, LTDC_SRCR, SRCR_IMR);
> +
> + if (event) {
> + crtc->state->event = NULL;
> +
> + spin_lock_irq(&crtc->dev->event_lock);
> + if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
> + drm_crtc_arm_vblank_event(crtc, event);
> + else
> + drm_crtc_send_vblank_event(crtc, event);
> + spin_unlock_irq(&crtc->dev->event_lock);
> + }
I believe that we're guaranteed that crtc->state->event is NULL in the
disable call, since your atomic_flush() already armed or sent the event
and NULLed out the pointer.
> +struct drm_connector *ltdc_rgb_connector_create(struct drm_device *ddev)
> +{
> + struct drm_connector *connector;
> + int err;
> +
> + connector = devm_kzalloc(ddev->dev, sizeof(*connector), GFP_KERNEL);
> + if (!connector) {
> + DRM_ERROR("Failed to allocate connector\n");
> + return NULL;
> + }
> +
> + connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> + err = drm_connector_init(ddev, connector, <dc_rgb_connector_funcs,
> + DRM_MODE_CONNECTOR_LVDS);
I think DRM_MODE_CONNECTOR_DPI (and _ENCODER_DPI) are slightly more
accurate descriptions, if I'm interpreting your pinmux setup right.
It's cosmetic, though.
> +static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
> +{
> + struct device *dev = ddev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *entity, *port = NULL;
> + struct drm_panel *panel = NULL;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + /*
> + * Parse ltdc node to get remote port and find RGB panel / HDMI slave
> + * If a dsi or a bridge (hdmi, lvds...) is connected to ltdc,
> + * a remote port & RGB panel will not be found.
> + */
> + for_each_endpoint_of_node(np, entity) {
> + if (!of_device_is_available(entity))
> + continue;
> +
> + port = of_graph_get_remote_port_parent(entity);
> + if (port) {
> + panel = of_drm_find_panel(port);
> + of_node_put(port);
> + if (panel) {
> + DRM_DEBUG_DRIVER("remote panel %s\n",
> + port->full_name);
> + } else {
> + DRM_DEBUG_DRIVER("panel missing\n");
> + of_node_put(entity);
> + }
> + }
> + }
Future work: You may find the new drm_of_find_panel_or_bridge() useful
to drop this loop.
> +
> + return panel;
> +}
> +
> +int ltdc_load(struct drm_device *ddev)
> +{
> + struct platform_device *pdev = to_platform_device(ddev->dev);
> + struct ltdc_device *ldev = ddev->dev_private;
> + struct device *dev = ddev->dev;
> + struct device_node *np = dev->of_node;
> + struct drm_encoder *encoder;
> + struct drm_connector *connector = NULL;
> + struct drm_crtc *crtc;
> + struct reset_control *rstc;
> + struct resource res;
> + int irq, ret, i;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + ldev->panel = ltdc_get_panel(ddev);
> + if (!ldev->panel)
> + return -EPROBE_DEFER;
> +
> + rstc = of_reset_control_get(np, NULL);
> +
> + mutex_init(&ldev->err_lock);
> +
> + ldev->pixel_clk = devm_clk_get(dev, "lcd");
> + if (IS_ERR(ldev->pixel_clk)) {
> + DRM_ERROR("Unable to get lcd clock\n");
> + return -ENODEV;
> + }
> +
> + if (clk_prepare_enable(ldev->pixel_clk)) {
> + DRM_ERROR("Unable to prepare pixel clock\n");
> + return -ENODEV;
> + }
Future work: You may want to move the pixel clock enable into the CRTC's
.enable() and disable in .disable(). It sounded in previous versions
like the HW uses that clock for all register accesses, so you'd need to
protect a couple of other places, but that should save power when the
device is off, right?
> +
> + if (of_address_to_resource(np, 0, &res)) {
> + DRM_ERROR("Unable to get resource\n");
> + return -ENODEV;
> + }
> +
> + ldev->regs = devm_ioremap_resource(dev, &res);
> + if (IS_ERR(ldev->regs)) {
> + DRM_ERROR("Unable to get ltdc registers\n");
> + return PTR_ERR(ldev->regs);
> + }
> +
> + for (i = 0; i < MAX_IRQ; i++) {
> + irq = platform_get_irq(pdev, i);
> + if (irq < 0)
> + continue;
> +
> + ret = devm_request_threaded_irq(dev, irq, ltdc_irq,
> + ltdc_irq_thread, IRQF_ONESHOT,
> + dev_name(dev), ddev);
> + if (ret) {
> + DRM_ERROR("Failed to register LTDC interrupt\n");
> + return ret;
> + }
> + }
> +
> + if (!IS_ERR(rstc))
> + reset_control_deassert(rstc);
> +
> + /* Disable interrupts */
> + reg_clear(ldev->regs, LTDC_IER,
> + IER_LIE | IER_RRIE | IER_FUIE | IER_TERRIE);
> +
> + ret = ltdc_get_caps(ddev);
> + if (ret) {
> + DRM_ERROR("hardware identifier (0x%08x) not supported!\n",
> + ldev->caps.hw_version);
> + return ret;
> + }
> +
> + DRM_INFO("ltdc hw version 0x%08x - ready\n", ldev->caps.hw_version);
> +
> + if (ltdc_create_encoders(ddev)) {
> + DRM_ERROR("Failed to create encoders\n");
> + return -EINVAL;
> + }
> +
> + if (ldev->panel) {
> + encoder = ltdc_rgb_encoder_find(ddev);
> + if (!encoder) {
> + DRM_ERROR("Failed to find RGB encoder\n");
> + ret = -EINVAL;
> + goto err;
> + }
Given that ltdc_create_encoders() only does work if ldev->panel, its
body could probably be moved in here and then ltdc_rgb_encoder_find
could be dropped.
> +
> + connector = ltdc_rgb_connector_create(ddev);
> + if (!connector) {
> + DRM_ERROR("Failed to create RGB connector\n");
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + ret = drm_mode_connector_attach_encoder(connector, encoder);
> + if (ret) {
> + DRM_ERROR("Failed to attach connector to encoder\n");
> + goto err;
> + }
> +
> + drm_panel_attach(ldev->panel, connector);
> + }
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v5 04/10] drm/stm: Add STM32 LTDC driver
From: Daniel Vetter @ 2017-04-11 20:51 UTC (permalink / raw)
To: Eric Anholt
Cc: Yannick Fertre, Alexandre TORGUE, Thierry Reding, David Airlie,
Maxime Coquelin, Russell King, Mark Rutland, Rob Herring,
Arnd Bergmann, Benjamin Gaignard,
devicetree-u79uwXL29TY76Z2rM5mHXA, kernel-F5mvAk5X5gdBDgjK7y7TUQ,
Philippe Cornu, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Fabien Dessenne, Mickael Reulier, Vincent Abriou,
Gabriel FERNANDEZ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <87wpaqeks2.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>
On Tue, Apr 11, 2017 at 01:45:01PM -0700, Eric Anholt wrote:
> Yannick Fertre <yannick.fertre-qxv4g6HH51o@public.gmane.org> writes:
> > +static void ltdc_crtc_disable(struct drm_crtc *crtc)
> > +{
> > + struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> > + struct drm_pending_vblank_event *event = crtc->state->event;
> > +
> > + DRM_DEBUG_DRIVER("\n");
> > +
> > + if (!crtc->enabled) {
> > + DRM_DEBUG_DRIVER("already disabled\n");
> > + return;
> > + }
>
> I think this crtc->enabled is a given for the disable() being called.
Yup, one design principle of atomic (compared to the legacy modeset
helpers) is to correctly keep track of hw state and not call a hook when
not needed. If you don't trust them, conver them to WARN_ON, but otherwise
best to remove.
But like Eric said, totally fine in a follow-up patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU
From: Heiko Stübner @ 2017-04-11 20:52 UTC (permalink / raw)
To: Guillaume Tucker
Cc: Neil Armstrong, Rob Herring, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Sjoerd Simons, Wookey,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, John Reitan,
Enric Balletbo i Serra,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <936a53ee-2ed8-11e4-a860-744256676af9-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
Hi Guillaume,
Am Dienstag, 11. April 2017, 18:40:37 CEST schrieb Guillaume Tucker:
> On 03/04/17 09:12, Neil Armstrong wrote:
> > On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
> >> +Optional:
> >> +
> >> +- clocks : Phandle to clock for the Mali Midgard device.
> >> +- clock-names : Shall be "clk_mali".
> >> +- mali-supply : Phandle to regulator for the Mali device. Refer to
> >> + Documentation/devicetree/bindings/regulator/regulator.txt for details.
> >> +- operating-points : Refer to
> >> Documentation/devicetree/bindings/power/opp.txt + for details.
> >
> > Please add :
> > * Must be one of the following:
> > "arm,mali-t820"
> >
> > * And, optionally, one of the vendor specific compatible:
> > "amlogic,meson-gxm-mali"
> >
> > with my Ack for the amlogic platform.
>
> It seems to me that as long as the GPU architecture hasn't been
> modified (I don't think I've ever encountered such a case) then
> it has to be a standard ARM Mali type regardless of the SoC
> vendor. So unless a Mali-T820 in the Amlogic S912 SoC is not the
> same as a T820 in a different SoC, please forgive me but I don't
> understand why a vendor compatible string is needed. My main
> concern is that it's going to be very hard to keep that list
> up-to-date with all existing Midgard SoC variants. If do we need
> to add vendor compatible strings to correctly describe the
> hardware then I'm happy to add the amlogic one in my patch v3; I
> would just like to understand why that's necessary.
SoC vendors in most cases hook ip blocks into their socs in different
and often strange ways. After all it's not some discrete ic you solder
onto a board, but instead a part of the soc itself.
So in most cases you will have some hooks outside the actual gpu iospace
that can be used to tune different things about how the gpu interacts with
the system. Which is probably also the reason the midgard kernel driver
has this ugly "platform" subdirectory for compile-time platform selection.
On my rk3288 for example we have [0] in the chromeos tree, that handles
the oddities of the midgard on the rk3288 used in a lot of Chromebooks.
There are soc-specific oddities of frequencies, frequency-scaling and
whatnot. And there are also more gpu-specific setting in syscon areas
of the soc (pmu and grf) that can also influence the gpus performance
and might need tweaking at some point.
That doesn't even take into account that there may even be differences
on how things are synthesized that we don't know about. See all the
variants of the dw_hdmi ip block (imx, rockchip, meson [more?]) .
So we really want to have the special compatibles in place, to be prepared
for the future per-soc oddities that always appear :-) .
Heiko
[0] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/drivers/gpu/arm/midgard/platform/rk/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: Document STM32 I2S bindings
From: Mark Brown @ 2017-04-11 21:10 UTC (permalink / raw)
To: Olivier MOYSAN
Cc: mark.rutland@arm.com, Rob Herring, alsa-devel@alsa-project.org,
Alexandre TORGUE, devicetree@vger.kernel.org, Arnaud POULIQUEN,
tiwai@suse.com, lgirdwood@gmail.com, mcoquelin.stm32@gmail.com,
linux-arm-kernel@lists.infradead.org, Benjamin GAIGNARD
In-Reply-To: <73caede5-ea6f-8b2b-2270-227f7ef31823@st.com>
[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]
On Tue, Apr 11, 2017 at 03:44:52PM +0000, Olivier MOYSAN wrote:
> However, regarding STM32 I2S driver, I'm wondering if selecting DAI
> is the best way to configure interface as tx, rx or fd.
Why do you even need to configure this?
> Maybe, it is more relevant to configure DAI according to DMA
> configuration from I2S node.
> This would moreover avoid to allocate 2 dmas channels when not
> necessary (tx or rx only).
> If you agree with this, I will implement this change in a v2.
That sounds wrong, I'd expect this wiring to be done statically as part
of the .dtsi for the SoC (or just grabbed as needed at runtime if
things are flexbile enough) rather than being a configuration thing done
per board... I had thought that this was configuration reflecting
different ways of taping out the IP with different feature sets, is that
not the case?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH V2] clk: hi6220: Add the hi655x's pmic clock
From: Daniel Lezcano @ 2017-04-11 21:19 UTC (permalink / raw)
To: Lee Jones
Cc: sboyd-sgV2jX0FEOL9JmXXK+q4OQ, mturquette-rdvid1DuHRBWk0Htik3J/w,
xuwei5-C8/M+/jPZTeaMJb+Lgu22Q, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-clk-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170411140613.jvhzsvrxa4evwxjh@dell>
On Tue, Apr 11, 2017 at 03:06:13PM +0100, Lee Jones wrote:
> On Sat, 08 Apr 2017, Daniel Lezcano wrote:
>
> > The hi655x multi function device is a PMIC providing regulators.
> >
> > The PMIC also provides a clock for the WiFi and the Bluetooth, let's implement
> > this clock in order to add it in the hi655x MFD and allow proper wireless
> > initialization.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >
> > Changelog:
> >
> > V2:
> > - Added COMPILE_TEST option, compiled on x86
> > - Removed useless parenthesis
> > - Used of_clk_hw_simple_get() instead of deref dance
> > - Do bailout if the clock-names is not specified
> > - Rollback on error
> > - Folded mfd line change and binding
>
> Why did you do that?
I thought as the V1 had comments you would have waited for the V2 and as it was
trivial enough, it could be folded and picked up via the clk tree via with your
acked-by.
I realize it was not a good idea.
Do you want to drop it from your tree or shall I resubmit a V3 without the mfd
change?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> 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
* [PATCH v2 0/2] Add Maxim Integrated MAX31760 fan controller driver.
From: John Muir @ 2017-04-11 21:32 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring,
Pawel Moll, Ian Campbell, Kumar Gala,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
Cc: John Muir, Anatol Pomazau, Mark Segal
In-Reply-To: <20170404192034.158901-1-john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>
Add a device driver for the MAX31760 I2C device with a hwmon interface and
a few open firmware device properties.
v2:
- Fixup device tree binding documentation.
- max32760: Fix OF code and add some comments.
John Muir (2):
hwmon: Add MAX31760 fan controller driver.
devicetree: Document the max31760 device binding.
.../devicetree/bindings/hwmon/max31760.txt | 72 +
Documentation/hwmon/max31760 | 41 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/max31760.c | 1461 ++++++++++++++++++++
5 files changed, 1585 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/max31760.txt
create mode 100644 Documentation/hwmon/max31760
create mode 100644 drivers/hwmon/max31760.c
--
2.12.2.715.g7642488e1d-goog
--
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
* [PATCH v2 1/2] hwmon: Add MAX31760 fan controller driver.
From: John Muir @ 2017-04-11 21:32 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring,
Pawel Moll, Ian Campbell, Kumar Gala,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
Cc: John Muir, Anatol Pomazau, Mark Segal
In-Reply-To: <20170411213218.138718-1-john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>
Add a driver for the Maxim Integrated MAX31760 Precision Fan
Speed Controller.
v2:
- Fixup open firmware code.
- Add a few comments.
Signed-off-by: John Muir <john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>
---
Documentation/hwmon/max31760 | 41 ++
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/max31760.c | 1461 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 1513 insertions(+)
create mode 100644 Documentation/hwmon/max31760
create mode 100644 drivers/hwmon/max31760.c
diff --git a/Documentation/hwmon/max31760 b/Documentation/hwmon/max31760
new file mode 100644
index 000000000000..6f53ac6112a5
--- /dev/null
+++ b/Documentation/hwmon/max31760
@@ -0,0 +1,41 @@
+Kernel driver max31760
+======================
+
+Supported chips:
+ * Maxim Integrated MAX31760
+ Prefix: 'max31760'
+ Addresses scanned: none
+ Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX31760.pdf
+
+Author:
+ John Muir <john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>
+
+Description
+-----------
+
+The MAX31760 integrates temperature sensing along with precision PWM fan
+control. Please read the datasheet referenced above for a comprehensive
+description of this device.
+
+This device driver's hwmon integration provides the common sysfs interfaces to
+manage two fans and two temperature sensors, and pwm controls for the fan speed.
+A temperature to pwm lookup table is exposed via a series of 'auto_point'
+configuration files. See Documentation/hwmon/sysfs-interface for more
+information.
+
+The following custom controls are defined (in the custom sub-directory):
+
+control - Accepts control commands:
+ "reset" - Execute a soft reset of the device.
+ "clearff" - Clear the fan fault.
+
+eeprom_read - Read from the EEPROM into registers.
+eeprom_write - Write register contents to the EEPROM.
+ Write "0" to these to read or write the entire register
+ contents. Write a bit-field as per the datasheet to write a
+ portion of the register contents.
+
+pwm1_fan_fault - PWM value in the range of 0 to 255 used when a fan is faulty.
+
+pwm1_ramp_rate - PWM increment per second when the PWM value is changed.
+ Accepted values are 8, 16, 32, or 255 (instantaneous).
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 337e9078df0a..83c9b46a3876 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -895,6 +895,16 @@ config SENSORS_MAX6697
This driver can also be built as a module. If so, the module
will be called max6697.
+config SENSORS_MAX31760
+ tristate "Maxim MAX31760 fan controller"
+ depends on I2C
+ help
+ If you say yes here you get support for the Maxim Integrated
+ MAX31760 Precision Fan-Speed Controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called max31760.
+
config SENSORS_MAX31790
tristate "Maxim MAX31790 sensor chip"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d2bdccc000e6..e0ab73e0caa6 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -120,6 +120,7 @@ obj-$(CONFIG_SENSORS_MAX6639) += max6639.o
obj-$(CONFIG_SENSORS_MAX6642) += max6642.o
obj-$(CONFIG_SENSORS_MAX6650) += max6650.o
obj-$(CONFIG_SENSORS_MAX6697) += max6697.o
+obj-$(CONFIG_SENSORS_MAX31760) += max31760.o
obj-$(CONFIG_SENSORS_MAX31790) += max31790.o
obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
diff --git a/drivers/hwmon/max31760.c b/drivers/hwmon/max31760.c
new file mode 100644
index 000000000000..f3de3526d802
--- /dev/null
+++ b/drivers/hwmon/max31760.c
@@ -0,0 +1,1461 @@
+/* Maxim Integrated MAX31760 Precision Fan-Speed Controller driver
+ *
+ * Copyright (C) 2017 Google, Inc.
+ * Author: muirj
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#define DRIVER_NAME "max31760"
+
+/*
+ * MAX31760 registers.
+ * Indentation helps identify how these constants apply:
+ * Register number.
+ * Per-register bit fields.
+ * Values for multi-bit fields.
+ */
+#define MAX31760_REG_CR1 0x00 /* Control Register 1 */
+#define MAX31760_CR1_TIS 0x01 /* Temperature Index Source */
+#define MAX31760_CR1_MTI 0x02 /* Maximum Temperature Index */
+#define MAX31760_CR1_PPS 0x04 /* PWM Polarity */
+#define MAX31760_CR1_DRV 0x18 /* PWM Frequency */
+#define MAX31760_DRV_33HZ 0x00
+#define MAX31760_DRV_150HZ 0x08
+#define MAX31760_DRV_1500HZ 0x10
+#define MAX31760_DRV_25KHZ MAX31760_CR1_DRV
+#define MAX31760_CR1_HYST 0x20 /* Lookup Table Hysteresis: 2C or 4C */
+#define MAX31760_CR1_POR 0x40 /* Software Power-On Reset */
+#define MAX31760_CR1_ALTMSK 0x80 /* Alert Mask */
+#define MAX31760_REG_CR2 0x01 /* Control Register 2 */
+#define MAX31760_CR2_DFC 0x01 /* Direct Fan Control */
+#define MAX31760_CR2_FSST 0x02 /* Fan Sense Signal Type */
+#define MAX31760_CR2_RDPS 0x04 /* RD Polarity Selection */
+#define MAX31760_CR2_FSEN 0x08 /* FS Input Enable */
+#define MAX31760_CR2_FFMODE 0x10 /* FF Functionality Selection */
+#define MAX31760_CR2_SPEN 0x20 /* Spin-up Enable */
+#define MAX31760_CR2_ALERTS 0x40 /* Alerts Functionality Selection */
+#define MAX31760_CR2_STBY 0x80 /* Standby Mode Enable */
+#define MAX31760_REG_CR3 0x02 /* Control Register 3 */
+#define MAX31760_CR3_TACH1E 0x01 /* Tachometer 1 Enable */
+#define MAX31760_CR3_TACH2E 0x02 /* Tachometer 2 Enable */
+#define MAX31760_CR3_PSEN 0x04 /* Pulse Stretch Enable */
+#define MAX31760_CR3_TACHFL 0x08 /* Fan Fail When 100% Duty Cycle Only */
+#define MAX31760_CR3_RAMP 0x30 /* PWM Duty-Cycle Ramp Rate */
+#define MAX31760_RAMP_SLOW 0x00
+#define MAX31760_RAMP_SMED 0x10
+#define MAX31760_RAMP_MEDF 0x20
+#define MAX31760_RAMP_FAST MAX31760_CR3_RAMP
+#define MAX31760_CR3_FF_0 0x40 /* 0 Duty-Cycle Fan-Fail Detection */
+#define MAX31760_CR3_CLR_FF 0x80 /* Clear Fan Fail */
+#define MAX31760_REG_FFDC 0x03 /* Fan Fault Duty Cycle */
+#define MAX31760_REG_MASK 0x04 /* Alert Mask Register */
+#define MAX31760_MASK_TACH1AM 0x01 /* TACH1 Alarm Mask */
+#define MAX31760_MASK_TACH2AM 0x02 /* TACH2 Alarm Mask */
+#define MAX31760_MASK_ROTAM 0x04 /* Remote Overtemperature Alarm Mask */
+#define MAX31760_MASK_RHAM 0x08 /* Remote High Temperature Alarm Mask */
+#define MAX31760_MASK_LOTAM 0x10 /* Local Overtemperature Alarm Mask */
+#define MAX31760_MASK_LHAM 0x20 /* Local High Temperature Alarm Mask */
+#define MAX31760_REG_IFR 0x05 /* Ideality Factor Register */
+#define MAX31760_IFR_MASK 0x3f /* Mask for value of the IFR */
+#define MAX31760_REG_RHSH 0x06 /* Remote High Set-point MSB */
+#define MAX31760_REG_RHSL 0x07 /* Remote High Set-point LSB */
+#define MAX31760_REG_LOTSH 0x08 /* Local Overtemperature Set-point MSB */
+#define MAX31760_REG_LOTSL 0x09 /* Local Overtemperature Set-point LSB */
+#define MAX31760_REG_ROTSH 0x0a /* Remote Overtemperature Set-point MSB */
+#define MAX31760_REG_ROTSL 0x0b /* Remote Overtemperature Set-point LSB */
+#define MAX31760_REG_LHSH 0x0c /* Local High Set-point MSB */
+#define MAX31760_REG_LHSL 0x0d /* Local High Set-point LSB */
+#define MAX31760_REG_TCTH 0x0e /* TACH Count Threshold Register, MSB */
+#define MAX31760_REG_TCTL 0x0f /* TACH Count Threshold Register, LSB */
+#define MAX31760_REG_USER 0x10 /* 8 bytes General Purpose User Memory */
+#define MAX31760_REG_USER0 0x10 /* Custom Control Register USER0 */
+#define MAX31760_USER0_PULSE1 0x07 /* Fan1 Pulses per revolution */
+#define MAX31760_USER0_PULSE2 0x38 /* Fan2 Pulses per revolution */
+#define MAX31760_REG_LUT 0x20 /* 48-Byte Lookup Table (LUT) */
+#define MAX31760_LUT_COUNT 48
+#define MAX31760_REG_PWMR 0x50 /* Direct Duty-Cycle Control Register */
+
+#define MAX31760_REG_PWMV 0x51 /* Current PWM Duty-Cycle Register */
+#define MAX31760_REG_TC1H 0x52 /* TACH1 Count Register, MSB */
+#define MAX31760_REG_TC1L 0x53 /* TACH1 Count Register, LSB */
+#define MAX31760_REG_TC2H 0x54 /* TACH2 Count Register, MSB */
+#define MAX31760_REG_TC2L 0x55 /* TACH2 Count Register, LSB */
+#define MAX31760_REG_RTH 0x56 /* Remote Temperature Reading Register, MSB */
+#define MAX31760_REG_RTL 0x57 /* Remote Temperature Reading Register, LSB */
+#define MAX31760_REG_LTH 0x58 /* Local Temperature Reading Register, MSB */
+#define MAX31760_REG_LTL 0x59 /* Local Temperature Reading Register, LSB */
+#define MAX31760_REG_SR 0x5a /* Status Register */
+#define MAX31760_SR_TACH1A 0x01 /* TACH1 Alarm */
+#define MAX31760_SR_TACH2A 0x02 /* TACH2 Alarm */
+#define MAX31760_SR_ROTA 0x04 /* Remote Overtemperature Alarm */
+#define MAX31760_SR_RHA 0x08 /* Remote High Temperature Alarm */
+#define MAX31760_SR_LOTA 0x10 /* Local Overtemperature Alarm */
+#define MAX31760_SR_LHA 0x20 /* Local High Temperature Alarm */
+#define MAX31760_SR_RDFA 0x40 /* Remote Diode Fault Alarm */
+#define MAX31760_SR_PC 0x80 /* Program Corrupt Bit */
+
+#define MAX31760_REG_EEX 0x5b /* Load EEPROM to RAM; Write RAM to EEPROM */
+#define MAX31760_EEX_LW 0x80 /* Load from or write to EEPROM */
+#define MAX31760_EEX_BLKS 0x1F /* Blocks to load/write */
+
+#define MAX31760_TEMP_MIN_MC -40000 /* Minimum Millicelcius */
+#define MAX31760_TEMP_MAX_MC 127875 /* Maximum Millicelcius */
+#define MAX31760_TEMP_HIGH_HYST 1000 /* 1C hysteresis on high temp alarms. */
+#define MAX31760_TEMP_OVER_HYST 10000 /* 10C hysteresis on over temp alarms. */
+#define MAX31760_LUT_HYST_CLEAR 2000 /* LUT hysteresis: bit clear. */
+#define MAX31760_LYT_HYST_THRESH 3000 /* LUT hysteresis: store threshold. */
+#define MAX31760_LUT_HYST_SET 4000 /* LUT hysteresis: bit set. */
+
+#define MAX31760_NUM_TEMPS 2 /* Number of temperature sensors. */
+#define MAX31760_NUM_FANS 2 /* Number of fans. */
+#define MAX31760_FAN_PULSES_DEF 2 /* Default number of fan pulses. */
+#define MAX31760_FAN_PULSES_MAX 8 /* Maximum number of fan pulses. */
+#define MAX31760_PWM_ENABLE_FULL 0 /* pwmX_enable: Set PWM at full power. */
+#define MAX31760_PWM_ENABLE_MANUAL 1 /* pwmX_enable: Set manual mode. */
+#define MAX31760_PWM_ENABLE_AUTO 2 /* pwmX_enable: Set automatic mode. */
+
+#define MAX31760_LUT_AUTO_ATTRS 3 /* Number of LUT auto-point attributes. */
+#define MAX31760_LUT_AUTO_ATTR_COUNT (MAX31760_LUT_COUNT * \
+ MAX31760_LUT_AUTO_ATTRS)
+#define MAX31760_LUT_NAME_SIZE 32 /* Fit: pwm1_auto_pointXX_temp_hyst\0. */
+
+/*
+ * struct max31760_dev_attr - for generated device attributes
+ * @sdattr: Sensor device attribute.
+ * @name: Name of this attribute.
+ */
+struct max31760_dev_attr {
+ struct sensor_device_attribute sdattr;
+ char name[MAX31760_LUT_NAME_SIZE];
+};
+
+/*
+ * struct max31760 - device data
+ * @regmap: Register map.
+ * @fan_pulses: Quick access to the number of fan tach pulses (per fan).
+ * @fan_label: Labels for the fans if provided in open firmware.
+ * @temp_label: Labels for the temperature sensors if provided in open firmware.
+ * @lut_dev_attrs:
+ * Device attributes for the temperature to PWM lookup table.
+ * @lut_attrs: Pointers to the struct attribute in each lut_dev_attr.
+ * @lut_group: Attribute group for the LUT attributes.
+ * @attr_groups:Sysfs attribute groups for this device.
+ */
+struct max31760 {
+ struct regmap *regmap;
+ int fan_pulses[MAX31760_NUM_FANS];
+ const char *fan_label[MAX31760_NUM_FANS];
+ const char *temp_label[MAX31760_NUM_TEMPS];
+ struct max31760_dev_attr lut_dev_attrs[MAX31760_LUT_AUTO_ATTR_COUNT];
+ struct attribute *lut_attrs[MAX31760_LUT_AUTO_ATTR_COUNT + 1];
+ struct attribute_group lut_group;
+ const struct attribute_group *attr_groups[4];
+};
+
+static bool max31760_readable_reg(struct device *dev, unsigned int reg)
+{
+ return reg != MAX31760_REG_EEX;
+}
+
+static bool max31760_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MAX31760_REG_PWMV ... MAX31760_REG_SR:
+ return false;
+ default:
+ return true;
+ }
+}
+
+static bool max31760_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MAX31760_REG_MASK:
+ case MAX31760_REG_PWMR ... MAX31760_REG_EEX:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_config max31760_regmap_config = {
+ /*
+ * Device has an EEPROM to store the register values, so don't define
+ * reg_defaults: read the current values from the hardware.
+ */
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MAX31760_REG_EEX,
+ .writeable_reg = max31760_writeable_reg,
+ .readable_reg = max31760_readable_reg,
+ .volatile_reg = max31760_volatile_reg,
+ .val_format_endian = REGMAP_ENDIAN_BIG,
+ .cache_type = REGCACHE_RBTREE,
+ .use_single_rw = true,
+};
+
+/* Convert 11-bit MAX31760 register value to milliCelsius */
+static inline int max31760_temp_reg_to_mC(s16 val)
+{
+ return (val & ~0x0f) * 1000 / 256;
+}
+
+/* Convert milliCelsius to left adjusted 11-bit MAX31760 register value */
+static inline u16 max31760_mC_to_temp_reg(int val)
+{
+ return (val * 256) / 1000;
+}
+
+/* Convert tachometer value to RPM. */
+static inline long max31760_rpm_from_tach(u16 tach_count, int pulses)
+{
+ return 60L * 100000L / (long)tach_count / (long)pulses;
+}
+
+/* Convert RPM to tachometer value. */
+static inline u16 max31760_tach_from_rpm(long rpm, int pulses)
+{
+ long tach = 60L * 100000L / rpm / (long)pulses;
+
+ if (tach < 0)
+ tach = 0;
+ else if (tach > (long)USHRT_MAX)
+ tach = USHRT_MAX;
+
+ return tach;
+}
+
+/*
+ * Read two subsequent registers into a 16-bit word, treating the first as the
+ * most significant byte.
+ */
+static int max31760_read_word(struct regmap *regmap, unsigned int regmsb,
+ u16 *word)
+{
+ int err;
+ unsigned int msb_val;
+ unsigned int lsb_val;
+
+ err = regmap_read(regmap, regmsb, &msb_val);
+ if (err < 0)
+ return err;
+ err = regmap_read(regmap, regmsb + 1, &lsb_val);
+ if (err < 0)
+ return err;
+
+ *word = ((msb_val << 8) & 0xff00) | (lsb_val & 0xff);
+ return 0;
+}
+
+/*
+ * Write a 16-bit word into two subsequent registers, treating the first as the
+ * most significant byte.
+ */
+static int max31760_write_word(struct regmap *regmap, unsigned int regmsb,
+ u16 word)
+{
+ int err;
+ unsigned int val;
+
+ val = (word >> 8) & 0xff;
+ err = regmap_write(regmap, regmsb, val);
+ if (err < 0)
+ return err;
+
+ val = word & 0xff;
+ return regmap_write(regmap, regmsb + 1, val);
+}
+
+/*
+ * Read an alarm which may be flagged in the status register, or masked in the
+ * alarm mask register. Reading from the status register will cause the bit in
+ * the mask register to be set.
+ */
+static int max31760_read_alarm(struct device *dev, unsigned int srflag,
+ unsigned int maskflag, long *val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int srval;
+ unsigned int maskval;
+ int err;
+
+ err = regmap_read(max31760->regmap, MAX31760_REG_SR, &srval);
+ if (err < 0)
+ return err;
+ err = regmap_read(max31760->regmap, MAX31760_REG_MASK, &maskval);
+ if (err < 0)
+ return err;
+
+ *val = !!((srval & srflag) | (maskval & maskflag));
+ return 0;
+}
+
+static int max31760_read_temp(struct device *dev, u32 attr, int channel,
+ long *val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int reg;
+ unsigned int regval;
+ unsigned int srflag;
+ unsigned int maskflag;
+ u16 temp;
+ int err;
+ int hyst = 0;
+
+ switch (attr) {
+ case hwmon_temp_emergency_hyst:
+ hyst = MAX31760_TEMP_OVER_HYST;
+ /* fallthrough */
+ case hwmon_temp_max_hyst:
+ if (attr == hwmon_temp_max_hyst)
+ hyst = MAX31760_TEMP_HIGH_HYST;
+ /* fallthrough */
+ case hwmon_temp_input:
+ case hwmon_temp_max:
+ case hwmon_temp_emergency:
+ switch (attr) {
+ case hwmon_temp_input:
+ reg = channel ? MAX31760_REG_RTH : MAX31760_REG_LTH;
+ break;
+ case hwmon_temp_max_hyst:
+ case hwmon_temp_max:
+ reg = channel ? MAX31760_REG_RHSH : MAX31760_REG_LHSH;
+ break;
+ case hwmon_temp_emergency_hyst:
+ case hwmon_temp_emergency:
+ reg = channel ? MAX31760_REG_ROTSH : MAX31760_REG_LOTSH;
+ break;
+ }
+ err = max31760_read_word(max31760->regmap, reg, &temp);
+ if (err < 0)
+ return err;
+ *val = max31760_temp_reg_to_mC(temp) - hyst;
+ break;
+ case hwmon_temp_max_alarm:
+ case hwmon_temp_emergency_alarm:
+ switch (attr) {
+ case hwmon_temp_max_alarm:
+ srflag = channel ? MAX31760_SR_RHA : MAX31760_SR_LHA;
+ maskflag = channel ? MAX31760_MASK_RHAM :
+ MAX31760_MASK_LHAM;
+ break;
+ case hwmon_temp_emergency_alarm:
+ srflag = channel ? MAX31760_SR_ROTA : MAX31760_SR_LOTA;
+ maskflag = channel ? MAX31760_MASK_RHAM :
+ MAX31760_MASK_LHAM;
+ break;
+ }
+ return max31760_read_alarm(dev, srflag, maskflag, val);
+ case hwmon_temp_fault:
+ err = regmap_read(max31760->regmap, MAX31760_REG_SR, ®val);
+ if (err < 0)
+ return err;
+ *val = !!(regval & MAX31760_SR_RDFA);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int max31760_read_fan(struct device *dev, u32 attr, int channel,
+ long *val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ u16 tach_count;
+ unsigned int reg;
+ unsigned int regval;
+ unsigned int srflag;
+ unsigned int maskflag;
+ int err;
+
+ switch (attr) {
+ case hwmon_fan_input:
+ case hwmon_fan_min:
+ switch (attr) {
+ case hwmon_fan_input:
+ reg = channel ? MAX31760_REG_TC2H : MAX31760_REG_TC1H;
+ break;
+ case hwmon_fan_min:
+ reg = MAX31760_REG_TCTH;
+ break;
+ }
+ err = max31760_read_word(max31760->regmap, reg, &tach_count);
+ if (err)
+ return err;
+ *val = max31760_rpm_from_tach(tach_count,
+ max31760->fan_pulses[channel]);
+ break;
+ case hwmon_fan_fault:
+ /* TODO: Read FF/FS GPIO input when available. */
+ /* fallthrough */
+ case hwmon_fan_min_alarm:
+ srflag = channel ? MAX31760_SR_TACH2A : MAX31760_SR_TACH1A;
+ maskflag = channel ? MAX31760_MASK_TACH2AM :
+ MAX31760_MASK_TACH1AM;
+ return max31760_read_alarm(dev, srflag, maskflag, val);
+ case hwmon_fan_pulses:
+ err = regmap_read(max31760->regmap, MAX31760_REG_USER0,
+ ®val);
+ if (err)
+ return err;
+ if (channel)
+ *val = (regval & MAX31760_USER0_PULSE2) >> 3;
+ else
+ *val = regval & MAX31760_USER0_PULSE1;
+ if (*val == 0)
+ *val = MAX31760_FAN_PULSES_DEF;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int max31760_read_pwm(struct device *dev, u32 attr, int channel,
+ long *val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ int err;
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ /* Note that this is the current value, not the value stored to
+ * the duty-cycle register.
+ */
+ err = regmap_read(max31760->regmap, MAX31760_REG_PWMV, ®val);
+ if (err)
+ return err;
+ *val = regval;
+ break;
+ case hwmon_pwm_enable:
+ err = regmap_read(max31760->regmap, MAX31760_REG_CR2, ®val);
+ if (err)
+ return err;
+ if (regval & MAX31760_CR2_DFC)
+ *val = MAX31760_PWM_ENABLE_MANUAL;
+ else
+ *val = MAX31760_PWM_ENABLE_AUTO;
+ break;
+ case hwmon_pwm_freq:
+ err = regmap_read(max31760->regmap, MAX31760_REG_CR1, ®val);
+ if (err)
+ return err;
+ switch (regval & MAX31760_CR1_DRV) {
+ case MAX31760_DRV_33HZ:
+ default:
+ *val = 33;
+ break;
+ case MAX31760_DRV_150HZ:
+ *val = 150;
+ break;
+ case MAX31760_DRV_1500HZ:
+ *val = 1500;
+ break;
+ case MAX31760_DRV_25KHZ:
+ *val = 25000;
+ break;
+ }
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int max31760_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ switch (type) {
+ case hwmon_temp:
+ return max31760_read_temp(dev, attr, channel, val);
+ case hwmon_fan:
+ return max31760_read_fan(dev, attr, channel, val);
+ case hwmon_pwm:
+ return max31760_read_pwm(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int max31760_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, const char **str)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_temp:
+ if (attr != hwmon_temp_label)
+ return -EOPNOTSUPP;
+ *str = max31760->temp_label[channel];
+ break;
+ case hwmon_fan:
+ if (attr != hwmon_fan_label)
+ return -EOPNOTSUPP;
+ *str = max31760->fan_label[channel];
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+/* Write a temperature to the two adjacent registers starting at regmsb. */
+static int max31760_write_temp_reg(struct regmap *regmap,
+ unsigned int regmsb, long temp)
+{
+ u16 word;
+
+ temp = clamp_val(temp, MAX31760_TEMP_MIN_MC, MAX31760_TEMP_MAX_MC);
+ word = max31760_mC_to_temp_reg(temp);
+
+ return max31760_write_word(regmap, regmsb, word);
+}
+
+static int max31760_write_temp(struct device *dev, u32 attr, int channel,
+ long val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_temp_max:
+ return max31760_write_temp_reg(max31760->regmap,
+ channel ? MAX31760_REG_RHSH :
+ MAX31760_REG_LHSH,
+ val);
+ case hwmon_temp_emergency:
+ return max31760_write_temp_reg(max31760->regmap,
+ channel ? MAX31760_REG_ROTSH :
+ MAX31760_REG_LOTSH,
+ val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+/* Update the quick access fan pulses value. */
+static void max31760_update_fan_pulses(struct max31760 *max31760, int channel,
+ int pulses)
+{
+ if (pulses > MAX31760_FAN_PULSES_MAX)
+ pulses = MAX31760_FAN_PULSES_MAX;
+ else if (pulses <= 0)
+ pulses = MAX31760_FAN_PULSES_DEF;
+ max31760->fan_pulses[channel] = pulses;
+}
+
+static int max31760_write_fan(struct device *dev, u32 attr, int channel,
+ long val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ unsigned int mask;
+ u16 tach;
+
+ switch (attr) {
+ case hwmon_fan_min:
+ tach = max31760_tach_from_rpm(val,
+ max31760->fan_pulses[channel]);
+ return max31760_write_word(max31760->regmap, MAX31760_REG_TCTH,
+ tach);
+ case hwmon_fan_pulses:
+ max31760_update_fan_pulses(max31760, channel, val);
+ regval = (unsigned int)max31760->fan_pulses[channel];
+ if (channel) {
+ regval <<= 3;
+ mask = MAX31760_USER0_PULSE2;
+ } else {
+ mask = MAX31760_USER0_PULSE1;
+ }
+ return regmap_update_bits(max31760->regmap, MAX31760_REG_USER0,
+ mask, regval);
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int max31760_write_pwm(struct device *dev, u32 attr, int channel,
+ long val)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ int err;
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ regval = (unsigned int)val & 0xff;
+ return regmap_write(max31760->regmap, MAX31760_REG_PWMR,
+ regval);
+ case hwmon_pwm_enable:
+ switch (val) {
+ case MAX31760_PWM_ENABLE_FULL:
+ err = regmap_write(max31760->regmap, MAX31760_REG_PWMR,
+ 0xff);
+ if (err)
+ return err;
+ /* fallthrough */
+ case MAX31760_PWM_ENABLE_MANUAL:
+ return regmap_update_bits(max31760->regmap,
+ MAX31760_REG_CR2,
+ MAX31760_CR2_DFC,
+ MAX31760_CR2_DFC);
+ default:
+ case MAX31760_PWM_ENABLE_AUTO:
+ return regmap_update_bits(max31760->regmap,
+ MAX31760_REG_CR2,
+ MAX31760_CR2_DFC, 0);
+ }
+ break;
+ case hwmon_pwm_freq:
+ if (val < 91)
+ regval = MAX31760_DRV_33HZ;
+ else if (val < 825)
+ regval = MAX31760_DRV_150HZ;
+ else if (val < 11000)
+ regval = MAX31760_DRV_1500HZ;
+ else
+ regval = MAX31760_DRV_25KHZ;
+ return regmap_update_bits(max31760->regmap, MAX31760_REG_CR1,
+ MAX31760_CR1_DRV, regval);
+ }
+ return 0;
+}
+
+static int max31760_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_temp:
+ return max31760_write_temp(dev, attr, channel, val);
+ case hwmon_fan:
+ return max31760_write_fan(dev, attr, channel, val);
+ case hwmon_pwm:
+ return max31760_write_pwm(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t max31760_is_visible(const void *dvrdata,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ struct max31760 *max31760 = (struct max31760 *)dvrdata;
+
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_input:
+ case hwmon_temp_max_hyst:
+ case hwmon_temp_max_alarm:
+ case hwmon_temp_emergency_hyst:
+ case hwmon_temp_emergency_alarm:
+ case hwmon_temp_fault:
+ return 0444;
+ case hwmon_temp_label:
+ if (max31760->temp_label[channel])
+ return 0444;
+ return 0;
+ case hwmon_temp_max:
+ case hwmon_temp_emergency:
+ return 0644;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ case hwmon_fan_fault:
+ case hwmon_fan_min_alarm:
+ return 0444;
+ case hwmon_fan_label:
+ if (max31760->fan_label[channel])
+ return 0444;
+ return 0;
+ case hwmon_fan_min:
+ case hwmon_fan_pulses:
+ return 0644;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ case hwmon_pwm_enable:
+ case hwmon_pwm_freq:
+ return 0644;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static u32 max31760_chip_config[] = {
+ HWMON_C_REGISTER_TZ,
+ 0
+};
+
+static const struct hwmon_channel_info max31760_chip = {
+ .type = hwmon_chip,
+ .config = max31760_chip_config,
+};
+
+static u32 max31760_temp_config[] = {
+ /*
+ * Local temperature sensor:
+ * Local high set point (LHS) -> MAX,
+ * Local over-temperature set point (LOTS) -> EMERGENCY
+ * There is no fault flag for this temperature sensor.
+ */
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MAX | HWMON_T_EMERGENCY |
+ HWMON_T_MAX_ALARM | HWMON_T_EMERGENCY_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_EMERGENCY_HYST,
+ /*
+ * Remote temperature sensor:
+ * Remote high set point (RHS) -> MAX,
+ * Remote over-temperature set point (ROTS) -> EMERGENCY
+ */
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MAX | HWMON_T_EMERGENCY |
+ HWMON_T_MAX_ALARM | HWMON_T_EMERGENCY_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_EMERGENCY_HYST | HWMON_T_FAULT,
+ 0
+};
+
+static const struct hwmon_channel_info max31760_temp = {
+ .type = hwmon_temp,
+ .config = max31760_temp_config,
+};
+
+static u32 max31760_fan_config[] = {
+ HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MIN_ALARM | HWMON_F_PULSES |
+ HWMON_F_FAULT | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MIN_ALARM | HWMON_F_PULSES |
+ HWMON_F_FAULT | HWMON_F_LABEL,
+ 0
+};
+
+static const struct hwmon_channel_info max31760_fan = {
+ .type = hwmon_fan,
+ .config = max31760_fan_config,
+};
+
+static u32 max31760_pwm_config[] = {
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE | HWMON_PWM_FREQ,
+ 0
+};
+
+static const struct hwmon_channel_info max31760_pwm = {
+ .type = hwmon_pwm,
+ .config = max31760_pwm_config,
+};
+
+static const struct hwmon_channel_info *max31760_info[] = {
+ &max31760_chip,
+ &max31760_temp,
+ &max31760_fan,
+ &max31760_pwm,
+ NULL
+};
+
+static const struct hwmon_ops max31760_hwmon_ops = {
+ .is_visible = max31760_is_visible,
+ .read = max31760_read,
+ .read_string = max31760_read_string,
+ .write = max31760_write,
+};
+
+static const struct hwmon_chip_info max31760_chip_info = {
+ .ops = &max31760_hwmon_ops,
+ .info = max31760_info,
+};
+
+/* Show which temperature sensors are used to drive the PWM lookup table. */
+static ssize_t max31760_pwm_auto_channels_temp_show(
+ struct device *dev, struct device_attribute *devattr, char *buf)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ int channels;
+ int err;
+
+ err = regmap_read(max31760->regmap, MAX31760_REG_CR1, ®val);
+ if (err < 0)
+ return err;
+
+ /*
+ * Auto channels is a bit-field. TIS bit clear: temp1 (local) is used
+ * for the LUT. TIS bit set: temp2 (remote) is used for the LUT.
+ * MTI bit set: maximum temp from both is used, TIS bit is ignored.
+ */
+ if (regval & MAX31760_CR1_MTI)
+ channels = 3;
+ else if (regval & MAX31760_CR1_TIS)
+ channels = 2;
+ else
+ channels = 1;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", channels);
+}
+
+/* Store which temperature sensors are used to drive the PWM lookup table. */
+static ssize_t max31760_pwm_auto_channels_temp_store(
+ struct device *dev, struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ unsigned int mask;
+ unsigned long channels;
+ int err;
+
+ err = kstrtoul(buf, 10, &channels);
+ if (err < 0)
+ return err;
+
+ switch (channels & 0x3) {
+ case 3:
+ mask = MAX31760_CR1_MTI;
+ regval = MAX31760_CR1_MTI;
+ break;
+ case 1:
+ mask = MAX31760_CR1_TIS | MAX31760_CR1_MTI;
+ regval = 0;
+ break;
+ default:
+ case 2:
+ mask = MAX31760_CR1_TIS | MAX31760_CR1_MTI;
+ regval = MAX31760_CR1_TIS;
+ break;
+ }
+
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR1, mask,
+ regval);
+ if (err < 0)
+ return err;
+
+ return count;
+}
+
+/* Show the PWM value at a lookup table index. */
+static ssize_t max31760_pwm_auto_point_pwm_show(
+ struct device *dev, struct device_attribute *devattr, char *buf)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ struct sensor_device_attribute *sensor_dev_attr =
+ to_sensor_dev_attr(devattr);
+ unsigned int reg = MAX31760_REG_LUT + sensor_dev_attr->index;
+ unsigned int regval;
+ int err;
+
+ err = regmap_read(max31760->regmap, reg, ®val);
+ if (err < 0)
+ return err;
+
+ return snprintf(buf, PAGE_SIZE, "%u\n", regval);
+}
+
+/* Store the PWM value at a lookup table index. */
+static ssize_t max31760_pwm_auto_point_pwm_store(
+ struct device *dev, struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ struct sensor_device_attribute *sensor_dev_attr =
+ to_sensor_dev_attr(devattr);
+ unsigned int reg = MAX31760_REG_LUT + sensor_dev_attr->index;
+ unsigned int regval;
+ unsigned long pwm;
+ int err;
+
+ err = kstrtoul(buf, 10, &pwm);
+ if (err < 0)
+ return err;
+ regval = pwm & 0xff;
+
+ err = regmap_write(max31760->regmap, reg, regval);
+ if (err < 0)
+ return err;
+
+ return count;
+}
+
+/* Returns the temperature for the given PWM lookup table index. */
+static int max31760_pwm_auto_point_temp(int index)
+{
+ if (index == 0)
+ return MAX31760_TEMP_MIN_MC;
+ else
+ return (16 + index * 2) * 1000;
+}
+
+/* Show the temperature for a PWM lookup table index. */
+static ssize_t max31760_pwm_auto_point_temp_show(
+ struct device *dev, struct device_attribute *devattr, char *buf)
+{
+ struct sensor_device_attribute *sensor_dev_attr =
+ to_sensor_dev_attr(devattr);
+ int temp = max31760_pwm_auto_point_temp(sensor_dev_attr->index);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+}
+
+/* Show the temperature hysteresis for a PWM lookup table index. */
+static ssize_t max31760_pwm_auto_point_temp_hyst_show(
+ struct device *dev, struct device_attribute *devattr, char *buf)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ struct sensor_device_attribute *sensor_dev_attr =
+ to_sensor_dev_attr(devattr);
+ int temp = max31760_pwm_auto_point_temp(sensor_dev_attr->index);
+ unsigned int regval;
+ int err;
+
+ err = regmap_read(max31760->regmap, MAX31760_REG_CR1, ®val);
+ if (err < 0)
+ return err;
+
+ if (regval & MAX31760_CR1_HYST)
+ temp -= MAX31760_LUT_HYST_SET;
+ else
+ temp -= MAX31760_LUT_HYST_CLEAR;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+}
+
+/* Store the temperature hysteresis for a PWM lookup table index. */
+static ssize_t max31760_pwm_auto_point_temp_hyst_store(
+ struct device *dev, struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ struct sensor_device_attribute *sensor_dev_attr =
+ to_sensor_dev_attr(devattr);
+ int temp = max31760_pwm_auto_point_temp(sensor_dev_attr->index);
+ unsigned int regval;
+ long hyst;
+ int err;
+
+ err = kstrtol(buf, 10, &hyst);
+ if (err < 0)
+ return err;
+
+ temp -= hyst;
+ if (temp >= MAX31760_LYT_HYST_THRESH)
+ regval = MAX31760_CR1_HYST;
+ else
+ regval = 0;
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR1,
+ MAX31760_CR1_HYST, regval);
+ if (err < 0)
+ return err;
+ return count;
+}
+
+static SENSOR_DEVICE_ATTR(pwm1_auto_channels_temp, 0644,
+ max31760_pwm_auto_channels_temp_show,
+ max31760_pwm_auto_channels_temp_store, 0);
+static struct attribute *max31760_attrs[] = {
+ &sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
+ NULL
+};
+static const struct attribute_group max31760_group = {
+ .attrs = max31760_attrs,
+};
+
+/* Writes to the 'control' attribute. */
+static ssize_t max31760_control_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ int err;
+
+ if (sysfs_streq(buf, "reset")) {
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR1,
+ MAX31760_CR1_POR, MAX31760_CR1_POR);
+ if (err < 0)
+ return err;
+ } else if (sysfs_streq(buf, "clearff")) {
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR3,
+ MAX31760_CR3_CLR_FF,
+ MAX31760_CR3_CLR_FF);
+ if (err < 0)
+ return err;
+ } else {
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+/* Writes to the 'eeprom_read' attribute. */
+static ssize_t max31760_eeprom_read_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned long sections;
+ unsigned int regval;
+ int err;
+
+ err = kstrtoul(buf, 10, §ions);
+ if (err < 0)
+ return err;
+
+ if (sections == 0)
+ regval = MAX31760_EEX_BLKS;
+ else
+ regval = sections & MAX31760_EEX_BLKS;
+ regval |= MAX31760_EEX_LW;
+
+ err = regmap_write(max31760->regmap, MAX31760_REG_EEX, regval);
+ if (err < 0)
+ return err;
+
+ return count;
+}
+
+/* Writes to the 'eeprom_write' attribute. */
+static ssize_t max31760_eeprom_write_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned long sections;
+ unsigned int regval;
+ int err;
+
+ err = kstrtoul(buf, 10, §ions);
+ if (err < 0)
+ return err;
+
+ if (sections == 0)
+ regval = MAX31760_EEX_BLKS;
+ else
+ regval = sections & MAX31760_EEX_BLKS;
+
+ err = regmap_write(max31760->regmap, MAX31760_REG_EEX, regval);
+ if (err < 0)
+ return err;
+
+ return count;
+}
+
+/* Reads from the 'pwmX_fan_fault' attribute. */
+static ssize_t max31760_pwm_fan_fault_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ int err;
+
+ err = regmap_read(max31760->regmap, MAX31760_REG_FFDC, ®val);
+ if (err < 0)
+ return err;
+
+ return snprintf(buf, PAGE_SIZE, "%u\n", regval);
+}
+
+/* Writes to the 'pwmX_fan_fault' attribute. */
+static ssize_t max31760_pwm_fan_fault_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ unsigned long val;
+ int err;
+
+ err = kstrtoul(buf, 10, &val);
+ if (err)
+ return err;
+ regval = val & 0xff;
+
+ err = regmap_write(max31760->regmap, MAX31760_REG_FFDC, regval);
+ if (err < 0)
+ return err;
+ return count;
+}
+
+/* Reads from the 'pwmX_ramp_rate' attribute. */
+static ssize_t max31760_pwm_ramp_rate_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ int err;
+
+ err = regmap_read(max31760->regmap, MAX31760_REG_CR3, ®val);
+ if (err < 0)
+ return err;
+ switch (regval & MAX31760_CR3_RAMP) {
+ case MAX31760_RAMP_SLOW:
+ regval = 8;
+ break;
+ case MAX31760_RAMP_SMED:
+ regval = 16;
+ break;
+ case MAX31760_RAMP_MEDF:
+ regval = 32;
+ break;
+ case MAX31760_RAMP_FAST:
+ regval = 255;
+ break;
+ }
+
+ return snprintf(buf, PAGE_SIZE, "%u\n", regval);
+}
+
+/* Writes to the 'pwmX_ramp_rate' attribute. */
+static ssize_t max31760_pwm_ramp_rate_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ unsigned int regval;
+ unsigned long val;
+ int err;
+
+ err = kstrtoul(buf, 10, &val);
+ if (err)
+ return err;
+ if (val <= 12)
+ regval = MAX31760_RAMP_SLOW;
+ else if (val <= 24)
+ regval = MAX31760_RAMP_SMED;
+ else if (val <= 143)
+ regval = MAX31760_RAMP_MEDF;
+ else
+ regval = MAX31760_RAMP_FAST;
+
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR3,
+ MAX31760_CR3_RAMP, regval);
+ if (err < 0)
+ return err;
+ return count;
+}
+
+static SENSOR_DEVICE_ATTR(control, 0200, NULL, max31760_control_store, 0);
+static SENSOR_DEVICE_ATTR(eeprom_read, 0200, NULL, max31760_eeprom_read_store,
+ 0);
+static SENSOR_DEVICE_ATTR(eeprom_write, 0200, NULL, max31760_eeprom_write_store,
+ 0);
+static SENSOR_DEVICE_ATTR(pwm1_fan_fault, 0644, max31760_pwm_fan_fault_show,
+ max31760_pwm_fan_fault_store, 0);
+static SENSOR_DEVICE_ATTR(pwm1_ramp_rate, 0644, max31760_pwm_ramp_rate_show,
+ max31760_pwm_ramp_rate_store, 0);
+
+static struct attribute *max31760_custom_attrs[] = {
+ &sensor_dev_attr_control.dev_attr.attr,
+ &sensor_dev_attr_eeprom_read.dev_attr.attr,
+ &sensor_dev_attr_eeprom_write.dev_attr.attr,
+ &sensor_dev_attr_pwm1_fan_fault.dev_attr.attr,
+ &sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
+ NULL
+};
+static const struct attribute_group max31760_custom_group = {
+ .name = "custom",
+ .attrs = max31760_custom_attrs,
+};
+
+/* Generate auto_point sensor attributes. */
+static void max31760_setup_attr_groups(struct device *dev)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ struct max31760_dev_attr *lut_dev_attr = max31760->lut_dev_attrs;
+ struct device_attribute *dev_attr;
+ int attr_index = 0;
+ int i;
+
+ for (i = 0; i < MAX31760_LUT_COUNT; i++, lut_dev_attr++) {
+ snprintf(lut_dev_attr->name, MAX31760_LUT_NAME_SIZE,
+ "pwm1_auto_point%02d_pwm", i);
+ lut_dev_attr->sdattr.index = i;
+ dev_attr = &lut_dev_attr->sdattr.dev_attr;
+ dev_attr->attr.name = lut_dev_attr->name;
+ dev_attr->attr.mode = 0644;
+ dev_attr->show = max31760_pwm_auto_point_pwm_show;
+ dev_attr->store = max31760_pwm_auto_point_pwm_store;
+ max31760->lut_attrs[attr_index++] =
+ &lut_dev_attr->sdattr.dev_attr.attr;
+ }
+
+ for (i = 0; i < MAX31760_LUT_COUNT; i++, lut_dev_attr++) {
+ snprintf(lut_dev_attr->name, MAX31760_LUT_NAME_SIZE,
+ "pwm1_auto_point%02d_temp", i);
+ lut_dev_attr->sdattr.index = i;
+ dev_attr = &lut_dev_attr->sdattr.dev_attr;
+ dev_attr->attr.name = lut_dev_attr->name;
+ dev_attr->attr.mode = 0444;
+ dev_attr->show = max31760_pwm_auto_point_temp_show;
+ max31760->lut_attrs[attr_index++] =
+ &lut_dev_attr->sdattr.dev_attr.attr;
+ }
+
+ for (i = 0; i < MAX31760_LUT_COUNT; i++, lut_dev_attr++) {
+ snprintf(lut_dev_attr->name, MAX31760_LUT_NAME_SIZE,
+ "pwm1_auto_point%02d_temp_hyst", i);
+ lut_dev_attr->sdattr.index = i;
+ dev_attr = &lut_dev_attr->sdattr.dev_attr;
+ dev_attr->attr.name = lut_dev_attr->name;
+ dev_attr->attr.mode = 0644;
+ dev_attr->show = max31760_pwm_auto_point_temp_hyst_show;
+ dev_attr->store = max31760_pwm_auto_point_temp_hyst_store;
+ max31760->lut_attrs[attr_index++] =
+ &lut_dev_attr->sdattr.dev_attr.attr;
+ }
+
+ max31760->lut_group.attrs = max31760->lut_attrs;
+ max31760->attr_groups[0] = &max31760->lut_group;
+ max31760->attr_groups[1] = &max31760_group;
+ max31760->attr_groups[2] = &max31760_custom_group;
+}
+
+/* Update internal storage for the current register values. */
+static int max31760_update_from_registers(struct device *dev)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ long val;
+ int i;
+ int err;
+
+ for (i = 0; i < MAX31760_NUM_FANS; i++) {
+ err = max31760_read_fan(dev, hwmon_fan_pulses, i, &val);
+ if (err)
+ return err;
+ max31760->fan_pulses[i] = val;
+ }
+
+ /* Clear standby bit in case it is set. */
+ return regmap_update_bits(max31760->regmap, MAX31760_REG_CR2,
+ MAX31760_CR2_STBY, 0);
+}
+
+/* Configure registers which have associated device properties. */
+static int max31760_of_init(struct device *dev)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ struct device_node *node;
+ const char *label;
+ int err;
+ bool fan_enabled[MAX31760_NUM_FANS] = {0};
+ u32 reg;
+ u32 val;
+
+ for_each_available_child_of_node(dev->of_node, node) {
+ err = of_property_read_u32(node, "reg", ®);
+ if (err) {
+ dev_err(dev, "invalid reg on sub-node: %s", node->name);
+ return err;
+ }
+
+ if (!strcmp(node->name, "fan")) {
+ if (reg >= MAX31760_NUM_FANS) {
+ dev_err(dev, "invalid reg on fan: %s",
+ node->name);
+ return -EINVAL;
+ }
+
+ err = of_property_read_string(node, "label", &label);
+ if (!err)
+ max31760->fan_label[reg] = label;
+
+ fan_enabled[reg] = of_device_is_available(node);
+ } else if (!strcmp(node->name, "temp")) {
+ if (reg >= MAX31760_NUM_TEMPS) {
+ dev_err(dev, "invalid reg on fan: %s",
+ node->name);
+ return -EINVAL;
+ }
+
+ err = of_property_read_string(node, "label", &label);
+ if (!err)
+ max31760->temp_label[reg] = label;
+
+ err = of_property_read_u32(node, "ideality", &val);
+ if (reg == 1 && !err) {
+ /* Only external temp sensor has ideality. */
+ err = regmap_write(max31760->regmap,
+ MAX31760_REG_IFR,
+ val & 0x3f);
+ if (err)
+ return err;
+ }
+ } else {
+ dev_err(dev, "invalid subnode with name: %s",
+ node->name);
+ continue;
+ }
+ }
+
+ val = 0;
+ if (of_property_read_bool(dev->of_node, "maxim,pwm-polarity-negative"))
+ val |= MAX31760_CR1_PPS;
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR1,
+ MAX31760_CR1_PPS, val);
+ if (err)
+ return err;
+
+ /* Put ALERT pin into comparator mode: interrupts aren't supported. */
+ val = MAX31760_CR2_ALERTS | MAX31760_CR2_FFMODE;
+ if (of_property_read_bool(dev->of_node, "maxim,fan-spin-up-enabled"))
+ val |= MAX31760_CR2_SPEN;
+ if (of_property_read_bool(dev->of_node, "maxim,fan-rd-signal"))
+ val |= MAX31760_CR2_FSST;
+ if (of_property_read_bool(dev->of_node, "maxim,fan-rd-polarity-high"))
+ val |= MAX31760_CR2_RDPS;
+ if (of_property_read_bool(dev->of_node, "maxim,fan-signal-enabled"))
+ val |= MAX31760_CR2_FSEN;
+ /*
+ * Firmware configuration parameters planned:
+ * maxim,fan-fail-interrupt -> remove MAX31760_CR2_FFMODE
+ * maxim,temp-alert-interrupt -> remove MAX31760_CR2_ALERTS
+ */
+
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR2,
+ MAX31760_CR2_SPEN | MAX31760_CR2_FSST |
+ MAX31760_CR2_RDPS | MAX31760_CR2_FSEN |
+ MAX31760_CR2_ALERTS | MAX31760_CR2_FFMODE,
+ val);
+ if (err)
+ return err;
+
+ val = (fan_enabled[0] ? MAX31760_CR3_TACH1E : 0) |
+ (fan_enabled[1] ? MAX31760_CR3_TACH2E : 0);
+ if (of_property_read_bool(dev->of_node, "maxim,fan-fail-full-only"))
+ val |= MAX31760_CR3_TACHFL;
+ if (of_property_read_bool(dev->of_node,
+ "maxim,pwm-pulse-stretch-enabled"))
+ val |= MAX31760_CR3_PSEN;
+ if (of_property_read_bool(dev->of_node, "maxim,pwm-zero-fan-can-fail"))
+ val |= MAX31760_CR3_FF_0;
+
+ return regmap_update_bits(max31760->regmap, MAX31760_REG_CR3,
+ MAX31760_CR3_TACH1E | MAX31760_CR3_TACH2E |
+ MAX31760_CR3_TACHFL | MAX31760_CR3_PSEN |
+ MAX31760_CR3_FF_0, val);
+}
+
+static int max31760_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct device *hwmon_dev;
+ struct max31760 *max31760;
+ int err;
+
+ max31760 = devm_kzalloc(dev, sizeof(*max31760), GFP_KERNEL);
+ if (!max31760)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, max31760);
+
+ max31760->regmap = devm_regmap_init_i2c(client,
+ &max31760_regmap_config);
+ if (IS_ERR(max31760->regmap)) {
+ err = PTR_ERR(max31760->regmap);
+ dev_err(dev, "regmap init failed: %d", err);
+ return err;
+ }
+
+ err = max31760_of_init(dev);
+ if (err) {
+ dev_err(dev, "failed to initialize from firmware: %d", err);
+ return err;
+ }
+
+ err = max31760_update_from_registers(dev);
+ if (err) {
+ dev_err(dev, "failed to update from registers: %d", err);
+ return err;
+ }
+
+ max31760_setup_attr_groups(dev);
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+ max31760,
+ &max31760_chip_info,
+ max31760->attr_groups);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static int __maybe_unused max31760_suspend(struct device *dev)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+
+ return regmap_update_bits(max31760->regmap, MAX31760_REG_CR2,
+ MAX31760_CR2_STBY, MAX31760_CR2_STBY);
+}
+
+static int __maybe_unused max31760_resume(struct device *dev)
+{
+ struct max31760 *max31760 = dev_get_drvdata(dev);
+ int err;
+
+ err = regmap_update_bits(max31760->regmap, MAX31760_REG_CR2,
+ MAX31760_CR2_STBY, 0);
+ if (err)
+ dev_err(dev, "Could not clear Standby bit: %d", err);
+ return err;
+}
+
+static SIMPLE_DEV_PM_OPS(max31760_dev_pm_ops, max31760_suspend,
+ max31760_resume);
+
+static const struct i2c_device_id max31760_i2c_ids[] = {
+ { "max31760", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max31760_i2c_ids);
+
+#ifdef CONFIG_OF
+static const struct of_device_id max31760_of_ids[] = {
+ { .compatible = "maxim,max31760", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, max31760_of_ids);
+#endif
+
+static struct i2c_driver max31760_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .pm = &max31760_dev_pm_ops,
+ .of_match_table = of_match_ptr(max31760_of_ids),
+ },
+ .probe = max31760_probe,
+ .id_table = max31760_i2c_ids,
+};
+
+module_i2c_driver(max31760_driver);
+
+MODULE_AUTHOR("John Muir <john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Maxim Integrated MAX31760 Precision Fan-Speed Controller Driver");
+MODULE_LICENSE("GPL");
--
2.12.2.715.g7642488e1d-goog
--
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 v2 2/2] devicetree: Document the max31760 device binding.
From: John Muir @ 2017-04-11 21:32 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring,
Pawel Moll, Ian Campbell, Kumar Gala,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
Cc: John Muir, Anatol Pomazau, Mark Segal
In-Reply-To: <20170411213218.138718-1-john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>
v2:
- Fixup based on comments.
Signed-off-by: John Muir <john-eXjPKP/gKhgAvxtiuMwx3w@public.gmane.org>
---
.../devicetree/bindings/hwmon/max31760.txt | 72 ++++++++++++++++++++++
1 file changed, 72 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/max31760.txt
diff --git a/Documentation/devicetree/bindings/hwmon/max31760.txt b/Documentation/devicetree/bindings/hwmon/max31760.txt
new file mode 100644
index 000000000000..760fdf0b55e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max31760.txt
@@ -0,0 +1,72 @@
+MAX31760 fan controller
+-----------------------
+
+This device supports I2C only. Fan sub-nodes must be defined in order to enable
+the fan tachometer input. See also the datasheet:
+https://datasheets.maximintegrated.com/en/ds/MAX31760.pdf
+
+Required node properties:
+ - compatible: "maxim,max31760"
+ - reg: The I2C address of the device. This is 0x50 - 0x57 depending on the
+ hardware configuration.
+ - #address-cells: Must be 1.
+ - #size-cells: Must be 0.
+
+Optional node properties:
+ - maxim,fan-fail-full-only: Boolean; Assert a fan failure only when the PWM is
+ at 100%.
+ - maxim,fan-rd-signal: Boolean; Fans provide a rotation detection (RD) signal
+ instead of generating square-wave pulses.
+ - maxim,fan-rd-polarity-high: Boolean; RD is high when the fan is running, not
+ low. Only relevant when fan-rd-signal is true.
+ - maxim,fan-signal-enabled: Boolean; Externally driving FF/FS low should force
+ PWM output to 100%.
+ - maxim,fan-spin-up-enabled: Boolean; For fan startup set the PWM to 100% until
+ tach is detected or two seconds have passed before reducing to the target
+ value.
+ - maxim,pwm-polarity-negative: Boolean; 100% PWM is when PWM is low, not high.
+ - maxim,pwm-pulse-stretch-enabled: Boolean; Enable PWM pulse stretching.
+ - maxim,pwm-zero-fan-can-fail: Boolean; Enable fan failure detection while
+ ramping to 0% PWM.
+
+Fan sub-nodes must be present in order to enable the fan.
+
+Required fan sub-node properties:
+ - reg: Fan address. Must be <0x00> or <0x01>.
+
+Optional fan sub-node properties:
+ - label: String; Assigned to the hwmon fanX_label property.
+
+Temperature sub-nodes are optional.
+
+Required temp sub-node properties:
+ - reg: Temperature sensor address. Must be <0x00> or <0x01>.
+
+Optional temp sub-node properties:
+ - label: String; Assigned to the hwmon tempX_label property.
+ - ideality: For temperature node with reg 1 only: Set ideality factor for the
+ remote temperature sensor. Integer with range 0 to 63, representing a
+ multiplication factor of 0.9844 to 1.0489. Default: 24 (1.0080).
+
+Example:
+ max31760@50 {
+ compatible = "maxim,max31760";
+ reg = <0x50>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ maxim,fan-spin-up-enabled;
+
+ fan@0 {
+ reg = <0x00>;
+ label = "Left";
+ };
+ fan@1 {
+ reg = <0x01>;
+ label = "Right";
+ };
+ temp@1 {
+ reg = <0x01>;
+ label = "CPU";
+ };
+ };
--
2.12.2.715.g7642488e1d-goog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers
From: Florian Fainelli @ 2017-04-11 22:18 UTC (permalink / raw)
To: netdev
Cc: davem, Andrew Lunn, Rob Herring, Frank Rowand,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, open list
In-Reply-To: <20170410214258.9409-1-f.fainelli@gmail.com>
On 04/10/2017 02:42 PM, Florian Fainelli wrote:
> A MDIO bus driver can set phy_mask to indicate which PHYs should be
> probed and which should not. Right now, of_mdiobus_register() always
> sets mdio->phy_mask to ~0 which means: don't probe anything yourself,
> and let the Device Tree scanning do it based on the availability of
> child nodes.
>
> When MDIO buses are stacked together (on purpose, as is done by DSA), we
> run into possible double probing which is, at best unnecessary, and at
> worse, can cause problems if that's not expected (e.g: during probe
> deferral).
>
> Fix this by remember the original mdio->phy_mask, and make sure that if
> it was set to all 0xF, we set it to zero internally in order not to
> influence how the child PHY/MDIO device registration is going to behave.
> When the original mdio->phy_mask is set to something non-zero, we honor
> this value and utilize it as a hint to register only the child nodes
> that we have both found, and indicated to be necessary.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Sending this as RFC because a quick look at the current tree makes
> me think we are fine, but I would appreciate some review/feedback
> before this gets merged.
To give some more background and rational for this change.
On a platform where we have a parent MDIO bus, backed by the
mdio-bcm-unimac.c driver, we also register a slave MII bus (through
net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
assignment of of_node. This slave MII bus is created in order to
intercept reads/writes to problematic addresses (e.g: that clashes with
another piece of hardware).
This means that the slave DSA MII bus inherits all child nodes from the
originating master MII bus. This also means that when the slave MII bus
is probed via of_mdiobus_register(), we probe the same devices twice:
once through the master, another time through the slave.
With this change, we avoid double probing, because when creating the
slave MDIO bus, we carefully set phy_mask to intentionally restrict the
child PHY/MDIO device's creation to the relevant devices.
>
> Thank you!
>
> drivers/of/of_mdio.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 0b2979816dbf..6bfbf00623cb 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -209,6 +209,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> {
> struct device_node *child;
> bool scanphys = false;
> + u32 orig_phy_mask;
> int addr, rc;
>
> /* Do not continue if the node is disabled */
> @@ -217,8 +218,15 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>
> /* Mask out all PHYs from auto probing. Instead the PHYs listed in
> * the device tree are populated after the bus has been registered */
> + orig_phy_mask = mdio->phy_mask;
> mdio->phy_mask = ~0;
>
> + /* If the original phy_mask was all 0xf, we make it zero here in order
> + * to get child Device Tree nodes to be probed successfully
> + */
> + if (orig_phy_mask == mdio->phy_mask)
> + orig_phy_mask = 0;
> +
> mdio->dev.of_node = np;
>
> /* Register the MDIO bus */
> @@ -234,6 +242,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> continue;
> }
>
> + /* Honor hints from the mdio bus */
> + if (orig_phy_mask & BIT(addr))
> + continue;
> +
> if (of_mdiobus_child_is_phy(child))
> of_mdiobus_register_phy(mdio, child, addr);
> else
>
--
Florian
^ permalink raw reply
* Re: [PATCH v3 7/7] media: platform: rcar_drif: Add DRIF support
From: Laurent Pinchart @ 2017-04-11 22:24 UTC (permalink / raw)
To: Ramesh Shanmugasundaram
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, crope-X3B1VOXEql0,
chris.paterson2-zM6kxYcvzFBBDgjK7y7TUQ,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1486479757-32128-8-git-send-email-ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
Hi Ramesh,
Thank you for the patch.
On Tuesday 07 Feb 2017 15:02:37 Ramesh Shanmugasundaram wrote:
> This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3 SoCs.
> The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
> device represents a channel and each channel can have one or two
> sub-channels respectively depending on the target board.
>
> DRIF supports only Rx functionality. It receives samples from a RF
> frontend tuner chip it is interfaced with. The combination of DRIF and the
> tuner device, which is registered as a sub-device, determines the receive
> sample rate and format.
>
> In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
> the tuner device, which can be provided by a third party vendor. DRIF acts
> as a slave device and the tuner device acts as a master transmitting the
> samples. The driver allows asynchronous binding of a tuner device that
> is registered as a v4l2 sub-device. The driver can learn about the tuner
> it is interfaced with based on port endpoint properties of the device in
> device tree. The V4L2 SDR device inherits the controls exposed by the
> tuner device.
>
> The device can also be configured to use either one or both of the data
> pins at runtime based on the master (tuner) configuration.
>
> Signed-off-by: Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
> ---
> drivers/media/platform/Kconfig | 25 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/rcar_drif.c | 1534 +++++++++++++++++++++++++++++++++
> 3 files changed, 1560 insertions(+)
> create mode 100644 drivers/media/platform/rcar_drif.c
[snip]
> diff --git a/drivers/media/platform/rcar_drif.c
> b/drivers/media/platform/rcar_drif.c new file mode 100644
> index 0000000..88950e3
> --- /dev/null
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -0,0 +1,1534 @@
[snip]
> +/*
> + * The R-Car DRIF is a receive only MSIOF like controller with an
> + * external master device driving the SCK. It receives data into a FIFO,
> + * then this driver uses the SYS-DMAC engine to move the data from
> + * the device to memory.
> + *
> + * Each DRIF channel DRIFx (as per datasheet) contains two internal
> + * channels DRIFx0 & DRIFx1 within itself with each having its own
> resources
> + * like module clk, register set, irq and dma. These internal channels
> share
> + * common CLK & SYNC from master. The two data pins D0 & D1 shall be
> + * considered to represent the two internal channels. This internal split
> + * is not visible to the master device.
> + *
> + * Depending on the master device, a DRIF channel can use
> + * (1) both internal channels (D0 & D1) to receive data in parallel (or)
> + * (2) one internal channel (D0 or D1) to receive data
> + *
> + * The primary design goal of this controller is to act as Digitial Radio
s/Digitial/Digital/
> + * Interface that receives digital samples from a tuner device. Hence the
> + * driver exposes the device as a V4L2 SDR device. In order to qualify as
> + * a V4L2 SDR device, it should possess tuner interface as mandated by the
> + * framework. This driver expects a tuner driver (sub-device) to bind
> + * asynchronously with this device and the combined drivers shall expose
> + * a V4L2 compliant SDR device. The DRIF driver is independent of the
> + * tuner vendor.
> + *
> + * The DRIF h/w can support I2S mode and Frame start synchronization pulse
> mode.
> + * This driver is tested for I2S mode only because of the availability of
> + * suitable master devices. Hence, not all configurable options of DRIF h/w
> + * like lsb/msb first, syncdl, dtdl etc. are exposed via DT and I2S
> defaults
> + * are used. These can be exposed later if needed after testing.
> + */
[snip]
> +#define to_rcar_drif_buf_pair(sdr, ch_num,
> idx) (sdr->ch[!(ch_num)]->buf[idx])
You should enclose both sdr and idx in parenthesis, as they can be
expressions.
> +
> +#define for_each_rcar_drif_channel(ch, ch_mask) \
> + for_each_set_bit(ch, ch_mask, RCAR_DRIF_MAX_CHANNEL)
> +
> +static const unsigned int num_hwbufs = 32;
Is there a specific reason to make this a static const instead of a #define ?
> +/* Debug */
> +static unsigned int debug;
> +module_param(debug, uint, 0644);
> +MODULE_PARM_DESC(debug, "activate debug info");
> +
> +#define rdrif_dbg(level, sdr, fmt, arg...) \
> + v4l2_dbg(level, debug, &sdr->v4l2_dev, fmt, ## arg)
> +
> +#define rdrif_err(sdr, fmt, arg...) \
> + dev_err(sdr->v4l2_dev.dev, fmt, ## arg)
> +
> +/* Stream formats */
> +struct rcar_drif_format {
> + u32 pixelformat;
> + u32 buffersize;
> + u32 wdlen;
> + u32 num_ch;
> +};
> +
> +/* Format descriptions for capture */
> +static const struct rcar_drif_format formats[] = {
> + {
> + .pixelformat = V4L2_SDR_FMT_PCU16BE,
> + .buffersize = RCAR_SDR_BUFFER_SIZE,
> + .wdlen = 16,
> + .num_ch = 2,
How about aligning the = as in the other lines ?
num_ch is always set to 2. Should we remove it for now, and add it back later
when we'll support single-channel formats ? I think we should avoid carrying
dead code.
> + },
> + {
> + .pixelformat = V4L2_SDR_FMT_PCU18BE,
> + .buffersize = RCAR_SDR_BUFFER_SIZE,
> + .wdlen = 18,
> + .num_ch = 2,
> + },
> + {
> + .pixelformat = V4L2_SDR_FMT_PCU20BE,
> + .buffersize = RCAR_SDR_BUFFER_SIZE,
> + .wdlen = 20,
> + .num_ch = 2,
> + },
> +};
> +
> +static const unsigned int NUM_FORMATS = ARRAY_SIZE(formats);
Same question here, can't this be a define ? I think I'd even avoid
NUM_FORMATS completely and use ARRAY_SIZE(formats) directly in the code, to
make the boundary check more explicit when iterating over the array.
> +
> +/* Buffer for a received frame from one or both internal channels */
> +struct rcar_drif_frame_buf {
> + /* Common v4l buffer stuff -- must be first */
> + struct vb2_v4l2_buffer vb;
> + struct list_head list;
> +};
> +
> +struct rcar_drif_async_subdev {
> + struct v4l2_subdev *sd;
> + struct v4l2_async_subdev asd;
> +};
> +
> +/* DMA buffer */
> +struct rcar_drif_hwbuf {
> + void *addr; /* CPU-side address */
> + unsigned int status; /* Buffer status flags */
> +};
> +
> +/* Internal channel */
> +struct rcar_drif {
> + struct rcar_drif_sdr *sdr; /* Group device */
> + struct platform_device *pdev; /* Channel's pdev */
> + void __iomem *base; /* Base register address */
> + resource_size_t start; /* I/O resource offset */
> + struct dma_chan *dmach; /* Reserved DMA channel */
> + struct clk *clkp; /* Module clock */
> + struct rcar_drif_hwbuf *buf[RCAR_DRIF_MAX_NUM_HWBUFS]; /* H/W bufs */
> + dma_addr_t dma_handle; /* Handle for all bufs */
> + unsigned int num; /* Channel number */
> + bool acting_sdr; /* Channel acting as SDR device */
> +};
> +
> +/* DRIF V4L2 SDR */
> +struct rcar_drif_sdr {
> + struct device *dev; /* Platform device */
> + struct video_device *vdev; /* V4L2 SDR device */
> + struct v4l2_device v4l2_dev; /* V4L2 device */
> +
> + /* Videobuf2 queue and queued buffers list */
> + struct vb2_queue vb_queue;
> + struct list_head queued_bufs;
> + spinlock_t queued_bufs_lock; /* Protects queued_bufs */
> +
> + struct mutex v4l2_mutex; /* To serialize ioctls */
> + struct mutex vb_queue_mutex; /* To serialize streaming ioctls */
> + struct v4l2_ctrl_handler ctrl_hdl; /* SDR control handler */
> + struct v4l2_async_notifier notifier; /* For subdev (tuner) */
> +
> + /* Current V4L2 SDR format array index */
> + unsigned int fmt_idx;
Instead of storing the index I would store a pointer to the corresponding
rcar_drif_format, looking up information about the current format will then be
easier.
> +
> + /* Device tree SYNC properties */
> + u32 mdr1;
> +
> + /* Internals */
> + struct rcar_drif *ch[RCAR_DRIF_MAX_CHANNEL]; /* DRIFx0,1 */
> + unsigned long hw_ch_mask; /* Enabled channels per DT */
> + unsigned long cur_ch_mask; /* Used channels for an SDR FMT */
> + u32 num_hw_ch; /* Num of DT enabled channels */
> + u32 num_cur_ch; /* Num of used channels */
> + u32 hwbuf_size; /* Each DMA buffer size */
> + u32 produced; /* Buffers produced by sdr dev */
> +};
> +
> +/* Allocate buffer context */
> +static int rcar_drif_alloc_bufctxt(struct rcar_drif_sdr *sdr)
> +{
> + struct rcar_drif_hwbuf *bufctx;
> + unsigned int i, idx;
> +
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + bufctx = kcalloc(num_hwbufs, sizeof(*bufctx), GFP_KERNEL);
How about embedding the buffer contexts in the rcar_drif structure instead of
just storing pointers there ? The rcar_drif_hwbuf structure is pretty small,
it won't make a big difference, and will simplify the code.
> + if (!bufctx)
> + return -ENOMEM;
> +
> + for (idx = 0; idx < num_hwbufs; idx++)
> + sdr->ch[i]->buf[idx] = bufctx + idx;
> + }
> + return 0;
> +}
[snip]
> +/* Release DMA channel */
> +static void rcar_drif_release_dmachannel(struct rcar_drif_sdr *sdr)
I would name the function rcar_drif_release_dma_channels as it handles all
channels. Same for rcar_drif_alloc_dma_channels.
> +{
> + unsigned int i;
> +
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask)
> + if (sdr->ch[i]->dmach) {
> + dma_release_channel(sdr->ch[i]->dmach);
> + sdr->ch[i]->dmach = NULL;
> + }
> +}
> +
> +/* Allocate DMA channel */
> +static int rcar_drif_alloc_dmachannel(struct rcar_drif_sdr *sdr)
> +{
> + struct dma_slave_config dma_cfg;
> + unsigned int i;
> + int ret = -ENODEV;
> +
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + struct rcar_drif *ch = sdr->ch[i];
> +
> + ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx");
> + if (!ch->dmach) {
> + rdrif_err(sdr, "ch%u: dma channel req failed\n", i);
> + goto dmach_error;
> + }
> +
> + /* Configure slave */
> + memset(&dma_cfg, 0, sizeof(dma_cfg));
> + dma_cfg.src_addr = (phys_addr_t)(ch->start +
RCAR_DRIF_SIRFDR);
> + dma_cfg.dst_addr = 0;
This isn't needed as you memset the whole structure to 0.
> + dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> + ret = dmaengine_slave_config(ch->dmach, &dma_cfg);
> + if (ret) {
> + rdrif_err(sdr, "ch%u: dma slave config failed\n", i);
> + goto dmach_error;
> + }
> + }
> + return 0;
> +
> +dmach_error:
> + rcar_drif_release_dmachannel(sdr);
> + return ret;
> +}
[snip]
> +/* Set MDR defaults */
> +static inline void rcar_drif_set_mdr1(struct rcar_drif_sdr *sdr)
> +{
> + unsigned int i;
> +
> + /* Set defaults for enabled internal channels */
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + /* Refer MSIOF section in manual for this register setting */
> + writel(RCAR_DRIF_SITMDR1_PCON,
> + sdr->ch[i]->base + RCAR_DRIF_SITMDR1);
I would create a rcar_drif_write(struct rcar_drif *ch, u32 offset, u32 data)
function, the code will become clearer. Same for the read operation.
> + /* Setup MDR1 value */
> + writel(sdr->mdr1, sdr->ch[i]->base + RCAR_DRIF_SIRMDR1);
> +
> + rdrif_dbg(2, sdr, "ch%u: mdr1 = 0x%08x",
> + i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR1));
Once you've debugged the driver I'm not sure those debugging statements are
still needed.
> + }
> +}
> +
> +/* Extract bitlen and wdcnt from given word length */
> +static int rcar_drif_convert_wdlen(struct rcar_drif_sdr *sdr,
> + u32 wdlen, u32 *bitlen, u32 *wdcnt)
> +{
> + unsigned int i, nr_wds;
> +
> + /* FIFO register size is 32 bits */
> + for (i = 0; i < 32; i++) {
> + nr_wds = wdlen % (32 - i);
> + if (nr_wds == 0) {
> + *bitlen = 32 - i;
> + *wdcnt = wdlen / *bitlen;
Can't you store the bitlen and wdcnt values in the rcar_drif_format structure
instead of recomputing them every time ?
> + break;
> + }
> + }
> +
> + /* Sanity check range */
> + if (i == 32 || !(*bitlen >= 8 && *bitlen <= 32) ||
> + !(*wdcnt >= 1 && *wdcnt <= 64)) {
> + rdrif_err(sdr, "invalid wdlen %u configured\n", wdlen);
> + return -EINVAL;
You shouldn't have invalid wdlen values in the driver. I would remove this
check as it makes error handling in the caller more complex.
> + }
> +
> + return 0;
> +}
> +
> +/* Set DRIF receive format */
> +static int rcar_drif_set_format(struct rcar_drif_sdr *sdr)
> +{
> + u32 bitlen, wdcnt, wdlen;
> + unsigned int i;
> + int ret = -EINVAL;
> +
> + wdlen = formats[sdr->fmt_idx].wdlen;
> + rdrif_dbg(2, sdr, "setfmt: idx %u, wdlen %u, num_ch %u\n",
> + sdr->fmt_idx, wdlen, formats[sdr->fmt_idx].num_ch);
> +
> + /* Sanity check */
> + if (formats[sdr->fmt_idx].num_ch > sdr->num_cur_ch) {
> + rdrif_err(sdr, "fmt idx %u current ch %u mismatch\n",
> + sdr->fmt_idx, sdr->num_cur_ch);
> + return ret;
This should never happen, it should be caught at set format time.
> + }
> +
> + /* Get bitlen & wdcnt from wdlen */
> + ret = rcar_drif_convert_wdlen(sdr, wdlen, &bitlen, &wdcnt);
> + if (ret)
> + return ret;
> +
> + /* Setup group, bitlen & wdcnt */
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + u32 mdr;
> +
> + /* Two groups */
> + mdr = RCAR_DRIF_MDR_GRPCNT(2) | RCAR_DRIF_MDR_BITLEN(bitlen) |
> + RCAR_DRIF_MDR_WDCNT(wdcnt);
> + writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR2);
> +
> + mdr = RCAR_DRIF_MDR_BITLEN(bitlen) |
RCAR_DRIF_MDR_WDCNT(wdcnt);
> + writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR3);
> +
> + rdrif_dbg(2, sdr, "ch%u: new mdr[2,3] = 0x%08x, 0x%08x\n",
> + i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR2),
> + readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR3));
> + }
> + return ret;
> +}
> +
> +/* Release DMA buffers */
> +static void rcar_drif_release_buf(struct rcar_drif_sdr *sdr)
> +{
> + unsigned int i;
> +
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + struct rcar_drif *ch = sdr->ch[i];
> +
> + /* First entry contains the dma buf ptr */
> + if (ch->buf[0] && ch->buf[0]->addr) {
> + dma_free_coherent(&ch->pdev->dev,
> + sdr->hwbuf_size * num_hwbufs,
> + ch->buf[0]->addr, ch->dma_handle);
> + ch->buf[0]->addr = NULL;
> + }
> + }
> +}
> +
> +/* Request DMA buffers */
> +static int rcar_drif_request_buf(struct rcar_drif_sdr *sdr)
> +{
> + int ret = -ENOMEM;
> + unsigned int i, j;
> + void *addr;
> +
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + struct rcar_drif *ch = sdr->ch[i];
> +
> + /* Allocate DMA buffers */
> + addr = dma_alloc_coherent(&ch->pdev->dev,
> + sdr->hwbuf_size * num_hwbufs,
> + &ch->dma_handle, GFP_KERNEL);
> + if (!addr) {
> + rdrif_err(sdr,
> + "ch%u: dma alloc failed. num_hwbufs %u size %u\n",
> + i, num_hwbufs, sdr->hwbuf_size);
> + goto alloc_error;
> + }
> +
> + /* Split the chunk and populate bufctxt */
> + for (j = 0; j < num_hwbufs; j++) {
> + ch->buf[j]->addr = addr + (j * sdr->hwbuf_size);
> + ch->buf[j]->status = 0;
> + }
> + }
> +
> + return 0;
> +
> +alloc_error:
> + return ret;
> +}
> +
> +/* Setup vb_queue minimum buffer requirements */
> +static int rcar_drif_queue_setup(struct vb2_queue *vq,
> + unsigned int *num_buffers, unsigned int *num_planes,
> + unsigned int sizes[], struct device *alloc_devs[])
> +{
> + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> +
> + /* Need at least 16 buffers */
> + if (vq->num_buffers + *num_buffers < 16)
> + *num_buffers = 16 - vq->num_buffers;
> +
> + *num_planes = 1;
> + sizes[0] = PAGE_ALIGN(formats[sdr->fmt_idx].buffersize);
> +
> + rdrif_dbg(2, sdr, "num_bufs %d sizes[0] %d\n", *num_buffers,
sizes[0]);
> + return 0;
> +}
> +
> +/* Enqueue buffer */
> +static void rcar_drif_buf_queue(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vb->vb2_queue);
> + struct rcar_drif_frame_buf *fbuf =
> + container_of(vbuf, struct rcar_drif_frame_buf, vb);
> + unsigned long flags;
> +
> + rdrif_dbg(2, sdr, "buf_queue idx %u\n", vb->index);
> + spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> + list_add_tail(&fbuf->list, &sdr->queued_bufs);
> + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> +}
> +
> +/* Get a frame buf from list */
> +static struct rcar_drif_frame_buf *
> +rcar_drif_get_fbuf(struct rcar_drif_sdr *sdr)
> +{
> + struct rcar_drif_frame_buf *fbuf;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> + fbuf = list_first_entry_or_null(&sdr->queued_bufs, struct
> + rcar_drif_frame_buf, list);
> + if (!fbuf) {
> + /*
> + * App is late in enqueing buffers. Samples lost & there will
> + * be a gap in sequence number when app recovers
> + */
> + rdrif_dbg(1, sdr, "\napp late: prod %u\n", sdr->produced);
> + sdr->produced++; /* Increment the produced count anyway */
> + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> + return NULL;
> + }
> + list_del(&fbuf->list);
> + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> +
> + return fbuf;
> +}
> +
> +static inline bool rcar_drif_buf_pairs_done(struct rcar_drif_hwbuf *buf1,
> + struct rcar_drif_hwbuf *buf2)
> +{
> + return (buf1->status & buf2->status & RCAR_DRIF_BUF_DONE);
> +}
> +
> +/* Channel DMA complete */
> +static void rcar_drif_channel_complete(struct rcar_drif *ch, u32 idx)
> +{
> + u32 str;
> +
> + ch->buf[idx]->status |= RCAR_DRIF_BUF_DONE;
> +
> + /* Check for DRIF errors */
> + str = readl(ch->base + RCAR_DRIF_SISTR);
> + if (unlikely(str & RCAR_DRIF_RFOVF)) {
> + /* Writing the same clears it */
> + writel(str, ch->base + RCAR_DRIF_SISTR);
> +
> + /* Overflow: some samples are lost */
> + ch->buf[idx]->status |= RCAR_DRIF_BUF_OVERFLOW;
> + }
> +}
> +
> +/* Deliver buffer to user */
> +static void rcar_drif_deliver_buf(struct rcar_drif *ch)
> +{
> + struct rcar_drif_sdr *sdr = ch->sdr;
> + u32 idx = sdr->produced % num_hwbufs;
> + struct rcar_drif_frame_buf *fbuf;
> + bool overflow = false;
> +
> + rcar_drif_channel_complete(ch, idx);
> +
> + if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL) {
> + struct rcar_drif_hwbuf *bufi, *bufq;
> +
> + if (ch->num) {
> + bufi = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> + bufq = ch->buf[idx];
> + } else {
> + bufi = ch->buf[idx];
> + bufq = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> + }
> +
> + /* Check if both DMA buffers are done */
> + if (!rcar_drif_buf_pairs_done(bufi, bufq))
> + return;
> +
> + /* Clear buf done status */
> + bufi->status &= ~RCAR_DRIF_BUF_DONE;
> + bufq->status &= ~RCAR_DRIF_BUF_DONE;
> +
> + /* Get fbuf */
> + fbuf = rcar_drif_get_fbuf(sdr);
> + if (!fbuf)
> + return;
> +
> + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> + bufi->addr, sdr->hwbuf_size);
> + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0) + sdr-
>hwbuf_size,
> + bufq->addr, sdr->hwbuf_size);
Ouch ! That's a high data rate memcpy that can be avoided. Why don't you DMA
directly to the vb2 buffers ? You will need to use videobuf2-dma-contig
instead of videobuf2-vmalloc, but apart from that there should be no issue.
> + if ((bufi->status | bufq->status) & RCAR_DRIF_BUF_OVERFLOW) {
> + overflow = true;
> + /* Clear the flag in status */
> + bufi->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> + bufq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> + }
> + } else {
> + struct rcar_drif_hwbuf *bufiq;
> +
> + /* Get fbuf */
> + fbuf = rcar_drif_get_fbuf(sdr);
> + if (!fbuf)
> + return;
> +
> + bufiq = ch->buf[idx];
> +
> + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> + bufiq->addr, sdr->hwbuf_size);
> +
> + if (bufiq->status & RCAR_DRIF_BUF_OVERFLOW) {
> + overflow = true;
> + /* Clear the flag in status */
> + bufiq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> + }
> + }
> +
> + rdrif_dbg(2, sdr, "ch%u: prod %u\n", ch->num, sdr->produced);
> +
> + fbuf->vb.field = V4L2_FIELD_NONE;
> + fbuf->vb.sequence = sdr->produced++;
> + fbuf->vb.vb2_buf.timestamp = ktime_get_ns();
> + vb2_set_plane_payload(&fbuf->vb.vb2_buf, 0,
> + formats[sdr->fmt_idx].buffersize);
> +
> + /* Set error state on overflow */
> + if (overflow)
> + vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> + else
> + vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_DONE);
Maybe
vb2_buffer_done(&fbuf->vb.vb2_buf,
overflow ? VB2_BUF_STATE_ERROR: VB2_BUF_STATE_DONE);
> +}
> +
> +/* DMA callback for each stage */
> +static void rcar_drif_dma_complete(void *dma_async_param)
> +{
> + struct rcar_drif *ch = dma_async_param;
> + struct rcar_drif_sdr *sdr = ch->sdr;
> +
> + mutex_lock(&sdr->vb_queue_mutex);
Isn't the complete callback potentially called in interrupt context ? I know
the rcar-dmac driver uses a threaded interrupt handler for that, but is that a
guarantee of the DMA engine API ?
> +
> + /* DMA can be terminated while the callback was waiting on lock */
> + if (!vb2_is_streaming(&sdr->vb_queue))
Can it ? The streaming flag is cleared after the stop_streaming operation is
called, which will terminate all DMA transfers synchronously.
> + goto stopped;
> +
> + rcar_drif_deliver_buf(ch);
> +stopped:
> + mutex_unlock(&sdr->vb_queue_mutex);
> +}
> +
> +static int rcar_drif_qbuf(struct rcar_drif *ch)
> +{
> + struct rcar_drif_sdr *sdr = ch->sdr;
> + dma_addr_t addr = ch->dma_handle;
> + struct dma_async_tx_descriptor *rxd;
> + dma_cookie_t cookie;
> + int ret = -EIO;
> +
> + /* Setup cyclic DMA with given buffers */
> + rxd = dmaengine_prep_dma_cyclic(ch->dmach, addr,
> + sdr->hwbuf_size * num_hwbufs,
> + sdr->hwbuf_size, DMA_DEV_TO_MEM,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!rxd) {
> + rdrif_err(sdr, "ch%u: prep dma cyclic failed\n", ch->num);
> + return ret;
> + }
> +
> + /* Submit descriptor */
> + rxd->callback = rcar_drif_dma_complete;
> + rxd->callback_param = ch;
> + cookie = dmaengine_submit(rxd);
> + if (dma_submit_error(cookie)) {
> + rdrif_err(sdr, "ch%u: dma submit failed\n", ch->num);
> + return ret;
> + }
> +
> + dma_async_issue_pending(ch->dmach);
> + return 0;
> +}
> +
> +/* Enable reception */
> +static int rcar_drif_enable_rx(struct rcar_drif_sdr *sdr)
> +{
> + unsigned int i;
> + u32 ctr;
> + int ret;
> +
> + /*
> + * When both internal channels are enabled, they can be synchronized
> + * only by the master
> + */
> +
> + /* Enable receive */
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> + ctr |= (RCAR_DRIF_SICTR_RX_RISING_EDGE |
> + RCAR_DRIF_SICTR_RX_EN);
> + writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> + }
> +
> + /* Check receive enabled */
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> + ctr, ctr & RCAR_DRIF_SICTR_RX_EN,
> + 2, 500000);
A 2µs sleep for a 500ms total timeout seems very low to me, that will stress
the CPU. Same comment for the other locations where you use
readl_poll_timeout.
How long does the channel typically take to get enabled ?
> + if (ret) {
> + rdrif_err(sdr, "ch%u: rx en failed. ctr 0x%08x\n",
> + i, readl(sdr->ch[i]->base +
RCAR_DRIF_SICTR));
> + break;
> + }
> + }
> + return ret;
> +}
> +
> +/* Disable reception */
> +static void rcar_drif_disable_rx(struct rcar_drif_sdr *sdr)
> +{
> + unsigned int i;
> + u32 ctr;
> + int ret;
> +
> + /* Disable receive */
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> + ctr &= ~RCAR_DRIF_SICTR_RX_EN;
> + writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> + }
> +
> + /* Check receive disabled */
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> + ctr, !(ctr & RCAR_DRIF_SICTR_RX_EN),
> + 2, 500000);
How long does the channel typically take to get disabled ?
> + if (ret)
> + dev_warn(&sdr->vdev->dev,
> + "ch%u: failed to disable rx. ctr 0x%08x\n",
> + i, readl(sdr->ch[i]->base + RCAR_DRIF_SICTR));
> + }
> +}
> +
> +/* Start channel */
> +static int rcar_drif_start_channel(struct rcar_drif *ch)
> +{
> + struct rcar_drif_sdr *sdr = ch->sdr;
> + u32 ctr, str;
> + int ret;
> +
> + /* Reset receive */
> + writel(RCAR_DRIF_SICTR_RESET, ch->base + RCAR_DRIF_SICTR);
> + ret = readl_poll_timeout(ch->base + RCAR_DRIF_SICTR,
> + ctr, !(ctr & RCAR_DRIF_SICTR_RESET),
The alignment is weird.
> + 2, 500000);
> + if (ret) {
> + rdrif_err(sdr, "ch%u: failed to reset rx. ctr 0x%08x\n",
> + ch->num, readl(ch->base + RCAR_DRIF_SICTR));
> + return ret;
> + }
> +
> + /* Queue buffers for DMA */
> + ret = rcar_drif_qbuf(ch);
> + if (ret)
> + return ret;
> +
> + /* Clear status register flags */
> + str = RCAR_DRIF_RFFUL | RCAR_DRIF_REOF | RCAR_DRIF_RFSERR |
> + RCAR_DRIF_RFUDF | RCAR_DRIF_RFOVF;
> + writel(str, ch->base + RCAR_DRIF_SISTR);
> +
> + /* Enable DMA receive interrupt */
> + writel(0x00009000, ch->base + RCAR_DRIF_SIIER);
> +
> + return ret;
> +}
> +
> +/* Start receive operation */
> +static int rcar_drif_start(struct rcar_drif_sdr *sdr)
> +{
> + unsigned int i;
> + int ret;
> +
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + ret = rcar_drif_start_channel(sdr->ch[i]);
> + if (ret)
> + goto start_error;
> + }
> +
> + sdr->produced = 0;
> + ret = rcar_drif_enable_rx(sdr);
> +start_error:
Don't you need to stop the channels that were successfully started if an error
occurs ?
> + return ret;
> +}
> +
> +/* Stop channel */
> +static void rcar_drif_stop_channel(struct rcar_drif *ch)
> +{
> + struct rcar_drif_sdr *sdr = ch->sdr;
> + int ret, retries = 3;
> +
> + /* Disable DMA receive interrupt */
> + writel(0x00000000, ch->base + RCAR_DRIF_SIIER);
> +
> + do {
> + /* Terminate all DMA transfers */
> + ret = dmaengine_terminate_sync(ch->dmach);
> + if (!ret)
> + break;
> + rdrif_dbg(2, sdr, "stop retry\n");
> + } while (--retries);
Why do you need to retry the terminate operation, why does it fail ?
> + WARN_ON(!retries);
> +}
[snip]
> +/* Start streaming */
> +static int rcar_drif_start_streaming(struct vb2_queue *vq, unsigned int
> count)
> +{
> + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> + unsigned int i, j;
> + int ret;
> +
> + mutex_lock(&sdr->v4l2_mutex);
I'm surprised, aren't the start_streaming and stop_streaming operations called
with the video device lock held already by the v4l2-ioctl layer ? I think they
should be, if they're not there's probably a bug somewhere.
> + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> + ret = clk_prepare_enable(sdr->ch[i]->clkp);
> + if (ret)
> + goto start_error;
> + }
> +
> + /* Set default MDRx settings */
> + rcar_drif_set_mdr1(sdr);
> +
> + /* Set new format */
> + ret = rcar_drif_set_format(sdr);
> + if (ret)
> + goto start_error;
> +
> + if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL)
> + sdr->hwbuf_size =
> + formats[sdr->fmt_idx].buffersize / RCAR_DRIF_MAX_CHANNEL;
> + else
> + sdr->hwbuf_size = formats[sdr->fmt_idx].buffersize;
> +
> + rdrif_dbg(1, sdr, "num_hwbufs %u, hwbuf_size %u\n",
> + num_hwbufs, sdr->hwbuf_size);
> +
> + /* Alloc DMA channel */
> + ret = rcar_drif_alloc_dmachannel(sdr);
> + if (ret)
> + goto start_error;
> +
> + /* Alloc buf context */
> + ret = rcar_drif_alloc_bufctxt(sdr);
> + if (ret)
> + goto start_error;
> +
> + /* Request buffers */
> + ret = rcar_drif_request_buf(sdr);
> + if (ret)
> + goto start_error;
> +
> + /* Start Rx */
> + ret = rcar_drif_start(sdr);
> + if (ret)
> + goto start_error;
> +
> + mutex_unlock(&sdr->v4l2_mutex);
> + rdrif_dbg(1, sdr, "started\n");
> + return ret;
> +
> +start_error:
As there's a single error label I would call this "error". Up to you.
> + rcar_drif_release_queued_bufs(sdr, VB2_BUF_STATE_QUEUED);
> + rcar_drif_release_buf(sdr);
> + rcar_drif_release_bufctxt(sdr);
> + rcar_drif_release_dmachannel(sdr);
> + for (j = 0; j < i; j++)
> + clk_disable_unprepare(sdr->ch[j]->clkp);
> +
> + mutex_unlock(&sdr->v4l2_mutex);
> + return ret;
> +}
[snip]
> +/* Vb2 ops */
> +static struct vb2_ops rcar_drif_vb2_ops = {
You can make this static const.
> + .queue_setup = rcar_drif_queue_setup,
> + .buf_queue = rcar_drif_buf_queue,
> + .start_streaming = rcar_drif_start_streaming,
> + .stop_streaming = rcar_drif_stop_streaming,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> +};
[snip]
> +static int rcar_drif_g_fmt_sdr_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct rcar_drif_sdr *sdr = video_drvdata(file);
> +
> + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
I believe the core ioctl handling code already does this for you. Same for the
other ioctl handlers in
> + return 0;
> +}
> +
> +static int rcar_drif_s_fmt_sdr_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct rcar_drif_sdr *sdr = video_drvdata(file);
> + struct vb2_queue *q = &sdr->vb_queue;
> + unsigned int i;
> +
> + if (vb2_is_busy(q))
> + return -EBUSY;
> +
> + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> + for (i = 0; i < NUM_FORMATS; i++) {
> + if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
The code would become more readable (at least in my opinion) if you just added
a break here, and moved the code below after the loop. In case the requested
format isn't found (i == NUM_FORMATS) you can then set i to 0 and proceed,
that will select the first available format as a default.
> + sdr->fmt_idx = i;
> + f->fmt.sdr.buffersize = formats[i].buffersize;
> +
> + /*
> + * If a format demands one channel only out of two
> + * enabled channels, pick the 0th channel.
> + */
> + if (formats[i].num_ch < sdr->num_hw_ch) {
> + sdr->cur_ch_mask = BIT(0);
> + sdr->num_cur_ch = formats[i].num_ch;
> + } else {
> + sdr->cur_ch_mask = sdr->hw_ch_mask;
> + sdr->num_cur_ch = sdr->num_hw_ch;
> + }
> +
> + rdrif_dbg(1, sdr, "cur: idx %u mask %lu num %u\n",
> + i, sdr->cur_ch_mask, sdr->num_cur_ch);
> + return 0;
> + }
> + }
> +
> + if (rcar_drif_set_default_format(sdr)) {
> + rdrif_err(sdr, "cannot set default format\n");
> + return -EINVAL;
> + }
> +
> + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> + return 0;
> +}
> +
> +static int rcar_drif_try_fmt_sdr_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct rcar_drif_sdr *sdr = video_drvdata(file);
> + unsigned int i;
> +
> + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> + for (i = 0; i < NUM_FORMATS; i++) {
> + if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
> + f->fmt.sdr.buffersize = formats[i].buffersize;
> + return 0;
> + }
> + }
> +
> + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
The result of the TRY_FMT ioctl should not depend on the currently configured
format. I would return a fixed format (for instance the first one in the
formats array) in the default case.
> + return 0;
> +}
> +
> +/* Tuner subdev ioctls */
> +static int rcar_drif_enum_freq_bands(struct file *file, void *priv,
> + struct v4l2_frequency_band *band)
> +{
> + struct rcar_drif_sdr *sdr = video_drvdata(file);
> + struct v4l2_subdev *sd;
> + int ret = 0;
> +
> + v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> + ret = v4l2_subdev_call(sd, tuner, enum_freq_bands, band);
This won't work as-is when you'll have multiple subdevs. As the driver only
supports a single connected subdev at the moment, I suggest storing a pointer
to that subdev in the rcar_drif_sdr structure, and calling operations on that
subdev explicitly instead of looping over all subdevs. The comment holds for
all other ioctls.
> + if (ret)
> + break;
> + }
> + return ret;
> +}
[snip]
> +static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct rcar_drif_sdr *sdr =
> + container_of(notifier, struct rcar_drif_sdr, notifier);
> +
> + /* Nothing to do at this point */
If there's nothing to do you can just leave the bound callback unimplemented,
it's optional.
> + rdrif_dbg(2, sdr, "bound asd: %s\n", asd->match.of.node->name);
> + return 0;
> +}
> +
> +/* Sub-device registered notification callback */
> +static int rcar_drif_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct rcar_drif_sdr *sdr =
> + container_of(notifier, struct rcar_drif_sdr, notifier);
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + sdr->v4l2_dev.ctrl_handler = &sdr->ctrl_hdl;
> +
> + ret = v4l2_device_register_subdev_nodes(&sdr->v4l2_dev);
> + if (ret) {
> + rdrif_err(sdr, "failed register subdev nodes ret %d\n", ret);
> + return ret;
> + }
Do you need to expose subdev nodes to userspace ? Can't everything be handled
from the V4L2 SDR node ?
> + v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> + ret = v4l2_ctrl_add_handler(sdr->v4l2_dev.ctrl_handler,
> + sd->ctrl_handler, NULL);
Shouldn't you undo this somewhere when unbinding the subdevs ?
> + if (ret) {
> + rdrif_err(sdr, "failed ctrl add hdlr ret %d\n", ret);
> + return ret;
> + }
> + }
> + rdrif_dbg(2, sdr, "notify complete\n");
> + return 0;
> +}
[snip]
> +/* Parse sub-devs (tuner) to find a matching device */
> +static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr,
> + struct device *dev)
> +{
> + struct v4l2_async_notifier *notifier = &sdr->notifier;
> + struct rcar_drif_async_subdev *rsd;
> + struct device_node *node;
> +
> + notifier->subdevs = devm_kzalloc(dev, sizeof(*notifier->subdevs),
> + GFP_KERNEL);
> + if (!notifier->subdevs)
> + return -ENOMEM;
> +
> + node = of_graph_get_next_endpoint(dev->of_node, NULL);
> + if (!node)
> + return 0;
> +
> + rsd = devm_kzalloc(dev, sizeof(*rsd), GFP_KERNEL);
> + if (!rsd) {
> + of_node_put(node);
If you move the allocation above of_graph_get_next_endpoint() you won't have
to call of_node_put() in the error path.
> + return -ENOMEM;
> + }
> +
> + notifier->subdevs[notifier->num_subdevs] = &rsd->asd;
> + rsd->asd.match.of.node = of_graph_get_remote_port_parent(node);
Aren't you missing an of_node_put() on the returned node ? Or does the async
framework take care of that ?
> + of_node_put(node);
> + if (!rsd->asd.match.of.node) {
> + dev_warn(dev, "bad remote port parent\n");
> + return -EINVAL;
> + }
> +
> + rsd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> + notifier->num_subdevs++;
> +
> + /* Get the endpoint properties */
> + rcar_drif_get_ep_properties(sdr, node);
> + return 0;
> +}
> +
> +/* Check if the given device is the primary bond */
> +static bool rcar_drif_primary_bond(struct platform_device *pdev)
> +{
> + if (of_find_property(pdev->dev.of_node, "renesas,primary-bond", NULL))
> + return true;
> +
> + return false;
How about
return of_property_read_bool(pdev->dev.of_node,
"renesas,primary-bond");
> +}
> +
> +/* Get the bonded platform dev if enabled */
> +static struct platform_device *rcar_drif_enabled_bond(struct
> platform_device *p)
> +{
> + struct device_node *np;
> +
> + np = of_parse_phandle(p->dev.of_node, "renesas,bonding", 0);
The function takes a reference to np, you need to call of_node_put() on it
(only if the returned pointer isn't NULL).
> + if (np && of_device_is_available(np))
> + return of_find_device_by_node(np);
of_find_device_by_node() takes a reference to the returned device, you need to
call device_put() on it when you don't need it anymore.
> + return NULL;
> +}
> +
> +/* Proble internal channel */
> +static int rcar_drif_channel_probe(struct platform_device *pdev)
> +{
> + struct rcar_drif *ch;
> + struct resource *res;
> + void __iomem *base;
> + struct clk *clkp;
Maybe s/clkp/clk/ ?
> + int ret;
> +
> + /* Peripheral clock */
> + clkp = devm_clk_get(&pdev->dev, "fck");
> + if (IS_ERR(clkp)) {
> + ret = PTR_ERR(clkp);
> + dev_err(&pdev->dev, "clk get failed (%d)\n", ret);
> + return ret;
> + }
Isn't the clock managed automatically by runtime PM ?
> + /* Register map */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base)) {
> + ret = PTR_ERR(base);
> + dev_err(&pdev->dev, "ioremap failed (%d)\n", ret);
> + return ret;
devm_ioremap_resource() already prints an error message, you can remove this
one.
> + }
> +
> + /* Reserve memory for enabled channel */
> + ch = devm_kzalloc(&pdev->dev, sizeof(*ch), GFP_KERNEL);
> + if (!ch) {
> + ret = PTR_ERR(ch);
> + dev_err(&pdev->dev, "failed alloc channel\n");
Memory allocation failures already print error messages, you can remove this
one.
> + return ret;
> + }
> + ch->pdev = pdev;
> + ch->clkp = clkp;
> + ch->base = base;
> + ch->start = res->start;
If you allocated the ch structure first you could set the fields directly
without a need for local variables.
> + platform_set_drvdata(pdev, ch);
> + return 0;
> +}
> +
> +static int rcar_drif_probe(struct platform_device *pdev)
> +{
> + struct rcar_drif *ch, *b_ch = NULL;
> + struct platform_device *b_pdev;
> + struct rcar_drif_sdr *sdr;
> + int ret;
> +
> + /* Probe internal channel */
> + ret = rcar_drif_channel_probe(pdev);
> + if (ret)
> + return ret;
I would have done it the other way around, inlining the
rcar_drif_channel_probe() function here as that's the common case, and moving
the V4L2 SDR device initialization code to a different function.
> + /* Check if both channels of the bond are enabled */
> + b_pdev = rcar_drif_enabled_bond(pdev);
> + if (b_pdev) {
> + /* Check if current channel acting as primary-bond */
> + if (!rcar_drif_primary_bond(pdev)) {
> + dev_notice(&pdev->dev, "probed\n");
> + return 0;
> + }
> +
> + /* Check if the other device is probed */
> + b_ch = platform_get_drvdata(b_pdev);
> + if (!b_ch) {
> + dev_info(&pdev->dev, "defer probe\n");
> + return -EPROBE_DEFER;
> + }
Isn't this all very racy ? What if the other channel's device is removed while
this one is probed ?
> + /* Set the other channel number */
> + b_ch->num = 1;
Reading data from the other channel's private structure is one thing, but
writing it makes me shiver :-S Could we make it so that 0 is the slave and 1
the master ? That way you would set ch->num = 1 instead of b_ch->num = 1,
keeping all modifications to the private structure local to the device being
probed.
> + }
> +
> + /* Channel acting as SDR instance */
> + ch = platform_get_drvdata(pdev);
> + ch->acting_sdr = true;
> +
> + /* Reserve memory for SDR structure */
> + sdr = devm_kzalloc(&pdev->dev, sizeof(*sdr), GFP_KERNEL);
> + if (!sdr) {
> + ret = PTR_ERR(sdr);
> + dev_err(&pdev->dev, "failed alloc drif context\n");
> + return ret;
> + }
> + sdr->dev = &pdev->dev;
> + sdr->hw_ch_mask = BIT(ch->num);
> +
> + /* Establish links between SDR and channel(s) */
> + ch->sdr = sdr;
> + sdr->ch[ch->num] = ch;
> + if (b_ch) {
> + sdr->ch[b_ch->num] = b_ch;
> + b_ch->sdr = sdr;
> + sdr->hw_ch_mask |= BIT(b_ch->num);
> + }
> + sdr->num_hw_ch = hweight_long(sdr->hw_ch_mask);
> +
> + /* Validate any supported format for enabled channels */
> + ret = rcar_drif_set_default_format(sdr);
> + if (ret) {
> + dev_err(sdr->dev, "failed to set default format\n");
> + return ret;
> + }
> +
> + /* Set defaults */
> + sdr->hwbuf_size = RCAR_DRIF_DEFAULT_HWBUF_SIZE;
> +
> + mutex_init(&sdr->v4l2_mutex);
> + mutex_init(&sdr->vb_queue_mutex);
> + spin_lock_init(&sdr->queued_bufs_lock);
> + INIT_LIST_HEAD(&sdr->queued_bufs);
> +
> + /* Init videobuf2 queue structure */
> + sdr->vb_queue.type = V4L2_BUF_TYPE_SDR_CAPTURE;
> + sdr->vb_queue.io_modes = VB2_READ | VB2_MMAP | VB2_DMABUF;
> + sdr->vb_queue.drv_priv = sdr;
> + sdr->vb_queue.buf_struct_size = sizeof(struct rcar_drif_frame_buf);
> + sdr->vb_queue.ops = &rcar_drif_vb2_ops;
> + sdr->vb_queue.mem_ops = &vb2_vmalloc_memops;
> + sdr->vb_queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +
> + /* Init videobuf2 queue */
> + ret = vb2_queue_init(&sdr->vb_queue);
> + if (ret) {
> + dev_err(sdr->dev, "could not initialize vb2 queue\n");
> + return ret;
> + }
> +
> + /* Register the v4l2_device */
> + ret = v4l2_device_register(&pdev->dev, &sdr->v4l2_dev);
> + if (ret) {
> + dev_err(sdr->dev, "failed v4l2_device_register (%d)\n", ret);
Maybe "failed to register V4L2 device" to make it a real sentence ? :-)
> + return ret;
> + }
> +
> + /*
> + * Parse subdevs after v4l2_device_register because if the subdev
> + * is already probed, bound and complete will be called immediately
> + */
> + ret = rcar_drif_parse_subdevs(sdr, &pdev->dev);
> + if (ret)
> + goto err_unreg_v4l2;
> +
> + sdr->notifier.bound = rcar_drif_notify_bound;
> + sdr->notifier.complete = rcar_drif_notify_complete;
> +
> + v4l2_ctrl_handler_init(&sdr->ctrl_hdl, 10);
Possibly a stupid question, why 10, if you don't create any control in this
driver ?
> + /* Register notifier */
> + ret = v4l2_async_notifier_register(&sdr->v4l2_dev, &sdr->notifier);
> + if (ret < 0) {
> + dev_err(sdr->dev, "notifier registration failed (%d)\n", ret);
> + goto err_free_ctrls;
> + }
> +
> + /* Init video_device structure */
> + sdr->vdev = video_device_alloc();
> + if (!sdr->vdev) {
> + ret = -ENOMEM;
> + goto err_unreg_notif;
> + }
> + snprintf(sdr->vdev->name, sizeof(sdr->vdev->name), "R-Car DRIF");
> + sdr->vdev->fops = &rcar_drif_fops;
> + sdr->vdev->ioctl_ops = &rcar_drif_ioctl_ops;
> + sdr->vdev->release = video_device_release;
> + sdr->vdev->lock = &sdr->v4l2_mutex;
> + sdr->vdev->queue = &sdr->vb_queue;
> + sdr->vdev->queue->lock = &sdr->vb_queue_mutex;
> + sdr->vdev->ctrl_handler = &sdr->ctrl_hdl;
> + sdr->vdev->v4l2_dev = &sdr->v4l2_dev;
> + sdr->vdev->device_caps = V4L2_CAP_SDR_CAPTURE | V4L2_CAP_TUNER |
> + V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
> + video_set_drvdata(sdr->vdev, sdr);
> +
> + /* Register V4L2 SDR device */
> + ret = video_register_device(sdr->vdev, VFL_TYPE_SDR, -1);
> + if (ret) {
> + dev_err(sdr->dev, "failed video_register_device (%d)\n", ret);
Same here, "failed to register video device" ?
> + goto err_unreg_notif;
> + }
> +
> + dev_notice(sdr->dev, "probed\n");
Do you think this message is really useful ? I believe it would just add a bit
more noise to the kernel log, without any real use.
> + return 0;
> +
> +err_unreg_notif:
> + video_device_release(sdr->vdev);
> + v4l2_async_notifier_unregister(&sdr->notifier);
> +err_free_ctrls:
> + v4l2_ctrl_handler_free(&sdr->ctrl_hdl);
> +err_unreg_v4l2:
> + v4l2_device_unregister(&sdr->v4l2_dev);
> + return ret;
> +}
> +
> +static int rcar_drif_remove(struct platform_device *pdev)
> +{
> + struct rcar_drif *ch = platform_get_drvdata(pdev);
> + struct rcar_drif_sdr *sdr = ch->sdr;
> +
> + if (!ch->acting_sdr) {
Isn't it possible to check the channel number instead and remove the
acting_sdr field ?
> + /* Nothing to do */
> + dev_notice(&pdev->dev, "removed\n");
> + return 0;
> + }
> +
> + /* SDR instance */
> + v4l2_ctrl_handler_free(sdr->vdev->ctrl_handler);
> + v4l2_async_notifier_unregister(&sdr->notifier);
> + v4l2_device_unregister(&sdr->v4l2_dev);
> + video_unregister_device(sdr->vdev);
> + dev_notice(&pdev->dev, "removed\n");
Even more than the probed message, I think this one can go away.
> + return 0;
> +}
> +
> +static int __maybe_unused rcar_drif_suspend(struct device *dev)
> +{
> + return 0;
Maybe a /* FIXME: Implement suspend/resume support */ ?
> +}
> +
> +static int __maybe_unused rcar_drif_resume(struct device *dev)
> +{
> + return 0;
Same here ?
> +}
[snip]
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 6/7] dt-bindings: media: Add Renesas R-Car DRIF binding
From: Laurent Pinchart @ 2017-04-11 22:35 UTC (permalink / raw)
To: Ramesh Shanmugasundaram
Cc: robh+dt, mark.rutland, mchehab, hverkuil, sakari.ailus, crope,
chris.paterson2, geert+renesas, linux-media, devicetree,
linux-renesas-soc
In-Reply-To: <1486479757-32128-7-git-send-email-ramesh.shanmugasundaram@bp.renesas.com>
Hi Ramesh,
Thank you for the patch.
On Tuesday 07 Feb 2017 15:02:36 Ramesh Shanmugasundaram wrote:
> Add binding documentation for Renesas R-Car Digital Radio Interface
> (DRIF) controller.
>
> Signed-off-by: Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@bp.renesas.com>
> ---
> .../devicetree/bindings/media/renesas,drif.txt | 186 ++++++++++++++++++
> 1 file changed, 186 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/renesas,drif.txt
>
> diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt
> b/Documentation/devicetree/bindings/media/renesas,drif.txt new file mode
> 100644
> index 0000000..6315609
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
> @@ -0,0 +1,186 @@
> +Renesas R-Car Gen3 Digital Radio Interface controller (DRIF)
> +------------------------------------------------------------
> +
> +R-Car Gen3 DRIF is a SPI like receive only slave device. A general
> +representation of DRIF interfacing with a master device is shown below.
> +
> ++---------------------+ +---------------------+
> +| |-----SCK------->|CLK |
> +| Master |-----SS-------->|SYNC DRIFn (slave) |
> +| |-----SD0------->|D0 |
> +| |-----SD1------->|D1 |
> ++---------------------+ +---------------------+
> +
> +As per the datasheet, each DRIF channel (drifn) is made up of two internal
> +channels (drifn0 & drifn1). These two internal channels share the common
> +CLK & SYNC. Each internal channel has its own dedicated resources like
> +irq, dma channels, address space & clock. This internal split is not
> +visible to the external master device.
> +
> +The device tree model represents each internal channel as a separate node.
> +The internal channels sharing the CLK & SYNC are tied together by their
> +phandles using a new property called "renesas,bonding". For the rest of
> +the documentation, unless explicitly stated, the word channel implies an
> +internal channel.
> +
> +When both internal channels are enabled they need to be managed together
> +as one (i.e.) they cannot operate alone as independent devices. Out of the
> +two, one of them needs to act as a primary device that accepts common
> +properties of both the internal channels. This channel is identified by a
> +new property called "renesas,primary-bond".
> +
> +To summarize,
> + - When both the internal channels that are bonded together are enabled,
> + the zeroth channel is selected as primary-bond. This channels accepts
> + properties common to all the members of the bond.
> + - When only one of the bonded channels need to be enabled, the property
> + "renesas,bonding" or "renesas,primary-bond" will have no effect. That
> + enabled channel can act alone as any other independent device.
> +
> +Required properties of an internal channel:
> +-------------------------------------------
> +- compatible: "renesas,r8a7795-drif" if DRIF controller is a part of
> R8A7795 SoC.
> + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible
> device.
> + When compatible with the generic version, nodes must list the
> + SoC-specific version corresponding to the platform first
> + followed by the generic version.
> +- reg: offset and length of that channel.
> +- interrupts: associated with that channel.
> +- clocks: phandle and clock specifier of that channel.
> +- clock-names: clock input name string: "fck".
> +- dmas: phandles to the DMA channels.
> +- dma-names: names of the DMA channel: "rx".
> +- renesas,bonding: phandle to the other channel.
> +
> +Optional properties of an internal channel:
> +-------------------------------------------
> +- power-domains: phandle to the respective power domain.
> +
> +Required properties of an internal channel when:
> + - It is the only enabled channel of the bond (or)
> + - If it acts as primary among enabled bonds
> +--------------------------------------------------------
> +- pinctrl-0: pin control group to be used for this channel.
> +- pinctrl-names: must be "default".
> +- renesas,primary-bond: empty property indicating the channel acts as
> primary
> + among the bonded channels.
> +- port: child port node of a channel that defines the local and remote
> + endpoints. The remote endpoint is assumed to be a third party tuner
> + device endpoint.
You should refer to the OF graphs bindings here. How about the following to
document the port node ?
- port: child port node corresponding to the data input, in accordance with
the video interface bindings defined in
Documentation/devicetree/bindings/media/video-interfaces.txt. The port node
must contain at least one endpoint.
> +Optional endpoint property:
> +---------------------------
> +- renesas,sync-active : Indicates sync signal polarity, 0/1 for low/high
> + respectively. This property maps to SYNCAC bit in the
> + hardware manual. The default is 1 (active high)
> +
> +Example
> +--------
> +
> +SoC common dtsi file
> +
> + drif00: rif@e6f40000 {
> + compatible = "renesas,r8a7795-drif",
> + "renesas,rcar-gen3-drif";
> + reg = <0 0xe6f40000 0 0x64>;
> + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg CPG_MOD 515>;
> + clock-names = "fck";
> + dmas = <&dmac1 0x20>, <&dmac2 0x20>;
> + dma-names = "rx", "rx";
> + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> + renesas,bonding = <&drif01>;
> + status = "disabled";
> + };
> +
> + drif01: rif@e6f50000 {
> + compatible = "renesas,r8a7795-drif",
> + "renesas,rcar-gen3-drif";
> + reg = <0 0xe6f50000 0 0x64>;
> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg CPG_MOD 514>;
> + clock-names = "fck";
> + dmas = <&dmac1 0x22>, <&dmac2 0x22>;
> + dma-names = "rx", "rx";
> + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> + renesas,bonding = <&drif00>;
> + status = "disabled";
> + };
> +
> +
> +Board specific dts file
> +
> +(1) Both internal channels enabled:
> +-----------------------------------
> +
> +When interfacing with a third party tuner device with two data pins as
> shown
> +below.
> +
> ++---------------------+ +---------------------+
> +| |-----SCK------->|CLK |
> +| Master |-----SS-------->|SYNC DRIFn (slave) |
> +| |-----SD0------->|D0 |
> +| |-----SD1------->|D1 |
> ++---------------------+ +---------------------+
> +
> +pfc {
> + ...
> +
> + drif0_pins: drif0 {
> + groups = "drif0_ctrl_a", "drif0_data0_a",
> + "drif0_data1_a";
> + function = "drif0";
> + };
> + ...
> +}
> +
> +&drif00 {
> + pinctrl-0 = <&drif0_pins>;
> + pinctrl-names = "default";
> + renesas,primary-bond;
> + status = "okay";
> + port {
> + drif0_ep: endpoint {
> + remote-endpoint = <&tuner_ep>;
> + };
> + };
> +};
> +
> +&drif01 {
> + status = "okay";
> +};
> +
> +(2) Internal channel 1 alone is enabled:
> +----------------------------------------
> +
> +When interfacing with a third party tuner device with one data pin as shown
> +below.
> +
> ++---------------------+ +---------------------+
> +| |-----SCK------->|CLK |
> +| Master |-----SS-------->|SYNC DRIFn (slave) |
> +| | |D0 (unused) |
> +| |-----SD-------->|D1 |
> ++---------------------+ +---------------------+
> +
> +pfc {
> + ...
> +
> + drif0_pins: drif0 {
> + groups = "drif0_ctrl_a", "drif0_data1_a";
> + function = "drif0";
> + };
> + ...
> +}
> +
> +&drif01 {
> + pinctrl-0 = <&drif0_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> + port {
> + drif0_ep: endpoint {
> + remote-endpoint = <&tuner_ep>;
> + renesas,syncac-active = <0>;
> + };
> + };
> +};
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v3 6/7] dt-bindings: media: Add Renesas R-Car DRIF binding
From: Laurent Pinchart @ 2017-04-11 22:41 UTC (permalink / raw)
To: Ramesh Shanmugasundaram
Cc: Rob Herring, mark.rutland@arm.com, mchehab@kernel.org,
hverkuil@xs4all.nl, sakari.ailus@linux.intel.com, crope@iki.fi,
Chris Paterson, geert+renesas@glider.be,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <HK2PR06MB0545DC72450687BA47610DA6C35A0@HK2PR06MB0545.apcprd06.prod.outlook.com>
Hello,
On Thursday 16 Feb 2017 11:02:55 Ramesh Shanmugasundaram wrote:
> Hi Rob,
>
> Thank you for the review comments.
>
> > Subject: Re: [PATCH v3 6/7] dt-bindings: media: Add Renesas R-Car DRIF
> > binding
> >
> > On Tue, Feb 07, 2017 at 03:02:36PM +0000, Ramesh Shanmugasundaram wrote:
> >> Add binding documentation for Renesas R-Car Digital Radio Interface
> >> (DRIF) controller.
> >>
> >> Signed-off-by: Ramesh Shanmugasundaram
> >> <ramesh.shanmugasundaram@bp.renesas.com>
> >> ---
> >>
> >> .../devicetree/bindings/media/renesas,drif.txt | 186 ++++++++++++++
> >> 1 file changed, 186 insertions(+)
> >> create mode 100644
> >>
> >> Documentation/devicetree/bindings/media/renesas,drif.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt
> >> b/Documentation/devicetree/bindings/media/renesas,drif.txt
> >> new file mode 100644
> >> index 0000000..6315609
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
> >> @@ -0,0 +1,186 @@
> >> +Renesas R-Car Gen3 Digital Radio Interface controller (DRIF)
> >> +------------------------------------------------------------
> >> +
> >> +R-Car Gen3 DRIF is a SPI like receive only slave device. A general
> >> +representation of DRIF interfacing with a master device is shown below.
> >> +
> >> ++---------------------+ +---------------------+
> >> +| |-----SCK------->|CLK |
> >> +| Master |-----SS-------->|SYNC DRIFn (slave) |
> >> +| |-----SD0------->|D0 |
> >> +| |-----SD1------->|D1 |
> >> ++---------------------+ +---------------------+
> >> +
> >> +As per the datasheet, each DRIF channel (drifn) is made up of two
> >> +internal channels (drifn0 & drifn1). These two internal channels
> >> +share the common CLK & SYNC. Each internal channel has its own
> >> +dedicated resources like irq, dma channels, address space & clock.
> >> +This internal split is not visible to the external master device.
> >> +
> >> +The device tree model represents each internal channel as a separate
> >> node.
> >> +The internal channels sharing the CLK & SYNC are tied together by
> >> +their phandles using a new property called "renesas,bonding". For the
> >> +rest of the documentation, unless explicitly stated, the word channel
> >> +implies an internal channel.
> >> +
> >> +When both internal channels are enabled they need to be managed
> >> +together as one (i.e.) they cannot operate alone as independent
> >> +devices. Out of the two, one of them needs to act as a primary device
> >> +that accepts common properties of both the internal channels. This
> >> +channel is identified by a new property called "renesas,primary-bond".
> >> +
> >> +To summarize,
> >> + - When both the internal channels that are bonded together are
> >> enabled,
> >> + the zeroth channel is selected as primary-bond. This channels
> >> accepts
> >> + properties common to all the members of the bond.
> >> + - When only one of the bonded channels need to be enabled, the
> >> property
> >> + "renesas,bonding" or "renesas,primary-bond" will have no effect.
> >> That
> >> + enabled channel can act alone as any other independent device.
> >> +
> >> +Required properties of an internal channel:
> >> +-------------------------------------------
> >> +- compatible: "renesas,r8a7795-drif" if DRIF controller is a part of
> >> R8A7795 SoC.
> >> + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible
> >> device.
> >> + When compatible with the generic version, nodes must list the
> >> + SoC-specific version corresponding to the platform first
> >> + followed by the generic version.
> >> +- reg: offset and length of that channel.
> >> +- interrupts: associated with that channel.
> >> +- clocks: phandle and clock specifier of that channel.
> >> +- clock-names: clock input name string: "fck".
> >> +- dmas: phandles to the DMA channels.
> >> +- dma-names: names of the DMA channel: "rx".
> >> +- renesas,bonding: phandle to the other channel.
> >> +
> >> +Optional properties of an internal channel:
> >> +-------------------------------------------
> >> +- power-domains: phandle to the respective power domain.
> >> +
> >> +Required properties of an internal channel when:
> >> + - It is the only enabled channel of the bond (or)
> >> + - If it acts as primary among enabled bonds
> >> +--------------------------------------------------------
> >> +- pinctrl-0: pin control group to be used for this channel.
> >> +- pinctrl-names: must be "default".
> >> +- renesas,primary-bond: empty property indicating the channel acts as
> >> primary
> >> + among the bonded channels.
> >> +- port: child port node of a channel that defines the local and remote
> >> + endpoints. The remote endpoint is assumed to be a third party tuner
> >> + device endpoint.
> >> +
> >> +Optional endpoint property:
> >> +---------------------------
> >> +- renesas,sync-active : Indicates sync signal polarity, 0/1 for
> >> low/high
> >> + respectively. This property maps to SYNCAC bit in the
> >> + hardware manual. The default is 1 (active high)
> >
> > Why does this belong in the endpoint? I'd prefer to not have vendor
> > specific properties in endpoints. Is this a property of the tuner or DRIF?
In the general case, the sync signal polarity is a property of the tuner (and
in some cases it could even be configurable on the tuner side), which could
then be queried at runtime from the tuner by the DRIF driver. However, there
could be logic on the board that would invert the polarity, so we need to
specify it on the DRIF side as well. As the polarity can differ between
different tuners, it makes sense to specify it in the endpoint, in case
multiple tuners are connected (keeping in mind that only one of them can be
used at a time). However, I we don't support connecting multiple tuners at
this time, and I don't think we ever will, but I could be wrong there.
> This property is similar to the properties in
> Documentation/devicetree/bindings/media/video-interfaces.txt (e.g.
> hsync-active, vsync-active).Hence, Laurent & Hans suggested this to be
> defined as an endpoint property and try to standardize it.
>
> I think I see your point. As endpoint properties can be defined on both
> endpoints, having a vendor specific property is a problem with a third
> party tuner. We could remove the vendor tag and make it a generic property
> "sync-active", if you are OK with it?
>
> This property can be defined for both tuner and DRIF. However, it would
> mostly be a constant in the case of tuner because as per I2S spec,
> transmitter WS (sync) changes from high->low & low->high always. Only DRIF
> allows the option to latch when WS high->low or low->high - both cases.
>
> In a traditional use case it is always WS high->low latching to get the
> first data. However, with DRIF & MAX2175 combo, our latest investigations
> reveal that latching when WS low->high provided better synchronization on
> all cases. There is no loss of data by doing this. Hence, it would be nice
> to retain this as a configurable property.
>
> Please advice.
>
> >> +
> >> +Example
> >> +--------
> >> +
> >> +SoC common dtsi file
> >> +
> >> + drif00: rif@e6f40000 {
> >> + compatible = "renesas,r8a7795-drif",
> >> + "renesas,rcar-gen3-drif";
> >> + reg = <0 0xe6f40000 0 0x64>;
> >> + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> >> + clocks = <&cpg CPG_MOD 515>;
> >> + clock-names = "fck";
> >> + dmas = <&dmac1 0x20>, <&dmac2 0x20>;
> >> + dma-names = "rx", "rx";
> >> + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> >> + renesas,bonding = <&drif01>;
> >> + status = "disabled";
> >
> > Don't put "status" in examples.
>
> OK. Will note this down for future patches. Will be corrected in the next
> version.
>
> >> + };
> >> +
> >> + drif01: rif@e6f50000 {
> >> + compatible = "renesas,r8a7795-drif",
> >> + "renesas,rcar-gen3-drif";
> >> + reg = <0 0xe6f50000 0 0x64>;
> >> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> >> + clocks = <&cpg CPG_MOD 514>;
> >> + clock-names = "fck";
> >> + dmas = <&dmac1 0x22>, <&dmac2 0x22>;
> >> + dma-names = "rx", "rx";
> >> + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> >> + renesas,bonding = <&drif00>;
> >> + status = "disabled";
> >> + };
> >> +
> >> +
> >> +Board specific dts file
> >
> > Chip vs. board in not relevant to the binding doc. Please combine them
> > here in your example.
>
> Will do.
--
Regards,
Laurent Pinchart
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox