Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v2 6/8] dt-bindings: power: Add Actions Semi S700 SPS
From: Rob Herring @ 2017-11-16  4:35 UTC (permalink / raw)
  To: Andreas Färber
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Thomas Liau,
	Jeff Chen, 张东风, 刘炜,
	张天益, 梅利,
	support-om/0PrzuAKC1Z/+hSey0Gg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171113233427.5386-7-afaerber-l3A5Bk7waGM@public.gmane.org>

On Tue, Nov 14, 2017 at 12:34:25AM +0100, Andreas Färber wrote:
> Define power domains for Actions Semi S700 SoC Smart Power System (SPS).
> 
> Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
> ---
>  v2: New
>  
>  .../devicetree/bindings/power/actions,owl-sps.txt    |  2 ++
>  include/dt-bindings/power/owl-s700-powergate.h       | 20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+)
>  create mode 100644 include/dt-bindings/power/owl-s700-powergate.h
> 
> diff --git a/Documentation/devicetree/bindings/power/actions,owl-sps.txt b/Documentation/devicetree/bindings/power/actions,owl-sps.txt
> index 007b9a7ae723..78edd63641e8 100644
> --- a/Documentation/devicetree/bindings/power/actions,owl-sps.txt
> +++ b/Documentation/devicetree/bindings/power/actions,owl-sps.txt
> @@ -2,10 +2,12 @@ Actions Semi Owl Smart Power System (SPS)
>  
>  Required properties:
>  - compatible          :  "actions,s500-sps" for S500
> +                         "actions,s700-sps" for S700
>  - reg                 :  Offset and length of the register set for the device.
>  - #power-domain-cells :  Must be 1.
>                           See macros in:
>                            include/dt-bindings/power/owl-s500-powergate.h for S500
> +                          include/dt-bindings/power/owl-s700-powergate.h for S700
>  
>  
>  Example:
> diff --git a/include/dt-bindings/power/owl-s700-powergate.h b/include/dt-bindings/power/owl-s700-powergate.h
> new file mode 100644
> index 000000000000..4ded16bc074f
> --- /dev/null
> +++ b/include/dt-bindings/power/owl-s700-powergate.h
> @@ -0,0 +1,20 @@
> +/*
> + * Actions Semi S700 SPS
> + *
> + * Copyright (c) 2017 Andreas Färber
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)

The decree now for SPDX tags is make this the first line using "//".

Otherwise, 

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 1/3] DT: i2c: W83773G is a trivial device
From: Rob Herring @ 2017-11-16  4:33 UTC (permalink / raw)
  To: Lei YU
  Cc: Mark Rutland, Jean Delvare, Guenter Roeck, Jonathan Corbet,
	Jiri Kosina, devicetree, linux-kernel, linux-hwmon, Joel Stanley,
	Andrew Jeffery
In-Reply-To: <1510543655-27059-2-git-send-email-mine260309@gmail.com>

On Mon, Nov 13, 2017 at 11:27:32AM +0800, Lei YU wrote:
> Signed-off-by: Lei YU <mine260309@gmail.com>
> ---
>  Documentation/devicetree/bindings/trivial-devices.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.txt b/Documentation/devicetree/bindings/trivial-devices.txt
> index af284fb..63ad2f1 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/trivial-devices.txt
> @@ -188,3 +188,4 @@ ti,tmp103		Low Power Digital Temperature Sensor with SMBUS/Two Wire Serial Inter
>  ti,tmp275		Digital Temperature Sensor
>  winbond,w83793		Winbond/Nuvoton H/W Monitor
>  winbond,wpct301		i2c trusted platform module (TPM)
> +nuvoton,w83773g		Nuvoton Temperature Sensor

Keep alphabetical order please.

Send trivial-devices.txt changes separately to me and I'll take them to 
avoid any conflicts.

Rob

^ permalink raw reply

* Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver
From: Rob Herring @ 2017-11-16  4:30 UTC (permalink / raw)
  To: Tim Harvey
  Cc: devicetree@vger.kernel.org, alsa-devel,
	linux-kernel@vger.kernel.org, Hans Verkuil, Mauro Carvalho Chehab,
	Philipp Zabel, Steve Longerbeam, Hans Verkuil, Shawn Guo,
	linux-media
In-Reply-To: <CAJ+vNU0NuNjZePQBaxWHT4HtYGxZJ_DUSvuHvb6wAFDGA_xX9A@mail.gmail.com>

On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring <robh@kernel.org> wrote:
> > On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> >> Add support for the TDA1997x HDMI receivers.
> >>
> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >> v3:
> >>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
> >>  - fixed missing break
> >>  - use only hdmi_infoframe_log for infoframe logging
> >>  - simplify tda1997x_s_stream error handling
> >>  - add delayed work proc to handle hotplug enable/disable
> >>  - fix set_edid (disable HPD before writing, enable after)
> >>  - remove enabling edid by default
> >>  - initialize timings
> >>  - take quant range into account in colorspace conversion
> >>  - remove vendor/product tracking (we provide this in log_status via infoframes)
> >>  - add v4l_controls
> >>  - add more detail to log_status
> >>  - calculate vhref generator timings
> >>  - timing detection fixes (rounding errors, hswidth errors)
> >>  - rename configure_input/configure_conv functions
> >>
> >> v2:
> >>  - implement dv timings enum/cap
> >>  - remove deprecated g_mbus_config op
> >>  - fix dv_query_timings
> >>  - add EDID get/set handling
> >>  - remove max-pixel-rate support
> >>  - add audio codec DAI support
> >>  - change audio bindings
> >> ---
> >>  drivers/media/i2c/Kconfig            |    9 +
> >>  drivers/media/i2c/Makefile           |    1 +
> >>  drivers/media/i2c/tda1997x.c         | 3485 ++++++++++++++++++++++++++++++++++
> >>  include/dt-bindings/media/tda1997x.h |   78 +
> >
> > This belongs with the binding documentation patch.
> >
> 
> Rob,
> 
> Thanks - missed that. I will move it for v4.
> 
> Regarding your previous comment to the v2 series:
> > The rest of the binding looks fine, but I have some reservations about
> > this. I think this should be common probably. There's been a few
> > bindings for display recently that deal with the interface format. Maybe
> > some vendor property is needed here to map a standard interface format
> > back to pin configuration.
> 
> I take it this is not an 'Ack' for the bindings?
> 
> Which did you feel should be made common? I admit I was surprised
> there wasn't a common binding for audio bus format (i2s|spdif) but if
> you were referring to the video data that would probably be much more
> complicated.

The video data. Either you have to try to come up with some way to map 
color components to signals/pins (and even cycles) or you just enumerate 
the formats and keep adding to them when new ones appear. There's h/w 
that allows the former, but in the end you have to interoperate, so 
enumerating the formats is probably enough.

> I was hoping one of the media/driver maintainers would respond to your
> comment with thoughts as I'm not familiar with a very wide variety of
> receivers.

I am hoping, too.

Rob

^ permalink raw reply

* Re: [PATCH 2/2] leds: lm3692x: Introduce LM3692x dual string driver
From: Andrew F. Davis @ 2017-11-16  3:48 UTC (permalink / raw)
  To: Dan Murphy, robh+dt, mark.rutland, rpurdie, jacek.anaszewski,
	pavel
  Cc: devicetree, linux-kernel, linux-leds
In-Reply-To: <20171113205052.25884-2-dmurphy@ti.com>

On 11/13/2017 02:50 PM, Dan Murphy wrote:
> Introducing the LM3692x Dual-String white LED driver.
> 
> Data sheet is located
> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---

Looks like v3 is posted but I can't find it in my mailbox :/ but the
comments below should still be valid.

>  drivers/leds/leds-lm3692x.c | 380 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 380 insertions(+)
>  create mode 100644 drivers/leds/leds-lm3692x.c
> 
> diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
> new file mode 100644
> index 000000000000..beb753abc7b0
> --- /dev/null
> +++ b/drivers/leds/leds-lm3692x.c
> @@ -0,0 +1,380 @@
> +/*
> + * TI lm3692x LED Driver
> + *
> + * Copyright (C) 2017 Texas Instruments
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/slab.h>
> +
> +#define LM3692X_LED_NAME	"lm3692x_led"
> +
> +#define LM3692X_REV		0x0
> +#define LM3692X_RESET		0x1
> +#define LM3692X_EN		0x10
> +#define LM3692X_BRT_CTRL	0x11
> +#define LM3692X_PWM_CTRL	0x12
> +#define LM3692X_BOOST_CTRL	0x13
> +#define LM3692X_AUTO_FREQ_HI	0x15
> +#define LM3692X_AUTO_FREQ_LO	0x16
> +#define LM3692X_BL_ADJ_THRESH	0x17
> +#define LM3692X_BRT_LSB		0x18
> +#define LM3692X_BRT_MSB		0x19
> +#define LM3692X_FAULT_CTRL	0x1e
> +#define LM3692X_FAULT_FLAGS	0x1f
> +
> +#define LM3692X_SW_RESET	BIT(0)
> +#define LM3692X_DEVICE_EN	BIT(0)
> +#define LM3692X_LED1_EN		BIT(1)
> +#define LM3692X_LED2_EN		BIT(2)
> +
> +/* Brightness Control Bits */
> +#define LM3692X_BL_ADJ_POL	BIT(0)
> +#define LM3692X_RAMP_RATE_125us	0x00
> +#define LM3692X_RAMP_RATE_250us	BIT(1)
> +#define LM3692X_RAMP_RATE_500us BIT(2)
> +#define LM3692X_RAMP_RATE_1ms	(BIT(1) | BIT(2))
> +#define LM3692X_RAMP_RATE_2ms	BIT(3)
> +#define LM3692X_RAMP_RATE_4ms	(BIT(3) | BIT(1))
> +#define LM3692X_RAMP_RATE_8ms	(BIT(2) | BIT(3))
> +#define LM3692X_RAMP_RATE_16ms	(BIT(1) | BIT(2) | BIT(3))

The above look like you are just counting in binary, I'd just list the
numbers then shift them by one.

> +#define LM3692X_RAMP_EN		BIT(4)
> +#define LM3692X_BRHT_MODE_REG	0x00
> +#define LM3692X_BRHT_MODE_PWM	BIT(5)
> +#define LM3692X_BRHT_MODE_MULTI_RAMP BIT(6)
> +#define LM3692X_BRHT_MODE_RAMP_MULTI (BIT(5) | BIT(6))
> +#define LM3692X_MAP_MODE_EXP	BIT(7)
> +
> +/* PWM Register Bits */
> +#define LM3692X_PWM_FILTER_100	BIT(0)
> +#define LM3692X_PWM_FILTER_150	BIT(1)
> +#define LM3692X_PWM_FILTER_200	(BIT(0) | BIT(1))
> +#define LM3692X_PWM_HYSTER_1LSB BIT(2)
> +#define LM3692X_PWM_HYSTER_2LSB	BIT(3)
> +#define LM3692X_PWM_HYSTER_3LSB (BIT(3) | BIT(2))
> +#define LM3692X_PWM_HYSTER_4LSB BIT(4)
> +#define LM3692X_PWM_HYSTER_5LSB (BIT(4) | BIT(2))
> +#define LM3692X_PWM_HYSTER_6LSB (BIT(4) | BIT(3))

Same here, looks like a couple below too.

> +#define LM3692X_PWM_POLARITY	BIT(5)
> +#define LM3692X_PWM_SAMP_4MHZ	BIT(6)
> +#define LM3692X_PWM_SAMP_24MHZ	BIT(7)
> +
> +/* Boost Control Bits */
> +#define LM3692X_OCP_PROT_1A	BIT(0)
> +#define LM3692X_OCP_PROT_1_25A	BIT(1)
> +#define LM3692X_OCP_PROT_1_5A	(BIT(0) | BIT(1))
> +#define LM3692X_OVP_21V		BIT(2)
> +#define LM3692X_OVP_25V		BIT(3)
> +#define LM3692X_OVP_29V		(BIT(2) | BIT(3))
> +#define LM3692X_MIN_IND_22UH	BIT(4)
> +#define LM3692X_BOOST_SW_1MHZ	BIT(5)
> +#define LM3692X_BOOST_SW_NO_SHIFT	BIT(6)
> +
> +/* Fault Control Bits */
> +#define LM3692X_FAULT_CTRL_OVP BIT(0)
> +#define LM3692X_FAULT_CTRL_OCP BIT(1)
> +#define LM3692X_FAULT_CTRL_TSD BIT(2)
> +#define LM3692X_FAULT_CTRL_OPEN BIT(3)
> +
> +/* Fault Flag Bits */
> +#define LM3692X_FAULT_FLAG_OVP BIT(0)
> +#define LM3692X_FAULT_FLAG_OCP BIT(1)
> +#define LM3692X_FAULT_FLAG_TSD BIT(2)
> +#define LM3692X_FAULT_FLAG_SHRT BIT(3)
> +#define LM3692X_FAULT_FLAG_OPEN BIT(4)
> +
> +/**
> + * struct lm3692x_led -
> + * @lock - Lock for reading/writing the device
> + * @client - Pointer to the I2C client
> + * @led_dev - led class device pointer
> + * @regmap - Devices register map
> + * @enable_gpio - VDDIO/EN gpio to enable communication interface
> + * @regulator - LED supply regulator pointer
> + * @label - LED label
> + **/
> +struct lm3692x_led {
> +	struct mutex lock;
> +	struct i2c_client *client;
> +	struct led_classdev led_dev;
> +	struct regmap *regmap;
> +	struct gpio_desc *enable_gpio;
> +	struct regulator *regulator;
> +	const char *label;
> +};
> +
> +static const struct reg_default lm3692x_reg_defs[] = {
> +	{LM3692X_EN, 0xf},
> +	{LM3692X_BRT_CTRL, 0x61},
> +	{LM3692X_PWM_CTRL, 0x73},
> +	{LM3692X_BOOST_CTRL, 0x6f},
> +	{LM3692X_AUTO_FREQ_HI, 0x0},
> +	{LM3692X_AUTO_FREQ_LO, 0x0},
> +	{LM3692X_BL_ADJ_THRESH, 0x0},
> +	{LM3692X_BRT_LSB, 0x7},
> +	{LM3692X_BRT_MSB, 0xff},
> +	{LM3692X_FAULT_CTRL, 0x7},
> +};
> +
> +static const struct regmap_config lm3692x_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = LM3692X_FAULT_FLAGS,
> +	.reg_defaults = lm3692x_reg_defs,
> +	.num_reg_defaults = ARRAY_SIZE(lm3692x_reg_defs),
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int lm3692x_fault_check(struct lm3692x_led *led)
> +{
> +	int ret, fault;
> +	unsigned int read_buf;
> +
> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
> +	if (ret)
> +		goto out;
> +
> +	fault = read_buf;
> +
> +	if (fault)
> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
> +			fault);
> +out:
> +	return ret;
> +}
> +
> +static int lm3692x_brightness_set(struct led_classdev *led_cdev,
> +				enum led_brightness brt_val)
> +{
> +	struct lm3692x_led *led =
> +			container_of(led_cdev, struct lm3692x_led, led_dev);
> +	int led_brightness = (brt_val << 4) + 7;

Magic numbers?

BRIGHTNESS_SHIFT/MASK maybe?

And below we undo the operation anyway before we write it to the
register. Why not just drop that and to the shift/mask when you write it.

> +	int ret;
> +
> +	if (brt_val == LED_OFF)
> +		led_brightness = 0;
> +
> +	mutex_lock(&led->lock);
> +
> +	ret = lm3692x_fault_check(led);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_MSB,
> +			(led_brightness & 0xff0) >> 4);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot write MSB\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_LSB,
> +			led_brightness & 0x07);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot write LSB\n");
> +		goto out;
> +	}
> +out:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int lm3692x_init(struct lm3692x_led *led)
> +{
> +	int ret;
> +
> +	if (led->regulator) {
> +		ret = regulator_enable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to enable regulator\n");
> +	}
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 1);
> +
> +	ret = lm3692x_fault_check(led);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
> +	if (ret)
> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
> +	/*
> +	 * For glitch free operation, the following data should
> +	 * only be written while device enable bit is 0
> +	 * Per Section 7.5.14 of the data sheet
> +	 */
> +	ret = regmap_write(led->regmap, LM3692X_PWM_CTRL,
> +		LM3692X_PWM_FILTER_100 | LM3692X_PWM_SAMP_24MHZ);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming PWM CTRL\n");
> +
> +	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
> +			LM3692X_BRHT_MODE_RAMP_MULTI |
> +			LM3692X_BL_ADJ_POL |
> +			LM3692X_RAMP_RATE_250us);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming BOOST CTRL\n");
> +
> +	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_HI, 0x00);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming AUTO HI FREQ\n");
> +
> +	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_LO, 0x00);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming AUTO LO FREQ\n");
> +
> +
> +	ret = regmap_write(led->regmap, LM3692X_BL_ADJ_THRESH, 0x00);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming BL Threshold\n");
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL,
> +			LM3692X_BL_ADJ_POL | LM3692X_PWM_HYSTER_4LSB);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming BRT CTRL\n");
> +
> +	return ret;
> +out:
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 0);
> +
> +	if (led->regulator) {
> +		ret = regulator_disable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to disable regulator\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static int lm3692x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct lm3692x_led *led;
> +	struct device_node *np = client->dev.of_node;
> +
> +	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	if (client->dev.of_node) {
> +		ret = of_property_read_string(np, "label", &led->label);
> +		if (ret)
> +			led->label = LM3692X_LED_NAME;
> +	}
> +
> +	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
> +						   "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(led->enable_gpio)) {
> +		ret = PTR_ERR(led->enable_gpio);
> +		dev_err(&client->dev, "Failed to get enable gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	led->regulator = devm_regulator_get(&client->dev, "vled");
> +	if (IS_ERR(led->regulator))
> +		led->regulator = NULL;
> +
> +	led->client = client;
> +	led->led_dev.name = led->label;
> +	led->led_dev.max_brightness = LED_FULL;
> +	led->led_dev.brightness_set_blocking = lm3692x_brightness_set;
> +
> +	mutex_init(&led->lock);
> +
> +	i2c_set_clientdata(client, led);
> +
> +	led->regmap = devm_regmap_init_i2c(client, &lm3692x_regmap_config);
> +	if (IS_ERR(led->regmap)) {
> +		ret = PTR_ERR(led->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = lm3692x_init(led);
> +	if (ret)
> +		return ret;
> +
> +	ret = led_classdev_register(&client->dev, &led->led_dev);
> +	if (ret) {
> +		dev_err(&client->dev, "led register err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lm3692x_remove(struct i2c_client *client)
> +{
> +	struct lm3692x_led *led = i2c_get_clientdata(client);
> +	int ret;
> +
> +	led_classdev_unregister(&led->led_dev);
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 0);
> +
> +	if (led->regulator) {
> +		ret = regulator_disable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to disable regulator\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id lm3692x_id[] = {
> +	{ "lm3692x", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, lm3692x_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_lm3692x_leds_match[] = {
> +	{ .compatible = "ti,lm3692x", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_lm3692x_leds_match);
> +#endif
> +
> +static struct i2c_driver lm3692x_driver = {
> +	.driver = {
> +		.name	= "lm3692x",
> +		.of_match_table = of_match_ptr(of_lm3692x_leds_match),
> +	},
> +	.probe		= lm3692x_probe,
> +	.remove		= lm3692x_remove,
> +	.id_table	= lm3692x_id,
> +};
> +module_i2c_driver(lm3692x_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments LM3692X LED driver");
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_LICENSE("GPL");
> 

^ permalink raw reply

* Re: [GIT PULL] DeviceTree updates for 4.15
From: Linus Torvalds @ 2017-11-16  3:00 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqLbJP4T014Mgb4-RTmuTtV7nOiFDBPPzak=d9NHb3-w9Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

So these two warnings seems to be happening:

  drivers/of/unittest-data/testcases.dtb: Warning
(interrupts_extended_property): Could not get phandle node for
/__local_fixups__/testcase-data/interrupts/interrupts-extended0:interrupts-extended(cell
3)

  drivers/of/unittest-data/testcases.dtb: Warning
(interrupts_property): interrupts size is (4), expected multiple of 8
in /testcase-data/testcase-device2

and honestly, warnings during "make allmodconfig" are not acceptable.
Right now we have a few module license warnings that I hope are going
to go away, but the devicetree one I didn't even notice when it
appeared. I'm actually surprised, because I do a "make allmodconfig"
in between all pulls, and I didn't notice it after doing the
devicetree pull.

Of course, that is probably just exactly because those module-license
warnings have made me warning-blind, which is why warnings aren't
acceptable.

"It's a benign warning" is never true, exactly because a couple of
benign warnings that people get used to then end up hiding all the
_bad_ cases.

I have no idea how that dts test-case is _supposed_ to work, so I'll
just write this plaintive email. Please fix it.

           Linus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 1/2] dt-bindings: rtc: Add Spreadtrum SC27xx RTC documentation
From: Baolin Wang @ 2017-11-16  2:16 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Baolin Wang, Alessandro Zummo, Rob Herring, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA, LKML,
	linux-rtc-u79uwXL29TY76Z2rM5mHXA, Mark Brown
In-Reply-To: <20171115160123.gn27j4ki5mxv4v4j-m++hUPXGwpdeoWH0uzbU5w@public.gmane.org>

On 16 November 2017 at 00:01, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On 09/11/2017 at 11:34:16 +0800, Baolin Wang wrote:
>> This patch adds the binding documentation for Spreadtrum SC27xx series
>> RTC device.
>>
>> Signed-off-by: Baolin Wang <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> ---
>> Changes since v2:
>>  - No updates.
>>
>> Changes since v1:
>>  - Use a specific chip name as compatible string.
>>  - Remove useless alias.
>>  - Add the parent MFD node.
>> ---
>>  .../devicetree/bindings/rtc/sprd,sc27xx-rtc.txt    |   27 ++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/rtc/sprd,sc27xx-rtc.txt
>>
> Applied, thanks.

Thanks Alexandre and Rob.

-- 
Baolin Wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] MIPS: implement a "bootargs-append" DT property
From: Daniel Gimpelevich @ 2017-11-16  1:46 UTC (permalink / raw)
  To: Rob Herring
  Cc: James Hogan, Linux-MIPS, Frank Rowand,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Geert Uytterhoeven
In-Reply-To: <CAL_JsqJoEEkoA1sGocGFXE7WWhCmkb5k2OxVRZ3OnigptoL8_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Tue, 2017-11-14 at 11:18 -0600, Rob Herring wrote:
> I understand you can't apply random strings in any order you like. I'm
> questioning the need to do that and what are the concrete example(s)
> where you need that ability.

Concrete:
Pre-DT bootloader provides "console=/dev/someunusualdevice" and
"root=/dev/mtdblock1" in argv. The dtb arranges flash differently and
supplies "root=/dev/mtd2" in bootargs. Kernel uses /dev/mtdblock1 and
panics, or alternatively uses /dev/mtd2 with no console.

> I'd generally expect that only board specific options are in the dtb,
> and a distro will add it's own options common for all and the specific
> arch into the bootloader config files. And generally, the last thing
> loaded gets the last say in what is set.

Bootloaders and their configs are not distro-dependent.
> 
> What h/w specific options would be needed for recovery kernel? That's
> getting into putting not just Linux specifics into the dtb, but distro
> specifics there. While yes, the dts files often already have bootargs
> filled with Linux options, the intention is really that the bootloader
> fills in bootargs. And if you have multiple kernels or OSs, then the
> bootloader provides the mechanism to choose and boot with the right
> options.

Kernels _are_ distro-specific. A recovery kernel might also have options
that are not hardware specific in addition to options that are.

> I think the kernel (being last) should fully decide what to do:
> append, prepend, and/or override. There was some work a while back to
> support more flexible command line handling and be arch neutral, but
> it never got merged.

The kernel would not always have all the information needed for this
decision now like it would before DT. The dtb should decide what
precedence its bootargs have, and the kernel should decide what
precedence compiled-in bootargs have. Such logic is provided by this
patch, but not completely, because there is still no "bootargs-prepend"
property. The only use case I can think of for "bootargs-prepend" would
be to provide a default command line that is specific to a board that
would sometimes have one bootloader, and sometimes another. Currently,
such a situation can be worked around using a board-specific kernel, but
this mechanism is a relic from pre-DT days.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 00/12] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator
From: Takiguchi, Yasunari @ 2017-11-16  1:07 UTC (permalink / raw)
  To: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
  Cc: tbird20d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	Yamamoto, Masayuki, Nozawa, Hideki (STWN), Yonezawa, Kota,
	Matsumoto, Toshihiko, Watanabe, Satoshi (SSS),
	yasunari.takiguchi-7U/KSKJipcs
In-Reply-To: <20171013054635.20946-1-Yasunari.Takiguchi-7U/KSKJipcs@public.gmane.org>

Hi, all

I sent the patch series of Sony CXD2880 DVB-T2/T tuner + demodulator driver version 4 on 13th/Oct.
Are there any comments, advices and review results for them?

I'd like to get better understanding of current review status for our codes.

Regards,
Takiguchi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v11 1/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver documentation
From: Niklas Söderlund @ 2017-11-15 23:00 UTC (permalink / raw)
  To: Rob Herring
  Cc: Laurent Pinchart, Hans Verkuil, linux-media, linux-renesas-soc,
	tomoharu.fukawa.eb, Kieran Bingham, Sakari Ailus,
	Geert Uytterhoeven, devicetree
In-Reply-To: <20171115195936.jkq4ssgfp4tko3lv@rob-hp-laptop>

Hi Rob,

Thanks for your feedback.

On 2017-11-15 13:59:36 -0600, Rob Herring wrote:
> On Sat, Nov 11, 2017 at 01:25:25AM +0100, Niklas Söderlund wrote:
> > Documentation for Renesas R-Car MIPI CSI-2 receiver. The CSI-2 receivers
> > are located between the video sources (CSI-2 transmitters) and the video
> > grabbers (VIN) on Gen3 of Renesas R-Car SoC.
> > 
> > Each CSI-2 device is connected to more then one VIN device which
> > simultaneously can receive video from the same CSI-2 device. Each VIN
> > device can also be connected to more then one CSI-2 device. The routing
> > of which link are used are controlled by the VIN devices. There are only
> > a few possible routes which are set by hardware limitations, which are
> > different for each SoC in the Gen3 family.
> > 
> > To work with the limitations of routing possibilities it is necessary
> > for the DT bindings to describe which VIN device is connected to which
> > CSI-2 device. This is why port 1 needs to to assign reg numbers for each
> > VIN device that be connected to it. To setup and to know which links are
> > valid for each SoC is the responsibility of the VIN driver since the
> > register to configure it belongs to the VIN hardware.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  .../bindings/media/renesas,rcar-csi2.txt           | 104 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  2 files changed, 105 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
> > new file mode 100644
> > index 0000000000000000..24705d8997b14a10
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
> > @@ -0,0 +1,104 @@
> > +Renesas R-Car MIPI CSI-2
> > +------------------------
> > +
> > +The rcar-csi2 device provides MIPI CSI-2 capabilities for the Renesas R-Car
> > +family of devices. It is to be used in conjunction with the R-Car VIN module,
> > +which provides the video capture capabilities.
> > +
> > +Mandatory properties
> > +--------------------
> > + - compatible: Must be one or more of the following
> > +   - "renesas,r8a7795-csi2" for the R8A7795 device.
> > +   - "renesas,r8a7796-csi2" for the R8A7796 device.
> > +
> > + - reg: the register base and size for the device registers
> > + - interrupts: the interrupt for the device
> > + - clocks: Reference to the parent clock
> > +
> > +The device node shall contain two 'port' child nodes according to the
> > +bindings defined in Documentation/devicetree/bindings/media/
> > +video-interfaces.txt. Port 0 shall connect the node that is the video
> > +source for to the CSI-2. Port 1 shall connect all the R-Car VIN
> > +modules, which can make use of the CSI-2 module.
> > +
> > +- Port 0 - Video source (Mandatory)
> > +	- Endpoint 0 - sub-node describing the endpoint that is the video source
> > +
> > +- Port 1 - VIN instances (Mandatory for all VIN present in the SoC)
> > +	- Endpoint 0 - sub-node describing the endpoint that is VIN0
> > +	- Endpoint 1 - sub-node describing the endpoint that is VIN1
> > +	- Endpoint 2 - sub-node describing the endpoint that is VIN2
> > +	- Endpoint 3 - sub-node describing the endpoint that is VIN3
> > +	- Endpoint 4 - sub-node describing the endpoint that is VIN4
> > +	- Endpoint 5 - sub-node describing the endpoint that is VIN5
> > +	- Endpoint 6 - sub-node describing the endpoint that is VIN6
> > +	- Endpoint 7 - sub-node describing the endpoint that is VIN7
> > +
> > +Example:
> > +
> > +	csi20: csi2@fea80000 {
> > +		compatible = "renesas,r8a7796-csi2", "renesas,rcar-gen3-csi2";
> > +		reg = <0 0xfea80000 0 0x10000>;
> > +		interrupts = <0 184 IRQ_TYPE_LEVEL_HIGH>;
> > +		clocks = <&cpg CPG_MOD 714>;
> > +		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> > +		resets = <&cpg 714>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				reg = <0>;
> > +
> > +				csi20_in: endpoint@0 {
> 
> unit-address without reg property is not valid.

Ops, my bad thanks for noticing.

> 
> Otherwise,
> 
> Acked-by: Rob Herring <robh@kernel.org>

Thanks, will fix the above and re-send.

> 
> 
> > +					clock-lanes = <0>;
> > +					data-lanes = <1>;
> > +					remote-endpoint = <&adv7482_txb>;
> > +				};
> > +			};
> > +
> > +			port@1 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				reg = <1>;
> > +
> > +				csi20vin0: endpoint@0 {
> > +					reg = <0>;
> > +					remote-endpoint = <&vin0csi20>;
> > +				};
> > +				csi20vin1: endpoint@1 {
> > +					reg = <1>;
> > +					remote-endpoint = <&vin1csi20>;
> > +				};
> > +				csi20vin2: endpoint@2 {
> > +					reg = <2>;
> > +					remote-endpoint = <&vin2csi20>;
> > +				};
> > +				csi20vin3: endpoint@3 {
> > +					reg = <3>;
> > +					remote-endpoint = <&vin3csi20>;
> > +				};
> > +				csi20vin4: endpoint@4 {
> > +					reg = <4>;
> > +					remote-endpoint = <&vin4csi20>;
> > +				};
> > +				csi20vin5: endpoint@5 {
> > +					reg = <5>;
> > +					remote-endpoint = <&vin5csi20>;
> > +				};
> > +				csi20vin6: endpoint@6 {
> > +					reg = <6>;
> > +					remote-endpoint = <&vin6csi20>;
> > +				};
> > +				csi20vin7: endpoint@7 {
> > +					reg = <7>;
> > +					remote-endpoint = <&vin7csi20>;
> > +				};
> > +			};
> > +		};
> > +	};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index adbf69306e9ee3d2..fe999e9de76e3cb3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8565,6 +8565,7 @@ L:	linux-media@vger.kernel.org
> >  L:	linux-renesas-soc@vger.kernel.org
> >  T:	git git://linuxtv.org/media_tree.git
> >  S:	Supported
> > +F:	Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
> >  F:	Documentation/devicetree/bindings/media/rcar_vin.txt
> >  F:	drivers/media/platform/rcar-vin/
> >  
> > -- 
> > 2.15.0
> > 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply

* Re: [PATCH v7 01/25] rcar-vin: add Gen3 devicetree bindings documentation
From: Niklas Söderlund @ 2017-11-15 22:58 UTC (permalink / raw)
  To: Rob Herring
  Cc: Laurent Pinchart, Hans Verkuil, linux-media, linux-renesas-soc,
	tomoharu.fukawa.eb, Kieran Bingham, devicetree
In-Reply-To: <20171115200226.wd343hd3a52jjhdd@rob-hp-laptop>

Hi Rob,

Thanks for your feedback, much appreciated!

On 2017-11-15 14:02:26 -0600, Rob Herring wrote:
> On Sat, Nov 11, 2017 at 01:38:11AM +0100, Niklas Söderlund wrote:
> > Document the devicetree bindings for the CSI-2 inputs available on Gen3.
> > 
> > There is a need to add a custom property 'renesas,id' and to define
> > which CSI-2 input is described in which endpoint under the port@1 node.
> > This information is needed since there are a set of predefined routes
> > between each VIN and CSI-2 block. This routing table will be kept
> > inside the driver but in order for it to act on it it must know which
> > VIN and CSI-2 is which.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  .../devicetree/bindings/media/rcar_vin.txt         | 116 ++++++++++++++++++---
> >  1 file changed, 104 insertions(+), 12 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> > index 6e4ef8caf759e5d3..df1abd0fb20386f8 100644
> > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> > @@ -2,8 +2,12 @@ Renesas R-Car Video Input driver (rcar_vin)
> >  -------------------------------------------
> >  
> >  The rcar_vin device provides video input capabilities for the Renesas R-Car
> > -family of devices. The current blocks are always slaves and suppot one input
> > -channel which can be either RGB, YUYV or BT656.
> > +family of devices.
> > +
> > +Each VIN instance has a single parallel input that supports RGB and YUV video,
> > +with both external synchronization and BT.656 synchronization for the latter.
> > +Depending on the instance the VIN input is connected to external SoC pins, or
> > +on Gen3 to a CSI-2 receiver.
> >  
> >   - compatible: Must be one or more of the following
> >     - "renesas,vin-r8a7795" for the R8A7795 device
> > @@ -28,21 +32,38 @@ channel which can be either RGB, YUYV or BT656.
> >  Additionally, an alias named vinX will need to be created to specify
> >  which video input device this is.
> >  
> > -The per-board settings:
> > +The per-board settings Gen2:
> >   - port sub-node describing a single endpoint connected to the vin
> >     as described in video-interfaces.txt[1]. Only the first one will
> >     be considered as each vin interface has one input port.
> >  
> > -   These settings are used to work out video input format and widths
> > -   into the system.
> > +The per-board settings Gen3:
> > +
> > +Gen3 can support both a single connected parallel input source from
> > +external SoC pins (port0) and/or multiple parallel input sources from
> > +local SoC CSI-2 receivers (port1) depending on SoC.
> >  
> > +- renesas,id - ID number of the VIN, VINx in the documentation.
> 
> Why is this needed? We try to avoid indexes unless that's the only way a 
> device is addressed (and then we use reg).

This is unfortunately needed (or something similar) as there is a 
register in one VIN instance which controls the routing of the incoming 
CSI-2 video streams, not only to itself, but also to other VIN instances 
inside the same SoC.

To be more specific I will try to clarify this using the R-Car H3 as an 
example. On the H3 there are 8 instances of the capture hardware (VIN0 - 
VIN7) and 3 instances off CSI-2 receivers (CSI20, CSI40 and CSI41) which 
receives CSI-2 streams, split the possible multiple virtual channels 
(VC) encoded in CSI-2 streams and forwards it to the VIN's.

The problem is that VIN0 and VIN4 are different from the other VIN's, 
they have one register (CHSEL) which controls the limited number of 
possible routes of video streams between a CSI-2 + VC to a specific VIN.  
The CHSEL register in VIN0 controls the routing for VIN0-3 and the one 
in VIN4 controls VIN4-7 (the two subgroups are similar so lets only 
consider VIN0-3).

There are only a handful of routes possible and the kicker is that 
changing the CHSEL value in VIN0, directly reflects all routes for 
VIN0-VIN3 per this table:

CHSEL reg in VIN0:  0         1         2         3         4
Video sent to VIN0: CSI40/VC0 CSI20/VC0 CSI40/VC1 CSI40/VC0 CSI20/VC0
Video sent to VIN1: CSI20/VC0 CSI40/VC1 CSI40/VC0 CSI40/VC1 CSI20/VC1
Video sent to VIN2: CSI20/VC1 CSI40/VC0 CSI20/VC0 CSI40/VC2 CSI20/VC2
Video sent to VIN3: CSI40/VC1 CSI20/VC1 CSI20/VC1 CSI40/VC3 CSI20/VC3

So if CHSEL in VIN0 is set to 2 the following routes are active: VIN0: 
CSI40/VC1, VIN1: CSI40/VC0, VIN2: CSI20/VC0, CSI20/VC1. These routing 
tables are different for most SoCs in the R-Car Gen3 family, and are 
kept inside the driver code.

So the renesas,id properly is used so that the rcar-vin driver knows 
which of the driver instances corresponds to a specific VINx number from 
the documentation so that using the media device API the correct links 
can be added and that the routing table is enforced so a user can't try 
to activate a CSI-2 + VC to a VIN link which is not possible due to 
other routes already being active.

I agree that if possible I would like to not have the need for this 
property, but I can't think of another way on how to give this knowledge 
to the driver in a better way. I'm happy to try out other ideas :-)

Dose this explanation make sens to you?

-- 
Regards,
Niklas Söderlund

^ permalink raw reply

* Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver
From: Pavel Machek @ 2017-11-15 22:23 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Jacek Anaszewski, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, rpurdie-Fm38FmjxZ/leoWH0uzbU5w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-leds-u79uwXL29TY76Z2rM5mHXA, Lee Jones, Daniel Thompson,
	Jingoo Han
In-Reply-To: <0a78c883-f074-cc19-3c0c-fc05607400e9-l0cyMroinI0@public.gmane.org>

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

Hi!

> > Shouldn't the driver be targeted for backlight subsystem then?
> > 
> > Adding backlight maintainers.
> > 
> 
> Great point!  I was not aware of the backlight subsystem.
> 
> Looks like I need to create a back light version as well.
> 
> Like the lp8788 did since this can be used as a LED driver beyond
> display back lighting.

No, definitely not two drivers for lp8788 hardware.

If that does not yet exist... you want to create glue layer to be able
to use LED as a display backlight. (It may already exist, no idea).

...

Actually or maybe a LED trigger. Just set LED's trigger to "this is
display backlight".

Other solutions are acceptable, but not two hardware drivers.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* [PATCH v4 5/5] misc serdev: w2sg0004: add debugging code and Kconfig
From: H. Nikolaus Schaller @ 2017-11-15 21:37 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Russell King, Arnd Bergmann, Greg Kroah-Hartman,
	H. Nikolaus Schaller, Kevin Hilman, Andreas Färber,
	Thierry Reding, Jonathan Cameron
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	letux-kernel-S0jZdbWzriLCfDggNXIi3w,
	kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cover.1510781875.git.hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>

This allows to set CONFIG_W2SG0004_DEBUG which will
make the driver report more activities and it will turn on the
GPS module during boot while the driver assumes that it
is off. This can be used to debug the correct functioning of
the hardware. Therefore we add it as an option to the driver
because we think it is of general use (and a little tricky to
add by system testers).

Normally it should be off.

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 drivers/misc/Kconfig    |  8 ++++++++
 drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 09d171d68408..f15ff5a97e42 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -528,4 +528,12 @@ config W2SG0004
 	  is opened/closed.
 	  It also provides a rfkill gps name to control the LNA power.
 
+config W2SG0004_DEBUG
+	bool "W2SG0004 on/off debugging"
+	depends on W2SG0004
+	help
+	  Enable driver debugging mode of W2SG0004 GPS. If you say y here
+	  this will turn on the module and you can check if it is turned
+	  off by the driver.
+
 endmenu
diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
index 12e14b5e0a99..51ddc807eac5 100644
--- a/drivers/misc/w2sg0004.c
+++ b/drivers/misc/w2sg0004.c
@@ -22,6 +22,10 @@
  *
  */
 
+#ifdef CONFIG_W2SG0004_DEBUG
+#define DEBUG 1
+#endif
+
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -253,6 +257,7 @@ static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 	pr_debug("%s() tty = %p\n", __func__, tty);
 
 	data = w2sg_get_by_minor(tty->index);
+	pr_debug("%s() data = %p\n", __func__, data);
 
 	if (!data)
 		return -ENODEV;
@@ -333,6 +338,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 
 	w2sg_by_minor[minor] = data;
 
+	pr_debug("w2sg serdev_device_set_drvdata\n");
+
 	serdev_device_set_drvdata(serdev, data);
 
 	data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
@@ -366,6 +373,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 
 	INIT_DELAYED_WORK(&data->work, toggle_work);
 
+	pr_debug("w2sg devm_gpio_request\n");
+
 	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
 				"w2sg0004-on-off");
 	if (err < 0)
@@ -379,6 +388,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	serdev_device_set_baudrate(data->uart, 9600);
 	serdev_device_set_flow_control(data->uart, false);
 
+	pr_debug("w2sg rfkill_alloc\n");
+
 	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
 				&w2sg0004_rfkill_ops, data);
 	if (rf_kill == NULL) {
@@ -386,6 +397,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 		goto err_rfkill;
 	}
 
+	pr_debug("w2sg register rfkill\n");
+
 	err = rfkill_register(rf_kill);
 	if (err) {
 		dev_err(&serdev->dev, "Cannot register rfkill device\n");
@@ -394,6 +407,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 
 	data->rf_kill = rf_kill;
 
+	pr_debug("w2sg alloc_tty_driver\n");
+
 	/* allocate the tty driver */
 	data->tty_drv = alloc_tty_driver(1);
 	if (!data->tty_drv)
@@ -419,6 +434,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	 */
 	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
 
+	pr_debug("w2sg tty_register_driver\n");
+
 	/* register the tty driver */
 	err = tty_register_driver(data->tty_drv);
 	if (err) {
@@ -428,16 +445,34 @@ static int w2sg_probe(struct serdev_device *serdev)
 		goto err_rfkill;
 	}
 
+	pr_debug("w2sg call tty_port_init\n");
+
 	tty_port_init(&data->port);
 	data->port.ops = &w2sg_port_ops;
 
 	pr_debug("w2sg call tty_port_register_device\n");
 
+	pr_debug("w2sg call tty_port_register_device\n");
+
 	data->dev = tty_port_register_device(&data->port,
 			data->tty_drv, minor, &serdev->dev);
 
+	pr_debug("w2sg tty_port_register_device -> %p\n", data->dev);
+	pr_debug("w2sg port.tty = %p\n", data->port.tty);
+
 	pr_debug("w2sg probed\n");
 
+#ifdef CONFIG_W2SG0004_DEBUG
+	pr_debug("w2sg DEBUGGING MODE enabled\n");
+	/* turn on for debugging rx notifications */
+	pr_debug("w2sg power gpio ON\n");
+	gpio_set_value_cansleep(data->on_off_gpio, 1);
+	mdelay(100);
+	pr_debug("w2sg power gpio OFF\n");
+	gpio_set_value_cansleep(data->on_off_gpio, 0);
+	mdelay(300);
+#endif
+
 	/* keep off until user space requests the device */
 	w2sg_set_power(data, false);
 
@@ -447,6 +482,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	rfkill_destroy(rf_kill);
 	serdev_device_close(data->uart);
 out:
+	pr_debug("w2sg error %d\n", err);
+
 	return err;
 }
 
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v4 4/5] DTS: gta04: add serdev node for w2sg00x4
From: H. Nikolaus Schaller @ 2017-11-15 21:37 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Russell King, Arnd Bergmann, Greg Kroah-Hartman,
	H. Nikolaus Schaller, Kevin Hilman, Andreas Färber,
	Thierry Reding, Jonathan Cameron
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	letux-kernel-S0jZdbWzriLCfDggNXIi3w,
	kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cover.1510781875.git.hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>

GTA04 has a W2SG0004/84 connected to UART2 of the OMAP3
processor. A GPIO can pulse the on/off toggle switch.

The VSIM regulator is used to power on/off the LNA of an external
active GPS antenna so that a driver can turn the LNA off if GPS is
not needed to save battery energy.

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index 4504908c23fe..e967d738aaaf 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -477,6 +477,12 @@
 &uart2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart2_pins>;
+
+	gps: gps {
+		compatible = "wi2wi,w2sg0004";
+		lna-supply = <&vsim>;   /* LNA regulator */
+		enable-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;    /* GPIO_145: trigger for turning on/off w2sg0004 */
+	};
 };
 
 &uart3 {
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v4 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver
From: H. Nikolaus Schaller @ 2017-11-15 21:37 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Russell King, Arnd Bergmann, Greg Kroah-Hartman,
	H. Nikolaus Schaller, Kevin Hilman, Andreas Färber,
	Thierry Reding, Jonathan Cameron
  Cc: devicetree, linux-kernel, linux-omap, letux-kernel, kernel,
	linux-arm-kernel
In-Reply-To: <cover.1510781875.git.hns@goldelico.com>

Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.

Use serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
and turn on/off the module. It also detects if the module is turned on (sends data)
but should be off, e.g. if it was already turned on during boot or power-on-reset.

Additionally, rfkill block/unblock can be used to control an external LNA
(and power down the module if not needed).

The driver concept is based on code developed by NeilBrown <neilb@suse.de>
but simplified and adapted to use the new serdev API introduced in 4.11.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/misc/Kconfig    |  10 +
 drivers/misc/Makefile   |   1 +
 drivers/misc/w2sg0004.c | 545 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 556 insertions(+)
 create mode 100644 drivers/misc/w2sg0004.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 8136dc7e863d..09d171d68408 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -518,4 +518,14 @@ source "drivers/misc/mic/Kconfig"
 source "drivers/misc/genwqe/Kconfig"
 source "drivers/misc/echo/Kconfig"
 source "drivers/misc/cxl/Kconfig"
+
+config W2SG0004
+	tristate "W2SG00x4 on/off control"
+	depends on GPIOLIB && SERIAL_DEV_BUS
+	help
+          Enable on/off control of W2SG00x4 GPS moduled connected
+	  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
+	  is opened/closed.
+	  It also provides a rfkill gps name to control the LNA power.
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index ad0e64fdba34..abcb667e0ff0 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.o
 obj-y				+= mic/
 obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
+obj-$(CONFIG_W2SG0004)		+= w2sg0004.o
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
new file mode 100644
index 000000000000..12e14b5e0a99
--- /dev/null
+++ b/drivers/misc/w2sg0004.c
@@ -0,0 +1,545 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
+ *
+ * This receiver has an ON/OFF pin which must be toggled to
+ * turn the device 'on' of 'off'.  A high->low->high toggle
+ * will switch the device on if it is off, and off if it is on.
+ *
+ * To enable receiving on/off requests we register with the
+ * UART power management notifications.
+ *
+ * It is not possible to directly detect the state of the device.
+ * However when it is on it will send characters on a UART line
+ * regularly.
+ *
+ * To detect that the power state is out of sync (e.g. if GPS
+ * was enabled before a reboot), we register for UART data received
+ * notifications.
+ *
+ * In addition we register as a rfkill client so that we can
+ * control the LNA power.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rfkill.h>
+#include <linux/serdev.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <linux/workqueue.h>
+
+/*
+ * There seems to be restrictions on how quickly we can toggle the
+ * on/off line.  data sheets says "two rtc ticks", whatever that means.
+ * If we do it too soon it doesn't work.
+ * So we have a state machine which uses the common work queue to ensure
+ * clean transitions.
+ * When a change is requested we record that request and only act on it
+ * once the previous change has completed.
+ * A change involves a 10ms low pulse, and a 990ms raised level, so only
+ * one change per second.
+ */
+
+enum w2sg_state {
+	W2SG_IDLE,	/* is not changing state */
+	W2SG_PULSE,	/* activate on/off impulse */
+	W2SG_NOPULSE	/* deactivate on/off impulse */
+};
+
+struct w2sg_data {
+	struct		rfkill *rf_kill;
+	struct		regulator *lna_regulator;
+	int		lna_blocked;	/* rfkill block gps active */
+	int		lna_is_off;	/* LNA is currently off */
+	int		is_on;		/* current state (0/1) */
+	unsigned long	last_toggle;
+	unsigned long	backoff;	/* time to wait since last_toggle */
+	int		on_off_gpio;	/* the on-off gpio number */
+	struct		serdev_device *uart;	/* uart connected to the chip */
+	struct		tty_driver *tty_drv;	/* this is the user space tty */
+	struct		device *dev;	/* from tty_port_register_device() */
+	struct		tty_port port;
+	int		open_count;	/* how often we were opened */
+	enum		w2sg_state state;
+	int		requested;	/* requested state (0/1) */
+	int		suspended;
+	struct delayed_work work;
+	int		discard_count;
+};
+
+static struct w2sg_data *w2sg_by_minor[1];
+
+static int w2sg_set_lna_power(struct w2sg_data *data)
+{
+	int ret = 0;
+	int off = data->suspended || !data->requested || data->lna_blocked;
+
+	pr_debug("%s: %s\n", __func__, off ? "off" : "on");
+
+	if (off != data->lna_is_off) {
+		data->lna_is_off = off;
+		if (!IS_ERR_OR_NULL(data->lna_regulator)) {
+			if (off)
+				regulator_disable(data->lna_regulator);
+			else
+				ret = regulator_enable(data->lna_regulator);
+		}
+	}
+
+	return ret;
+}
+
+static void w2sg_set_power(void *pdata, int val)
+{
+	struct w2sg_data *data = (struct w2sg_data *) pdata;
+
+	pr_debug("%s to state=%d (requested=%d)\n", __func__, val, data->requested);
+
+	if (val && !data->requested) {
+		data->requested = true;
+	} else if (!val && data->requested) {
+		data->backoff = HZ;
+		data->requested = false;
+	} else
+		return;
+
+	pr_debug("w2sg00x4 scheduled for %d\n", data->requested);
+
+	if (!data->suspended)
+		schedule_delayed_work(&data->work, 0);
+}
+
+/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
+
+static int w2sg_uart_receive_buf(struct serdev_device *serdev,
+				const unsigned char *rxdata,
+				size_t count)
+{
+	struct w2sg_data *data =
+		(struct w2sg_data *) serdev_device_get_drvdata(serdev);
+
+	if (!data->requested && !data->is_on) {
+		/*
+		 * we have received characters while the w2sg
+		 * should have been be turned off
+		 */
+		data->discard_count += count;
+		if ((data->state == W2SG_IDLE) &&
+		    time_after(jiffies,
+		    data->last_toggle + data->backoff)) {
+			/* Should be off by now, time to toggle again */
+			pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
+				data->discard_count);
+
+			data->discard_count = 0;
+
+			data->is_on = true;
+			data->backoff *= 2;
+			if (!data->suspended)
+				schedule_delayed_work(&data->work, 0);
+		}
+	} else if (data->open_count > 0) {
+		int n;
+
+		pr_debug("w2sg00x4: push %lu chars to tty port\n",
+			(unsigned long) count);
+
+		/* pass to user-space */
+		n = tty_insert_flip_string(&data->port, rxdata, count);
+		if (n != count)
+			pr_err("w2sg00x4: did loose %lu characters\n",
+				(unsigned long) (count - n));
+		tty_flip_buffer_push(&data->port);
+		return n;
+	}
+
+	/* assume we have processed everything */
+	return count;
+}
+
+/* try to toggle the power state by sending a pulse to the on-off GPIO */
+
+static void toggle_work(struct work_struct *work)
+{
+	struct w2sg_data *data = container_of(work, struct w2sg_data,
+					      work.work);
+
+	switch (data->state) {
+	case W2SG_IDLE:
+		if (data->requested == data->is_on)
+			return;
+
+		w2sg_set_lna_power(data);	/* update LNA power state */
+		gpio_set_value_cansleep(data->on_off_gpio, 0);
+		data->state = W2SG_PULSE;
+
+		pr_debug("w2sg: power gpio ON\n");
+
+		schedule_delayed_work(&data->work,
+				      msecs_to_jiffies(10));
+		break;
+
+	case W2SG_PULSE:
+		gpio_set_value_cansleep(data->on_off_gpio, 1);
+		data->last_toggle = jiffies;
+		data->state = W2SG_NOPULSE;
+		data->is_on = !data->is_on;
+
+		pr_debug("w2sg: power gpio OFF\n");
+
+		schedule_delayed_work(&data->work,
+				      msecs_to_jiffies(10));
+		break;
+
+	case W2SG_NOPULSE:
+		data->state = W2SG_IDLE;
+
+		pr_debug("w2sg: idle\n");
+
+		break;
+
+	}
+}
+
+static int w2sg_rfkill_set_block(void *pdata, bool blocked)
+{
+	struct w2sg_data *data = pdata;
+
+	pr_debug("%s: blocked: %d\n", __func__, blocked);
+
+	data->lna_blocked = blocked;
+
+	return w2sg_set_lna_power(data);
+}
+
+static struct rfkill_ops w2sg0004_rfkill_ops = {
+	.set_block = w2sg_rfkill_set_block,
+};
+
+static struct serdev_device_ops serdev_ops = {
+	.receive_buf = w2sg_uart_receive_buf,
+};
+
+/*
+ * we are a man-in the middle between the user-space visible tty port
+ * and the serdev tty where the chip is connected.
+ * This allows us to recognise when the device should be powered on
+ * or off and handle the "false" state that data arrives while no
+ * users-space tty client exists.
+ */
+
+static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
+{
+	return w2sg_by_minor[minor];
+}
+
+static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct w2sg_data *data;
+	int retval;
+
+	pr_debug("%s() tty = %p\n", __func__, tty);
+
+	data = w2sg_get_by_minor(tty->index);
+
+	if (!data)
+		return -ENODEV;
+
+	retval = tty_standard_install(driver, tty);
+	if (retval)
+		goto error_init_termios;
+
+	tty->driver_data = data;
+
+	return 0;
+
+error_init_termios:
+	tty_port_put(&data->port);
+	return retval;
+}
+
+static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
+{
+	struct w2sg_data *data = tty->driver_data;
+
+	pr_debug("%s() data = %p open_count = ++%d\n", __func__, data, data->open_count);
+
+	w2sg_set_power(data, ++data->open_count > 0);
+
+	return tty_port_open(&data->port, tty, file);
+}
+
+static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
+{
+	struct w2sg_data *data = tty->driver_data;
+
+	pr_debug("%s()\n", __func__);
+
+	w2sg_set_power(data, --data->open_count > 0);
+
+	tty_port_close(&data->port, tty, file);
+}
+
+static int w2sg_tty_write(struct tty_struct *tty,
+		const unsigned char *buffer, int count)
+{
+	struct w2sg_data *data = tty->driver_data;
+
+	/* simply pass down to UART */
+	return serdev_device_write_buf(data->uart, buffer, count);
+}
+
+static const struct tty_operations w2sg_serial_ops = {
+	.install = w2sg_tty_install,
+	.open = w2sg_tty_open,
+	.close = w2sg_tty_close,
+	.write = w2sg_tty_write,
+};
+
+static const struct tty_port_operations w2sg_port_ops = {
+};
+
+static int w2sg_probe(struct serdev_device *serdev)
+{
+	struct w2sg_data *data;
+	struct rfkill *rf_kill;
+	int err;
+	int minor;
+	enum of_gpio_flags flags;
+
+	pr_debug("%s()\n", __func__);
+
+	minor = 0;
+	if (w2sg_by_minor[minor]) {
+		pr_err("w2sg minor is already in use!\n");
+		return -ENODEV;
+	}
+
+	data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	w2sg_by_minor[minor] = data;
+
+	serdev_device_set_drvdata(serdev, data);
+
+	data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
+						     "enable-gpios", 0,
+						     &flags);
+	/* defer until we have all gpios */
+	if (data->on_off_gpio == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
+							"lna");
+	if (IS_ERR(data->lna_regulator)) {
+		/* defer until we can get the regulator */
+		if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		data->lna_regulator = NULL;
+	}
+	pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
+
+	data->lna_blocked = true;
+	data->lna_is_off = true;
+
+	data->is_on = false;
+	data->requested = false;
+	data->state = W2SG_IDLE;
+	data->last_toggle = jiffies;
+	data->backoff = HZ;
+
+	data->uart = serdev;
+
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+
+	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
+				"w2sg0004-on-off");
+	if (err < 0)
+		goto out;
+
+	gpio_direction_output(data->on_off_gpio, false);
+
+	serdev_device_set_client_ops(data->uart, &serdev_ops);
+	serdev_device_open(data->uart);
+
+	serdev_device_set_baudrate(data->uart, 9600);
+	serdev_device_set_flow_control(data->uart, false);
+
+	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
+				&w2sg0004_rfkill_ops, data);
+	if (rf_kill == NULL) {
+		err = -ENOMEM;
+		goto err_rfkill;
+	}
+
+	err = rfkill_register(rf_kill);
+	if (err) {
+		dev_err(&serdev->dev, "Cannot register rfkill device\n");
+		goto err_rfkill;
+	}
+
+	data->rf_kill = rf_kill;
+
+	/* allocate the tty driver */
+	data->tty_drv = alloc_tty_driver(1);
+	if (!data->tty_drv)
+		return -ENOMEM;
+
+	/* initialize the tty driver */
+	data->tty_drv->owner = THIS_MODULE;
+	data->tty_drv->driver_name = "w2sg0004";
+	data->tty_drv->name = "ttyGPS";
+	data->tty_drv->major = 0;
+	data->tty_drv->minor_start = 0;
+	data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
+	data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
+	data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
+	data->tty_drv->init_termios = tty_std_termios;
+	data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
+					      HUPCL | CLOCAL;
+	/*
+	 * optional:
+	 * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
+					115200, 115200);
+	 * w2sg_tty_termios(&data->tty_drv->init_termios);
+	 */
+	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
+
+	/* register the tty driver */
+	err = tty_register_driver(data->tty_drv);
+	if (err) {
+		pr_err("%s - tty_register_driver failed(%d)\n",
+			__func__, err);
+		put_tty_driver(data->tty_drv);
+		goto err_rfkill;
+	}
+
+	tty_port_init(&data->port);
+	data->port.ops = &w2sg_port_ops;
+
+	pr_debug("w2sg call tty_port_register_device\n");
+
+	data->dev = tty_port_register_device(&data->port,
+			data->tty_drv, minor, &serdev->dev);
+
+	pr_debug("w2sg probed\n");
+
+	/* keep off until user space requests the device */
+	w2sg_set_power(data, false);
+
+	return 0;
+
+err_rfkill:
+	rfkill_destroy(rf_kill);
+	serdev_device_close(data->uart);
+out:
+	return err;
+}
+
+static void w2sg_remove(struct serdev_device *serdev)
+{
+	struct w2sg_data *data = serdev_device_get_drvdata(serdev);
+	int minor;
+
+	cancel_delayed_work_sync(&data->work);
+
+	/* what is the right sequence to avoid problems? */
+	serdev_device_close(data->uart);
+
+	minor = 0;
+	tty_unregister_device(data->tty_drv, minor);
+
+	tty_unregister_driver(data->tty_drv);
+}
+
+static int w2sg_suspend(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	data->suspended = true;
+
+	cancel_delayed_work_sync(&data->work);
+
+	w2sg_set_lna_power(data);	/* shuts down if needed */
+
+	if (data->state == W2SG_PULSE) {
+		msleep(10);
+		gpio_set_value_cansleep(data->on_off_gpio, 1);
+		data->last_toggle = jiffies;
+		data->is_on = !data->is_on;
+		data->state = W2SG_NOPULSE;
+	}
+
+	if (data->state == W2SG_NOPULSE) {
+		msleep(10);
+		data->state = W2SG_IDLE;
+	}
+
+	if (data->is_on) {
+		pr_info("GPS off for suspend %d %d %d\n", data->requested,
+			data->is_on, data->lna_is_off);
+
+		gpio_set_value_cansleep(data->on_off_gpio, 0);
+		msleep(10);
+		gpio_set_value_cansleep(data->on_off_gpio, 1);
+		data->is_on = 0;
+	}
+
+	return 0;
+}
+
+static int w2sg_resume(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	data->suspended = false;
+
+	pr_info("GPS resuming %d %d %d\n", data->requested,
+		data->is_on, data->lna_is_off);
+
+	schedule_delayed_work(&data->work, 0);	/* enables LNA if needed */
+
+	return 0;
+}
+
+static const struct of_device_id w2sg0004_of_match[] = {
+	{ .compatible = "wi2wi,w2sg0004" },
+	{ .compatible = "wi2wi,w2sg0084" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
+
+SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
+
+static struct serdev_device_driver w2sg_driver = {
+	.probe		= w2sg_probe,
+	.remove		= w2sg_remove,
+	.driver = {
+		.name	= "w2sg0004",
+		.owner	= THIS_MODULE,
+		.pm	= &w2sg_pm_ops,
+		.of_match_table = of_match_ptr(w2sg0004_of_match)
+	},
+};
+
+module_serdev_device_driver(w2sg_driver);
+
+MODULE_ALIAS("w2sg0004");
+
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
+MODULE_LICENSE("GPL v2");
-- 
2.12.2

^ permalink raw reply related

* [PATCH v4 2/5] dt-bindings: gps: add w2sg00x4 bindings documentation (GPS module with UART))
From: H. Nikolaus Schaller @ 2017-11-15 21:37 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Russell King, Arnd Bergmann, Greg Kroah-Hartman,
	H. Nikolaus Schaller, Kevin Hilman, Andreas Färber,
	Thierry Reding, Jonathan Cameron
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	letux-kernel-S0jZdbWzriLCfDggNXIi3w,
	kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cover.1510781875.git.hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>

add bindings documentation for Wi2Wi W2SG00x4 GPS module.

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/gps/wi2wi,w2sg0004.txt     | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt

diff --git a/Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
new file mode 100644
index 000000000000..bcf307382afb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
@@ -0,0 +1,24 @@
+Wi2Wi GPS module connected through UART
+
+Should be a subnode of the SoC UART it is connected to.
+
+Required properties:
+- compatible:	should be one of (depending on precise model)
+	"wi2wi,w2sg0004"
+	"wi2wi,w2sg0084"
+- enable-gpio:	the GPIO that controls the module's power toggle
+	input. A positive impulse of sufficent length toggles the
+	power state.
+
+Optional properties:
+- lna-supply:	an (optional) LNA regulator that is enabled together with the GPS receiver
+
+Example:
+
+&uart2 {
+	w2sg0004: gps {
+		compatible = "wi2wi,w2sg0004";
+		lna-supply = <&vsim>;   /* LNA regulator */
+		enable-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;    /* GPIO_145: trigger for turning on/off w2sg0004 */
+        };
+};
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v4 1/5] dt-bindings: vendor prefix for Wi2Wi, Inc.
From: H. Nikolaus Schaller @ 2017-11-15 21:37 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Russell King, Arnd Bergmann, Greg Kroah-Hartman,
	H. Nikolaus Schaller, Kevin Hilman, Andreas Färber,
	Thierry Reding, Jonathan Cameron
  Cc: devicetree, linux-kernel, linux-omap, letux-kernel, kernel,
	linux-arm-kernel
In-Reply-To: <cover.1510781875.git.hns@goldelico.com>

Introduce vendor prefix for Wi2Wi, Inc. for W2SG00x4 GPS modules
and W2CBW003 Bluetooth/WiFi combo (CSR/Marvell).

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 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 1afd298eddd7..ad4381738954 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -366,6 +366,7 @@ voipac	Voipac Technologies s.r.o.
 wd	Western Digital Corp.
 wetek	WeTek Electronics, limited.
 wexler	Wexler
+wi2wi	Wi2Wi, Inc.
 winbond Winbond Electronics corp.
 winstar	Winstar Display Corp.
 wlf	Wolfson Microelectronics
-- 
2.12.2

^ permalink raw reply related

* [PATCH v4 0/5] misc serdev: new serdev based driver for Wi2Wi w2sg00x4 GPS module
From: H. Nikolaus Schaller @ 2017-11-15 21:37 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Russell King, Arnd Bergmann, Greg Kroah-Hartman,
	H. Nikolaus Schaller, Kevin Hilman, Andreas Färber,
	Thierry Reding, Jonathan Cameron
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	letux-kernel-S0jZdbWzriLCfDggNXIi3w,
	kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

Changes V4:
* removed all pdata remains
* added one missing Signes-off-By

2017-11-15 16:19:17: Changes V3:
* worked in suggestions by kbuild test robot
* added misc+serdev to the subject

