* [PATCH v2 2/5] mtd: nand: add core support for on-die ECC
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
A number of NAND flashes have a capability called "on-die ECC" where the
NAND chip itself is capable of detecting and correcting errors.
Linux already has support for using the ECC implementation of the NAND
controller, or a software based ECC implementation, but not for using
the ECC implementation of the NAND controller. However, such an
implementation is sometimes useful in situations where the NAND
controller provides ECC algorithms that are not strong enough for the
NAND chip used on the system. A typical case is a NAND chip that
requires a 4-bit ECC, while the NAND controller only provides a 1-bit
ECC algorithm.
This commit introduces the support for the NAND_ECC_ON_DIE ECC mode:
- Parsing of the "on-die" value for the "nand-ecc-mode" Device Tree
property
- Handling NAND_ECC_ON_DIE case in nand_scan_tail(). The idea is that
the vendor specific code for the NAND chip must implement
->read_page() and ->write_page(). It may optionally provide its own
->read_page_raw() and ->write_page_raw() as well. For OOB operation,
we assume the standard operations are good enough, but they can be
overridden by the vendor specific code if needed.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
drivers/mtd/nand/nand_base.c | 13 +++++++++++++
include/linux/mtd/nand.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ed49a1d..b639e88 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4109,6 +4109,7 @@ static const char * const nand_ecc_modes[] = {
[NAND_ECC_HW] = "hw",
[NAND_ECC_HW_SYNDROME] = "hw_syndrome",
[NAND_ECC_HW_OOB_FIRST] = "hw_oob_first",
+ [NAND_ECC_ON_DIE] = "on-die",
};
static int of_get_nand_ecc_mode(struct device_node *np)
@@ -4649,6 +4650,18 @@ int nand_scan_tail(struct mtd_info *mtd)
}
break;
+ case NAND_ECC_ON_DIE:
+ if (!ecc->read_page || !ecc->write_page) {
+ WARN(1, "No ECC functions supplied; on-die ECC not possible\n");
+ ret = -EINVAL;
+ goto err_free;
+ }
+ if (!ecc->read_oob)
+ ecc->read_oob = nand_read_oob_std;
+ if (!ecc->write_oob)
+ ecc->write_oob = nand_write_oob_std;
+ break;
+
case NAND_ECC_NONE:
pr_warn("NAND_ECC_NONE selected by board driver. This is not recommended!\n");
ecc->read_page = nand_read_page_raw;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 8f67b15..6035220 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -116,6 +116,7 @@ typedef enum {
NAND_ECC_HW,
NAND_ECC_HW_SYNDROME,
NAND_ECC_HW_OOB_FIRST,
+ NAND_ECC_ON_DIE,
} nand_ecc_modes_t;
enum nand_ecc_algo {
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 3/5] mtd: nand: export nand_{read,write}_page_raw()
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
The nand_read_page_raw() and nand_write_page_raw() functions might be
re-used by vendor-specific implementations of the read_page/write_page
functions. Instead of having vendor-specific code duplicate this code,
it is much better to export those functions and allow them to be
re-used.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
drivers/mtd/nand/nand_base.c | 10 ++++++----
include/linux/mtd/nand.h | 8 ++++++++
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b639e88..3282738 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1456,14 +1456,15 @@ EXPORT_SYMBOL(nand_check_erased_ecc_chunk);
*
* Not for syndrome calculating ECC controllers, which use a special oob layout.
*/
-static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
- uint8_t *buf, int oob_required, int page)
+int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page)
{
chip->read_buf(mtd, buf, mtd->writesize);
if (oob_required)
chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
return 0;
}
+EXPORT_SYMBOL(nand_read_page_raw);
/**
* nand_read_page_raw_syndrome - [INTERN] read raw page data without ecc
@@ -2401,8 +2402,8 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
*
* Not for syndrome calculating ECC controllers, which use a special oob layout.
*/
-static int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
- const uint8_t *buf, int oob_required, int page)
+int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required, int page)
{
chip->write_buf(mtd, buf, mtd->writesize);
if (oob_required)
@@ -2410,6 +2411,7 @@ static int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
return 0;
}
+EXPORT_SYMBOL(nand_write_page_raw);
/**
* nand_write_page_raw_syndrome - [INTERN] raw page write function
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6035220..7a01d2e 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -1259,6 +1259,14 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
int page);
+/* Default read_page_raw implementation */
+int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page);
+
+/* Default write_page_raw implementation */
+int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required, int page);
+
/* Reset and initialize a NAND device */
int nand_reset(struct nand_chip *chip, int chipnr);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 4/5] mtd: nand: add support for Micron on-die ECC
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Now that the core NAND subsystem has support for on-die ECC, this commit
brings the necessary code to support on-die ECC on Micron NANDs.
In micron_nand_init(), we detect if the Micron NAND chip supports on-die
ECC mode, by checking a number of conditions:
- It must be an ONFI NAND
- It must be a SLC NAND
- Enabling *and* disabling on-die ECC must work
- The on-die ECC must be correcting 4 bits per 512 bytes of data. Some
Micron NAND chips have an on-die ECC able to correct 8 bits per 512
bytes of data, but they work slightly differently and therefore we
don't support them in this patch.
Then, if the on-die ECC cannot be disabled (some Micron NAND have on-die
ECC forcefully enabled), we bail out, as we don't support such
NANDs. Indeed, the implementation of raw_read()/raw_write() make the
assumption that on-die ECC can be disabled. Support for Micron NANDs
with on-die ECC forcefully enabled can easily be added, but in the
absence of such HW for testing, we preferred to simply bail out.
If the on-die ECC is supported, and requested in the Device Tree, then
it is indeed enabled, by using custom implementations of the
->read_page(), ->read_page_raw(), ->write_page() and ->write_page_raw()
operation to properly handle the on-die ECC.
In the non-raw functions, we need to enable the internal ECC engine
before issuing the NAND_CMD_READ0 or NAND_CMD_SEQIN commands, which is
why we set the NAND_ECC_CUSTOM_PAGE_ACCESS option at initialization
time (it asks the NAND core to let the NAND driver issue those
commands).
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
drivers/mtd/nand/nand_micron.c | 215 +++++++++++++++++++++++++++++++++++++++++
include/linux/mtd/nand.h | 2 +
2 files changed, 217 insertions(+)
diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
index 8770110..0987f32 100644
--- a/drivers/mtd/nand/nand_micron.c
+++ b/drivers/mtd/nand/nand_micron.c
@@ -17,6 +17,12 @@
#include <linux/mtd/nand.h>
+/*
+ * Special Micron status bit that indicates when the block has been
+ * corrected by on-die ECC and should be rewritten
+ */
+#define NAND_STATUS_WRITE_RECOMMENDED BIT(3)
+
struct nand_onfi_vendor_micron {
u8 two_plane_read;
u8 read_cache;
@@ -66,9 +72,191 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
return 0;
}
+static int micron_nand_on_die_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ if (section >= 4)
+ return -ERANGE;
+
+ oobregion->offset = (section * 16) + 8;
+ oobregion->length = 8;
+
+ return 0;
+}
+
+static int micron_nand_on_die_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ if (section >= 4)
+ return -ERANGE;
+
+ oobregion->offset = (section * 16) + 2;
+ oobregion->length = 6;
+
+ return 0;
+}
+
+static const struct mtd_ooblayout_ops micron_nand_on_die_ooblayout_ops = {
+ .ecc = micron_nand_on_die_ooblayout_ecc,
+ .free = micron_nand_on_die_ooblayout_free,
+};
+
+static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
+{
+ u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+
+ if (enable)
+ feature[0] |= ONFI_FEATURE_ON_DIE_ECC_EN;
+
+ return chip->onfi_set_features(nand_to_mtd(chip), chip,
+ ONFI_FEATURE_ON_DIE_ECC, feature);
+}
+
+static int
+micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required,
+ int page)
+{
+ int status;
+ int max_bitflips = 0;
+
+ micron_nand_on_die_ecc_setup(chip, true);
+
+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
+ chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
+ status = chip->read_byte(mtd);
+ if (status & NAND_STATUS_FAIL)
+ mtd->ecc_stats.failed++;
+ /*
+ * The internal ECC doesn't tell us the number of bitflips
+ * that have been corrected, but tells us if it recommends to
+ * rewrite the block. If it's the case, then we pretend we had
+ * a number of bitflips equal to the ECC strength, which will
+ * hint the NAND core to rewrite the block.
+ */
+ else if (status & NAND_STATUS_WRITE_RECOMMENDED)
+ max_bitflips = chip->ecc.strength;
+
+ chip->cmdfunc(mtd, NAND_CMD_READ0, -1, -1);
+
+ nand_read_page_raw(mtd, chip, buf, oob_required, page);
+
+ micron_nand_on_die_ecc_setup(chip, false);
+
+ return max_bitflips;
+}
+
+static int
+micron_nand_write_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required,
+ int page)
+{
+ micron_nand_on_die_ecc_setup(chip, true);
+
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+ nand_write_page_raw(mtd, chip, buf, oob_required, page);
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+ micron_nand_on_die_ecc_setup(chip, false);
+
+ return 0;
+}
+
+static int
+micron_nand_read_page_raw_on_die_ecc(struct mtd_info *mtd,
+ struct nand_chip *chip,
+ uint8_t *buf, int oob_required,
+ int page)
+{
+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
+ nand_read_page_raw(mtd, chip, buf, oob_required, page);
+
+ return 0;
+}
+
+static int
+micron_nand_write_page_raw_on_die_ecc(struct mtd_info *mtd,
+ struct nand_chip *chip,
+ const uint8_t *buf, int oob_required,
+ int page)
+{
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+ nand_write_page_raw(mtd, chip, buf, oob_required, page);
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+ return 0;
+}
+
+enum {
+ /* The NAND flash doesn't support on-die ECC */
+ MICRON_ON_DIE_UNSUPPORTED,
+
+ /*
+ * The NAND flash supports on-die ECC and it can be
+ * enabled/disabled by a set features command.
+ */
+ MICRON_ON_DIE_SUPPORTED,
+
+ /*
+ * The NAND flash supports on-die ECC, and it cannot be
+ * disabled.
+ */
+ MICRON_ON_DIE_MANDATORY,
+};
+
+/*
+ * Try to detect if the NAND support on-die ECC. To do this, we enable
+ * the feature, and read back if it has been enabled as expected. We
+ * also check if it can be disabled, because some Micron NANDs do not
+ * allow disabling the on-die ECC and we don't support such NANDs for
+ * now.
+ *
+ * This function also has the side effect of disabling on-die ECC if
+ * it had been left enabled by the firmware/bootloader.
+ */
+static int micron_supports_on_die_ecc(struct nand_chip *chip)
+{
+ u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+ int ret;
+
+ if (chip->onfi_version == 0)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ if (chip->bits_per_cell != 1)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ ret = micron_nand_on_die_ecc_setup(chip, true);
+ if (ret)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ chip->onfi_get_features(nand_to_mtd(chip), chip,
+ ONFI_FEATURE_ON_DIE_ECC, feature);
+ if ((feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN) == 0)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ ret = micron_nand_on_die_ecc_setup(chip, false);
+ if (ret)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ chip->onfi_get_features(nand_to_mtd(chip), chip,
+ ONFI_FEATURE_ON_DIE_ECC, feature);
+ if (feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN)
+ return MICRON_ON_DIE_MANDATORY;
+
+ /*
+ * Some Micron NANDs have an on-die ECC of 4/512, some other
+ * 8/512. We only support the former.
+ */
+ if (chip->onfi_params.ecc_bits != 4)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ return MICRON_ON_DIE_SUPPORTED;
+}
+
static int micron_nand_init(struct nand_chip *chip)
{
struct mtd_info *mtd = nand_to_mtd(chip);
+ int ondie;
int ret;
ret = micron_nand_onfi_init(chip);
@@ -78,6 +266,33 @@ static int micron_nand_init(struct nand_chip *chip)
if (mtd->writesize == 2048)
chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
+ ondie = micron_supports_on_die_ecc(chip);
+
+ if (ondie == MICRON_ON_DIE_MANDATORY) {
+ pr_err("On-die ECC forcefully enabled, not supported\n");
+ return -EINVAL;
+ }
+
+ if (chip->ecc.mode == NAND_ECC_ON_DIE) {
+ if (ondie == MICRON_ON_DIE_UNSUPPORTED) {
+ pr_err("On-die ECC selected but not supported\n");
+ return -EINVAL;
+ }
+
+ chip->ecc.options = NAND_ECC_CUSTOM_PAGE_ACCESS;
+ chip->ecc.bytes = 32;
+ chip->ecc.strength = 4;
+ chip->ecc.algo = NAND_ECC_BCH;
+ chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
+ chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
+ chip->ecc.read_page_raw =
+ micron_nand_read_page_raw_on_die_ecc;
+ chip->ecc.write_page_raw =
+ micron_nand_write_page_raw_on_die_ecc;
+
+ mtd_set_ooblayout(mtd, µn_nand_on_die_ooblayout_ops);
+ }
+
return 0;
}
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7a01d2e..f019916 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -258,6 +258,8 @@ struct nand_chip;
/* Vendor-specific feature address (Micron) */
#define ONFI_FEATURE_ADDR_READ_RETRY 0x89
+#define ONFI_FEATURE_ON_DIE_ECC 0x90
+#define ONFI_FEATURE_ON_DIE_ECC_EN BIT(3)
/* ONFI subfeature parameters length */
#define ONFI_SUBFEATURE_PARAM_LEN 4
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 5/5] mtd: nand: fsmc_nand: handle on-die ECC case
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
This commit adjusts the fsmc_nand driver so that it accepts the
NAND_ECC_ON_DIE case. It simply does nothing in this case, since both
the ECC operations and OOB layout will be defined by the NAND chip code
rather than by the NAND controller code.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
drivers/mtd/nand/fsmc_nand.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index cea50d2..5c058e8 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -986,6 +986,9 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
break;
}
+ case NAND_ECC_ON_DIE:
+ break;
+
default:
dev_err(&pdev->dev, "Unsupported ECC mode!\n");
goto err_probe;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [RFC 1/3] dt-binding: soc: qcom: Add binding for RFSA
From: Bjorn Andersson @ 2017-04-29 20:02 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Gross, David Brown, Frank Rowand, Mark Rutland,
linux-arm-msm, linux-soc, devicetree, linux-kernel
In-Reply-To: <20170428174239.x62opv5vzx2ce2wt@rob-hp-laptop>
On Fri 28 Apr 10:42 PDT 2017, Rob Herring wrote:
> On Sat, Apr 22, 2017 at 10:35:17AM -0700, Bjorn Andersson wrote:
> > This adds the binding for describing shared memory buffers for
> > implementing the remote filesystem protocol.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >
> > My initial attempt was to mimic the ramoops of just adding the compatible to
> > the reserved-memory node, but I have not been able to figure out a sane way of
> > getting hold of the base address in the case that the memory region is
> > described my a "size" only (done on some platforms).
>
> I prefer the ramoops way.
>
Me too :)
> > The problem is that we create the reserved_mem objects (and remove the
> > memblocks) while we're still operating on the flattened representation, so
> > without a phandle it doesn't seem like we have anything to perform the
> > comparison with later on.
>
> I'm not sure I follow.
>
In my first attempt I extended of_platform_default_populate_init() to
also probe my platform_driver and like ramoops acquired the values of
reg and memremap() these. This works fine.
But for some platforms the memory-region doesn't need a fixed location,
it's just required to be a consecutive chunk of physical ram. So I
replace the "reg = <>" with a "size = <>", this causes
__reserved_mem_alloc_size() to carve out a chunk of memory somewhere
and update the associated reserved_mem entry. The reserved_mem will be
populated with the phandle from the [flattened] of_node.
Later my platform_device is instantiated and I need to figure out the
base address that was picked earlier - so that I know which region to
memremap().
But as my "rfsa-node" stands on its own it will not have any "phandle",
so the reserved_mem->phandle will remain 0. Further more I only have the
unflattened of_node - so I can't just compare the address with the
flattened version previously used.
So the problem at hand is how to match my pdev->dev.of_node to an entry
in the reserved_mem array (in of_reserved_mem.c).
> >
> > .../devicetree/bindings/soc/qcom/qcom,rfsa.txt | 43 ++++++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> >
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > new file mode 100644
> > index 000000000000..b4de0de74e46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > @@ -0,0 +1,43 @@
> > +Qualcomm Remote File System Access binding
> > +
> > +This binding describes the Qualcomm RFSA, which serves the purpose of managing
> > +the shared memory region used for remote processors to access block device data
> > +using the Remote Filesystem protocol.
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <stringlist>
> > + Definition: must be:
> > + "qcom,rfsa"
>
> No versioning?
>
The binding is used to describe a chunk of memory and an associated
client-id of this memory, so I'm not sure if we need a version.
> > +
> > +- memory-region:
> > + Usage: required
> > + Value type: <prop-encoded-array>
> > + Definition: handle to memory reservation the associated rfsa region.
> > +
> > +- qcom,client-id:
> > + Usage: required
> > + Value type: <u32>
> > + Definition: identifier of the client to use this region for buffers.
>
> What determines these numbers?
>
The bigger picture of this is that the remote processors (e.g. modem)
needs access to block storage, so it sends request to a service on the
system with storage access (i.e the application processor) which will
read and write from the file system, storing blocks of data in the
rfsa-memory.
So the client-id is the hard coded identifier of each such remote system
requesting IO - each one with its own memory carveout.
In later platforms we need to configure the SMMU for each remote in
order to give them access to these cerveouts, so I don't see that its
possible to mash them together in one chunk and handle the client-id
thing only in the application.
> > +
> > += EXAMPLE
> > +The following example shows the RFSA setup for APQ8016, with the RFSA region
> > +for the Hexagon DSP (id #1) located at 0x86700000.
> > +
> > + reserved-memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + rmtfs: rmtfs@86700000 {
>
> I think you should have a compatible here even with the 2 node approach.
>
I'm hoping we can figure out a way to fix the Linux implementation so
that we can describe it fully in one node.
> > + reg = <0x0 0x86700000 0x0 0xe0000>;
> > + no-map;
> > + };
> > + };
> > +
> > + hexagon-rfsa {
> > + compatible = "qcom,rfsa";
> > + memory-region = <&rmtfs>;
> > +
> > + qcom,client-id = <1>;
> > + };
> > --
> > 2.12.0
> >
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver
From: Peter Rosin @ 2017-04-29 21:29 UTC (permalink / raw)
To: Philipp Zabel, linux-media-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Steve Longerbeam, Sakari Ailus,
Pavel Machek, Rob Herring, Mark Rutland, Vladimir Zapolskiy,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Sascha Hauer, Steve Longerbeam
In-Reply-To: <20170428141330.16187-2-p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 2017-04-28 16:13, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
> This has been last sent as part of the i.MX media series.
>
> Changes since https://patchwork.kernel.org/patch/9647869/:
> - Split out the actual mux operation to be provided by the mux controller
> framework [1]. GPIO and MMIO control can be provided by individual mux
> controller drivers [2][3].
> [1] https://patchwork.kernel.org/patch/9695837/
> [2] https://patchwork.kernel.org/patch/9695839/
> [3] https://patchwork.kernel.org/patch/9704509/
> - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
> vidsw with video_mux.
> - Made the mux inactive by default, only activated by user interaction.
> - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
> - Reuse subdev.entity.num_pads instead of keeping our own count.
> - Removed implicit link disabling. Instead, trying to enable a second
> sink pad link yields -EBUSY.
> - Merged _async_init into _probe.
> - Removed superfluous pad index check from _set_format.
> - Added is_source_pad helper to tell source and sink pads apart.
> - Removed test for status property in endpoint nodes. Disable the remote
> device or sever the endpoint link to disable a sink pad.
> ---
> drivers/media/platform/Kconfig | 6 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 349 insertions(+)
> create mode 100644 drivers/media/platform/video-mux.c
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c9106e105baba..b046a6d39fee5 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> To compile this driver as a module, choose M here: the
> module will be called arv.
>
> +config VIDEO_MUX
> + tristate "Video Multiplexer"
> + depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> + help
> + This driver provides support for N:1 video bus multiplexers.
> +
> config VIDEO_OMAP3
> tristate "OMAP 3 Camera support"
> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 349ddf6a69da2..fd2735ca3ff75 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o
>
> obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE) += m2m-deinterlace.o
>
> +obj-$(CONFIG_VIDEO_MUX) += video-mux.o
> +
> obj-$(CONFIG_VIDEO_S3C_CAMIF) += s3c-camif/
> obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) += exynos4-is/
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> new file mode 100644
> index 0000000000000..419541729f67e
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,341 @@
> +/*
> + * video stream multiplexer controlled via mux control
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2017?
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +
> +struct video_mux {
> + struct v4l2_subdev subdev;
> + struct media_pad *pads;
> + struct v4l2_mbus_framefmt *format_mbus;
> + struct v4l2_of_endpoint *endpoint;
> + struct mux_control *mux;
> + int active;
> +};
> +
> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct video_mux, subdev);
> +}
> +
> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> +{
> + return pad == vmux->subdev.entity.num_pads - 1;
> +}
> +
> +static int video_mux_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + int ret;
> +
> + /*
> + * The mux state is determined by the enabled sink pad link.
> + * Enabling or disabling the source pad link has no effect.
> + */
> + if (is_source_pad(vmux, local->index))
> + return 0;
> +
> + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> + remote->entity->name, remote->index, local->entity->name,
> + local->index, flags & MEDIA_LNK_FL_ENABLED);
> +
> + if (flags & MEDIA_LNK_FL_ENABLED) {
> + if (vmux->active == local->index)
Here, you shortcut the mux_control_select_trylock test and return "OK"
based on a driver-local variable that is intended to keep track of mux
ownership.
> + return 0;
> +
> + if (vmux->active >= 0)
Here too (and this check is not needed, the situation will be covered by
the mux_control_try_select call).
> + return -EBUSY;
> +
> + dev_dbg(sd->dev, "setting %d active\n", local->index);
> + ret = mux_control_try_select(vmux->mux, local->index);
> + if (ret < 0)
> + return ret;
> + vmux->active = local->index;
> + } else {
> + if (vmux->active != local->index)
> + return 0;
> +
> + dev_dbg(sd->dev, "going inactive\n");
> + mux_control_deselect(vmux->mux);
But here you let go of the mux *before* you clear the driver-local
ownership indicator. That looks suspicious. My guess is that this is
"safe" because the upper layers has some serialization, but I don't
know. Anyway, even if there is something saving you in the upper
layers, it looks out of order and unneeded. I would have moved the
below vmux->active = -1; statement up to before the above deselect.
With that fixed, mux usage looks good to me, so you can add an Acked-
by from me if you wish (goes for the bindings patch as well).
Cheers,
peda
> + vmux->active = -1;
> + }
> +
> + return 0;
> +}
> +
> +static struct media_entity_operations video_mux_ops = {
> + .link_setup = video_mux_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static bool video_mux_endpoint_disabled(struct device_node *ep)
> +{
> + struct device_node *rpp = of_graph_get_remote_port_parent(ep);
> +
> + return !of_device_is_available(rpp);
> +}
> +
> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
> + struct v4l2_mbus_config *cfg)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_of_endpoint *endpoint;
> + struct media_pad *pad;
> + int ret;
> +
> + if (vmux->active == -1) {
> + dev_err(sd->dev, "no configuration for inactive mux\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * Retrieve media bus configuration from the entity connected to the
> + * active input
> + */
> + pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
> + if (pad) {
> + sd = media_entity_to_v4l2_subdev(pad->entity);
> + ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> + if (ret == -ENOIOCTLCMD)
> + pad = NULL;
> + else if (ret < 0) {
> + dev_err(sd->dev, "failed to get source configuration\n");
> + return ret;
> + }
> + }
> + if (!pad) {
> + endpoint = &vmux->endpoint[vmux->active];
> +
> + /* Mirror the input side on the output side */
> + cfg->type = endpoint->bus_type;
> + if (cfg->type == V4L2_MBUS_PARALLEL ||
> + cfg->type == V4L2_MBUS_BT656)
> + cfg->flags = endpoint->bus.parallel.flags;
> + }
> +
> + return 0;
> +}
> +
> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_subdev *upstream_sd;
> + struct media_pad *pad;
> +
> + if (vmux->active == -1) {
> + dev_err(sd->dev, "Can not start streaming on inactive mux\n");
> + return -EINVAL;
> + }
> +
> + pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
> + if (!pad) {
> + dev_err(sd->dev, "Failed to find remote source pad\n");
> + return -ENOLINK;
> + }
> +
> + if (!is_media_entity_v4l2_subdev(pad->entity)) {
> + dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
> + return -ENODEV;
> + }
> +
> + upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> + return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
> +}
> +
> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
> + .g_mbus_config = video_mux_g_mbus_config,
> + .s_stream = video_mux_s_stream,
> +};
> +
> +static struct v4l2_mbus_framefmt *
> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad, u32 which)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_format(sd, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &vmux->format_mbus[pad];
> + default:
> + return NULL;
> + }
> +}
> +
> +static int video_mux_get_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *sdformat)
> +{
> + sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
> + sdformat->which);
> + return 0;
> +}
> +
> +static int video_mux_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *sdformat)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_mbus_framefmt *mbusformat;
> +
> + mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
> + sdformat->which);
> + if (!mbusformat)
> + return -EINVAL;
> +
> + /* Source pad mirrors active sink pad, no limitations on sink pads */
> + if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
> + sdformat->format = vmux->format_mbus[vmux->active];
> +
> + *mbusformat = sdformat->format;
> +
> + return 0;
> +}
> +
> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> + .get_fmt = video_mux_get_format,
> + .set_fmt = video_mux_set_format,
> +};
> +
> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
> + .pad = &video_mux_pad_ops,
> + .video = &video_mux_subdev_video_ops,
> +};
> +
> +static int video_mux_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct v4l2_of_endpoint endpoint;
> + struct device_node *ep;
> + struct video_mux *vmux;
> + unsigned int num_pads = 0;
> + int ret;
> + int i;
> +
> + vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
> + if (!vmux)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, vmux);
> +
> + v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
> + snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
> + vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + vmux->subdev.dev = dev;
> +
> + /*
> + * The largest numbered port is the output port. It determines
> + * total number of pads.
> + */
> + for_each_endpoint_of_node(np, ep) {
> + of_graph_parse_endpoint(ep, &endpoint.base);
> + num_pads = max(num_pads, endpoint.base.port + 1);
> + }
> +
> + if (num_pads < 2) {
> + dev_err(dev, "Not enough ports %d\n", num_pads);
> + return -EINVAL;
> + }
> +
> + vmux->mux = devm_mux_control_get(dev, NULL);
> + if (IS_ERR(vmux->mux)) {
> + ret = PTR_ERR(vmux->mux);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get mux: %d\n", ret);
> + return ret;
> + }
> +
> + vmux->active = -1;
> + vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
> + GFP_KERNEL);
> + vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
> + num_pads, GFP_KERNEL);
> + vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
> + (num_pads - 1), GFP_KERNEL);
> +
> + for (i = 0; i < num_pads - 1; i++)
> + vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
> + vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
> +
> + vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
> + ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
> + vmux->pads);
> + if (ret < 0)
> + return ret;
> +
> + vmux->subdev.entity.ops = &video_mux_ops;
> +
> + for_each_endpoint_of_node(np, ep) {
> + v4l2_of_parse_endpoint(ep, &endpoint);
> +
> + if (video_mux_endpoint_disabled(ep)) {
> + dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
> + continue;
> + }
> +
> + vmux->endpoint[endpoint.base.port] = endpoint;
> + }
> +
> + return v4l2_async_register_subdev(&vmux->subdev);
> +}
> +
> +static int video_mux_remove(struct platform_device *pdev)
> +{
> + struct video_mux *vmux = platform_get_drvdata(pdev);
> + struct v4l2_subdev *sd = &vmux->subdev;
> +
> + v4l2_async_unregister_subdev(sd);
> + media_entity_cleanup(&sd->entity);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id video_mux_dt_ids[] = {
> + { .compatible = "video-mux", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
> +
> +static struct platform_driver video_mux_driver = {
> + .probe = video_mux_probe,
> + .remove = video_mux_remove,
> + .driver = {
> + .of_match_table = video_mux_dt_ids,
> + .name = "video-mux",
> + },
> +};
> +
> +module_platform_driver(video_mux_driver);
> +
> +MODULE_DESCRIPTION("video stream multiplexer");
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> +MODULE_LICENSE("GPL");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver
From: Peter Rosin @ 2017-04-29 21:42 UTC (permalink / raw)
To: Philipp Zabel, linux-media-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Steve Longerbeam, Sakari Ailus,
Pavel Machek, Rob Herring, Mark Rutland, Vladimir Zapolskiy,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Sascha Hauer, Steve Longerbeam
In-Reply-To: <beb9f7c4-4959-1bb2-03e2-c5ccecbb8368-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 2017-04-29 23:29, Peter Rosin wrote:
> On 2017-04-28 16:13, Philipp Zabel wrote:
>> This driver can handle SoC internal and external video bus multiplexers,
>> controlled by mux controllers provided by the mux controller framework,
>> such as MMIO register bitfields or GPIOs. The subdevice passes through
>> the mbus configuration of the active input to the output side.
>>
>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> ---
>> This has been last sent as part of the i.MX media series.
>>
>> Changes since https://patchwork.kernel.org/patch/9647869/:
>> - Split out the actual mux operation to be provided by the mux controller
>> framework [1]. GPIO and MMIO control can be provided by individual mux
>> controller drivers [2][3].
>> [1] https://patchwork.kernel.org/patch/9695837/
>> [2] https://patchwork.kernel.org/patch/9695839/
>> [3] https://patchwork.kernel.org/patch/9704509/
>> - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>> vidsw with video_mux.
>> - Made the mux inactive by default, only activated by user interaction.
>> - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>> - Reuse subdev.entity.num_pads instead of keeping our own count.
>> - Removed implicit link disabling. Instead, trying to enable a second
>> sink pad link yields -EBUSY.
>> - Merged _async_init into _probe.
>> - Removed superfluous pad index check from _set_format.
>> - Added is_source_pad helper to tell source and sink pads apart.
>> - Removed test for status property in endpoint nodes. Disable the remote
>> device or sever the endpoint link to disable a sink pad.
>> ---
>> drivers/media/platform/Kconfig | 6 +
>> drivers/media/platform/Makefile | 2 +
>> drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 349 insertions(+)
>> create mode 100644 drivers/media/platform/video-mux.c
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index c9106e105baba..b046a6d39fee5 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>> To compile this driver as a module, choose M here: the
>> module will be called arv.
>>
>> +config VIDEO_MUX
>> + tristate "Video Multiplexer"
>> + depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
>> + help
>> + This driver provides support for N:1 video bus multiplexers.
>> +
>> config VIDEO_OMAP3
>> tristate "OMAP 3 Camera support"
>> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 349ddf6a69da2..fd2735ca3ff75 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o
>>
>> obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE) += m2m-deinterlace.o
>>
>> +obj-$(CONFIG_VIDEO_MUX) += video-mux.o
>> +
>> obj-$(CONFIG_VIDEO_S3C_CAMIF) += s3c-camif/
>> obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) += exynos4-is/
>> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> new file mode 100644
>> index 0000000000000..419541729f67e
>> --- /dev/null
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * video stream multiplexer controlled via mux control
>> + *
>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> 2017?
>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/platform_device.h>
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +
>> +struct video_mux {
>> + struct v4l2_subdev subdev;
>> + struct media_pad *pads;
>> + struct v4l2_mbus_framefmt *format_mbus;
>> + struct v4l2_of_endpoint *endpoint;
>> + struct mux_control *mux;
>> + int active;
>> +};
>> +
>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>> +{
>> + return container_of(sd, struct video_mux, subdev);
>> +}
>> +
>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
>> +{
>> + return pad == vmux->subdev.entity.num_pads - 1;
>> +}
>> +
>> +static int video_mux_link_setup(struct media_entity *entity,
>> + const struct media_pad *local,
>> + const struct media_pad *remote, u32 flags)
>> +{
>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + int ret;
>> +
>> + /*
>> + * The mux state is determined by the enabled sink pad link.
>> + * Enabling or disabling the source pad link has no effect.
>> + */
>> + if (is_source_pad(vmux, local->index))
>> + return 0;
>> +
>> + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
>> + remote->entity->name, remote->index, local->entity->name,
>> + local->index, flags & MEDIA_LNK_FL_ENABLED);
>> +
>> + if (flags & MEDIA_LNK_FL_ENABLED) {
>> + if (vmux->active == local->index)
>
> Here, you shortcut the mux_control_select_trylock test and return "OK"
> based on a driver-local variable that is intended to keep track of mux
> ownership.
>
>> + return 0;
>> +
>> + if (vmux->active >= 0)
>
> Here too (and this check is not needed, the situation will be covered by
> the mux_control_try_select call).
>
>> + return -EBUSY;
>> +
>> + dev_dbg(sd->dev, "setting %d active\n", local->index);
>> + ret = mux_control_try_select(vmux->mux, local->index);
>> + if (ret < 0)
>> + return ret;
>> + vmux->active = local->index;
>> + } else {
>> + if (vmux->active != local->index)
>> + return 0;
>> +
>> + dev_dbg(sd->dev, "going inactive\n");
>> + mux_control_deselect(vmux->mux);
>
> But here you let go of the mux *before* you clear the driver-local
> ownership indicator. That looks suspicious. My guess is that this is
> "safe" because the upper layers has some serialization, but I don't
> know. Anyway, even if there is something saving you in the upper
> layers, it looks out of order and unneeded. I would have moved the
> below vmux->active = -1; statement up to before the above deselect.
>
> With that fixed, mux usage looks good to me, so you can add an Acked-
> by from me if you wish (goes for the bindings patch as well).
Ouch, that was a bit too soon. If there is *no* serialization in the
upper layers, this is *not* ok, even with my reordering. There must be
only one call to mux_control_deselect, and w/o serialization there
is a race where you might get multiple deselect calls when several
callers makes it through the active != index check before any of them
manages to set active = -1. That race must be taken care of!
Cheers,
peda
>> + vmux->active = -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct media_entity_operations video_mux_ops = {
>> + .link_setup = video_mux_link_setup,
>> + .link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +static bool video_mux_endpoint_disabled(struct device_node *ep)
>> +{
>> + struct device_node *rpp = of_graph_get_remote_port_parent(ep);
>> +
>> + return !of_device_is_available(rpp);
>> +}
>> +
>> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
>> + struct v4l2_mbus_config *cfg)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + struct v4l2_of_endpoint *endpoint;
>> + struct media_pad *pad;
>> + int ret;
>> +
>> + if (vmux->active == -1) {
>> + dev_err(sd->dev, "no configuration for inactive mux\n");
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Retrieve media bus configuration from the entity connected to the
>> + * active input
>> + */
>> + pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
>> + if (pad) {
>> + sd = media_entity_to_v4l2_subdev(pad->entity);
>> + ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>> + if (ret == -ENOIOCTLCMD)
>> + pad = NULL;
>> + else if (ret < 0) {
>> + dev_err(sd->dev, "failed to get source configuration\n");
>> + return ret;
>> + }
>> + }
>> + if (!pad) {
>> + endpoint = &vmux->endpoint[vmux->active];
>> +
>> + /* Mirror the input side on the output side */
>> + cfg->type = endpoint->bus_type;
>> + if (cfg->type == V4L2_MBUS_PARALLEL ||
>> + cfg->type == V4L2_MBUS_BT656)
>> + cfg->flags = endpoint->bus.parallel.flags;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + struct v4l2_subdev *upstream_sd;
>> + struct media_pad *pad;
>> +
>> + if (vmux->active == -1) {
>> + dev_err(sd->dev, "Can not start streaming on inactive mux\n");
>> + return -EINVAL;
>> + }
>> +
>> + pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
>> + if (!pad) {
>> + dev_err(sd->dev, "Failed to find remote source pad\n");
>> + return -ENOLINK;
>> + }
>> +
>> + if (!is_media_entity_v4l2_subdev(pad->entity)) {
>> + dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
>> + return -ENODEV;
>> + }
>> +
>> + upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
>> +
>> + return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
>> + .g_mbus_config = video_mux_g_mbus_config,
>> + .s_stream = video_mux_s_stream,
>> +};
>> +
>> +static struct v4l2_mbus_framefmt *
>> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + unsigned int pad, u32 which)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +
>> + switch (which) {
>> + case V4L2_SUBDEV_FORMAT_TRY:
>> + return v4l2_subdev_get_try_format(sd, cfg, pad);
>> + case V4L2_SUBDEV_FORMAT_ACTIVE:
>> + return &vmux->format_mbus[pad];
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> +static int video_mux_get_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *sdformat)
>> +{
>> + sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> + sdformat->which);
>> + return 0;
>> +}
>> +
>> +static int video_mux_set_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *sdformat)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + struct v4l2_mbus_framefmt *mbusformat;
>> +
>> + mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> + sdformat->which);
>> + if (!mbusformat)
>> + return -EINVAL;
>> +
>> + /* Source pad mirrors active sink pad, no limitations on sink pads */
>> + if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
>> + sdformat->format = vmux->format_mbus[vmux->active];
>> +
>> + *mbusformat = sdformat->format;
>> +
>> + return 0;
>> +}
>> +
>> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
>> + .get_fmt = video_mux_get_format,
>> + .set_fmt = video_mux_set_format,
>> +};
>> +
>> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
>> + .pad = &video_mux_pad_ops,
>> + .video = &video_mux_subdev_video_ops,
>> +};
>> +
>> +static int video_mux_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> + struct v4l2_of_endpoint endpoint;
>> + struct device_node *ep;
>> + struct video_mux *vmux;
>> + unsigned int num_pads = 0;
>> + int ret;
>> + int i;
>> +
>> + vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
>> + if (!vmux)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, vmux);
>> +
>> + v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
>> + snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
>> + vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> + vmux->subdev.dev = dev;
>> +
>> + /*
>> + * The largest numbered port is the output port. It determines
>> + * total number of pads.
>> + */
>> + for_each_endpoint_of_node(np, ep) {
>> + of_graph_parse_endpoint(ep, &endpoint.base);
>> + num_pads = max(num_pads, endpoint.base.port + 1);
>> + }
>> +
>> + if (num_pads < 2) {
>> + dev_err(dev, "Not enough ports %d\n", num_pads);
>> + return -EINVAL;
>> + }
>> +
>> + vmux->mux = devm_mux_control_get(dev, NULL);
>> + if (IS_ERR(vmux->mux)) {
>> + ret = PTR_ERR(vmux->mux);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Failed to get mux: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + vmux->active = -1;
>> + vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
>> + GFP_KERNEL);
>> + vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
>> + num_pads, GFP_KERNEL);
>> + vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
>> + (num_pads - 1), GFP_KERNEL);
>> +
>> + for (i = 0; i < num_pads - 1; i++)
>> + vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
>> + vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
>> + ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
>> + vmux->pads);
>> + if (ret < 0)
>> + return ret;
>> +
>> + vmux->subdev.entity.ops = &video_mux_ops;
>> +
>> + for_each_endpoint_of_node(np, ep) {
>> + v4l2_of_parse_endpoint(ep, &endpoint);
>> +
>> + if (video_mux_endpoint_disabled(ep)) {
>> + dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
>> + continue;
>> + }
>> +
>> + vmux->endpoint[endpoint.base.port] = endpoint;
>> + }
>> +
>> + return v4l2_async_register_subdev(&vmux->subdev);
>> +}
>> +
>> +static int video_mux_remove(struct platform_device *pdev)
>> +{
>> + struct video_mux *vmux = platform_get_drvdata(pdev);
>> + struct v4l2_subdev *sd = &vmux->subdev;
>> +
>> + v4l2_async_unregister_subdev(sd);
>> + media_entity_cleanup(&sd->entity);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id video_mux_dt_ids[] = {
>> + { .compatible = "video-mux", },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
>> +
>> +static struct platform_driver video_mux_driver = {
>> + .probe = video_mux_probe,
>> + .remove = video_mux_remove,
>> + .driver = {
>> + .of_match_table = video_mux_dt_ids,
>> + .name = "video-mux",
>> + },
>> +};
>> +
>> +module_platform_driver(video_mux_driver);
>> +
>> +MODULE_DESCRIPTION("video stream multiplexer");
>> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
>> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
>> +MODULE_LICENSE("GPL");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v1 1/2] PCI: mediatek: Add Mediatek PCIe host controller support
From: kbuild test robot @ 2017-04-29 23:28 UTC (permalink / raw)
Cc: kbuild-all, Bjorn Helgaas, Rob Herring, Arnd Bergmann, linux-pci,
devicetree, linux-mediatek, linux-arm-kernel, linux-kernel,
Red Hung, Ryder Lee
In-Reply-To: <1493370634-7038-2-git-send-email-ryder.lee@mediatek.com>
[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]
Hi Ryder,
[auto build test ERROR on pci/next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ryder-Lee/Add-PCIe-host-driver-support-for-Mediatek-SoCs/20170429-181028
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `mtk_pcie_probe':
>> drivers/pci/host/.tmp_gl_pcie-mediatek.o:(.text+0x5b68c): undefined reference to `pci_fixup_irqs'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45164 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/4] of: remove *phandle properties from expanded device tree
From: kbuild test robot @ 2017-04-30 0:22 UTC (permalink / raw)
To: frowand.list
Cc: kbuild-all, Rob Herring, stephen.boyd, devicetree, linux-kernel
In-Reply-To: <1493110049-30165-2-git-send-email-frowand.list@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8074 bytes --]
Hi Frank,
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/frowand-list-gmail-com/of-remove-phandle-properties-from-expanded-device-tree/20170426-000149
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
In file included from include/linux/kobject.h:21:0,
from include/linux/device.h:17,
from include/linux/node.h:17,
from include/linux/cpu.h:16,
from drivers/of/base.c:25:
drivers/of/base.c: In function '__of_add_phandle_sysfs':
>> drivers/of/base.c:198:23: error: 'pp' undeclared (first use in this function)
sysfs_bin_attr_init(&pp->attr);
^
include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
(attr)->key = &__key; \
^~~~
drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
sysfs_bin_attr_init(&pp->attr);
^~~~~~~~~~~~~~~~~~~
drivers/of/base.c:198:23: note: each undeclared identifier is reported only once for each function it appears in
sysfs_bin_attr_init(&pp->attr);
^
include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
(attr)->key = &__key; \
^~~~
drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
sysfs_bin_attr_init(&pp->attr);
^~~~~~~~~~~~~~~~~~~
vim +/pp +198 drivers/of/base.c
19 */
20
21 #define pr_fmt(fmt) "OF: " fmt
22
23 #include <linux/console.h>
24 #include <linux/ctype.h>
> 25 #include <linux/cpu.h>
26 #include <linux/module.h>
27 #include <linux/of.h>
28 #include <linux/of_device.h>
29 #include <linux/of_graph.h>
30 #include <linux/spinlock.h>
31 #include <linux/slab.h>
32 #include <linux/string.h>
33 #include <linux/proc_fs.h>
34
35 #include "of_private.h"
36
37 LIST_HEAD(aliases_lookup);
38
39 struct device_node *of_root;
40 EXPORT_SYMBOL(of_root);
41 struct device_node *of_chosen;
42 struct device_node *of_aliases;
43 struct device_node *of_stdout;
44 static const char *of_stdout_options;
45
46 struct kset *of_kset;
47
48 /*
49 * Used to protect the of_aliases, to hold off addition of nodes to sysfs.
50 * This mutex must be held whenever modifications are being made to the
51 * device tree. The of_{attach,detach}_node() and
52 * of_{add,remove,update}_property() helpers make sure this happens.
53 */
54 DEFINE_MUTEX(of_mutex);
55
56 /* use when traversing tree through the child, sibling,
57 * or parent members of struct device_node.
58 */
59 DEFINE_RAW_SPINLOCK(devtree_lock);
60
61 int of_n_addr_cells(struct device_node *np)
62 {
63 const __be32 *ip;
64
65 do {
66 if (np->parent)
67 np = np->parent;
68 ip = of_get_property(np, "#address-cells", NULL);
69 if (ip)
70 return be32_to_cpup(ip);
71 } while (np->parent);
72 /* No #address-cells property for the root node */
73 return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
74 }
75 EXPORT_SYMBOL(of_n_addr_cells);
76
77 int of_n_size_cells(struct device_node *np)
78 {
79 const __be32 *ip;
80
81 do {
82 if (np->parent)
83 np = np->parent;
84 ip = of_get_property(np, "#size-cells", NULL);
85 if (ip)
86 return be32_to_cpup(ip);
87 } while (np->parent);
88 /* No #size-cells property for the root node */
89 return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
90 }
91 EXPORT_SYMBOL(of_n_size_cells);
92
93 #ifdef CONFIG_NUMA
94 int __weak of_node_to_nid(struct device_node *np)
95 {
96 return NUMA_NO_NODE;
97 }
98 #endif
99
100 #ifndef CONFIG_OF_DYNAMIC
101 static void of_node_release(struct kobject *kobj)
102 {
103 /* Without CONFIG_OF_DYNAMIC, no nodes gets freed */
104 }
105 #endif /* CONFIG_OF_DYNAMIC */
106
107 struct kobj_type of_node_ktype = {
108 .release = of_node_release,
109 };
110
111 static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
112 struct bin_attribute *bin_attr, char *buf,
113 loff_t offset, size_t count)
114 {
115 struct property *pp = container_of(bin_attr, struct property, attr);
116 return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
117 }
118
119 static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
120 struct bin_attribute *bin_attr, char *buf,
121 loff_t offset, size_t count)
122 {
123 phandle phandle;
124 struct device_node *np;
125
126 np = container_of(bin_attr, struct device_node, attr_phandle);
127 phandle = cpu_to_be32(np->phandle);
128 return memory_read_from_buffer(buf, count, &offset, &phandle,
129 sizeof(phandle));
130 }
131
132 /* always return newly allocated name, caller must free after use */
133 static const char *safe_name(struct kobject *kobj, const char *orig_name)
134 {
135 const char *name = orig_name;
136 struct kernfs_node *kn;
137 int i = 0;
138
139 /* don't be a hero. After 16 tries give up */
140 while (i < 16 && (kn = sysfs_get_dirent(kobj->sd, name))) {
141 sysfs_put(kn);
142 if (name != orig_name)
143 kfree(name);
144 name = kasprintf(GFP_KERNEL, "%s#%i", orig_name, ++i);
145 }
146
147 if (name == orig_name) {
148 name = kstrdup(orig_name, GFP_KERNEL);
149 } else {
150 pr_warn("Duplicate name in %s, renamed to \"%s\"\n",
151 kobject_name(kobj), name);
152 }
153 return name;
154 }
155
156 int __of_add_property_sysfs(struct device_node *np, struct property *pp)
157 {
158 int rc;
159
160 /* Important: Don't leak passwords */
161 bool secure = strncmp(pp->name, "security-", 9) == 0;
162
163 if (!IS_ENABLED(CONFIG_SYSFS))
164 return 0;
165
166 if (!of_kset || !of_node_is_attached(np))
167 return 0;
168
169 sysfs_bin_attr_init(&pp->attr);
170 pp->attr.attr.name = safe_name(&np->kobj, pp->name);
171 pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
172 pp->attr.size = secure ? 0 : pp->length;
173 pp->attr.read = of_node_property_read;
174
175 rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
176 WARN(rc, "error adding attribute %s to node %s\n", pp->name, np->full_name);
177 return rc;
178 }
179
180 /*
181 * In the imported device tree (fdt), phandle is a property. In the
182 * internal data structure it is instead stored in the struct device_node.
183 * Make phandle visible in sysfs as if it was a property.
184 */
185 int __of_add_phandle_sysfs(struct device_node *np)
186 {
187 int rc;
188
189 if (!IS_ENABLED(CONFIG_SYSFS))
190 return 0;
191
192 if (!of_kset || !of_node_is_attached(np))
193 return 0;
194
195 if (!np->phandle || np->phandle == 0xffffffff)
196 return 0;
197
> 198 sysfs_bin_attr_init(&pp->attr);
199 np->attr_phandle.attr.name = "phandle";
200 np->attr_phandle.attr.mode = 0444;
201 np->attr_phandle.size = sizeof(np->phandle);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45067 bytes --]
^ permalink raw reply
* Re: [PATCH v1 1/2] PCI: mediatek: Add Mediatek PCIe host controller support
From: kbuild test robot @ 2017-04-30 4:25 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, Bjorn Helgaas, Rob Herring, Arnd Bergmann,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Red Hung, Ryder Lee
In-Reply-To: <1493370634-7038-2-git-send-email-ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]
Hi Ryder,
[auto build test ERROR on pci/next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ryder-Lee/Add-PCIe-host-driver-support-for-Mediatek-SoCs/20170429-181028
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `pci_host_bridge_of_msi_domain':
>> (.text+0x2f154): undefined reference to `pci_fixup_irqs'
drivers/built-in.o: In function `mtk_pcie_probe':
pcie-mediatek.c:(.text+0x2f956): undefined reference to `pci_fixup_irqs'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49161 bytes --]
^ permalink raw reply
* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:24 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Mårten Lindahl, jic23-DgEjT+Ai2ygdnm+yROfE0A,
lars-Qo5EllUWu/uELgA04lAiVw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.20.1704212149070.2470-M0QeDd4q1oXQbIPoIc8EuQ@public.gmane.org>
On Fri, 2017-04-21 at 22:19 +0200, Peter Meerwald-Stadler wrote:
> > From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
>
> comments below
Hi! Thanks for the comments! Please see my reply below.
Thanks,
Mårten
>
> > This adds support for the Texas Instruments ADC084S021 ADC chip.
> >
> > Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> > ---
> > .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
> > 4 files changed, 380 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > new file mode 100644
> > index 0000000..921eb46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > @@ -0,0 +1,25 @@
> > +* Texas Instruments' ADC084S021
> > +
> > +Required properties:
> > + - compatible : Must be "ti,adc084s021"
> > + - reg : SPI chip select number for the device
> > + - vref-supply : The regulator supply for ADC reference voltage
> > + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> > +
> > +Optional properties:
> > + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
> > + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
> > + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
> > +
> > +
> > +Example:
> > +adc@0 {
> > + compatible = "ti,adc084s021";
> > + reg = <0>;
> > + vref-supply = <&adc_vref>;
> > + spi-cpol;
> > + spi-cpha;
> > + spi-cs-high;
> > + spi-max-frequency = <16000000>;
> > + pl022,com-mode = <0x2>; /* DMA */
>
> what is this?
It does not belong here. It is removed i v2. This dt-bindings part is
split into its own patch in v2.
>
> > +};
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index dedae7a..13141e5 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -560,6 +560,18 @@ config TI_ADC0832
> > This driver can also be built as a module. If so, the module will be
> > called ti-adc0832.
> >
> > +config TI_ADC084S021
> > + tristate "Texas Instruments ADC084S021"
> > + depends on SPI
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + If you say yes here you get support for Texas Instruments ADC084S021
> > + chips.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called ti-adc084s021.
> > +
> > config TI_ADC12138
> > tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index d001262..b1a6158 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> > obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> > obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> > +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> > obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> > obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> > obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> > diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
> > new file mode 100644
> > index 0000000..4f33b91
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-adc084s021.c
> > @@ -0,0 +1,342 @@
> > +/**
> > + * Copyright (C) 2017 Axis Communications AB
> > + *
> > + * Driver for Texas Instruments' ADC084S021 ADC chip.
> > + * Datasheets can be found here:
> > + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define MODULE_NAME "adc084s021"
> > +#define DRIVER_VERSION "1.0"
>
> is only used once at the very end...
Removed in v2.
>
> > +#define ADC_RESOLUTION 8
> > +#define ADC_N_CHANNELS 4
>
> we want a consistent prefix, such as ADC084S021_
I use values inline i v2.
>
> > +
> > +struct adc084s021_configuration {
> > + const struct iio_chan_spec *channels;
> > + u8 num_channels;
>
> no need for u8, perhaps unsigned?
I removed this struct in v2 and use direct addressing.
>
> > +};
> > +
> > +struct adc084s021 {
> > + struct spi_device *spi;
> > + struct spi_message message;
> > + struct spi_transfer spi_trans[2];
> > + struct regulator *reg;
> > + struct mutex lock;
> > + /*
> > + * DMA (thus cache coherency maintenance) requires the
> > + * transfer buffers to live in their own cache lines.
> > + */
> > + union {
> > + u8 tx_buf[2];
> > + u8 rx_buf[2];
> > + } ____cacheline_aligned;
> > + u8 cur_adc_values[ADC_N_CHANNELS];
> > +};
> > +
> > +/**
> > + * Event triggered when value changes on a channel
> > + */
> > +static const struct iio_event_spec adc084s021_event = {
> > + .type = IIO_EV_TYPE_CHANGE,
> > + .dir = IIO_EV_DIR_NONE,
> > +};
> > +
> > +/**
> > + * Channel specification
> > + */
> > +#define ADC084S021_VOLTAGE_CHANNEL(num) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .channel = (num), \
> > + .address = (num << 3), \
>
> parenthesis should be around (num)
Fixed in v2.
>
> > + .indexed = 1, \
> > + .scan_index = num, \
>
> parenthesis?
Fixed in v2.
>
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 8, \
> > + .storagebits = 32, \
> > + .shift = 24 - ((num << 3)), \
>
> no need for (( )) around the expression, parenthesis should be around num
>
> the shift doesn't make sense, you are shifting in
>
> adc084s021_adc_conversion() already and return just 8 bits (so storagebits
> should be 8)?
>
> you could/should use realbits = 8, storagebits = 16, shift = 4 and
> endianness = IIO_BE if I read Figure 1 of the datasheet correctly
>
This macro has been fixed according to your suggestion (and the
datasheet) in v2.
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>
> likely this is missing _SCALE
> IIO expects data to be returned in millivolt, see
> Documentation/ABI/testing/sysfs-bus-iio
>
Added IIO_CHAN_INFO_SCALE in v2.
> > + .event_spec = &adc084s021_event, \
> > + .num_event_specs = 1, \
>
> ARRAY_SIZE(&adc084s021_event) to be extensible?
Removed events in v2.
>
> > + }
> > +
> > +static const struct iio_chan_spec adc084s021_channels[] = {
> > + ADC084S021_VOLTAGE_CHANNEL(0),
> > + ADC084S021_VOLTAGE_CHANNEL(1),
> > + ADC084S021_VOLTAGE_CHANNEL(2),
> > + ADC084S021_VOLTAGE_CHANNEL(3),
> > + IIO_CHAN_SOFT_TIMESTAMP(4),
> > +};
> > +
> > +static const struct adc084s021_configuration adc084s021_config[] = {
> > + { adc084s021_channels, ARRAY_SIZE(adc084s021_channels) },
>
> for just one configuration, this is not really needed; so you plan/forsee
> more chips being added soonish?
No more than this chip supported by this driver, so I use direct
addressing in v2.
>
> > +};
> > +
> > +/**
> > + * Read an ADC channel and return its value.
> > + *
> > + * @adc: The ADC SPI data.
> > + * @channel: The IIO channel data structure.
> > + */
> > +static int adc084s021_adc_conversion(struct adc084s021 *adc,
> > + struct iio_chan_spec const *channel)
> > +{
> > + u16 value;
>
> value should be u8, but is not really needed
Fixed in v2.
>
> > + int ret;
> > +
> > + mutex_lock(&adc->lock);
> > + adc->tx_buf[0] = channel->address;
> > +
> > + /* Do the transfer */
> > + ret = spi_sync(adc->spi, &adc->message);
> > +
>
> no newline here please
Fixed in v2.
>
> > + if (ret < 0) {
> > + mutex_unlock(&adc->lock);
> > + return ret;
> > + }
> > +
> > + value = (adc->rx_buf[0] << 4) | (adc->rx_buf[1] >> 4);
>
> I recommend using __be16 for rx_buf and
> ret = (be16_to_cpu(adc->rx_buf) >> 4) & 0xff;
Fixed in v2.
>
> > + mutex_unlock(&adc->lock);
> > +
> > + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
> > + value, channel->channel);
> > + return value;
> > +}
> > +
> > +/**
> > + * Make a readout of requested IIO channel info.
>
> no need to document this, it is found in every IIO driver...
Removed documentation for standard driver functions in v2.
>
> > + *
> > + * @indio_dev: The industrial I/O device.
> > + * @channel: The IIO channel data structure.
> > + * @val: First element of value (integer).
> > + * @val2: Second element of value (fractional).
> > + * @mask: The info_mask to read.
> > + */
> > +static int adc084s021_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + struct adc084s021 *adc = iio_priv(indio_dev);
> > + int retval;
>
> how about using ret everywhere?
Fixed in v2.
>
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
>
> probably want iio_device_claim_direct_mode() so to not interfere with
> buffered access
Fixed in v2.
>
> > + retval = adc084s021_adc_conversion(adc, channel);
> > + if (retval < 0)
> > + return retval;
> > +
> > + *val = retval;
> > + return IIO_VAL_INT;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +/**
> > + * Read enabled ADC channels and push data to the buffer.
> > + *
> > + * @irq: The interrupt number (not used).
> > + * @pollfunc: Pointer to the poll func.
> > + */
> > +static irqreturn_t adc084s021_trigger_handler(int irq, void *pollfunc)
> > +{
> > + struct iio_poll_func *pf = pollfunc;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct adc084s021 *adc = iio_priv(indio_dev);
> > + u8 *data;
> > + s64 timestamp;
> > + int value, scan_index;
> > +
> > + data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>
> pre-allocate buffer with maximum size statically; allocation is
> potentially expensive; don't forget space for the timestamp and padding...
Fixed in v2.
>
> > + if (!data) {
> > + iio_trigger_notify_done(indio_dev->trig);
> > + return IRQ_NONE;
> > + }
> > +
> > + timestamp = iio_get_time_ns(indio_dev);
> > +
> > + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
> > + const struct iio_chan_spec *channel =
> > + &indio_dev->channels[scan_index];
> > + value = adc084s021_adc_conversion(adc, channel);
>
> lock is taken and released for each channel, probably want to do it just
> once?
Fixed in v2.
>
> > + data[scan_index] = value;
> > +
> > + /*
> > + * Compare read data to previous read. If it differs send
> > + * event notification for affected channel.
> > + */
> > + if (adc->cur_adc_values[scan_index] != (u8)value) {
>
> cur_adc_values is not initialized (probably set to 0);
> so on first read, should the notification be sent?
Events no longer used in v2, so no need for this check anymore.
>
> > + adc->cur_adc_values[scan_index] = (u8)value;
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_VOLTAGE, 0,
> > + IIO_NO_MOD, IIO_EV_DIR_NONE,
> > + IIO_EV_TYPE_CHANGE,
> > + channel->channel, 0, 0),
> > + timestamp);
> > + dev_dbg(&indio_dev->dev,
> > + "new value on ch%d: 0x%02X (ts %llu)\n",
> > + channel->channel, value, timestamp);
> > + }
> > + }
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> > + iio_trigger_notify_done(indio_dev->trig);
> > + kfree(data);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info adc084s021_info = {
> > + .read_raw = adc084s021_read_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +/**
> > + * Create and register ADC IIO device for SPI.
>
> comment is rather pointless
Fixed in v2.
>
> > + */
> > +static int adc084s021_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct adc084s021 *adc;
> > + int config = spi_get_device_id(spi)->driver_data;
> > + int retval;
>
> ret maybe
Using ret everywhere in v2.
>
> > +
> > + /* Allocate an Industrial I/O device */
>
> obviously...
:) Fixed in v2.
>
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > + if (!indio_dev) {
> > + dev_err(&spi->dev, "Failed to allocate IIO device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + adc = iio_priv(indio_dev);
> > + adc->spi = spi;
> > + spi->bits_per_word = ADC_RESOLUTION;
> > +
> > + /* Update the SPI device with config and connect the iio dev */
> > + retval = spi_setup(spi);
> > + if (retval) {
> > + dev_err(&spi->dev, "Failed to update SPI device\n");
> > + return retval;
> > + }
> > + spi_set_drvdata(spi, indio_dev);
> > +
> > + /* Initiate the Industrial I/O device */
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->dev.of_node = spi->dev.of_node;
> > + indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &adc084s021_info;
> > + indio_dev->channels = adc084s021_config[config].channels;
> > + indio_dev->num_channels = adc084s021_config[config].num_channels;
>
> could do it directly as long there is just one configuration
Yes, fixed in v2.
>
> > +
> > + /* Create SPI transfer for channel reads */
> > + adc->spi_trans[0].tx_buf = &adc->tx_buf[0];
> > + adc->spi_trans[0].len = 2;
> > + adc->spi_trans[0].speed_hz = spi->max_speed_hz;
> > + adc->spi_trans[0].bits_per_word = spi->bits_per_word;
> > + adc->spi_trans[1].rx_buf = &adc->rx_buf[0];
> > + adc->spi_trans[1].len = 2;
> > + adc->spi_trans[1].speed_hz = spi->max_speed_hz;
> > + adc->spi_trans[1].bits_per_word = spi->bits_per_word;
> > +
> > + /* Setup SPI message for channel reads */
> > + spi_message_init(&adc->message);
> > + spi_message_add_tail(&adc->spi_trans[0], &adc->message);
> > + spi_message_add_tail(&adc->spi_trans[1], &adc->message);
> > +
> > + adc->reg = devm_regulator_get(&spi->dev, "vref");
> > + if (IS_ERR(adc->reg))
> > + return PTR_ERR(adc->reg);
> > +
> > + retval = regulator_enable(adc->reg);
> > + if (retval < 0)
> > + return retval;
> > +
> > + mutex_init(&adc->lock);
> > +
> > + /* Setup triggered buffer with pollfunction */
> > + retval = iio_triggered_buffer_setup(indio_dev, NULL,
>
> devm_()
I have not changed this yet in v2 since Jonathan has another opinion
about this.
>
> > + adc084s021_trigger_handler, NULL);
> > + if (retval) {
> > + dev_err(&spi->dev, "Failed to setup triggered buffer\n");
> > + goto buffer_setup_failed;
> > + }
> > +
> > + retval = iio_device_register(indio_dev);
> > + if (retval) {
> > + dev_err(&spi->dev, "Failed to register IIO device\n");
> > + goto device_register_failed;
> > + }
> > +
> > + dev_info(&spi->dev, "probed!\n");
>
> no logging please, just outputs clutter
Fixed in v2.
>
> > + return 0;
> > +
> > +device_register_failed:
> > + iio_triggered_buffer_cleanup(indio_dev);
> > +buffer_setup_failed:
> > + regulator_disable(adc->reg);
> > + return retval;
> > +}
> > +
> > +/**
> > + * Unregister ADC IIO device for SPI.
> > + */
> > +static int adc084s021_remove(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > + struct adc084s021 *adc = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > + iio_triggered_buffer_cleanup(indio_dev);
> > + regulator_disable(adc->reg);
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id adc084s021_of_match[] = {
> > + { .compatible = "ti,adc084s021", },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, adc084s021_of_match);
> > +
> > +static const struct spi_device_id adc084s021_id[] = {
> > + { MODULE_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(spi, adc084s021_id);
> > +
> > +static struct spi_driver adc084s021_driver = {
> > + .driver = {
> > + .name = MODULE_NAME,
> > + .of_match_table = of_match_ptr(adc084s021_of_match),
> > + },
> > + .probe = adc084s021_probe,
> > + .remove = adc084s021_remove,
> > + .id_table = adc084s021_id,
> > +};
> > +
> > +module_spi_driver(adc084s021_driver);
> > +
> > +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
> > +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_VERSION(DRIVER_VERSION);
> >
>
^ permalink raw reply
* Re: [PATCH] regulator: Allow for asymmetric settling times
From: Mark Brown @ 2017-04-30 12:30 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Matthias Kaehlcke, Liam Girdwood, Rob Herring, Mark Rutland,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris
In-Reply-To: <59044881.7090901-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]
On Sat, Apr 29, 2017 at 01:32:09PM +0530, Laxman Dewangan wrote:
> On Saturday 29 April 2017 05:36 AM, Matthias Kaehlcke wrote:
> > -- regulator-settling-time-us: Settling time, in microseconds, for voltage
> > - change if regulator have the constant time for any level voltage change.
> > - This is useful when regulator have exponential voltage change.
> > +- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
> > + increase if the regulator needs a constant time to settle after voltage
> > + increases of any level. This is useful for regulators with exponential
> > + voltage changes.
> > +- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
> > + decrease if the regulator needs a constant time to settle after voltage
> > + decreases of any level. This is useful for regulators with exponential
> > + voltage changes.
> Can we have regulator-settling-time-us also so if it is there then up/down
> same.
> If up/down different then separate properties can be used.
Removing the existing binding would also break existing DTs using it
which we obviously don't want. I don't see any reason to even deprecate
it, like Laxman says it's nice and convenient for people with symmetric
performance.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Peter Meerwald-Stadler, Mårten Lindahl,
lars-Qo5EllUWu/uELgA04lAiVw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5dec6eec-c214-c14d-8dc9-ff298854fc1c-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, 2017-04-23 at 20:13 +0100, Jonathan Cameron wrote:
> On 21/04/17 21:19, Peter Meerwald-Stadler wrote:
> >
> >> From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >
> > comments below
> A few more from me. Laptop battery gone flat so not so thorough on top few lines!
>
> Jonathan
Hi Jonathan! Thanks for the comments! Please see my reply below.
Thanks,
Mårten
> >
> >> This adds support for the Texas Instruments ADC084S021 ADC chip.
> >>
> >> Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >> ---
> >> .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
> >> drivers/iio/adc/Kconfig | 12 +
> >> drivers/iio/adc/Makefile | 1 +
> >> drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
> >> 4 files changed, 380 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> new file mode 100644
> >> index 0000000..921eb46
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> @@ -0,0 +1,25 @@
> >> +* Texas Instruments' ADC084S021
> >> +
> >> +Required properties:
> >> + - compatible : Must be "ti,adc084s021"
> >> + - reg : SPI chip select number for the device
> >> + - vref-supply : The regulator supply for ADC reference voltage
> >> + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> >> +
> >> +Optional properties:
> >> + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
> >> + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
> >> + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
> >> +
> >> +
> >> +Example:
> >> +adc@0 {
> >> + compatible = "ti,adc084s021";
> >> + reg = <0>;
> >> + vref-supply = <&adc_vref>;
> >> + spi-cpol;
> >> + spi-cpha;
> >> + spi-cs-high;
> >> + spi-max-frequency = <16000000>;
> >> + pl022,com-mode = <0x2>; /* DMA */
> >
> > what is this?
> >
> >> +};
> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >> index dedae7a..13141e5 100644
> >> --- a/drivers/iio/adc/Kconfig
> >> +++ b/drivers/iio/adc/Kconfig
> >> @@ -560,6 +560,18 @@ config TI_ADC0832
> >> This driver can also be built as a module. If so, the module will be
> >> called ti-adc0832.
> >>
> >> +config TI_ADC084S021
> >> + tristate "Texas Instruments ADC084S021"
> >> + depends on SPI
> >> + select IIO_BUFFER
> >> + select IIO_TRIGGERED_BUFFER
> >> + help
> >> + If you say yes here you get support for Texas Instruments ADC084S021
> >> + chips.
> >> +
> >> + This driver can also be built as a module. If so, the module will be
> >> + called ti-adc084s021.
> >> +
> >> config TI_ADC12138
> >> tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
> >> depends on SPI
> >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> index d001262..b1a6158 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> >> obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> >> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> >> obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> >> +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> >> obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> >> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> >> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> >> diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
> >> new file mode 100644
> >> index 0000000..4f33b91
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ti-adc084s021.c
> >> @@ -0,0 +1,342 @@
> >> +/**
> >> + * Copyright (C) 2017 Axis Communications AB
> >> + *
> >> + * Driver for Texas Instruments' ADC084S021 ADC chip.
> >> + * Datasheets can be found here:
> >> + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
> >> + *
> >> + * 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.
> >> + */
> >> +
> >> +#include <linux/err.h>
> >> +#include <linux/spi/spi.h>
> >> +#include <linux/module.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/iio/buffer.h>
> >> +#include <linux/iio/events.h>
> >> +#include <linux/iio/triggered_buffer.h>
> >> +#include <linux/iio/trigger_consumer.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#define MODULE_NAME "adc084s021"
> >> +#define DRIVER_VERSION "1.0"
> >
> > is only used once at the very end...
> >
> >> +#define ADC_RESOLUTION 8
> Put it inline...
Fixed in v2.
> >> +#define ADC_N_CHANNELS 4
> Belongs inline. No additional info provided by having it here.
Fixed in v2.
> >
> > we want a consistent prefix, such as ADC084S021_
> >
> >> +
> >> +struct adc084s021_configuration {
> >> + const struct iio_chan_spec *channels;
> >> + u8 num_channels;
> >
> > no need for u8, perhaps unsigned?
> >
> >> +};
> >> +
> >> +struct adc084s021 {
> >> + struct spi_device *spi;
> >> + struct spi_message message;
> >> + struct spi_transfer spi_trans[2];
> >> + struct regulator *reg;
> >> + struct mutex lock;
> >> + /*
> >> + * DMA (thus cache coherency maintenance) requires the
> >> + * transfer buffers to live in their own cache lines.
> >> + */
> >> + union {
> >> + u8 tx_buf[2];
> >> + u8 rx_buf[2];
> >> + } ____cacheline_aligned;
> >> + u8 cur_adc_values[ADC_N_CHANNELS];
> >> +};
> >> +
> >> +/**
> >> + * Event triggered when value changes on a channel
> >> + */
> >> +static const struct iio_event_spec adc084s021_event = {
> >> + .type = IIO_EV_TYPE_CHANGE,
> >> + .dir = IIO_EV_DIR_NONE,
> >> +};
> Not the intent of that type of event at all.
I removed using events in v2.
> >> +
> >> +/**
> >> + * Channel specification
> >> + */
> >> +#define ADC084S021_VOLTAGE_CHANNEL(num) \
> >> + { \
> >> + .type = IIO_VOLTAGE, \
> >> + .channel = (num), \
> >> + .address = (num << 3), \
> >
> > parenthesis should be around (num)
> >
> >> + .indexed = 1, \
> >> + .scan_index = num, \
> >
> > parenthesis?
> >
> >> + .scan_type = { \
> >> + .sign = 'u', \
> >> + .realbits = 8, \
> >> + .storagebits = 32, \
> >> + .shift = 24 - ((num << 3)), \
> >
> > no need for (( )) around the expression, parenthesis should be around num
> >
> > the shift doesn't make sense, you are shifting in
> >
> > adc084s021_adc_conversion() already and return just 8 bits (so storagebits
> > should be 8)?
> >
> > you could/should use realbits = 8, storagebits = 16, shift = 4 and
> > endianness = IIO_BE if I read Figure 1 of the datasheet correctly
> >
> >> + }, \
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> >
> > likely this is missing _SCALE
> > IIO expects data to be returned in millivolt, see
> > Documentation/ABI/testing/sysfs-bus-iio
> >
> >> + .event_spec = &adc084s021_event, \
> >> + .num_event_specs = 1, \
> >
> > ARRAY_SIZE(&adc084s021_event) to be extensible?
> >
> >> + }
> >> +
> >> +static const struct iio_chan_spec adc084s021_channels[] = {
> >> + ADC084S021_VOLTAGE_CHANNEL(0),
> >> + ADC084S021_VOLTAGE_CHANNEL(1),
> >> + ADC084S021_VOLTAGE_CHANNEL(2),
> >> + ADC084S021_VOLTAGE_CHANNEL(3),
> >> + IIO_CHAN_SOFT_TIMESTAMP(4),
> >> +};
> >> +
> >> +static const struct adc084s021_configuration adc084s021_config[] = {
> >> + { adc084s021_channels, ARRAY_SIZE(adc084s021_channels) },
> >
> > for just one configuration, this is not really needed; so you plan/forsee
> > more chips being added soonish?
> >
> >> +};
> >> +
> >> +/**
> >> + * Read an ADC channel and return its value.
> >> + *
> >> + * @adc: The ADC SPI data.
> >> + * @channel: The IIO channel data structure.
> >> + */
> >> +static int adc084s021_adc_conversion(struct adc084s021 *adc,
> >> + struct iio_chan_spec const *channel)
> >> +{
> >> + u16 value;
> >
> > value should be u8, but is not really needed
> >
> >> + int ret;
> >> +
> >> + mutex_lock(&adc->lock);
> >> + adc->tx_buf[0] = channel->address;
> >> +
> >> + /* Do the transfer */
> >> + ret = spi_sync(adc->spi, &adc->message);
> >> +
> >
> > no newline here please
> >
> >> + if (ret < 0) {
> >> + mutex_unlock(&adc->lock);
> >> + return ret;
> >> + }
> >> +
> >> + value = (adc->rx_buf[0] << 4) | (adc->rx_buf[1] >> 4);
> >
> > I recommend using __be16 for rx_buf and
> > ret = (be16_to_cpu(adc->rx_buf) >> 4) & 0xff;
> >
> >> + mutex_unlock(&adc->lock);
> >> +
> >> + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
> >> + value, channel->channel);
> >> + return value;
> >> +}
> >> +
> >> +/**
> >> + * Make a readout of requested IIO channel info.
> >
> > no need to document this, it is found in every IIO driver...
> >
> >> + *
> >> + * @indio_dev: The industrial I/O device.
> >> + * @channel: The IIO channel data structure.
> >> + * @val: First element of value (integer).
> >> + * @val2: Second element of value (fractional).
> >> + * @mask: The info_mask to read.
> >> + */
> >> +static int adc084s021_read_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *channel, int *val,
> >> + int *val2, long mask)
> >> +{
> >> + struct adc084s021 *adc = iio_priv(indio_dev);
> >> + int retval;
> >
> > how about using ret everywhere?
> Agreed.
Fixed in v2.
> >
> >> +
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_RAW:
> >
> > probably want iio_device_claim_direct_mode() so to not interfere with
> > buffered accessBorderline case as all you will do is queue up additional spi transfers.
> At least after you have dropped the unusual events stuff.
>
> Arguably this will mean sysfs reads could make the buffered data flow
> less deterministic though so maybe on claiming direct mode (which
> prevents it running concurrently with buffered capture.
Fixed in v2. And no events anymore.
> >
> >> + retval = adc084s021_adc_conversion(adc, channel);
> >> + if (retval < 0)
> >> + return retval;
> >> +
> >> + *val = retval;
> >> + return IIO_VAL_INT;
> >> +
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * Read enabled ADC channels and push data to the buffer.
> >> + *
> >> + * @irq: The interrupt number (not used).
> >> + * @pollfunc: Pointer to the poll func.
> >> + */
> >> +static irqreturn_t adc084s021_trigger_handler(int irq, void *pollfunc)
> >> +{
> >> + struct iio_poll_func *pf = pollfunc;
> >> + struct iio_dev *indio_dev = pf->indio_dev;
> >> + struct adc084s021 *adc = iio_priv(indio_dev);
> >> + u8 *data;
> >> + s64 timestamp;
> >> + int value, scan_index;
> >> +
> >> + data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >
> > pre-allocate buffer with maximum size statically; allocation is
> > potentially expensive; don't forget space for the timestamp and padding...
> >
> >> + if (!data) {
> >> + iio_trigger_notify_done(indio_dev->trig);
> >> + return IRQ_NONE;
> >> + }
> >> +
> >> + timestamp = iio_get_time_ns(indio_dev);
> >> +
> >> + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> >> + indio_dev->masklength) {
> >> + const struct iio_chan_spec *channel =
> >> + &indio_dev->channels[scan_index];
> >> + value = adc084s021_adc_conversion(adc, channel);
> >
> > lock is taken and released for each channel, probably want to do it just
> > once?
> >
> >> + data[scan_index] = value;
> >> +
> >> + /*
> >> + * Compare read data to previous read. If it differs send
> >> + * event notification for affected channel.
> >> + */
> >> + if (adc->cur_adc_values[scan_index] != (u8)value) {
> >
> > cur_adc_values is not initialized (probably set to 0);
> > so on first read, should the notification be sent?
> ? This is 'unusual' to say the least. Why the events given the data
> is available via the buffer. If you really want to do this stuff, it
> ought to be in userspace.
>
> Are you trying to emulate the filtering input does?
I removed the check for previous value and sending events in v2.
> >
> >> + adc->cur_adc_values[scan_index] = (u8)value;
> >> + iio_push_event(indio_dev,
> >> + IIO_EVENT_CODE(IIO_VOLTAGE, 0,
> >> + IIO_NO_MOD, IIO_EV_DIR_NONE,
> >> + IIO_EV_TYPE_CHANGE,
> >> + channel->channel, 0, 0),
> >> + timestamp);
> >> + dev_dbg(&indio_dev->dev,
> >> + "new value on ch%d: 0x%02X (ts %llu)\n",
> >> + channel->channel, value, timestamp);
> >> + }
> >> + }
> >> +
> >> + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> >> + iio_trigger_notify_done(indio_dev->trig);
> >> + kfree(data);
> blank line here please.
Fixed in v2.
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static const struct iio_info adc084s021_info = {
> >> + .read_raw = adc084s021_read_raw,
> >> + .driver_module = THIS_MODULE,
> >> +};
> >> +
> >> +/**
> >> + * Create and register ADC IIO device for SPI.
> >
> > comment is rather pointless
> >
> >> + */
> >> +static int adc084s021_probe(struct spi_device *spi)
> >> +{
> >> + struct iio_dev *indio_dev;
> >> + struct adc084s021 *adc;
> >> + int config = spi_get_device_id(spi)->driver_data;
> >> + int retval;
> >
> > ret maybe
> >
> >> +
> >> + /* Allocate an Industrial I/O device */
> >
> > obviously...
> :) Yeah, clear out any comments that don't tell us much.
Fixed in v2.
> >
> >> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> >> + if (!indio_dev) {
> >> + dev_err(&spi->dev, "Failed to allocate IIO device\n");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + adc = iio_priv(indio_dev);
> >> + adc->spi = spi;
> >> + spi->bits_per_word = ADC_RESOLUTION;
> This surprised me somewhat as I was expecting an odd value for this
> (and was going to ask if standard power of 2 options would work).
> If it had been inline, this would have required slightly less review
> time which I always like (particularly when crammed into an airline seat!)
Yes, I am using inline value in v2.
> >> +
> >> + /* Update the SPI device with config and connect the iio dev */
> >> + retval = spi_setup(spi);
> >> + if (retval) {
> >> + dev_err(&spi->dev, "Failed to update SPI device\n");
> >> + return retval;
> >> + }
> >> + spi_set_drvdata(spi, indio_dev);
> >> +
> >> + /* Initiate the Industrial I/O device */
> :>> + indio_dev->dev.parent = &spi->dev;
> >> + indio_dev->dev.of_node = spi->dev.of_node;
> >> + indio_dev->name = spi_get_device_id(spi)->name;
> >> + indio_dev->modes = INDIO_DIRECT_MODE;
> >> + indio_dev->info = &adc084s021_info;
> >> + indio_dev->channels = adc084s021_config[config].channels;
> >> + indio_dev->num_channels = adc084s021_config[config].num_channels;
> >
> > could do it directly as long there is just one configuration
> That would certainly be preferable unless V2 is going to show up
> with multiple options!
Fixed. No more options supported in v2.
> >
> >> +
> >> + /* Create SPI transfer for channel reads */
> >> + adc->spi_trans[0].tx_buf = &adc->tx_buf[0];
> >> + adc->spi_trans[0].len = 2;
> >> + adc->spi_trans[0].speed_hz = spi->max_speed_hz;
> >> + adc->spi_trans[0].bits_per_word = spi->bits_per_word;
> >> + adc->spi_trans[1].rx_buf = &adc->rx_buf[0];
> >> + adc->spi_trans[1].len = 2;
> >> + adc->spi_trans[1].speed_hz = spi->max_speed_hz;
> >> + adc->spi_trans[1].bits_per_word = spi->bits_per_word;
> >> +
> >> + /* Setup SPI message for channel reads */
> >> + spi_message_init(&adc->message);
> >> + spi_message_add_tail(&adc->spi_trans[0], &adc->message);
> >> + spi_message_add_tail(&adc->spi_trans[1], &adc->message);
> spi_init_with_transfers to save a bit of boilerplate.
Fixed in v2.
> >> +
> >> + adc->reg = devm_regulator_get(&spi->dev, "vref");
> >> + if (IS_ERR(adc->reg))
> >> + return PTR_ERR(adc->reg);
> >> +
> >> + retval = regulator_enable(adc->reg);
> >> + if (retval < 0)
> >> + return retval;
> Given we either have slow sysfs accesses or know we have buffered
> access on going. Have you considered enabling and disabling
> the regulator dynamically? The fact that this often makes sense is
> why Mark and co from the regulators side of things haven't provided
> a devm_regulator_enable... You would need a preenable and postdisable
> to make sure it was on for buffered access.
Yes, I am using preenable and postdisable functions for regulator in v2.
> >> +
> >> + mutex_init(&adc->lock);
> >> +
> >> + /* Setup triggered buffer with pollfunction */
> >> + retval = iio_triggered_buffer_setup(indio_dev, NULL,
> >
> > devm_()
> I disagree. It would change the ordering wrt to the regulator_enable.
> Now, obviously it won't actually matter, but it will make the code
> less obviously correct and for the small burden of extra unwind
> code I'd keep using the non devm version.
I did not change this in v2. So I kept the non devm_ functions.
> >
> >> + adc084s021_trigger_handler, NULL);
> >> + if (retval) {
> >> + dev_err(&spi->dev, "Failed to setup triggered buffer\n");
> >> + goto buffer_setup_failed;
> >> + }
> >> +
> >> + retval = iio_device_register(indio_dev);
> >> + if (retval) {
> >> + dev_err(&spi->dev, "Failed to register IIO device\n");
> Hmm. If we were to flesh out some error messages for the few
> cases where there is no info provided in iio_device_register we could
> probably clean out a fair bit of boiler plate reporting in drivers.
> Ah well, one for the future!
If you insist I will remove it :)
> >> + goto device_register_failed;
> >> + }
> >> +
> >> + dev_info(&spi->dev, "probed!\n");
> >
> > no logging please, just outputs clutter
> >
> >> + return 0;
> >> +
> >> +device_register_failed:
> >> + iio_triggered_buffer_cleanup(indio_dev);
> >> +buffer_setup_failed:
> >> + regulator_disable(adc->reg);
> >> + return retval;
> >> +}
> >> +
> >> +/**
> >> + * Unregister ADC IIO device for SPI.
> >> + */
> >> +static int adc084s021_remove(struct spi_device *spi)
> >> +{
> >> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> >> + struct adc084s021 *adc = iio_priv(indio_dev);
> >> +
> >> + iio_device_unregister(indio_dev);
> >> + iio_triggered_buffer_cleanup(indio_dev);
> >> + regulator_disable(adc->reg);
> blank line here preferred (slightly!)
Fixed in v2.
> >> + return 0;
> >> +}
> >> +
> >> +static const struct of_device_id adc084s021_of_match[] = {
> >> + { .compatible = "ti,adc084s021", },
> >> + {},
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(of, adc084s021_of_match);
> >> +
> >> +static const struct spi_device_id adc084s021_id[] = {
> >> + { MODULE_NAME, 0},
> >> + {}
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(spi, adc084s021_id);
> >> +
> >> +static struct spi_driver adc084s021_driver = {
> >> + .driver = {
> >> + .name = MODULE_NAME,
> >> + .of_match_table = of_match_ptr(adc084s021_of_match),
> >> + },
> >> + .probe = adc084s021_probe,
> >> + .remove = adc084s021_remove,
> >> + .id_table = adc084s021_id,
> >> +};
> >> +
> Convention often says to not bother with a blank line here or before
> the MODULE_DEVICE_TABLE above. Gives a visual indication that these macros
> are being passed the structures.
> (really minor point!)
Fixed in v2.
> >> +module_spi_driver(adc084s021_driver);
> >> +
> >> +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
> >> +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_VERSION(DRIVER_VERSION);
> >>
> >
>
^ permalink raw reply
* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:42 UTC (permalink / raw)
To: Rob Herring
Cc: Mårten Lindahl, jic23-DgEjT+Ai2ygdnm+yROfE0A,
knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170428135248.du6smmzktqldj4u4@rob-hp-laptop>
On Fri, 2017-04-28 at 08:52 -0500, Rob Herring wrote:
> On Fri, Apr 21, 2017 at 04:58:23PM +0200, Mårten Lindahl wrote:
> > From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >
> > This adds support for the Texas Instruments ADC084S021 ADC chip.
> >
> > Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> > ---
> > .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
>
> It's preferred to put bindings in a separate patch.
I am sorry for that. I split this into its own patch in v2.
>
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
> > 4 files changed, 380 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > new file mode 100644
> > index 0000000..921eb46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > @@ -0,0 +1,25 @@
> > +* Texas Instruments' ADC084S021
> > +
> > +Required properties:
> > + - compatible : Must be "ti,adc084s021"
> > + - reg : SPI chip select number for the device
> > + - vref-supply : The regulator supply for ADC reference voltage
> > + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> > +
> > +Optional properties:
> > + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
> > + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
> > + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
>
> How are these optional? A given device should have specific properties
> required here.
No, they are not optional. I removed this section in v2 and updated the
required properties section.
>
> Also, no need to define them, "per spi-bus bindings" is enough.
Fixed in v2.
>
> Rob
Thanks,
Mårten
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4 1/9] PM / OPP: Allow OPP table to be used for power-domains
From: Mark Brown @ 2017-04-30 12:49 UTC (permalink / raw)
To: Sudeep Holla
Cc: Rajendra Nayak, Viresh Kumar, Rafael Wysocki, ulf.hansson,
Kevin Hilman, Viresh Kumar, Nishanth Menon, Stephen Boyd,
linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh+dt,
lina.iyer, devicetree
In-Reply-To: <c66322f5-e2eb-5556-a5a5-14998a2f195d@arm.com>
[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]
On Thu, Apr 27, 2017 at 10:42:49AM +0100, Sudeep Holla wrote:
> On 26/04/17 14:55, Mark Brown wrote:
> > As I'm getting fed up of saying: if the values you are setting are not
> > voltages and do not behave like voltages then the hardware should not be
> > represented as a voltage regulator since if they are represented as
> > voltage regulators things will expect to be able to control them as
> > voltage regulators. This hardware is quite clearly providing OPPs
> > directly, I would expect this to be handled in the OPP code somehow.
> I agree with you that we need to be absolutely sure on what it actually
> represents.
> But as more and more platform are pushing such power controls to
> dedicated M3 or similar processors, we need abstraction. Though we are
> controlling hardware, we do so indirectly. Since there were discussions
> around device tree representing hardware vs platform, I tend to think,
> we are moving towards platform(something similar to ACPI).
I don't think there's a meaningful hardware/platform distinction here -
in terms of what DT is describing the platform bit is just what the
hardware (the microcontrollers) happen to do, DT doesn't much care about
that though.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v2 0/2] add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:53 UTC (permalink / raw)
To: jic23-DgEjT+Ai2ygdnm+yROfE0A
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA, Mårten Lindahl
From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
This adds support for the Texas Instruments ADC084S021 ADC chip.
---
Changes in v2:
- Split dt-bindings and iio/adc into separate patches
- Updated dt-bindings after Robs comments
- Removed most #defines to inlines
- Corrected channel macro
- Removed configuration array with only one item
- Updated func adc084s021_adc_conversion to use be16_to_cpu
- Added IIO_CHAN_INFO_SCALE to func adc084s021_read_raw
- Use iio_device_claim_direct_mode in func adc084s021_read_raw
- Removed documentation for standard driver functions
- Changed retval to ret everywhere
- Removed dynamic alloc for data buffer in trigger handler
- Keeping mutex for all iterations in trigger handler
- Removed usage of events in this driver
- Removed info log in probe
- Use spi_message_init_with_transfers for spi message structs
- Use preenable and postdisable functions for regulator
- Inserted blank line before last return in all functions
Mårten Lindahl (2):
dt-bindings: iio: adc: add driver for the ti-adc084s021 chip
iio: adc: add driver for the ti-adc084s021 chip
.../devicetree/bindings/iio/adc/ti-adc084s021.txt | 19 ++
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-adc084s021.c | 294 +++++++++++++++++++++
4 files changed, 326 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
create mode 100644 drivers/iio/adc/ti-adc084s021.c
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCHv2 1/2] dt-bindings: iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:53 UTC (permalink / raw)
To: jic23-DgEjT+Ai2ygdnm+yROfE0A
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA, Mårten Lindahl
In-Reply-To: <1493556822-2601-1-git-send-email-marten.lindahl-VrBV9hrLPhE@public.gmane.org>
From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
This adds support for the Texas Instruments ADC084S021 ADC chip.
Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
---
Changes in v2:
- Updated the 'Required properties' section
- Removed the 'Optional properties' section
.../devicetree/bindings/iio/adc/ti-adc084s021.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
new file mode 100644
index 0000000..4259e50
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
@@ -0,0 +1,19 @@
+* Texas Instruments' ADC084S021
+
+Required properties:
+ - compatible : Must be "ti,adc084s021"
+ - reg : SPI chip select number for the device
+ - vref-supply : The regulator supply for ADC reference voltage
+ - spi-cpol : Per spi-bus bindings
+ - spi-cpha : Per spi-bus bindings
+ - spi-max-frequency : Per spi-bus bindings
+
+Example:
+adc@0 {
+ compatible = "ti,adc084s021";
+ reg = <0>;
+ vref-supply = <&adc_vref>;
+ spi-cpol;
+ spi-cpha;
+ spi-max-frequency = <16000000>;
+};
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCHv2 2/2] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:53 UTC (permalink / raw)
To: jic23-DgEjT+Ai2ygdnm+yROfE0A
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA, Mårten Lindahl
In-Reply-To: <1493556822-2601-1-git-send-email-marten.lindahl-VrBV9hrLPhE@public.gmane.org>
From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
This adds support for the Texas Instruments ADC084S021 ADC chip.
Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
---
Changes in v2:
- Removed most #defines in favor of inlines
- Corrected channel macro
- Removed configuration array with only one item
- Updated func adc084s021_adc_conversion to use be16_to_cpu
- Added IIO_CHAN_INFO_SCALE to func adc084s021_read_raw
- Use iio_device_claim_direct_mode in func adc084s021_read_raw
- Removed documentation for standard driver functions
- Changed retval to ret everywhere
- Removed dynamic alloc for data buffer in trigger handler
- Keeping mutex for all iterations in trigger handler
- Removed usage of events in this driver
- Removed info log in probe
- Use spi_message_init_with_transfers for spi message structs
- Use preenable and postdisable functions for regulator
- Inserted blank line before last return in all functions
drivers/iio/adc/Kconfig | 12 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-adc084s021.c | 294 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 307 insertions(+)
create mode 100644 drivers/iio/adc/ti-adc084s021.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dedae7a..13141e5 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -560,6 +560,18 @@ config TI_ADC0832
This driver can also be built as a module. If so, the module will be
called ti-adc0832.
+config TI_ADC084S021
+ tristate "Texas Instruments ADC084S021"
+ depends on SPI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ If you say yes here you get support for Texas Instruments ADC084S021
+ chips.
+
+ This driver can also be built as a module. If so, the module will be
+ called ti-adc084s021.
+
config TI_ADC12138
tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d001262..b1a6158 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
obj-$(CONFIG_STM32_ADC) += stm32-adc.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
+obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
new file mode 100644
index 0000000..2dce257
--- /dev/null
+++ b/drivers/iio/adc/ti-adc084s021.c
@@ -0,0 +1,294 @@
+/**
+ * Copyright (C) 2017 Axis Communications AB
+ *
+ * Driver for Texas Instruments' ADC084S021 ADC chip.
+ * Datasheets can be found here:
+ * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/regulator/consumer.h>
+
+#define ADC084S021_DRIVER_NAME "adc084s021"
+
+struct adc084s021 {
+ struct spi_device *spi;
+ struct spi_message message;
+ struct spi_transfer spi_trans[2];
+ struct regulator *reg;
+ struct mutex lock;
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ */
+ union {
+ u16 tx_buf;
+ __be16 rx_buf;
+ } ____cacheline_aligned;
+};
+
+/**
+ * Channel specification
+ */
+#define ADC084S021_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .channel = (num), \
+ .address = (num) << 3, \
+ .indexed = 1, \
+ .scan_index = (num), \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 8, \
+ .storagebits = 16, \
+ .shift = 4, \
+ .endianness = IIO_BE, \
+ }, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+ }
+
+static const struct iio_chan_spec adc084s021_channels[] = {
+ ADC084S021_VOLTAGE_CHANNEL(0),
+ ADC084S021_VOLTAGE_CHANNEL(1),
+ ADC084S021_VOLTAGE_CHANNEL(2),
+ ADC084S021_VOLTAGE_CHANNEL(3),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+/**
+ * Read an ADC channel and return its value.
+ *
+ * @adc: The ADC SPI data.
+ * @channel: The IIO channel data structure.
+ */
+static int adc084s021_adc_conversion(struct adc084s021 *adc,
+ struct iio_chan_spec const *channel)
+{
+ u8 value;
+ int ret;
+
+ adc->tx_buf = channel->address;
+
+ /* Do the transfer */
+ ret = spi_sync(adc->spi, &adc->message);
+ if (ret < 0)
+ return ret;
+
+ value = (be16_to_cpu(adc->rx_buf) >> channel->scan_type.shift) & 0xff;
+
+ dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
+ value, channel->channel);
+
+ return value;
+}
+
+static int adc084s021_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct adc084s021 *adc = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ ret = adc084s021_adc_conversion(adc, channel);
+ iio_device_release_direct_mode(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ ret = regulator_get_voltage(adc->reg);
+ if (ret < 0)
+ return ret;
+
+ *val = ret / 1000;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * Read enabled ADC channels and push data to the buffer.
+ *
+ * @irq: The interrupt number (not used).
+ * @pollfunc: Pointer to the poll func.
+ */
+static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void *pollfunc)
+{
+ struct iio_poll_func *pf = pollfunc;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct adc084s021 *adc = iio_priv(indio_dev);
+ __be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes timestamp */
+ int scan_index;
+ int i = 0;
+
+ mutex_lock(&adc->lock);
+
+ for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ const struct iio_chan_spec *channel =
+ &indio_dev->channels[scan_index];
+ data[i++] = adc084s021_adc_conversion(adc, channel);
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, data,
+ iio_get_time_ns(indio_dev));
+ mutex_unlock(&adc->lock);
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int adc084s021_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct adc084s021 *adc = iio_priv(indio_dev);
+
+ return regulator_enable(adc->reg);
+}
+
+static int adc084s021_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct adc084s021 *adc = iio_priv(indio_dev);
+
+ return regulator_disable(adc->reg);
+}
+
+static const struct iio_info adc084s021_info = {
+ .read_raw = adc084s021_read_raw,
+ .driver_module = THIS_MODULE,
+};
+
+static const struct iio_buffer_setup_ops adc084s021_buffer_setup_ops = {
+ .preenable = adc084s021_buffer_preenable,
+ .postenable = iio_triggered_buffer_postenable,
+ .predisable = iio_triggered_buffer_predisable,
+ .postdisable = adc084s021_buffer_postdisable,
+};
+
+static int adc084s021_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct adc084s021 *adc;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+ if (!indio_dev) {
+ dev_err(&spi->dev, "Failed to allocate IIO device\n");
+ return -ENOMEM;
+ }
+
+ adc = iio_priv(indio_dev);
+ adc->spi = spi;
+ spi->bits_per_word = 8;
+
+ /* Update the SPI device with config and connect the iio dev */
+ ret = spi_setup(spi);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to update SPI device\n");
+ return ret;
+ }
+ spi_set_drvdata(spi, indio_dev);
+
+ /* Initiate the Industrial I/O device */
+ indio_dev->dev.parent = &spi->dev;
+ indio_dev->dev.of_node = spi->dev.of_node;
+ indio_dev->name = spi_get_device_id(spi)->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &adc084s021_info;
+ indio_dev->channels = adc084s021_channels;
+ indio_dev->num_channels = ARRAY_SIZE(adc084s021_channels);
+
+ /* Create SPI transfer for channel reads */
+ adc->spi_trans[0].tx_buf = &adc->tx_buf;
+ adc->spi_trans[0].len = 2;
+ adc->spi_trans[0].speed_hz = spi->max_speed_hz;
+ adc->spi_trans[0].bits_per_word = spi->bits_per_word;
+ adc->spi_trans[1].rx_buf = &adc->rx_buf;
+ adc->spi_trans[1].len = 2;
+ adc->spi_trans[1].speed_hz = spi->max_speed_hz;
+ adc->spi_trans[1].bits_per_word = spi->bits_per_word;
+
+ spi_message_init_with_transfers(&adc->message, adc->spi_trans, 2);
+
+ adc->reg = devm_regulator_get(&spi->dev, "vref");
+ if (IS_ERR(adc->reg))
+ return PTR_ERR(adc->reg);
+
+ mutex_init(&adc->lock);
+
+ /* Setup triggered buffer with pollfunction */
+ ret = iio_triggered_buffer_setup(indio_dev, NULL,
+ adc084s021_buffer_trigger_handler,
+ &adc084s021_buffer_setup_ops);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to setup triggered buffer\n");
+ return ret;
+ }
+
+ ret = iio_device_register(indio_dev);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to register IIO device\n");
+ iio_triggered_buffer_cleanup(indio_dev);
+ }
+
+ return ret;
+}
+
+static int adc084s021_remove(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+ iio_device_unregister(indio_dev);
+ iio_triggered_buffer_cleanup(indio_dev);
+
+ return 0;
+}
+
+static const struct of_device_id adc084s021_of_match[] = {
+ { .compatible = "ti,adc084s021", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, adc084s021_of_match);
+
+static const struct spi_device_id adc084s021_id[] = {
+ { ADC084S021_DRIVER_NAME, 0},
+ {}
+};
+MODULE_DEVICE_TABLE(spi, adc084s021_id);
+
+static struct spi_driver adc084s021_driver = {
+ .driver = {
+ .name = ADC084S021_DRIVER_NAME,
+ .of_match_table = of_match_ptr(adc084s021_of_match),
+ },
+ .probe = adc084s021_probe,
+ .remove = adc084s021_remove,
+ .id_table = adc084s021_id,
+};
+module_spi_driver(adc084s021_driver);
+
+MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
+MODULE_DESCRIPTION("Texas Instruments ADC084S021");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("1.0");
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Jonathan Cameron @ 2017-04-30 15:13 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Peter Meerwald-Stadler, Mårten Lindahl,
lars-Qo5EllUWu/uELgA04lAiVw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493288121.21359.33.camel-VrBV9hrLPhE@public.gmane.org>
On 27/04/17 11:15, Mårten Lindahl wrote:
> On Sun, 2017-04-23 at 20:13 +0100, Jonathan Cameron wrote:
>> On 21/04/17 21:19, Peter Meerwald-Stadler wrote:
>>>
>>>> From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
>>>
>>> comments below
>> A few more from me. Laptop battery gone flat so not so thorough on top few lines!
>>
>> Jonathan
>
> Hi, and thanks for your comments! I have a new patch where I resolved it
> all. But before I send it I wonder about your comments about the events.
> Please see below.
>
> Thanks,
> Mårten
>>>
>>>> This adds support for the Texas Instruments ADC084S021 ADC chip.
>>>>
>>>> Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
>>>> ---
>>>> .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
>>>> drivers/iio/adc/Kconfig | 12 +
>>>> drivers/iio/adc/Makefile | 1 +
>>>> drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
>>>> 4 files changed, 380 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
>>>> create mode 100644 drivers/iio/adc/ti-adc084s021.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
>>>> new file mode 100644
>>>> index 0000000..921eb46
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
>>>> @@ -0,0 +1,25 @@
>>>> +* Texas Instruments' ADC084S021
>>>> +
>>>> +Required properties:
>>>> + - compatible : Must be "ti,adc084s021"
>>>> + - reg : SPI chip select number for the device
>>>> + - vref-supply : The regulator supply for ADC reference voltage
>>>> + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
>>>> +
>>>> +Optional properties:
>>>> + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
>>>> + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
>>>> + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
>>>> +
>>>> +
>>>> +Example:
>>>> +adc@0 {
>>>> + compatible = "ti,adc084s021";
>>>> + reg = <0>;
>>>> + vref-supply = <&adc_vref>;
>>>> + spi-cpol;
>>>> + spi-cpha;
>>>> + spi-cs-high;
>>>> + spi-max-frequency = <16000000>;
>>>> + pl022,com-mode = <0x2>; /* DMA */
>>>
>>> what is this?
>>>
>>>> +};
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index dedae7a..13141e5 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -560,6 +560,18 @@ config TI_ADC0832
>>>> This driver can also be built as a module. If so, the module will be
>>>> called ti-adc0832.
>>>>
>>>> +config TI_ADC084S021
>>>> + tristate "Texas Instruments ADC084S021"
>>>> + depends on SPI
>>>> + select IIO_BUFFER
>>>> + select IIO_TRIGGERED_BUFFER
>>>> + help
>>>> + If you say yes here you get support for Texas Instruments ADC084S021
>>>> + chips.
>>>> +
>>>> + This driver can also be built as a module. If so, the module will be
>>>> + called ti-adc084s021.
>>>> +
>>>> config TI_ADC12138
>>>> tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
>>>> depends on SPI
>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>> index d001262..b1a6158 100644
>>>> --- a/drivers/iio/adc/Makefile
>>>> +++ b/drivers/iio/adc/Makefile
>>>> @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
>>>> obj-$(CONFIG_STM32_ADC) += stm32-adc.o
>>>> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>>> obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>>> +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
>>>> obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>>>> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>>>> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>>>> diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
>>>> new file mode 100644
>>>> index 0000000..4f33b91
>>>> --- /dev/null
>>>> +++ b/drivers/iio/adc/ti-adc084s021.c
>>>> @@ -0,0 +1,342 @@
>>>> +/**
>>>> + * Copyright (C) 2017 Axis Communications AB
>>>> + *
>>>> + * Driver for Texas Instruments' ADC084S021 ADC chip.
>>>> + * Datasheets can be found here:
>>>> + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
>>>> + *
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/spi/spi.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/events.h>
>>>> +#include <linux/iio/triggered_buffer.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#define MODULE_NAME "adc084s021"
>>>> +#define DRIVER_VERSION "1.0"
>>>
>>> is only used once at the very end...
>>>
>>>> +#define ADC_RESOLUTION 8
>> Put it inline...
>>>> +#define ADC_N_CHANNELS 4
>> Belongs inline. No additional info provided by having it here.
>>>
>>> we want a consistent prefix, such as ADC084S021_
>>>
>>>> +
>>>> +struct adc084s021_configuration {
>>>> + const struct iio_chan_spec *channels;
>>>> + u8 num_channels;
>>>
>>> no need for u8, perhaps unsigned?
>>>
>>>> +};
>>>> +
>>>> +struct adc084s021 {
>>>> + struct spi_device *spi;
>>>> + struct spi_message message;
>>>> + struct spi_transfer spi_trans[2];
>>>> + struct regulator *reg;
>>>> + struct mutex lock;
>>>> + /*
>>>> + * DMA (thus cache coherency maintenance) requires the
>>>> + * transfer buffers to live in their own cache lines.
>>>> + */
>>>> + union {
>>>> + u8 tx_buf[2];
>>>> + u8 rx_buf[2];
>>>> + } ____cacheline_aligned;
>>>> + u8 cur_adc_values[ADC_N_CHANNELS];
>>>> +};
>>>> +
>>>> +/**
>>>> + * Event triggered when value changes on a channel
>>>> + */
>>>> +static const struct iio_event_spec adc084s021_event = {
>>>> + .type = IIO_EV_TYPE_CHANGE,
>>>> + .dir = IIO_EV_DIR_NONE,
>>>> +};
>> Not the intent of that type of event at all.
>>>> +
>>>> +/**
>>>> + * Channel specification
>>>> + */
>>>> +#define ADC084S021_VOLTAGE_CHANNEL(num) \
>>>> + { \
>>>> + .type = IIO_VOLTAGE, \
>>>> + .channel = (num), \
>>>> + .address = (num << 3), \
>>>
>>> parenthesis should be around (num)
>>>
>>>> + .indexed = 1, \
>>>> + .scan_index = num, \
>>>
>>> parenthesis?
>>>
>>>> + .scan_type = { \
>>>> + .sign = 'u', \
>>>> + .realbits = 8, \
>>>> + .storagebits = 32, \
>>>> + .shift = 24 - ((num << 3)), \
>>>
>>> no need for (( )) around the expression, parenthesis should be around num
>>>
>>> the shift doesn't make sense, you are shifting in
>>>
>>> adc084s021_adc_conversion() already and return just 8 bits (so storagebits
>>> should be 8)?
>>>
>>> you could/should use realbits = 8, storagebits = 16, shift = 4 and
>>> endianness = IIO_BE if I read Figure 1 of the datasheet correctly
>>>
>>>> + }, \
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>>
>>> likely this is missing _SCALE
>>> IIO expects data to be returned in millivolt, see
>>> Documentation/ABI/testing/sysfs-bus-iio
>>>
>>>> + .event_spec = &adc084s021_event, \
>>>> + .num_event_specs = 1, \
>>>
>>> ARRAY_SIZE(&adc084s021_event) to be extensible?
>>>
>>>> + }
>>>> +
>>>> +static const struct iio_chan_spec adc084s021_channels[] = {
>>>> + ADC084S021_VOLTAGE_CHANNEL(0),
>>>> + ADC084S021_VOLTAGE_CHANNEL(1),
>>>> + ADC084S021_VOLTAGE_CHANNEL(2),
>>>> + ADC084S021_VOLTAGE_CHANNEL(3),
>>>> + IIO_CHAN_SOFT_TIMESTAMP(4),
>>>> +};
>>>> +
>>>> +static const struct adc084s021_configuration adc084s021_config[] = {
>>>> + { adc084s021_channels, ARRAY_SIZE(adc084s021_channels) },
>>>
>>> for just one configuration, this is not really needed; so you plan/forsee
>>> more chips being added soonish?
>>>
>>>> +};
>>>> +
>>>> +/**
>>>> + * Read an ADC channel and return its value.
>>>> + *
>>>> + * @adc: The ADC SPI data.
>>>> + * @channel: The IIO channel data structure.
>>>> + */
>>>> +static int adc084s021_adc_conversion(struct adc084s021 *adc,
>>>> + struct iio_chan_spec const *channel)
>>>> +{
>>>> + u16 value;
>>>
>>> value should be u8, but is not really needed
>>>
>>>> + int ret;
>>>> +
>>>> + mutex_lock(&adc->lock);
>>>> + adc->tx_buf[0] = channel->address;
>>>> +
>>>> + /* Do the transfer */
>>>> + ret = spi_sync(adc->spi, &adc->message);
>>>> +
>>>
>>> no newline here please
>>>
>>>> + if (ret < 0) {
>>>> + mutex_unlock(&adc->lock);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + value = (adc->rx_buf[0] << 4) | (adc->rx_buf[1] >> 4);
>>>
>>> I recommend using __be16 for rx_buf and
>>> ret = (be16_to_cpu(adc->rx_buf) >> 4) & 0xff;
>>>
>>>> + mutex_unlock(&adc->lock);
>>>> +
>>>> + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
>>>> + value, channel->channel);
>>>> + return value;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Make a readout of requested IIO channel info.
>>>
>>> no need to document this, it is found in every IIO driver...
>>>
>>>> + *
>>>> + * @indio_dev: The industrial I/O device.
>>>> + * @channel: The IIO channel data structure.
>>>> + * @val: First element of value (integer).
>>>> + * @val2: Second element of value (fractional).
>>>> + * @mask: The info_mask to read.
>>>> + */
>>>> +static int adc084s021_read_raw(struct iio_dev *indio_dev,
>>>> + struct iio_chan_spec const *channel, int *val,
>>>> + int *val2, long mask)
>>>> +{
>>>> + struct adc084s021 *adc = iio_priv(indio_dev);
>>>> + int retval;
>>>
>>> how about using ret everywhere?
>> Agreed.
>>>
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>
>>> probably want iio_device_claim_direct_mode() so to not interfere with
>>> buffered accessBorderline case as all you will do is queue up additional spi transfers.
>> At least after you have dropped the unusual events stuff.
>>
>> Arguably this will mean sysfs reads could make the buffered data flow
>> less deterministic though so maybe on claiming direct mode (which
>> prevents it running concurrently with buffered capture.
>>>
>>>> + retval = adc084s021_adc_conversion(adc, channel);
>>>> + if (retval < 0)
>>>> + return retval;
>>>> +
>>>> + *val = retval;
>>>> + return IIO_VAL_INT;
>>>> +
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +}
>>>> +
>>>> +/**
>>>> + * Read enabled ADC channels and push data to the buffer.
>>>> + *
>>>> + * @irq: The interrupt number (not used).
>>>> + * @pollfunc: Pointer to the poll func.
>>>> + */
>>>> +static irqreturn_t adc084s021_trigger_handler(int irq, void *pollfunc)
>>>> +{
>>>> + struct iio_poll_func *pf = pollfunc;
>>>> + struct iio_dev *indio_dev = pf->indio_dev;
>>>> + struct adc084s021 *adc = iio_priv(indio_dev);
>>>> + u8 *data;
>>>> + s64 timestamp;
>>>> + int value, scan_index;
>>>> +
>>>> + data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>>>
>>> pre-allocate buffer with maximum size statically; allocation is
>>> potentially expensive; don't forget space for the timestamp and padding...
>>>
>>>> + if (!data) {
>>>> + iio_trigger_notify_done(indio_dev->trig);
>>>> + return IRQ_NONE;
>>>> + }
>>>> +
>>>> + timestamp = iio_get_time_ns(indio_dev);
>>>> +
>>>> + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
>>>> + indio_dev->masklength) {
>>>> + const struct iio_chan_spec *channel =
>>>> + &indio_dev->channels[scan_index];
>>>> + value = adc084s021_adc_conversion(adc, channel);
>>>
>>> lock is taken and released for each channel, probably want to do it just
>>> once?
>>>
>>>> + data[scan_index] = value;
>>>> +
>>>> + /*
>>>> + * Compare read data to previous read. If it differs send
>>>> + * event notification for affected channel.
>>>> + */
>>>> + if (adc->cur_adc_values[scan_index] != (u8)value) {
>>>
>>> cur_adc_values is not initialized (probably set to 0);
>>> so on first read, should the notification be sent?
>> ? This is 'unusual' to say the least. Why the events given the data
>> is available via the buffer. If you really want to do this stuff, it
>> ought to be in userspace.
>>
>> Are you trying to emulate the filtering input does?
>
> The buffer is continuously updated with readings that may have the same
> data for many iterations, so my idea was to send an event with timestamp
> so that whenever some "new" (changed) data is written there it can be
> more easily located in the buffer by the consumer, by matching event
> timestamp with buffer timestamp. Is there another way to do this or
> should the buffer readings be continuously analysed by another module or
> application?
>
> When prototyping this driver I even tweaked the IIO_EVENT_CODE to
> include the new value in the event signal itself. But I wasn't brave
> enough to show it here :) Please tell me, am I totally wrong by even
> trying that idea?
It's an interesting thought, but I wouldn't do it the way you have.
If this makes sense it ought to be a core facility rather than implemented
in every driver. The obvious method would be something like:
1) A new consumer device that does the filtering.
2) Configfs based interface to attach it to an existing driver.
Now this falls into the area where a hard coded set of filters to
create the events may not be the way to go (not flexible enough).
Possibly we'd be looking at ebpf programs loaded into the kernel to
do this (it's quite similar in a way to network packet filtering).
I'd also be tempted to not do it as events, but rather via a buffer
that just contains data when the values have changed.
Anyhow, the one place it doesn't belong is in an initially submission
of a new driver as it complicates matters and ties together generic
infrastructure with a particular driver submission.
So pull it out for now.
Jonathan
>
>>>
>>>> + adc->cur_adc_values[scan_index] = (u8)value;
>>>> + iio_push_event(indio_dev,
>>>> + IIO_EVENT_CODE(IIO_VOLTAGE, 0,
>>>> + IIO_NO_MOD, IIO_EV_DIR_NONE,
>>>> + IIO_EV_TYPE_CHANGE,
>>>> + channel->channel, 0, 0),
>>>> + timestamp);
>>>> + dev_dbg(&indio_dev->dev,
>>>> + "new value on ch%d: 0x%02X (ts %llu)\n",
>>>> + channel->channel, value, timestamp);
>>>> + }
>>>> + }
>>>> +
>>>> + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
>>>> + iio_trigger_notify_done(indio_dev->trig);
>>>> + kfree(data);
>> blank line here please.
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static const struct iio_info adc084s021_info = {
>>>> + .read_raw = adc084s021_read_raw,
>>>> + .driver_module = THIS_MODULE,
>>>> +};
>>>> +
>>>> +/**
>>>> + * Create and register ADC IIO device for SPI.
>>>
>>> comment is rather pointless
>>>
>>>> + */
>>>> +static int adc084s021_probe(struct spi_device *spi)
>>>> +{
>>>> + struct iio_dev *indio_dev;
>>>> + struct adc084s021 *adc;
>>>> + int config = spi_get_device_id(spi)->driver_data;
>>>> + int retval;
>>>
>>> ret maybe
>>>
>>>> +
>>>> + /* Allocate an Industrial I/O device */
>>>
>>> obviously...
>> :) Yeah, clear out any comments that don't tell us much.
>>>
>>>> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
>>>> + if (!indio_dev) {
>>>> + dev_err(&spi->dev, "Failed to allocate IIO device\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + adc = iio_priv(indio_dev);
>>>> + adc->spi = spi;
>>>> + spi->bits_per_word = ADC_RESOLUTION;
>> This surprised me somewhat as I was expecting an odd value for this
>> (and was going to ask if standard power of 2 options would work).
>> If it had been inline, this would have required slightly less review
>> time which I always like (particularly when crammed into an airline seat!)
>>>> +
>>>> + /* Update the SPI device with config and connect the iio dev */
>>>> + retval = spi_setup(spi);
>>>> + if (retval) {
>>>> + dev_err(&spi->dev, "Failed to update SPI device\n");
>>>> + return retval;
>>>> + }
>>>> + spi_set_drvdata(spi, indio_dev);
>>>> +
>>>> + /* Initiate the Industrial I/O device */
>> :>> + indio_dev->dev.parent = &spi->dev;
>>>> + indio_dev->dev.of_node = spi->dev.of_node;
>>>> + indio_dev->name = spi_get_device_id(spi)->name;
>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>> + indio_dev->info = &adc084s021_info;
>>>> + indio_dev->channels = adc084s021_config[config].channels;
>>>> + indio_dev->num_channels = adc084s021_config[config].num_channels;
>>>
>>> could do it directly as long there is just one configuration
>> That would certainly be preferable unless V2 is going to show up
>> with multiple options!
>>>
>>>> +
>>>> + /* Create SPI transfer for channel reads */
>>>> + adc->spi_trans[0].tx_buf = &adc->tx_buf[0];
>>>> + adc->spi_trans[0].len = 2;
>>>> + adc->spi_trans[0].speed_hz = spi->max_speed_hz;
>>>> + adc->spi_trans[0].bits_per_word = spi->bits_per_word;
>>>> + adc->spi_trans[1].rx_buf = &adc->rx_buf[0];
>>>> + adc->spi_trans[1].len = 2;
>>>> + adc->spi_trans[1].speed_hz = spi->max_speed_hz;
>>>> + adc->spi_trans[1].bits_per_word = spi->bits_per_word;
>>>> +
>>>> + /* Setup SPI message for channel reads */
>>>> + spi_message_init(&adc->message);
>>>> + spi_message_add_tail(&adc->spi_trans[0], &adc->message);
>>>> + spi_message_add_tail(&adc->spi_trans[1], &adc->message);
>> spi_init_with_transfers to save a bit of boilerplate.
>>>> +
>>>> + adc->reg = devm_regulator_get(&spi->dev, "vref");
>>>> + if (IS_ERR(adc->reg))
>>>> + return PTR_ERR(adc->reg);
>>>> +
>>>> + retval = regulator_enable(adc->reg);
>>>> + if (retval < 0)
>>>> + return retval;
>> Given we either have slow sysfs accesses or know we have buffered
>> access on going. Have you considered enabling and disabling
>> the regulator dynamically? The fact that this often makes sense is
>> why Mark and co from the regulators side of things haven't provided
>> a devm_regulator_enable... You would need a preenable and postdisable
>> to make sure it was on for buffered access.
>>>> +
>>>> + mutex_init(&adc->lock);
>>>> +
>>>> + /* Setup triggered buffer with pollfunction */
>>>> + retval = iio_triggered_buffer_setup(indio_dev, NULL,
>>>
>>> devm_()
>> I disagree. It would change the ordering wrt to the regulator_enable.
>> Now, obviously it won't actually matter, but it will make the code
>> less obviously correct and for the small burden of extra unwind
>> code I'd keep using the non devm version.
>>>
>>>> + adc084s021_trigger_handler, NULL);
>>>> + if (retval) {
>>>> + dev_err(&spi->dev, "Failed to setup triggered buffer\n");
>>>> + goto buffer_setup_failed;
>>>> + }
>>>> +
>>>> + retval = iio_device_register(indio_dev);
>>>> + if (retval) {
>>>> + dev_err(&spi->dev, "Failed to register IIO device\n");
>> Hmm. If we were to flesh out some error messages for the few
>> cases where there is no info provided in iio_device_register we could
>> probably clean out a fair bit of boiler plate reporting in drivers.
>> Ah well, one for the future!
>>>> + goto device_register_failed;
>>>> + }
>>>> +
>>>> + dev_info(&spi->dev, "probed!\n");
>>>
>>> no logging please, just outputs clutter
>>>
>>>> + return 0;
>>>> +
>>>> +device_register_failed:
>>>> + iio_triggered_buffer_cleanup(indio_dev);
>>>> +buffer_setup_failed:
>>>> + regulator_disable(adc->reg);
>>>> + return retval;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Unregister ADC IIO device for SPI.
>>>> + */
>>>> +static int adc084s021_remove(struct spi_device *spi)
>>>> +{
>>>> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>>> + struct adc084s021 *adc = iio_priv(indio_dev);
>>>> +
>>>> + iio_device_unregister(indio_dev);
>>>> + iio_triggered_buffer_cleanup(indio_dev);
>>>> + regulator_disable(adc->reg);
>> blank line here preferred (slightly!)
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id adc084s021_of_match[] = {
>>>> + { .compatible = "ti,adc084s021", },
>>>> + {},
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, adc084s021_of_match);
>>>> +
>>>> +static const struct spi_device_id adc084s021_id[] = {
>>>> + { MODULE_NAME, 0},
>>>> + {}
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(spi, adc084s021_id);
>>>> +
>>>> +static struct spi_driver adc084s021_driver = {
>>>> + .driver = {
>>>> + .name = MODULE_NAME,
>>>> + .of_match_table = of_match_ptr(adc084s021_of_match),
>>>> + },
>>>> + .probe = adc084s021_probe,
>>>> + .remove = adc084s021_remove,
>>>> + .id_table = adc084s021_id,
>>>> +};
>>>> +
>> Convention often says to not bother with a blank line here or before
>> the MODULE_DEVICE_TABLE above. Gives a visual indication that these macros
>> are being passed the structures.
>> (really minor point!)
>>>> +module_spi_driver(adc084s021_driver);
>>>> +
>>>> +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
>>>> +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_VERSION(DRIVER_VERSION);
>>>>
>>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/4] TI Bluetooth serdev support
From: Adam Ford @ 2017-04-30 15:14 UTC (permalink / raw)
To: Rob Herring
Cc: Marcel Holtmann, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Johan Hedberg,
Gustavo Padovan, Satish Patel, Wei Xu, Eyal Reizer,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170405183005.20570-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Wed, Apr 5, 2017 at 1:30 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> This series adds serdev support to the HCI LL protocol used on TI BT
> modules and enables support on HiKey board with with the WL1835 module.
> With this the custom TI UIM daemon and btattach are no longer needed.
Without UIM daemon, what instruction do you use to load the BT firmware?
I was thinking 'hciattach' but I was having trouble. I was hoping you
might have some insight.
hciattach -t 30 -s 115200 /dev/ttymxc1 texas 3000000 flow Just
returns a timeout.
I modified my i.MX6 device tree per the binding documentation and
setup the regulators and enable GPIO pins.
adam
>
> The series is available on this git branch[1]. Patch 2 is just clean-up
> and can be applied independently. Patch 3 is dependent on the series
> "Nokia H4+ support". I'd suggest both series are merged thru the BT tree.
>
> Rob
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git ti-bluetooth
>
> Rob Herring (4):
> dt-bindings: net: Add TI WiLink shared transport binding
> bluetooth: hci_uart: remove unused hci_uart_init_tty
> bluetooth: hci_uart: add LL protocol serdev driver support
> arm64: dts: hikey: add WL1835 Bluetooth device node
>
> .../devicetree/bindings/net/ti,wilink-st.txt | 35 +++
> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 5 +
> drivers/bluetooth/hci_ldisc.c | 19 --
> drivers/bluetooth/hci_ll.c | 261 ++++++++++++++++++++-
> drivers/bluetooth/hci_uart.h | 1 -
> 5 files changed, 300 insertions(+), 21 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/ti,wilink-st.txt
>
> --
> 2.10.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 2/2] iio: adc: add driver for the ti-adc084s021 chip
From: Jonathan Cameron @ 2017-04-30 15:51 UTC (permalink / raw)
To: Mårten Lindahl
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA, Mårten Lindahl
In-Reply-To: <1493556822-2601-3-git-send-email-marten.lindahl-VrBV9hrLPhE@public.gmane.org>
On 30/04/17 13:53, Mårten Lindahl wrote:
> From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
>
> This adds support for the Texas Instruments ADC084S021 ADC chip.
>
> Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
A few more bits inline. Mostly stuff that has come up
in the V2 changes (and the inevitable bits I missed the
first time!)
Jonathan
> ---
> Changes in v2:
> - Removed most #defines in favor of inlines
> - Corrected channel macro
> - Removed configuration array with only one item
> - Updated func adc084s021_adc_conversion to use be16_to_cpu
> - Added IIO_CHAN_INFO_SCALE to func adc084s021_read_raw
> - Use iio_device_claim_direct_mode in func adc084s021_read_raw
> - Removed documentation for standard driver functions
> - Changed retval to ret everywhere
> - Removed dynamic alloc for data buffer in trigger handler
> - Keeping mutex for all iterations in trigger handler
> - Removed usage of events in this driver
> - Removed info log in probe
> - Use spi_message_init_with_transfers for spi message structs
> - Use preenable and postdisable functions for regulator
> - Inserted blank line before last return in all functions
>
> drivers/iio/adc/Kconfig | 12 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-adc084s021.c | 294 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 307 insertions(+)
> create mode 100644 drivers/iio/adc/ti-adc084s021.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7a..13141e5 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -560,6 +560,18 @@ config TI_ADC0832
> This driver can also be built as a module. If so, the module will be
> called ti-adc0832.
>
> +config TI_ADC084S021
> + tristate "Texas Instruments ADC084S021"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + If you say yes here you get support for Texas Instruments ADC084S021
> + chips.
> +
> + This driver can also be built as a module. If so, the module will be
> + called ti-adc084s021.
> +
> config TI_ADC12138
> tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d001262..b1a6158 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
> new file mode 100644
> index 0000000..2dce257
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc084s021.c
> @@ -0,0 +1,294 @@
> +/**
> + * Copyright (C) 2017 Axis Communications AB
> + *
> + * Driver for Texas Instruments' ADC084S021 ADC chip.
> + * Datasheets can be found here:
> + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
> + *
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define ADC084S021_DRIVER_NAME "adc084s021"
> +
> +struct adc084s021 {
> + struct spi_device *spi;
> + struct spi_message message;
> + struct spi_transfer spi_trans[2];
> + struct regulator *reg;
> + struct mutex lock;
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + */
> + union {
> + u16 tx_buf;
> + __be16 rx_buf;
> + } ____cacheline_aligned;
> +};
> +
> +/**
> + * Channel specification
> + */
Comment doesn't add much so I'd drop it.
> +#define ADC084S021_VOLTAGE_CHANNEL(num) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .channel = (num), \
> + .address = (num) << 3, \
This does feel a little pointless as you can just do the shift inline and
use the channel value instead. Doesn't really matter though.
> + .indexed = 1, \
> + .scan_index = (num), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 8, \
> + .storagebits = 16, \
> + .shift = 4, \
> + .endianness = IIO_BE, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
> + }
> +
> +static const struct iio_chan_spec adc084s021_channels[] = {
> + ADC084S021_VOLTAGE_CHANNEL(0),
> + ADC084S021_VOLTAGE_CHANNEL(1),
> + ADC084S021_VOLTAGE_CHANNEL(2),
> + ADC084S021_VOLTAGE_CHANNEL(3),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +/**
> + * Read an ADC channel and return its value.
> + *
> + * @adc: The ADC SPI data.
> + * @channel: The IIO channel data structure.
> + */
> +static int adc084s021_adc_conversion(struct adc084s021 *adc,
> + struct iio_chan_spec const *channel)
> +{
> + u8 value;
> + int ret;
> +
> + adc->tx_buf = channel->address;
> +
> + /* Do the transfer */
> + ret = spi_sync(adc->spi, &adc->message);
> + if (ret < 0)
> + return ret;
> +
> + value = (be16_to_cpu(adc->rx_buf) >> channel->scan_type.shift) & 0xff;You want to do this for the read_raw sysfs calls, but not the buffered ones
(where this shift and mask is made userspace's problem).
> +
> + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
> + value, channel->channel);
> +
> + return value;
> +}
> +
> +static int adc084s021_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct adc084s021 *adc = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret < 0)
> + return ret;
> +
Unless I'm dozing off this morning, you don't have any power..
So you'll need to enable the regulator first in this path.
Note that the runtime power management autosuspend stuff can
be good for this as it stops the power cycling if a short burst
of readings is taken.
> + ret = adc084s021_adc_conversion(adc, channel);
> + iio_device_release_direct_mode(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + *val = ret;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + ret = regulator_get_voltage(adc->reg);
Given the regulator is currently off as far as this device is concerned
it's possible the answer will be 0...
> + if (ret < 0)
> + return ret;
> +
> + *val = ret / 1000;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +/**
> + * Read enabled ADC channels and push data to the buffer.
> + *
> + * @irq: The interrupt number (not used).
> + * @pollfunc: Pointer to the poll func.
> + */
> +static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void *pollfunc)
> +{
> + struct iio_poll_func *pf = pollfunc;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct adc084s021 *adc = iio_priv(indio_dev);
> + __be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes timestamp */
> + int scan_index;
> + int i = 0;
> +
> + mutex_lock(&adc->lock);
> +
> + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + const struct iio_chan_spec *channel =
> + &indio_dev->channels[scan_index];
> + data[i++] = adc084s021_adc_conversion(adc, channel);
This is clearly a rather simplistic way of handling the read outs.
Perfectly good for an initial version (which may never get improved
on!) but you could do the spi transfers for a set of channels much
more efficiently.
Figure 1 on the datasheet shows how the control register for the next
read can be written in parallel with the previous read. That would
mean each scan took N + 1 2 byte transfers rather than 2N as currently.
I'm not particularly suggesting you do this, but thought I'd just
comment on the possibility as it is a common situation with spi
ADCs (sometimes you get a greater lag between setup and read out
making this even fiddlier!)
If you do want to do this, the trick is to do your transfer setup
and creation stuff in preenable so that it can be customised for
whatever channels are enabled.
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, data,
> + iio_get_time_ns(indio_dev));
> + mutex_unlock(&adc->lock);
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int adc084s021_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct adc084s021 *adc = iio_priv(indio_dev);
> +
> + return regulator_enable(adc->reg);
> +}
> +
> +static int adc084s021_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct adc084s021 *adc = iio_priv(indio_dev);
> +
> + return regulator_disable(adc->reg);
> +}
> +
> +static const struct iio_info adc084s021_info = {
> + .read_raw = adc084s021_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static const struct iio_buffer_setup_ops adc084s021_buffer_setup_ops = {
> + .preenable = adc084s021_buffer_preenable,
> + .postenable = iio_triggered_buffer_postenable,
> + .predisable = iio_triggered_buffer_predisable,
> + .postdisable = adc084s021_buffer_postdisable,
> +};
> +
> +static int adc084s021_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct adc084s021 *adc;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> + if (!indio_dev) {
> + dev_err(&spi->dev, "Failed to allocate IIO device\n");
> + return -ENOMEM;
> + }
> +
> + adc = iio_priv(indio_dev);
> + adc->spi = spi;
> + spi->bits_per_word = 8;
> +
> + /* Update the SPI device with config and connect the iio dev */
> + ret = spi_setup(spi);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to update SPI device\n");
> + return ret;
> + }
> + spi_set_drvdata(spi, indio_dev);
> +
> + /* Initiate the Industrial I/O device */
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->dev.of_node = spi->dev.of_node;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &adc084s021_info;
> + indio_dev->channels = adc084s021_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adc084s021_channels);
> +
> + /* Create SPI transfer for channel reads */
> + adc->spi_trans[0].tx_buf = &adc->tx_buf;
> + adc->spi_trans[0].len = 2;
> + adc->spi_trans[0].speed_hz = spi->max_speed_hz;
You don't need to set these for individual transfers unless they
are different from how the spi bus has been set up...
It's handled in __spi_validate in drivers/spi/spi.c
list_for_each_entry(xfer, &message->transfers, transfer_list) {
message->frame_length += xfer->len;
if (!xfer->bits_per_word)
xfer->bits_per_word = spi->bits_per_word;
if (!xfer->speed_hz)
xfer->speed_hz = spi->max_speed_hz;
...
So it will set them spi->... in the core if you haven't overridden.
> + adc->spi_trans[0].bits_per_word = spi->bits_per_word;
> + adc->spi_trans[1].rx_buf = &adc->rx_buf;
> + adc->spi_trans[1].len = 2;
> + adc->spi_trans[1].speed_hz = spi->max_speed_hz;
> + adc->spi_trans[1].bits_per_word = spi->bits_per_word;
> +
> + spi_message_init_with_transfers(&adc->message, adc->spi_trans, 2);
> +
> + adc->reg = devm_regulator_get(&spi->dev, "vref");
> + if (IS_ERR(adc->reg))
> + return PTR_ERR(adc->reg);
> +
> + mutex_init(&adc->lock);
> +
> + /* Setup triggered buffer with pollfunction */
> + ret = iio_triggered_buffer_setup(indio_dev, NULL,
> + adc084s021_buffer_trigger_handler,
> + &adc084s021_buffer_setup_ops);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to setup triggered buffer\n");
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to register IIO device\n");
> + iio_triggered_buffer_cleanup(indio_dev);
With devm usage this won't be needed any more.
Hmm. We should improve the error message from the core as it'll allow us
to remove a lot of boiler plate in drivers. Ah well, a project for
another day.
> + }
> +
> + return ret;
> +}
> +
> +static int adc084s021_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> + iio_device_unregister(indio_dev);
> + iio_triggered_buffer_cleanup(indio_dev);
Now you have moved to dynamically enabling the regulator, it makes
sense to use the devm_ versions of iio_device_register and
iio_triggered_buffer setup.
With those, you'll be able to get rid of the remove callback entirely as
there will be nothing to do.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id adc084s021_of_match[] = {
> + { .compatible = "ti,adc084s021", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, adc084s021_of_match);
> +
> +static const struct spi_device_id adc084s021_id[] = {
> + { ADC084S021_DRIVER_NAME, 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, adc084s021_id);
> +
> +static struct spi_driver adc084s021_driver = {
> + .driver = {
> + .name = ADC084S021_DRIVER_NAME,
> + .of_match_table = of_match_ptr(adc084s021_of_match),
> + },
> + .probe = adc084s021_probe,
> + .remove = adc084s021_remove,
> + .id_table = adc084s021_id,
> +};
> +module_spi_driver(adc084s021_driver);
> +
> +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
> +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("1.0");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/4] TI Bluetooth serdev support
From: Sebastian Reichel @ 2017-04-30 16:04 UTC (permalink / raw)
To: Adam Ford
Cc: Mark Rutland, Rob Herring, Johan Hedberg, devicetree,
Gustavo Padovan, Marcel Holtmann, Wei Xu, linux-bluetooth,
Eyal Reizer, netdev, Satish Patel, linux-arm-kernel
In-Reply-To: <CAHCN7xJUxZm1qKAxT0kaK6qoDg+HWOJK7sTH-q9za4HJuUwe8Q@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1160 bytes --]
Hi,
On Sun, Apr 30, 2017 at 10:14:20AM -0500, Adam Ford wrote:
> On Wed, Apr 5, 2017 at 1:30 PM, Rob Herring <robh@kernel.org> wrote:
> > This series adds serdev support to the HCI LL protocol used on TI BT
> > modules and enables support on HiKey board with with the WL1835 module.
> > With this the custom TI UIM daemon and btattach are no longer needed.
>
> Without UIM daemon, what instruction do you use to load the BT firmware?
>
> I was thinking 'hciattach' but I was having trouble. I was hoping you
> might have some insight.
>
> hciattach -t 30 -s 115200 /dev/ttymxc1 texas 3000000 flow Just
> returns a timeout.
>
> I modified my i.MX6 device tree per the binding documentation and
> setup the regulators and enable GPIO pins.
If you configured everything correctly no userspace interaction is
required. The driver should request the firmware automatically once
you power up the bluetooth device.
Apart from DT changes make sure, that the following options are
enabled and check dmesg for any hints.
CONFIG_SERIAL_DEV_BUS
CONFIG_SERIAL_DEV_CTRL_TTYPORT
CONFIG_BT_HCIUART
CONFIG_BT_HCIUART_LL
-- Sebastian
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 1/3] ARM: dts: rockchip: Move cros-ec-sbs to rk3288-veyron-chromebook-sbs
From: Paul Kocialkowski @ 2017-04-30 18:30 UTC (permalink / raw)
To: linux-arm-kernel, linux-rockchip, devicetree, linux-kernel
Cc: Heiko Stuebner, Rob Herring, Mark Rutland, Russell King,
Paul Kocialkowski
This moves the cros-ec-sbs dtsi to a new rk3288-veyron-chromebook-sbs
dtsi since it only concerns rk3288 veyron Chromebooks.
Other Chromebooks (such as the tegra124 nyans) also have sbs batteries
and don't use this dtsi, that only makes sense when used with
rk3288-veyron-chromebook anyway.
Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
.../boot/dts/{cros-ec-sbs.dtsi => rk3288-veyron-chromebook-sbs.dtsi} | 0
arch/arm/boot/dts/rk3288-veyron-jaq.dts | 2 +-
arch/arm/boot/dts/rk3288-veyron-jerry.dts | 2 +-
arch/arm/boot/dts/rk3288-veyron-pinky.dts | 2 +-
arch/arm/boot/dts/rk3288-veyron-speedy.dts | 2 +-
5 files changed, 4 insertions(+), 4 deletions(-)
rename arch/arm/boot/dts/{cros-ec-sbs.dtsi => rk3288-veyron-chromebook-sbs.dtsi} (100%)
diff --git a/arch/arm/boot/dts/cros-ec-sbs.dtsi b/arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi
similarity index 100%
rename from arch/arm/boot/dts/cros-ec-sbs.dtsi
rename to arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi
diff --git a/arch/arm/boot/dts/rk3288-veyron-jaq.dts b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
index d33f5763c39c..f217a978e47a 100644
--- a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
@@ -45,7 +45,7 @@
/dts-v1/;
#include "rk3288-veyron-chromebook.dtsi"
-#include "cros-ec-sbs.dtsi"
+#include "rk3288-veyron-chromebook-sbs.dtsi"
/ {
model = "Google Jaq";
diff --git a/arch/arm/boot/dts/rk3288-veyron-jerry.dts b/arch/arm/boot/dts/rk3288-veyron-jerry.dts
index cdea751f2a8c..bec607574165 100644
--- a/arch/arm/boot/dts/rk3288-veyron-jerry.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-jerry.dts
@@ -44,7 +44,7 @@
/dts-v1/;
#include "rk3288-veyron-chromebook.dtsi"
-#include "cros-ec-sbs.dtsi"
+#include "rk3288-veyron-chromebook-sbs.dtsi"
/ {
model = "Google Jerry";
diff --git a/arch/arm/boot/dts/rk3288-veyron-pinky.dts b/arch/arm/boot/dts/rk3288-veyron-pinky.dts
index 995cff42fa43..c81ad5bf1121 100644
--- a/arch/arm/boot/dts/rk3288-veyron-pinky.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-pinky.dts
@@ -44,7 +44,7 @@
/dts-v1/;
#include "rk3288-veyron-chromebook.dtsi"
-#include "cros-ec-sbs.dtsi"
+#include "rk3288-veyron-chromebook-sbs.dtsi"
/ {
model = "Google Pinky";
diff --git a/arch/arm/boot/dts/rk3288-veyron-speedy.dts b/arch/arm/boot/dts/rk3288-veyron-speedy.dts
index cc0b78cefe34..8aea9c3ff6e2 100644
--- a/arch/arm/boot/dts/rk3288-veyron-speedy.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-speedy.dts
@@ -44,7 +44,7 @@
/dts-v1/;
#include "rk3288-veyron-chromebook.dtsi"
-#include "cros-ec-sbs.dtsi"
+#include "rk3288-veyron-chromebook-sbs.dtsi"
/ {
model = "Google Speedy";
--
2.12.2
^ permalink raw reply related
* [PATCH 2/3] ARM: dts: rockchip: List charger as power supply for sbs battery
From: Paul Kocialkowski @ 2017-04-30 18:30 UTC (permalink / raw)
To: linux-arm-kernel, linux-rockchip, devicetree, linux-kernel
Cc: Heiko Stuebner, Rob Herring, Mark Rutland, Russell King,
Paul Kocialkowski
In-Reply-To: <20170430183054.24563-1-contact@paulk.fr>
This lists the GPIO charger node as power supply for the battery, which
in turns allows receiving external power notifications and synchronizing
the charging state as soon as possible.
Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi | 1 +
arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi b/arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi
index 71f5c5ecce46..8e4d2b9a35e1 100644
--- a/arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron-chromebook-sbs.dtsi
@@ -48,5 +48,6 @@
reg = <0xb>;
sbs,i2c-retry-count = <2>;
sbs,poll-retry-count = <1>;
+ power-supplies = <&charger>;
};
};
diff --git a/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi b/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
index d752a315f884..fd4a3886c94b 100644
--- a/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
@@ -99,7 +99,7 @@
pwm-delay-us = <10000>;
};
- gpio-charger {
+ charger: gpio-charger {
compatible = "gpio-charger";
charger-type = "mains";
gpios = <&gpio0 RK_PB0 GPIO_ACTIVE_HIGH>;
--
2.12.2
^ permalink raw reply related
* [PATCH 3/3] ARM: dts: rockchip: List charger as power supply for minnie
From: Paul Kocialkowski @ 2017-04-30 18:30 UTC (permalink / raw)
To: linux-arm-kernel, linux-rockchip, devicetree, linux-kernel
Cc: Heiko Stuebner, Rob Herring, Mark Rutland, Russell King,
Paul Kocialkowski
In-Reply-To: <20170430183054.24563-1-contact@paulk.fr>
This lists the GPIO charger node as power supply for the battery, which
in turns allows receiving external power notifications and synchronizing
the charging state as soon as possible.
Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
arch/arm/boot/dts/rk3288-veyron-minnie.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
index 544de6027aaa..3f97f33bd783 100644
--- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
@@ -151,6 +151,7 @@
battery: bq27500@55 {
compatible = "ti,bq27500";
reg = <0x55>;
+ power-supplies = <&charger>;
};
};
--
2.12.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox