Re: [PATCH v3 0/3] mailbox: Add APM X-Gene platform mailbox driver
Hi Duc, On ma, 2015-11-16 at 11:22 -0800, Duc Dang wrote: > Hi Jassi, Paul, > > Do you have any comment on this patch set? Are you perhaps confusing me with another Paul? (I think I made a minor comment or two on this series about half a year ago. But I'm not the maintainer here and it's the maintainers verdict that you're probably interested in mostly. So it's not clear to me why you specifically ask for my comment.) Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support
Hi Maxime,
On ma, 2015-08-31 at 09:49 +0200, Maxime Coquelin wrote:
> On 07/09/2015 10:17 AM, Paul Bolle wrote:
> > > > +static int __exit st_fdma_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct st_fdma_dev *fdev = platform_get_drvdata(pdev);
> > > > +
> > > > + wait_for_completion(&fdev->fw_ack);
> > > > +
> > > > + st_fdma_clk_disable(fdev);
> > > > +
> > > > + return 0;
> > > > +}
> > Since this driver is built-in only this means st_fdma_remove() can
> > never be used, right?
> It's not because a driver is built-in only that it does not need a
> remove callback.
> An instance can be probed/removed any time via driver's bind/unbind
> SysFS entries.
> Am I missing something?
(This discussion is moot because Peter already stated that a new version
will be modular.)
It follows from the __exit tag that st_fdma_remove() should never be
part of the kernel image (in this version of the patch), doesn't it?
(I don't know what happens in this situation if an unbind sysfs entry is
used to remove a driver. I've never tried that.)
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
On ma, 2015-08-03 at 16:56 -0400, Matt Porter wrote:
> --- /dev/null
> +++ b/drivers/iio/temperature/max6675.c
> +static const struct spi_device_id max6675_spi_ids[] = {
> + {"max6675", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, max6675_spi_ids);
> +MODULE_ALIAS("spi:max6675");
For the "spi" alias this is "belt and suspenders":
modinfo ./max6675.ko | grep alias
alias: spi:max6675
alias: acpi*:MXIM6675:*
alias: of:N*T*Cmaxim,max6675*
alias: spi:max6675
I'd drop the MODULE_ALIAS().
(Mark Brown made it quite clear I shouldn't nag people about the origin
of the various strings used in these module aliases. So I won't. But if
you'd volunteer to explain me where "max6675" might come from for the
spi alias that would, at least, satisfy my curiosity.)
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver
On di, 2015-08-04 at 16:49 +0800, Leo Yan wrote:
> On Tue, Aug 04, 2015 at 10:30:24AM +0200, Paul Bolle wrote:
> > On ma, 2015-08-03 at 09:13 +0800, Leo Yan wrote:
> > > --- /dev/null
> > > +++ b/drivers/mailbox/hisilicon/Kconfig
> >
> > > +config HISI_MBOX
> > > + bool "Hisilicon's Mailbox"
> > > + depends on ARCH_HISI || OF
> >
> > ARCH_HISI is available on either ARM64 or ARM. ARM64 selects OF. On ARM
> > ARCH_HISI depends on ARCH_MULTIV7 which depends on ARCH_MULTIPLATFORM.
> > That selects USE_OF which on its turn selects OF.
> >
> > So, HISI_MBOX implies OF, correct?
>
> Exactly, will simply use "depends on ARCH_HISI".
That change would alter the dependencies. My remark was made just to
make clear why I think CONFIG_OF will always be defined. But now I guess
you actually meant
depends on ARCH_HISI && OF
And that would, I think, indeed be equivalent to
depends on ARCH_HISI
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver
(This RFC was part of this mornings catch of my crude mail filter. So,
for what it's worth, what follows are a few random comments for the few
things I'm able to spot.)
On ma, 2015-08-03 at 09:13 +0800, Leo Yan wrote:
> --- /dev/null
> +++ b/drivers/mailbox/hisilicon/Kconfig
> +config HISI_MBOX
> + bool "Hisilicon's Mailbox"
> + depends on ARCH_HISI || OF
ARCH_HISI is available on either ARM64 or ARM. ARM64 selects OF. On ARM
ARCH_HISI depends on ARCH_MULTIV7 which depends on ARCH_MULTIPLATFORM.
That selects USE_OF which on its turn selects OF.
So, HISI_MBOX implies OF, correct?
> + help
> + Support for mailbox drivers on Hisilicon series of SoCs.
> +
> +config HI6220_MBOX
> + tristate "Hi6220 Mailbox Controller"
> + depends on HISI_MBOX
> + help
> + An implementation of the hi6220 mailbox. It is used to send message
> + between application processors and MCU. Say Y here if you want to
> build
> + the Hi6220 mailbox controller driver.
> --- /dev/null
> +++ b/drivers/mailbox/hisilicon/common.c
> +int hisi_mbox_register(struct hisi_mbox_hw *mbox_hw)
> +{
> + [...]
> +
> + mbox->chan = devm_kzalloc(dev,
> + mbox_hw->chan_num * sizeof(struct mbox_chan), GFP_KERNEL);
> + if (!mbox)
if (!mbox->chan)
> + return -ENOMEM;
> +
> + [...]
> +}
> --- /dev/null
> +++ b/drivers/mailbox/hisilicon/hi6220-mailbox.c
> +static struct platform_driver hi6220_mbox_driver = {
> + .driver = {
> + [...]
> + .of_match_table = of_match_ptr(hi6220_mbox_of_match),
of_match_ptr(x) becomes NULL when CONFIG_OF is not defined. But I think
CONFIG_OF will always be defined, see above. So of_match_ptr() isn't per
se needed.
> + },
> + [...]
> +};
> +static int __init hi6220_mbox_init(void)
> +{
> + return platform_driver_register(&hi6220_mbox_driver);
> +}
> +module_init(hi6220_mbox_init);
> +
> +static void __exit hi6220_mbox_exit(void)
> +{
> + platform_driver_unregister(&hi6220_mbox_driver);
> +}
> +module_exit(hi6220_mbox_exit);
This could be flattened into one line:
module_platform_driver(hi6220_mbox_driver);
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP
On do, 2015-07-30 at 12:45 +0100, Lee Jones wrote: > On Wed, 29 Jul 2015, Paul Bolle wrote: > > Besides, if I read sti_max_probe() correctly, without OF support > > loading > > this module won't accomplish much. So what would another way of > > autoloading this module buy you? > > I think this line can be safely removed. Looking at this patch a little more I notice there's no line reading MODULE_DEVICE_TABLE(of, sti_mailbox_match); The three mailbox drivers that currently support OF do have such a line. So "another way of autoloading" was rather sloppy as now I don't see how this driver, if build as a module, will get auto-loaded. Maybe there's an auto-loading mechanism that I'm not aware of here. Or perhaps auto -loading is not needed. Similar question for patch 5/6 and the lack of a line reading MODULE_DEVICE_TABLE(of, mbox_test_match); Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip
On wo, 2015-07-29 at 14:15 +0200, Robert Baldyga wrote:
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/Kconfig
> @@ -0,0 +1,22 @@
> +config NFC_S3FWRN5
> + tristate "Samsung S3FWRN5 NFC driver"
> + depends on NFC_NCI
> + default n
> + ---help---
> + Core driver for Samsung S3FWRN5 NFC chip.
> +
> + To compile this driver as a module, choose m here. The module will
> + be called s3fwrn5.ko.
> + Say N if unsure.
If I scanned the code correctly then all this module does is exporting
three functions to s3fwrn5_i2c.ko. Note that there's also no
module_unit() in sight. So it's a library, of sorts. And there's no
reason to load this module without also loading s3fwrn5_i2c.ko. Likewise
there's no reason to build it without also building s3fwrn5_i2c.ko.
So perhaps you could merge the two Kconfig symbols you add in this
patch.
Or, if you'd like to keep both Kconfig symbols, for whatever reason, you
might want to make NFC_S3FWRN5 a "silent" symbol that will be selected
by NFC_S3FWRN5_I2C. (That probably requires NFC_S3FWRN5_I2C to depend on
both NFC_NCI and I2C.)
Would either of the above two options work here?
> +config NFC_S3FWRN5_I2C
> + tristate "Samsung S3FWRN5 I2C support"
> + depends on NFC_S3FWRN5 && I2C
> + default y
You only added "default y" to make setting this symbol in "make *config"
a one step process, right?
> + ---help---
> + This module adds support for an I2C interface to the S3FWRN5 chip.
> + Select this if your platform is using the I2C bus.
> +
> + To compile this driver as a module, choose m here. The module will
> + be called s3fwrn5_i2c.ko.
> + Say N if unsure.
(This advice is at odds with "default y" above, by the way.)
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/Makefile
> +s3fwrn5-objs = core.o firmware.o nci.o
> +s3fwrn5_i2c-objs = i2c.o
> +
> +obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
> +obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/core.c
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> +int s3fwrn5_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
> + struct s3fwrn5_phy_ops *phy_ops, unsigned int max_payload)
> +{
> + [...]
> +}
> +EXPORT_SYMBOL(s3fwrn5_probe);
> +
> +void s3fwrn5_remove(struct nci_dev *ndev)
> +{
> + [...]
> +}
> +EXPORT_SYMBOL(s3fwrn5_remove);
> +
> +int s3fwrn5_recv_frame(struct nci_dev *ndev, struct sk_buff *skb,
> + enum s3fwrn5_mode mode)
> +{
> + [...]
> +}
> +EXPORT_SYMBOL(s3fwrn5_recv_frame);
> +MODULE_LICENSE("GPL");
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/i2c.c
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> +static int s3fwrn5_i2c_fw_read(struct s3fwrn5_i2c_phy *phy)
> +{
> + [...]
> +
> + return s3fwrn5_recv_frame(phy->ndev, skb, S3FWRN5_MODE_FW);
> +}
> +
> +static int s3fwrn5_i2c_nci_read(struct s3fwrn5_i2c_phy *phy)
> +{
> + [...]
> +
> + return s3fwrn5_recv_frame(phy->ndev, skb, S3FWRN5_MODE_NCI);
> +}
> +static int s3fwrn5_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + [...]
> +
> + ret = s3fwrn5_probe(&phy->ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops,
> + S3FWRN5_I2C_MAX_PAYLOAD);
> + [...]
> + s3fwrn5_remove(phy->ndev);
> +
> + [...]
> +}
> +
> +static int s3fwrn5_i2c_remove(struct i2c_client *client)
> +{
> + [...]
> +
> + s3fwrn5_remove(phy->ndev);
> +
> + [...]
> +}
> +MODULE_LICENSE("GPL");
Nit: both modules' code contain a comment referring to the "GNU General
Public License, version 2". They also both use the "GPL" ident in their
MODULE_LICENSE() macro. And, according to include/linux/module.h, that
ident states the license is GPL v2 or later. So I think either the
comments or the idents need to change.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] pci:host: Add Altera PCIe host controller driver
On di, 2015-07-28 at 18:45 +0800, Ley Foon Tan wrote:
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> +config PCIE_ALTERA
> + bool "Altera PCIe controller"
> + depends on ARCH_SOCFPGA
> + depends on OF
> + select PCI_MSI_IRQ_DOMAIN if PCI_MSI
> + help
> + Say Y here if you want to enable PCIe controller support for Altera
> + SoCFPGA family of SoCs.
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> +obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
> --- /dev/null
> +++ b/drivers/pci/host/pcie-altera.c
> +#include
> +static const struct of_device_id altera_pcie_of_match[] = {
> + { .compatible = "altr,pcie-root-port-1.0", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altera_pcie_of_match);
> +static struct platform_driver altera_pcie_driver = {
> + [...]
> + .owner = THIS_MODULE,
> + ]...]
> +};
> +
> +module_platform_driver(altera_pcie_driver);
> +
> +MODULE_AUTHOR("Ley Foon Tan ");
> +MODULE_DESCRIPTION("Altera PCIe host controller driver");
> +MODULE_LICENSE("GPL v2");
PCIE_ALTERA is a bool symbol. So pcie-altera.o is built-in only. Yet
pcie-altera.c uses a number of module-specific constructs. Should
PCIE_ALTERA perhaps be tristate?
Likewise for PCIE_ALTERA_MSI in 4/6.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] iio: Support triggered events
On wo, 2015-07-29 at 00:58 -0700, Christoph Hellwig wrote: > Btw, who came up with that meaning? The default Linux license is GPLv2 > only and unless othewise specified that's what we should get by default. I cobbled together a short history of these license idents in https://lkml.kernel.org/r/1426071405.4244.88.camel@x220 . Hope this helps, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] iio: Support triggered events
Just a nit, I'm afraid.
On di, 2015-07-28 at 02:07 +0300, Vladimir Barinov wrote:
> --- /dev/null
> +++ b/drivers/iio/industrialio-triggered-event.c
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And, according to include/linux/module.h, this states the license is GPL
v2 or later. So either the comment or the ident used in the
MODULE_LICENSE() macro needs to change.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP
On ma, 2015-07-27 at 10:44 +0100, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-sti.c
> +static int sti_mbox_probe(struct platform_device *pdev)
> +{
> + [...]
> +
> + match = of_match_device(sti_mailbox_match, &pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "No configuration found\n");
> + return -ENODEV;
> + }
> + pdev->dev.platform_data = (struct sti_mbox_pdata *) match->data;
> +
> + [...]
> +}
> +static struct platform_driver sti_mbox_driver = {
> + .probe = sti_mbox_probe,
> + .remove = sti_mbox_remove,
> + .driver = {
> + .name = "sti-mailbox",
> + .of_match_table = sti_mailbox_match,
> + },
> +};
> +module_platform_driver(sti_mbox_driver);
> +MODULE_ALIAS("platform:mailbox-sti");
It seems this alias is only useful if there's a corresponding struct
platform_device with a "mailbox-sti" .name. I couldn't spot that
platform_device.
Besides, if I read sti_max_probe() correctly, without OF support loading
this module won't accomplish much. So what would another way of
autoloading this module buy you?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mtd: atmel-quadspi: add driver for Atmel QSPI controller
On do, 2015-07-16 at 17:27 +0200, Cyrille Pitchen wrote:
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c
> +static struct platform_driver atmel_qspi_driver = {
> + .driver = {
> + [...]
> + .bus= &platform_bus_type,
> + [...]
> +};
> +module_platform_driver(atmel_qspi_driver);
Nit: on module init this will basically do
__platform_driver_register(&atmel_qspi_driver, THIS_MODULE);
which will again set bus to &platform_bus_type. So you might as well
drop that line.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] drm/layerscape: Add Freescale DCU DRM driver
A question and a nit follow.
On vr, 2015-07-10 at 19:17 +0800, Jianwei Wang wrote:
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +MODULE_ALIAS("platform:fsl-dcu-drm");
Question: this appears to be only useful if there's a corresponding
struct platform_device. That is, a platform_device with a "fsl-dcu-drm"
.name. It will fire off a "MODALIAS=platform:fsl-dcu-drm" uevent when
it's created.
I couldn't find this corresponding platform_device. Does it exist? Or is
this alias needed for some other reason?
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +#define DRIVER_NAME "fsl-dcu-drm"
Nit: I don't think DRIVER_NAME is actually used anywhere.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] dmaengine: st_fdma: Add xbar support
On wo, 2015-07-08 at 17:11 +0100, Peter Griffin wrote:
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> +config ST_FDMA_XBAR
> + bool "ST FDMA crossbar"
> + depends on ST_FDMA
> + default y
> + help
> + Enable support for ST FDMA crossbar.
> + xbar add flexibility and increase the number of peripheral request
> + can be used by fdma xbar can multiplex until 96 peripheral requests
> + to one of 3 fdma controller
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index f68e6d8..19f18b1 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> +obj-$(CONFIG_ST_FDMA_XBAR) += st_fdma_xbar.o
ST_FDMA_XBAR is a bool symbol, so st_fdma_xbar.o can only be built-in.
> --- /dev/null
> +++ b/drivers/dma/st_fdma_xbar.c
> +#include
Needed?
> +void st_fdma_xbar_free(struct st_fdma_xbar_dev *device)
> +{
> + platform_device_put(device->pdev);
> + kfree(device);
> +}
Unused.
> +static const struct of_device_id st_fdma_xbar_match[] = {
> + { .compatible = "st,fdma-xbar-1.0", .data = (void *)XBAR_1_0_MAX_REQ },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, st_fdma_xbar_match);
See my remark on 3/7: MODULE_DEVICE_TABLE() will be preprocessed away.
> +static struct platform_driver st_fdma_xbar_driver = {
> + .driver = {
> + .name = "st-fdma-xbar",
> + .of_match_table = st_fdma_xbar_match,
> + },
> + .probe = st_fdma_xbar_probe,
> + .remove = st_fdma_xbar_remove,
> +};
> +module_platform_driver(st_fdma_xbar_driver);
See my remark on 3/7: builtin_platform_driver().
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("STMicroelectronics FDMA cross bar");
> +MODULE_AUTHOR("Ludovic.barre ");
See my remark on 3/7: will be preprocessed away.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support
On wo, 2015-07-08 at 17:11 +0100, Peter Griffin wrote:
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> +config ST_FDMA
> + bool "ST FDMA dmaengine support"
> + depends on ARCH_STI
> + select DMA_ENGINE
> + select FW_LOADER
> + select FW_LOADER_USER_HELPER_FALLBACK
> + select LIBELF_32
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Enable support for ST FDMA controller.
> + It supports 16 independent DMA channels, accepts up to 32 DMA requests
> +
> + Say Y here if you have such a chipset.
> + If unsure, say N.
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
>
> +obj-$(CONFIG_ST_FDMA) += st_fdma.o
ST_FDMA is a bool symbol, so st_fdma.o can only be built-in.
> --- /dev/null
> +++ b/drivers/dma/st_fdma.c
> +#include
Needed?
> +void *st_fdma_seg_to_mem(struct st_fdma_dev *fdev, u64 da, int len)
static?
> +{
> + [...]
> +}
> +static int
> +st_fdma_elf_load_segments(struct st_fdma_dev *fdev, const struct
> firmware *fw)
> +{
> + [...]
> + dst = st_fdma_seg_to_mem(fdev, da, memsz);
> + [...]
> +}
> +static const struct of_device_id st_fdma_match[] = {
> + { .compatible = "st,fdma_mpe31", .data = &fdma_mpe31 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, st_fdma_match);
MODULE_DEVICE_TABLE() is preprocessed away for built-in only code.
> +static int __exit st_fdma_remove(struct platform_device *pdev)
> +{
> + struct st_fdma_dev *fdev = platform_get_drvdata(pdev);
> +
> + wait_for_completion(&fdev->fw_ack);
> +
> + st_fdma_clk_disable(fdev);
> +
> + return 0;
> +}
Since this driver is built-in only this means st_fdma_remove() can never
be used, right?
> +static struct platform_driver st_fdma_platform_driver = {
> + .driver = {
> + .name = "st-fdma",
> + .of_match_table = st_fdma_match,
> + .pm = ST_FDMA_PM,
> + },
> + .probe = st_fdma_probe,
> + .remove = st_fdma_remove,
> +};
> +module_platform_driver(st_fdma_platform_driver);
So can .remove be dropped?
Since v4.2-rc1 there's builtin_platform_driver() for built-in only code.
> +bool st_fdma_filter_fn(struct dma_chan *chan, void *param)
> +{
> + [...]
> +}
> +EXPORT_SYMBOL(st_fdma_filter_fn);
This series adds no users of this export. I suppose they will be added
in another series. Is that correct?
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("STMicroelectronics FDMA engine driver");
> +MODULE_AUTHOR("Ludovic.barre ");
These macros will, basically, be preprocessed away for built-in only
code.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] mmc: sdhci-of-at91: introduce driver for the Atmel SDMMC
A nit only: a license mismatch.
On vr, 2015-07-03 at 16:17 +0200, Ludovic Desroches wrote:
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-at91.c
>
> + * 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 states the license is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And, according to include/linux/module.h, this states the license is
(only) GPL v2. So I think that either the comment at the top of this
file or the ident used in MODULE_LICENSE() needs to change.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.
On Thu, 2015-06-25 at 09:47 +0200, Michal Simek wrote: > It has to be platform_device somewhere for sure. > In past we had folder in arch/microblaze/platform folder. > Currently you can add this code to for example > arch/microblaze/kernel/platform.c > > But as I said I don't think it is really important. There will be a > lot > of others drivers in the kernel which can be used as platform drivers > but you are not able to find out platform_device for it. Because, like probably happens with this driver, the OF infrastructure makes sure the .probe and .remove functions will eventually called? > The important part is that driver can work as is. Sure. But I was talking about the MODULE_ALIAS() macro. Trivial as it is, since I still don't see where a "MODALIAS=platform:xilinx-mailbox" uevent could come from I still don't see the point of this line. As I asked in my first message: what breaks if this line is dropped? > Also it is quite common that users create own BSP for their custom > boards but they don't push it to mainline. I can't recall what BSP means. Anyhow, why should we care about boards not pushed into mainline? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.
On Thu, 2015-06-25 at 08:55 +0200, Michal Simek wrote:
> On 06/24/2015 10:36 PM, Paul Bolle wrote:
> > On Tue, 2015-06-23 at 11:00 -0700, Moritz Fischer wrote:
> > > +MODULE_ALIAS("platform:xilinx-mailbox");
> >
> > So I think this MODULE_ALIAS() is only useful if, in short, there's
> > a corresponding platform_device created. Ie, a platform_device with
> > a name "xilinx-mailbox" that will fire of a "MODALIAS=platform:xili
> > nx-mailbox" when it's created.
> >
> > I couldn't spot such a platform_device. Provided git grep didn't
> > let me down here: what breaks if this line is dropped?
>
> IRC you don't need to have this platform_device in the kernel
> present. Only one thing which should be check is that this driver can
> be used as platform device driver.
>
> The only one problematic part is devm_clk_get() and this should be
> checked if you can use this as platform driver. From the first look
> it looks like that this will break it.
>
> Anyway if Moritz is able to use this a platform driver he can keep
> this line there. If not, it should be removed.
But, assuming this works as a platform driver, where does the "xilinx
-mailbox" platform device originate?
Paul Bolle
PS Evolution 3.16 is nearly unbearable in its handling of replies to
plain text messages. Doe anyone know how to make it handle them
sensibly?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] [media] tsin: c8sectpfe: Add Kconfig and Makefile for the driver.
On Wed, 2015-06-24 at 16:11 +0100, Peter Griffin wrote: > --- /dev/null > +++ b/drivers/media/tsin/c8sectpfe/Makefile > +c8sectpfe-y += c8sectpfe-core.o c8sectpfe-common.o c8sectpfe-dvb.o > + > +obj-$(CONFIG_DVB_C8SECTPFE) += c8sectpfe.o > + > +ifneq ($(CONFIG_DVB_C8SECTPFE),) > + c8sectpfe-y += c8sectpfe-debugfs.o > +endif Isn't the above equivalent to c8sectpfe-y += c8sectpfe-core.o c8sectpfe-common.o c8sectpfe-dvb.o c8sectpfe-debugfs.o obj-$(CONFIG_DVB_C8SECTPFE) += c8sectpfe.o Or am I missing something subtle here? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.
On Tue, 2015-06-23 at 11:00 -0700, Moritz Fischer wrote:
> +MODULE_ALIAS("platform:xilinx-mailbox");
So I think this MODULE_ALIAS() is only useful if, in short, there's a
corresponding platform_device created. Ie, a platform_device with a
name "xilinx-mailbox" that will fire of a "MODALIAS=platform:xilinx
-mailbox" when it's created.
I couldn't spot such a platform_device. Provided git grep didn't let me
down here: what breaks if this line is dropped?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] arm: twr-k70f120m: IOMUX driver for Kinetis SoC
On Tue, 2015-06-23 at 23:19 +0200, Paul Osmialowski wrote:
> --- a/drivers/pinctrl/freescale/Kconfig
> +++ b/drivers/pinctrl/freescale/Kconfig
> +config PINCTRL_KINETIS
> + bool "Kinetis pinctrl driver"
> + depends on OF
> + depends on SOC_K70
> + select PINMUX
> + help
> + Say Y here to enable the Kinetis pinctrl driver
> --- a/drivers/pinctrl/freescale/Makefile
> +++ b/drivers/pinctrl/freescale/Makefile
> +obj-$(CONFIG_PINCTRL_KINETIS)+= pinctrl-kinetis.o
> --- /dev/null
> +++ b/drivers/pinctrl/freescale/pinctrl-kinetis.c
> +#include
> +static struct pinctrl_desc kinetis_pinctrl_desc = {
> + [...]
> + .owner = THIS_MODULE,
> +};
> +static void __exit kinetis_pinctrl_exit(void)
> +{
> + platform_driver_unregister(&kinetis_pinctrl_driver);
> +}
> +module_exit(kinetis_pinctrl_exit);
> +
> +MODULE_DESCRIPTION("Freescale Kinetis pinctrl driver");
> +MODULE_LICENSE("GPL v2");
pinctrl-kinetis.o can only be built-in, right? But the code uses a few
module specific constructs. Did you mean to make PINCTRL_KINETIS
tristate instead?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms
On Tue, 2015-06-23 at 09:28 +0100, Lee Jones wrote: > BTW, do you have a script that does this now, or are you still > hand-rolling these? There was a script detecting Kconfig problems that is basically retired. (The messages I sent were always manually written.) That script's function is now performed, much better, by the undertaker -checkpatch bot (courtesy of a group of people apparently all affiliated with the university of Erlangen, Germany). We're in virtual contact but that group is doing fine so I'm basically not contributing. Anyhow, my reply to your patch was the result of a manual scan for a small set of common (and mostly trivial) mistakes in patches that I try to do regularly. All pretty basic stuff, otherwise I wouldn't been able to spot these mistakes. Does this answer your question? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms
On Mon, 2015-06-22 at 16:43 +0100, Lee Jones wrote:
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> +config ARM_ST_CPUFREQ
> +>> bool "ST CPUFreq support"
> +>> depends on SOC_STIH407
> +>> help
> +>> OPP list for cpufreq-dt driver can be provided through DT or can be
> +>> created at runtime. Select this if you want create OPP list at
> runtime.
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> +obj-$(CONFIG_ARM_ST_CPUFREQ) += st-cpufreq.o
> --- /dev/null
> +++ b/drivers/cpufreq/st-cpufreq.c
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +#include
> +MODULE_DEVICE_TABLE(of, sti_cpufreq_of_match);
>
> +module_platform_driver(sti_cpufreq);
>
> +MODULE_AUTHOR("Ajitpal Singh ");
> +MODULE_DESCRIPTION("Creates an OPP list for cpufreq-cpu0 at runtime");
> +MODULE_LICENSE("GPL v2");
(There's a mismatch between the license used in the comment at the top
of this file and the ident used in the MODULE_LICENSE() macro. See
include/linux/module.h.)
st-cpufreq.o can only be built-in. But the code contains of few lines
that are only useful if the code can be modular. Was ARM_ST_CPUFREQ
perhaps meant to be tristate?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] power: Add support for DA9150 Fuel-Gauge
On Thu, 2015-06-18 at 17:06 +0100, Adam Thomson wrote:
> --- /dev/null
> +++ b/drivers/power/da9150-fg.c
> +/* Register external function to give battery temperature */
> +void da9150_fg_register_temp_cb(struct power_supply *psy, da9150_read_temp_t
> cb,
> + void *cb_context)
> +{
> + struct da9150_fg *fg = dev_get_drvdata(psy->dev.parent);
> +
> + fg->read_bat_temp = cb;
> + fg->bat_temp_context = cb_context;
> +}
> +EXPORT_SYMBOL_GPL(da9150_fg_register_temp_cb);
This series doesn't add a user of this export. Actually, it doesn't even
add a local caller of this function. Is a patch that adds a user of this
function queued somewhere?
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
Re: [PATCH 1/4] mfd: da9150: Add support for Fuel-Gauge
On Thu, 2015-06-18 at 17:06 +0100, Adam Thomson wrote: > --- /dev/null > +++ b/include/linux/mfd/da9150/fg.h > +/* > + * Function template to provide battery temperature. Should provide > + * 0.1 degrees C resolution return values. > + */ > +typedef int (*da9150_read_temp_t)(void *context); > + > +/* Register temp callback function */ > +void da9150_fg_register_temp_cb(struct power_supply *psy, da9150_read_temp_t > cb, > + void *cb_context); The pedant in me noticed that this function is actually added in 3/4. So this chunk might be moved to 3/4, if you like to entertain pedantry like that, that is. (But see my remark on 3/4 too.) Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in
Re: [PATCH v2 2/2] mfd: flexcom: add a driver for Atmel Flexible Serial Communication Unit
On Thu, 2015-06-18 at 15:05 +0200, Cyrille Pitchen wrote:
> --- /dev/null
> +++ b/drivers/mfd/atmel-flexcom.c
> +MODULE_ALIAS("platform:atmel_flexcom");
(The day before yesterday and yesterday I had a, well, lively
conversation regarding this macro. The interesting bits start at
https://lkml.org/lkml/2015/6/17/383 . Let's see how things go here.)
As I understand it, this alias is only useful if there's a corresponding
struct platform_device, somewhere. Ie, this alias implies a
platform_device that will fire of a "MODALIAS=platform:atmel_flexcom"
uevent when it's created. That would be a platform_device using a
"atmel_flexcom" name.
If that's correct, then I think this MODULE_ALIAS macro isn't needed
here, as I couldn't find a platform_device using that name. (But perhaps
a patch that adds it is pending, somewhere.)
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH linux-next 2/2] mfd: flexcom: add a driver for Atmel Flexible Serial Communication Unit
Just a nit: a license mismatch.
On Mon, 2015-06-15 at 18:38 +0200, Cyrille Pitchen wrote:
> --- /dev/null
> +++ b/drivers/mfd/atmel-flexcom.c
> + * 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 states the license is GPL v2 (or later)
> +MODULE_LICENSE("GPL v2");
According to include/linux/module.h this states the license is (just)
GPL v2. So I think that either the comment at the top of this file or
the ident used in the MODULE_LICENSE() macro needs to change.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT)
Some remarks (that might not touch the subjects you want to get feedback
on for an RFC).
On Fri, 2015-06-12 at 23:04 +0300, Pantelis Antoniou wrote:
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> +config DEV_OVERLAYMGR
> + tristate "Device overlay manager"
> + depends on OF
> + select OF_OVERLAY
> + default n
Why bother with "default n"?
> + help
> + Say Y here to include support for the automagical dev
> + overlay manager.
> --- /dev/null
> +++ b/drivers/misc/devovmgr.c
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
You're including twice.
> +/* copy of drivers/pci/pci.h */
Because?
> +static inline const struct pci_device_id *
> +pci_match_one_device(const struct pci_device_id *id, const struct pci_dev
> *dev)
> +{
> + if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
> + (id->device == PCI_ANY_ID || id->device == dev->device) &&
> + (id->subvendor == PCI_ANY_ID ||
> + id->subvendor == dev->subsystem_vendor) &&
> + (id->subdevice == PCI_ANY_ID ||
> + id->subdevice == dev->subsystem_device) &&
> + !((id->class ^ dev->class) & id->class_mask))
> + return id;
> + return NULL;
> +}
> +#if IS_ENABLED(CONFIG_USB)
> +/* in drivers/usb/core/driver.c */
> +extern int usb_match_device(struct usb_device *dev,
> + const struct usb_device_id *id);
And that's an internal function of the usb core, isn't it?
> +static int __init dovmgr_init(void)
> +{
> + int ret;
> +
> + config_group_init(&dovmgr_subsys.su_group);
> +
> +#if IS_ENABLED(CONFIG_PCI)
> + ret = dovmgr_pci_init();
> + if (ret != 0)
> + goto err_no_pci_init;
> +#endif
> +#if IS_ENABLED(CONFIG_USB)
> + ret = dovmgr_usb_init();
> + if (ret != 0)
> + goto err_no_usb_init;
> +#endif
> +
> + ret = configfs_register_subsystem(&dovmgr_subsys);
> + if (ret != 0) {
> + pr_err("%s: failed to register subsys\n", __func__);
> + goto err_no_configfs;
> + }
> + pr_info("%s: OK\n", __func__);
> + return 0;
> +
> +err_no_configfs:
> +#if IS_ENABLED(CONFIG_USB)
> + dovmgr_usb_cleanup();
> +err_no_usb_init:
> +#endif
> +#if IS_ENABLED(CONFIG_PCI)
> + dovmgr_pci_cleanup();
> +err_no_pci_init:
> +#endif
> + return ret;
> +}
> +late_initcall(dovmgr_init);
Lot's of "#if IS_ENABLED(CONFIG_USB)" and "#if IS_ENABLED(CONFIG_PCI)"
in the code. The above function is a rather ugly example.
Is there a point to all this if neither PCI nor USB is enabled?
USB can be 'm'. Does this build and work in that case?
There's no MODULE_LICENSE() macro. If this is a module then loading it
will taint the kernel.
There's also no function that is, well, called by module_exit() to allow
(easy) unloading (and do the needed cleaning up on unload). Did you
intend DEV_OVERLAYMGR to be bool instead?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] powerpc/rcpm: add RCPM driver
Just a nit.
On Thu, 2015-06-11 at 14:32 +0800, yuantian.t...@freescale.com wrote:
> --- /dev/null
> +++ b/arch/powerpc/sysdev/fsl_rcpm.c
> +int fsl_rcpm_init(void)
This is used only through early_initcall(). I took the cargo cult
approach of looking at the other uses of early_initcall() in
arch/powerpc/. That approach tells me this function could be static and
marked as __init. Would that work here too?
> +{
> + [...]
> +}
> +
> +/* need to call this before SMP init */
> +early_initcall(fsl_rcpm_init);
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 1/3] ASoC: mediatek: Add AFE platform driver
On Fri, 2015-06-12 at 09:55 +0800, Koro Chen wrote: > On Thu, 2015-06-11 at 09:03 +0200, Paul Bolle wrote: > > (What does negating a bool twice do?) > > > Because bool actually can be unsigned char, although actually in this > driver, the caller always passes "true" or "false" to this function. bool is _Bool in the kernel (see include/linux/types.h). So whenever you see a bool in the kernel you can assume it's either 0 or 1. Are there any cases where this conveniently simple rule doesn't hold? But here the discussion is moot, because as you say, the function will only be passed false or true so we know "enable" is either 0 or 1 and double negating will do nothing. > Do you think if this is the case, should I still need to do !!? So you should not, as it's confusing at best. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ASoC: mediatek: Add AFE platform driver
On Thu, 2015-06-11 at 09:03 +0200, Paul Bolle wrote: > Is SND_SOC_MEDIATEK perhaps meant to be bool? s/bool/tristate/, of course. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ASoC: mediatek: Add AFE platform driver
On Wed, 2015-06-10 at 22:24 +0800, Koro Chen wrote:
> --- /dev/null
> +++ b/sound/soc/mediatek/Kconfig
> +config SND_SOC_MEDIATEK
> + bool "ASoC support for Mediatek chip"
> + depends on ARCH_MEDIATEK
> + help
> + This adds ASoC platform driver support for Mediatek chip
> + that can be used with other codecs.
> + Select Y if you have such device.
> + Ex: MT8173
> --- /dev/null
> +++ b/sound/soc/mediatek/Makefile
> +obj-$(CONFIG_SND_SOC_MEDIATEK) += mtk-afe-pcm.o
> --- /dev/null
> +++ b/sound/soc/mediatek/mtk-afe-pcm.c
> +#include
> +static void mtk_afe_set_i2s_enable(struct mtk_afe *afe, bool enable)
> +{
> + unsigned int val;
> +
> + regmap_read(afe->regmap, AFE_I2S_CON2, &val);
> + if (!!(val & AFE_I2S_CON2_EN) == !!enable)
(What does negating a bool twice do?)
> + return; /* must skip soft reset */
> +
> + /* I2S soft reset begin */
> + regmap_update_bits(afe->regmap, AUDIO_TOP_CON1, 0x4, 0x4);
> +
> + /* input */
> + regmap_update_bits(afe->regmap, AFE_I2S_CON2, 0x1, !!enable);
Ditto.
> +
> + /* output */
> + regmap_update_bits(afe->regmap, AFE_I2S_CON1, 0x1, !!enable);
Ditto.
> +
> + /* I2S soft reset end */
> + udelay(1);
> + regmap_update_bits(afe->regmap, AUDIO_TOP_CON1, 0x4, 0);
> +}
> +static const struct of_device_id mtk_afe_pcm_dt_match[] = {
> + { .compatible = "mediatek,mt8173-afe-pcm", },
> + { }
> +};
> +static struct platform_driver mtk_afe_pcm_driver = {
> + .driver = {
> +.name = "mtk-afe-pcm",
> +.owner = THIS_MODULE,
> +.of_match_table = mtk_afe_pcm_dt_match,
> +.pm = &mtk_afe_pm_ops,
> + },
> + .probe = mtk_afe_pcm_dev_probe,
> + .remove = mtk_afe_pcm_dev_remove,
> +};
> +MODULE_DESCRIPTION("Mediatek ALSA SoC AFE platform driver");
> +MODULE_AUTHOR("Koro Chen ");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, mtk_afe_pcm_dt_match);
(The common pattern is to have MODULE_DEVICE_TABLE() directly follow
that table.)
SND_SOC_MEDIATEK is a bool symbol and mtk-afe-pcm.o is built-in only.
But the code uses a few module specific constructs. I spotted
THIS_MODULE, MODULE_DESCRIPTION, MODULE_AUTHOR, MODULE_LICENSE, and
MODULE_DEVICE_TABLE.
Is SND_SOC_MEDIATEK perhaps meant to be bool?
Likewise for SND_SOC_MT8173_MAX98090 (in 2/3) and
SND_SOC_MT8173_RT5650_RT5676 (in 3/3).
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver
On Mon, 2015-06-08 at 20:29 +0800, Pi-Cheng Chen wrote: > --- /dev/null > +++ b/drivers/cpufreq/mt8173-cpufreq.c > +#include Weren't you going to drop this include? > +module_init(mt8173_cpufreq_driver_init); For built-in code this is equivalent to, speaking from memory: device_initcall(mt8173_cpufreq_driver_init); Why don't you just use that directly? Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] soc: Add driver for Freescale Vybrid Platform
>soc_dev_attr = devm_kzalloc(&pdev->dev,
> + sizeof(info->soc_dev_attr), GFP_KERNEL);
> + if (!info->soc_dev_attr)
> + return -ENOMEM;
> +
> + info->soc_dev_attr->machine = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "Freescale Vybrid");
> + info->soc_dev_attr->soc_id = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "%016llx", soc_id);
> + info->soc_dev_attr->family = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "Freescale Vybrid VF%s",
> + soc_type);
> + info->soc_dev_attr->revision = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "%08x", rom_rev);
> +
> + info->soc_dev = soc_device_register(info->soc_dev_attr);
> + if (IS_ERR(info->soc_dev))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int vf610_soc_remove(struct platform_device *pdev)
> +{
> + struct vf610_soc *info = platform_get_drvdata(pdev);
> +
> + if (info->soc_dev)
> + soc_device_unregister(info->soc_dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id vf610_soc_bus_match[] = {
> + { .compatible = "fsl,vf610-mscm-cpucfg", },
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver vf610_soc_driver = {
> + .probe = vf610_soc_probe,
> + .remove = vf610_soc_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = vf610_soc_bus_match,
> + },
> +};
> +module_platform_driver(vf610_soc_driver);
The Kconfig symbol is tristate now, but all module specific code is gone
from this file (ie, MODULE_DEVICE_TABLE, MODULE_DESCRIPTION and
MODULE_LICENSE). Why's that?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Thu, 2015-06-04 at 16:07 +0200, Paul Bolle wrote:
> On Wed, 2015-06-03 at 12:37 +0300, Yaniv Gardi wrote:
> > +static int ufs_qcom_probe(struct platform_device *pdev)
> > +{
> > + dev_set_drvdata(&pdev->dev, (void *)&ufs_hba_qcom_vops);
>
> (Cast to void * should not be needed.)
Only if ufs_hba_qcom_vops wasn't a _const_ struct ufs_hba_variant_ops,
that is. Never mind.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Wed, 2015-06-03 at 12:37 +0300, Yaniv Gardi wrote:
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> EXPORT_SYMBOL(ufs_hba_qcom_vops);
Nothing uses this export. It's still a (static) symbol that is not
included in any header. I think this export serves no purpose. Am I
missing something subtle here?
> +/**
> + * ufs_qcom_probe - probe routine of the driver
> + * @pdev: pointer to Platform device handle
> + *
> + * Always return 0
> + */
> +static int ufs_qcom_probe(struct platform_device *pdev)
> +{
> + dev_set_drvdata(&pdev->dev, (void *)&ufs_hba_qcom_vops);
(Cast to void * should not be needed.)
> + return 0;
> +}
> +
> +/**
> + * ufs_qcom_remove - set driver_data of the device to NULL
> + * @pdev: pointer to platform device handle
> + *
> + * Always return 0
> + */
> +static int ufs_qcom_remove(struct platform_device *pdev)
> +{
> + dev_set_drvdata(&pdev->dev, NULL);
> + return 0;
> +}
> +
> +static const struct of_device_id ufs_qcom_of_match[] = {
> + { .compatible = "qcom,ufs_variant"},
> + {},
> +};
> +
> +static struct platform_driver ufs_qcom_pltform = {
> + .probe = ufs_qcom_probe,
> + .remove = ufs_qcom_remove,
> + .driver = {
> + .name = "ufs_qcom",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(ufs_qcom_of_match),
> + },
> +};
> +module_platform_driver(ufs_qcom_pltform);
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> + struct device_node *node = pdev->dev.of_node;
> + struct device_node *ufs_variant_node;
> + struct platform_device *ufs_variant_pdev;
> - hba->vops = get_variant_ops(&pdev->dev);
> + err = of_platform_populate(node, NULL, NULL, &pdev->dev);
> + if (err)
> + dev_err(&pdev->dev,
> + "%s: of_platform_populate() failed\n", __func__);
> +
> + ufs_variant_node = of_get_next_available_child(node, NULL);
> +
> + if (!ufs_variant_node) {
> + dev_dbg(&pdev->dev, "failed to find ufs_variant_node child\n");
> + } else {
> + ufs_variant_pdev = of_find_device_by_node(ufs_variant_node);
> +
> + if (ufs_variant_pdev)
> + hba->vops = (struct ufs_hba_variant_ops *)
> + dev_get_drvdata(&ufs_variant_pdev->dev);
(Another cast that I think is not needed.)
> + }
If I scanned this correctly, the dev_set_drvdata() and dev_get_drvdata()
pair adds an actual user of ufs_hba_qcom_vops. So that ends the obvious
issue I think the code currently has. And I gladly defer to the scsi
people to determine whether that is done the right way.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
Just a nit, that I spotted while scanning for other issues.
On Mon, 2015-06-01 at 15:20 +0300, Vladimir Barinov wrote:
> --- /dev/null
> +++ b/drivers/iio/adc/hi-843x.c
> +ssize_t hi843x_debounce_soft_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + [...]
> +}
>+
>+[...]
>+
> +ssize_t hi843x_threshold_socval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + [...]
> +}
It seems all these *_show and *_store functions should be made static,
right?
> +static IIO_DEVICE_ATTR(debounce_soft, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_show, hi843x_debounce_soft_store, 0);
> +static IIO_DEVICE_ATTR(debounce_soft_delay, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_delay_show, hi843x_debounce_soft_delay_store, 0);
> +static IIO_DEVICE_ATTR(sensing_mode, S_IRUGO | S_IWUSR,
> + hi843x_sensing_mode_show, hi843x_sensing_mode_store, 0);
> +static IIO_DEVICE_ATTR(test_enable, S_IRUGO | S_IWUSR,
> + hi843x_test_enable_show, hi843x_test_enable_store, 0);
> +static IIO_DEVICE_ATTR(test_mode, S_IRUGO | S_IWUSR,
> + hi843x_test_mode_show, hi843x_test_mode_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gohys_show, hi843x_threshold_gohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gocval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gocval_show, hi843x_threshold_gocval_store, 0);
> +static IIO_DEVICE_ATTR(threshold_sohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_sohys_show, hi843x_threshold_sohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_socval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_socval_show, hi843x_threshold_socval_store, 0);
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Add TI CDCE925 I2C controlled clock synthesizer driver
On Tue, 2015-06-02 at 10:43 +0200, Mike Looijmans wrote: > Out of curiousity, I did try a compile on the x86 host, but couldn't select > the driver because it depends on CONFIG_OF, so I just compiled if for the ARM > target to verify that it still compiles in kernel 4.1. How did you manage to > compile the driver on the x86? With this hack: make drivers/clk/clk-cdce925.o (I thought this is documented somewhere, but I can't actually find the documentation. I guess it's just a feature of make.) It bypasses much of the build infrastructure, but comes in handy when you quickly want to know whether something can actually be build. Eg, you see some code and wonder "how can this build?" (Sometimes this requires setting CFLAGS on the make command line, or adding things like CONFIG_WHATEVER=y to the command line to get sane results, for some value of sane. By that time I usually give up.) Downside is, of course, that you can build things outside of the supported configurations. So it's best to state clearly that one built something using this hack. I sinned against that rule, but apparently got lucky, because this driver is not 32 bits only! Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Add TI CDCE925 I2C controlled clock synthesizer driver
On Mon, 2015-06-01 at 12:13 +0200, Mike Looijmans wrote:
> --- /dev/null
> +++ b/drivers/clk/clk-cdce925.c
> +static int cdce925_regmap_i2c_write(
> + void *context, const void *data, size_t count)
> + dev_dbg(&i2c->dev, "%s(%u) %#x %#x\n", __func__, count,
> + reg_data[0], reg_data[1]);
For some silly reason (ie, I mistakenly thought I spotted an issue) I
did a quick build of this file. That triggered some noise on x86_64.
Excerpt:
drivers/clk/clk-cdce925.c: In function ‘cdce925_regmap_i2c_write’:
include/linux/dynamic_debug.h:64:16: warning: format ‘%u’ expects argument of
type ‘unsigned int’, but argument 5 has type ‘size_t’ [-Wformat=]
static struct _ddebug __aligned(8) \
^
[...]
drivers/clk/clk-cdce925.c:505:2: note: in expansion of macro ‘dev_dbg’
dev_dbg(&i2c->dev, "%s(%u) %#x %#x\n", __func__, count,
^
A quick look at Documentation/printk-formats.txt suggested this (very
lightly tested) fix:
@@ -502,7 +502,7 @@ static int cdce925_regmap_i2c_write(
reg_data[0] = CDCE925_I2C_COMMAND_BYTE_TRANSFER | ((u8 *)data)[0];
reg_data[1] = ((u8 *)data)[1];
- dev_dbg(&i2c->dev, "%s(%u) %#x %#x\n", __func__, count,
+ dev_dbg(&i2c->dev, "%s(%zu) %#x %#x\n", __func__, count,
reg_data[0], reg_data[1]);
ret = i2c_master_send(i2c, reg_data, count);
> +static int cdce925_regmap_i2c_read(void *context,
> +const void *reg, size_t reg_size, void *val, size_t val_size)
> + dev_dbg(&i2c->dev, "%s(%u, %u) %#x %#x\n", __func__,
> + reg_size, val_size, reg_data[0], *((u8 *)val));
Likewise:
@@ -547,7 +547,7 @@ static int cdce925_regmap_i2c_read(void *context,
ret = i2c_transfer(i2c->adapter, xfer, 2);
if (likely(ret == 2)) {
- dev_dbg(&i2c->dev, "%s(%u, %u) %#x %#x\n", __func__,
+ dev_dbg(&i2c->dev, "%s(%zu, %zu) %#x %#x\n", __func__,
reg_size, val_size, reg_data[0], *((u8 *)val));
return 0;
} else if (ret < 0)
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver
On Fri, 2015-05-29 at 12:42 -0700, Eric Anholt wrote: > This gives us a function for making mailbox property channel requests > of the firmware, which is most notable in that it will let us get and > set clock rates. > > v2: Drop power-domains stuff for now since we don't have the driver > core support to make it useful. Move to drivers/firmware/. > Capitalize the enums. De-global the firmware variable. Use the > firmware device to allocate our DMA buffer, so that the dma-ranges > DT property gets respected. Simplify the property tag transaction > interface even more, leaving a multi-tag interface still > available. For conciseness, rename "raspberrypi" to "rpi" on all > functions/enums/structs, and the "firmware" variable to "fw". > Print when the driver is probed successfully, since debugging > -EPROBE_DEFER handling is such a big part of bcm2835 development. > Drop -EBUSY mailbox handling since the mailbox core has been fixed > to return -EPROBE_DEFER in -next. > > v3: Use kernel-doc style for big comments (from Noralf), drop stale > comment, use "dev" when freeing DMA as well. > > v4: Move description comment into copyright comment, drop a dead > initialization of "ret", and print the firmware revision at probe > time. > > v5: Rename the compatible to "raspberrypi,bcm2835-firmware", move > include to not say "property", add functions to get struct > rpi_firmware from the device_node and put when done, make property > functions take the rpi_firmware instead and never return > -EPROBE_DEFER, put the driver under its own RASPBERRYPI_FIRMWARE > Kconfig. > > v6: Drop the try_module_get/module_put stuff, since all clients will > be referencing our symbols in order to call those functions, > anyway. Fix the kerneldoc comments for the changes in v5. (This style of commit explanation is getting quite common. I must say I rather dislike it. I think people should just update the entire commit explanation when needed, and not simply paste any changes at the end of it, thereby forcing the the reader to determine which older parts are actually overruled by newer parts. Besides, many, or maybe even most, of the changes are really not interesting enough to keep in the commit explanation.) > --- /dev/null > +++ b/drivers/firmware/raspberrypi.c > +EXPORT_SYMBOL_GPL(rpi_firmware_property_list); > +EXPORT_SYMBOL_GPL(rpi_firmware_property); A patch that uses these exports hit lkml recently: https://lkml.org/lkml/2015/5/28/596 . > +EXPORT_SYMBOL_GPL(rpi_firmware_get); But I didn't spot a (recent) patch that uses this export. Is it queued somewhere? Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] net: dsa: add new driver for ar8xxx family
Just a nit: a license mismatch.
On Thu, 2015-05-28 at 18:42 -0700, Mathieu Olivari wrote:
> --- /dev/null
> +++ b/drivers/net/dsa/ar8xxx.c
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And this states, according to include/linux/module.h, that the license
is GPL v2 or later. So I think that either the comment at the top of
this file or the ident used in the MODULE_LICENSE() macro needs to
change.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] soc: Add driver for Freescale Vybrid Platform
On Wed, 2015-05-27 at 18:37 +0530, maitysancha...@gmail.com wrote: > > > +module_platform_driver(vf610_soc_driver); > > > > (The series starting at https://lkml.org/lkml/2015/5/10/131 would allow > > to use builtin_platform_driver() for built-in only code.) > > Thanks for bringing this to my attention. I am subscribed to the mailing > list however this skipped me. > However that series has not been merged yet, so I can't use builtin_* > yet? Correct. If that series get merged, which is not certain at all, you might consider changing this. Probably no one will notice if you don't. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] soc: Add driver for Freescale Vybrid Platform
On Tue, 2015-05-26 at 17:06 +0530, Sanchayan Maity wrote:
> --- /dev/null
> +++ b/drivers/soc/fsl/Kconfig
> +config SOC_VF610
> +bool "SoC bus device for the Freescale Vybrid platform"
> +select SOC_BUS
> +help
> + Include support for the SoC bus on the Freescale Vybrid platform
> + providing some sysfs information about the module variant.
> \ No newline at end of file
(That review comment is courtesy of git.)
> --- /dev/null
> +++ b/drivers/soc/fsl/Makefile
> +obj-$(CONFIG_SOC_VF610) += soc-vf610.o
> --- /dev/null
> +++ b/drivers/soc/fsl/soc-vf610.c
> +MODULE_DEVICE_TABLE(of, vf610_soc_bus_match);
> +module_platform_driver(vf610_soc_driver);
(The series starting at https://lkml.org/lkml/2015/5/10/131 would allow
to use builtin_platform_driver() for built-in only code.)
> +MODULE_DESCRIPTION("Freescale VF610 SoC bus driver");
> +MODULE_LICENSE("GPL v2");
I think soc-vf610.o can only be built-in. But its code contains a few
module specific macros. Was it perhaps intended for SOC_VF610 to be
tristate?
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] stmmac: add ipq806x glue layer
Just a nit: a license mismatch.
On Tue, 2015-05-26 at 12:27 -0700, Mathieu Olivari wrote:
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
I have no idea which license this is.
> +MODULE_LICENSE("GPL");
But I do know that it's not GPL v2 or later (see
include/linux/module.h).
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] drivers: ata: add support for Ceva sata host controller
[Adding Bjorn.]
> On Thu, May 21, 2015 at 09:27:05AM +0530, Suneel Garapati wrote:
> > --- /dev/null
> > +++ b/drivers/ata/ahci_ceva.c
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > with
> > + * this program. If not, see <http://www.gnu.org/licenses/>.
> > +MODULE_LICENSE("GPLv2");
You meant "GPL v2".
"GPLv2" is not one of the idents that pass license_is_gpl_compatible().
Loading a module with that ident should trigger a warning and taint the
kernel. See linux/module.h (or linux/license.h) for acceptable idents.
This is a bit of a gotcha. A patch that would have helped here was
posted in https://lkml.org/lkml/2015/4/7/824 . I don't know why that
patch didn't make it into checkpatch.pl. Bjorn?
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] soc: Mediatek: Add SCPSYS power domain driver
On Wed, 2015-05-20 at 16:18 +0200, Sascha Hauer wrote:
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> +config MTK_SCPSYS
> + bool "MediaTek SCPSYS Support"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + select REGMAP
> + select MTK_INFRACFG
> + help
> + Say yes here to add support for the MediaTek SCPSYS power domain
> + driver.
> +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> +#include
> +MODULE_DEVICE_TABLE(of, of_scpsys_match_tbl);
> +
> +static struct platform_driver scpsys_drv = {
> + .owner = THIS_MODULE,
> +};
> +
> +module_platform_driver_probe(scpsys_drv, scpsys_probe);
(A patch was recently submitted that would allow built-in only code to
use builtin_platform_driver_probe(), see
https://lkml.org/lkml/2015/5/10/125 .)
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_DESCRIPTION("MediaTek MT8173 scpsys driver");
> +MODULE_LICENSE("GPL v2");
MTK_SCPSYS was changed from tristate to bool in this version. As
mtk-scpsys.o can now only be built-in I think the above module specific
macros can safely be dropped. Probably ditto for the module.h include.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] soc: mediatek: Add infracfg misc driver support
Just a nit: an unneeded macro.
On Wed, 2015-05-20 at 16:18 +0200, Sascha Hauer wrote:
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> +config MTK_INFRACFG
> + bool "MediaTek INFRACFG Support"
> + depends on ARCH_MEDIATEK
> + select REGMAP
> + help
> + Say yes here to add support for the MediaTek INFRACFG controller. The
> + INFRACFG controller contains various infrastructure registers not
> + directly associated to any device.
> +
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-infracfg.c
> +#include
> +MODULE_LICENSE("GPL v2");
MTK_INFRACFG was changed from tristate to bool in this version.
Note that for built-in code MODULE_LICENSE() will be effectively
preprocessed away. So you can drop that macro, and the include of
linux/module.h too. (I did a quick compile to see if nothing else
requires module.h, and it compiled cleanly without that include.)
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Regulator: add driver for Freescale MC34708
Just a nit: a license mismatch.
On Tue, 2015-04-28 at 16:17 +0200, Martin Fuzzey wrote:
> --- /dev/null
> +++ b/drivers/regulator/mc34708-regulator.c
> + * 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 states the license is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And, according to inlcude/linux/module.h, this states the license is
(just) GPL v2. So I think either to comment at the top of this file or
the ident used in the MODULE_LICENSE() macro should be changed.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol
Just one nit: a license mismatch.
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> --- /dev/null
> +++ b/drivers/mailbox/scpi_protocol.c
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program. If not, see <http://www.gnu.org/licenses/>.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro should be changed.
Likewise for 2/4 and 4/4.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] power_supply: Add support for Richtek rt9455 battery charger
Just a single nit: a license mismatch.
On Fri, 2015-04-24 at 10:57 +0300, Anda-Maria Nicolae wrote:
> --- /dev/null
> +++ b/drivers/power/rt9455_charger.c
> + * 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.
This states the license is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And, according to include/linux/module.h, this states the license is
(only) GPL v2. So I think either the license comment or the ident used
in the MODULE_LICENSE() macro should be changed.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH V2 1/2] input: misc: da9063: OnKey driver
On Fri, 2015-04-24 at 13:45 +, Opensource [Steve Twiss] wrote: > That seems to be a fairly common mistake in the kernel. It's an easy mistake to make. And as long as people pick an ident that passes license_is_gpl_compatible() the module will build and load just fine. > When I did a > straw-poll, around 10% of files came up with "GPL v2" and contained > "any later version" text. That seems plausible. I'm inclined to think the difference between "GPL" and "GPL v2" is mainly an accident of history (see https://lkml.org/lkml/2015/3/11/188 for some background). But apparently were stuck with these two idents. Anyhow, in the last two months I've not come up with a better plan than regularly check patches for mismatches like the one you made. If you have a better idea, I'll be all ears. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/4] mailbox: add support for APM X-Gene platform mailbox driver
On Wed, 2015-04-22 at 15:18 -0700, Feng Kan wrote:
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-xgene-slimpro.c
> +static struct platform_driver slimpro_mbox_driver = {
> + .probe = slimpro_mbox_probe,
> + .remove = slimpro_mbox_remove,
> + .driver = {
> + .name = "xgene-slimpro-mbox",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(slimpro_of_match),
> + },
> +};
> +
> +static int __init slimpro_mbox_init(void)
> +{
> + return platform_driver_register(&slimpro_mbox_driver);
> +}
> +
> +subsys_initcall(slimpro_mbox_init);
After a quick scan of this driver you'd expect something like
static void __exit slimpro_mbox_exit(void)
{
platform_driver_unregister(&slimpro_mbox_driver);
}
module_exit(slimpro_mbox_exit);
too here. At least, I do. Is there some non-obvious (to me) reason this
driver can't be unloaded if it is built as a module?
> +MODULE_DESCRIPTION("APM X-Gene SLIMpro Mailbox Driver");
> +MODULE_LICENSE("GPL");
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] ASoC: wm8998: Initial WM8998 codec driver
A license mismatch is all I spotted.
On Tue, 2015-04-21 at 13:33 +0100, Richard Fitzgerald wrote:
> --- /dev/null
> +++ b/sound/soc/codecs/wm8998.c
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro needs to change.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver
On Mon, 2015-04-20 at 09:17 -0500, Josh Cartwright wrote: > On Mon, Apr 20, 2015 at 05:27:26PM +0800, pi-cheng.chen wrote: > > --- a/drivers/cpufreq/Kconfig.arm > > +++ b/drivers/cpufreq/Kconfig.arm > > > +config ARM_MT8173_CPUFREQ > > + bool "Mediatek MT8173 CPUFreq support" > > + depends on ARCH_MEDIATEK && REGULATOR > > I think you want to 'select REGULATOR' here; because REGULATOR isn't > a user-visible option. REGULATOR is user visible, at least in next-20150420. > > + help > > + This adds the CPUFreq driver support for Mediatek MT8173 SoC. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver
On Mon, 2015-04-20 at 17:27 +0800, pi-cheng.chen wrote: > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > +config ARM_MT8173_CPUFREQ > + bool "Mediatek MT8173 CPUFreq support" > + depends on ARCH_MEDIATEK && REGULATOR > + help > + This adds the CPUFreq driver support for Mediatek MT8173 SoC. > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o ARM_MT8173_CPUFREQ is a bool symbol, so mt8173-cpufreq.o will never be part of a module. (If that's incorrect you can stop reading here.) > --- /dev/null > +++ b/drivers/cpufreq/mt8173-cpufreq.c > +#include I guess this include is not needed. And, for what it's worth, make ARCH=arm CROSS_COMPILE=arm-linux-gnu- drivers/cpufreq/mt8173-cpufreq.o compiles silently even if it's dropped. > +module_init(mt8173_cpufreq_driver_init); According to include/linux/init.h, for built-in only code, this is equivalent to device_initcall([...]). And two runs of make ARCH=arm CROSS_COMPILE=arm-linux-gnu- drivers/cpufreq/mt8173-cpufreq.i confirm that. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] MIPS: ath79: Add basic device tree support
On Sat, 2015-04-18 at 18:31 +0200, Alban wrote: > On Sat, 18 Apr 2015 09:05:52 +0200 > Paul Bolle wrote: > > > On Fri, 2015-04-17 at 16:24 +0200, Alban Bedel wrote: > > > > > --- a/arch/mips/ath79/Kconfig > > > +++ b/arch/mips/ath79/Kconfig > > > > > > +choice > > > + prompt "Builtin devicetree selection" > > > + default DTB_ATH79_NONE > > > + help > > > + Select the devicetree. > > > + > > > + config DTB_ATH79_NONE > > > + bool "None" > > > +endchoice > > > > This adds a choice block with one config entry. So what this achieves > > is that DTB_ATH79_NONE will always be 'y', right? Besides I didn't > > notice on a user of CONFIG_DTB_ATH79_NONE. So as far as I can see > > this choice has no effect on the build. > > > > Why was this choice entry added? > > This menu is for boards that need a built-in DTB, as MIPS currently > doesn't support appended DTBs it is the only way to load a DTB on > boards stuck with a broken boot loader. But as no board has been added > yet it only contain the entry for boards that don't need a built-in > DTB, which do nothing as you pointed out. > > A board is then added in this menu in patch 14, for some reasons git > send-email didn't send the whole serie at once :( I saw 14/14 fly by now. (And I saw 12/12 of v2, which basically replaces 14/14.) That makes things clear. Perhaps, if you're going to send a v3, you might move this change into 12/12. At least, I see little reason to add this choice block in two separate steps in this series. (Side note: you _might_ be able to drop config DTB_ATH79_NONE entirely if you also replace default DTB_ATH79_NONE with optional optional is a little used option for choice blocks. I haven't tested this, so this suggestion could end up wasting your time. And even then this all looks like a complicated way to set just DTB_TL_WR1043ND_V1. I suppose you expect to add things to this choice block in the future.) Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 2/3] power-domain: rockchip: add power domain driver
On Sun, 2015-04-19 at 18:44 +0800, Caesar Wang wrote:
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
PM_GENERIC_DOMAINS is a bool symbol, so pm_domains.o can never be part
of a module. Is that correct?
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/pm_domains.c
> +#include
This include could probably be dropped.
> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
> + {
> + .compatible = "rockchip,rk3288-power-controller",
> + .data = (void *)&rk3288_pmu,
> + },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, rockchip_pm_domain_dt_match);
The MODULE_DEVICE_TABLE() macro will be preprocessed away for built-in
code, according to include/linux/module.h.
> +static struct platform_driver rockchip_pm_domain_driver = {
> + .probe = rockchip_pm_domain_probe,
> + .driver = {
> + .name = "rockchip-pm-domain",
> + .owner = THIS_MODULE,
According to include/linux/export.h THIS_MODULE is equivalent to NULL
for built-in code. So I think this line is not needed.
> + .of_match_table = rockchip_pm_domain_dt_match,
> + /*
> + * We can't forcibly eject devices form power domain,
> + * so we can't really remove power domains once they
> + * were added.
> + */
> + .suppress_bind_attrs = true,
> + },
> +};
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1 2/6] regulator: da9062: DA9062 regulator driver
I spotted only a license mismatch.
On Fri, 2015-04-17 at 15:23 +0100, S Twiss wrote:
> + * 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.
This states the license is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And, according to include/linux/module.h, this states the license is
just GPL v2. So I think either the license comment or the license ident
used in the MODULE_LICENSE() macro needs to change.
The same mismatch can be found in 3/6, 4/6, and 5/6.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1 1/6] mfd: da9062: DA9062 MFD core driver
On Fri, 2015-04-17 at 15:23 +0100, S Twiss wrote:
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> +config MFD_DA9062
> + bool "Dialog Semiconductor DA9062 PMIC Support"
> + select MFD_CORE
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + depends on I2C=y
> + help
> + Say yes here for support for the Dialog Semiconductor DA9062 PMIC.
> + This includes the I2C driver and core APIs.
> + Additional drivers must be enabled in order to use the functionality
> + of the device.
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> +obj-$(CONFIG_MFD_DA9062) += da9062-core.o
MFD_DA9062 is a bool symbol. So da9062-core.o can never be part of a
module, right?
> --- /dev/null
> +++ b/drivers/mfd/da9062-core.c
> +#include
So you might not need this include.
> +MODULE_DEVICE_TABLE(of, da9062_dt_ids);
This macro will be preprocessed away for built-in code, according to
include/linux/module.h.
> +MODULE_DEVICE_TABLE(i2c, da9062_i2c_id);
Ditto.
> +static struct i2c_driver da9062_i2c_driver = {
> + .driver = {
> + .name = "da9062",
> + .of_match_table = of_match_ptr(da9062_dt_ids),
> + },
> + .probe= da9062_i2c_probe,
> + .remove = da9062_i2c_remove,
> + .id_table = da9062_i2c_id,
> +};
> +
> +module_i2c_driver(da9062_i2c_driver);
After looking at a few levels of #defines I think this is equivalent to
i2c_register_driver(NULL, da9062_i2c_driver);
for built-in code. Did I grep that right?
> +MODULE_DESCRIPTION("CORE device driver for Dialog DA9062");
> +MODULE_AUTHOR("S Twiss ");
> +MODULE_LICENSE("GPL v2");
These macros will be effectively preprocessed away for built-in only
code.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH V2 1/2] input: misc: da9063: OnKey driver
There's still a license mismatch left (it probably got lost in the noise
when I finally noticed that the header comment mentioned the LGPL in
V1).
On Fri, 2015-04-17 at 13:03 +0100, S Twiss wrote:
> --- /dev/null
> +++ b/drivers/input/misc/da9063-onkey.c
> + * 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.
This states the license is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And, according to include/linux/module.h, this states the license is
just GPL v2. So I think either the license comment or the license ident
used in the MODULE_LICENSE() macro needs to change.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/2] tee: generic TEE subsystem
On Fri, 2015-04-17 at 22:07 +0200, Arnd Bergmann wrote:
> On Friday 17 April 2015 09:50:56 Jens Wiklander wrote:
> > +static const struct file_operations tee_fops = {
> > + .owner = THIS_MODULE,
> > + .open = tee_open,
> > + .release = tee_release,
> > + .unlocked_ioctl = tee_ioctl
> > +};
>
> Add a .compat_ioctl function, to make it work on arm64 as well.
> If you got all the data structures right, you can use the same
> tee_ioctl function.
>
> Minor nit: put a comma behind the last line in each struct initialization
> to make it easier to add another callback.
And another nit: this is built-in only code, right? So, according to
include/linux/export.h, THIS_MODULE will be basically equivalent to
NULL. So I think you can drop that line.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] MIPS: ath79: Add basic device tree support
On Fri, 2015-04-17 at 16:24 +0200, Alban Bedel wrote: > --- a/arch/mips/ath79/Kconfig > +++ b/arch/mips/ath79/Kconfig > > +choice > + prompt "Builtin devicetree selection" > + default DTB_ATH79_NONE > + help > + Select the devicetree. > + > + config DTB_ATH79_NONE > + bool "None" > +endchoice This adds a choice block with one config entry. So what this achieves is that DTB_ATH79_NONE will always be 'y', right? Besides I didn't notice on a user of CONFIG_DTB_ATH79_NONE. So as far as I can see this choice has no effect on the build. Why was this choice entry added? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] clocksource: st_lpc: Add LPC timer as a clocksource.
On Fri, 2015-04-17 at 11:50 +0100, Peter Griffin wrote:
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> +config CLKSRC_ST_LPC_CLOCK
> + bool
> + depends on ARCH_STI
> + select CLKSRC_OF if OF
> + help
> + Enable this option to use the Low Power controller timer
> + as clock source.
> +
> +config CLKSRC_ST_LPC_TIMER_SCHED_CLOCK
> + bool
> + depends on ST_LPC_CLOCK
It looks like you meant
depends on CLKSRC_ST_LPC_CLOCK
> + default y
> + help
> + Use Low Power controller timer clock source as sched_clock
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> +obj-$(CONFIG_CLKSRC_ST_LPC_CLOCK)+= st_lpc.o
> --- /dev/null
> +++ b/drivers/clocksource/st_lpc.c
> +#ifdef CONFIG_CLKSRC_LPC_TIMER_SCHED_CLOCK
#ifdef CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK here?
> +static u64 notrace st_lpc_sched_clock_read(void)
> +{
> + return st_lpc_counter_read();
> +}
> +#endif
> +#ifdef CONFIG_CLKSRC_LPC_TIMER_SCHED_CLOCK
Again, #ifdef CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK here?
> + sched_clock_register(st_lpc_sched_clock_read, 64, rate);
> +#endif
Assuming the above suggestions are correct: checkkconfigsymbols.py, as
shipped in linux-next, helps detect stuff like this. See
scripts/checkkconfigsymbols.py --help.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/3] nand: pl353: Add basic driver for arm pl353 smc nand interface
On Mon, 2015-04-13 at 21:42 +0530, Punnaiah Choudary Kalluri wrote:
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> +obj-$(CONFIG_MTD_NAND_PL353) += pl353_nand.o
(I think pl353_nand.o can be part of a module. If that's incorrect, you
can stop reading here.)
> --- /dev/null
> +++ b/drivers/mtd/nand/pl353_nand.c
> + * This program is free software; you can redistribute it and/or modify it
> under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
This states the license of this driver is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And according to include/linux/module.h this states the license is GPL
v2. So either the comment at the top of this file or the license ident
used in the MODULE_LICENSE() macro needs to change.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/2] memory: pl353: Add driver for arm pl353 static memory controller
On Mon, 2015-04-13 at 21:41 +0530, Punnaiah Choudary Kalluri wrote:
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> +config PL353_SMC
> + bool "ARM PL353 Static Memory Controller (SMC) driver"
> + depends on ARM
> + help
> + This driver is for the ARM PL353 Static Memory Controller (SMC)
> + module.
This adds a bool symbol.
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> +obj-$(CONFIG_PL353_SMC) += pl353-smc.o
Which means pl353-smc.o can never be part of a module, right?
(If that's not right you can stop reading here.)
> --- /dev/null
> +++ b/drivers/memory/pl353-smc.c
> + * 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.
This states the license is GPL v2 or later.
> +#include
I wonder whether this include is needed, since this is built-in only
code.
> +MODULE_DEVICE_TABLE(of, pl353_smc_of_match);
According to include/linux/module.h this will be preprocessed away for
built-in code.
> +static struct platform_driver pl353_smc_driver = {
> + .probe = pl353_smc_probe,
> + .remove = pl353_smc_remove,
> + .driver = {
> + .name = "pl353-smc",
> + .owner = THIS_MODULE,
THIS_MODULE will be equivalent to NULL for built-in code, according to
include/linux/export.h.
> + .pm = &pl353_smc_dev_pm_ops,
> + .of_match_table = pl353_smc_of_match,
> + },
> +};
> +module_platform_driver(pl353_smc_driver);
Speaking from memory: for built-in only code this is equivalent to
having a wrapper that only does
register_platform_driver(&pl353_smc_driver);
and mark that wrapper with device_initcall().
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("ARM PL353 SMC Driver");
> +MODULE_LICENSE("GPL v2");
For built-in only code these macros will be effectively preprocessed
away.
(Would you make PL353_SMC a tristate symbol then you should note that
according to include/linux/module.h "GPL" is the license ident that
matches the license stated in the comment at the top of this file.)
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
On Mon, 2015-04-13 at 21:17 +0800, Bintian wrote: > Thank you very much for code review and testing on arm! s/testing/crosscompiling for/ > On 2015/4/13 19:56, Paul Bolle wrote: > > I wonder what checkpatch had to say about the length of the lines seen > > in this patch. > > Yes, I ran this script before sending out this patch set, it reports > warnings about "line over 80 characters ", but I find it's easier to > read than shrinking one line to several lines, so just keep it, do I > need to fix it? I'll leave that to the maintainers of drivers/clk/. (I don't care deeply. So on second thought I should not have made that remark.) Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
ARRAY_SIZE(hi6220_div_clks_sys), clk_data);
> +
> + if (!clk_data_ao)
> + return;
> +
> + /* enable high speed clock on UART1 mux */
> + clk_set_parent(clk_data->clk_data.clks[HI6220_UART1_SRC],
> + clk_data_ao->clk_data.clks[HI6220_150M]);
> +}
> +CLK_OF_DECLARE(hi6220_clk_sys, "hisilicon,sysctrl", hi6220_clk_sys_init);
> +
> +
> +/* clocks in media controller */
> +static const char *clk_1000_1200_src[] __initconst = { "pll_gpu_gate",
> "media_syspll_src", };
> +static const char *clk_1440_1200_src[] __initconst = { "media_syspll_src",
> "media_pll_src", };
> +static const char *clk_1000_1440_src[] __initconst = { "pll_gpu_gate",
> "media_pll_src", };
> +
> +static struct hisi_gate_clock hi6220_separated_gate_clks_media[] __initdata
> = {
> + { HI6220_DSI_PCLK, "dsi_pclk", "vpucodec",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 0, 0, },
> + { HI6220_G3D_PCLK, "g3d_pclk", "vpucodec",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 1, 0, },
> + { HI6220_ACLK_CODEC_VPU, "aclk_codec_vpu", "ade_core_src",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 3, 0, },
> + { HI6220_ISP_SCLK, "isp_sclk", "isp_sclk_src",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 5, 0, },
> + { HI6220_ADE_CORE, "ade_core", "ade_core_src",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 6, 0, },
> + { HI6220_MED_MMU,"media_mmu","mmu_clk",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 8, 0, },
> + { HI6220_CFG_CSI4PHY,"cfg_csi4phy", "clk_tcxo",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 9, 0, },
> + { HI6220_CFG_CSI2PHY,"cfg_csi2phy", "clk_tcxo",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 10, 0, },
> + { HI6220_ISP_SCLK_GATE, "isp_sclk_gate","media_pll_src",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 11, 0, },
> + { HI6220_ISP_SCLK_GATE1, "isp_sclk_gate1", "media_pll_src",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 12, 0, },
> + { HI6220_ADE_CORE_GATE, "ade_core_gate","media_pll_src",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 14, 0, },
> + { HI6220_CODEC_VPU_GATE, "codec_vpu_gate", "clk_1000_1440",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 15, 0, },
> + { HI6220_MED_SYSPLL, "media_syspll_src", "media_syspll",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x520, 17, 0, },
> +};
> +
> +static struct hisi_mux_clock hi6220_mux_clks_media[] __initdata = {
> + { HI6220_1440_1200, "clk_1440_1200", clk_1440_1200_src,
> ARRAY_SIZE(clk_1440_1200_src), CLK_SET_RATE_PARENT, 0x51c, 0, 1, 0, },
> + { HI6220_1000_1200, "clk_1000_1200", clk_1000_1200_src,
> ARRAY_SIZE(clk_1000_1200_src), CLK_SET_RATE_PARENT, 0x51c, 1, 1, 0, },
> + { HI6220_1000_1440, "clk_1000_1440", clk_1000_1440_src,
> ARRAY_SIZE(clk_1000_1440_src), CLK_SET_RATE_PARENT, 0x51c, 6, 1, 0, },
> +};
> +
> +static struct hi6220_divider_clock hi6220_div_clks_media[] __initdata = {
> + { HI6220_CODEC_JPEG,"codec_jpeg_aclk", "media_pll_src",
> CLK_SET_RATE_PARENT, 0xcbc, 0, 4, 23, },
> + { HI6220_ISP_SCLK_SRC, "isp_sclk_src","isp_sclk_gate",
> CLK_SET_RATE_PARENT, 0xcbc, 8, 4, 15, },
> + { HI6220_ISP_SCLK1, "isp_sclk1", "isp_sclk_gate1",
> CLK_SET_RATE_PARENT, 0xcbc, 24, 4, 31, },
> + { HI6220_ADE_CORE_SRC, "ade_core_src","ade_core_gate",
> CLK_SET_RATE_PARENT, 0xcc0, 16, 3, 23, },
> + { HI6220_ADE_PIX_SRC, "ade_pix_src", "clk_1440_1200",
> CLK_SET_RATE_PARENT, 0xcc0, 24, 6, 31, },
> + { HI6220_G3D_CLK, "g3d_clk", "clk_1000_1200",
> CLK_SET_RATE_PARENT, 0xcc4, 8, 4, 15, },
> + { HI6220_CODEC_VPU_SRC, "codec_vpu_src", "codec_vpu_gate",
> CLK_SET_RATE_PARENT, 0xcc4, 24, 6, 31, },
> +};
> +
> +static void __init hi6220_clk_media_init(struct device_node *np)
> +{
> + struct hisi_clock_data *clk_data;
> +
> + clk_data = hisi_clk_init(np, HI6220_MEDIA_NR_CLKS);
> + if (!clk_data)
> + return;
> +
> + hisi_clk_register_gate_sep(hi6220_separated_gate_clks_media,
> + ARRAY_SIZE(hi6220_separated_gate_clks_media),
> clk_data);
> +
> + hisi_clk_register_mux(hi6220_mux_clks_media,
> + ARRAY_SIZE(hi6220_mux_clks_media), clk_data);
> +
> + hi6220_clk_register_divider(hi6220_div_clks_media,
> + ARRAY_SIZE(hi6220_div_clks_media), clk_data);
> +}
> +CLK_OF_DECLARE(hi6220_clk_media, "hisilicon,mediactrl",
> hi6220_clk_media_init);
> +
> +
> +/* clocks in pmctrl */
> +static struct hisi_gate_clock hi6220_gate_clks_power[] __initdata = {
> + { HI6220_PLL_GPU_GATE, "pll_gpu_gate", "gpupll",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x8, 0, 0, },
> + { HI6220_PLL1_DDR_GATE, "pll1_ddr_gate", "ddrpll1",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x10, 0, 0, },
> + { HI6220_PLL_DDR_GATE, "pll_ddr_gate", "ddrpll0",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x18, 0, 0, },
> + { HI6220_PLL_MEDIA_GATE, "pll_media_gate", "media_pll",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x38, 0, 0, },
> + { HI6220_PLL0_BBP_GATE, "pll0_bbp_gate", "bbppll0",
> CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x48, 0, 0, },
> +};
> +
> +static struct hi6220_divider_clock hi6220_div_clks_power[] __initdata = {
> + { HI6220_DDRC_SRC, "ddrc_src", "ddr_sel_src", CLK_SET_RATE_PARENT,
> 0x5a8, 0, 4, 0, },
> + { HI6220_DDRC_AXI1, "ddrc_axi1", "ddrc_src",CLK_SET_RATE_PARENT,
> 0x5a8, 8, 2, 0, },
> +};
> +
> +static void __init hi6220_clk_power_init(struct device_node *np)
> +{
> + struct hisi_clock_data *clk_data;
> +
> + clk_data = hisi_clk_init(np, HI6220_POWER_NR_CLKS);
> + if (!clk_data)
> + return;
> +
> + hisi_clk_register_gate(hi6220_gate_clks_power,
> + ARRAY_SIZE(hi6220_gate_clks_power), clk_data);
> +
> + hi6220_clk_register_divider(hi6220_div_clks_power,
> + ARRAY_SIZE(hi6220_div_clks_power), clk_data);
> +}
> +CLK_OF_DECLARE(hi6220_clk_power, "hisilicon,pmctrl", hi6220_clk_power_init);
There's nothing module specific in this file. And the lack of a
MODULE_LICENSE() macro is also telling. If this was built as a module
loading that module - ignoring the undefined symbols - would taint the
kernel.
It seems to me that COMMON_CLK_HI6220 is meant to be a bool symbol.
Paul Bolle
I wonder what checkpatch had to say about the length of the lines seen
in this patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] PCI: st: Provide support for the sti PCIe controller
Something I didn't spot in my first look at this patch.
On Fri, 2015-04-10 at 11:12 +0200, Gabriel FERNANDEZ wrote:
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
>
> +config PCI_ST
> + bool "ST PCIe controller"
> + depends on ARCH_STI || (ARM && COMPILE_TEST)
> + select PCIE_DW
> + help
> + Enable PCIe controller support on ST Socs. This controller is based
> + on Designware hardware and therefore the driver re-uses the
> + Designware core functions to implement the driver.
You can't have ARCH_STI without ARM, so ARM will always be set if this
driver is enabled. Correct?
> --- /dev/null
> +++ b/drivers/pci/host/pci-st.c
> + if (IS_ENABLED(CONFIG_ARM)) {
> + /*
> + * We have to hook the abort handler so that we can intercept
> + * bus errors when doing config read/write that return UR,
> + * which is flagged up as a bus error
> + */
> + hook_fault_code(16+6, st_pcie_abort_handler, SIGBUS, 0,
> + "imprecise external abort");
> + }
So, unless I'm missing something obvious here, IS_ENABLED(CONFIG_ARM)
will always evaluate to 1. Can't that test be dropped?
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] axs_nand - add driver for NAND controller used on Synopsys AXS dev boards
What happened to 1/2? Whatever that was, I spotted a license mismatch in
this 2/2.
On Wed, 2015-04-08 at 11:57 +0300, Alexey Brodkin wrote:
> --- /dev/null
> +++ b/drivers/mtd/nand/axs_nand.c
> @@ -0,0 +1,412 @@
> +/*
> + * Copyright (C) 2014 Synopsys, Inc. (www.synopsys.com)
> + *
> + * Driver for NAND controller on Synopsys AXS development board.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License v2. See the file COPYING in the main directory of this archive for
> + * more details.
> + */
This states the license of this file is GPL v2.
> +MODULE_LICENSE("GPL");
According to include/linux/module.h this states the license is GPL v2 or
later.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1 1/2] input: misc: da9063: OnKey driver
On Thu, 2015-04-09 at 10:12 +, Opensource [Steve Twiss] wrote:
> On 09 April 2015 09:48 Paul Bolle wrote:
>
> > > This patch is dependent on PATCH V1 2/2
> > This patch is dependent on PATCH V1 1/2
> > Which makes it all rather confusing.
>
> It is a bit circular, I'll take that out for the next patch
>
> > > +++ b/drivers/input/misc/da9063-onkey.c
> > > @@ -0,0 +1,227 @@
> > > +/* da9063-onkey.c - Onkey device driver for DA9063
> > > + * Copyright (C) 2013 Dialog Semiconductor Ltd.
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Library General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2 of the License, or (at your option) any later version.
> > > + *
> > > + * This library 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
> > > + * Library General Public License for more details.
> > > + */
> >
> > This states the license is GPL v2 or later.
> >
> > > +MODULE_LICENSE("GPL v2");
> >
> > And (according to include/linux/module.h) this states the license is
> > (just) GPL v2.
>
> Ah, I'll fix that for the future.
My reading comprehension is declining. Because I only now noticed that
the comment at the top of this file states the license is LGPL v2 or
later (note the L).
Grepping the tree for "Library General" gave over 100 hits, a few of
them even in drivers/, so I suppose LPGL v2 is fine for the kernel. It
_feels_ a bit odd to use it for a driver, but that might just be me.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1 1/2] input: misc: da9063: OnKey driver
A license mismatch was all I spotted.
On Wed, 2015-04-08 at 15:53 +0100, Steve Twiss wrote:
> From: Steve Twiss
>
> Add OnKey driver support for DA9063
>
> This patch is dependent on PATCH V1 2/2
(But I also spotted this line. And then I checked 2/2, which has:
This patch is dependent on PATCH V1 1/2
Which makes it all rather confusing.
Anyhow, neither patch should need to carry such a line. The 1/2 and 2/2
in the Subject: line should suffice.)
> Signed-off-by: Steve Twiss
>
> ---
> This patch applies against linux-next and v4.0-rc6
> --- /dev/null
> +++ b/drivers/input/misc/da9063-onkey.c
> @@ -0,0 +1,227 @@
> +/* da9063-onkey.c - Onkey device driver for DA9063
> + * Copyright (C) 2013 Dialog Semiconductor Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Library General Public License for more details.
> + */
This states the license is GPL v2 or later.
> +MODULE_LICENSE("GPL v2");
And (according to include/linux/module.h) this states the license is
(just) GPL v2.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] pinctrl: Add driver for Alphascale asm9260 pinctrl
On Mon, 2015-04-06 at 10:38 +0200, Oleksij Rempel wrote: > If you won't to say: "You have a mismatch between header and > MODULE_LICENSE, please make sure it will match." > You saying some thing like this: "I was right last time. Make module > License like I saying." No, that's not what I wrote. > I'm confuse, what is your actual point? Do you trying to prove some thing? My point is that there's a mismatch between the license described in the comment at the top of this file and the ident used in the MODULE_LICENSE() macro. In my comments on v2 I wrote: By the way, you probably want to use "GPL v2" as the license ident [...]. In this v3 I noticed the same mismatch (which was not surprising because you already stated that "GPL" actually did match what's stated at the comment in the top of this file). Therefor I wrote: So only "GPL v2" matches what's found in the comment at top of this file. There now seem to be a few options: - change either the comment at the top of this file or the license ident used in MODULE_LICENSE() to make them actually match; - show that I misread the comment at top of this file; - or show that my reading of module.h is incorrect. (Another option would be a patch that somehow merges the "GPL" and "GPL v2" license idents. That patch would put an end to discussions like the one we're having here. I'm _not_ volunteering to submit it.) Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] pinctrl: Add driver for Alphascale asm9260 pinctrl
A license nit follows.
On Sun, 2015-04-05 at 08:26 +0200, Oleksij Rempel wrote:
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-asm9260.c
> @@ -0,0 +1,733 @@
> +/*
> + * Pinctrl driver for the Alphascale ASM9260 SoC
> + *
> + * Copyright (c) 2014, Oleksij Rempel
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
This states the license of this file is GPL v2.
> +MODULE_LICENSE("GPL");
Your reading of include/linux/module.h (see
https://lkml.org/lkml/2015/4/5/7 ) is incorrect. Because according to
that header "GPL" means
GNU Public License v2 or later
while "GPL v2" means
GNU Public License v2
So only "GPL v2" matches what's found in the comment at top of this
file.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/3] pinctrl: Add Pistachio SoC pin control driver
Hi Andrew, On Tue, 2015-03-31 at 11:37 -0700, Andrew Bresticker wrote: > I have no immediate plans to make this a module, but since the changes > to make it buildable as a module have no overhead (at least I think > they don't!) I'd prefer to leave them in for consistency and to > eliminate any need for these changes in the future. The people reading the code or reviewing the patch might wonder, just like I did, about the mismatch between the code and the Kconfig symbol. Dead code - even if it's easily dealt with during the build - shouldn't be added without a clear benefit. And I don't think consistency or doing now what might be done in the future bring enough benefits. > I'll leave it up to LinusW though. Right, it's Linus decision. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/3] pinctrl: Add Pistachio SoC pin control driver
Hi Andrew, On Tue, 2015-03-31 at 09:56 -0700, Andrew Bresticker wrote: > On Tue, Mar 31, 2015 at 1:10 AM, Paul Bolle wrote: > > The patch adds a mismatch between the Kconfig symbol (a bool) and the > > code (which suggests that a modular build is also possible). > > Nearly all of the pinctrl drivers (with the exception of qcom and > intel) are like this. Could be, I didn't check. Perhaps copy and pasting is to blame. (Copy and pasting appears to me a sensible way to start writing a new driver). > They use a bool Kconfig symbol but they are > written so that they could be built as a module in the future. Did I miss a comment or a remark in the commit explanation that explains this? Anyhow, if that modular future is not expected to be the near future, can you perhaps carry these lines in a branch called, say pinctrl-modular? Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/3] pinctrl: Add Pistachio SoC pin control driver
The patch adds a mismatch between the Kconfig symbol (a bool) and the
code (which suggests that a modular build is also possible).
On Mon, 2015-03-30 at 16:16 -0700, Andrew Bresticker wrote:
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> +config PINCTRL_PISTACHIO
> + def_bool y if MACH_PISTACHIO
This adds a bool symbol.
> + select PINMUX
> + select GENERIC_PINCONF
> + select GPIOLIB_IRQCHIP
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> +obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o
So pinctrl-pistachio.o will never be part of a module.
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-pistachio.c
> +#include
Chances are this include is not needed.
> +static struct pinctrl_desc pistachio_pinctrl_desc = {
> + .name = "pistachio-pinctrl",
> + .pctlops = &pistachio_pinctrl_ops,
> + .pmxops = &pistachio_pinmux_ops,
> + .confops = &pistachio_pinconf_ops,
> + .owner = THIS_MODULE,
According to include/linux/export.h THIS_MODULE is equivalent to NULL,
so this can probably be dropped.
> +};
> +#define GPIO_BANK(_bank, _pin_base, _npins) \
> + { \
> + .gpio_chip = { \
> + .label = "GPIO" #_bank, \
> + .request = pistachio_gpio_request, \
> + .free = pistachio_gpio_free,\
> + .get_direction = pistachio_gpio_get_direction, \
> + .direction_input = pistachio_gpio_direction_input, \
> + .direction_output = pistachio_gpio_direction_output, \
> + .get = pistachio_gpio_get, \
> + .set = pistachio_gpio_set, \
> + .base = _pin_base, \
> + .ngpio = _npins,\
> + .owner = THIS_MODULE, \
Ditto.
> + }, \
> + .irq_chip = { \
> + .name = "GPIO" #_bank, \
> + .irq_startup = pistachio_gpio_irq_startup, \
> + .irq_ack = pistachio_gpio_irq_ack, \
> + .irq_mask = pistachio_gpio_irq_mask,\
> + .irq_unmask = pistachio_gpio_irq_unmask,\
> + .irq_set_type = pistachio_gpio_irq_set_type,\
> + }, \
> + .gpio_range = { \
> + .name = "GPIO" #_bank, \
> + .id = _bank,\
> + .base = _pin_base, \
> + .pin_base = _pin_base, \
> + .npins = _npins,\
> + }, \
> + }
> +MODULE_DEVICE_TABLE(of, pistachio_pinctrl_of_match);
According to include/linux/module.h this will be preprocessed away.
> +module_platform_driver(pistachio_pinctrl_driver);
This seems to be equivalent to adding a wrapper that does
platform_driver_register(&pistachio_pinctrl_driver);
and marking that wrapper with device_initcall(). I don't think there's
one line macro to do that.
> +MODULE_AUTHOR("Andrew Bresticker ");
> +MODULE_AUTHOR("Damien Horsley ");
> +MODULE_DESCRIPTION("Pistachio pinctrl driver");
> +MODULE_LICENSE("GPL v2");
These macros will (basically) be preprocessed away.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] pinctrl: Add driver for Alphascale asm9260 pinctrl
This patch adds a mismatch between the Kconfig symbol (bool) and the
code (which assumes a modular built too).
On Fri, 2015-03-27 at 10:36 +0100, Oleksij Rempel wrote:
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -47,6 +47,14 @@ config PINCTRL_AS3722
> open drain configuration for the GPIO pins of AS3722 devices. It also
> supports the GPIO functionality through gpiolib.
>
> +config PINCTRL_ASM9260
> + bool "Pinctrl driver for Alphascale asm9260"
This adds a bool symbol.
> + depends on MACH_ASM9260
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + Say Y here to enable the Alphascale asm9260 pinctrl driver
> +
> -- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> +obj-$(CONFIG_PINCTRL_ASM9260)+= pinctrl-asm9260.o
So this object can now only be built-in.
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-asm9260.c
> @@ -0,0 +1,733 @@
> +/*
> + * Pinctrl driver for the Alphascale ASM9260 SoC
> + *
> + * Copyright (c) 2014, Oleksij Rempel
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
This include is probably not needed.
> +#include
> +#include
> +#include
> +
> +#include "core.h"
> +#include "pinctrl-utils.h"
> +/*
> + * Pin control driver setup
> + */
> +static struct pinctrl_desc asm9260_pinctrl_desc = {
> + .pctlops= &asm9260_pinctrl_ops,
> + .pmxops = &asm9260_pinmux_ops,
> + .confops= &asm9260_pinconf_ops,
> + .owner = THIS_MODULE,
THIS_MODULE is, basically, equivalent to NULL for built-in code.
> +};
> +
> +MODULE_DEVICE_TABLE(of, asm9260_pinctrl_of_match);
This will be preprocessed away for built-in code.
> +module_platform_driver(asm9260_pinctrl_driver);
The built-in equivalent of this seems to be a wrapper that
only does
platform_driver_register(&asm9260_pinctrl_driver);
and mark that wrapper as a device_initcall(). There appears to be no
macro that does all that in one line.
> +MODULE_AUTHOR("Oleksij Rempel ");
> +MODULE_DESCRIPTION("Alphascale ASM9260 pinctrl driver");
> +MODULE_LICENSE("GPL");
These three macros will be, effectively, preprocessed away for built-in
code. (By the way, you probably want to use "GPL v2" as the license
ident if it would be possible to build this code modular.)
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] PCI: st: Provide support for the sti PCIe controller
Hi Fabrice, Fabrice Gasnier schreef op wo 18-03-2015 om 09:49 [+0100]: > On 03/16/2015 04:11 PM, Paul Bolle wrote: > >> +config PCI_ST > >> + bool "ST PCIe controller" > > You add a bool Kconfig symbol. A week or two ago I saw some patches fly > > by that - I think - allowed PCIe controllers to be built modular. > > Thanks for your review. > > Are you talking about "PCI: Export symbols of PCI functions" patch, that > is part of a series > named "pci: iproc: Add Broadcom iProc PCIe support" ? Yes, that is the series I was thinking about. (I made you search lkml, and that was a bit rude. But you found the patch anyhow.) > This controller doesn't look like to be based on pcie-designware core > driver. > Other vendors that are using "pcie-designware" core driver are also make > it bool. > The current core driver doesn't support module loading/unloading as I > see it. > If this is required, I also think this should be part of another patchset. > > What do you think ? I wouldn't know whether your driver might work as a loadable module, but other people reading this surely will. But if it can't work as a module you should drop all the module related macros etc. I spotted. Because then they serve no purpose. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] TTY: add support for tty_slave devices.
Just two nits to look into once you get to fix up all the smaller issues. NeilBrown schreef op wo 18-03-2015 om 16:58 [+1100]: > --- /dev/null > +++ b/drivers/tty/slave/Kconfig > @@ -0,0 +1,7 @@ > +menuconfig TTY_SLAVE > + tristate "TTY slave devices" > + depends on OF > + help > + Devices which attach via a uart, but need extra > + driver support for power management etc. > + This blank line makes "git am" whine: "new blank line at EOF". > --- /dev/null > +++ b/drivers/tty/slave/tty_slave_core.c [...] This file doesn't have a MODULE_LICENSE() macro. So I think that, if this driver is built as a module and loaded, kernel/module.c will set its license to "unspecified" and taint the kernel. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] PCI: st: Provide support for the sti PCIe controller
On Mon, 2015-03-16 at 15:20 +0100, Gabriel FERNANDEZ wrote:
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> +config PCI_ST
> + bool "ST PCIe controller"
You add a bool Kconfig symbol. A week or two ago I saw some patches fly
by that - I think - allowed PCIe controllers to be built modular.
> + depends on ARCH_STI || (ARM && COMPILE_TEST)
> + select PCIE_DW
> + help
> + Enable PCIe controller support on ST Socs. This controller is based
> + on Designware hardware and therefore the driver re-uses the
> + Designware core functions to implement the driver.
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> +obj-$(CONFIG_PCI_ST) += pci-st.o
If you keep that symbol bool this objectfile will never be part of a
module.
> diff --git a/drivers/pci/host/pci-st.c b/drivers/pci/host/pci-st.c
> new file mode 100644
> index 000..47d
> --- /dev/null
> +++ b/drivers/pci/host/pci-st.c
> +#include
For built-in code this include is, probably, not needed.
> +MODULE_DEVICE_TABLE(of, st_pcie_of_match);
For built-in code that macro will always be preprocessed away.
> +/* ST PCIe driver does not allow module unload */
> +static int __init pcie_init(void)
> +{
> + return platform_driver_probe(&st_pcie_driver, st_pcie_probe);
> +}
> +device_initcall(pcie_init);
I think the module unload comment is a bit odd for built-in only code.
> +MODULE_AUTHOR("Fabrice Gasnier ");
> +MODULE_DESCRIPTION("PCI express Driver for ST SoCs");
> +MODULE_LICENSE("GPL v2");
These three macros will be, basically, always preprocessed away as long
as this code can't be built to be modular.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/15] serial: stm32-usart: Add STM32 USART Driver
Just a license nit, I'm afraid.
On Thu, 2015-03-12 at 22:55 +0100, Maxime Coquelin wrote:
> --- /dev/null
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -0,0 +1,695 @@
> +/*
> + * Copyright (C) Maxime Coquelin 2015
> + * Author: Maxime Coquelin
> + * License terms: GNU General Public License (GPL), version 2
> + *
> + * Inspired by st-asc.c from STMicroelectronics (c)
> + */
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And
MODULE_LICENSE("GPL v2");
would match that statement.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] drm/bridge: Add IT6151 bridge driver
Just a few nits, I'm afraid. On Wed, 2015-03-11 at 14:18 +0800, CK Hu wrote: > drivers/gpu/drm/bridge/Kconfig | 10 + > drivers/gpu/drm/bridge/Makefile | 1 + I applied 1/2 and 2/2 on top of next-20150312 to check a trivial issue. The chunks for these two files needed context changes to git this patch applied. > drivers/gpu/drm/bridge/it6151.c | 601 > > include/drm/bridge/it6151.h | 34 +++ > 4 files changed, 646 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/it6151.c > create mode 100644 include/drm/bridge/it6151.h > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index f38bbcd..2b3a78e 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -11,3 +11,13 @@ config DRM_PTN3460 > select DRM_PANEL > ---help--- > ptn3460 eDP-LVDS bridge chip driver. > + > +config DRM_IT6151 > + bool "Enable IT6151FN : MIPI to eDP Converter" > + depends on DRM > + select DRM_KMS_HELPER > + help > + Choose this option if you have IT6151 for display > + The IT6151 is a high-performance and low-power > + MIPI to eDP converter > + (This empty line makes git am whine: "new blank line at EOF.".) > --- /dev/null > +++ b/drivers/gpu/drm/bridge/it6151.c > +#include This file can only be built-in. So I couldn't help but notice this include. And if I remove it make drivers/gpu/drm/bridge/it6151.o still runs without warning or errors. Unless I've missed something non-obvious I'd say it is not needed. > + (Empty line at end of file.) > --- /dev/null > +++ b/include/drm/bridge/it6151.h > + (Another empty line at end of file.) Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
Moritz,
On Wed, 2015-03-11 at 15:00 -0700, Moritz Fischer wrote:
> thanks for your feedback. While developing this I looked at other drivers in
> the tree and many of your comments would apply to them, too?
Could be. I didn't check. It's basically stuff that get's silently
handled by the preprocessor. So I wouldn't be surprised if a lot of
drivers do similar things.
> On Wed, Mar 11, 2015 at 2:31 AM, Paul Bolle
> >> +config POWER_RESET_SYSCON_POWEROFF
> >> + bool "Generic SYSCON regmap poweroff driver"
> >
> > This adds a bool symbol.
>
> Is that an issue or is it just to substantiate the rest of your comments
> below?
It's just an observation.
> >> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
> >
> > So this objectfile can never be part of a module.
>
> Same comment would apply for power/reset/qnap-poweroff.c, right?
That could be. I didn't check it while looking at this patch.
> >> +#include
> >
> > Is that include needed?
>
> Wouldn't that be needed if MODULE_DEVICE_TABLE etc, is used?
Perhaps, but I don't think those macros are used.
> >> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
> >
> > This will be preprocessed away.
>
> In my experiments it didn't work without this, are you sure it's not needed?
MODULE_DEVICE_TABLE() is defined only in include/linux/module.h. The few
related lines read (in next-20150311):
#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name)
\
extern const typeof(name) __mod_##type##__##name##_device_table
\
__attribute__ ((unused, alias(__stringify(name
#else /* !MODULE */
#define MODULE_DEVICE_TABLE(type, name)
#endif
So I think MODULE_DEVICE_TABLE() is always preprocessed away for boolean
code. Did I get that right?
> >> +module_platform_driver(syscon_poweroff_driver);
> >
> > I think the built-in equivalent of this would be adding a wrapper that
> > only does
> > platform_driver_register(&syscon_poweroff_driver);
>
> I couldn't find other examples in the tree doing this. Any pointers?
Please note "think". But see drivers/cpufreq/dbx500-cpufreq.c:
static int __init dbx500_cpufreq_register(void)
{
return platform_driver_register(&dbx500_cpufreq_plat_driver);
}
device_initcall(dbx500_cpufreq_register);
or drivers/gpio/gpio-vf610.c:
static int __init gpio_vf610_init(void)
{
return platform_driver_register(&vf610_gpio_driver);
}
device_initcall(gpio_vf610_init);
(There are probably more.) Please note that both files use MODULE_*
macros but can actually only be built-in.
> >> +MODULE_LICENSE("GPL v2");
> >
> > You probably meant
> > MODULE_LICENSE("GPL");
>
> Sorry about that. Will fix.
That's a very easy mistake to make anyway. There's currently a thread
were (another) Dmitry and I discuss the merits of differentiating
between "GPL" and "GPL v2" in MODULE_LICENSE().
> >> +MODULE_AUTHOR("Moritz Fischer ");
> >> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
> >> +MODULE_ALIAS("platform:syscon-poweroff");
> >
> > But these four macros will all be effectively preprocessed away, anyhow.
>
> I'll remove them.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] NFC: nxp-nci: Add support for NXP NCI chips
On Mon, 2015-03-09 at 11:12 +0100, clement.perroch...@effinnov.com
wrote:
> --- /dev/null
> +++ b/drivers/nfc/nxp-nci/core.c
> @@ -0,0 +1,186 @@
> +/*
> + * Generic driver for NXP NCI NFC chips
> + *
> + * Copyright (C) 2014 NXP Semiconductors All rights reserved.
> + *
> + * Authors: Clément Perrochaud
> + *
> + * Derived from PN544 device driver:
> + * Copyright (C) 2012 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably want
MODULE_LICENSE("GPL v2");
here.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] NFC: nxp-nci_i2c: Add I2C support to NXP NCI driver
On Mon, 2015-03-09 at 11:12 +0100, clement.perroch...@effinnov.com
wrote:
> --- /dev/null
> +++ b/drivers/nfc/nxp-nci/i2c.c
> @@ -0,0 +1,415 @@
> +/*
> + * I2C link layer for the NXP NCI driver
> + *
> + * Copyright (C) 2014 NXP Semiconductors All rights reserved.
> + *
> + * Authors: Clément Perrochaud
> + *
> + * Derived from PN544 device driver:
> + * Copyright (C) 2012 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably want
MODULE_LICENSE("GPL v2");
here.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] soc: Mediatek: Add SCPSYS power domain driver
On Tue, 2015-03-10 at 16:41 +0100, Sascha Hauer wrote:
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -0,0 +1,345 @@
> +/*
> + * Copyright (c) 2015 Pengutronix, Sascha Hauer
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably want
MODULE_LICENSE("GPL v2");
here.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] power: rt5033_charger: Add RT5033 charger device driver
On Mon, 2015-03-09 at 10:23 +0900, Beomho Seo wrote:
> --- /dev/null
> +++ b/drivers/power/rt5033_charger.c
> @@ -0,0 +1,485 @@
> +/*
> + * Battery charger driver for RT5033
> + *
> + * Copyright (C) 2014 Samsung Electronics, Co., Ltd.
> + * Author: Beomho Seo
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published bythe Free Software Foundation.
> + */
(bythe?)
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably meant
MODULE_LICENSE("GPL v2");
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
MODULE_LICENSE(): "GPL" vs. "GPL v2" (Was: [PATCH 1/2] Input: add support for Semtech SX8654 I2C touchscreen controller)
[Added Rusty and Dave.]
On Sat, 2015-03-07 at 20:51 -0800, Dmitry Torokhov wrote:
> On Sat, Mar 07, 2015 at 11:38:55PM +0100, Paul Bolle wrote:
> > From a technological standpoint it would be easy to declare "GPL" (or
> > any other string) to mean "GPL v2 compatible", which is, I think, all
> > that matters. But license_is_gpl_compatible() doesn't do that. And I
> > fear that's for a reason. Is my fear unfounded?
>
> Well we might ask Rusty on the off chance that he remembers but my guess
> would be that he added "GPL v2" in addition to "GPL" and other license
> stings because at the time there was one driver,
> drivers/net/tulip/xircom_tulip_cb.c, that used MODULE_LICENSE("GPL v2").
Dmitry and I discussed the fact that license_is_gpl_compatible() accepts
both "GPL" and "GPL v2" as strings indicating a GPL compatible license.
"GPL" is documented to mean "GNU Public License v2 or later" while "GPL
v2" is documented to mean "GNU Public License v2".
It turns out people regularly confuse these license string. (To
demonstrate just how often I hope to send comments for all patches sent
yesterday and the day before yesterday that I spotted getting that
wrong. I already sent a bunch of similar messages last week.) That's not
surprising, because either one passes the compatibility test, whatever
the actual license of the module might be.
So are these two strings intended to correctly identify to the actual
GPL version(s) of that module? Or is the fact that there are checks for
both strings mostly a result of the way these two strings were
introduced in a few steps? (I'll summarize these steps at the end of
this message.)
Paul Bolle
0) MODULE_LICENSE() was introduced in v2.4.10. It documented four
"idents" acceptable for free software modules:
"GPL"
"GPL and additional rights"
"Dual BSD/GPL"
"Dual MPL/GPL"
1) In v2.4.11 the license ident used in
drivers/net/pcmcia/xircom_tulip_cb.c was changed from "GPL" to "GPL v2".
2) In v2.5.40
"GPL v2"
was also documented as an acceptable free software ident. At that time
only drivers/net/tulip/xircom_tulip_cb.c used that ident.
3) And in v2.5.55 license_is_gpl_compatible() was added. It accepted
both license idents (since it accepted all then documented license
idents). At that time the only user of "GPL v2" was still
drivers/net/tulip/xircom_tulip_cb.c. (At that time there were over a
thousand users of "GPL".)
For comparison: in v4.0-rc3 there are nearly six thousand users of "GPL"
and over a thousand users of "GPL v2".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] Input: touchscreen-iproc: Add Broadcom iProc touchscreen driver
One nit is all I can report. On Tue, 2015-03-10 at 18:17 -0700, Jonathan Richardson wrote: > --- a/drivers/input/touchscreen/Kconfig > +++ b/drivers/input/touchscreen/Kconfig > +config TOUCHSCREEN_IPROC > + tristate "IPROC touch panel driver support" > + help > + Say Y here if you want to add support for the IPROC touch > + controller to your system. > + > + If unsure, say N. > + > + To compile this driver as a module, choose M here: the > + module will be called iproc-ts. The module will be called "bcm_iproc_tsc", won't it? > --- a/drivers/input/touchscreen/Makefile > +++ b/drivers/input/touchscreen/Makefile > +obj-$(CONFIG_TOUCHSCREEN_IPROC) += bcm_iproc_tsc.o Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
On Tue, 2015-03-10 at 16:21 -0700, Moritz Fischer wrote:
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> +config POWER_RESET_SYSCON_POWEROFF
> + bool "Generic SYSCON regmap poweroff driver"
This adds a bool symbol.
> + depends on OF
> + select MFD_SYSCON
> + help
> + Poweroff support for generic SYSCON mapped register poweroff.
> +
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
So this objectfile can never be part of a module.
> --- /dev/null
> +++ b/drivers/power/reset/syscon-poweroff.c
> @@ -0,0 +1,97 @@
> +/*
> + * Generic Syscon Poweroff Driver
> + *
> + * Copyright (c) 2015, National Instruments Corp.
> + * Author: Moritz Fischer
> + *
> + * 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
Is that include needed?
> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
This will be preprocessed away.
> +module_platform_driver(syscon_poweroff_driver);
I think the built-in equivalent of this would be adding a wrapper that
only does
platform_driver_register(&syscon_poweroff_driver);
and have that wrapper be marked with device_initcall(). Apparently
there's no macro that does all that in one line.
> +MODULE_LICENSE("GPL v2");
You probably meant
MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Moritz Fischer ");
> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
> +MODULE_ALIAS("platform:syscon-poweroff");
But these four macros will all be effectively preprocessed away, anyhow.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver
On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote: > This is because I added also support for COMPILE_TEST coverage as per > Uwe advice, > and thought it was necessary to have an entry for this. > Maybe I'm just wrong? I missed that you added COMPILE_TEST. A quick scan of your idea doesn't show any obvious issues. (Note that I don't really know how people actually use COMPILE_TEST. I guess things like "make allyesconfig" are involved.) Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/8] pinctrl: cygnus: add initial IOMUX driver support
Ray Jui schreef op ma 09-03-2015 om 12:40 [-0700]: > I don't see this as an "issue" to be quite honest. (Off topic: is issue a, well, strong word? To my (non-English) mind it's rather neutral, carrying by itself less urgency than, say, problem. If I'm wrong I might have confused quite a few people by using it quite often. And that would be a real issue!) > By saying that, I at > least agree with you that these are not information that's mandatory to > be in the driver given what we already have. MODULE_LICENSE is covered > by license header. MODULE_DESCRIPTION is covered by descriptions in > Kconfig. MODULE_AUTHOR is much less important than what's in the > MAINTAINERS list. > > Since I have to submit a new patch series to address the "ngpios" issue > that Linus mentioned in the other email, I don't mind removing all these > MODULE_* macros in the driver all together. I see. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/8] pinctrl: cygnus: add initial IOMUX driver support
Ray Jui schreef op ma 09-03-2015 om 12:00 [-0700]: > I think it depends on how you see it. Based on this logic, then one can > also argue comments in the code will be pre-processed away and are not > needed. They at least serve the same documentation purpose in a way. So why not make them comments? And even that might not be needed: - MODULE_LICENSE() only summarizes, in just a few words, what takes a few paragraphs in the customary comment at the top of a file; - MODULE_DESCRIPTION() repeats what, in general, has been said in the Kconfig entry for that driver and in the git commit explanation; - and I'm not sure what the benefit is of MODULE_AUTHOR() in the first place (even for actually modular drivers). > So > far I haven't seen other people complaining that having these module > based macros in the driver are confusing when the Kconfig has a bool. Perhaps that's just because review doesn't spot all issues. Patch bandwidth exceeding review bandwidth and all that. Anyhow, right now there's another thread discussing the questions my review comments raise. Eg, "The Kconfig symbol is bool, there is module related code in the driver, why note make the Kconfig symbol tristate (and the driver modular)?". I think that is one of the questions mixing built-in and modular semantics raises. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 6/8] pinctrl: cygnus: add gpio/pinconf driver
Linus Walleij schreef op ma 09-03-2015 om 17:41 [+0100]: > As pointed out in another mail on similar subject, I think these macros > are a kind of obsolete documentation and if they should be dropped we > need to go over an entire subsystem at a time and remove all boolean users > in a big patch. And, as I stated in that other thread, we could start by first checking for things like this during review. (And if people were to do clean up patches I'd suggest to do them one driver at a time. Just to keep things manageable.) Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/8] pinctrl: cygnus: add initial IOMUX driver support
Linus Walleij schreef op ma 09-03-2015 om 17:28 [+0100]: > I think you're right. Or I fear you're right. > > But this problem is present in so many drivers that a generic > fixup needs to be done with a script and across an entire subsystem > at once, Why don't we start with checking for similar cases during review, like I'm now doing for only a week or two? > and besides I'm not sure of these macros disturb so much. I think they're confusing at best. Ie, when reading the code and the corresponding Kconfig file one has to wonder: should the Kconfig symbol actually be tristate or should it stay bool but did someone forget to delete the module-specific code? > They are documentation in a sense, albeit a kind of documentation > we used before we had git to record the actual authors of the > code. They're useful, mostly, for module utilities. Outside that scope they add information that thousands of files (that can also only be built-in but do not have these macros) do not have and, apparently, do not need. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
Hi Yong, Yong Wu schreef op ma 09-03-2015 om 19:57 [+0800]: > On Fri, 2015-03-06 at 12:30 +0100, Paul Bolle wrote: > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > --- a/drivers/soc/mediatek/Kconfig > > > +++ b/drivers/soc/mediatek/Kconfig > > > @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP > > > PMIC wrapper is a proprietary hardware in MT8173 to make > > > communication protocols to access PMIC device. > > > This driver implement access protocols for MT8173. > > > + > > > +config MTK_SMI > > > +bool > > > > Nit: make this one tab instead of 8 spaces, please. > > > > > + help > > > + Smi help enable/disable iommu in mt8173 and control the > > > + clock of each local arbiter. > > > + It should be true while MTK_IOMMU enable. > > > > I don't think anyone using the *config tools will ever see this text, as > > there's no prompt. So you might as well make this a comment or drop it > > altogether. > > > We could search it in the tool even though we don't see it. In next > version, I will try to make it a comment. > > Is this selected by more than just MTK_IOMMU (see 2/5)? If not, I think > > MTK_SMI will be set and unset in lockstep with MTK_IOMMU. In other > > words, you could as well use one Kconfig symbol. > > > if we disable MTK_IOMMU, the MTK_SMI also should be selected.That is > because > if the multimedia h/w want to work, the clock of the local arbiters always > should be opened. This is a bit confusing, I'm afraid. Do you mean to say that it ought to be possible for MTK_SMI to be 'y' even if MTK_IOMMU would be 'n'? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] media: i2c: add support for omnivision's ov2659 sensor
On Sat, 2015-03-07 at 15:21 +, Lad Prabhakar wrote:
> --- /dev/null
> +++ b/drivers/media/i2c/ov2659.c
> @@ -0,0 +1,1495 @@
> +/*
> + * Omnivision OV2659 CMOS Image Sensor driver
> + *
> + * Copyright (C) 2015 Texas Instruments, Inc.
> + *
> + * Benoit Parrot
> + * Lad, Prabhakar
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
This states the license of this modules is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably want
MODULE_LICENSE("GPL v2");
here.
(If you think this is a bit silly, and somewhere I think so too, you're
invited to jump into the discussion starting at
https://lkml.org/lkml/2015/3/7/119 .)
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Input: add support for Semtech SX8654 I2C touchscreen controller
On Sat, 2015-03-07 at 14:26 -0800, Dmitry Torokhov wrote: > On March 7, 2015 2:12:20 PM PST, Paul Bolle wrote: > I was talking about them being treated differently from technological > standpoint (i.e. the code), not from legal one. >From a technological standpoint it would be easy to declare "GPL" (or any other string) to mean "GPL v2 compatible", which is, I think, all that matters. But license_is_gpl_compatible() doesn't do that. And I fear that's for a reason. Is my fear unfounded? > If you want to fix up input drivers I'll take such patch, but I am > sure more such cases will sneak in unless you also make sure that > there are tools (such as checkpatch.pl) that can alert developers to >the inconsistency. checkpatch.pl crossed my mind too. But in just over a week of checking the license comments of (a subset of) the submitted patches I've come to think that just won't work. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Input: add support for Semtech SX8654 I2C touchscreen controller
On Sat, 2015-03-07 at 14:02 -0800, Dmitry Torokhov wrote: > On March 7, 2015 1:54:41 PM PST, Paul Bolle wrote: > >By that logic we might as well simplify the logic of > >license_is_gpl_compatible() and MODULE_LICENSE() quite a bit. Why check > >for six variants instead of just one and be done with it? > > Because nobody wants to go through hundreds of drivers and change them? Not fun, but surely doable. > >Anyhow, "GPL" and "GPL v2" are both allowed but not identical. So, > >unless a patch is applied to treat them interchangeably, somehow, in > >the module license checking code, > > They are treated interchangeably as far as I can see. Where do you see > "GPL" is being treated differently than "GPL v2". I'm not going to explain here why "GPL v2" or "GPL v2 or later" differ. "GPL" is documented to mean "GPL v2 or later". "GPL v2" is documented to mean just that (see include/linux/module.h). Again, you're free to submit a patch to somehow simplify that. But unless a patch like that is applied, we should make sure MODULE_LICENSE() matches the actual license of the module involved. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Input: add support for Semtech SX8654 I2C touchscreen controller
On Sat, 2015-03-07 at 13:25 -0800, Dmitry Torokhov wrote:
> I am not sure if anyone cares about exact version of GPL in module
> information (2 only vs 2+) since it only used to figure out if the
> module taints kernel or not. In fact there are more modules that are v2
> only that claim GPL than the ones claiming GPL v2.
>
> dtor@dtor-ws:~/kernel/master$ for file in `grep -r -l
> 'MODULE_LICENSE("GPL")'`; do grep -H '2 as published' $file; done | wc -l
> 259
> dtor@dtor-ws:~/kernel/master$ for file in `grep -r -l 'MODULE_LICENSE("GPL
> v2")'`; do grep -H '2 as published' $file; do ne | wc -l
> 150
>
> Also:
>
> dtor@dtor-ws:~/kernel/master$ for file in `grep -r -l 'MODULE_LICENSE("GPL
> v2")'`; do grep -H '2 or ' $file; done | wc -l
> 68
> dtor@dtor-ws:~/kernel/master$ for file in `grep -r -l
> 'MODULE_LICENSE("GPL")'`; do grep -H '2 or ' $file; done | wc -l
> 237
By that logic we might as well simplify the logic of
license_is_gpl_compatible() and MODULE_LICENSE() quite a bit. Why check
for six variants instead of just one and be done with it?
Anyhow, "GPL" and "GPL v2" are both allowed but not identical. So,
unless a patch is applied to treat them interchangeably, somehow, in the
module license checking code, we ought to make each instance of
MODULE_LICENSE() match the actual license of the module it's used for.
Yes, that's annoying. You're free to submit a patch to end all the
busywork this brings along. But I fear there's a reason for all that
busywork. Please prove me wrong. It would make everyone's life a bit
easier.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