2017-11-12 22:00:02: Changes V2:
* reduced to submit only w2sg00x4 GPS driver code
* add DT node for GTA04 device to make use of the driver
* split into base code and a debugging Kconfig option (brings device into false power state after boot)
* worked in comments by kbuild robot and Rob Herring

2017-05-21 12:44:07: RFC V1
* RFC concerning new serdev based drivers for Wi2Wi w2sg00x4 GPS module and w2cbw003 bluetooth

H. Nikolaus Schaller (5):
  dt-bindings: vendor prefix for Wi2Wi, Inc.
  dt-bindings: gps: add w2sg00x4 bindings documentation (GPS module with
    UART))
  misc serdev: Add w2sg0004 (gps receiver) power control driver
  DTS: gta04: add serdev node for w2sg00x4
  misc serdev: w2sg0004: add debugging code and Kconfig

 .../devicetree/bindings/gps/wi2wi,w2sg0004.txt     |  24 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 arch/arm/boot/dts/omap3-gta04.dtsi                 |   6 +
 drivers/misc/Kconfig                               |  18 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/w2sg0004.c                            | 582 +++++++++++++++++++++
 6 files changed, 632 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
 create mode 100644 drivers/misc/w2sg0004.c

-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver
From: Dan Murphy @ 2017-11-15 21:25 UTC (permalink / raw)
  To: Pavel Machek
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	rpurdie-Fm38FmjxZ/leoWH0uzbU5w,
	jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-leds-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171115211236.GD6183@amd>

