* Re: [PATCH 3/3] arm64: dts: qcom: Add initial support for Xiaomi Redmi Note 8T
From: Eli Riggs @ 2020-05-19 11:20 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel,
~postmarketos/upstreaming
In-Reply-To: <20200519061114.GC2165@builder.lan>
On Mon, 18 May 2020 23:11:14 -0700
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> On Sun 17 May 04:54 PDT 2020, Eli Riggs wrote:
>
> > Adds initial device tree for Xiaomi Redmi Note 8T, codename
> > xiaomi-willow. It uses the sm6125 SoC. Currently only boots into
> > initrd shell over UART. Requires appended DTB with qcom,board-id =
> > <0x22 0x0> and qcom,msm-id = <0x18a 0x10000> to actually boot.
> >
>
> If I read this correctly you need to supply board-id and msm-id in
> order to be able to get this booting?
>
> Even though we don't like them, I would prefer if you just add them in
> the dts file, in this patch.
Yes, AFAICT the bootloader iterates over the appended DTBs and will
only boot if it finds one with those matching properties.
> > dtb-$(CONFIG_ARCH_QCOM) += sm8250-mtp.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb diff --git
> > a/arch/arm64/boot/dts/qcom/sm6125-xiaomi-willow.dts
> > b/arch/arm64/boot/dts/qcom/sm6125-xiaomi-willow.dts new file mode
> > 100644 index 0000000000000..444b32ccb9d48 --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sm6125-xiaomi-willow.dts
> > @@ -0,0 +1,19 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
>
> Please make this GPL/BSD dual license.
>
> Apart from these few remarks your patches looks good, looking forward
> to see more of this platform!
>
> Regards,
> Bjorn
OK, thanks!
^ permalink raw reply
* Re: [PATCH 2/3] arm64: dts: qcom: Add initial sm6125 SoC support
From: Eli Riggs @ 2020-05-19 11:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel,
~postmarketos/upstreaming
In-Reply-To: <20200519060848.GB2165@builder.lan>
On Mon, 18 May 2020 23:08:48 -0700
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> Please use dual GPL/BSD license for dts files, if you can.
Unfortunately the downstream tree I ported has a GPL-2-only header.
> [...review]
OK
> Given that you won't get very far without GCC and e.g. pinctrl
> driver I would prefer to see some patches for those as well, to
> ensure that this will be able to go beyond basic UART.
Cleaning up my gcc and clk-smd-rpm drivers now, as well as another
patchset for pm6125, qusb2-phy, dwc3, and sdhci. TLMM in the vague
future.
Eli
^ permalink raw reply
* Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
From: Valdis Klētnieks @ 2020-05-19 11:17 UTC (permalink / raw)
To: Qii Wang
Cc: Joe Perches, Geert Uytterhoeven, Wolfram Sang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
srv_heupstream, leilk.liu, Linux Kernel Mailing List,
linux-mediatek, Linux I2C, Linux ARM
In-Reply-To: <1589857073.25512.34.camel@mhfsdcap03>
[-- Attachment #1: Type: text/plain, Size: 599 bytes --]
On Tue, 19 May 2020 10:57:53 +0800, Qii Wang said:
> (1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
> * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
>
> I think 1000000000 and clk_src is too big, maybe I can reduce then with
> be divided all by 1000.
Yes, it's definitely too big, the 3 DIV_ROUND_UP calls in mtk_i2c_check_ac_timing()
end up causing a build issue during modpost on a 32-bit RPi4:
ERROR: modpost: "__aeabi_uldivmod" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
ERROR: modpost: "__aeabi_ldivmod" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH] arm64: dts: rockchip: fix pinctrl-names for gpio-leds node on rk3326-odroid-go2
From: Johan Jonker @ 2020-05-19 11:14 UTC (permalink / raw)
To: heiko; +Cc: robh+dt, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
The 'pinctrl-names' property should contain a list of names
to the assigned states. The value 'led_pins' in the gpio-leds
node on rk3326-odroid-go2 is not a state that is normally used,
so change it the common name 'default'.
Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
index 46826b6e2..b3a8f9365 100644
--- a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
@@ -127,7 +127,7 @@
leds: gpio-leds {
compatible = "gpio-leds";
- pinctrl-names = "led_pins";
+ pinctrl-names = "default";
pinctrl-0 = <&blue_led_pin>;
blue_led: led-0 {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller
From: Lee Jones @ 2020-05-19 10:49 UTC (permalink / raw)
To: Lubomir Rintel; +Cc: Rob Herring, Mark Rutland, devicetree, linux-kernel
In-Reply-To: <20200504192354.GA414543@furthur.local>
On Mon, 04 May 2020, Lubomir Rintel wrote:
> Hi,
>
> thanks for your review. There are some inline responses below. Where I'm not
> responding it means that I'll be just fixing what you've pointed out.
>
> On Wed, Apr 29, 2020 at 07:00:37AM +0100, Lee Jones wrote:
> > On Sat, 25 Apr 2020, Lubomir Rintel wrote:
> >
> > > This driver provides access to the EC RAM of said embedded controller
> > > attached to the I2C bus as well as optionally supporting its slightly weird
> > > power-off/restart protocol.
> > >
> > > A particular implementation of the EC firmware can be identified by a
> > > model byte. If this driver identifies the Dell Ariel platform, it
> > > registers the appropriate cells.
> > >
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > > drivers/mfd/Kconfig | 10 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/ene-kb3930.c | 209 +++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 220 insertions(+)
> > > create mode 100644 drivers/mfd/ene-kb3930.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 0a59249198d3..dae18a2beab5 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -398,6 +398,16 @@ config MFD_DLN2
> > > etc. must be enabled in order to use the functionality of
> > > the device.
> > >
> > > +config MFD_ENE_KB3930
> > > + tristate "ENE KB3930 Embedded Controller support"
> > > + depends on I2C
> > > + depends on MACH_MMP3_DT || COMPILE_TEST
> > > + select MFD_CORE
> > > + help
> > > + This adds support for accessing the registers on ENE KB3930, Embedded
> > > + Controller. Additional drivers such as LEDS_ARIEL must be enabled in
> > > + order to use the functionality of the device.
> > > +
> > > config MFD_EXYNOS_LPASS
> > > tristate "Samsung Exynos SoC Low Power Audio Subsystem"
> > > depends on ARCH_EXYNOS || COMPILE_TEST
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index f935d10cbf0f..2d2f5bc12841 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> > > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> > > obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o
> > > +obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o
> > > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o
> > >
> > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
> > > diff --git a/drivers/mfd/ene-kb3930.c b/drivers/mfd/ene-kb3930.c
> > > new file mode 100644
> > > index 000000000000..1123f3a1c816
> > > --- /dev/null
> > > +++ b/drivers/mfd/ene-kb3930.c
> > > @@ -0,0 +1,209 @@
> > > +// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
> > > +/*
> > > + * ENE KB3930 Embedded Controller Driver
> > > + *
> > > + * Copyright (C) 2020 Lubomir Rintel
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/mfd/core.h>
> >
> > Alphabetical.
> >
> > > +enum {
> > > + EC_DATA_IN = 0x00,
> > > + EC_RAM_OUT = 0x80,
> > > + EC_RAM_IN = 0x81,
> > > +};
> >
> > Are these registers?
>
> These are I2C registers that are multiplexing access to the EC RAM.
> Should I add a comment or make it clearer in some other way?
A comment sounds good.
> > > +enum {
> > > + EC_MODEL_ID = 0x30,
> > > + EC_VERSION_MAJ = 0x31,
> > > + EC_VERSION_MIN = 0x32,
> > > +};
> >
> > As above?
>
> These are the locations in EC RAM, multiplexed via EC_DATA_IN and
> EC_RAM_IN/OUT.
As above.
> > > +struct kb3930 {
> > > + struct i2c_client *client;
> > > + struct regmap *ec_ram;
> >
> > This is usually called 'regmap'.
>
> Yes. But the device has a set of registers directly on the I2C bus as
> well as another set of registers in the RAM access to which is
> multiplexed via a pair of I2C registers.
>
> This regmap is for the latter register block which is the only one
> exposed currently. I believe it still makes sense to make it obvious
> this is not the I2C registers in case the driver is extended to expose
> those in future.
ram_regmap or similar.
> > > + struct gpio_descs *off_gpios;
> > > +};
> > > +
> > > +struct kb3930 *global_kb3930;
> >
> > Globals are massively frowned upon. Please move it.
>
> This is necessary for the pm_power_off hook that takes no argument. All
> other MFD drivers that implement power off use a global:
Ah, it's one of those:
static struct kb3920_power_off
> ab8500-sysctrl.c: static struct device *sysctrl_dev;
> axp20x.c: static struct axp20x_dev *axp20x_pm_power_off;
> dm355evm_msp.c: static struct i2c_client *msp430;
> max77620.c: static struct max77620_chip *max77620_scratch;
> max8907.c: static struct max8907 *max8907_pm_off;
> palmas.c: static struct palmas *palmas_dev;
> retu-mfd.c: static struct retu_dev *retu_pm_power_off;
> rk808.c: static struct i2c_client *rk808_i2c_client;
> rn5t618.c: static struct rn5t618 *rn5t618_pm_power_off;
> tps6586x.c: static struct device *tps6586x_dev;
> tps65910.c: static struct i2c_client *tps65910_i2c_client;
> tps80031.c: static struct tps80031 *tps80031_power_off_dev;
> twl-core.c: static struct twl_private *twl_priv;
[...]
> > > + ariel_ec_cells,
> > > + ARRAY_SIZE(ariel_ec_cells),
> > > + NULL, 0, NULL);
> > > + if (ret < 0)
> > > + return ret;
> > > + } else {
> > > + dev_err(dev, "unknown board model: %02x\n", model_id);
> > > + return -ENODEV;
> >
> > If you reverse the logic here, you can put this in the if() and omit
> > the else.
>
> It is intentionally structured this way.
>
> Though the driver currently only supports the 'J' version of the EC
> firmware, other versions are possible, with different cells exposed via
> the EC RAM registers.
It's difficult to review based on whatifs.
I think it's worth doing it right for the current situation and adapt
it *if* things change in the future.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v11 2/6] mfd: mp2629: Add support for mps battery charger
From: Lee Jones @ 2020-05-19 10:37 UTC (permalink / raw)
To: Saravanan Sekar
Cc: andy.shevchenko, robh+dt, jic23, knaack.h, lars, pmeerw, sre,
devicetree, linux-kernel, linux-iio, linux-pm
In-Reply-To: <20200430155810.21383-3-sravanhome@gmail.com>
On Thu, 30 Apr 2020, Saravanan Sekar wrote:
> mp2629 is a highly-integrated switching-mode battery charge management
> device for single-cell Li-ion or Li-polymer battery.
>
> Add MFD core enables chip access for ADC driver for battery readings,
> and a power supply battery-charger driver
>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/mfd/Kconfig | 9 +++++
> drivers/mfd/Makefile | 2 +
> drivers/mfd/mp2629.c | 79 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/mp2629.h | 17 ++++++++
> 4 files changed, 107 insertions(+)
> create mode 100644 drivers/mfd/mp2629.c
> create mode 100644 include/linux/mfd/mp2629.h
For my own reference (apply this as-is to your sign-off block):
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 2/2] mfd: Introduce QTI I2C PMIC controller
From: Lee Jones @ 2020-05-19 10:33 UTC (permalink / raw)
To: Guru Das Srinagesh
Cc: devicetree, linux-arm-msm, Rob Herring, Subbaraman Narayanamurthy,
David Collins, linux-kernel, tglx, jason, maz
In-Reply-To: <041dd80e01d1bf86da2edc2747e922c473b50509.1588037638.git.gurus@codeaurora.org>
[Adding IRQ chaps]
Seeing as a vast portion of this driver is IRQ domain related, I see
good reason to separate it out into a driver in its own right. At the
very least it will require an IRQ *-by tag.
On Mon, 27 Apr 2020, Guru Das Srinagesh wrote:
> The Qualcomm Technologies, Inc. I2C PMIC Controller is used by
> multi-function PMIC devices which communicate over the I2C bus. The
> controller enumerates all child nodes as platform devices, and
> instantiates a regmap interface for them to communicate over the I2C
> bus.
>
> The controller also controls interrupts for all of the children platform
> devices. The controller handles the summary interrupt by deciphering
> which peripheral triggered the interrupt, and which of the peripheral
> interrupts were triggered. Finally, it calls the interrupt handlers for
> each of the virtual interrupts that were registered.
> Nicholas Troast is the original author of this driver.
>
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
> drivers/mfd/Kconfig | 11 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/qcom-i2c-pmic.c | 737 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 749 insertions(+)
> create mode 100644 drivers/mfd/qcom-i2c-pmic.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 54b6aa4..bf112eb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1002,6 +1002,17 @@ config MFD_PM8XXX
> Say M here if you want to include support for PM8xxx chips as a
> module. This will build a module called "pm8xxx-core".
>
> +config MFD_I2C_PMIC
"I2C PMIC" is too generic.
> + tristate "QTI I2C PMIC support"
I don't see QTI used very often.
What's the difference between "QTI" and "QCOM"?
> + depends on I2C && OF
> + select IRQ_DOMAIN
> + select REGMAP_I2C
> + help
> + This enables support for controlling Qualcomm Technologies, Inc.
> + PMICs over I2C. The driver controls interrupts, and provides register
> + access for all of the device's peripherals. Some QTI PMIC chips
> + support communication over both I2C and SPMI.
> +
> config MFD_QCOM_RPM
> tristate "Qualcomm Resource Power Manager (RPM)"
> depends on ARCH_QCOM && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7761f84..26f0b80 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -199,6 +199,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> obj-$(CONFIG_MFD_PM8XXX) += qcom-pm8xxx.o ssbi.o
> +obj-$(CONFIG_MFD_I2C_PMIC) += qcom-i2c-pmic.o
> obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o
> obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o
> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> diff --git a/drivers/mfd/qcom-i2c-pmic.c b/drivers/mfd/qcom-i2c-pmic.c
> new file mode 100644
> index 0000000..d0f600a
> --- /dev/null
> +++ b/drivers/mfd/qcom-i2c-pmic.c
> @@ -0,0 +1,737 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
This is out of date.
No author tag?
> + */
> +
> +#define pr_fmt(fmt) "I2C PMIC: %s: " fmt, __func__
Outside of in-development code I don't see a reason for adding the
function name to system log entries. IMHO, it just dirties the log on
production/released H/W. Please considering removing this
altogether in from plain device drivers.
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
I'm going to skip all of the IRQ code until we know what we're doing
with it.
#########
<-- IRQ -->
#########
> +#define I2C_INTR_STATUS_BASE 0x0550
> +#define INT_RT_STS_OFFSET 0x10
> +#define INT_SET_TYPE_OFFSET 0x11
> +#define INT_POL_HIGH_OFFSET 0x12
> +#define INT_POL_LOW_OFFSET 0x13
> +#define INT_LATCHED_CLR_OFFSET 0x14
> +#define INT_EN_SET_OFFSET 0x15
> +#define INT_EN_CLR_OFFSET 0x16
> +#define INT_LATCHED_STS_OFFSET 0x18
> +#define INT_PENDING_STS_OFFSET 0x19
> +#define INT_MID_SEL_OFFSET 0x1A
> +#define INT_MID_SEL_MASK GENMASK(1, 0)
> +#define INT_PRIORITY_OFFSET 0x1B
> +#define INT_PRIORITY_BIT BIT(0)
> +
> +enum {
> + IRQ_SET_TYPE = 0,
> + IRQ_POL_HIGH,
> + IRQ_POL_LOW,
> + IRQ_LATCHED_CLR, /* not needed but makes life easy */
> + IRQ_EN_SET,
> + IRQ_MAX_REGS,
> +};
> +
> +struct i2c_pmic_periph {
> + void *data;
> + u16 addr;
> + u8 cached[IRQ_MAX_REGS];
> + u8 synced[IRQ_MAX_REGS];
> + u8 wake;
> + struct mutex lock;
> +};
> +
> +struct i2c_pmic {
> + struct device *dev;
> + struct regmap *regmap;
> + struct irq_domain *domain;
> + struct i2c_pmic_periph *periph;
> + struct pinctrl *pinctrl;
> + struct mutex irq_complete;
> + const char *pinctrl_name;
> + int num_periphs;
> + int summary_irq;
> + bool resume_completed;
> + bool irq_waiting;
> +};
> +
> +static void i2c_pmic_irq_bus_lock(struct irq_data *d)
> +{
> + struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> + mutex_lock(&periph->lock);
> +}
> +
> +static void i2c_pmic_sync_type_polarity(struct i2c_pmic *chip,
> + struct i2c_pmic_periph *periph)
> +{
> + int rc;
> +
> + /* did any irq type change? */
> + if (periph->cached[IRQ_SET_TYPE] ^ periph->synced[IRQ_SET_TYPE]) {
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_SET_TYPE_OFFSET,
> + periph->cached[IRQ_SET_TYPE]);
> + if (rc < 0) {
> + pr_err("Couldn't set periph 0x%04x irqs 0x%02x type rc=%d\n",
> + periph->addr, periph->cached[IRQ_SET_TYPE], rc);
> + return;
> + }
> +
> + periph->synced[IRQ_SET_TYPE] = periph->cached[IRQ_SET_TYPE];
> + }
> +
> + /* did any polarity high change? */
> + if (periph->cached[IRQ_POL_HIGH] ^ periph->synced[IRQ_POL_HIGH]) {
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_POL_HIGH_OFFSET,
> + periph->cached[IRQ_POL_HIGH]);
> + if (rc < 0) {
> + pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity high rc=%d\n",
> + periph->addr, periph->cached[IRQ_POL_HIGH], rc);
> + return;
> + }
> +
> + periph->synced[IRQ_POL_HIGH] = periph->cached[IRQ_POL_HIGH];
> + }
> +
> + /* did any polarity low change? */
> + if (periph->cached[IRQ_POL_LOW] ^ periph->synced[IRQ_POL_LOW]) {
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_POL_LOW_OFFSET,
> + periph->cached[IRQ_POL_LOW]);
> + if (rc < 0) {
> + pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity low rc=%d\n",
> + periph->addr, periph->cached[IRQ_POL_LOW], rc);
> + return;
> + }
> +
> + periph->synced[IRQ_POL_LOW] = periph->cached[IRQ_POL_LOW];
> + }
> +}
> +
> +static void i2c_pmic_sync_enable(struct i2c_pmic *chip,
> + struct i2c_pmic_periph *periph)
> +{
> + u8 en_set, en_clr;
> + int rc;
> +
> + /* determine which irqs were enabled and which were disabled */
> + en_clr = periph->synced[IRQ_EN_SET] & ~periph->cached[IRQ_EN_SET];
> + en_set = ~periph->synced[IRQ_EN_SET] & periph->cached[IRQ_EN_SET];
> +
> + /* were any irqs disabled? */
> + if (en_clr) {
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_EN_CLR_OFFSET, en_clr);
> + if (rc < 0) {
> + pr_err("Couldn't disable periph 0x%04x irqs 0x%02x rc=%d\n",
> + periph->addr, en_clr, rc);
> + return;
> + }
> + }
> +
> + /* were any irqs enabled? */
> + if (en_set) {
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_EN_SET_OFFSET, en_set);
> + if (rc < 0) {
> + pr_err("Couldn't enable periph 0x%04x irqs 0x%02x rc=%d\n",
> + periph->addr, en_set, rc);
> + return;
> + }
> + }
> +
> + /* irq enabled status was written to hardware */
> + periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
> +}
> +
> +static void i2c_pmic_irq_bus_sync_unlock(struct irq_data *d)
> +{
> + struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> + struct i2c_pmic *chip = periph->data;
> +
> + i2c_pmic_sync_type_polarity(chip, periph);
> + i2c_pmic_sync_enable(chip, periph);
> + mutex_unlock(&periph->lock);
> +}
> +
> +static void i2c_pmic_irq_disable(struct irq_data *d)
> +{
> + struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> + periph->cached[IRQ_EN_SET] &= ~d->hwirq & 0xFF;
> +}
> +
> +static void i2c_pmic_irq_enable(struct irq_data *d)
> +{
> + struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> + periph->cached[IRQ_EN_SET] |= d->hwirq & 0xFF;
> +}
> +
> +static int i2c_pmic_irq_set_type(struct irq_data *d, unsigned int irq_type)
> +{
> + struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> + switch (irq_type) {
> + case IRQ_TYPE_EDGE_RISING:
> + periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> + break;
> + case IRQ_TYPE_EDGE_BOTH:
> + periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
> + periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> + break;
> + default:
> + pr_err("irq type 0x%04x is not supported\n", irq_type);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_pmic_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> + struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> + if (on)
> + periph->wake |= d->hwirq & 0xFF;
> + else
> + periph->wake &= ~d->hwirq & 0xFF;
> +
> + return 0;
> +}
> +#else
> +#define i2c_pmic_irq_set_wake NULL
> +#endif
> +
> +static struct irq_chip i2c_pmic_irq_chip = {
> + .name = "i2c_pmic_irq_chip",
> + .irq_bus_lock = i2c_pmic_irq_bus_lock,
> + .irq_bus_sync_unlock = i2c_pmic_irq_bus_sync_unlock,
> + .irq_disable = i2c_pmic_irq_disable,
> + .irq_enable = i2c_pmic_irq_enable,
> + .irq_set_type = i2c_pmic_irq_set_type,
> + .irq_set_wake = i2c_pmic_irq_set_wake,
> +};
> +
> +static struct i2c_pmic_periph *i2c_pmic_find_periph(struct i2c_pmic *chip,
> + irq_hw_number_t hwirq)
> +{
> + int i;
> +
> + for (i = 0; i < chip->num_periphs; i++)
> + if (chip->periph[i].addr == (hwirq & 0xFF00))
> + return &chip->periph[i];
> +
> + pr_err_ratelimited("Couldn't find periph struct for hwirq 0x%04lx\n",
> + hwirq);
> + return NULL;
> +}
> +
> +static int i2c_pmic_domain_map(struct irq_domain *d, unsigned int virq,
> + irq_hw_number_t hwirq)
> +{
> + struct i2c_pmic *chip = d->host_data;
> + struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
> +
> + if (!periph)
> + return -ENODEV;
> +
> + irq_set_chip_data(virq, periph);
> + irq_set_chip_and_handler(virq, &i2c_pmic_irq_chip, handle_level_irq);
> + irq_set_nested_thread(virq, 1);
> + irq_set_noprobe(virq);
> + return 0;
> +}
> +
> +static int i2c_pmic_domain_xlate(struct irq_domain *d,
> + struct device_node *ctrlr, const u32 *intspec,
> + unsigned int intsize, unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (intsize != 3)
> + return -EINVAL;
> +
> + if (intspec[0] > 0xFF || intspec[1] > 0x7 ||
> + intspec[2] > IRQ_TYPE_SENSE_MASK)
> + return -EINVAL;
> +
> + /*
> + * Interrupt specifiers are triplets
> + * <peripheral-address, irq-number, IRQ_TYPE_*>
> + *
> + * peripheral-address - The base address of the peripheral
> + * irq-number - The zero based bit position of the peripheral's
> + * interrupt registers corresponding to the irq
> + * where the LSB is 0 and the MSB is 7
> + * IRQ_TYPE_* - Please refer to linux/irq.h
> + */
> + *out_hwirq = intspec[0] << 8 | BIT(intspec[1]);
> + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops i2c_pmic_domain_ops = {
> + .map = i2c_pmic_domain_map,
> + .xlate = i2c_pmic_domain_xlate,
> +};
> +
> +static void i2c_pmic_irq_ack_now(struct i2c_pmic *chip, u16 hwirq)
> +{
> + int rc;
> +
> + rc = regmap_write(chip->regmap,
> + (hwirq & 0xFF00) | INT_LATCHED_CLR_OFFSET,
> + hwirq & 0xFF);
> + if (rc < 0)
> + pr_err_ratelimited("Couldn't ack 0x%04x rc=%d\n", hwirq, rc);
> +}
> +
> +static void i2c_pmic_irq_disable_now(struct i2c_pmic *chip, u16 hwirq)
> +{
> + struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
> + int rc;
> +
> + if (!periph)
> + return;
> +
> + mutex_lock(&periph->lock);
> + periph->cached[IRQ_EN_SET] &= ~hwirq & 0xFF;
> +
> + rc = regmap_write(chip->regmap,
> + (hwirq & 0xFF00) | INT_EN_CLR_OFFSET,
> + hwirq & 0xFF);
> + if (rc < 0) {
> + pr_err_ratelimited("Couldn't disable irq 0x%04x rc=%d\n",
> + hwirq, rc);
> + goto unlock;
> + }
> +
> + periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
> +
> +unlock:
> + mutex_unlock(&periph->lock);
> +}
> +
> +static void i2c_pmic_periph_status_handler(struct i2c_pmic *chip,
> + u16 periph_address, u8 periph_status)
> +{
> + unsigned int hwirq, virq;
> + int i;
> +
> + while (periph_status) {
> + i = ffs(periph_status) - 1;
> + periph_status &= ~BIT(i);
> + hwirq = periph_address | BIT(i);
> + virq = irq_find_mapping(chip->domain, hwirq);
> + if (virq == 0) {
> + pr_err_ratelimited("Couldn't find mapping; disabling 0x%04x\n",
> + hwirq);
> + i2c_pmic_irq_disable_now(chip, hwirq);
> + continue;
> + }
> +
> + handle_nested_irq(virq);
> + i2c_pmic_irq_ack_now(chip, hwirq);
> + }
> +}
> +
> +static void i2c_pmic_summary_status_handler(struct i2c_pmic *chip,
> + struct i2c_pmic_periph *periph,
> + u8 summary_status)
> +{
> + unsigned int periph_status;
> + int rc, i;
> +
> + while (summary_status) {
> + i = ffs(summary_status) - 1;
> + summary_status &= ~BIT(i);
> +
> + rc = regmap_read(chip->regmap,
> + periph[i].addr | INT_LATCHED_STS_OFFSET,
> + &periph_status);
> + if (rc < 0) {
> + pr_err_ratelimited("Couldn't read 0x%04x | INT_LATCHED_STS rc=%d\n",
> + periph[i].addr, rc);
> + continue;
> + }
> +
> + i2c_pmic_periph_status_handler(chip, periph[i].addr,
> + periph_status);
> + }
> +}
> +
> +static irqreturn_t i2c_pmic_irq_handler(int irq, void *dev_id)
> +{
> + struct i2c_pmic *chip = dev_id;
> + struct i2c_pmic_periph *periph;
> + unsigned int summary_status;
> + int rc, i;
> +
> + mutex_lock(&chip->irq_complete);
> + chip->irq_waiting = true;
> + if (!chip->resume_completed) {
> + pr_debug("IRQ triggered before device-resume\n");
> + disable_irq_nosync(irq);
> + mutex_unlock(&chip->irq_complete);
> + return IRQ_HANDLED;
> + }
> + chip->irq_waiting = false;
> +
> + for (i = 0; i < DIV_ROUND_UP(chip->num_periphs, BITS_PER_BYTE); i++) {
> + rc = regmap_read(chip->regmap, I2C_INTR_STATUS_BASE + i,
> + &summary_status);
> + if (rc < 0) {
> + pr_err_ratelimited("Couldn't read I2C_INTR_STATUS%d rc=%d\n",
> + i, rc);
> + continue;
> + }
> +
> + if (summary_status == 0)
> + continue;
> +
> + periph = &chip->periph[i * 8];
> + i2c_pmic_summary_status_handler(chip, periph, summary_status);
> + }
> +
> + mutex_unlock(&chip->irq_complete);
> +
> + return IRQ_HANDLED;
> +}
#########
<-- IRQ -->
#########
> +static int i2c_pmic_parse_dt(struct i2c_pmic *chip)
> +{
> + struct device_node *node = chip->dev->of_node;
s/node/np/
> + int rc, i;
> + u32 temp;
Temp is seldom good nomenclature.
"periph_addr"
> + if (!node) {
> + pr_err("missing device tree\n");
Why are you using pr_err() over dev_err()?
Please use proper grammar (less full-stops) in prints and comments.
The above goes for all - I won't mention it again.
> + return -EINVAL;
> + }
Can this happen.
> + chip->num_periphs = of_property_count_u32_elems(node,
> + "qcom,periph-map");
Prefer the break after the '='.
If you change 's/node/np' you don't need to break at all.
> + if (chip->num_periphs < 0) {
> + pr_err("missing qcom,periph-map property rc=%d\n",
Prefer it if you use plain English for user facing comments.
"Peripheral map not defined"
> + chip->num_periphs);
> + return chip->num_periphs;
> + }
> +
> + if (chip->num_periphs == 0) {
> + pr_err("qcom,periph-map must contain at least one address\n");
"Peripheral map is empty"
> + return -EINVAL;
> + }
> +
> + chip->periph = devm_kcalloc(chip->dev, chip->num_periphs,
> + sizeof(*chip->periph), GFP_KERNEL);
> + if (!chip->periph)
> + return -ENOMEM;
> +
> + for (i = 0; i < chip->num_periphs; i++) {
> + rc = of_property_read_u32_index(node, "qcom,periph-map",
> + i, &temp);
> + if (rc < 0) {
> + pr_err("Couldn't read qcom,periph-map[%d]
> rc=%d\n",
"Peripheral map entry %d is invalid"
> + i, rc);
> + return rc;
> + }
> +
> + chip->periph[i].addr = (u16)(temp << 8);
Worth a comment I think.
> + chip->periph[i].data = chip;
'data' is also not a great variable name, if you can avoid it.
> + mutex_init(&chip->periph[i].lock);
> + }
> +
> + of_property_read_string(node, "pinctrl-names", &chip->pinctrl_name);
> +
> + return rc;
Why return rc here?
> +}
> +
> +#define MAX_I2C_RETRIES 3
Why 3?
> +static int i2c_pmic_read(struct regmap *map, unsigned int reg, void *val,
> + size_t val_count)
> +{
> + int rc, retries = 0;
> +
> + do {
> + rc = regmap_bulk_read(map, reg, val, val_count);
> + } while (rc == -ENOTCONN && retries++ < MAX_I2C_RETRIES);
> +
> + if (retries > 1)
> + pr_err("i2c_pmic_read failed for %d retries, rc = %d\n",
> + retries - 1, rc);
Is this always an error?
Why would the user care if it failed once, then succeeded?
> + return rc;
> +}
> +
> +static int i2c_pmic_determine_initial_status(struct i2c_pmic *chip)
> +{
> + int rc, i;
> +
> + for (i = 0; i < chip->num_periphs; i++) {
> + rc = i2c_pmic_read(chip->regmap,
> + chip->periph[i].addr | INT_SET_TYPE_OFFSET,
> + chip->periph[i].cached, IRQ_MAX_REGS);
> + if (rc < 0) {
> + pr_err("Couldn't read irq data rc=%d\n", rc);
"IRQ"
> + return rc;
> + }
> +
> + memcpy(chip->periph[i].synced, chip->periph[i].cached,
> + IRQ_MAX_REGS * sizeof(*chip->periph[i].synced));
> + }
> +
> + return 0;
> +}
> +
> +static struct regmap_config i2c_pmic_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .max_register = 0xFFFF,
> +};
> +
> +static int i2c_pmic_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct i2c_pmic *chip;
We usually call this ddata. Especially relevant as you are storing it
in *driver_data for reclaim in the functions above.
> + int rc = 0;
'ret' is by far the more common name for this.
$ git grep "int ret[;\|,]" | wc -l
39365
$ git grep "int rc[;\|,]" | wc -l
6761
Let's keep things as consistent as possible please.
> + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->dev = &client->dev;
> + chip->regmap = devm_regmap_init_i2c(client, &i2c_pmic_regmap_config);
> + if (!chip->regmap)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, chip);
'\n'
> + if (!of_property_read_bool(chip->dev->of_node, "interrupt-controller"))
> + goto probe_children;
> +
> + chip->domain = irq_domain_add_tree(client->dev.of_node,
> + &i2c_pmic_domain_ops, chip);
> + if (!chip->domain) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + rc = i2c_pmic_parse_dt(chip);
> + if (rc < 0) {
If you return an error or 0 here, you can change this to:
if (rc)
> + pr_err("Couldn't parse device tree rc=%d\n", rc);
Looks like you already have prints in i2c_pmic_parse_dt().
No need for an additional one.
> + goto cleanup;
> + }
> +
> + rc = i2c_pmic_determine_initial_status(chip);
> + if (rc < 0) {
> + pr_err("Couldn't determine initial status rc=%d\n", rc);
As above.
> + goto cleanup;
> + }
> +
> + if (chip->pinctrl_name) {
> + chip->pinctrl = devm_pinctrl_get_select(chip->dev,
> + chip->pinctrl_name);
> + if (IS_ERR(chip->pinctrl)) {
> + pr_err("Couldn't select %s pinctrl rc=%ld\n",
> + chip->pinctrl_name, PTR_ERR(chip->pinctrl));
> + rc = PTR_ERR(chip->pinctrl);
Do this before the print, then you only need to do it once.
> + goto cleanup;
> + }
> + }
> +
> + chip->resume_completed = true;
Why can't you disable interrupts before suspending?
> + mutex_init(&chip->irq_complete);
> +
> + rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + i2c_pmic_irq_handler,
> + IRQF_ONESHOT | IRQF_SHARED,
> + "i2c_pmic_stat_irq", chip);
> + if (rc < 0) {
> + pr_err("Couldn't request irq %d rc=%d\n", client->irq, rc);
"IRQ"
> + goto cleanup;
> + }
> +
> + chip->summary_irq = client->irq;
> + enable_irq_wake(client->irq);
> +
> +probe_children:
> + of_platform_populate(chip->dev->of_node, NULL, NULL, chip->dev);
devm_*?
> + pr_info("I2C PMIC probe successful\n");
This is superfluous, please remove it.
'\n' here.
> + return rc;
> +
> +cleanup:
> + if (chip->domain)
> + irq_domain_remove(chip->domain);
> + i2c_set_clientdata(client, NULL);
'\n' here.
> + return rc;
> +}
> +
> +static int i2c_pmic_remove(struct i2c_client *client)
> +{
> + struct i2c_pmic *chip = i2c_get_clientdata(client);
> +
> + of_platform_depopulate(chip->dev);
If you use devm_* this becomes superfluous.
> + if (chip->domain)
> + irq_domain_remove(chip->domain);
'\n' here.
> + i2c_set_clientdata(client, NULL);
'\n' here.
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_pmic_suspend_noirq(struct device *dev)
> +{
> + struct i2c_pmic *chip = dev_get_drvdata(dev);
> +
> + if (chip->irq_waiting) {
> + pr_err_ratelimited("Aborting suspend, an interrupt was detected while suspending\n");
> + return -EBUSY;
I haven't seen this before. Why does this platform require it?
What happens when you return early from .suspend_noirq?
Does this system just resume?
> + }
'\n' here.
> + return 0;
> +}
> +
> +static int i2c_pmic_suspend(struct device *dev)
> +{
> + struct i2c_pmic *chip = dev_get_drvdata(dev);
> + struct i2c_pmic_periph *periph;
> + int rc = 0, i;
> +
> + for (i = 0; i < chip->num_periphs; i++) {
> + periph = &chip->periph[i];
> +
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_EN_CLR_OFFSET, 0xFF);
> + if (rc < 0) {
> + pr_err_ratelimited("Couldn't clear 0x%04x irqs rc=%d\n",
"IRQs"
Same goes for all the other mentions of it.
> + periph->addr, rc);
> + continue;
> + }
> +
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_EN_SET_OFFSET,
> + periph->wake);
> + if (rc < 0)
> + pr_err_ratelimited("Couldn't enable 0x%04x wake irqs 0x%02x rc=%d\n",
> + periph->addr, periph->wake, rc);
> + }
'\n' here.
> + if (!rc) {
> + mutex_lock(&chip->irq_complete);
> + chip->resume_completed = false;
> + mutex_unlock(&chip->irq_complete);
> + }
> +
> + return rc;
> +}
> +
> +static int i2c_pmic_resume(struct device *dev)
> +{
> + struct i2c_pmic *chip = dev_get_drvdata(dev);
> + struct i2c_pmic_periph *periph;
> + int rc = 0, i;
> +
> + for (i = 0; i < chip->num_periphs; i++) {
> + periph = &chip->periph[i];
> +
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_EN_CLR_OFFSET, 0xFF);
> + if (rc < 0) {
> + pr_err("Couldn't clear 0x%04x irqs rc=%d\n",
> + periph->addr, rc);
> + continue;
> + }
> +
> + rc = regmap_write(chip->regmap,
> + periph->addr | INT_EN_SET_OFFSET,
> + periph->synced[IRQ_EN_SET]);
> + if (rc < 0)
> + pr_err("Couldn't restore 0x%04x synced irqs 0x%02x rc=%d\n",
> + periph->addr, periph->synced[IRQ_EN_SET], rc);
> + }
> +
> + mutex_lock(&chip->irq_complete);
> + chip->resume_completed = true;
'\n' here.
> + if (chip->irq_waiting) {
> + mutex_unlock(&chip->irq_complete);
Move this above the if(), then you can remove it from the else too.
> + /* irq was pending, call the handler */
> + i2c_pmic_irq_handler(chip->summary_irq, chip);
> + enable_irq(chip->summary_irq);
> + } else {
> + mutex_unlock(&chip->irq_complete);
> + }
> +
> + return rc;
> +}
> +#else
> +static int i2c_pmic_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +static int i2c_pmic_resume(struct device *dev)
> +{
> + return 0;
> +}
> +static int i2c_pmic_suspend_noirq(struct device *dev)
> +{
> + return 0
> +}
> +#endif
> +static const struct dev_pm_ops i2c_pmic_pm_ops = {
> + .suspend = i2c_pmic_suspend,
> + .suspend_noirq = i2c_pmic_suspend_noirq,
> + .resume = i2c_pmic_resume,
> +};
See how drivers/mfd/arizona-core.c removes all of this boilerplate.
> +static const struct of_device_id i2c_pmic_match_table[] = {
> + { .compatible = "qcom,i2c-pmic", },
> + { },
> +};
> +static const struct i2c_device_id i2c_pmic_id[] = {
> + { "i2c-pmic", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, i2c_pmic_id);
If you use probe_new, you can remove this table altogether.
> +static struct i2c_driver i2c_pmic_driver = {
> + .driver = {
> + .name = "i2c_pmic",
> + .pm = &i2c_pmic_pm_ops,
> + .of_match_table = i2c_pmic_match_table,
> + },
> + .probe = i2c_pmic_probe,
> + .remove = i2c_pmic_remove,
> + .id_table = i2c_pmic_id,
> +};
> +
Remove this line.
> +module_i2c_driver(i2c_pmic_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("i2c:i2c_pmic");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
From: Mark Brown @ 2020-05-19 10:32 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Ramil Zaripov, Alexey Malahov, Thomas Bogendoerfer,
Paul Burton, Ralf Baechle, Lee Jones, Miquel Raynal,
Arnd Bergmann, Rob Herring, linux-mips, devicetree, John Garry,
Chuanhong Guo, Andy Shevchenko, Eddie James, Geert Uytterhoeven,
Chris Packham, Tomer Maimon, Masahisa Kojima, Krzysztof Kozlowski,
Florian Fainelli, Jassi Brar, linux-kernel, linux-spi
In-Reply-To: <20200518211727.jrzo6tn7slqzxoyl@mobilestation>
[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]
On Tue, May 19, 2020 at 12:17:27AM +0300, Serge Semin wrote:
> Here is what we need to do to perform the EEPROM-read operation:
> 1) Enable EEPROM-read mode.
> 2) Initialize a corresponding registers with a number of SPI transfer words
> (with bits-per-word taken into account) to read.
> 3) Push opcode + address + dummy bytes into the Tx FIFO. When it's done and
> the Tx FIFO is empty, the controller will proceed with read operations by
> pushing zeros (or ones, don't remember what level it's by default) to MOSI
> and pulling data from MISO into the RX FIFO.
> 4) Keep up with getting data from the Rx FIFO so one wouldn't get overflown.
> Regarding programming write each time. Well, it's up to the driver implementation.
> If opcode, address, dummy bytes and number of words to read are the same as before,
> then re-programming isn't required.
Ah, nice. This should be useful for far more than just flash - most
register reads will also be able to take advantage of this, they follow
a similar write then read pattern.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v1 4/4] of: platform: Batch fwnode parsing when adding all top level devices
From: Marek Szyprowski @ 2020-05-19 10:32 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring, Frank Rowand,
Len Brown, Android Kernel Team, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
ACPI Devel Maling List, Ji Luo, Linux Samsung SOC
In-Reply-To: <18332705-dd61-9a0e-d931-ae610c8fb600@samsung.com>
Hi
On 19.05.2020 09:11, Marek Szyprowski wrote:
> On 19.05.2020 08:48, Saravana Kannan wrote:
>> On Mon, May 18, 2020 at 11:25 PM Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>> On 15.05.2020 07:35, Saravana Kannan wrote:
>>>> The fw_devlink_pause() and fw_devlink_resume() APIs allow batching the
>>>> parsing of the device tree nodes when a lot of devices are added. This
>>>> will significantly cut down parsing time (as much a 1 second on some
>>>> systems). So, use them when adding devices for all the top level
>>>> device
>>>> tree nodes in a system.
>>>>
>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>> This patch recently landed in linux-next 20200518. Sadly, it causes
>>> regression on Samsung Exynos5433-based TM2e board:
>>>
>>> ...
>>>
>>> Both issues, the lack of DMA for SPI device and Synchronous abort in
>>> I2S
>>> probe are new after applying this patch. I'm trying to investigate
>>> which
>>> resources are missing and why. The latter issue means typically that
>>> the
>>> registers for the given device has been accessed without enabling the
>>> needed clocks or power domains.
>> Did you try this copy-pasta fix that I sent later?
>> https://lore.kernel.org/lkml/20200517173453.157703-1-saravanak@google.com/
>>
>>
>> Not every system would need it (my test setup didn't), but it helps
>> some cases.
>>
>> If that fix doesn't help, then some tips for debugging the failing
>> drivers.
>> What this pause/resume patch effectively (not explicitly) does is:
>> 1. Doesn't immediately probe the devices as they are added in
>> of_platform_default_populate_init()
>> 2. Adds them in order to the deferred probe list.
>> 3. Then kicks off deferred probe on them in the order they were added.
>>
>> These drivers are just not handling -EPROBE_DEFER correctly or
>> assuming probe order and that's causing these issues.
>>
>> So, we can either fix that or you can try adding some code to flush
>> the deferred probe workqueue at the end of fw_devlink_resume().
>>
>> Let me know how it goes.
>
> So far it looks that your patch revealed a hidden issue in exynos5433
> clocks configuration, because adding clk_ignore_unused parameter to
> kernel command line fixes the boot. I'm still investigating it, so
> probable you can ignore my regression report. I will let you know asap
> I finish checking it.
>
Okay, I confirm that the issue is in the Exynos I2S driver and
Exynos5433 clock provider. I've posted a quick workaround. I'm sorry for
the noise, your patch is fine.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH 01/10] spi: dw: Add support for polled operation via no IRQ specified in DT
From: Lars Povlsen @ 2020-05-19 10:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lars Povlsen, Mark Brown, SoC Team,
Microchip Linux Driver Support, linux-spi, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List,
Alexandre Belloni, Serge Semin
In-Reply-To: <CAHp75VcA-oDboufsDNx1ZR4+HBwYt7LdLOpbfs7-bM9ByucKJA@mail.gmail.com>
Andy Shevchenko writes:
> On Wed, May 13, 2020 at 5:03 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>>
>> With this change a SPI controller can be added without having a IRQ
>> associated, and causing all transfers to be polled. For SPI controllers
>> without DMA, this can significantly improve performance by less
>> interrupt handling overhead.
>
> ...
>
>> +#define VALID_IRQ(i) (i >= 0)
>
> drivers/rtc/rtc-cmos.c:95:#define is_valid_irq(n) ((n) > 0)
>
> Candidate to be in include/linux/irq.h ?
>
> ...
>
>> + if (VALID_IRQ(dws->irq))
>> + free_irq(dws->irq, master);
>
> Isn't free_irq() aware of invalid ones (not found IRQ in the tree or
> any other backend container won't do anything)?
>
>
>> err_free_master:
>> spi_controller_put(master);
>> return ret;
>> --
>> 2.26.2
I'll rework this with Serge.
Thank you!
--
Lars Povlsen,
Microchip
^ permalink raw reply
* Re: [PATCH v3] arm64: dts: qcom: apq8016-sbc-d3: Add Qualcomm APQ8016 SBC + D3Camera mezzanine
From: Manivannan Sadhasivam @ 2020-05-19 10:22 UTC (permalink / raw)
To: Robert Foss
Cc: Vinod Koul, Bjorn Andersson, Rob Herring, linux-arm-msm,
devicetree, linux-kernel, Nicolas Dechesne
In-Reply-To: <20200518082129.2103683-1-robert.foss@linaro.org>
Hi Robert,
On Mon, May 18, 2020 at 10:21:29AM +0200, Robert Foss wrote:
> Add device treee support for the Qualcomm APQ8016 SBC, otherwise known as
> the Dragonboard 410c with the D3Camera mezzanine expansion board.
>
> The D3Camera mezzanine ships in a kit with a OmniVision 5640 sensor module,
> which is what this DT targets.
>
What is the motivation behind adding this new dts? We have been using the
userspace tool [1] for applying this as an overlay for some time. But if we
start adding dts for mezzanines then for sure we'll end up with some good
numbers which will flood arch/{..}/qcom directory.
I could understand that one of the motivation is to provide nice user experience
to users but that's also taken care by the dt-update tool IMO.
Thanks,
Mani
[1] https://github.com/96boards/dt-update
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>
> Changes since v2:
> - Vinod: Change copyright assignment to Linaro
>
> Changes since v1:
> - Vinod: Changed license to GPL+BSD
> - Vinod: Changed copyright year to 2020
> - Nico: Changed name of mezzanine to d3camera
>
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> .../boot/dts/qcom/apq8016-sbc-d3camera.dts | 45 +++++++++++++++++++
> 2 files changed, 46 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/apq8016-sbc-d3camera.dts
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index cc103f7020fd..3f95b522694e 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3camera.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8096-ifc6640.dtb
> dtb-$(CONFIG_ARCH_QCOM) += ipq6018-cp01-c1.dtb
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc-d3camera.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3camera.dts
> new file mode 100644
> index 000000000000..752e5ec47499
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3camera.dts
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include "apq8016-sbc.dtsi"
> +
> +/ {
> + model = "Qualcomm Technologies, Inc. APQ 8016 SBC w/ D3Camera Mezzanine";
> + compatible = "qcom,apq8016-sbc", "qcom,apq8016", "qcom,sbc";
> +};
> +
> +&cci_i2c0 {
> + /delete-node/ camera_rear@3b;
> +
> + camera_rear@76 {
> + compatible = "ovti,ov5640";
> + reg = <0x76>;
> +
> + enable-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&camera_rear_default>;
> +
> + clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
> + clock-names = "xclk";
> + clock-frequency = <23880000>;
> +
> + vdddo-supply = <&camera_vdddo_1v8>;
> + vdda-supply = <&camera_vdda_2v8>;
> + vddd-supply = <&camera_vddd_1v5>;
> +
> + status = "ok";
> +
> + port {
> + ov5640_ep: endpoint {
> + clock-lanes = <1>;
> + data-lanes = <0 2>;
> + remote-endpoint = <&csiphy0_ep>;
> + };
> + };
> + };
> +};
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH 01/10] spi: dw: Add support for polled operation via no IRQ specified in DT
From: Lars Povlsen @ 2020-05-19 10:21 UTC (permalink / raw)
To: Mark Brown
Cc: SoC Team, Microchip Linux Driver Support, linux-spi, devicetree,
linux-kernel, linux-arm-kernel, Alexandre Belloni
In-Reply-To: <20200513143753.GI4803@sirena.org.uk>
On 13/05/20 15:37, Mark Brown wrote:
> Date: Wed, 13 May 2020 15:37:53 +0100
> From: Mark Brown <broonie@kernel.org>
> To: Lars Povlsen <lars.povlsen@microchip.com>
> Cc: SoC Team <soc@kernel.org>, Microchip Linux Driver Support
> <UNGLinuxDriver@microchip.com>, linux-spi@vger.kernel.org,
> devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-arm-kernel@lists.infradead.org, Alexandre Belloni
> <alexandre.belloni@bootlin.com>
> Subject: Re: [PATCH 01/10] spi: dw: Add support for polled operation via no
> IRQ specified in DT
> User-Agent: Mutt/1.10.1 (2018-07-13)
>
> On Wed, May 13, 2020 at 04:00:22PM +0200, Lars Povlsen wrote:
>
> > +#define VALID_IRQ(i) (i >= 0)
> > +
>
> This isn't something that should be defined by an individual driver, it
> should be in a generic header.
Thanks, I will work with Serge on getting this integrated right.
---Lars
^ permalink raw reply
* Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support
From: Jonathan Albrieux @ 2020-05-19 10:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Thomas Gleixner
In-Reply-To: <20200519100815.GA1634618@smile.fi.intel.com>
On Tue, May 19, 2020 at 01:08:15PM +0300, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 11:48:35AM +0200, Jonathan Albrieux wrote:
> > On Tue, May 19, 2020 at 12:22:12PM +0300, Andy Shevchenko wrote:
> > > On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
>
> ...
>
> > > I dunno if it's your first submission to Linux kernel project or other OSS,
> > > but here you missed a changelog. Rule of thumb is to provide a summary of
> > > the changes done in the history of the evolution of a patch series.
> > >
> >
> > Oh thank you and sorry for not having included it.
> >
> > Does the changelog needs to be added to all patch files or just on the ones
> > subject of the changes?
>
> Up to you and maintainer of the corresponding subsystem.
>
> My common sense tells me that
> 1) if there is a cover letter, just put a joined changelog there
> 2) otherwise, put changelog in each patch.
>
> I saw in practice all possible variants, i.e.
> a) cover letter w/o changelog + changelog per patch;
> b) cover letter w/ changelog + changelog per patch;
> c) cover letter w/ changelog.
>
> I think any of it is fine in general.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thank you for the tips, will work on it!
Best regards,
Jonathan Albrieux
^ permalink raw reply
* Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support
From: Andy Shevchenko @ 2020-05-19 10:08 UTC (permalink / raw)
To: Jonathan Albrieux
Cc: linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Thomas Gleixner
In-Reply-To: <20200519094835.GB10391@ict14-OptiPlex-980>
On Tue, May 19, 2020 at 11:48:35AM +0200, Jonathan Albrieux wrote:
> On Tue, May 19, 2020 at 12:22:12PM +0300, Andy Shevchenko wrote:
> > On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
...
> > I dunno if it's your first submission to Linux kernel project or other OSS,
> > but here you missed a changelog. Rule of thumb is to provide a summary of
> > the changes done in the history of the evolution of a patch series.
> >
>
> Oh thank you and sorry for not having included it.
>
> Does the changelog needs to be added to all patch files or just on the ones
> subject of the changes?
Up to you and maintainer of the corresponding subsystem.
My common sense tells me that
1) if there is a cover letter, just put a joined changelog there
2) otherwise, put changelog in each patch.
I saw in practice all possible variants, i.e.
a) cover letter w/o changelog + changelog per patch;
b) cover letter w/ changelog + changelog per patch;
c) cover letter w/ changelog.
I think any of it is fine in general.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ARM: dts: r9a06g032: Correct GIC compatible value order
From: André Przywara @ 2020-05-19 10:05 UTC (permalink / raw)
To: Geert Uytterhoeven, Magnus Damm
Cc: linux-renesas-soc, linux-arm-kernel, devicetree
In-Reply-To: <20200519095431.5650-1-geert+renesas@glider.be>
On 19/05/2020 10:54, Geert Uytterhoeven wrote:
> According to commit 61efb56e30f1c54e ("dt-bindings: arm: gic: Allow
> combining arm,gic-400 compatible strings"), "arm,gic-400" should be
> listed first.
Thanks for taking care!
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> Aforementioned commit is in robh/for-next.
> ---
> arch/arm/boot/dts/r9a06g032.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
> index 4c1ab49c7d39aeed..ee59cc84f2121488 100644
> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -174,7 +174,7 @@
> };
>
> gic: interrupt-controller@44101000 {
> - compatible = "arm,cortex-a7-gic", "arm,gic-400";
> + compatible = "arm,gic-400", "arm,cortex-a7-gic";
> interrupt-controller;
> #interrupt-cells = <3>;
> reg = <0x44101000 0x1000>, /* Distributer */
>
^ permalink raw reply
* [PATCH] ARM: dts: r9a06g032: Correct GIC compatible value order
From: Geert Uytterhoeven @ 2020-05-19 9:54 UTC (permalink / raw)
To: Magnus Damm
Cc: André Przywara, linux-renesas-soc, linux-arm-kernel,
devicetree, Geert Uytterhoeven
According to commit 61efb56e30f1c54e ("dt-bindings: arm: gic: Allow
combining arm,gic-400 compatible strings"), "arm,gic-400" should be
listed first.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Aforementioned commit is in robh/for-next.
---
arch/arm/boot/dts/r9a06g032.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 4c1ab49c7d39aeed..ee59cc84f2121488 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -174,7 +174,7 @@
};
gic: interrupt-controller@44101000 {
- compatible = "arm,cortex-a7-gic", "arm,gic-400";
+ compatible = "arm,gic-400", "arm,cortex-a7-gic";
interrupt-controller;
#interrupt-cells = <3>;
reg = <0x44101000 0x1000>, /* Distributer */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support
From: Jonathan Albrieux @ 2020-05-19 9:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Thomas Gleixner
In-Reply-To: <20200519092212.GT1634618@smile.fi.intel.com>
On Tue, May 19, 2020 at 12:22:12PM +0300, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
> > Convert documentation from txt format to yaml. Add documentation about
> > reset-gpio.
>
> Trailing white space (though it doesn't matter here, in cover letter).
>
Ok will fix it!
> > Deassert reset on ak8975_power_on, assert reset on ak8975_power_off.
> >
> > Without reset's deassertion during ak8975_power_on, driver's probe fails
> > on ak8975_who_i_am while checking for device identity for AK09911 chip
> >
> > AK09911 has an active low reset gpio to handle register's reset.
> > AK09911 datasheed says that, if not used, reset pin should be connected
> > to VID. This patch emulates this situation
>
> I dunno if it's your first submission to Linux kernel project or other OSS,
> but here you missed a changelog. Rule of thumb is to provide a summary of
> the changes done in the history of the evolution of a patch series.
>
Oh thank you and sorry for not having included it.
Does the changelog needs to be added to all patch files or just on the ones
subject of the changes?
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best regards,
Jonathan Albrieux
^ permalink raw reply
* Re: [PATCH v2 3/3] iio: magnetometer: ak8975: Add gpio reset support
From: Jonathan Albrieux @ 2020-05-19 9:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Thomas Gleixner, Jonathan Cameron, Jilayne Lovejoy
In-Reply-To: <20200519092127.GS1634618@smile.fi.intel.com>
On Tue, May 19, 2020 at 12:21:27PM +0300, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 08:57:43AM +0200, Jonathan Albrieux wrote:
>
> Thanks for an update, my comments below.
>
Thanks to you for taking time to review the code!
> > According to AK09911 datasheet, if reset gpio is provided then
>
> Trailing whitespace.
>
> > deassert reset on ak8975_power_on and assert reset on ak8975_power_off.
> >
> > Without reset's deassertion during ak8975_power_on, driver's probe fails on
> > ak8975_who_i_am while checking for device identity for AK09911 chip
> >
> > AK09911 has an active low reset gpio to handle register's reset.
> > AK09911 datasheed says that, if not used, reset pin should be connected
> > to VID. This patch emulates this situation
>
> Please, put periods at the end of the phrases.
>
> > /*
> > - * According to the datasheet the power supply rise time i 200us
> > + * According to the datasheet the power supply rise time is 200us
> > * and the minimum wait time before mode setting is 100us, in
> > - * total 300 us. Add some margin and say minimum 500us here.
> > + * total 300us. Add some margin and say minimum 500us here.
> > */
>
> This is not related change (perhaps, you may do in separate patch), but I'm not
> against it. Up to maintainer.
>
Ok I will separate the patch
> > + /*
> > + * According to AK09911 datasheet, if reset gpio is provided then
> > + * deassert reset on ak8975_power_on and assert reset on
> > + * ak8975_power_off.
>
> We try to refer to the code accordingly to the kernel doc recommendations (even
> for non-kdoc comments). So, here, 'function' as 'function()' (without quotes)
> and so on.
>
Will fix this as like as the other changes you pointed out about periods and
trailing spaces as soon as possible
> > + */
> > + reset_gpiod = devm_gpiod_get_optional(&client->dev,
> > + "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(reset_gpiod))
> > + return PTR_ERR(reset_gpiod);
> > +
> > /* Register with IIO */
> > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > if (indio_dev == NULL)
> > @@ -866,6 +883,7 @@ static int ak8975_probe(struct i2c_client *client,
> >
> > data->client = client;
> > data->eoc_gpiod = eoc_gpiod;
> > + data->reset_gpiod = reset_gpiod;
> > data->eoc_irq = 0;
> >
> > err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
> > --
> > 2.17.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Again, thank you,
Best regards,
Jonathan Albrieux
^ permalink raw reply
* Re: [PATCH 07/10] spi: spi-dw-mchp: Add Sparx5 support
From: Lars Povlsen @ 2020-05-19 9:29 UTC (permalink / raw)
To: Mark Brown
Cc: SoC Team, Microchip Linux Driver Support, linux-spi, devicetree,
linux-kernel, linux-arm-kernel, Alexandre Belloni
In-Reply-To: <20200514102516.GD5127@sirena.org.uk>
[Sorry about the slight delay on getting back on this]
On 14/05/20 11:25, Mark Brown wrote:
> Date: Thu, 14 May 2020 11:25:16 +0100
> From: Mark Brown <broonie@kernel.org>
> To: Lars Povlsen <lars.povlsen@microchip.com>
> Cc: SoC Team <soc@kernel.org>, Microchip Linux Driver Support
> <UNGLinuxDriver@microchip.com>, linux-spi@vger.kernel.org,
> devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-arm-kernel@lists.infradead.org, Alexandre Belloni
> <alexandre.belloni@bootlin.com>
> Subject: Re: [PATCH 07/10] spi: spi-dw-mchp: Add Sparx5 support
> User-Agent: Mutt/1.10.1 (2018-07-13)
>
> On Wed, May 13, 2020 at 04:00:28PM +0200, Lars Povlsen wrote:
>
> > +static void dw_spi_mchp_set_cs_owner(struct dw_spi_mchp *dwsmchp,
> > + const struct dw_spi_mchp_props *props,
> > + u8 cs, u8 owner)
> > {
> > + u8 dummy = (owner == MSCC_IF_SI_OWNER_SIBM ?
> > + MSCC_IF_SI_OWNER_SIMC : MSCC_IF_SI_OWNER_SIBM);
>
> Please write normal conditional statements to improve legibility.
>
I will take your recommendation to heart.
> > +static void dw_spi_mchp_set_cs(struct spi_device *spi, bool nEnable)
> > +{
> > + bool enable = !nEnable; /* This keeps changing in the API... */
>
> No, it doesn't. The API has not changed for more than a decade.
>
I will remove the comment.
I think the comment was related to when we got bitten by the below
change, but alas.
commit ada9e3fcc175db4538f5b5e05abf5dedf626e550
Author: Charles Keepax <ckeepax@opensource.cirrus.com>
Date: Wed Nov 27 15:39:36 2019 +0000
spi: dw: Correct handling of native chipselect
This patch reverts commit 6e0a32d6f376 ("spi: dw: Fix default polarity
of native chipselect").
> > + } else if (props->ss_force_ena_off) {
> > + if (enable) {
> > + /* Ensure CS toggles, so start off all disabled */
> > + regmap_write(dwsmchp->syscon, props->ss_force_val_off,
> > + ~0);
>
> What's all this force_ena_off stuff about? The controller should not be
> making decisions about management of the chip select, this will break
> users.
>
Our controller is not using DMA, but the FIFO interface. And as the DW
controller drops CS when the FIFO runs empty, this will upset SPI
devices. The "ss_force" is something the HW designes put on top to
"override" the CS. We could of course use the GPIO's specifically to
overcome this - but the "boot" CS 0 is a builtin CS, with no
underlying GPIO.
Add to this that the HW dept decided to add *2* physical SPI busses to
the same controller. That we also need to switch between. And ensure
CS gets dropped correctly before changing tracks...
Long story, lot of grief...
> > + if (pdev->dev.of_node) {
> > + int i;
> > +
> > + for (i = 0; i < dws->num_cs; i++) {
> > + int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
> > + "cs-gpios", i);
> > +
> > + if (cs_gpio == -EPROBE_DEFER) {
> > + ret = cs_gpio;
> > + goto out;
> > + }
> > +
> > + if (gpio_is_valid(cs_gpio)) {
> > + ret = devm_gpio_request(&pdev->dev, cs_gpio,
> > + dev_name(&pdev->dev));
> > + if (ret)
> > + goto out;
>
> Set use_gpio_descriptors and let the core manage the GPIO.
Good suggestion, just the ticket!
And thank you very much for your time & comments.
---Lars
^ permalink raw reply
* Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support
From: Andy Shevchenko @ 2020-05-19 9:22 UTC (permalink / raw)
To: Jonathan Albrieux
Cc: linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Thomas Gleixner
In-Reply-To: <20200519065749.4624-1-jonathan.albrieux@gmail.com>
On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
> Convert documentation from txt format to yaml. Add documentation about
> reset-gpio.
Trailing white space (though it doesn't matter here, in cover letter).
> Deassert reset on ak8975_power_on, assert reset on ak8975_power_off.
>
> Without reset's deassertion during ak8975_power_on, driver's probe fails
> on ak8975_who_i_am while checking for device identity for AK09911 chip
>
> AK09911 has an active low reset gpio to handle register's reset.
> AK09911 datasheed says that, if not used, reset pin should be connected
> to VID. This patch emulates this situation
I dunno if it's your first submission to Linux kernel project or other OSS,
but here you missed a changelog. Rule of thumb is to provide a summary of
the changes done in the history of the evolution of a patch series.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 3/3] iio: magnetometer: ak8975: Add gpio reset support
From: Andy Shevchenko @ 2020-05-19 9:21 UTC (permalink / raw)
To: Jonathan Albrieux
Cc: linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
Lars-Peter Clausen, Linus Walleij,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Thomas Gleixner, Jonathan Cameron, Jilayne Lovejoy
In-Reply-To: <20200519065749.4624-4-jonathan.albrieux@gmail.com>
On Tue, May 19, 2020 at 08:57:43AM +0200, Jonathan Albrieux wrote:
Thanks for an update, my comments below.
> According to AK09911 datasheet, if reset gpio is provided then
Trailing whitespace.
> deassert reset on ak8975_power_on and assert reset on ak8975_power_off.
>
> Without reset's deassertion during ak8975_power_on, driver's probe fails on
> ak8975_who_i_am while checking for device identity for AK09911 chip
>
> AK09911 has an active low reset gpio to handle register's reset.
> AK09911 datasheed says that, if not used, reset pin should be connected
> to VID. This patch emulates this situation
Please, put periods at the end of the phrases.
> /*
> - * According to the datasheet the power supply rise time i 200us
> + * According to the datasheet the power supply rise time is 200us
> * and the minimum wait time before mode setting is 100us, in
> - * total 300 us. Add some margin and say minimum 500us here.
> + * total 300us. Add some margin and say minimum 500us here.
> */
This is not related change (perhaps, you may do in separate patch), but I'm not
against it. Up to maintainer.
> + /*
> + * According to AK09911 datasheet, if reset gpio is provided then
> + * deassert reset on ak8975_power_on and assert reset on
> + * ak8975_power_off.
We try to refer to the code accordingly to the kernel doc recommendations (even
for non-kdoc comments). So, here, 'function' as 'function()' (without quotes)
and so on.
> + */
> + reset_gpiod = devm_gpiod_get_optional(&client->dev,
> + "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset_gpiod))
> + return PTR_ERR(reset_gpiod);
> +
> /* Register with IIO */
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (indio_dev == NULL)
> @@ -866,6 +883,7 @@ static int ak8975_probe(struct i2c_client *client,
>
> data->client = client;
> data->eoc_gpiod = eoc_gpiod;
> + data->reset_gpiod = reset_gpiod;
> data->eoc_irq = 0;
>
> err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 01/20] dt-bindings: arm: gic: Allow combining arm,gic-400 compatible strings
From: André Przywara @ 2020-05-19 9:19 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rob Herring, Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Mark Rutland, Marc Zyngier
In-Reply-To: <CAMuHMdXkHF3ioOVzDZQARiO2i1z8rVjdN_Q0VggaVD4Ln7J+Dw@mail.gmail.com>
On 19/05/2020 08:39, Geert Uytterhoeven wrote:
Hi Geert,
> On Wed, May 13, 2020 at 12:31 PM Andre Przywara <andre.przywara@arm.com> wrote:
>> The arm,gic-400 compatible is probably the best matching string for the
>> GIC in most modern SoCs, but was only introduced later into the kernel.
>> For historic reasons and to keep compatibility, some SoC DTs were thus
>> using a combination of this name and one of the older strings, which
>> currently the binding denies.
>>
>> Add a stanza to the DT binding to allow "arm,gic-400", followed by
>> either "arm,cortex-a15-gic" or "arm,cortex-a7-gic". This fixes binding
>> compliance for quite some SoC .dtsi files in the kernel tree.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
> Thanks for your patch, I was just looking into this issue ;-)
>
>> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
>> @@ -39,6 +39,12 @@ properties:
>> - qcom,msm-8660-qgic
>> - qcom,msm-qgic2
>>
>> + - items:
>> + - const: arm,gic-400
>> + - enum:
>> + - arm,cortex-a15-gic
>> + - arm,cortex-a7-gic
>> +
>> - items:
>> - const: arm,arm1176jzf-devchip-gic
>> - const: arm,arm11mp-gic
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r9a06g032.dtsi#n177
> has them in the other order.
>
> What do you think is the preferred solution: reverting the order, or dropping
> one or the other?
Reverting the order would be the right thing. Theoretically this might
change what the drivers match against, but there should be no difference
between those strings anyway. And certainly Linux does not care which of
the many strings it sees.
The proper order is not really obvious here, but the cortex-a{15,7}-gic
names serve as the missing "arm,gic-v2" generic fallback string here, I
think just for historical reasons.
Cheers,
Andre.
^ permalink raw reply
* Re: [PATCH v2 1/2] arm64: dts: rockchip: fix defines in pd_vio node for rk3399
From: Heiko Stuebner @ 2020-05-19 9:14 UTC (permalink / raw)
To: Johan Jonker
Cc: Heiko Stuebner, linux-arm-kernel, robh+dt, devicetree,
linux-rockchip, linux-kernel
In-Reply-To: <20200428203003.3318-1-jbx6244@gmail.com>
On Tue, 28 Apr 2020 22:30:02 +0200, Johan Jonker wrote:
> A test with the command below gives for example this error:
>
> arch/arm64/boot/dts/rockchip/rk3399-evb.dt.yaml: pd_vio@15:
> 'pd_tcpc0@RK3399_PD_TCPC0', 'pd_tcpc1@RK3399_PD_TCPC1'
> do not match any of the regexes:
> '.*-names$', '.*-supply$', '^#.*-cells$',
> '^#[a-zA-Z0-9,+\\-._]{0,63}$',
> '^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}$',
> '^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}@[0-9a-fA-F]+(,[0-9a-fA-F]+)*$',
> '^__.*__$', 'pinctrl-[0-9]+'
>
> [...]
Applied, thanks!
[1/1] arm64: dts: rockchip: fix pd_tcpc0 and pd_tcpc1 node position on rk3399
commit: 2b99e6196663199409540fb95798dba464e34343
Best regards,
--
Heiko Stuebner <heiko@sntech.de>
^ permalink raw reply
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Sakari Ailus @ 2020-05-19 9:03 UTC (permalink / raw)
To: Kieran Bingham
Cc: Niklas Söderlund, linux-renesas-soc, linux-media, devicetree,
linux-kernel, Mauro Carvalho Chehab, Kieran Bingham,
Laurent Pinchart, Jacopo Mondi, Niklas Söderlund,
Hans Verkuil, Hyun Kwon, Manivannan Sadhasivam, Rob Herring,
Jacopo Mondi, Laurent Pinchart
In-Reply-To: <8932699a-b321-2308-8903-31268af774cb@ideasonboard.com>
Hi Kieran,
On Tue, May 19, 2020 at 09:55:24AM +0100, Kieran Bingham wrote:
> Hi Sakari,
>
> On 19/05/2020 09:10, Sakari Ailus wrote:
> > Hi Kieran,
> >
> > On Mon, May 18, 2020 at 12:45:18PM +0100, Kieran Bingham wrote:
> >> Hi Sakari,
> >>
> >> There are only fairly minor comments here, fix ups will be included in a
> >> v10.
> >>
> >> Is there anything major blocking integration?
> >
> > Not that I can see. But please see my comments below.
>
> Thanks,
>
> We might have some more work tidying up the DT validation anyway which
> has come too late, and perhaps is going to bump this to v5.9 now anyway.
>
> I can still try but ... ;-S
>
> At least hopefully now we /can/ see a path to integration though.
>
> I probably don't care if it's 5.8 or 5.9 as long as it's not 8.5 ;-)
>
> >>
> >> Regards
> >>
> >> Kieran
> >>
> >>
> >>
> >> On 16/05/2020 22:51, Sakari Ailus wrote:
> >>> Hi Kieran,
> >>>
> >>> Thanks for the update.
> >>>
> >>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
> >>>
> >>> ...
> >>>
> >>>> +static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >>>> + struct v4l2_subdev_pad_config *cfg,
> >>>> + struct v4l2_subdev_mbus_code_enum *code)
> >>>> +{
> >>>> + if (code->pad || code->index > 0)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> >>>
> >>> Why UYVY8_2X8 and not UYVY8_1X16? In general, the single sample / pixel
> >>> variant of the format is generally used on the serial busses. This choice
> >>> was made when serial busses were introduced.
> >>
> >> Ok - I presume this doesn't really have much effect anyway, they just
> >> have to match for the transmitter/receiver?
> >
> > In this case, yes. But it's harder to change later, so let's indeed do that
> > now.
>
> Yes indeed, I have to change my test scripts for the new configuration
> (or we should update the scripts to get the configuration from the
> device ;D)
>
>
> >> But it makes sense to me, so I'll update to the 1x16 variant.
> >
> > ...
>
> done anyway ;-)
>
> I see the ADV748x is using the 2x8 variants though ... (all the more
> reason for our scripts to /get/ the correct version when propagating
> formats).
>
> Perhaps I should/could add the 1x16 formats to the ADV748x too. (later)
It's a driver bug, yes.
>
>
> >>> And as you don't, you also won't know which frequencies are known to be
> >>> safe to use. That said, perhaps where this device is used having a random
> >>> frequency on that bus could not be an issue. Perhaps.
> >>
> >> Does this generate a range? or a list of static supported frequencies?
> >>
> >> We configure the pixel clock based upon the number of cameras connected,
> >> and their pixel rates etc ...
> >>
> >> Are you saying that the frequency of this clock should be validated to
> >> be a specific range? or are you talking about a different frequency?
> >
> > It depends on the system. In general, only frequencies known to be safe
> > should be used. If this one has enough shielding to guarantee there won't
> > be problems in using a random frequency in the entire range, is there a
> > guarantee that will be the case for all systems with this chip?
>
> I have no idea here... Maybe Niklas knows more having dealt more with
> the RCar-VIN/CSI parts.
>
> It seems like this is something we can add later if necessary, by
> extending the descriptions in the DT?
Works for me.
Niklas, any idea?
--
Regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Kieran Bingham @ 2020-05-19 8:55 UTC (permalink / raw)
To: Sakari Ailus, Niklas Söderlund
Cc: linux-renesas-soc, linux-media, devicetree, linux-kernel,
Mauro Carvalho Chehab, Kieran Bingham, Laurent Pinchart,
Jacopo Mondi, Niklas Söderlund, Hans Verkuil, Hyun Kwon,
Manivannan Sadhasivam, Rob Herring, Jacopo Mondi,
Laurent Pinchart
In-Reply-To: <20200519081019.GB3877@valkosipuli.retiisi.org.uk>
Hi Sakari,
On 19/05/2020 09:10, Sakari Ailus wrote:
> Hi Kieran,
>
> On Mon, May 18, 2020 at 12:45:18PM +0100, Kieran Bingham wrote:
>> Hi Sakari,
>>
>> There are only fairly minor comments here, fix ups will be included in a
>> v10.
>>
>> Is there anything major blocking integration?
>
> Not that I can see. But please see my comments below.
Thanks,
We might have some more work tidying up the DT validation anyway which
has come too late, and perhaps is going to bump this to v5.9 now anyway.
I can still try but ... ;-S
At least hopefully now we /can/ see a path to integration though.
I probably don't care if it's 5.8 or 5.9 as long as it's not 8.5 ;-)
>>
>> Regards
>>
>> Kieran
>>
>>
>>
>> On 16/05/2020 22:51, Sakari Ailus wrote:
>>> Hi Kieran,
>>>
>>> Thanks for the update.
>>>
>>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
>>>
>>> ...
>>>
>>>> +static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> + if (code->pad || code->index > 0)
>>>> + return -EINVAL;
>>>> +
>>>> + code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>>>
>>> Why UYVY8_2X8 and not UYVY8_1X16? In general, the single sample / pixel
>>> variant of the format is generally used on the serial busses. This choice
>>> was made when serial busses were introduced.
>>
>> Ok - I presume this doesn't really have much effect anyway, they just
>> have to match for the transmitter/receiver?
>
> In this case, yes. But it's harder to change later, so let's indeed do that
> now.
Yes indeed, I have to change my test scripts for the new configuration
(or we should update the scripts to get the configuration from the
device ;D)
>> But it makes sense to me, so I'll update to the 1x16 variant.
>
> ...
done anyway ;-)
I see the ADV748x is using the 2x8 variants though ... (all the more
reason for our scripts to /get/ the correct version when propagating
formats).
Perhaps I should/could add the 1x16 formats to the ADV748x too. (later)
>>> And as you don't, you also won't know which frequencies are known to be
>>> safe to use. That said, perhaps where this device is used having a random
>>> frequency on that bus could not be an issue. Perhaps.
>>
>> Does this generate a range? or a list of static supported frequencies?
>>
>> We configure the pixel clock based upon the number of cameras connected,
>> and their pixel rates etc ...
>>
>> Are you saying that the frequency of this clock should be validated to
>> be a specific range? or are you talking about a different frequency?
>
> It depends on the system. In general, only frequencies known to be safe
> should be used. If this one has enough shielding to guarantee there won't
> be problems in using a random frequency in the entire range, is there a
> guarantee that will be the case for all systems with this chip?
I have no idea here... Maybe Niklas knows more having dealt more with
the RCar-VIN/CSI parts.
It seems like this is something we can add later if necessary, by
extending the descriptions in the DT?
--
Kieran
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox