Devicetree
 help / color / mirror / Atom feed
* [PATCH] drm/panel: simple: Add Sharp LQ035Q7DB03 panel support
From: Vladimir Zapolskiy @ 2018-05-21 23:23 UTC (permalink / raw)
  To: Thierry Reding, Rob Herring; +Cc: David Airlie, devicetree, dri-devel

The change adds support for Sharp LQ035Q7DB03 3.5" QVGA panel.

Note that this aged panel is already found in the kernel sources,
for instance in board mach files mach-mx21ads.c, mach-mx27ads.c,
mach-pcm043.c, lpd270.c and imx27-phytec-phycore-rdk.dts.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 .../bindings/display/panel/sharp,lq035q7db03.txt   |  7 ++++++
 drivers/gpu/drm/panel/panel-simple.c               | 27 ++++++++++++++++++++++
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt

diff --git a/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt b/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt
new file mode 100644
index 0000000..42027e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt
@@ -0,0 +1,7 @@
+Sharp LQ035Q7DB03 3.5" QVGA TFT LCD panel
+
+Required properties:
+- compatible: should be "sharp,lq035q7db03"
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index cbf1ab4..8970261 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1764,6 +1764,30 @@ static const struct panel_desc samsung_ltn140at29_301 = {
 	},
 };
 
+static const struct drm_display_mode sharp_lq035q7db03_mode = {
+	.clock = 5500,
+	.hdisplay = 240,
+	.hsync_start = 240 + 16,
+	.hsync_end = 240 + 16 + 7,
+	.htotal = 240 + 16 + 7 + 5,
+	.vdisplay = 320,
+	.vsync_start = 320 + 9,
+	.vsync_end = 320 + 9 + 1,
+	.vtotal = 320 + 9 + 1 + 7,
+	.vrefresh = 60,
+};
+
+static const struct panel_desc sharp_lq035q7db03 = {
+	.modes = &sharp_lq035q7db03_mode,
+	.num_modes = 1,
+	.bpc = 6,
+	.size = {
+		.width = 54,
+		.height = 72,
+	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
 static const struct display_timing sharp_lq101k1ly04_timing = {
 	.pixelclock = { 60000000, 65000000, 80000000 },
 	.hactive = { 1280, 1280, 1280 },
@@ -2236,6 +2260,9 @@ static const struct of_device_id platform_of_match[] = {
 		.compatible = "samsung,ltn140at29-301",
 		.data = &samsung_ltn140at29_301,
 	}, {
+		.compatible = "sharp,lq035q7db03",
+		.data = &sharp_lq035q7db03,
+	}, {
 		.compatible = "sharp,lq101k1ly04",
 		.data = &sharp_lq101k1ly04,
 	}, {
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Andy Shevchenko @ 2018-05-21 23:05 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Jacek Anaszewski, Rob Herring, Mark Rutland, devicetree,
	Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <3b20c9cf-5f7e-3ed9-454d-c8e8ca6c0627@ti.com>

On Tue, May 22, 2018 at 12:44 AM, Dan Murphy <dmurphy@ti.com> wrote:


>>> +    child = device_get_next_child_node(&led->client->dev, child);
>>> +    if (!child) {
>>> +        dev_err(&led->client->dev, "No LED Child node\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
>>> +    if (ret) {
>>> +        dev_err(&led->client->dev, "reg DT property missing\n");
>>> +        goto out_err;
>>> +    }
>>> +
>>> +    if (led->led_mode > LM3601X_LED_TORCH ||
>>> +        led->led_mode < LM3601X_LED_IR) {
>>> +        dev_warn(&led->client->dev, "Invalid led mode requested\n");
>>> +        ret = -EINVAL;
>>> +        goto out_err;
>>> +    }
>>> +
>>> +    ret = fwnode_property_read_string(child, "label", &name);
>>> +    if (ret) {
>>> +        if (led->led_mode == LM3601X_LED_TORCH)
>>> +            name = "torch";
>>> +        else
>>> +            name = "infrared";
>>> +    }
>>> +
>>> +    snprintf(led->led_name, sizeof(led->led_name),
>>> +        "%s:%s", node->name, name);
>>
>> Reading once again my recent explanation regarding this I realized
>> that I didn't provide clear conclusion, which is: we no longer
>> use child node name for LED class device name if label is absent.
>> (apart from that - you're using parent DT node now, i.e.
>> led-controller).
>>
>> Please follow what was done for drivers/leds/leds-cr0014114.c.
>
> Hmmm.  If this is calling dev->of_node->name to store the name will this
> work in non-DT configurations?

I didn't found this kind of use in linux-next, perhaps I missed something?

In the driver Jacek referred to I found, though, use of of_node, which
at some point should be changed to fwnode.

For now you can fill that if you want to using something like this
(IIRC it should work):

if (is_of_node(fwnode))
 ...->of_node = to_of_node(...);

> I have not dug to deeply into the fwnode code to find out how the nodes
> get populated.  So my question may not even be valid.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* [PATCH v2 0/4] PCI: cadence: Host and EP driver updates for PHY and power management
From: Alan Douglas @ 2018-05-21 22:58 UTC (permalink / raw)
  To: bhelgaas@google.com, kishon@ti.com, lorenzo.pieralisi@arm.com
  Cc: cyrille.pitchen@free-electrons.com, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	nsekhar@ti.com

This is a series of patches for the cadence PCIe host and EP drivers, to:
 - Add optional list of generic PHYs to host and EP drivers
 - Add PHY bindings to devicetree
 - Add Power Management ops, which will enable/disable PHYs if present
- Update cdns_pcie_writel function signature

Changes in v2:
    Split commit into four patches
    Re-based on v4.17-rc1

Alan Douglas (4):
  PCI: cadence: Update cdns_pcie_writel function signature
  PCI: cadence: Add generic PHY support to host and EP drivers
  dt-bindings: PCI: cadence: Add DT bindings for optional PHYs
  PCI: cadence: Add Power Management ops for host and EP

 .../devicetree/bindings/pci/cdns,cdns-pcie-ep.txt  |   4 +
 .../bindings/pci/cdns,cdns-pcie-host.txt           |   2 +
 drivers/pci/cadence/pcie-cadence-ep.c              |  15 ++-
 drivers/pci/cadence/pcie-cadence-host.c            |  34 ++++++
 drivers/pci/cadence/pcie-cadence.c                 | 123 +++++++++++++++++++++
 drivers/pci/cadence/pcie-cadence.h                 |  13 ++-
 6 files changed, 189 insertions(+), 2 deletions(-)

--

^ permalink raw reply

* [PATCH 2/2] ARM: pxa: dts: add pin definitions for extended GPIOs
From: Daniel Mack @ 2018-05-21 22:00 UTC (permalink / raw)
  To: robert.jarzmik, haojian.zhuang
  Cc: devicetree, robh+dt, Daniel Mack, linux-arm-kernel
In-Reply-To: <20180521220044.821-1-daniel@zonque.org>

The PXA3xx series features some extended GPIO banks which are named GPIO0_2,
GPIO1_2 etc. The PXA300, PXA310 and PXA320 have different numbers of such
pins, and they also have variant-specific register offsets.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 arch/arm/boot/dts/pxa3xx.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/pxa3xx.dtsi b/arch/arm/boot/dts/pxa3xx.dtsi
index a13ac52e4fd2..446e612688c0 100644
--- a/arch/arm/boot/dts/pxa3xx.dtsi
+++ b/arch/arm/boot/dts/pxa3xx.dtsi
@@ -8,6 +8,13 @@
 	 (gpio <= 98) ? (0x0400 + 4 * (gpio - 27)) :	\
 	 (gpio <= 127) ? (0x0600 + 4 * (gpio - 99)) :	\
 	 0)
+#define MFP_PIN_PXA300_0_2	0x674
+#define MFP_PIN_PXA300_1_2	0x678
+#define MFP_PIN_PXA300_2_2	0x2dc
+#define MFP_PIN_PXA300_3_2	0x2e0
+#define MFP_PIN_PXA300_4_2	0x2e4
+#define MFP_PIN_PXA300_5_2	0x2e8
+#define MFP_PIN_PXA300_6_2	0x2ec
 
 #define MFP_PIN_PXA310(gpio)				\
 	((gpio <= 2) ? (0x00b4 + 4 * gpio) :		\
@@ -18,6 +25,17 @@
 	 (gpio <= 262) ? 0 :				\
 	 (gpio <= 268) ? (0x052c + 4 * (gpio - 263)) :	\
 	 0)
+#define MFP_PIN_PXA310_0_2	0x674
+#define MFP_PIN_PXA310_1_2	0x678
+#define MFP_PIN_PXA310_2_2	0x2dc
+#define MFP_PIN_PXA310_3_2	0x2e0
+#define MFP_PIN_PXA310_4_2	0x2e4
+#define MFP_PIN_PXA310_5_2	0x2e8
+#define MFP_PIN_PXA310_6_2	0x2ec
+#define MFP_PIN_PXA310_7_2	0x52c
+#define MFP_PIN_PXA310_8_2	0x530
+#define MFP_PIN_PXA310_9_2	0x534
+#define MFP_PIN_PXA310_10_2	0x538
 
 #define MFP_PIN_PXA320(gpio)				\
 	((gpio <= 4) ? (0x0124 + 4 * gpio) :		\
@@ -30,6 +48,12 @@
 	 (gpio <= 98) ? (0x04f0 + 4 * (gpio - 74)) :	\
 	 (gpio <= 127) ? (0x0600 + 4 * (gpio - 99)) :	\
 	 0)
+#define MFP_PIN_PXA320_0_2	0x674
+#define MFP_PIN_PXA320_1_2	0x678
+#define MFP_PIN_PXA320_2_2	0x67c
+#define MFP_PIN_PXA320_3_2	0x680
+#define MFP_PIN_PXA320_4_2	0x284
+#define MFP_PIN_PXA320_5_2	0x28c
 
 /*
  * MFP Alternate functions for pins having a gpio.
-- 
2.14.3

^ permalink raw reply related

* [PATCH 1/2] ARM: pxa: dts: add gpio-ranges to gpio controller
From: Daniel Mack @ 2018-05-21 22:00 UTC (permalink / raw)
  To: robert.jarzmik, haojian.zhuang
  Cc: devicetree, robh+dt, Daniel Mack, linux-arm-kernel

The PXA GPIO driver calls out to the pinctrl driver for claiming pins
unless the config has CONFIG_PINCTRL unset. IOW, if a pinctrl driver is
active, it must be visible to the GPIO driver.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 arch/arm/boot/dts/pxa3xx.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/pxa3xx.dtsi b/arch/arm/boot/dts/pxa3xx.dtsi
index 982d1a62661d..a13ac52e4fd2 100644
--- a/arch/arm/boot/dts/pxa3xx.dtsi
+++ b/arch/arm/boot/dts/pxa3xx.dtsi
@@ -148,6 +148,7 @@
 			compatible = "intel,pxa3xx-gpio";
 			reg = <0x40e00000 0x10000>;
 			clocks = <&clks CLK_GPIO>;
+			gpio-ranges = <&pinctrl 0 0 128>;
 			interrupt-names = "gpio0", "gpio1", "gpio_mux";
 			interrupts = <8 9 10>;
 			gpio-controller;
-- 
2.14.3

^ permalink raw reply related

* [PATCH] dt-bindings: Add vendor prefix for Logic PD
From: Vladimir Zapolskiy @ 2018-05-21 21:59 UTC (permalink / raw)
  To: Rob Herring; +Cc: devicetree

Logic PD is an electronics design, engineering and manufacturing
company, which offers system on module and baseboard products.

Website: https://www.logicpd.com/

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index d63a218..b695d62 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -201,6 +201,7 @@ linaro	Linaro Limited
 linksys	Belkin International, Inc. (Linksys)
 linux	Linux-specific binding
 lltc	Linear Technology Corporation
+logicpd	Logic PD, Inc.
 lsi	LSI Corp. (LSI Logic)
 lwn	Liebherr-Werk Nenzing GmbH
 macnica	Macnica Americas
-- 
2.10.2


^ permalink raw reply related

* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Dan Murphy @ 2018-05-21 21:44 UTC (permalink / raw)
  To: Jacek Anaszewski, robh+dt, mark.rutland, andy.shevchenko
  Cc: devicetree, linux-kernel, linux-leds
In-Reply-To: <69a920da-f759-de50-865c-0ebc43044b66@gmail.com>

Jacek

Thanks for the review

On 05/21/2018 04:09 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> Thank you for the update.
> 
> On 05/21/2018 08:09 PM, Dan Murphy wrote:
>> Introduce the family of LED devices that can
>> drive a torch, strobe or IR LED.
>>
>> The LED driver can be configured with a strobe
>> timer to execute a strobe flash.  The IR LED
>> brightness is controlled via the torch brightness
>> register.
>>
>> The data sheet for each the LM36010 and LM36011
>> LED drivers can be found here:
>> http://www.ti.com/product/LM36010
>> http://www.ti.com/product/LM36011
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v7 - Numerous fixes to many to list.  Highlights are moved from OF APIs to device_property
>> APIs, updated LED registration, timeout to reg algorithim, and fixed label generation -
>> https://patchwork.kernel.org/patch/10401437/
>>
>> v6 - This driver has been heavily modified from v5.  There is no longer reading
>> of individual child nodes.  There are too many changes to list here see -
>> https://patchwork.kernel.org/patch/10392123/
>> v5 - Fixed magic numbers, change reg cache type, added of_put_node to release
>> the dt node ref, and I did not change the remove function to leave the LED in its
>> state on driver removal - https://patchwork.kernel.org/patch/10391741/
>> v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/
>> v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed
>> RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type,
>> fixed unlock and extra semi colon in strobe_set, removed unnecessary out label
>> in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/
>> v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/
>>
>>   drivers/leds/Kconfig        |   9 +
>>   drivers/leds/Makefile       |   1 +
>>   drivers/leds/leds-lm3601x.c | 501 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 511 insertions(+)
>>   create mode 100644 drivers/leds/leds-lm3601x.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 2c896c0e69e1..50ae536f343f 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -145,6 +145,15 @@ config LEDS_LM3692X
>>         This option enables support for the TI LM3692x family
>>         of white LED string drivers used for backlighting.
>>   +config LEDS_LM3601X
>> +    tristate "LED support for LM3601x Chips"
>> +    depends on LEDS_CLASS && I2C && OF
>> +    depends on LEDS_CLASS_FLASH
>> +    select REGMAP_I2C
>> +    help
>> +      This option enables support for the TI LM3601x family
>> +      of flash, torch and indicator classes.
>> +
>>   config LEDS_LOCOMO
>>       tristate "LED Support for Locomo device"
>>       depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 91eca81cae82..b79807fe1b67 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG)        += leds-mlxreg.o
>>   obj-$(CONFIG_LEDS_NIC78BX)        += leds-nic78bx.o
>>   obj-$(CONFIG_LEDS_MT6323)        += leds-mt6323.o
>>   obj-$(CONFIG_LEDS_LM3692X)        += leds-lm3692x.o
>> +obj-$(CONFIG_LEDS_LM3601X)        += leds-lm3601x.o
>>     # LED SPI Drivers
>>   obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c
>> new file mode 100644
>> index 000000000000..7c71cb11d474
>> --- /dev/null
>> +++ b/drivers/leds/leds-lm3601x.c
>> @@ -0,0 +1,501 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Flash and torch driver for Texas Instruments LM3601X LED
>> +// Flash driver chip family
>> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/leds.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <uapi/linux/uleds.h>
>> +
>> +#define LM3601X_LED_IR        0x0
>> +#define LM3601X_LED_TORCH    0x1
>> +
>> +/* Registers */
>> +#define LM3601X_ENABLE_REG    0x01
>> +#define LM3601X_CFG_REG        0x02
>> +#define LM3601X_LED_FLASH_REG    0x03
>> +#define LM3601X_LED_TORCH_REG    0x04
>> +#define LM3601X_FLAGS_REG    0x05
>> +#define LM3601X_DEV_ID_REG    0x06
>> +
>> +#define LM3601X_SW_RESET    BIT(7)
>> +
>> +/* Enable Mode bits */
>> +#define LM3601X_MODE_STANDBY    0x00
>> +#define LM3601X_MODE_IR_DRV    BIT(0)
>> +#define LM3601X_MODE_TORCH    BIT(1)
>> +#define LM3601X_MODE_STROBE    (BIT(0) | BIT(1))
>> +#define LM3601X_STRB_EN        BIT(2)
>> +#define LM3601X_STRB_EDGE_TRIG    BIT(3)
>> +#define LM3601X_IVFM_EN        BIT(4)
>> +
>> +#define LM36010_BOOST_LIMIT_28    BIT(5)
>> +#define LM36010_BOOST_FREQ_4MHZ    BIT(6)
>> +#define LM36010_BOOST_MODE_PASS    BIT(7)
>> +
>> +/* Flag Mask */
>> +#define LM3601X_FLASH_TIME_OUT    BIT(0)
>> +#define LM3601X_UVLO_FAULT    BIT(1)
>> +#define LM3601X_THERM_SHUTDOWN    BIT(2)
>> +#define LM3601X_THERM_CURR    BIT(3)
>> +#define LM36010_CURR_LIMIT    BIT(4)
>> +#define LM3601X_SHORT_FAULT    BIT(5)
>> +#define LM3601X_IVFM_TRIP    BIT(6)
>> +#define LM36010_OVP_FAULT    BIT(7)
>> +
>> +#define LM3601X_MAX_TORCH_I_UA    376000
>> +#define LM3601X_MIN_TORCH_I_UA    2400
>> +#define LM3601X_TORCH_REG_DIV    3000
>> +
>> +#define LM3601X_MAX_STROBE_I_UA    1500000
>> +#define LM3601X_MIN_STROBE_I_UA    11000
>> +#define LM3601X_STROBE_REG_DIV    11800
>> +
>> +#define LM3601X_TIMEOUT_MASK    0x1e
>> +#define LM3601X_ENABLE_MASK    0x03
>> +
>> +#define LM3601X_LOWER_STEP_US    40000
>> +#define LM3601X_UPPER_STEP_US    200000
>> +#define LM3601X_MIN_TIMEOUT_US    40000
>> +#define LM3601X_MAX_TIMEOUT_US    1600000
>> +#define LM3601X_TIMEOUT_XOVER    400000
>> +
>> +enum lm3601x_type {
>> +    CHIP_LM36010 = 0,
>> +    CHIP_LM36011,
>> +};
>> +
>> +/**
>> + * struct lm3601x_led -
>> + * @fled_cdev: flash led class device pointer
> 
> s/led/LED/

Ack

> 
>> + * @client: Pointer to the I2C client
>> + * @regmap: Devices register map
>> + * @lock: Lock for reading/writing the device
>> + * @led_name: LED label for the Torch or IR LED
>> + * @strobe_timeout: the timeout for the strobe
>> + * @last_flag: last known flags register value
>> + * @torch_current_max: maximum current for the torch
>> + * @strobe_current_max: maximum current for the strobe
>> + * @max_strobe_timeout: maximum timeout for the strobe
>> + * @led_mode: The mode to enable either IR or Torch
>> + */
>> +struct lm3601x_led {
>> +    struct led_classdev_flash fled_cdev;
>> +    struct i2c_client *client;
>> +    struct regmap *regmap;
>> +    struct mutex lock;
>> +
>> +    char led_name[LED_MAX_NAME_SIZE];
>> +
>> +    unsigned int strobe_timeout;
>> +    unsigned int last_flag;
>> +
>> +    u32 torch_current_max;
>> +    u32 strobe_current_max;
>> +    u32 max_strobe_timeout;
>> +
>> +    u32 led_mode;
>> +};
>> +
>> +static const struct reg_default lm3601x_regmap_defs[] = {
>> +    { LM3601X_ENABLE_REG, 0x20 },
>> +    { LM3601X_CFG_REG, 0x15 },
>> +    { LM3601X_LED_FLASH_REG, 0x00 },
>> +    { LM3601X_LED_TORCH_REG, 0x00 },
>> +};
>> +
>> +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +    switch (reg) {
>> +    case LM3601X_FLAGS_REG:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>> +static const struct regmap_config lm3601x_regmap = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +
>> +    .max_register = LM3601X_DEV_ID_REG,
>> +    .reg_defaults = lm3601x_regmap_defs,
>> +    .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs),
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = lm3601x_volatile_reg,
>> +};
>> +
>> +static struct lm3601x_led *fled_cdev_to_led(
>> +                struct led_classdev_flash *fled_cdev)
>> +{
>> +    return container_of(fled_cdev, struct lm3601x_led, fled_cdev);
>> +}
>> +
>> +static int lm3601x_read_faults(struct lm3601x_led *led)
>> +{
>> +    int flags_val;
>> +    int ret;
>> +
>> +    ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val);
>> +    if (ret < 0)
>> +        return -EIO;
>> +
>> +    led->last_flag = 0;
>> +
>> +    if (flags_val & LM36010_OVP_FAULT)
>> +        led->last_flag |= LED_FAULT_OVER_VOLTAGE;
>> +
>> +    if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR))
>> +        led->last_flag |= LED_FAULT_OVER_TEMPERATURE;
>> +
>> +    if (flags_val & LM3601X_SHORT_FAULT)
>> +        led->last_flag |= LED_FAULT_SHORT_CIRCUIT;
>> +
>> +    if (flags_val & LM36010_CURR_LIMIT)
>> +        led->last_flag |= LED_FAULT_OVER_CURRENT;
>> +
>> +    if (flags_val & LM3601X_UVLO_FAULT)
>> +        led->last_flag |= LED_FAULT_UNDER_VOLTAGE;
>> +
>> +    if (flags_val & LM3601X_IVFM_TRIP)
>> +        led->last_flag |= LED_FAULT_INPUT_VOLTAGE;
>> +
>> +    if (flags_val & LM3601X_THERM_SHUTDOWN)
>> +        led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE;
>> +
>> +    return led->last_flag;
>> +}
>> +
>> +static int lm3601x_brightness_set(struct led_classdev *cdev,
>> +                    enum led_brightness brightness)
>> +{
>> +    struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
>> +    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>> +    int ret, led_mode_val;
>> +    u8 brightness_val;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    ret = lm3601x_read_faults(led);
>> +    if (ret < 0)
>> +        goto out;
>> +
>> +    if (led->led_mode == LM3601X_LED_TORCH)
>> +        led_mode_val = LM3601X_MODE_TORCH;
>> +    else
>> +        led_mode_val = LM3601X_MODE_IR_DRV;
>> +
>> +    if (brightness == LED_OFF) {
>> +        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                    led_mode_val, LED_OFF);
>> +        goto out;
>> +    }
>> +
>> +    if (brightness == LED_ON)
>> +        brightness_val = LED_ON;
> 
> Use of LED_ON enum can be a bit misleading here,
> It was designed for LEDs that can be only turned on/off.
> 
> You can use DIV_ROUND_UP to achieve the same in a more clear way.

ACK

> 
>> +    else
>> +        brightness_val = brightness / LM3601X_TORCH_REG_DIV;
>> +
>> +    ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val);
>> +    if (ret < 0)
>> +        goto out;
>> +
>> +    ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
>> +                led_mode_val);
>> +out:
>> +    mutex_unlock(&led->lock);
>> +    return ret;
>> +}
>> +
>> +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev,
>> +                bool state)
>> +{
>> +    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>> +    int timeout_reg_val = 0;
>> +    int current_timeout;
>> +    int ret;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
>> +    if (ret < 0)
>> +        goto out;
>> +
>> +    if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER)
>> +        timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07;
>> +    else
>> +        timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01;
>> +
>> +    if (led->strobe_timeout != current_timeout)
>> +        ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG,
>> +                    LM3601X_TIMEOUT_MASK, timeout_reg_val);
>> +
>> +    if (state)
>> +        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                    LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
>> +                    LM3601X_MODE_STROBE);
>> +    else
>> +        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                    LM3601X_MODE_STROBE, LED_OFF);
>> +
>> +    ret = lm3601x_read_faults(led);
>> +out:
>> +    mutex_unlock(&led->lock);
>> +    return ret;
>> +}
>> +
>> +static int lm3601x_strobe_brightness_set(struct led_classdev_flash *fled_cdev,
>> +                    u32 brightness)
>> +{
>> +    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>> +    int ret;
>> +    u8 brightness_val;
>> +
>> +    mutex_lock(&led->lock);
>> +    ret = lm3601x_read_faults(led);
>> +    if (ret < 0)
>> +        goto out;
>> +
>> +    if (brightness == LED_OFF) {
>> +        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                    LM3601X_MODE_STROBE, LED_OFF);
>> +        goto out;
>> +    }
>> +
>> +    if (brightness == LED_ON)
>> +        brightness_val = LED_ON;
> 
> Ditto.

ACK

> 
>> +    else
>> +        brightness_val = brightness / LM3601X_STROBE_REG_DIV;
>> +
>> +    ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val);
>> +out:
>> +    mutex_unlock(&led->lock);
>> +    return ret;
>> +}
>> +
>> +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev,
>> +                u32 timeout)
>> +{
>> +    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>> +    int ret = 0;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    led->strobe_timeout = timeout;
>> +
>> +    mutex_unlock(&led->lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev,
>> +                bool *state)
>> +{
>> +    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>> +    int ret;
>> +    int strobe_state;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state);
>> +    if (ret < 0)
>> +        goto out;
>> +
>> +    *state = strobe_state & LM3601X_MODE_STROBE;
>> +
>> +out:
>> +    mutex_unlock(&led->lock);
>> +    return ret;
>> +}
>> +
>> +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev,
>> +                u32 *fault)
>> +{
>> +    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>> +
>> +    lm3601x_read_faults(led);
>> +
>> +    *fault = led->last_flag;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct led_flash_ops strobe_ops = {
>> +    .flash_brightness_set    = lm3601x_strobe_brightness_set,
> 
> s/strobe/flash/

ACK

> 
>> +    .strobe_set        = lm3601x_strobe_set,
>> +    .strobe_get        = lm3601x_strobe_get,
>> +    .timeout_set        = lm3601x_strobe_timeout_set,
> 
> Ditto.

ACK

> 
>> +    .fault_get        = lm3601x_strobe_fault_get,
> 
> Ditto.

ACK

> 
>> +};
>> +
>> +static int lm3601x_register_leds(struct lm3601x_led *led)
>> +{
>> +    struct led_classdev *led_cdev;
>> +    struct led_flash_setting *setting;
>> +
>> +    led->fled_cdev.ops = &strobe_ops;
>> +
>> +    setting = &led->fled_cdev.timeout;
>> +    setting->min = LM3601X_MIN_TIMEOUT_US;
>> +    setting->max = led->max_strobe_timeout;
>> +    setting->step = LM3601X_LOWER_STEP_US;
>> +    setting->val = led->max_strobe_timeout;
>> +
>> +    setting = &led->fled_cdev.brightness;
>> +    setting->min = LM3601X_MIN_STROBE_I_UA;
>> +    setting->max = LM3601X_MAX_STROBE_I_UA;
>> +    setting->step = LM3601X_TORCH_REG_DIV;
>> +    setting->val = led->strobe_current_max;
>> +
>> +    led_cdev = &led->fled_cdev.led_cdev;
>> +    led_cdev->name = led->led_name;
>> +    led_cdev->brightness_set_blocking = lm3601x_brightness_set;
>> +    led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA;
> 
> LED class brightness is expressed in levels, so:
> 
> led_cdev->max_brightness = DIV_ROUND_UP(LM3601X_MAX_TORCH_I_UA, LM3601X_TORCH_REG_DIV)

ACK

> 
>> +    led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> +    return led_classdev_flash_register(&led->client->dev, &led->fled_cdev);
>> +}
>> +
>> +static int lm3601x_parse_node(struct lm3601x_led *led,
>> +                  struct device_node *node)
>> +{
>> +    struct fwnode_handle *child = NULL;
>> +    int ret = -ENODEV;
>> +    const char *name;
>> +
>> +    child = device_get_next_child_node(&led->client->dev, child);
>> +    if (!child) {
>> +        dev_err(&led->client->dev, "No LED Child node\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
>> +    if (ret) {
>> +        dev_err(&led->client->dev, "reg DT property missing\n");
>> +        goto out_err;
>> +    }
>> +
>> +    if (led->led_mode > LM3601X_LED_TORCH ||
>> +        led->led_mode < LM3601X_LED_IR) {
>> +        dev_warn(&led->client->dev, "Invalid led mode requested\n");
>> +        ret = -EINVAL;
>> +        goto out_err;
>> +    }
>> +
>> +    ret = fwnode_property_read_string(child, "label", &name);
>> +    if (ret) {
>> +        if (led->led_mode == LM3601X_LED_TORCH)
>> +            name = "torch";
>> +        else
>> +            name = "infrared";
>> +    }
>> +
>> +    snprintf(led->led_name, sizeof(led->led_name),
>> +        "%s:%s", node->name, name);
> 
> Reading once again my recent explanation regarding this I realized
> that I didn't provide clear conclusion, which is: we no longer
> use child node name for LED class device name if label is absent.
> (apart from that - you're using parent DT node now, i.e.
> led-controller).
> 
> Please follow what was done for drivers/leds/leds-cr0014114.c.

Hmmm.  If this is calling dev->of_node->name to store the name will this
work in non-DT configurations?

I have not dug to deeply into the fwnode code to find out how the nodes
get populated.  So my question may not even be valid.



> 
>> +    ret = fwnode_property_read_u32(child, "led-max-microamp",
>> +                    &led->torch_current_max);
>> +    if (ret < 0) {
>> +        dev_warn(&led->client->dev,
>> +            "led-max-microamp DT property missing\n");
>> +        goto out_err;
>> +    }
>> +
>> +    ret = fwnode_property_read_u32(child, "flash-max-microamp",
>> +                &led->strobe_current_max);
>> +    if (ret < 0) {
>> +        dev_warn(&led->client->dev,
>> +             "flash-max-microamp DT property missing\n");
>> +        goto out_err;
>> +    }
>> +
>> +    ret = fwnode_property_read_u32(child, "flash-max-timeout-us",
>> +                &led->max_strobe_timeout);
>> +    if (ret < 0) {
>> +        dev_warn(&led->client->dev,
>> +             "flash-max-timeout-us DT property missing\n");
>> +        goto out_err;
>> +    }
>> +
>> +out_err:
>> +    fwnode_handle_put(child);
>> +    return ret;
>> +}
>> +
>> +static int lm3601x_probe(struct i2c_client *client,
>> +            const struct i2c_device_id *id)
>> +{
>> +    struct lm3601x_led *led;
>> +    int err;
>> +
>> +    led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>> +    if (!led)
>> +        return -ENOMEM;
>> +
>> +    led->client = client;
>> +    i2c_set_clientdata(client, led);
>> +
>> +    err = lm3601x_parse_node(led, client->dev.of_node);
>> +    if (err < 0)
>> +        return -ENODEV;
>> +
>> +    led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);
>> +    if (IS_ERR(led->regmap)) {
>> +        err = PTR_ERR(led->regmap);
>> +        dev_err(&client->dev,
>> +            "Failed to allocate register map: %d\n", err);
>> +        return err;
>> +    }
>> +
>> +    mutex_init(&led->lock);
>> +
>> +    err = lm3601x_register_leds(led);
> 
> return lm3601x_register_leds(led)

ACK.  I will end up dumping the err variable altogether

> 
>> +
>> +    return err;
>> +}
>> +
>> +static int lm3601x_remove(struct i2c_client *client)
>> +{
>> +    struct lm3601x_led *led = i2c_get_clientdata(client);
>> +
>> +    regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +               LM3601X_ENABLE_MASK,
>> +               LM3601X_MODE_STANDBY);
>> +
> 
> mutex_destroy() is missing.
> 
>> +    return 0;
>> +}
>> +
>> +static const struct i2c_device_id lm3601x_id[] = {
>> +    { "LM36010", CHIP_LM36010 },
>> +    { "LM36011", CHIP_LM36011 },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lm3601x_id);
>> +
>> +static const struct of_device_id of_lm3601x_leds_match[] = {
>> +    { .compatible = "ti,lm36010", },
>> +    { .compatible = "ti,lm36011", },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
>> +
>> +static struct i2c_driver lm3601x_i2c_driver = {
>> +    .driver = {
>> +        .name = "lm3601x",
>> +        .of_match_table = of_lm3601x_leds_match,
>> +    },
>> +    .probe = lm3601x_probe,
>> +    .remove = lm3601x_remove,
>> +    .id_table = lm3601x_id,
>> +};
>> +module_i2c_driver(lm3601x_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X");
>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>> +MODULE_LICENSE("GPL v2");
>>
> 


-- 
------------------
Dan Murphy

^ permalink raw reply

* [PATCH v2] of: unittest: for strings, account for trailing \0 in property length field
From: Stefan M Schaeckeler @ 2018-05-21 21:42 UTC (permalink / raw)
  To: Rob Herring, Frank Rowand, devicetree@vger.kernel.org

For strings, account for trailing \0 in property length field:

This is consistent with how dtc builds string properties.

Function __of_prop_dup() would misbehave on such properties as it duplicates
properties based on the property length field creating new string values
without trailing \0s.

Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
Tested-by: Frank Rowand <frank.rowand@sony.com>
---
Changes in v2:
 - fixed spacing around + sign.

 drivers/of/unittest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 53c83d6..c996835 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -162,13 +162,13 @@ static void __init of_unittest_dynamic(void)
 	prop++;
 	prop->name = "new-property";
 	prop->value = "new-property-data-should-fail";
-	prop->length = strlen(prop->value);
+	prop->length = strlen(prop->value) + 1;
 	unittest(of_add_property(np, prop) != 0,
 		 "Adding an existing property should have failed\n");
 
 	/* Try to modify an existing property - should pass */
 	prop->value = "modify-property-data-should-pass";
-	prop->length = strlen(prop->value);
+	prop->length = strlen(prop->value) + 1;
 	unittest(of_update_property(np, prop) == 0,
 		 "Updating an existing property should have passed\n");
 
@@ -176,7 +176,7 @@ static void __init of_unittest_dynamic(void)
 	prop++;
 	prop->name = "modify-property";
 	prop->value = "modify-missing-property-data-should-pass";
-	prop->length = strlen(prop->value);
+	prop->length = strlen(prop->value) + 1;
 	unittest(of_update_property(np, prop) == 0,
 		 "Updating a missing property should have passed\n");
 
-- 
2.10.3.dirty

^ permalink raw reply related

* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Dan Murphy @ 2018-05-21 21:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, devicetree,
	Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <CAHp75VcShgaFoJVqbTGBXrrgEr-7JQcCLK4Qy-jydE5++f4VAQ@mail.gmail.com>

Andy

Thanks for the review.  I was pretty sure I missed somethings.

On 05/21/2018 03:44 PM, Andy Shevchenko wrote:
> On Mon, May 21, 2018 at 9:09 PM, Dan Murphy <dmurphy@ti.com> wrote:
>> Introduce the family of LED devices that can
>> drive a torch, strobe or IR LED.
>>
>> The LED driver can be configured with a strobe
>> timer to execute a strobe flash.  The IR LED
>> brightness is controlled via the torch brightness
>> register.
>>
>> The data sheet for each the LM36010 and LM36011
>> LED drivers can be found here:
>> http://www.ti.com/product/LM36010
>> http://www.ti.com/product/LM36011
> 
> Thanks for an update. My comments below.
> 
>> +config LEDS_LM3601X
>> +       tristate "LED support for LM3601x Chips"
>> +       depends on LEDS_CLASS && I2C && OF
> 
> Now OF here is superfluous.

This is the first one I missed.

ACK

> 
>> +       depends on LEDS_CLASS_FLASH
>> +       select REGMAP_I2C
>> +       help
>> +         This option enables support for the TI LM3601x family
>> +         of flash, torch and indicator classes.
> 
>> +#define LM3601X_TIMEOUT_MASK   0x1e
>> +#define LM3601X_ENABLE_MASK    0x03
> 
> I dunno if GENMASK() will be better here.

I could probably make it more readable as well.

I am thinking

#define LM3601X_ENABLE_MASK (LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV)

> 
>> +#define LM3601X_LOWER_STEP_US  40000
>> +#define LM3601X_UPPER_STEP_US  200000
>> +#define LM3601X_MIN_TIMEOUT_US 40000
>> +#define LM3601X_MAX_TIMEOUT_US 1600000
> 
>> +#define LM3601X_TIMEOUT_XOVER  400000
> 
> Missed unit?

Ack

> 
>> +       ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                               LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
>> +                               led_mode_val);
> 
>> +               ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>> +                                       LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
>> +                                       LM3601X_MODE_STROBE);
> 
> Perhaps
> #define ..._MODE_TORCH_WITH_IR   (..._TOCRH | ..._IR_DRV)
> ?
> 

This was defined already I just did not use the #define but I will probably
replace this with the LM3601X_ENABLE_MASK #define

>> +static int lm3601x_probe(struct i2c_client *client,
>> +                       const struct i2c_device_id *id)
>> +{
>> +       struct lm3601x_led *led;
>> +       int err;
> 
>> +       err = lm3601x_parse_node(led, client->dev.of_node);
>> +       if (err < 0)
> 
>> +               return -ENODEV;
> 
> Shouldn't be
> 
> return err;

Ack.  But I might just get rid of err altogether when I make the change below to
return the status of the register.

> 
> ?
> 
>> +       err = lm3601x_register_leds(led);
>> +
>> +       return err;
> 
> I will leave this to maintainers since you seems have strong
> objections against more or less standard pattern, i.e.
> 
> return lm3601x_register_leds();

ACk.  I was going to change that but I was concerned about loosing err.
But I can change it.

> 
>> +}
> 


-- 
------------------
Dan Murphy

^ permalink raw reply

* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Jacek Anaszewski @ 2018-05-21 21:09 UTC (permalink / raw)
  To: Dan Murphy, robh+dt, mark.rutland, andy.shevchenko
  Cc: devicetree, linux-kernel, linux-leds
In-Reply-To: <20180521180927.18472-2-dmurphy@ti.com>

Hi Dan,

Thank you for the update.

On 05/21/2018 08:09 PM, Dan Murphy wrote:
> Introduce the family of LED devices that can
> drive a torch, strobe or IR LED.
> 
> The LED driver can be configured with a strobe
> timer to execute a strobe flash.  The IR LED
> brightness is controlled via the torch brightness
> register.
> 
> The data sheet for each the LM36010 and LM36011
> LED drivers can be found here:
> http://www.ti.com/product/LM36010
> http://www.ti.com/product/LM36011
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v7 - Numerous fixes to many to list.  Highlights are moved from OF APIs to device_property
> APIs, updated LED registration, timeout to reg algorithim, and fixed label generation -
> https://patchwork.kernel.org/patch/10401437/
> 
> v6 - This driver has been heavily modified from v5.  There is no longer reading
> of individual child nodes.  There are too many changes to list here see -
> https://patchwork.kernel.org/patch/10392123/
> v5 - Fixed magic numbers, change reg cache type, added of_put_node to release
> the dt node ref, and I did not change the remove function to leave the LED in its
> state on driver removal - https://patchwork.kernel.org/patch/10391741/
> v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/
> v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed
> RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type,
> fixed unlock and extra semi colon in strobe_set, removed unnecessary out label
> in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/
> v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/
> 
>   drivers/leds/Kconfig        |   9 +
>   drivers/leds/Makefile       |   1 +
>   drivers/leds/leds-lm3601x.c | 501 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 511 insertions(+)
>   create mode 100644 drivers/leds/leds-lm3601x.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2c896c0e69e1..50ae536f343f 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -145,6 +145,15 @@ config LEDS_LM3692X
>   	  This option enables support for the TI LM3692x family
>   	  of white LED string drivers used for backlighting.
>   
> +config LEDS_LM3601X
> +	tristate "LED support for LM3601x Chips"
> +	depends on LEDS_CLASS && I2C && OF
> +	depends on LEDS_CLASS_FLASH
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for the TI LM3601x family
> +	  of flash, torch and indicator classes.
> +
>   config LEDS_LOCOMO
>   	tristate "LED Support for Locomo device"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 91eca81cae82..b79807fe1b67 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
>   obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
>   obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
>   obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
> +obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
>   
>   # LED SPI Drivers
>   obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c
> new file mode 100644
> index 000000000000..7c71cb11d474
> --- /dev/null
> +++ b/drivers/leds/leds-lm3601x.c
> @@ -0,0 +1,501 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Flash and torch driver for Texas Instruments LM3601X LED
> +// Flash driver chip family
> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
> +
> +#define LM3601X_LED_IR		0x0
> +#define LM3601X_LED_TORCH	0x1
> +
> +/* Registers */
> +#define LM3601X_ENABLE_REG	0x01
> +#define LM3601X_CFG_REG		0x02
> +#define LM3601X_LED_FLASH_REG	0x03
> +#define LM3601X_LED_TORCH_REG	0x04
> +#define LM3601X_FLAGS_REG	0x05
> +#define LM3601X_DEV_ID_REG	0x06
> +
> +#define LM3601X_SW_RESET	BIT(7)
> +
> +/* Enable Mode bits */
> +#define LM3601X_MODE_STANDBY	0x00
> +#define LM3601X_MODE_IR_DRV	BIT(0)
> +#define LM3601X_MODE_TORCH	BIT(1)
> +#define LM3601X_MODE_STROBE	(BIT(0) | BIT(1))
> +#define LM3601X_STRB_EN		BIT(2)
> +#define LM3601X_STRB_EDGE_TRIG	BIT(3)
> +#define LM3601X_IVFM_EN		BIT(4)
> +
> +#define LM36010_BOOST_LIMIT_28	BIT(5)
> +#define LM36010_BOOST_FREQ_4MHZ	BIT(6)
> +#define LM36010_BOOST_MODE_PASS	BIT(7)
> +
> +/* Flag Mask */
> +#define LM3601X_FLASH_TIME_OUT	BIT(0)
> +#define LM3601X_UVLO_FAULT	BIT(1)
> +#define LM3601X_THERM_SHUTDOWN	BIT(2)
> +#define LM3601X_THERM_CURR	BIT(3)
> +#define LM36010_CURR_LIMIT	BIT(4)
> +#define LM3601X_SHORT_FAULT	BIT(5)
> +#define LM3601X_IVFM_TRIP	BIT(6)
> +#define LM36010_OVP_FAULT	BIT(7)
> +
> +#define LM3601X_MAX_TORCH_I_UA	376000
> +#define LM3601X_MIN_TORCH_I_UA	2400
> +#define LM3601X_TORCH_REG_DIV	3000
> +
> +#define LM3601X_MAX_STROBE_I_UA	1500000
> +#define LM3601X_MIN_STROBE_I_UA	11000
> +#define LM3601X_STROBE_REG_DIV	11800
> +
> +#define LM3601X_TIMEOUT_MASK	0x1e
> +#define LM3601X_ENABLE_MASK	0x03
> +
> +#define LM3601X_LOWER_STEP_US	40000
> +#define LM3601X_UPPER_STEP_US	200000
> +#define LM3601X_MIN_TIMEOUT_US	40000
> +#define LM3601X_MAX_TIMEOUT_US	1600000
> +#define LM3601X_TIMEOUT_XOVER	400000
> +
> +enum lm3601x_type {
> +	CHIP_LM36010 = 0,
> +	CHIP_LM36011,
> +};
> +
> +/**
> + * struct lm3601x_led -
> + * @fled_cdev: flash led class device pointer

s/led/LED/

> + * @client: Pointer to the I2C client
> + * @regmap: Devices register map
> + * @lock: Lock for reading/writing the device
> + * @led_name: LED label for the Torch or IR LED
> + * @strobe_timeout: the timeout for the strobe
> + * @last_flag: last known flags register value
> + * @torch_current_max: maximum current for the torch
> + * @strobe_current_max: maximum current for the strobe
> + * @max_strobe_timeout: maximum timeout for the strobe
> + * @led_mode: The mode to enable either IR or Torch
> + */
> +struct lm3601x_led {
> +	struct led_classdev_flash fled_cdev;
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct mutex lock;
> +
> +	char led_name[LED_MAX_NAME_SIZE];
> +
> +	unsigned int strobe_timeout;
> +	unsigned int last_flag;
> +
> +	u32 torch_current_max;
> +	u32 strobe_current_max;
> +	u32 max_strobe_timeout;
> +
> +	u32 led_mode;
> +};
> +
> +static const struct reg_default lm3601x_regmap_defs[] = {
> +	{ LM3601X_ENABLE_REG, 0x20 },
> +	{ LM3601X_CFG_REG, 0x15 },
> +	{ LM3601X_LED_FLASH_REG, 0x00 },
> +	{ LM3601X_LED_TORCH_REG, 0x00 },
> +};
> +
> +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LM3601X_FLAGS_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config lm3601x_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = LM3601X_DEV_ID_REG,
> +	.reg_defaults = lm3601x_regmap_defs,
> +	.num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs),
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = lm3601x_volatile_reg,
> +};
> +
> +static struct lm3601x_led *fled_cdev_to_led(
> +				struct led_classdev_flash *fled_cdev)
> +{
> +	return container_of(fled_cdev, struct lm3601x_led, fled_cdev);
> +}
> +
> +static int lm3601x_read_faults(struct lm3601x_led *led)
> +{
> +	int flags_val;
> +	int ret;
> +
> +	ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val);
> +	if (ret < 0)
> +		return -EIO;
> +
> +	led->last_flag = 0;
> +
> +	if (flags_val & LM36010_OVP_FAULT)
> +		led->last_flag |= LED_FAULT_OVER_VOLTAGE;
> +
> +	if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR))
> +		led->last_flag |= LED_FAULT_OVER_TEMPERATURE;
> +
> +	if (flags_val & LM3601X_SHORT_FAULT)
> +		led->last_flag |= LED_FAULT_SHORT_CIRCUIT;
> +
> +	if (flags_val & LM36010_CURR_LIMIT)
> +		led->last_flag |= LED_FAULT_OVER_CURRENT;
> +
> +	if (flags_val & LM3601X_UVLO_FAULT)
> +		led->last_flag |= LED_FAULT_UNDER_VOLTAGE;
> +
> +	if (flags_val & LM3601X_IVFM_TRIP)
> +		led->last_flag |= LED_FAULT_INPUT_VOLTAGE;
> +
> +	if (flags_val & LM3601X_THERM_SHUTDOWN)
> +		led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE;
> +
> +	return led->last_flag;
> +}
> +
> +static int lm3601x_brightness_set(struct led_classdev *cdev,
> +					enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
> +	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
> +	int ret, led_mode_val;
> +	u8 brightness_val;
> +
> +	mutex_lock(&led->lock);
> +
> +	ret = lm3601x_read_faults(led);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (led->led_mode == LM3601X_LED_TORCH)
> +		led_mode_val = LM3601X_MODE_TORCH;
> +	else
> +		led_mode_val = LM3601X_MODE_IR_DRV;
> +
> +	if (brightness == LED_OFF) {
> +		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +					led_mode_val, LED_OFF);
> +		goto out;
> +	}
> +
> +	if (brightness == LED_ON)
> +		brightness_val = LED_ON;

Use of LED_ON enum can be a bit misleading here,
It was designed for LEDs that can be only turned on/off.

You can use DIV_ROUND_UP to achieve the same in a more clear way.

> +	else
> +		brightness_val = brightness / LM3601X_TORCH_REG_DIV;
> +
> +	ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +				LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
> +				led_mode_val);
> +out:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev,
> +				bool state)
> +{
> +	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
> +	int timeout_reg_val = 0;
> +	int current_timeout;
> +	int ret;
> +
> +	mutex_lock(&led->lock);
> +
> +	ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER)
> +		timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07;
> +	else
> +		timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01;
> +
> +	if (led->strobe_timeout != current_timeout)
> +		ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG,
> +					LM3601X_TIMEOUT_MASK, timeout_reg_val);
> +
> +	if (state)
> +		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +					LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
> +					LM3601X_MODE_STROBE);
> +	else
> +		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +					LM3601X_MODE_STROBE, LED_OFF);
> +
> +	ret = lm3601x_read_faults(led);
> +out:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int lm3601x_strobe_brightness_set(struct led_classdev_flash *fled_cdev,
> +					u32 brightness)
> +{
> +	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
> +	int ret;
> +	u8 brightness_val;
> +
> +	mutex_lock(&led->lock);
> +	ret = lm3601x_read_faults(led);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (brightness == LED_OFF) {
> +		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +					LM3601X_MODE_STROBE, LED_OFF);
> +		goto out;
> +	}
> +
> +	if (brightness == LED_ON)
> +		brightness_val = LED_ON;

Ditto.

> +	else
> +		brightness_val = brightness / LM3601X_STROBE_REG_DIV;
> +
> +	ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val);
> +out:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev,
> +				u32 timeout)
> +{
> +	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
> +	int ret = 0;
> +
> +	mutex_lock(&led->lock);
> +
> +	led->strobe_timeout = timeout;
> +
> +	mutex_unlock(&led->lock);
> +
> +	return ret;
> +}
> +
> +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev,
> +				bool *state)
> +{
> +	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
> +	int ret;
> +	int strobe_state;
> +
> +	mutex_lock(&led->lock);
> +
> +	ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state);
> +	if (ret < 0)
> +		goto out;
> +
> +	*state = strobe_state & LM3601X_MODE_STROBE;
> +
> +out:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev,
> +				u32 *fault)
> +{
> +	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
> +
> +	lm3601x_read_faults(led);
> +
> +	*fault = led->last_flag;
> +
> +	return 0;
> +}
> +
> +static const struct led_flash_ops strobe_ops = {
> +	.flash_brightness_set	= lm3601x_strobe_brightness_set,

s/strobe/flash/

> +	.strobe_set		= lm3601x_strobe_set,
> +	.strobe_get		= lm3601x_strobe_get,
> +	.timeout_set		= lm3601x_strobe_timeout_set,

Ditto.

> +	.fault_get		= lm3601x_strobe_fault_get,

Ditto.

> +};
> +
> +static int lm3601x_register_leds(struct lm3601x_led *led)
> +{
> +	struct led_classdev *led_cdev;
> +	struct led_flash_setting *setting;
> +
> +	led->fled_cdev.ops = &strobe_ops;
> +
> +	setting = &led->fled_cdev.timeout;
> +	setting->min = LM3601X_MIN_TIMEOUT_US;
> +	setting->max = led->max_strobe_timeout;
> +	setting->step = LM3601X_LOWER_STEP_US;
> +	setting->val = led->max_strobe_timeout;
> +
> +	setting = &led->fled_cdev.brightness;
> +	setting->min = LM3601X_MIN_STROBE_I_UA;
> +	setting->max = LM3601X_MAX_STROBE_I_UA;
> +	setting->step = LM3601X_TORCH_REG_DIV;
> +	setting->val = led->strobe_current_max;
> +
> +	led_cdev = &led->fled_cdev.led_cdev;
> +	led_cdev->name = led->led_name;
> +	led_cdev->brightness_set_blocking = lm3601x_brightness_set;
> +	led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA;

LED class brightness is expressed in levels, so:

led_cdev->max_brightness = DIV_ROUND_UP(LM3601X_MAX_TORCH_I_UA, 
LM3601X_TORCH_REG_DIV)

> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	return led_classdev_flash_register(&led->client->dev, &led->fled_cdev);
> +}
> +
> +static int lm3601x_parse_node(struct lm3601x_led *led,
> +			      struct device_node *node)
> +{
> +	struct fwnode_handle *child = NULL;
> +	int ret = -ENODEV;
> +	const char *name;
> +
> +	child = device_get_next_child_node(&led->client->dev, child);
> +	if (!child) {
> +		dev_err(&led->client->dev, "No LED Child node\n");
> +		return ret;
> +	}
> +
> +	ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
> +	if (ret) {
> +		dev_err(&led->client->dev, "reg DT property missing\n");
> +		goto out_err;
> +	}
> +
> +	if (led->led_mode > LM3601X_LED_TORCH ||
> +	    led->led_mode < LM3601X_LED_IR) {
> +		dev_warn(&led->client->dev, "Invalid led mode requested\n");
> +		ret = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	ret = fwnode_property_read_string(child, "label", &name);
> +	if (ret) {
> +		if (led->led_mode == LM3601X_LED_TORCH)
> +			name = "torch";
> +		else
> +			name = "infrared";
> +	}
> +
> +	snprintf(led->led_name, sizeof(led->led_name),
> +		"%s:%s", node->name, name);

Reading once again my recent explanation regarding this I realized
that I didn't provide clear conclusion, which is: we no longer
use child node name for LED class device name if label is absent.
(apart from that - you're using parent DT node now, i.e.
led-controller).

Please follow what was done for drivers/leds/leds-cr0014114.c.

> +	ret = fwnode_property_read_u32(child, "led-max-microamp",
> +					&led->torch_current_max);
> +	if (ret < 0) {
> +		dev_warn(&led->client->dev,
> +			"led-max-microamp DT property missing\n");
> +		goto out_err;
> +	}
> +
> +	ret = fwnode_property_read_u32(child, "flash-max-microamp",
> +				&led->strobe_current_max);
> +	if (ret < 0) {
> +		dev_warn(&led->client->dev,
> +			 "flash-max-microamp DT property missing\n");
> +		goto out_err;
> +	}
> +
> +	ret = fwnode_property_read_u32(child, "flash-max-timeout-us",
> +				&led->max_strobe_timeout);
> +	if (ret < 0) {
> +		dev_warn(&led->client->dev,
> +			 "flash-max-timeout-us DT property missing\n");
> +		goto out_err;
> +	}
> +
> +out_err:
> +	fwnode_handle_put(child);
> +	return ret;
> +}
> +
> +static int lm3601x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct lm3601x_led *led;
> +	int err;
> +
> +	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->client = client;
> +	i2c_set_clientdata(client, led);
> +
> +	err = lm3601x_parse_node(led, client->dev.of_node);
> +	if (err < 0)
> +		return -ENODEV;
> +
> +	led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);
> +	if (IS_ERR(led->regmap)) {
> +		err = PTR_ERR(led->regmap);
> +		dev_err(&client->dev,
> +			"Failed to allocate register map: %d\n", err);
> +		return err;
> +	}
> +
> +	mutex_init(&led->lock);
> +
> +	err = lm3601x_register_leds(led);

return lm3601x_register_leds(led)

> +
> +	return err;
> +}
> +
> +static int lm3601x_remove(struct i2c_client *client)
> +{
> +	struct lm3601x_led *led = i2c_get_clientdata(client);
> +
> +	regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +			   LM3601X_ENABLE_MASK,
> +			   LM3601X_MODE_STANDBY);
> +

mutex_destroy() is missing.