Pavel

On 11/15/2017 03:12 PM, Pavel Machek wrote:
> Hi!
> 
>> Introducing the LM3692x Dual-String white LED driver.
>>
>> Data sheet is located
> 
> "located at"? (Twice.)

Actually it is 3x since I call it out in the dt binding as well.

So what to eliminate?

> 
>> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
> 
>> +config LEDS_LM3692X
>> +	tristate "LED support for LM3692x Chips"
>> +	depends on LEDS_CLASS && I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  This option enables support for the TI LM3692x family
>> +	  of LED drivers.
> 
> "If unsure ..., module will be named..."
> 
> Might want to say this is for backlight LEDs here.

AcK

> 
>> +static int lm3692x_fault_check(struct lm3692x_led *led)
>> +{
>> +	int ret, fault;
>> +	unsigned int read_buf;
>> +
>> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fault = read_buf;
>> +
>> +	if (fault)
>> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
>> +			fault);
>> +
>> +	return ret;
>> +}
> 
> Get rid of "fault" variable?
> 
> Does fault need to be propagated to the caller?

I should probably set ret to fail if I see a fault or as you suggest propagate the fault

> 
> 
>> +static int lm3692x_init(struct lm3692x_led *led)
>> +{
>> +	int ret;
>> +
>> +	if (led->regulator) {
>> +		ret = regulator_enable(led->regulator);
>> +		if (ret)
>> +			dev_err(&led->client->dev,
>> +				"Failed to enable regulator\n");
>> +			goto out;
>> +	}
>> +
>> +	if (led->enable_gpio)
>> +		gpiod_direction_output(led->enable_gpio, 1);
>> +
>> +	ret = lm3692x_fault_check(led);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
>> +		goto out;
>> +	}
> 
> How often are those fails reached? Maybe regmap wrapper that would
> print "reading/writing register XY failed" would be enough?

Or maybe in the out label I can just dev_err once saying initializing the device failed.
These are really only called at probe time.  Its the only time init is called.

Dan

> 
> Otherwise looks good,
> 
> Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> 									Pavel
> 


-- 
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver
From: Pavel Machek @ 2017-11-15 21:12 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	rpurdie-Fm38FmjxZ/leoWH0uzbU5w,
	jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-leds-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171115194203.13572-2-dmurphy-l0cyMroinI0@public.gmane.org>

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

Hi!

> Introducing the LM3692x Dual-String white LED driver.
> 
> Data sheet is located

"located at"? (Twice.)

> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>

> +config LEDS_LM3692X
> +	tristate "LED support for LM3692x Chips"
> +	depends on LEDS_CLASS && I2C
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for the TI LM3692x family
> +	  of LED drivers.

"If unsure ..., module will be named..."

Might want to say this is for backlight LEDs here.

> +static int lm3692x_fault_check(struct lm3692x_led *led)
> +{
> +	int ret, fault;
> +	unsigned int read_buf;
> +
> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
> +	if (ret)
> +		return ret;
> +
> +	fault = read_buf;
> +
> +	if (fault)
> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
> +			fault);
> +
> +	return ret;
> +}

Get rid of "fault" variable?

Does fault need to be propagated to the caller?


> +static int lm3692x_init(struct lm3692x_led *led)
> +{
> +	int ret;
> +
> +	if (led->regulator) {
> +		ret = regulator_enable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to enable regulator\n");
> +			goto out;
> +	}
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 1);
> +
> +	ret = lm3692x_fault_check(led);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
> +		goto out;
> +	}

How often are those fails reached? Maybe regmap wrapper that would
print "reading/writing register XY failed" would be enough?

Otherwise looks good,

Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* [PATCH] ARM: dts: bcm2835-rpi: Avoid conflicts on i2c0
From: Stefan Wahren @ 2017-11-15 20:57 UTC (permalink / raw)
  To: Eric Anholt, Rob Herring, Mark Rutland
  Cc: Florian Fainelli, Scott Branden,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	pbrobinson-Re5JQEeQqe8AvxtiuMwx3w, Stefan Wahren

The GPU firmware of the Raspberry Pi 3 uses i2c0 to communicate to
the FXL6408 gpio expander. It's a bad idea to use the same interface
from the ARM side. Since this interface isn't used by the other
RPi boards, it's save to remove the complete node and avoid this
conflict.

Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
Fixes: 9d56c22a7861 ("ARM: bcm2835: Add devicetree for the Raspberry Pi 3.")
---
 arch/arm/boot/dts/bcm2835-rpi.dtsi | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index e36c392..995d17b 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -44,13 +44,6 @@
 	};
 };
 
-&i2c0 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&i2c0_gpio0>;
-	status = "okay";
-	clock-frequency = <100000>;
-};
-
 &i2c1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c1_gpio2>;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH v2 3/5] misc: Add w2sg0004 (gps receiver) power control driver
From: H. Nikolaus Schaller @ 2017-11-15 20:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, Benoît Cousson, Tony Lindgren, Russell King,
	Thierry Reding, Jonathan Cameron, Maxime Ripard, Jarkko Sakkinen,
	devicetree, linux-kernel, linux-omap, letux-kernel, kernel
In-Reply-To: <20171115201756.qjmo26wttg55d57m@rob-hp-laptop>

Hi Rob,

> Am 15.11.2017 um 21:17 schrieb Rob Herring <robh@kernel.org>:
> 
> On Sun, Nov 12, 2017 at 09:59:56PM +0100, H. Nikolaus Schaller wrote:
>> Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.
>> 
>> Use serdev API hooks to monitor and forward the UART traffic to /dev/GPSn
>> and turn on/off the module. It also detects if the module is turned on (sends data)
>> but should be off, e.g. if it was already turned on during boot or power-on-reset.
>> 
>> Additionally, rfkill block/unblock can be used to control an external LNA
>> (and power down the module if not needed).
>> 
>> The driver concept is based on code developed by NeilBrown <neilb@suse.de>
>> but simplified and adapted to use the new serdev API introduced in 4.11.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/misc/Kconfig     |  10 +
>> drivers/misc/Makefile    |   1 +
>> drivers/misc/w2sg0004.c  | 565 +++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/w2sg0004.h |  27 +++
>> 4 files changed, 603 insertions(+)
>> create mode 100644 drivers/misc/w2sg0004.c
>> create mode 100644 include/linux/w2sg0004.h
>> 
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 8136dc7e863d..09d171d68408 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -518,4 +518,14 @@ source "drivers/misc/mic/Kconfig"
>> source "drivers/misc/genwqe/Kconfig"
>> source "drivers/misc/echo/Kconfig"
>> source "drivers/misc/cxl/Kconfig"
>> +
>> +config W2SG0004
>> +	tristate "W2SG00x4 on/off control"
>> +	depends on GPIOLIB && SERIAL_DEV_BUS
>> +	help
>> +          Enable on/off control of W2SG00x4 GPS moduled connected
>> +	  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
>> +	  is opened/closed.
>> +	  It also provides a rfkill gps name to control the LNA power.
>> +
>> endmenu
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index ad0e64fdba34..abcb667e0ff0 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -51,6 +51,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.o
>> obj-y				+= mic/
>> obj-$(CONFIG_GENWQE)		+= genwqe/
>> obj-$(CONFIG_ECHO)		+= echo/
>> +obj-$(CONFIG_W2SG0004)		+= w2sg0004.o
>> obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>> obj-$(CONFIG_CXL_BASE)		+= cxl/
>> obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
>> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
>> new file mode 100644
>> index 000000000000..55101aa6beeb
>> --- /dev/null
>> +++ b/drivers/misc/w2sg0004.c
>> @@ -0,0 +1,565 @@
>> +/*
>> + * w2sg0004.c
> 
> Listing the filename is pointless IMO.

Ok.

> You need a license too. Use SPDX 
> tag (on first line with C++ style comment).

Well, this is too new to be aware of...
Isn't the MODULE_LICENSE enough any more?

> 
>> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
>> + *
>> + * This receiver has an ON/OFF pin which must be toggled to
>> + * turn the device 'on' of 'off'.  A high->low->high toggle
>> + * will switch the device on if it is off, and off if it is on.
>> + *
>> + * To enable receiving on/off requests we register with the
>> + * UART power management notifications.
>> + *
>> + * It is not possible to directly detect the state of the device.
>> + * However when it is on it will send characters on a UART line
>> + * regularly.
>> + *
>> + * To detect that the power state is out of sync (e.g. if GPS
>> + * was enabled before a reboot), we register for UART data received
>> + * notifications.
>> + *
>> + * In addition we register as a rfkill client so that we can
>> + * control the LNA power.
>> + *
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/rfkill.h>
>> +#include <linux/serdev.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +#include <linux/w2sg0004.h>
>> +#include <linux/workqueue.h>
>> +
>> +/*
>> + * There seems to be restrictions on how quickly we can toggle the
>> + * on/off line.  data sheets says "two rtc ticks", whatever that means.
>> + * If we do it too soon it doesn't work.
>> + * So we have a state machine which uses the common work queue to ensure
>> + * clean transitions.
>> + * When a change is requested we record that request and only act on it
>> + * once the previous change has completed.
>> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
>> + * one change per second.
>> + */
>> +
>> +enum w2sg_state {
>> +	W2SG_IDLE,	/* is not changing state */
>> +	W2SG_PULSE,	/* activate on/off impulse */
>> +	W2SG_NOPULSE	/* deactivate on/off impulse */
>> +};
>> +
>> +struct w2sg_data {
>> +	struct		rfkill *rf_kill;
>> +	struct		regulator *lna_regulator;
>> +	int		lna_blocked;	/* rfkill block gps active */
>> +	int		lna_is_off;	/* LNA is currently off */
>> +	int		is_on;		/* current state (0/1) */
>> +	unsigned long	last_toggle;
>> +	unsigned long	backoff;	/* time to wait since last_toggle */
>> +	int		on_off_gpio;	/* the on-off gpio number */
>> +	struct		serdev_device *uart;	/* uart connected to the chip */
>> +	struct		tty_driver *tty_drv;	/* this is the user space tty */
>> +	struct		device *dev;	/* from tty_port_register_device() */
>> +	struct		tty_port port;
>> +	int		open_count;	/* how often we were opened */
>> +	enum		w2sg_state state;
>> +	int		requested;	/* requested state (0/1) */
>> +	int		suspended;
>> +	struct delayed_work work;
>> +	int		discard_count;
>> +};
>> +
>> +static struct w2sg_data *w2sg_by_minor[1];
>> +
>> +static int w2sg_set_lna_power(struct w2sg_data *data)
>> +{
>> +	int ret = 0;
>> +	int off = data->suspended || !data->requested || data->lna_blocked;
>> +
>> +	pr_debug("%s: %s\n", __func__, off ? "off" : "on");
>> +
>> +	if (off != data->lna_is_off) {
>> +		data->lna_is_off = off;
>> +		if (!IS_ERR_OR_NULL(data->lna_regulator)) {
>> +			if (off)
>> +				regulator_disable(data->lna_regulator);
>> +			else
>> +				ret = regulator_enable(data->lna_regulator);
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void w2sg_set_power(void *pdata, int val)
>> +{
>> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
>> +
>> +	pr_debug("%s to state=%d (requested=%d)\n", __func__, val, data->requested);
>> +
>> +	if (val && !data->requested) {
>> +		data->requested = true;
>> +	} else if (!val && data->requested) {
>> +		data->backoff = HZ;
>> +		data->requested = false;
>> +	} else
>> +		return;
>> +
>> +	pr_debug("w2sg00x4 scheduled for %d\n", data->requested);
>> +
>> +	if (!data->suspended)
>> +		schedule_delayed_work(&data->work, 0);
>> +}
>> +
>> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
>> +
>> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
>> +				const unsigned char *rxdata,
>> +				size_t count)
>> +{
>> +	struct w2sg_data *data =
>> +		(struct w2sg_data *) serdev_device_get_drvdata(serdev);
>> +
>> +	if (!data->requested && !data->is_on) {
>> +		/*
>> +		 * we have received characters while the w2sg
>> +		 * should have been be turned off
>> +		 */
>> +		data->discard_count += count;
>> +		if ((data->state == W2SG_IDLE) &&
>> +		    time_after(jiffies,
>> +		    data->last_toggle + data->backoff)) {
>> +			/* Should be off by now, time to toggle again */
>> +			pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
>> +				data->discard_count);
>> +
>> +			data->discard_count = 0;
>> +
>> +			data->is_on = true;
>> +			data->backoff *= 2;
>> +			if (!data->suspended)
>> +				schedule_delayed_work(&data->work, 0);
>> +		}
>> +	} else if (data->open_count > 0) {
>> +		int n;
>> +
>> +		pr_debug("w2sg00x4: push %d chars to tty port\n", count);
>> +
>> +		/* pass to user-space */
>> +		n = tty_insert_flip_string(&data->port, rxdata, count);
>> +		if (n != count)
>> +			pr_err("w2sg00x4: did loose %d characters\n", count - n);
>> +		tty_flip_buffer_push(&data->port);
>> +		return n;
>> +	}
>> +
>> +	/* assume we have processed everything */
>> +	return count;
>> +}
>> +
>> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
>> +
>> +static void toggle_work(struct work_struct *work)
>> +{
>> +	struct w2sg_data *data = container_of(work, struct w2sg_data,
>> +					      work.work);
>> +
>> +	switch (data->state) {
>> +	case W2SG_IDLE:
>> +		if (data->requested == data->is_on)
>> +			return;
>> +
>> +		w2sg_set_lna_power(data);	/* update LNA power state */
>> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
>> +		data->state = W2SG_PULSE;
>> +
>> +		pr_debug("w2sg: power gpio ON\n");
>> +
>> +		schedule_delayed_work(&data->work,
>> +				      msecs_to_jiffies(10));
>> +		break;
>> +
>> +	case W2SG_PULSE:
>> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +		data->last_toggle = jiffies;
>> +		data->state = W2SG_NOPULSE;
>> +		data->is_on = !data->is_on;
>> +
>> +		pr_debug("w2sg: power gpio OFF\n");
>> +
>> +		schedule_delayed_work(&data->work,
>> +				      msecs_to_jiffies(10));
>> +		break;
>> +
>> +	case W2SG_NOPULSE:
>> +		data->state = W2SG_IDLE;
>> +
>> +		pr_debug("w2sg: idle\n");
>> +
>> +		break;
>> +
>> +	}
>> +}
>> +
>> +static int w2sg_rfkill_set_block(void *pdata, bool blocked)
>> +{
>> +	struct w2sg_data *data = pdata;
>> +
>> +	pr_debug("%s: blocked: %d\n", __func__, blocked);
>> +
>> +	data->lna_blocked = blocked;
>> +
>> +	return w2sg_set_lna_power(data);
>> +}
>> +
>> +static struct rfkill_ops w2sg0004_rfkill_ops = {
>> +	.set_block = w2sg_rfkill_set_block,
>> +};
>> +
>> +static struct serdev_device_ops serdev_ops = {
>> +	.receive_buf = w2sg_uart_receive_buf,
>> +};
>> +
>> +/*
>> + * we are a man-in the middle between the user-space visible tty port
>> + * and the serdev tty where the chip is connected.
>> + * This allows us to recognise when the device should be powered on
>> + * or off and handle the "false" state that data arrives while no
>> + * users-space tty client exists.
>> + */
>> +
>> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
>> +{
>> +	return w2sg_by_minor[minor];
>> +}
>> +
>> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>> +{
>> +	struct w2sg_data *data;
>> +	int retval;
>> +
>> +	pr_debug("%s() tty = %p\n", __func__, tty);
>> +
>> +	data = w2sg_get_by_minor(tty->index);
>> +
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	retval = tty_standard_install(driver, tty);
>> +	if (retval)
>> +		goto error_init_termios;
>> +
>> +	tty->driver_data = data;
>> +
>> +	return 0;
>> +
>> +error_init_termios:
>> +	tty_port_put(&data->port);
>> +	return retval;
>> +}
>> +
>> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
>> +{
>> +	struct w2sg_data *data = tty->driver_data;
>> +
>> +	pr_debug("%s() data = %p open_count = ++%d\n", __func__, data, data->open_count);
>> +
>> +	w2sg_set_power(data, ++data->open_count > 0);
>> +
>> +	return tty_port_open(&data->port, tty, file);
>> +}
>> +
>> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
>> +{
>> +	struct w2sg_data *data = tty->driver_data;
>> +
>> +	pr_debug("%s()\n", __func__);
>> +
>> +	w2sg_set_power(data, --data->open_count > 0);
>> +
>> +	tty_port_close(&data->port, tty, file);
>> +}
>> +
>> +static int w2sg_tty_write(struct tty_struct *tty,
>> +		const unsigned char *buffer, int count)
>> +{
>> +	struct w2sg_data *data = tty->driver_data;
>> +
>> +	/* simply pass down to UART */
>> +	return serdev_device_write_buf(data->uart, buffer, count);
>> +}
>> +
>> +
>> +static const struct tty_operations w2sg_serial_ops = {
>> +	.install = w2sg_tty_install,
>> +	.open = w2sg_tty_open,
>> +	.close = w2sg_tty_close,
>> +	.write = w2sg_tty_write,
>> +};
>> +
>> +static const struct tty_port_operations w2sg_port_ops = {
>> +};
>> +
>> +static int w2sg_probe(struct serdev_device *serdev)
>> +{
>> +	struct w2sg_pdata *pdata = NULL;
>> +	struct w2sg_data *data;
>> +	struct rfkill *rf_kill;
>> +	int err;
>> +	int minor;
>> +
>> +	pr_debug("%s()\n", __func__);
>> +
>> +	minor = 0;
>> +	if (w2sg_by_minor[minor]) {
>> +		pr_err("w2sg minor is already in use!\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (serdev->dev.of_node) {
>> +		struct device *dev = &serdev->dev;
>> +		enum of_gpio_flags flags;
>> +
>> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +		if (!pdata)
>> +			return -ENOMEM;
>> +
>> +		pdata->on_off_gpio = of_get_named_gpio_flags(dev->of_node,
>> +							     "enable-gpios", 0,
>> +							     &flags);
>> +
>> +		/* defer until we have all gpios */
>> +		if (pdata->on_off_gpio == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
>> +
>> +		pdata->lna_regulator = devm_regulator_get_optional(dev, "lna");
>> +		if (IS_ERR(pdata->lna_regulator)) {
>> +			/* defer until we can get the regulator */
>> +			if (PTR_ERR(pdata->lna_regulator) == -EPROBE_DEFER)
>> +				return -EPROBE_DEFER;
>> +
>> +			pdata->lna_regulator = NULL;
>> +		}
>> +
>> +		pr_debug("%s() lna_regulator = %p\n", __func__,
>> +			pdata->lna_regulator);
>> +
>> +		serdev->dev.platform_data = pdata;
>> +	}
>> +
>> +	data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (data == NULL)
>> +		return -ENOMEM;
>> +
>> +	w2sg_by_minor[minor] = data;
>> +
>> +	serdev_device_set_drvdata(serdev, data);
>> +
>> +	data->lna_regulator = pdata->lna_regulator;
> 
> NULL ptr dereference if you have no of_node.

has more or less already commented by Arnd and will be fixed in PATCH v4.

> 
>> +	data->lna_blocked = true;
>> +	data->lna_is_off = true;
>> +
>> +	data->on_off_gpio = pdata->on_off_gpio;
>> +
>> +	data->is_on = false;
>> +	data->requested = false;
>> +	data->state = W2SG_IDLE;
>> +	data->last_toggle = jiffies;
>> +	data->backoff = HZ;
>> +
>> +	data->uart = serdev;
>> +
>> +	INIT_DELAYED_WORK(&data->work, toggle_work);
>> +
>> +	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
>> +				"w2sg0004-on-off");
>> +	if (err < 0)
>> +		goto out;
>> +
>> +	gpio_direction_output(data->on_off_gpio, false);
>> +
>> +	serdev_device_set_client_ops(data->uart, &serdev_ops);
>> +	serdev_device_open(data->uart);
>> +
>> +	serdev_device_set_baudrate(data->uart, 9600);
>> +	serdev_device_set_flow_control(data->uart, false);
>> +
>> +	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
>> +				&w2sg0004_rfkill_ops, data);
>> +	if (rf_kill == NULL) {
>> +		err = -ENOMEM;
>> +		goto err_rfkill;
>> +	}
>> +
>> +	err = rfkill_register(rf_kill);
>> +	if (err) {
>> +		dev_err(&serdev->dev, "Cannot register rfkill device\n");
>> +		goto err_rfkill;
>> +	}
>> +
>> +	data->rf_kill = rf_kill;
>> +
>> +	/* allocate the tty driver */
>> +	data->tty_drv = alloc_tty_driver(1);
> 
> Is there an advantage or reason to make this a tty driver rather than 
> just a char driver?

Well, gpsd and other daemons might assume they can tcsetattr or a line discipline etc.

> 
> Perhaps this should be under drivers/tty if it is a tty driver.

Well, primarily it is a gps driver... And in some bright future there
should be specific gps interfaces (like networking or bluetooth).

But there is no drivers/gps and I am not sure if this one should be the
first. If you prefer it, it is a simple rename...

> 
>> +	if (!data->tty_drv)
>> +		return -ENOMEM;
>> +
>> +	/* initialize the tty driver */
>> +	data->tty_drv->owner = THIS_MODULE;
>> +	data->tty_drv->driver_name = "w2sg0004";
>> +	data->tty_drv->name = "ttyGPS";
>> +	data->tty_drv->major = 0;
>> +	data->tty_drv->minor_start = 0;
>> +	data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
>> +	data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
>> +	data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
>> +	data->tty_drv->init_termios = tty_std_termios;
>> +	data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
>> +					      HUPCL | CLOCAL;
>> +	/*
>> +	 * optional:
>> +	 * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
>> +					115200, 115200);
>> +	 * w2sg_tty_termios(&data->tty_drv->init_termios);
>> +	 */
>> +	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
>> +
>> +	/* register the tty driver */
>> +	err = tty_register_driver(data->tty_drv);
>> +	if (err) {
>> +		pr_err("%s - tty_register_driver failed(%d)\n",
>> +			__func__, err);
>> +		put_tty_driver(data->tty_drv);
>> +		goto err_rfkill;
>> +	}
>> +
>> +	tty_port_init(&data->port);
>> +	data->port.ops = &w2sg_port_ops;
>> +
>> +/*
>> + * FIXME: this appears to reenter this probe() function a second time
>> + * which only fails because the gpio is already assigned
>> + */
>> +
>> +	data->dev = tty_port_register_device(&data->port,
>> +			data->tty_drv, minor, &serdev->dev);
>> +
>> +	pr_debug("w2sg probed\n");
>> +
>> +	/* keep off until user space requests the device */
>> +	w2sg_set_power(data, false);
>> +
>> +	return 0;
>> +
>> +err_rfkill:
>> +	rfkill_destroy(rf_kill);
>> +	serdev_device_close(data->uart);
>> +out:
>> +	return err;
>> +}
>> +
>> +static void w2sg_remove(struct serdev_device *serdev)
>> +{
>> +	struct w2sg_data *data = serdev_device_get_drvdata(serdev);
>> +	int minor;
>> +
>> +	cancel_delayed_work_sync(&data->work);
>> +
>> +	/* what is the right sequence to avoid problems? */
>> +	serdev_device_close(data->uart);
>> +
>> +	minor = 0;
>> +	tty_unregister_device(data->tty_drv, minor);
>> +
>> +	tty_unregister_driver(data->tty_drv);
>> +}
>> +
>> +static int w2sg_suspend(struct device *dev)
>> +{
>> +	struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> +	data->suspended = true;
>> +
>> +	cancel_delayed_work_sync(&data->work);
>> +
>> +	w2sg_set_lna_power(data);	/* shuts down if needed */
>> +
>> +	if (data->state == W2SG_PULSE) {
>> +		msleep(10);
>> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +		data->last_toggle = jiffies;
>> +		data->is_on = !data->is_on;
>> +		data->state = W2SG_NOPULSE;
>> +	}
>> +
>> +	if (data->state == W2SG_NOPULSE) {
>> +		msleep(10);
>> +		data->state = W2SG_IDLE;
>> +	}
>> +
>> +	if (data->is_on) {
>> +		pr_info("GPS off for suspend %d %d %d\n", data->requested,
>> +			data->is_on, data->lna_is_off);
>> +
>> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
>> +		msleep(10);
>> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +		data->is_on = 0;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int w2sg_resume(struct device *dev)
>> +{
>> +	struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> +	data->suspended = false;
>> +
>> +	pr_info("GPS resuming %d %d %d\n", data->requested,
>> +		data->is_on, data->lna_is_off);
>> +
>> +	schedule_delayed_work(&data->work, 0);	/* enables LNA if needed */
>> +
>> +	return 0;
>> +}
>> +
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id w2sg0004_of_match[] = {
>> +	{ .compatible = "wi2wi,w2sg0004" },
>> +	{ .compatible = "wi2wi,w2sg0084" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
>> +#endif
>> +
>> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
>> +
>> +static struct serdev_device_driver w2sg_driver = {
>> +	.probe		= w2sg_probe,
>> +	.remove		= w2sg_remove,
>> +	.driver = {
>> +		.name	= "w2sg0004",
>> +		.owner	= THIS_MODULE,
>> +		.pm	= &w2sg_pm_ops,
>> +		.of_match_table = of_match_ptr(w2sg0004_of_match)
>> +	},
>> +};
>> +
>> +module_serdev_device_driver(w2sg_driver);
>> +
>> +MODULE_ALIAS("w2sg0004");
>> +
>> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/w2sg0004.h b/include/linux/w2sg0004.h
>> new file mode 100644
>> index 000000000000..ad0c4a18e01d
>> --- /dev/null
>> +++ b/include/linux/w2sg0004.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * UART slave to allow ON/OFF control of w2sg0004 GPS receiver.
>> + *
>> + * Copyright (C) 2011 Neil Brown <neil@brown.name>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +
>> +
>> +#ifndef __LINUX_W2SG0004_H
>> +#define __LINUX_W2SG0004_H
>> +
>> +#include <linux/regulator/consumer.h>
>> +
>> +struct w2sg_pdata {
> 
> You don't really support platform_data, so you can move this into the 
> driver.

Yes, I am working on that as recommended by Arnd.

> 
>> +	struct regulator *lna_regulator;	/* enable LNA power */
>> +	int	on_off_gpio;	/*  on-off input of the GPS module */
>> +};
>> +#endif /* __LINUX_W2SG0004_H */
>> -- 
>> 2.12.2
>> 


BR and thanks,
Nikolaus

^ permalink raw reply

* Re: [PATCH v2 2/5] DT: misc: w2sg0004: add bindings documentation (GPS module with serdev UART)
From: H. Nikolaus Schaller @ 2017-11-15 20:36 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, Benoît Cousson, Tony Lindgren, Russell King,
	Thierry Reding, Jonathan Cameron, Maxime Ripard, Jarkko Sakkinen,
	devicetree, linux-kernel, linux-omap, letux-kernel, kernel
In-Reply-To: <20171115200808.2nkldpoybhv5x6a5@rob-hp-laptop>

Hi Rob,

> Am 15.11.2017 um 21:08 schrieb Rob Herring <robh@kernel.org>:
> 
> On Sun, Nov 12, 2017 at 09:59:55PM +0100, H. Nikolaus Schaller wrote:
>> add bindings documentation for Wi2Wi W2SG00x4 GPS module
> 
> Add ... module.

Ok.

> 
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt    | 24 ++++++++++++++++++++++
> 
> The path doesn't have to match the kernel. Use bindings/gps/...

Ok.

> 
>> 1 file changed, 24 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> new file mode 100644
>> index 000000000000..ccec9361a1a4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> @@ -0,0 +1,24 @@
>> +Wi2Wi GPS module connected through UART
>> +
>> +Should be a subnode of the SoC UART it is connected to (serdev).
> 
> Could be connected to any kind of UART and serdev is a Linuxism.
> 
> With those fixed,
> 
> Acked-by: Rob Herring <robh@kernel.org>


Fine and thanks,
Nikolaus Schaller

PS: there is already a [PATCH v3] (no changes for 1/5 and 2/5)

^ permalink raw reply

* Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver
From: Dan Murphy @ 2017-11-15 20:31 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	rpurdie-Fm38FmjxZ/leoWH0uzbU5w, pavel-+ZI9xUNit7I,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-leds-u79uwXL29TY76Z2rM5mHXA, Lee Jones, Daniel Thompson,
	Jingoo Han
In-Reply-To: <cb5c5a65-d5a0-86a3-36b4-cf06baae9ec4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Jacek

On 11/15/2017 02:15 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> Thanks for the patch.
> 
> On 11/15/2017 08:42 PM, Dan Murphy wrote:
>> This adds the devicetree bindings for the LM3692x
>> I2C LED string driver.
>>
>> Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
>> Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
>> ---
>>
>> v3 - No changes
>> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
>>
>>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>> new file mode 100644
>> index 000000000000..cfef67bd4100
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>> @@ -0,0 +1,28 @@
>> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
>> +
>> +The LM36922 is an ultra-compact, highly efficient,
>> +two string white-LED driver designed for LCD display
>> +backlighting.
> 
> Shouldn't the driver be targeted for backlight subsystem then?
> 
> Adding backlight maintainers.
> 

Great point!  I was not aware of the backlight subsystem.

Looks like I need to create a back light version as well.

Like the lp8788 did since this can be used as a LED driver beyond display back lighting.

Unless the maintainers don't want it.

Dan


-- 
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 3/3] arm64: dts: renesas: eagle: add EtherAVB pins
From: Sergei Shtylyov @ 2017-11-15 20:21 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Simon Horman, Rob Herring, Catalin Marinas, Will Deacon,
	Linux-Renesas, devicetree@vger.kernel.org, Magnus Damm,
	Mark Rutland, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAMuHMdX8j__iF5xP33Na9pVM1nRWvrE7e68-zg0oE0gE5MOHXw@mail.gmail.com>

Hello!

On 11/15/2017 07:21 PM, Geert Uytterhoeven wrote:

>>>> Add the (previously omitted) EtherAVB pin data to the Eagle board's
>>>> device tree.
> 
>>>> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>>>> +++ renesas/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>>>> @@ -34,6 +34,9 @@
>>
>> [...]
>>>>
>>>> @@ -53,6 +56,11 @@
>>>>    };
>>>>
>>>>    &pfc {
>>>> +       avb_pins: avb {
>>>> +               groups = "avb0_mdio", "avb0_mii";
>>>
>>>
>>> Oh no, its'called "avb0_mdio" here, but "avb(0)_mdc" on all other
>>> R-Car Gen3 SoCs?
>>
>>
>>     Can you remember the reason? I don;t want to follow the bad example. :-)
> 
> Sorry, I don't know.

    Hm, you seem to have participated in the related discussions for H2/M3-W...
The reason was that omlu AVB_MDC signal was multiplexed, AVB_MDIO signal (and 
AVB MII signals) was mapped to a dedicated pin without any multiplexing, so 
the "avb_mdc" group initially contained only AVB_MDC.

> Gr{oetje,eeting}s,

MBR, Sergei

^ permalink raw reply

* Re: [PATCH v2 3/5] misc: Add w2sg0004 (gps receiver) power control driver
From: Rob Herring @ 2017-11-15 20:17 UTC (permalink / raw)
  To: H. Nikolaus Schaller
  Cc: Mark Rutland, Benoît Cousson, Tony Lindgren, Russell King,
	Thierry Reding, Jonathan Cameron, Maxime Ripard, Jarkko Sakkinen,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	letux-kernel-S0jZdbWzriLCfDggNXIi3w,
	kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf
In-Reply-To: <1dd7e2ccf3d0361c0bb947bacbc2c00066e528c4.1510520397.git.hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>

On Sun, Nov 12, 2017 at 09:59:56PM +0100, H. Nikolaus Schaller wrote:
> Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.
> 
> Use serdev API hooks to monitor and forward the UART traffic to /dev/GPSn
> and turn on/off the module. It also detects if the module is turned on (sends data)
> but should be off, e.g. if it was already turned on during boot or power-on-reset.
> 
> Additionally, rfkill block/unblock can be used to control an external LNA
> (and power down the module if not needed).
> 
> The driver concept is based on code developed by NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
> but simplified and adapted to use the new serdev API introduced in 4.11.
> 
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> ---
>  drivers/misc/Kconfig     |  10 +
>  drivers/misc/Makefile    |   1 +
>  drivers/misc/w2sg0004.c  | 565 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/w2sg0004.h |  27 +++
>  4 files changed, 603 insertions(+)
>  create mode 100644 drivers/misc/w2sg0004.c
>  create mode 100644 include/linux/w2sg0004.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 8136dc7e863d..09d171d68408 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -518,4 +518,14 @@ source "drivers/misc/mic/Kconfig"
>  source "drivers/misc/genwqe/Kconfig"
>  source "drivers/misc/echo/Kconfig"
>  source "drivers/misc/cxl/Kconfig"
> +
> +config W2SG0004
> +	tristate "W2SG00x4 on/off control"
> +	depends on GPIOLIB && SERIAL_DEV_BUS
> +	help
> +          Enable on/off control of W2SG00x4 GPS moduled connected
> +	  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
> +	  is opened/closed.
> +	  It also provides a rfkill gps name to control the LNA power.
> +
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index ad0e64fdba34..abcb667e0ff0 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.o
>  obj-y				+= mic/
>  obj-$(CONFIG_GENWQE)		+= genwqe/
>  obj-$(CONFIG_ECHO)		+= echo/
> +obj-$(CONFIG_W2SG0004)		+= w2sg0004.o
>  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
> new file mode 100644
> index 000000000000..55101aa6beeb
> --- /dev/null
> +++ b/drivers/misc/w2sg0004.c
> @@ -0,0 +1,565 @@
> +/*
> + * w2sg0004.c

Listing the filename is pointless IMO. You need a license too. Use SPDX 
tag (on first line with C++ style comment).

> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
> + *
> + * This receiver has an ON/OFF pin which must be toggled to
> + * turn the device 'on' of 'off'.  A high->low->high toggle
> + * will switch the device on if it is off, and off if it is on.
> + *
> + * To enable receiving on/off requests we register with the
> + * UART power management notifications.
> + *
> + * It is not possible to directly detect the state of the device.
> + * However when it is on it will send characters on a UART line
> + * regularly.
> + *
> + * To detect that the power state is out of sync (e.g. if GPS
> + * was enabled before a reboot), we register for UART data received
> + * notifications.
> + *
> + * In addition we register as a rfkill client so that we can
> + * control the LNA power.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/rfkill.h>
> +#include <linux/serdev.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/w2sg0004.h>
> +#include <linux/workqueue.h>
> +
> +/*
> + * There seems to be restrictions on how quickly we can toggle the
> + * on/off line.  data sheets says "two rtc ticks", whatever that means.
> + * If we do it too soon it doesn't work.
> + * So we have a state machine which uses the common work queue to ensure
> + * clean transitions.
> + * When a change is requested we record that request and only act on it
> + * once the previous change has completed.
> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
> + * one change per second.
> + */
> +
> +enum w2sg_state {
> +	W2SG_IDLE,	/* is not changing state */
> +	W2SG_PULSE,	/* activate on/off impulse */
> +	W2SG_NOPULSE	/* deactivate on/off impulse */
> +};
> +
> +struct w2sg_data {
> +	struct		rfkill *rf_kill;
> +	struct		regulator *lna_regulator;
> +	int		lna_blocked;	/* rfkill block gps active */
> +	int		lna_is_off;	/* LNA is currently off */
> +	int		is_on;		/* current state (0/1) */
> +	unsigned long	last_toggle;
> +	unsigned long	backoff;	/* time to wait since last_toggle */
> +	int		on_off_gpio;	/* the on-off gpio number */
> +	struct		serdev_device *uart;	/* uart connected to the chip */
> +	struct		tty_driver *tty_drv;	/* this is the user space tty */
> +	struct		device *dev;	/* from tty_port_register_device() */
> +	struct		tty_port port;
> +	int		open_count;	/* how often we were opened */
> +	enum		w2sg_state state;
> +	int		requested;	/* requested state (0/1) */
> +	int		suspended;
> +	struct delayed_work work;
> +	int		discard_count;
> +};
> +
> +static struct w2sg_data *w2sg_by_minor[1];
> +
> +static int w2sg_set_lna_power(struct w2sg_data *data)
> +{
> +	int ret = 0;
> +	int off = data->suspended || !data->requested || data->lna_blocked;
> +
> +	pr_debug("%s: %s\n", __func__, off ? "off" : "on");
> +
> +	if (off != data->lna_is_off) {
> +		data->lna_is_off = off;
> +		if (!IS_ERR_OR_NULL(data->lna_regulator)) {
> +			if (off)
> +				regulator_disable(data->lna_regulator);
> +			else
> +				ret = regulator_enable(data->lna_regulator);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void w2sg_set_power(void *pdata, int val)
> +{
> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
> +
> +	pr_debug("%s to state=%d (requested=%d)\n", __func__, val, data->requested);
> +
> +	if (val && !data->requested) {
> +		data->requested = true;
> +	} else if (!val && data->requested) {
> +		data->backoff = HZ;
> +		data->requested = false;
> +	} else
> +		return;
> +
> +	pr_debug("w2sg00x4 scheduled for %d\n", data->requested);
> +
> +	if (!data->suspended)
> +		schedule_delayed_work(&data->work, 0);
> +}
> +
> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
> +
> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
> +				const unsigned char *rxdata,
> +				size_t count)
> +{
> +	struct w2sg_data *data =
> +		(struct w2sg_data *) serdev_device_get_drvdata(serdev);
> +
> +	if (!data->requested && !data->is_on) {
> +		/*
> +		 * we have received characters while the w2sg
> +		 * should have been be turned off
> +		 */
> +		data->discard_count += count;
> +		if ((data->state == W2SG_IDLE) &&
> +		    time_after(jiffies,
> +		    data->last_toggle + data->backoff)) {
> +			/* Should be off by now, time to toggle again */
> +			pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
> +				data->discard_count);
> +
> +			data->discard_count = 0;
> +
> +			data->is_on = true;
> +			data->backoff *= 2;
> +			if (!data->suspended)
> +				schedule_delayed_work(&data->work, 0);
> +		}
> +	} else if (data->open_count > 0) {
> +		int n;
> +
> +		pr_debug("w2sg00x4: push %d chars to tty port\n", count);
> +
> +		/* pass to user-space */
> +		n = tty_insert_flip_string(&data->port, rxdata, count);
> +		if (n != count)
> +			pr_err("w2sg00x4: did loose %d characters\n", count - n);
> +		tty_flip_buffer_push(&data->port);
> +		return n;
> +	}
> +
> +	/* assume we have processed everything */
> +	return count;
> +}
> +
> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
> +
> +static void toggle_work(struct work_struct *work)
> +{
> +	struct w2sg_data *data = container_of(work, struct w2sg_data,
> +					      work.work);
> +
> +	switch (data->state) {
> +	case W2SG_IDLE:
> +		if (data->requested == data->is_on)
> +			return;
> +
> +		w2sg_set_lna_power(data);	/* update LNA power state */
> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
> +		data->state = W2SG_PULSE;
> +
> +		pr_debug("w2sg: power gpio ON\n");
> +
> +		schedule_delayed_work(&data->work,
> +				      msecs_to_jiffies(10));
> +		break;
> +
> +	case W2SG_PULSE:
> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
> +		data->last_toggle = jiffies;
> +		data->state = W2SG_NOPULSE;
> +		data->is_on = !data->is_on;
> +
> +		pr_debug("w2sg: power gpio OFF\n");
> +
> +		schedule_delayed_work(&data->work,
> +				      msecs_to_jiffies(10));
> +		break;
> +
> +	case W2SG_NOPULSE:
> +		data->state = W2SG_IDLE;
> +
> +		pr_debug("w2sg: idle\n");
> +
> +		break;
> +
> +	}
> +}
> +
> +static int w2sg_rfkill_set_block(void *pdata, bool blocked)
> +{
> +	struct w2sg_data *data = pdata;
> +
> +	pr_debug("%s: blocked: %d\n", __func__, blocked);
> +
> +	data->lna_blocked = blocked;
> +
> +	return w2sg_set_lna_power(data);
> +}
> +
> +static struct rfkill_ops w2sg0004_rfkill_ops = {
> +	.set_block = w2sg_rfkill_set_block,
> +};
> +
> +static struct serdev_device_ops serdev_ops = {
> +	.receive_buf = w2sg_uart_receive_buf,
> +};
> +
> +/*
> + * we are a man-in the middle between the user-space visible tty port
> + * and the serdev tty where the chip is connected.
> + * This allows us to recognise when the device should be powered on
> + * or off and handle the "false" state that data arrives while no
> + * users-space tty client exists.
> + */
> +
> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
> +{
> +	return w2sg_by_minor[minor];
> +}
> +
> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> +	struct w2sg_data *data;
> +	int retval;
> +
> +	pr_debug("%s() tty = %p\n", __func__, tty);
> +
> +	data = w2sg_get_by_minor(tty->index);
> +
> +	if (!data)
> +		return -ENODEV;
> +
> +	retval = tty_standard_install(driver, tty);
> +	if (retval)
> +		goto error_init_termios;
> +
> +	tty->driver_data = data;
> +
> +	return 0;
> +
> +error_init_termios:
> +	tty_port_put(&data->port);
> +	return retval;
> +}
> +
> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
> +{
> +	struct w2sg_data *data = tty->driver_data;
> +
> +	pr_debug("%s() data = %p open_count = ++%d\n", __func__, data, data->open_count);
> +
> +	w2sg_set_power(data, ++data->open_count > 0);
> +
> +	return tty_port_open(&data->port, tty, file);
> +}
> +
> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
> +{
> +	struct w2sg_data *data = tty->driver_data;
> +
> +	pr_debug("%s()\n", __func__);
> +
> +	w2sg_set_power(data, --data->open_count > 0);
> +
> +	tty_port_close(&data->port, tty, file);
> +}
> +
> +static int w2sg_tty_write(struct tty_struct *tty,
> +		const unsigned char *buffer, int count)
> +{
> +	struct w2sg_data *data = tty->driver_data;
> +
> +	/* simply pass down to UART */
> +	return serdev_device_write_buf(data->uart, buffer, count);
> +}
> +
> +
> +static const struct tty_operations w2sg_serial_ops = {
> +	.install = w2sg_tty_install,
> +	.open = w2sg_tty_open,
> +	.close = w2sg_tty_close,
> +	.write = w2sg_tty_write,
> +};
> +
> +static const struct tty_port_operations w2sg_port_ops = {
> +};
> +
> +static int w2sg_probe(struct serdev_device *serdev)
> +{
> +	struct w2sg_pdata *pdata = NULL;
> +	struct w2sg_data *data;
> +	struct rfkill *rf_kill;
> +	int err;
> +	int minor;
> +
> +	pr_debug("%s()\n", __func__);
> +
> +	minor = 0;
> +	if (w2sg_by_minor[minor]) {
> +		pr_err("w2sg minor is already in use!\n");
> +		return -ENODEV;
> +	}
> +
> +	if (serdev->dev.of_node) {
> +		struct device *dev = &serdev->dev;
> +		enum of_gpio_flags flags;
> +
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		pdata->on_off_gpio = of_get_named_gpio_flags(dev->of_node,
> +							     "enable-gpios", 0,
> +							     &flags);
> +
> +		/* defer until we have all gpios */
> +		if (pdata->on_off_gpio == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		pdata->lna_regulator = devm_regulator_get_optional(dev, "lna");
> +		if (IS_ERR(pdata->lna_regulator)) {
> +			/* defer until we can get the regulator */
> +			if (PTR_ERR(pdata->lna_regulator) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +
> +			pdata->lna_regulator = NULL;
> +		}
> +
> +		pr_debug("%s() lna_regulator = %p\n", __func__,
> +			pdata->lna_regulator);
> +
> +		serdev->dev.platform_data = pdata;
> +	}
> +
> +	data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	w2sg_by_minor[minor] = data;
> +
> +	serdev_device_set_drvdata(serdev, data);
> +
> +	data->lna_regulator = pdata->lna_regulator;

NULL ptr dereference if you have no of_node.

> +	data->lna_blocked = true;
> +	data->lna_is_off = true;
> +
> +	data->on_off_gpio = pdata->on_off_gpio;
> +
> +	data->is_on = false;
> +	data->requested = false;
> +	data->state = W2SG_IDLE;
> +	data->last_toggle = jiffies;
> +	data->backoff = HZ;
> +
> +	data->uart = serdev;
> +
> +	INIT_DELAYED_WORK(&data->work, toggle_work);
> +
> +	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
> +				"w2sg0004-on-off");
> +	if (err < 0)
> +		goto out;
> +
> +	gpio_direction_output(data->on_off_gpio, false);
> +
> +	serdev_device_set_client_ops(data->uart, &serdev_ops);
> +	serdev_device_open(data->uart);
> +
> +	serdev_device_set_baudrate(data->uart, 9600);
> +	serdev_device_set_flow_control(data->uart, false);
> +
> +	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
> +				&w2sg0004_rfkill_ops, data);
> +	if (rf_kill == NULL) {
> +		err = -ENOMEM;
> +		goto err_rfkill;
> +	}
> +
> +	err = rfkill_register(rf_kill);
> +	if (err) {
> +		dev_err(&serdev->dev, "Cannot register rfkill device\n");
> +		goto err_rfkill;
> +	}
> +
> +	data->rf_kill = rf_kill;
> +
> +	/* allocate the tty driver */
> +	data->tty_drv = alloc_tty_driver(1);

Is there an advantage or reason to make this a tty driver rather than 
just a char driver?

Perhaps this should be under drivers/tty if it is a tty driver.

> +	if (!data->tty_drv)
> +		return -ENOMEM;
> +
> +	/* initialize the tty driver */
> +	data->tty_drv->owner = THIS_MODULE;
> +	data->tty_drv->driver_name = "w2sg0004";
> +	data->tty_drv->name = "ttyGPS";
> +	data->tty_drv->major = 0;
> +	data->tty_drv->minor_start = 0;
> +	data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
> +	data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
> +	data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> +	data->tty_drv->init_termios = tty_std_termios;
> +	data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
> +					      HUPCL | CLOCAL;
> +	/*
> +	 * optional:
> +	 * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
> +					115200, 115200);
> +	 * w2sg_tty_termios(&data->tty_drv->init_termios);
> +	 */
> +	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
> +
> +	/* register the tty driver */
> +	err = tty_register_driver(data->tty_drv);
> +	if (err) {
> +		pr_err("%s - tty_register_driver failed(%d)\n",
> +			__func__, err);
> +		put_tty_driver(data->tty_drv);
> +		goto err_rfkill;
> +	}
> +
> +	tty_port_init(&data->port);
> +	data->port.ops = &w2sg_port_ops;
> +
> +/*
> + * FIXME: this appears to reenter this probe() function a second time
> + * which only fails because the gpio is already assigned
> + */
> +
> +	data->dev = tty_port_register_device(&data->port,
> +			data->tty_drv, minor, &serdev->dev);
> +
> +	pr_debug("w2sg probed\n");
> +
> +	/* keep off until user space requests the device */
> +	w2sg_set_power(data, false);
> +
> +	return 0;
> +
> +err_rfkill:
> +	rfkill_destroy(rf_kill);
> +	serdev_device_close(data->uart);
> +out:
> +	return err;
> +}
> +
> +static void w2sg_remove(struct serdev_device *serdev)
> +{
> +	struct w2sg_data *data = serdev_device_get_drvdata(serdev);
> +	int minor;
> +
> +	cancel_delayed_work_sync(&data->work);
> +
> +	/* what is the right sequence to avoid problems? */
> +	serdev_device_close(data->uart);
> +
> +	minor = 0;
> +	tty_unregister_device(data->tty_drv, minor);
> +
> +	tty_unregister_driver(data->tty_drv);
> +}
> +
> +static int w2sg_suspend(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	data->suspended = true;
> +
> +	cancel_delayed_work_sync(&data->work);
> +
> +	w2sg_set_lna_power(data);	/* shuts down if needed */
> +
> +	if (data->state == W2SG_PULSE) {
> +		msleep(10);
> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
> +		data->last_toggle = jiffies;
> +		data->is_on = !data->is_on;
> +		data->state = W2SG_NOPULSE;
> +	}
> +
> +	if (data->state == W2SG_NOPULSE) {
> +		msleep(10);
> +		data->state = W2SG_IDLE;
> +	}
> +
> +	if (data->is_on) {
> +		pr_info("GPS off for suspend %d %d %d\n", data->requested,
> +			data->is_on, data->lna_is_off);
> +
> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
> +		msleep(10);
> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
> +		data->is_on = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static int w2sg_resume(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	data->suspended = false;
> +
> +	pr_info("GPS resuming %d %d %d\n", data->requested,
> +		data->is_on, data->lna_is_off);
> +
> +	schedule_delayed_work(&data->work, 0);	/* enables LNA if needed */
> +
> +	return 0;
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id w2sg0004_of_match[] = {
> +	{ .compatible = "wi2wi,w2sg0004" },
> +	{ .compatible = "wi2wi,w2sg0084" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
> +#endif
> +
> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
> +
> +static struct serdev_device_driver w2sg_driver = {
> +	.probe		= w2sg_probe,
> +	.remove		= w2sg_remove,
> +	.driver = {
> +		.name	= "w2sg0004",
> +		.owner	= THIS_MODULE,
> +		.pm	= &w2sg_pm_ops,
> +		.of_match_table = of_match_ptr(w2sg0004_of_match)
> +	},
> +};
> +
> +module_serdev_device_driver(w2sg_driver);
> +
> +MODULE_ALIAS("w2sg0004");
> +
> +MODULE_AUTHOR("NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>");
> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/w2sg0004.h b/include/linux/w2sg0004.h
> new file mode 100644
> index 000000000000..ad0c4a18e01d
> --- /dev/null
> +++ b/include/linux/w2sg0004.h
> @@ -0,0 +1,27 @@
> +/*
> + * UART slave to allow ON/OFF control of w2sg0004 GPS receiver.
> + *
> + * Copyright (C) 2011 Neil Brown <neil-+NVA1uvv1dVBDLzU/O5InQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +
> +
> +#ifndef __LINUX_W2SG0004_H
> +#define __LINUX_W2SG0004_H
> +
> +#include <linux/regulator/consumer.h>
> +
> +struct w2sg_pdata {

You don't really support platform_data, so you can move this into the 
driver.

> +	struct regulator *lna_regulator;	/* enable LNA power */
> +	int	on_off_gpio;	/*  on-off input of the GPS module */
> +};
> +#endif /* __LINUX_W2SG0004_H */
> -- 
> 2.12.2
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


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