> +	return 0;
> +}
> +
> +static const struct i2c_device_id lm3601x_id[] = {
> +	{ "LM36010", CHIP_LM36010 },
> +	{ "LM36011", CHIP_LM36011 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, lm3601x_id);
> +
> +static const struct of_device_id of_lm3601x_leds_match[] = {
> +	{ .compatible = "ti,lm36010", },
> +	{ .compatible = "ti,lm36011", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
> +
> +static struct i2c_driver lm3601x_i2c_driver = {
> +	.driver = {
> +		.name = "lm3601x",
> +		.of_match_table = of_lm3601x_leds_match,
> +	},
> +	.probe = lm3601x_probe,
> +	.remove = lm3601x_remove,
> +	.id_table = lm3601x_id,
> +};
> +module_i2c_driver(lm3601x_i2c_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X");
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH v6 6/9] input: touchscreen: resistive-adc-touch: add generic resistive ADC touchscreen
From: Dmitry Torokhov @ 2018-05-21 21:06 UTC (permalink / raw)
  To: Eugen Hristev
  Cc: jic23, ludovic.desroches, alexandre.belloni, linux-arm-kernel,
	devicetree, linux-kernel, linux-iio, linux-input, nicolas.ferre,
	robh
In-Reply-To: <1526898477-9952-7-git-send-email-eugen.hristev@microchip.com>

On Mon, May 21, 2018 at 01:27:54PM +0300, Eugen Hristev wrote:
> This adds a generic resistive touchscreen (GRTS) driver, which is based
> on an IIO device (an ADC). It must be connected to the channels of an ADC
> to receive touch data. Then it will feed the data into the input subsystem
> where it registers an input device.
> It uses an IIO callback buffer to register to the IIO device
> 
> Some parts of this patch are based on initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I assume this will go through IIO tree? Just a couple of comsmetic
comments below, feel free to ignore.

> ---
> Changes in v6:
>  -  changed a dev_err to dev_dbg which was forgotten in v5.
> 
> Changes in v5:
>  - return error on probe if failed add_action_or_reset
>  - renamed property touchscreen-threshold-pressure to
> touchscreen-min-pressure, changed variables accordingly
> 
> Changes in v4:
>  - added a small description in file header
>  - changed MAX_POS_MASK to GRTS_MAX_POS_MASK
>  - initialized press with 0, as this value means no touch.
> press has to be initialized because compiler/checkpatch complains
> that can be used uninitialized.
>  - changed of_property_read_u32 to device_*
>  - set_abs_params for pressure will have range according to threshold
>  - changed evbit and keybit with set_capability call
>  - changed variable names to error instead of ret
>  - checked errors for add_action_or_reset
>  - cosmetic cleaning
> 
> Changes in v3:
>  - pressure now reported naturally growing down-up
>  - using helpers from touchscreen.h to parse DT properties
>  - added devm_add_action_or_reset to handle callback buffer clean on exit
>  - changed compatible and threshold property to adapt to changed bindings
> 
> Changes in v2:
>  - this is now a generic resistive adc touchscreen driver
> 
> 
>  drivers/input/touchscreen/Kconfig               |  13 ++
>  drivers/input/touchscreen/Makefile              |   1 +
>  drivers/input/touchscreen/resistive-adc-touch.c | 201 ++++++++++++++++++++++++
>  3 files changed, 215 insertions(+)
>  create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 4f15496..8f85d3a 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -92,6 +92,19 @@ config TOUCHSCREEN_AD7879_SPI
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7879-spi.
>  
> +config TOUCHSCREEN_ADC
> +	tristate "Generic ADC based resistive touchscreen"
> +	depends on IIO
> +	select IIO_BUFFER_CB
> +	help
> +	  Say Y here if you want to use the generic ADC
> +	  resistive touchscreen driver.
> +
> +	  If unsure, say N (but it's safe to say "Y").
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called resistive-adc-touch.ko.
> +
>  config TOUCHSCREEN_AR1021_I2C
>  	tristate "Microchip AR1020/1021 i2c touchscreen"
>  	depends on I2C && OF
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index dddae79..843c7f9 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C)	+= ad7879-i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
> +obj-$(CONFIG_TOUCHSCREEN_ADC)		+= resistive-adc-touch.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
> diff --git a/drivers/input/touchscreen/resistive-adc-touch.c b/drivers/input/touchscreen/resistive-adc-touch.c
> new file mode 100644
> index 0000000..05d629b
> --- /dev/null
> +++ b/drivers/input/touchscreen/resistive-adc-touch.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADC generic resistive touchscreen (GRTS)
> + * This is a generic input driver that connects to an ADC
> + * given the channels in device tree, and reports events to the input
> + * subsystem.
> + *
> + * Copyright (C) 2017,2018 Microchip Technology,
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + *
> + */
> +#include <linux/input.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME					"resistive-adc-touch"
> +#define GRTS_DEFAULT_PRESSURE_MIN			50000
> +#define GRTS_MAX_POS_MASK				GENMASK(11, 0)
> +
> +/**
> + * grts_state - generic resistive touch screen information struct
> + * @pressure_min:	number representing the minimum for the pressure
> + * @pressure:		are we getting pressure info or not
> + * @iio_chans:		list of channels acquired
> + * @iio_cb:		iio_callback buffer for the data
> + * @input:		the input device structure that we register
> + * @prop:		touchscreen properties struct
> + */
> +struct grts_state {
> +	u32				pressure_min;
> +	bool				pressure;
> +	struct iio_channel		*iio_chans;
> +	struct iio_cb_buffer		*iio_cb;
> +	struct input_dev		*input;
> +	struct touchscreen_properties	prop;
> +};
> +
> +static int grts_cb(const void *data, void *private)
> +{
> +	const u16 *touch_info = data;
> +	struct grts_state *st = private;
> +	unsigned int x, y, press = 0x0;
> +
> +	/* channel data coming in buffer in the order below */
> +	x = touch_info[0];
> +	y = touch_info[1];
> +	if (st->pressure)
> +		press = touch_info[2];
> +
> +	if ((!x && !y) || (st->pressure && (press < st->pressure_min))) {
> +		/* report end of touch */
> +		input_report_key(st->input, BTN_TOUCH, 0);
> +		input_sync(st->input);
> +		return 0;
> +	}
> +
> +	/* report proper touch to subsystem*/
> +	touchscreen_report_pos(st->input, &st->prop, x, y, false);
> +	if (st->pressure)
> +		input_report_abs(st->input, ABS_PRESSURE, press);
> +	input_report_key(st->input, BTN_TOUCH, 1);
> +	input_sync(st->input);
> +
> +	return 0;
> +}
> +
> +static int grts_open(struct input_dev *dev)
> +{
> +	int error;
> +	struct grts_state *st = input_get_drvdata(dev);
> +
> +	error = iio_channel_start_all_cb(st->iio_cb);
> +	if (error) {
> +		dev_err(dev->dev.parent, "failed to start callback buffer.\n");
> +		return error;
> +	}
> +	return 0;
> +}
> +
> +static void grts_close(struct input_dev *dev)
> +{
> +	struct grts_state *st = input_get_drvdata(dev);
> +
> +	iio_channel_stop_all_cb(st->iio_cb);
> +}
> +
> +static void grts_disable(void *data)
> +{
> +	iio_channel_release_all_cb(data);
> +}
> +
> +static int grts_probe(struct platform_device *pdev)
> +{
> +	struct grts_state *st;
> +	struct input_dev *input;
> +	struct device *dev = &pdev->dev;
> +	struct iio_channel *chan;
> +	int error;
> +
> +	st = devm_kzalloc(dev, sizeof(struct grts_state), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	error = device_property_read_u32(dev, "touchscreen-min-pressure",
> +					 &st->pressure_min);
> +	if (error) {
> +		dev_dbg(dev, "can't get touchscreen-min-pressure property.\n");
> +		st->pressure_min = GRTS_DEFAULT_PRESSURE_MIN;
> +	}

I do not think it makes sense to complain about missing property if
there is no "pressure" channel. Probably move down?

> +
> +	/* get the channels from IIO device */
> +	st->iio_chans = devm_iio_channel_get_all(dev);
> +	if (IS_ERR(st->iio_chans)) {
> +		if (PTR_ERR(st->iio_chans) != -EPROBE_DEFER)
> +			dev_err(dev, "can't get iio channels.\n");
> +		return PTR_ERR(st->iio_chans);

		error = PTR_ERR(st->iio_chans);
		if (error != -EPROBE_DEFER)
			dev_err(dev, "can't get iio channels.\n");
		return error;

> +	}
> +
> +	chan = &st->iio_chans[0];
> +	st->pressure = false;
> +	while (chan && chan->indio_dev) {
> +		if (!strcmp(chan->channel->datasheet_name, "pressure"))
> +			st->pressure = true;
> +		chan++;
> +	}
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "failed to allocate input device.\n");
> +		return -ENOMEM;
> +	}
> +
> +	input->name = DRIVER_NAME;
> +	input->id.bustype = BUS_HOST;
> +	input->open = grts_open;
> +	input->close = grts_close;
> +
> +	input_set_abs_params(input, ABS_X, 0, GRTS_MAX_POS_MASK - 1, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, GRTS_MAX_POS_MASK - 1, 0, 0);
> +	if (st->pressure)
> +		input_set_abs_params(input, ABS_PRESSURE, st->pressure_min,
> +				     0xffff, 0, 0);
> +
> +	input_set_capability(input, EV_KEY, BTN_TOUCH);
> +
> +	/* parse optional device tree properties */
> +	touchscreen_parse_properties(input, false, &st->prop);
> +
> +	st->input = input;
> +	input_set_drvdata(input, st);
> +
> +	error = input_register_device(input);
> +	if (error) {
> +		dev_err(dev, "failed to register input device.");
> +		return error;
> +	}
> +
> +	st->iio_cb = iio_channel_get_all_cb(dev, grts_cb, st);
> +	if (IS_ERR(st->iio_cb)) {
> +		dev_err(dev, "failed to allocate callback buffer.\n");
> +		error =  PTR_ERR(st->iio_cb);
> +		return error;

Simply:

		return PTR_ERR(st->iio_cb);

since you opted out of printing error codes in error messages.

> +	}
> +
> +	error = devm_add_action_or_reset(dev, grts_disable, st->iio_cb);
> +	if (error) {
> +		dev_err(dev, "failed to add disable action.\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id grts_of_match[] = {
> +	{
> +		.compatible = "resistive-adc-touch",
> +	}, {
> +		/* sentinel */
> +	},
> +};
> +
> +MODULE_DEVICE_TABLE(of, grts_of_match);
> +
> +static struct platform_driver grts_driver = {
> +	.probe = grts_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = of_match_ptr(grts_of_match),
> +	},
> +};
> +
> +module_platform_driver(grts_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
> +MODULE_DESCRIPTION("Generic ADC Resistive Touch Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

^ permalink raw reply

* [PATCH v5 4/4] gpiolib: discourage gpiochip_add_pin[group]_range for DT pinctrls
From: Christian Lamparter @ 2018-05-21 20:57 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel, linux-arm-msm, devicetree
  Cc: Mark Rutland, Stephen Boyd, Linus Walleij, Bjorn Andersson,
	David Brown, Rob Herring, Sven Eckelmann, Andy Gross
In-Reply-To: <0ae3376606a89bcdf3fe753a5c967f7103699e09.1526935804.git.chunkeey@gmail.com>

This patch adds the stern warning to the kerneldoc text of both
gpiochip_add_pin[group]_range() functions in hope of detering
developers from ever using them in their DeviceTree-supported
pinctrl drivers in the future.

For anyone affected: Please refer to Section 2.1 of
Documentation/devicetree/bindings/gpio/gpio.txt on how to
bind pinctrl and gpio drivers via the "gpio-ranges" property.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---

As a (final?) follow-up to Stephen's request of adding a warning
to notify devs and users alike about any potentially broken drivers:

of_gpiochip_add_pin_range() - which parses the gpio-ranges property -
calls both gpiochip_add_pin(group)_range functions. So adding a
warning/debug message into the code will not really work (unless
the DT-check will also take the call-trace into account... which is
possible, but at this point it would make more sense to just
refactor the code).
---
 drivers/gpio/gpiolib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1171db66c30..387bf0677ca8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2117,6 +2117,11 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_config);
  * @pctldev: the pin controller to map to
  * @gpio_offset: the start offset in the current gpio_chip number space
  * @pin_group: name of the pin group inside the pin controller
+ *
+ * Calling this function directly from a DeviceTree-supported
+ * pinctrl driver is DEPRECATED. Please see Section 2.1 of
+ * Documentation/devicetree/bindings/gpio/gpio.txt on how to
+ * bind pinctrl and gpio drivers via the "gpio-ranges" property.
  */
 int gpiochip_add_pingroup_range(struct gpio_chip *chip,
 			struct pinctrl_dev *pctldev,
@@ -2170,6 +2175,11 @@ EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);
  *
  * Returns:
  * 0 on success, or a negative error-code on failure.
+ *
+ * Calling this function directly from a DeviceTree-supported
+ * pinctrl driver is DEPRECATED. Please see Section 2.1 of
+ * Documentation/devicetree/bindings/gpio/gpio.txt on how to
+ * bind pinctrl and gpio drivers via the "gpio-ranges" property.
  */
 int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
 			   unsigned int gpio_offset, unsigned int pin_offset,
-- 
2.17.0

^ permalink raw reply related

* [PATCH v5 3/4] ARM: dts: qcom: add gpio-ranges property
From: Christian Lamparter @ 2018-05-21 20:57 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel, linux-arm-msm, devicetree
  Cc: Mark Rutland, Stephen Boyd, Linus Walleij, Bjorn Andersson,
	David Brown, Rob Herring, Sven Eckelmann, Andy Gross
In-Reply-To: <270904bcbf03a6b6ac3150f4a7daa15aa7e97586.1526935804.git.chunkeey@gmail.com>

This patch adds the gpio-ranges property to almost all of
the Qualcomm ARM platforms that utilize the pinctrl-msm
framework.

The gpio-ranges property is part of the gpiolib subsystem.
As a result, the binding text is available in section
"2.1 gpio- and pin-controller interaction" of
Documentation/devicetree/bindings/gpio/gpio.txt

For more information please see the patch titled:
"pinctrl: msm: fix gpio-hog related boot issues" from
this series.

Reported-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
Tested-by: Sven Eckelmann <sven.eckelmann@openmesh.com> [ipq4019]
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>

---
To help with git bisect, the DT update patch has been intentionally
placed after the "pinctrl: msm: fix gpio-hog related boot issues".
Otherwise - if the order was reveresed - and bisect decides to split
between these two patches, the gpiochip_add_pin_ranges() function
will be executed twice with the same parameters for the same pinctrl.
---
 arch/arm/boot/dts/qcom-apq8064.dtsi   | 1 +
 arch/arm/boot/dts/qcom-apq8084.dtsi   | 1 +
 arch/arm/boot/dts/qcom-ipq4019.dtsi   | 1 +
 arch/arm/boot/dts/qcom-ipq8064.dtsi   | 1 +
 arch/arm/boot/dts/qcom-mdm9615.dtsi   | 1 +
 arch/arm/boot/dts/qcom-msm8660.dtsi   | 1 +
 arch/arm/boot/dts/qcom-msm8960.dtsi   | 1 +
 arch/arm/boot/dts/qcom-msm8974.dtsi   | 1 +
 arch/arm64/boot/dts/qcom/ipq8074.dtsi | 3 ++-
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 1 +
 arch/arm64/boot/dts/qcom/msm8992.dtsi | 1 +
 arch/arm64/boot/dts/qcom/msm8994.dtsi | 1 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 1 +
 13 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 5341a39c0392..4001eeb52f20 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -344,6 +344,7 @@
 			reg = <0x800000 0x4000>;
 
 			gpio-controller;
+			gpio-ranges = <&tlmm_pinmux 0 0 90>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 0e1e98707e3f..d9481d083802 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -396,6 +396,7 @@
 			compatible = "qcom,apq8084-pinctrl";
 			reg = <0xfd510000 0x4000>;
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 147>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-ipq4019.dtsi b/arch/arm/boot/dts/qcom-ipq4019.dtsi
index 10d112a4078e..9a81d2da87a0 100644
--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
@@ -146,6 +146,7 @@
 			compatible = "qcom,ipq4019-pinctrl";
 			reg = <0x01000000 0x300000>;
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 100>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 1e0a3b446f7a..26eab9a68d90 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -108,6 +108,7 @@
 			reg = <0x800000 0x4000>;
 
 			gpio-controller;
+			gpio-ranges = <&qcom_pinmux 0 0 69>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-mdm9615.dtsi b/arch/arm/boot/dts/qcom-mdm9615.dtsi
index c852b69229c9..cfdaca5f259a 100644
--- a/arch/arm/boot/dts/qcom-mdm9615.dtsi
+++ b/arch/arm/boot/dts/qcom-mdm9615.dtsi
@@ -128,6 +128,7 @@
 		msmgpio: pinctrl@800000 {
 			compatible = "qcom,mdm9615-pinctrl";
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 88>;
 			#gpio-cells = <2>;
 			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/qcom-msm8660.dtsi b/arch/arm/boot/dts/qcom-msm8660.dtsi
index 33030f9419fe..47cf9c4ca062 100644
--- a/arch/arm/boot/dts/qcom-msm8660.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8660.dtsi
@@ -110,6 +110,7 @@
 			reg = <0x800000 0x4000>;
 
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 173>;
 			#gpio-cells = <2>;
 			interrupts = <0 16 0x4>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi
index 1733d8f40ab1..f6d8b1af5a8a 100644
--- a/arch/arm/boot/dts/qcom-msm8960.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8960.dtsi
@@ -102,6 +102,7 @@
 		msmgpio: pinctrl@800000 {
 			compatible = "qcom,msm8960-pinctrl";
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 152>;
 			#gpio-cells = <2>;
 			interrupts = <0 16 0x4>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index d9019a49b292..1250e071a6e2 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -696,6 +696,7 @@
 			compatible = "qcom,msm8974-pinctrl";
 			reg = <0xfd510000 0x4000>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 146>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
index 2bc5dec5614d..d2c36b467466 100644
--- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
@@ -24,11 +24,12 @@
 		ranges = <0 0 0 0xffffffff>;
 		compatible = "simple-bus";
 
-		pinctrl@1000000 {
+		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq8074-pinctrl";
 			reg = <0x1000000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 70>;
 			#gpio-cells = <0x2>;
 			interrupt-controller;
 			#interrupt-cells = <0x2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 66b318e1de80..9d5320b26188 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -326,6 +326,7 @@
 			reg = <0x1000000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 122>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index 171578747ed0..173b6bc60816 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -179,6 +179,7 @@
 			reg = <0xfd510000 0x4000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 146>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8994.dtsi b/arch/arm64/boot/dts/qcom/msm8994.dtsi
index f33c41d01c86..68705db4696b 100644
--- a/arch/arm64/boot/dts/qcom/msm8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8994.dtsi
@@ -141,6 +141,7 @@
 			reg = <0xfd510000 0x4000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 146>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 410ae787ebb4..b9b57808fc67 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -541,6 +541,7 @@
 			reg = <0x01010000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 150>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
-- 
2.17.0

^ permalink raw reply related

* [PATCH v5 2/4] pinctrl: msm: fix gpio-hog related boot issues
From: Christian Lamparter @ 2018-05-21 20:57 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel, linux-arm-msm, devicetree
  Cc: Mark Rutland, Stephen Boyd, Linus Walleij, Bjorn Andersson,
	David Brown, Rob Herring, Sven Eckelmann, Andy Gross
In-Reply-To: <910e5a85a6a8020069996e2ff397c93e9c5fe18c.1526935804.git.chunkeey@gmail.com>

Sven Eckelmann reported an issue with the current IPQ4019 pinctrl.
Setting up any gpio-hog in the device-tree for his device would
"kill the bootup completely":

| [    0.477838] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl@1000000/serial_pinmux, deferring probe
| [    0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@1000000/spi_0_pinmux, deferring probe
| [    1.298883] requesting hog GPIO enable USB2 power (chip 1000000.pinctrl, offset 58) failed, -517
| [    1.299609] gpiochip_add_data: GPIOs 0..99 (1000000.pinctrl) failed to register
| [    1.308589] ipq4019-pinctrl 1000000.pinctrl: Failed register gpiochip
| [    1.316586] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl@1000000/serial_pinmux, deferring probe
| [    1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@1000000/spi_0_pinmux, deferri

This was also verified on a RT-AC58U (IPQ4018) which would
no longer boot, if a gpio-hog was specified. (Tried forcing
the USB LED PIN (GPIO0) to high.).

The problem is that Pinctrl+GPIO registration is currently
peformed in the following order in pinctrl-msm.c:
	1. pinctrl_register()
	2. gpiochip_add()
	3. gpiochip_add_pin_range()

The actual error code -517 == -EPROBE_DEFER is coming from
pinctrl_get_device_gpio_range(), which is called through:
        gpiochip_add
            of_gpiochip_add
                of_gpiochip_scan_gpios
                    gpiod_hog
                        gpiochip_request_own_desc
                            __gpiod_request
                                chip->request
                                    gpiochip_generic_request
                                       pinctrl_gpio_request
                                          pinctrl_get_device_gpio_range

pinctrl_get_device_gpio_range() is unable to find any valid
pin ranges, since nothing has been added to the pinctrldev_list yet.
so the range can't be found, and the operation fails with -EPROBE_DEFER.

This patch fixes the issue by adding the "gpio-ranges" property to
the pinctrl device node of all upstream Qcom SoC. The pin ranges are
then added by the gpio core.

In order to remain compatible with older, existing DTs (and ACPI)
a check for the "gpio-ranges" property has been added to
msm_gpio_init(). This prevents the driver of adding the same entry
to the pinctrldev_list twice.

Reported-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
Tested-by: Sven Eckelmann <sven.eckelmann@openmesh.com> [ipq4019]
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ad80a17c9990..ace2bfbf1bee 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -890,11 +890,24 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add pin range\n");
-		gpiochip_remove(&pctrl->chip);
-		return ret;
+	/*
+	 * For DeviceTree-supported systems, the gpio core checks the
+	 * pinctrl's device node for the "gpio-ranges" property.
+	 * If it is present, it takes care of adding the pin ranges
+	 * for the driver. In this case the driver can skip ahead.
+	 *
+	 * In order to remain compatible with older, existing DeviceTree
+	 * files which don't set the "gpio-ranges" property or systems that
+	 * utilize ACPI the driver has to call gpiochip_add_pin_range().
+	 */
+	if (!of_property_read_bool(pctrl->dev->of_node, "gpio-ranges")) {
+		ret = gpiochip_add_pin_range(&pctrl->chip,
+			dev_name(pctrl->dev), 0, 0, chip->ngpio);
+		if (ret) {
+			dev_err(pctrl->dev, "Failed to add pin range\n");
+			gpiochip_remove(&pctrl->chip);
+			return ret;
+		}
 	}
 
 	ret = gpiochip_irqchip_add(chip,
-- 
2.17.0

^ permalink raw reply related

* [PATCH v5 1/4] dt-bindings: pinctrl: qcom: add gpio-ranges, gpio-reserved-ranges
From: Christian Lamparter @ 2018-05-21 20:57 UTC (permalink / raw)
  To: linux-gpio, linux-arm-kernel, linux-arm-msm, devicetree
  Cc: Mark Rutland, Stephen Boyd, Linus Walleij, Bjorn Andersson,
	David Brown, Rob Herring, Sven Eckelmann, Andy Gross

This patch adds the gpio-ranges and gpio-reserved-ranges property
definitions to the binding text files supported by the pinctrl-msm
driver framework.

gpio-ranges:
For DT-based platforms the pinctrl-msm framework currently relies
on the deprecated-for-DT gpiochip_add_pin_range() function to add
the range of GPIOs to be handled by the pin controller. Due to
interactions within gpiolib code, this causes the pinctrl-msm
driver to bail out (-517) during boot when a gpio-hog is declared.
This can be fatal and cause the system to not boot or reset
(for a detailed explanation and call-trace, refer to patch:
"pinctrl: msm: fix gpio-hog related boot issues" in this series).

gpio-reserved-ranges:
The binding has been added as a precaution since the TrustZone
firmware (aka QSEE), which is running as the hypervisor, might
have reserved certain, but undisclosed pins. Hence reading or
writing to the registers for those pins will cause an
XPU violation and this subsequently crashes the kernel.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 .../bindings/pinctrl/qcom,apq8064-pinctrl.txt         |  6 ++++++
 .../bindings/pinctrl/qcom,apq8084-pinctrl.txt         | 11 +++++++++++
 .../bindings/pinctrl/qcom,ipq4019-pinctrl.txt         |  6 ++++++
 .../bindings/pinctrl/qcom,ipq8064-pinctrl.txt         |  6 ++++++
 .../bindings/pinctrl/qcom,ipq8074-pinctrl.txt         | 10 ++++++++++
 .../bindings/pinctrl/qcom,mdm9615-pinctrl.txt         | 11 +++++++++++
 .../bindings/pinctrl/qcom,msm8660-pinctrl.txt         |  6 ++++++
 .../bindings/pinctrl/qcom,msm8916-pinctrl.txt         | 11 +++++++++++
 .../bindings/pinctrl/qcom,msm8960-pinctrl.txt         | 11 +++++++++++
 .../bindings/pinctrl/qcom,msm8974-pinctrl.txt         |  6 ++++++
 .../bindings/pinctrl/qcom,msm8994-pinctrl.txt         | 11 +++++++++++
 .../bindings/pinctrl/qcom,msm8996-pinctrl.txt         | 11 +++++++++++
 12 files changed, 106 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,apq8064-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,apq8064-pinctrl.txt
index a752a4716486..7f78c6bb4e35 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,apq8064-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,apq8064-pinctrl.txt
@@ -10,6 +10,11 @@ Required properties:
 - #gpio-cells : Should be two.
                 The first cell is the gpio pin number and the
                 second cell is used for optional parameters.
+- gpio-ranges: Range of pins managed by the GPIO controller.
+
+Optional properties:
+
+- gpio-reserved-ranges: Range of pins reserved by the TrustZone TEE.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
@@ -67,6 +72,7 @@ Example:
 
 		pinctrl-names = "default";
 		pinctrl-0 = <&gsbi5_uart_default>;
+		gpio-ranges = <&msmgpio 0 0 90>;
 
 		gsbi5_uart_default: gsbi5_uart_default {
 			mux {
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,apq8084-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,apq8084-pinctrl.txt
index c4ea61ac56f2..362f32b945af 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,apq8084-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,apq8084-pinctrl.txt
@@ -40,6 +40,16 @@ MSM8960 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
@@ -154,6 +164,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&tlmm 0 0 147>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 208 0>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,ipq4019-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,ipq4019-pinctrl.txt
index 93374f478b9e..7323bc54a1a0 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,ipq4019-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,ipq4019-pinctrl.txt
@@ -13,6 +13,11 @@ Required properties:
 - #gpio-cells : Should be two.
                 The first cell is the gpio pin number and the
                 second cell is used for optional parameters.
+- gpio-ranges: Range of pins managed by the GPIO controller.
+
+Optional properties:
+
+- gpio-reserved-ranges: Range of pins reserved by the TrustZone TEE.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
@@ -64,6 +69,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&tlmm 0 0 100>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 208 0>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,ipq8064-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,ipq8064-pinctrl.txt
index 6e88e91feb11..e6843ef15169 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,ipq8064-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,ipq8064-pinctrl.txt
@@ -10,6 +10,11 @@ Required properties:
 - #gpio-cells : Should be two.
                 The first cell is the gpio pin number and the
                 second cell is used for optional parameters.
+- gpio-ranges: Range of pins managed by the GPIO controller.
+
+Optional properties:
+
+- gpio-reserved-ranges: Range of pins reserved by the TrustZone TEE.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
@@ -67,6 +72,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&pinmux 0 0 69>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 32 0x4>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,ipq8074-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,ipq8074-pinctrl.txt
index 407b9443629d..e4d7ebcda0b0 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,ipq8074-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,ipq8074-pinctrl.txt
@@ -39,6 +39,15 @@ IPQ8074 platform.
 	Value type: <u32>
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
@@ -148,6 +157,7 @@ Example:
 		interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&tlmm 0 0 70>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,mdm9615-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,mdm9615-pinctrl.txt
index 1b52f01ddcb7..6a104f7800f9 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,mdm9615-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,mdm9615-pinctrl.txt
@@ -40,6 +40,16 @@ MDM9615 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
@@ -127,6 +137,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&msmgpio 0 0 88>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 16 0x4>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8660-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8660-pinctrl.txt
index df9a838ec5f9..311ea80a0101 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8660-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8660-pinctrl.txt
@@ -10,6 +10,11 @@ Required properties:
 - #gpio-cells : Should be two.
                 The first cell is the gpio pin number and the
                 second cell is used for optional parameters.
+- gpio-ranges: Range of pins managed by the GPIO controller.
+
+Optional properties:
+
+- gpio-reserved-ranges: Range of pins reserved by the TrustZone TEE.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
@@ -62,6 +67,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&msmgpio 0 0 173>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 16 0x4>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8916-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8916-pinctrl.txt
index 498caff6029e..99d1629f0940 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8916-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8916-pinctrl.txt
@@ -40,6 +40,16 @@ MSM8916 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
@@ -162,6 +172,7 @@ Example:
 		interrupts = <0 208 0>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&tlmm 0 0 122>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8960-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8960-pinctrl.txt
index eb8d8aa41f20..b1ad096ad60d 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8960-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8960-pinctrl.txt
@@ -40,6 +40,16 @@ MSM8960 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
@@ -156,6 +166,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&msmgpio 0 0 152>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 16 0x4>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8974-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8974-pinctrl.txt
index 453bd7c76d6b..54eda96a0d95 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8974-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8974-pinctrl.txt
@@ -10,6 +10,11 @@ Required properties:
 - #gpio-cells : Should be two.
                 The first cell is the gpio pin number and the
                 second cell is used for optional parameters.
+- gpio-ranges: Range of pins managed by the GPIO controller.
+
+Optional properties:
+
+- gpio-reserved-ranges: Range of pins reserved by the TrustZone TEE.
 
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
@@ -87,6 +92,7 @@ Example:
 
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&msmgpio 0 0 146>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		interrupts = <0 208 0>;
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8994-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8994-pinctrl.txt
index 13cd629f896e..58208f2eb455 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8994-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8994-pinctrl.txt
@@ -42,6 +42,16 @@ MSM8994 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
@@ -160,6 +170,7 @@ Example:
 		interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&msmgpio 0 0 146>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt
index aaf01e929eea..65f5e901ee1a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt
@@ -40,6 +40,16 @@ MSM8996 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- gpio-ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  Range of pins managed by the GPIO controller.
+
+- gpio-reserved-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Range of pins reserved by the TrustZone TEE.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
@@ -180,6 +190,7 @@ Example:
 		reg = <0x01010000 0x300000>;
 		interrupts = <0 208 0>;
 		gpio-controller;
+		gpio-ranges = <&tlmm 0 0 150>;
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
-- 
2.17.0

^ permalink raw reply related

* Re: [PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Wolfram Sang @ 2018-05-21 20:49 UTC (permalink / raw)
  To: Karthikeyan Ramasubramanian
  Cc: corbet, andy.gross, david.brown, robh+dt, mark.rutland, linux-doc,
	linux-arm-msm, devicetree, linux-i2c, evgreen, acourbot, swboyd,
	dianders, Sagar Dharia, Girish Mahadevan
In-Reply-To: <1521836461-6515-4-git-send-email-kramasub@codeaurora.org>

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

Hi,

On Fri, Mar 23, 2018 at 02:20:59PM -0600, Karthikeyan Ramasubramanian wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
> 
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>

Is one of these people interested in maintaining this driver? Then, an
entry for MAINTAINERS would be needed, too. (Same goes for
drivers/soc/qcom/ IMHO, but this is not my realm, so just saying)

> +static const struct geni_i2c_err_log gi2c_log[] = {
> +	[GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"},
> +	[NACK] = {-ENOTCONN, "NACK: slv unresponsive, check its power/reset-ln"},
> +	[GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"},
> +	[BUS_PROTO] = {-EPROTO, "Bus proto err, noisy/unepxected start/stop"},
> +	[ARB_LOST] = {-EBUSY, "Bus arbitration lost, clock line undriveable"},
> +	[GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"},
> +	[GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"},
> +	[GENI_ILLEGAL_CMD] = {-EILSEQ, "Illegal cmd, check GENI cmd-state machine"},
> +	[GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"},
> +	[GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"},
> +};

Please check Documentation/i2c/fault-codes for better -ERRNO values,
especially for NACK and ARB_LOST.

Rest looks good from a glimpse.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Andy Shevchenko @ 2018-05-21 20:44 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, devicetree,
	Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <20180521180927.18472-2-dmurphy@ti.com>

On Mon, May 21, 2018 at 9:09 PM, Dan Murphy <dmurphy@ti.com> wrote:
> Introduce the family of LED devices that can
> drive a torch, strobe or IR LED.
>
> The LED driver can be configured with a strobe
> timer to execute a strobe flash.  The IR LED
> brightness is controlled via the torch brightness
> register.
>
> The data sheet for each the LM36010 and LM36011
> LED drivers can be found here:
> http://www.ti.com/product/LM36010
> http://www.ti.com/product/LM36011

Thanks for an update. My comments below.

> +config LEDS_LM3601X
> +       tristate "LED support for LM3601x Chips"
> +       depends on LEDS_CLASS && I2C && OF

Now OF here is superfluous.

> +       depends on LEDS_CLASS_FLASH
> +       select REGMAP_I2C
> +       help
> +         This option enables support for the TI LM3601x family
> +         of flash, torch and indicator classes.

> +#define LM3601X_TIMEOUT_MASK   0x1e
> +#define LM3601X_ENABLE_MASK    0x03

I dunno if GENMASK() will be better here.

> +#define LM3601X_LOWER_STEP_US  40000
> +#define LM3601X_UPPER_STEP_US  200000
> +#define LM3601X_MIN_TIMEOUT_US 40000
> +#define LM3601X_MAX_TIMEOUT_US 1600000

> +#define LM3601X_TIMEOUT_XOVER  400000

Missed unit?

> +       ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +                               LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
> +                               led_mode_val);

> +               ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
> +                                       LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
> +                                       LM3601X_MODE_STROBE);

Perhaps
#define ..._MODE_TORCH_WITH_IR   (..._TOCRH | ..._IR_DRV)
?

> +static int lm3601x_probe(struct i2c_client *client,
> +                       const struct i2c_device_id *id)
> +{
> +       struct lm3601x_led *led;
> +       int err;

> +       err = lm3601x_parse_node(led, client->dev.of_node);
> +       if (err < 0)

> +               return -ENODEV;

Shouldn't be

return err;

?

> +       err = lm3601x_register_leds(led);
> +
> +       return err;

I will leave this to maintainers since you seems have strong
objections against more or less standard pattern, i.e.

return lm3601x_register_leds();

> +}

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* [v4 07/11] dt-bindings: hwmon: Add documents for PECI hwmon client drivers
From: Jae Hyun Yoo @ 2018-05-21 19:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Haiyue Wang, Vernon Mauery,
	James Feist, devicetree, linux-kernel
  Cc: Jae Hyun Yoo, Andrew Jeffery, Arnd Bergmann, Jason M Biils,
	Joel Stanley

This commit adds dt-bindings documents for PECI hwmon client drivers.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Haiyue Wang <haiyue.wang@linux.intel.com>
Reviewed-by: James Feist <james.feist@linux.intel.com>
Reviewed-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jason M Biils <jason.m.bills@linux.intel.com>
Cc: Joel Stanley <joel@jms.id.au>
---
 .../bindings/hwmon/peci-cputemp.txt           | 23 ++++++++++++++++++
 .../bindings/hwmon/peci-dimmtemp.txt          | 24 +++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
 create mode 100644 Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt

diff --git a/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt b/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
new file mode 100644
index 000000000000..2f59aee12d9e
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
@@ -0,0 +1,23 @@
+Bindings for Intel PECI (Platform Environment Control Interface) cputemp driver.
+
+Required properties:
+- compatible : Should be "intel,peci-cputemp".
+- reg        : Should contain address of a client CPU. Address range of CPU
+	       clients is starting from 0x30 based on PECI specification.
+
+Example:
+	peci-bus@0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		< more properties >
+
+		peci-cputemp@30 {
+			compatible = "intel,peci-cputemp";
+			reg = <0x30>;
+		};
+
+		peci-cputemp@31 {
+			compatible = "intel,peci-cputemp";
+			reg = <0x31>;
+		};
+	};
diff --git a/Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt b/Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt
new file mode 100644
index 000000000000..4592ce23f620
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt
@@ -0,0 +1,24 @@
+Bindings for Intel PECI (Platform Environment Control Interface) dimmtemp
+driver.
+
+Required properties:
+- compatible : Should be "intel,peci-dimmtemp".
+- reg        : Should contain address of a client CPU. Address range of CPU
+	       clients is starting from 0x30 based on PECI specification.
+
+Example:
+	peci-bus@0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		< more properties >
+
+		peci-dimmtemp@30 {
+			compatible = "intel,peci-dimmtemp";
+			reg = <0x30>;
+		};
+
+		peci-dimmtemp@31 {
+			compatible = "intel,peci-dimmtemp";
+			reg = <0x31>;
+		};
+	};
-- 
2.17.0

^ permalink raw reply related

* [v4 05/11] ARM: dts: aspeed: peci: Add PECI node
From: Jae Hyun Yoo @ 2018-05-21 19:58 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Joel Stanley, Andrew Jeffery,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel
  Cc: Jae Hyun Yoo, Jason M Biils, Ryan Chen

This commit adds PECI bus/adapter node of AST24xx/AST25xx into
aspeed-g4 and aspeed-g5.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Haiyue Wang <haiyue.wang@linux.intel.com>
Reviewed-by: James Feist <james.feist@linux.intel.com>
Reviewed-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Cc: Jason M Biils <jason.m.bills@linux.intel.com>
Cc: Ryan Chen <ryan_chen@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-g4.dtsi | 26 ++++++++++++++++++++++++++
 arch/arm/boot/dts/aspeed-g5.dtsi | 26 ++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 518d2bc7c7fc..c101601429b4 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -29,6 +29,7 @@
 		serial3 = &uart4;
 		serial4 = &uart5;
 		serial5 = &vuart;
+		peci0 = &peci0;
 	};
 
 	cpus {
@@ -270,6 +271,13 @@
 				};
 			};
 
+			peci: peci@1e78b000 {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0x0 0x1e78b000 0x60>;
+			};
+
 			uart2: serial@1e78d000 {
 				compatible = "ns16550a";
 				reg = <0x1e78d000 0x20>;
@@ -313,6 +321,24 @@
 	};
 };
 
+&peci {
+	peci0: peci-bus@0 {
+		compatible = "aspeed,ast2400-peci";
+		reg = <0x0 0x60>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <15>;
+		clocks = <&syscon ASPEED_CLK_GATE_REFCLK>;
+		resets = <&syscon ASPEED_RESET_PECI>;
+		clock-frequency = <24000000>;
+		msg-timing = <1>;
+		addr-timing = <1>;
+		rd-sampling-point = <8>;
+		cmd-timeout-ms = <1000>;
+		status = "disabled";
+	};
+};
+
 &i2c {
 	i2c_ic: interrupt-controller@0 {
 		#interrupt-cells = <1>;
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index f9917717dd08..0c4850c3f456 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -29,6 +29,7 @@
 		serial3 = &uart4;
 		serial4 = &uart5;
 		serial5 = &vuart;
+		peci0 = &peci0;
 	};
 
 	cpus {
@@ -320,6 +321,13 @@
 				};
 			};
 
+			peci: peci@1e78b000 {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0x0 0x1e78b000 0x60>;
+			};
+
 			uart2: serial@1e78d000 {
 				compatible = "ns16550a";
 				reg = <0x1e78d000 0x20>;
@@ -363,6 +371,24 @@
 	};
 };
 
+&peci {
+	peci0: peci-bus@0 {
+		compatible = "aspeed,ast2500-peci";
+		reg = <0x0 0x60>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <15>;
+		clocks = <&syscon ASPEED_CLK_GATE_REFCLK>;
+		resets = <&syscon ASPEED_RESET_PECI>;
+		clock-frequency = <24000000>;
+		msg-timing = <1>;
+		addr-timing = <1>;
+		rd-sampling-point = <8>;
+		cmd-timeout-ms = <1000>;
+		status = "disabled";
+	};
+};
+
 &i2c {
 	i2c_ic: interrupt-controller@0 {
 		#interrupt-cells = <1>;
-- 
2.17.0

^ permalink raw reply related

* [v4 04/11] dt-bindings: Add a document of PECI adapter driver for ASPEED AST24xx/25xx SoCs
From: Jae Hyun Yoo @ 2018-05-21 19:58 UTC (permalink / raw)
  To: Jason M Biils, Rob Herring, Mark Rutland, Joel Stanley,
	Andrew Jeffery, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel
  Cc: Jae Hyun Yoo, Benjamin Herrenschmidt, Greg KH, Milton Miller II,
	Pavel Machek, Robin Murphy, Ryan Chen

This commit adds a dt-bindings document of PECI adapter driver for ASPEED
AST24xx/25xx SoCs.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Haiyue Wang <haiyue.wang@linux.intel.com>
Reviewed-by: James Feist <james.feist@linux.intel.com>
Reviewed-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Jason M Biils <jason.m.bills@linux.intel.com>
Cc: Milton Miller II <miltonm@us.ibm.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../devicetree/bindings/peci/peci-aspeed.txt  | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt

diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
new file mode 100644
index 000000000000..8c35f905589d
--- /dev/null
+++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
@@ -0,0 +1,57 @@
+Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
+
+Required properties:
+- compatible        : Should be "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
+		      - aspeed,ast2400-peci: ASPEED AST2400 family PECI
+					     controller
+		      - aspeed,ast2500-peci: ASPEED AST2500 family PECI
+					     controller
+- reg               : Should contain PECI controller registers location and
+		      length.
+- #address-cells    : Should be <1> required to define a client address.
+- #size-cells       : Should be <0> required to define a client address.
+- interrupts        : Should contain PECI controller interrupt.
+- clocks            : Should contain clock source for PECI controller. Should
+		      reference the external oscillator clock in the second
+		      cell.
+- resets            : Should contain phandle to reset controller with the reset
+		      number in the second cell.
+- clock-frequency   : Should contain the operation frequency of PECI controller
+		      in units of Hz.
+		      187500 ~ 24000000
+
+Optional properties:
+- msg-timing        : Message timing negotiation period. This value will
+		      determine the period of message timing negotiation to be
+		      issued by PECI controller. The unit of the programmed
+		      value is four times of PECI clock period.
+		      0 ~ 255 (default: 1)
+- addr-timing       : Address timing negotiation period. This value will
+		      determine the period of address timing negotiation to be
+		      issued by PECI controller. The unit of the programmed
+		      value is four times of PECI clock period.
+		      0 ~ 255 (default: 1)
+- rd-sampling-point : Read sampling point selection. The whole period of a bit
+		      time will be divided into 16 time frames. This value will
+		      determine the time frame in which the controller will
+		      sample PECI signal for data read back. Usually in the
+		      middle of a bit time is the best.
+		      0 ~ 15 (default: 8)
+- cmd-timeout-ms    : Command timeout in units of ms.
+		      1 ~ 60000 (default: 1000)
+
+Example:
+	peci0: peci-bus@0 {
+		compatible = "aspeed,ast2500-peci";
+		reg = <0x0 0x60>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <15>;
+		clocks = <&syscon ASPEED_CLK_GATE_REFCLK>;
+		resets = <&syscon ASPEED_RESET_PECI>;
+		clock-frequency = <24000000>;
+		msg-timing = <1>;
+		addr-timing = <1>;
+		rd-sampling-point = <8>;
+		cmd-timeout-ms = <1000>;
+	};
-- 
2.17.0

^ permalink raw reply related

* [v4 01/11] dt-bindings: Add a document of PECI subsystem
From: Jae Hyun Yoo @ 2018-05-21 19:57 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, devicetree, linux-kernel
  Cc: Jae Hyun Yoo, Andrew Jeffery, Joel Stanley

This commit adds a document of generic PECI bus, adapter and client
driver.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Haiyue Wang <haiyue.wang@linux.intel.com>
Reviewed-by: James Feist <james.feist@linux.intel.com>
Reviewed-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Joel Stanley <joel@jms.id.au>
---
 .../devicetree/bindings/peci/peci.txt         | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/peci/peci.txt

diff --git a/Documentation/devicetree/bindings/peci/peci.txt b/Documentation/devicetree/bindings/peci/peci.txt
new file mode 100644
index 000000000000..72fc8ddd6a55
--- /dev/null
+++ b/Documentation/devicetree/bindings/peci/peci.txt
@@ -0,0 +1,59 @@
+Generic device tree configuration for PECI buses
+================================================
+
+Required properties:
+- compatible     : Should be "simple-bus".
+- #address-cells : Should be present if the device has sub-nodes.
+- #size-cells    : Should be present if the device has sub-nodes.
+- ranges         : Should contain PECI controller registers ranges.
+
+Example:
+	peci: peci@10000000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10000000 0x1000>;
+	};
+
+Generic device tree configuration for PECI adapters
+===================================================
+
+Required properties:
+- #address-cells : Should be <1>. Read more about client addresses below.
+- #size-cells    : Should be <0>. Read more about client addresses below.
+
+The cells properties above define that an address of CPU clients of a PECI bus
+are described by a single value.
+
+Example:
+	peci0: peci-bus@0 {
+		compatible = "soc,soc-peci";
+		reg = <0x0 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+Generic device tree configuration for PECI clients
+==================================================
+
+Required properties:
+- compatible : Should contain name of PECI client.
+- reg        : Should contain address of a client CPU. Address range of CPU
+	       clients is starting from 0x30 based on PECI specification.
+
+Example:
+	peci-bus@0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		< more properties >
+
+		function@30 {
+			compatible = "device,function";
+			reg = <0x30>;
+		};
+
+		function@31 {
+			compatible = "device,function";
+			reg = <0x31>;
+		};
+	};
-- 
2.17.0

^ permalink raw reply related

* [PATCH v4 00/10] PECI device driver introduction
From: Jae Hyun Yoo @ 2018-05-21 19:56 UTC (permalink / raw)
  To: Alan Cox, Andrew Jeffery, Andrew Lunn, Andy Shevchenko,
	Arnd Bergmann, Benjamin Herrenschmidt, Fengguang Wu, Greg KH,
	Guenter Roeck, Haiyue Wang, James Feist, Jason M Biils,
	Jean Delvare, Joel Stanley, Julia Cartwright, Miguel Ojeda,
	Milton Miller II, Pavel Machek, Randy Dunlap,
	Ryan Chen <ryan_chen@
  Cc: linux-kernel, linux-doc, devicetree, linux-hwmon,
	linux-arm-kernel, linux-aspeed, openbmc, Jae Hyun Yoo

Introduction of the Platform Environment Control Interface (PECI) bus
device driver. PECI is a one-wire bus interface that provides a
communication channel between an Intel processor and chipset components to
external monitoring or control devices. PECI is designed to support the
following sideband functions:

* Processor and DRAM thermal management
  - Processor fan speed control is managed by comparing Digital Thermal
    Sensor (DTS) thermal readings acquired via PECI against the
    processor-specific fan speed control reference point, or TCONTROL. Both
    TCONTROL and DTS thermal readings are accessible via the processor PECI
    client. These variables are referenced to a common temperature, the TCC
    activation point, and are both defined as negative offsets from that
    reference.
  - PECI based access to the processor package configuration space provides
    a means for Baseboard Management Controllers (BMC) or other platform
    management devices to actively manage the processor and memory power
    and thermal features.

* Platform Manageability
  - Platform manageability functions including thermal, power, and error
    monitoring. Note that platform 'power' management includes monitoring
    and control for both the processor and DRAM subsystem to assist with
    data center power limiting.
  - PECI allows read access to certain error registers in the processor MSR
    space and status monitoring registers in the PCI configuration space
    within the processor and downstream devices.
  - PECI permits writes to certain registers in the processor PCI
    configuration space.

* Processor Interface Tuning and Diagnostics
  - Processor interface tuning and diagnostics capabilities
    (Intel Interconnect BIST). The processors Intel Interconnect Built In
    Self Test (Intel IBIST) allows for infield diagnostic capabilities in
    the Intel UPI and memory controller interfaces. PECI provides a port to
    execute these diagnostics via its PCI Configuration read and write
    capabilities.

* Failure Analysis
  - Output the state of the processor after a failure for analysis via
    Crashdump.

PECI uses a single wire for self-clocking and data transfer. The bus
requires no additional control lines. The physical layer is a self-clocked
one-wire bus that begins each bit with a driven, rising edge from an idle
level near zero volts. The duration of the signal driven high depends on
whether the bit value is a logic '0' or logic '1'. PECI also includes
variable data transfer rate established with every message. In this way, it
is highly flexible even though underlying logic is simple.

The interface design was optimized for interfacing between an Intel
processor and chipset components in both single processor and multiple
processor environments. The single wire interface provides low board
routing overhead for the multiple load connections in the congested routing
area near the processor and chipset components. Bus speed, error checking,
and low protocol overhead provides adequate link bandwidth and reliability
to transfer critical device operating conditions and configuration
information.

This implementation provides the basic framework to add PECI extensions to
the Linux bus and device models. A hardware specific 'Adapter' driver can
be attached to the PECI bus to provide sideband functions described above.
It is also possible to access all devices on an adapter from userspace
through the /dev interface. A device specific 'Client' driver also can be
attached to the PECI bus so each processor client's features can be
supported by the 'Client' driver through an adapter connection in the bus.
This patch set includes Aspeed 24xx/25xx PECI driver and PECI
cputemp/dimmtemp drivers as the first implementation for both adapter and
client drivers on the PECI bus framework.

Please review.

Thanks,

-Jae

Changes from v3:
* Made code more simple and compact.
* Removed unused header file inclusion.
* Fixed incorrect error return values and messages.
* Removed DTS margin temperature from the peci-cputemp.
* Made some magic numbers use defines.
* Moved peci_get_cpu_id() into peci-core as a common function.
* Replaced the cancel_delayed_work() call with a cancel_delayed_work_sync().
* Replaced AST and Aspeed uses with ASPEED.
* Simplified peci command timeout checking logic using
  regmap_read_poll_timeout().
* Simplified endian swap codes using endian handling macros.
* Dropped regmap read/write error checking except for the first access.
* Added a PECI reset setting in the device tree node.
* Removed unnecessary sleep from the probe context.
* Removed IRQF_SHARED flag from irq request code in the ASPEED PECI driver.
* Fixed typos in documents.
* Combined peci-bus.txt, peci-adapter.txt and peci-client.txt into peci.txt.
* Fixed and swept documents to drop some incorrect or unnecessary
  descriptions.
* Fixed device tree to make unit-address format use reg contents.
* Simplified bit manipulations using <linux/bitfield.h>.
* Made client CPU model checking use <asm/intel-family.h> if available.
* Modified adapter heap allocation method to use kobject reference count
  based.
* Added the low-level PECI xfer IOCTL again to support the Redfish
  requirement.
* Added PM domain attach/detach code.
* Added logic for device instantiation through sysfs.
* Fix a bug of interrupt status checking code in peci-aspeed driver.

Changes from v2:
* Divided peci-hwmon driver into two drivers, peci-cputemp and
  peci-dimmtemp.
* Added generic dt binding documents for PECI bus, adapter and client.
* Removed in_atomic() call from the PECI core driver.
* Improved PECI commands masking logic.
* Added permission check logic for PECI ioctls.
* Removed unnecessary type casts.
* Fixed some invalid error return codes.
* Added the mark_updated() function to improve update interval checking
  logic.
* Fixed a bug in populated DIMM checking function.
* Fixed some typo, grammar and style issues in documents.
* Rewrote hwmon drivers to use devm_hwmon_device_register_with_info API.
* Made peci_match_id() function as a static.
* Replaced a deprecated create_singlethread_workqueue() call with an
  alloc_ordered_workqueue() call.
* Reordered local variable definitions in reversed xmas tree notation.
* Listed up client CPUs that can be supported by peci-cputemp and
  peci-dimmtemp hwmon drivers.
* Added CPU generation detection logic which checks CPUID signature through
  PECI connection.
* Improved interrupt handling logic in the Aspeed PECI adapter driver.
* Fixed SPDX license identifier style in header files.
* Changed some macros in peci.h to static inline functions.
* Dropped sleepable context checking code in peci-core.
* Adjusted rt_mutex protection scope in peci-core.
* Moved adapter->xfer() checking code into peci_register_adapter().
* Improved PECI command retry checking logic.
* Changed ioctl base from 'P' to 0xb6 to avoid confiliction and updated
  ioctl-number.txt to reflect the ioctl number of PECI subsystem.
* Added a comment to describe PECI retry action.
* Simplified return code handling of peci_ioctl_ping().
* Changed type of peci_ioctl_fn[] to static const.
* Fixed range checking code for valid PECI commands.
* Fixed the error return code on invalid PECI commands.
* Fixed incorrect definitions of PECI ioctl and its handling logic.

Changes from v1:
* Additionally implemented a core driver to support PECI linux bus driver
  model.
* Modified Aspeed PECI driver to make that to be an adapter driver in PECI
  bus.
* Modified PECI hwmon driver to make that to be a client driver in PECI
  bus.
* Simplified hwmon driver attribute labels and removed redundant strings.
* Removed core_nums from device tree setting of hwmon driver and modified
  core number detection logic to check the resolved_core register in client
  CPU's local PCI configuration area.
* Removed dimm_nums from device tree setting of hwmon driver and added
  populated DIMM detection logic to support dynamic creation.
* Removed indexing gap on core temperature and DIMM temperature attributes.
* Improved hwmon registration and dynamic attribute creation logic.
* Fixed structure definitions in PECI uapi header to make that use __u8,
  __u16 and etc.
* Modified wait_for_completion_interruptible_timeout error handling logic
  in Aspeed PECI driver to deliver errors correctly.
* Removed low-level xfer command from ioctl and kept only high-level PECI
  command suite as ioctls.
* Fixed I/O timeout logic in Aspeed PECI driver using ktime.
* Added a function into hwmon driver to simplify update delay checking.
* Added a function into hwmon driver to convert 10.6 to millidegree.
* Dropped non-standard attributes in hwmon driver.
* Fixed OF table for hwmon to make it indicate as a PECI client of Intel
  CPU target.
* Added a maintainer of PECI subsystem into MAINTAINERS document.

Jae Hyun Yoo (11):
  dt-bindings: Add a document of PECI subsystem
  Documentation: ioctl: Add ioctl numbers for PECI subsystem
  drivers/peci: Add support for PECI bus driver core
  dt-bindings: Add a document of PECI adapter driver for ASPEED
    AST24xx/25xx SoCs
  ARM: dts: aspeed: peci: Add PECI node
  drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx
  dt-bindings: hwmon: Add documents for PECI hwmon client drivers
  Documentation: hwmon: Add documents for PECI hwmon client drivers
  drivers/hwmon: Add PECI cputemp driver
  drivers/hwmon: Add PECI dimmtemp driver
  Add maintainers for the PECI subsystem

 .../bindings/hwmon/peci-cputemp.txt           |   23 +
 .../bindings/hwmon/peci-dimmtemp.txt          |   24 +
 .../devicetree/bindings/peci/peci-aspeed.txt  |   57 +
 .../devicetree/bindings/peci/peci.txt         |   59 +
 Documentation/hwmon/peci-cputemp              |   78 +
 Documentation/hwmon/peci-dimmtemp             |   50 +
 Documentation/ioctl/ioctl-number.txt          |    2 +
 MAINTAINERS                                   |   10 +
 arch/arm/boot/dts/aspeed-g4.dtsi              |   26 +
 arch/arm/boot/dts/aspeed-g5.dtsi              |   26 +
 drivers/Kconfig                               |    2 +
 drivers/Makefile                              |    1 +
 drivers/hwmon/Kconfig                         |   28 +
 drivers/hwmon/Makefile                        |    2 +
 drivers/hwmon/peci-cputemp.c                  |  407 +++++
 drivers/hwmon/peci-dimmtemp.c                 |  300 ++++
 drivers/hwmon/peci-hwmon.c                    |  124 ++
 drivers/hwmon/peci-hwmon.h                    |   51 +
 drivers/peci/Kconfig                          |   44 +
 drivers/peci/Makefile                         |    9 +
 drivers/peci/peci-aspeed.c                    |  496 ++++++
 drivers/peci/peci-core.c                      | 1451 +++++++++++++++++
 include/linux/peci.h                          |  105 ++
 include/uapi/linux/peci-ioctl.h               |  265 +++
 24 files changed, 3640 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
 create mode 100644 Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt
 create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
 create mode 100644 Documentation/devicetree/bindings/peci/peci.txt
 create mode 100644 Documentation/hwmon/peci-cputemp
 create mode 100644 Documentation/hwmon/peci-dimmtemp
 create mode 100644 drivers/hwmon/peci-cputemp.c
 create mode 100644 drivers/hwmon/peci-dimmtemp.c
 create mode 100644 drivers/hwmon/peci-hwmon.c
 create mode 100644 drivers/hwmon/peci-hwmon.h
 create mode 100644 drivers/peci/Kconfig
 create mode 100644 drivers/peci/Makefile
 create mode 100644 drivers/peci/peci-aspeed.c
 create mode 100644 drivers/peci/peci-core.c
 create mode 100644 include/linux/peci.h
 create mode 100644 include/uapi/linux/peci-ioctl.h

-- 
2.17.0

^ permalink raw reply

* RE: [PATCH v7 3/3] arm64: defconfig: Enable dw_mmc-bluefield driver
From: Liming Sun @ 2018-05-21 19:35 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rob Herring, Mark Rutland, Jaehoon Chung, Catalin Marinas,
	Will Deacon, linux-mmc@vger.kernel.org,
	devicetree@vger.kernel.org, Linux Kernel Mailing List
In-Reply-To: <CAPDyKFqBqTjjjFSkRBLKECmdn-b01hv51LOtqSSSTseWYL=Ykw@mail.gmail.com>

Thanks! I'll try to send this one to arm soc.

Best regards,
Liming

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Monday, May 21, 2018 7:36 AM
> To: Liming Sun <lsun@mellanox.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; Jaehoon Chung <jh80.chung@samsung.com>;
> Catalin Marinas <catalin.marinas@arm.com>; Will Deacon
> <will.deacon@arm.com>; linux-mmc@vger.kernel.org;
> devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH v7 3/3] arm64: defconfig: Enable dw_mmc-bluefield
> driver
> 
> On 8 May 2018 at 20:46, Liming Sun <lsun@mellanox.com> wrote:
> > This patch updates arm64 defconfig to enable dw_mmc-bluefield,
> > which is a driver extension of Synopsys Designware MMC for the
> > Mellanox BlueField Soc.
> >
> > Signed-off-by: Liming Sun <lsun@mellanox.com>
> > Reviewed-by: David Woods <dwoods@mellanox.com>
> 
> I have applied the other parts in the series, but not this one as it's
> probably better it goes via arm soc.
> 
> I can take it, but then I need an ack from arm64 folkz...
> 
> Kind regards
> Uffe
> 
> > ---
> >  arch/arm64/configs/defconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index ecf6137..43464ab 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -487,6 +487,7 @@ CONFIG_MMC_DW=y
> >  CONFIG_MMC_DW_EXYNOS=y
> >  CONFIG_MMC_DW_K3=y
> >  CONFIG_MMC_DW_ROCKCHIP=y
> > +CONFIG_MMC_DW_BLUEFIELD=y
> >  CONFIG_MMC_SUNXI=y
> >  CONFIG_MMC_BCM2835=y
> >  CONFIG_MMC_SDHCI_XENON=y
> > --
> > 1.8.3.1
> >

^ permalink raw reply

* Re: [PATCH RFC] ARM: dts: add Raspberry Pi Compute Module and IO board
From: Stefan Wahren @ 2018-05-21 19:28 UTC (permalink / raw)
  To: Rob Herring, Eric Anholt, Mark Rutland
  Cc: devicetree, Florian Fainelli, Scott Branden, Ray Jui, Phil Elwell,
	bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
In-Reply-To: <87d0xouapx.fsf@anholt.net>

> Eric Anholt <eric@anholt.net> hat am 21. Mai 2018 um 20:26 geschrieben:
> 
> 
> Stefan Wahren <stefan.wahren@i2se.com> writes:
> 
> > The Raspberry Pi Compute Module (CM1) is a SoM which contains a
> > BCM2835 processor, 512 MB RAM and a 4 GB eMMC. There is also a carrier
> > board which is called Compute Module IO Board.
> >
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> >  arch/arm/boot/dts/Makefile                |  1 +
> >  arch/arm/boot/dts/bcm2835-rpi-cm1-io1.dts | 92 +++++++++++++++++++++++++++++++
> >  arch/arm/boot/dts/bcm2835-rpi-cm1.dtsi    | 34 ++++++++++++
> >  3 files changed, 127 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/bcm2835-rpi-cm1-io1.dts
> >  create mode 100644 arch/arm/boot/dts/bcm2835-rpi-cm1.dtsi
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index ec2024e..a9883e8 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -73,6 +73,7 @@ dtb-$(CONFIG_ARCH_BCM2835) += \
> >  	bcm2835-rpi-b-rev2.dtb \
> >  	bcm2835-rpi-b-plus.dtb \
> >  	bcm2835-rpi-a-plus.dtb \
> > +	bcm2835-rpi-cm1-io1.dtb \
> >  	bcm2836-rpi-2-b.dtb \
> >  	bcm2837-rpi-3-b.dtb \
> >  	bcm2837-rpi-3-b-plus.dtb \
> > diff --git a/arch/arm/boot/dts/bcm2835-rpi-cm1-io1.dts b/arch/arm/boot/dts/bcm2835-rpi-cm1-io1.dts
> > new file mode 100644
> > index 0000000..4d9aa22
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/bcm2835-rpi-cm1-io1.dts
> > @@ -0,0 +1,92 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/dts-v1/;
> > +#include "bcm2835-rpi-cm1.dtsi"
> > +#include "bcm283x-rpi-usb-host.dtsi"
> > +
> > +/ {
> > +	compatible = "raspberrypi,compute-module", "brcm,bcm2835";
> > +	model = "Raspberry Pi Compute Module IO board rev1";
> > +};
> > +
> > +&dsi1 {
> > +	status = "okay";
> > +};
> > +
> > +&gpio {
> > +	/*
> > +	 * This is based on the official GPU firmware DT blob.
> > +	 *
> > +	 * Legend:
> > +	 * "NC" = not connected (no rail from the SoC)
> > +	 * "FOO" = GPIO line named "FOO" on the schematic
> > +	 * "FOO_N" = GPIO line named "FOO" on schematic, active low
> > +	 */
> > +	gpio-line-names = "GPIO0",
> > +			  "GPIO1",
> > +			  "GPIO2",
> > +			  "GPIO3",
> > +			  "GPIO4",
> > +			  "GPIO5",
> > +			  "GPIO6",
> > +			  "GPIO7",
> > +			  "GPIO8",
> > +			  "GPIO9",
> > +			  "GPIO10",
> > +			  "GPIO11",
> > +			  "GPIO12",
> > +			  "GPIO13",
> > +			  "GPIO14",
> > +			  "GPIO15",
> > +			  "GPIO16",
> > +			  "GPIO17",
> > +			  "GPIO18",
> > +			  "GPIO19",
> > +			  "GPIO20",
> > +			  "GPIO21",
> > +			  "GPIO22",
> > +			  "GPIO23",
> > +			  "GPIO24",
> > +			  "GPIO25",
> > +			  "GPIO26",
> > +			  "GPIO27",
> > +			  "GPIO28",
> > +			  "GPIO29",
> > +			  "GPIO30",
> > +			  "GPIO31",
> > +			  "GPIO32",
> > +			  "GPIO33",
> > +			  "GPIO34",
> > +			  "GPIO35",
> > +			  "GPIO36",
> > +			  "GPIO37",
> > +			  "GPIO38",
> > +			  "GPIO39",
> > +			  "GPIO40",
> > +			  "GPIO41",
> > +			  "GPIO42",
> > +			  "GPIO43",
> > +			  "GPIO44",
> > +			  "GPIO45",
> > +			  "HDMI_HPD_N",
> > +			  /* Also used as ACT LED */
> > +			  "EMMC_EN_N",
> > +			  /* Used by eMMC */
> > +			  "SD_CLK_R",
> > +			  "SD_CMD_R",
> > +			  "SD_DATA0_R",
> > +			  "SD_DATA1_R",
> > +			  "SD_DATA2_R",
> > +			  "SD_DATA3_R";
> > +
> > +	pinctrl-0 = <&gpioout &alt0>;
> > +};
> > +
> > +&hdmi {
> > +	hpd-gpios = <&gpio 46 GPIO_ACTIVE_HIGH>;
> > +};
> 
> I think this should be ACTIVE_LOW, since it's "HDMI_HPD_N_1V8", right?

I just copy & paste from the rpi-4.14/bcm2708-rpi-cm.dts. I thought the HDMI interface on my IO board is broken, but maybe this is a downstream issue.

> 
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_gpio14>;
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm/boot/dts/bcm2835-rpi-cm1.dtsi b/arch/arm/boot/dts/bcm2835-rpi-cm1.dtsi
> > new file mode 100644
> > index 0000000..ef22c2d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/bcm2835-rpi-cm1.dtsi
> > @@ -0,0 +1,34 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/dts-v1/;
> > +#include "bcm2835.dtsi"
> > +#include "bcm2835-rpi.dtsi"
> > +
> > +/ {
> > +	leds {
> > +		act {
> > +			gpios = <&gpio 47 GPIO_ACTIVE_LOW>;
> > +		};
> > +	};
> > +
> > +	reg_3v3: fixed-regulator {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "3V3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-always-on;
> > +	};
> > +
> > +	reg_1v8: fixed-regulator {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "1V8";
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +		regulator-always-on;
> > +	};
> > +};
> > +
> > +&sdhost {
> > +	non-removable;
> > +	vmmc-supply = <&reg_3v3>;
> > +	vqmmc-supply = <&reg_1v8>;
> > +};
> 
> Also, looking at some datasheets I have laying around, it says "eMMC I/O
> Voltage fixed at 1V8" -- is this regulator setup right, in that case?

Usually an eMMC has 2 different voltage sources:
vqmmc-supply -> supply node for IO line power (usually switchable, but fixed on Compute Module)
vmmc-supply -> supply node for card's power (usually fixed)

Do you have a specific concern (voltage, naming)?

Does this conversation help [1]?

Please also look at the CM schematics page 1 and 3.

[1] - https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=213772

> 
> With answers for these two issues, it will be:
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From: skannan @ 2018-05-21 19:06 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Taniya Das, Rafael J. Wysocki, linux-kernel, linux-pm,
	Stephen Boyd, robh, Rajendra Nayak, Amit Nischal, devicetree,
	amit.kucheria
In-Reply-To: <20180521090113.qo2lrafjrh2xi6va@vireshk-i7>

On 2018-05-21 02:01, Viresh Kumar wrote:
> On 19-05-18, 23:04, Taniya Das wrote:
>> The CPUfreq FW present in some QCOM chipsets offloads the steps 
>> necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this firmware.
>> 
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>  drivers/cpufreq/Kconfig.arm       |   9 ++
>>  drivers/cpufreq/Makefile          |   1 +
>>  drivers/cpufreq/qcom-cpufreq-fw.c | 317 
>> ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 327 insertions(+)
>>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>> 
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 96b35b8..571f6b4 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -301,3 +301,12 @@ config ARM_PXA2xx_CPUFREQ
>>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>> 
>>  	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_FW
>> +	bool "QCOM CPUFreq FW driver"
> 
> During last review I didn't say that this driver shouldn't be a
> module, but that you need to fix things to make it a module. I am fine
> though if you don't want this to be a module ever.
> 
>> +	help
>> +	 Support for the CPUFreq FW driver.
>> +	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
>> +	 necessary for changing the frequency of CPUs. The driver
>> +	 implements the cpufreq driver interface for this firmware.
>> +	 Say Y if you want to support CPUFreq FW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index 8d24ade..a3edbce 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= 
>> tegra124-cpufreq.o
>>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
>> 
>> 
>>  
>> ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c 
>> b/drivers/cpufreq/qcom-cpufreq-fw.c
>> new file mode 100644
>> index 0000000..0e66de0
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>> @@ -0,0 +1,317 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE			300000000UL
>> +#define XO_RATE				19200000UL
>> +#define LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		((val & GENMASK(18, 16)) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
>> +	void __iomem *perf_base;
>> +	void __iomem *lut_base;
>> +	cpumask_t related_cpus;
>> +	unsigned int max_cores;
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy, unsigned 
>> int index)
>> +{
>> +	struct cpufreq_qcom *c = policy->driver_data;
>> +
>> +	if (index >= LUT_MAX_ENTRIES) {
>> +		dev_err(c->dev,
>> +			"Passing an index (%u) that's greater than max (%d)\n",
>> +					index, LUT_MAX_ENTRIES - 1);
>> +		return -EINVAL;
>> +	}
> 
> This is never going to happen unless there is a bug in cpufreq core.
> You are allocating only 40 entries for the cpufreq table and this will
> always be 0-39. None of the other drivers is checking this I believe
> and neither should you. This is the only routine which will get call
> very frequently and we better not add unnecessary stuff here.
> 
>> +	writel_relaxed(index, c->perf_base);
>> +
>> +	/* Make sure the write goes through before proceeding */
>> +	mb();
> 
> Btw what happens right after this is done ? Are we guaranteed that the
> frequency is updated in the hardware after this ? What about enabling
> fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
> to see how that is done.

Yeah, I don't think this is needed really.

> 
>> +	return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	unsigned int index;
>> +
>> +	c = qcom_freq_domain_map[cpu];
>> +	if (!c)
>> +		return -ENODEV;
> 
> Return 0 on error here.
> 
>> +
>> +	index = readl_relaxed(c->perf_base);
>> +	index = min(index, LUT_MAX_ENTRIES - 1);
> 
> Will the hardware ever read a value over 39 here ?

The register could be initialized to whatever before the kernel is 
brought up. Don't want to depend on it being correct to avoid out of 
bounds access that could leak data.


>> +
>> +	return c->table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	struct cpufreq_qcom *c;
>> +
>> +	c = qcom_freq_domain_map[policy->cpu];
>> +	if (!c) {
>> +		pr_err("No scaling support for CPU%d\n", policy->cpu);
>> +		return -ENODEV;
>> +	}
>> +
>> +	cpumask_copy(policy->cpus, &c->related_cpus);
>> +	policy->freq_table = c->table;
>> +	policy->driver_data = c;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_fw_attr[] = {
>> +	&cpufreq_freq_attr_scaling_available_freqs,
>> +	&cpufreq_freq_attr_scaling_boost_freqs,
>> +	NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
>> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +	.verify		= cpufreq_generic_frequency_table_verify,
>> +	.target_index	= qcom_cpufreq_fw_target_index,
>> +	.get		= qcom_cpufreq_fw_get,
>> +	.init		= qcom_cpufreq_fw_cpu_init,
>> +	.name		= "qcom-cpufreq-fw",
>> +	.attr		= qcom_cpufreq_fw_attr,
>> +	.boost_enabled	= true,
>> +};
>> +
>> +static int qcom_read_lut(struct platform_device *pdev,
>> +			 struct cpufreq_qcom *c)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	u32 data, src, lval, i, core_count, prev_cc;
>> +
>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +				sizeof(*c->table), GFP_KERNEL);
>> +	if (!c->table)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
>> +		src = ((data & GENMASK(31, 30)) >> 30);
>> +		lval = (data & GENMASK(7, 0));
>> +		core_count = CORE_COUNT_VAL(data);
>> +
>> +		if (!src)
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +		else
>> +			c->table[i].frequency = XO_RATE * lval / 1000;
>> +
>> +		c->table[i].driver_data = c->table[i].frequency;
> 
> Why do you need to use driver_data here? Why can't you simple use
> frequency field in the below conditional expressions ?
> 
>> +
>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +			i, c->table[i].frequency, core_count);
>> +
>> +		if (core_count != c->max_cores)
>> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
>> +
>> +		/*
>> +		 * Two of the same frequencies with the same core counts means
>> +		 * end of table.
>> +		 */
>> +		if (i > 0 && c->table[i - 1].driver_data ==
>> +			c->table[i].driver_data && prev_cc == core_count) {
>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> 
> There can only be a single boost frequency at max ?

As of now, yes. If that changes, we'll change this code later.

>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>> +				prev->frequency = prev->driver_data;
> 
> Okay you are using driver_data as a local variable to keep this value
> safe which you might have overwritten. Maybe use a simple variable
> prev_freq for this. It would be much more readable in that case and
> you wouldn't end up abusing the driver_data field.
> 
>> +			}
>> +
>> +			break;
>> +		}
>> +		prev_cc = core_count;
>> +	}
>> +	c->table[i].frequency = CPUFREQ_TABLE_END;
> 
> Wouldn't you end up writing on c->table[40].frequency here if there
> are 40 frequency value present ?

Yeah, the loop condition needs to be fixed.

-Saravana

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox