Devicetree
 help / color / mirror / Atom feed
* Re: [RESEND PATCH v7 0/3] Add clockevent for timer-nps driver to NPS400 SoC
From: Vineet Gupta @ 2016-11-17 17:50 UTC (permalink / raw)
  To: Noam Camus, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A
  Cc: tglx-hfZtesqFncYOwBW4kG4KsQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-snps-arc-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Alexey.Brodkin-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <1479277873-18994-1-git-send-email-noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

On 11/15/2016 10:31 PM, Noam Camus wrote:
> From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Change log
> ---
> V6 --> V7
> Apply several comments made by Daniel Lezcano:
> 1) Remove CLOCK_EVT_FEAT_PERIODIC support. This way it is
>  pure oneshot driver. This simplifies driver so that:
>  nps_clkevent_add_thread()
>  nps_clkevent_rm_thread()
>  are more clearer without any vague logic if to change
>  TSI bit of current HW thread or not.
> 2) tick_resume is also calls nps_clkevent_rm_thread()
> 3) Few (hopefully last) typo fixes. 
> 
> V5 --> V6
> Apply several comments made by Daniel Lezcano:
> 1) nps_get_timer_clk() - use clk_put() on error scenario
> 2) nps_get_timer_clk() - return EINVAL and not plain 1
> 3) Fix typos in log (double checked with spell checker)
> 
> V4 --> V5
> Apply several comments made by Daniel Lezcano:
> 1) Add __init attribute to nps_get_timer_clk()
> 2) Fix return value of nps_get_timer_clk()
>  when failing to get clk rate
> 3) Change clocksource rate from 301 -> 300
> 
> V3 --> V4
> Main changes are [Thanks for the review]:
> Fix many typos at log [Daniel]
> Add handling for bad return values [Daniel and Thomas]
> Replace use of internal irqchip pointers with existing IRQ API [Thomas]
> Provide interrupt handler (percpu) with dev_id equal to evt [Thomas]
> Fix passing *clk by reference to nps_get_timer_clk() [Daniel]
> 
> V2 --> V3
> Apply Rob Herring comment about backword compatibility
> 
> V1 --> V2
> Apply Daniel Lezcano comments:
> 	CLOCKSOURCE_OF_DECLARE return value
> 	update hotplug callbacks usage
> 	squash of 2 first commits.
> In this version I created new commit to serve as preperation for adding clockevents.
> This way the last patch is more readable with clockevent content.
> ---
> 
> In first version of this driver we supported clocksource for the NPS400.
> The support for clockevent was taken from Synopsys ARC timer driver.
> This was good for working with our simulator of NPS400.
> However in NPS400 ASIC the timers behave differently than simulation.
> The timers in ASIC are shared between all threads whithin a core
> and hence need different driver to support this behaviour.
> 
> The idea of this design is that we got 16 HW threads per core
> each represented at bimask in a shared register in this core.
> So when thread wants that next clockevent expiration will produce
> timer interrupt to itself the correspondance bit in this register
> should be set.
> So theoretically if all 16 bits are set then all HW threads will get
> timer interrupt on next expiration of timer 0.
> 
> Note that we use Synopsys ARC design naming convention for the timers
> where:
> timer0 is used for clockevents
> timer1 is used for clocksource.
> 
> Noam Camus (3):
>   soc: Support for NPS HW scheduling
>   clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer
>   clocksource: Add clockevent support to NPS400 driver
> 
>  .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
>  .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
>  .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
>  arch/arc/plat-eznps/include/plat/ctop.h            |    2 -
>  drivers/clocksource/timer-nps.c                    |  223 ++++++++++++++++++--
>  include/soc/nps/mtm.h                              |   59 +++++
>  6 files changed, 294 insertions(+), 37 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
>  create mode 100644 include/soc/nps/mtm.h

Added to ARC for-next !

Thx,
-Vineet
--
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] soc/tegra: Implement Tegra186 PMC support
From: Sudeep Holla @ 2016-11-17 17:40 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Sudeep Holla, Rob Herring, Mark Rutland, Stephen Warren,
	Alexandre Courbot, Jon Hunter, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161117173110.GA7915-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>



On 17/11/16 17:31, Thierry Reding wrote:
> On Thu, Nov 17, 2016 at 05:28:53PM +0000, Sudeep Holla wrote:
>>
>>
>> On 17/11/16 17:16, Thierry Reding wrote:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> The power management controller on Tegra186 has changed in backwards-
>>> incompatible ways with respect to earlier generations. This implements a
>>> new driver that supports inversion of the PMU interrupt as well as the
>>> "recovery", "bootloader" and "forced-recovery" reboot commands.
>>>
>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  .../bindings/arm/tegra/nvidia,tegra186-pmc.txt     |  34 +++++
>>>  drivers/soc/tegra/Makefile                         |   2 +-
>>>  drivers/soc/tegra/pmc-tegra186.c                   | 169 +++++++++++++++++++++
>>>  3 files changed, 204 insertions(+), 1 deletion(-)
>>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
>>>  create mode 100644 drivers/soc/tegra/pmc-tegra186.c
>>>
>>
>> [...]
>>
>>> diff --git a/drivers/soc/tegra/pmc-tegra186.c b/drivers/soc/tegra/pmc-tegra186.c
>>> new file mode 100644
>>> index 000000000000..ee28eddd8e3c
>>> --- /dev/null
>>> +++ b/drivers/soc/tegra/pmc-tegra186.c
>>
>> [...]
>>
>>> +
>>> +	/*
>>> +	 * If available, call the system restart implementation that was
>>> +	 * registered earlier (typically PSCI).
>>> +	 */
>>> +	if (pmc->system_restart) {
>>> +		pmc->system_restart(reboot_mode, cmd);
>>> +		return NOTIFY_DONE;
>>> +	}
>>> +
>>
>> IIUC, Tegra186 implements PSCI v1.0 and it always takes above path.
>> So what other platforms does this driver support ? The name is
>> pmc-tegra186.c, hence the confusion.
>
> It's technically possible to run Tegra186 without PSCI enabled, or even
> boot it with firmware that doesn't implement PSCI. In such cases it

OK, with enable-method as "spin-table" I suppose ?

> would be nice to still be able to reboot via the PMC code above.

I assume it's only for development purposes as you won't have much CPU
power management support without PSCI.

-- 
Regards,
Sudeep

^ permalink raw reply

* Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes
From: Mike Looijmans @ 2016-11-17 17:40 UTC (permalink / raw)
  To: Guenter Roeck, Tom Levens
  Cc: jdelvare, robh+dt, mark.rutland, linux-kernel, linux-hwmon,
	devicetree
In-Reply-To: <410de6c9-a13e-51f7-4d66-6f4e2537c574@roeck-us.net>

On 17-11-16 17:56, Guenter Roeck wrote:
> On 11/17/2016 04:10 AM, Tom Levens wrote:
>> Updated version of the ltc2990 driver which supports all measurement
>> modes available in the chip. The mode can be set through a devicetree
>> attribute.
>
> property
>
>>
>> Signed-off-by: Tom Levens <tom.levens@cern.ch>
>> ---
>>
>> Changes since v1:
>>   * Refactored value conversion (patch 1/3)
>>   * Split the devicetree binding into separate patch (patch 2/3)
>>   * Specifying an invalid mode now returns -EINVAL, previously this
>>     only issued a warning and used the default value
>>   * Removed the "mode" sysfs attribute, as the mode of the chip is
>>     hardware specific and should not be user configurable. This allows
>> much
>>     simpler code as a result.
>>
>>  Documentation/hwmon/ltc2990 |   24 ++++---
>>  drivers/hwmon/Kconfig       |    7 +--
>>  drivers/hwmon/ltc2990.c     |  167
>> ++++++++++++++++++++++++++++++++++++-------
>>  3 files changed, 159 insertions(+), 39 deletions(-)
>>
>> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
>> index c25211e..3ed68f6 100644
>> --- a/Documentation/hwmon/ltc2990
>> +++ b/Documentation/hwmon/ltc2990
>> @@ -8,6 +8,7 @@ Supported chips:
>>      Datasheet: http://www.linear.com/product/ltc2990
>>
>>  Author: Mike Looijmans <mike.looijmans@topic.nl>
>> +        Tom Levens <tom.levens@cern.ch>
>>
>>
>>  Description
>> @@ -16,10 +17,8 @@ Description
>>  LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
>>  The chip's inputs can measure 4 voltages, or two inputs together (1+2
>> and 3+4)
>>  can be combined to measure a differential voltage, which is typically
>> used to
>> -measure current through a series resistor, or a temperature.
>> -
>> -This driver currently uses the 2x differential mode only. In order to
>> support
>> -other modes, the driver will need to be expanded.
>> +measure current through a series resistor, or a temperature with an
>> external
>> +diode.
>>
>>
>>  Usage Notes
>> @@ -32,12 +31,19 @@ devices explicitly.
>>  Sysfs attributes
>>  ----------------
>>
>> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
>> +temp1_input   Internal chip temperature in millidegrees Celcius
>> +
>> +A subset of the following attributes are visible, depending on the
>> measurement
>> +mode of the chip.
>> +
>> +in[1-4]_input Voltage at V[1-4] pin in millivolt
>> +temp2_input   External temperature sensor TR1 in millidegrees Celcius
>> +temp3_input   External temperature sensor TR2 in millidegrees Celcius
>> +curr1_input   Current in mA across V1-V2 assuming a 1mOhm sense resistor
>> +curr2_input   Current in mA across V3-V4 assuming a 1mOhm sense resistor
>> +
>>  The "curr*_input" measurements actually report the voltage drop
>> across the
>>  input pins in microvolts. This is equivalent to the current through a
>> 1mOhm
>>  sense resistor. Divide the reported value by the actual sense
>> resistor value
>>  in mOhm to get the actual value.
>> -
>> -in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
>> -temp1_input   Internal chip temperature in millidegrees Celcius
>> -curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense
>> resistor.
>> -curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense
>> resistor.
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 45cef3d..f7096ca 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -699,15 +699,12 @@ config SENSORS_LTC2945
>>        be called ltc2945.
>>
>>  config SENSORS_LTC2990
>> -    tristate "Linear Technology LTC2990 (current monitoring mode only)"
>> +    tristate "Linear Technology LTC2990"
>>      depends on I2C
>>      help
>>        If you say yes here you get support for Linear Technology LTC2990
>>        I2C System Monitor. The LTC2990 supports a combination of voltage,
>> -      current and temperature monitoring, but in addition to the Vcc
>> supply
>> -      voltage and chip temperature, this driver currently only supports
>> -      reading two currents by measuring two differential voltages across
>> -      series resistors.
>> +      current and temperature monitoring.
>>
>>        This driver can also be built as a module. If so, the module will
>>        be called ltc2990.
>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>> index 0ec4102..e8d36f5 100644
>> --- a/drivers/hwmon/ltc2990.c
>> +++ b/drivers/hwmon/ltc2990.c
>> @@ -6,11 +6,7 @@
>>   *
>>   * License: GPLv2
>>   *
>> - * This driver assumes the chip is wired as a dual current monitor, and
>> - * reports the voltage drop across two series resistors. It also reports
>> - * the chip's internal temperature and Vcc power supply voltage.
>> - *
>> - * Value conversion refactored
>> + * Value conversion refactored and support for all measurement modes
>> added
>>   * by Tom Levens <tom.levens@cern.ch>
>>   */
>>
>> @@ -21,6 +17,7 @@
>>  #include <linux/i2c.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> +#include <linux/of.h>
>>
>>  #define LTC2990_STATUS    0x00
>>  #define LTC2990_CONTROL    0x01
>> @@ -35,32 +32,96 @@
>>  #define LTC2990_CONTROL_KELVIN        BIT(7)
>>  #define LTC2990_CONTROL_SINGLE        BIT(6)
>>  #define LTC2990_CONTROL_MEASURE_ALL    (0x3 << 3)
>> -#define LTC2990_CONTROL_MODE_CURRENT    0x06
>> -#define LTC2990_CONTROL_MODE_VOLTAGE    0x07
>> +#define LTC2990_CONTROL_MODE_DEFAULT    0x06
>
> I think LTC2990_CONTROL_MODE_CURRENT was better - it describes the
> actual mode.
> Changing the define to _DEFAULT does not really improve code readability.

I think I understand what the author intended - the mode becomes an 
index into the array.

But I think the define can be dropped altogether, see below.

>
>> +#define LTC2990_CONTROL_MODE_MAX    0x07
>> +
>> +#define LTC2990_IN0    BIT(0)
>> +#define LTC2990_IN1    BIT(1)
>> +#define LTC2990_IN2    BIT(2)
>> +#define LTC2990_IN3    BIT(3)
>> +#define LTC2990_IN4    BIT(4)
>> +#define LTC2990_CURR1    BIT(5)
>> +#define LTC2990_CURR2    BIT(6)
>> +#define LTC2990_TEMP1    BIT(7)
>> +#define LTC2990_TEMP2    BIT(8)
>> +#define LTC2990_TEMP3    BIT(9)
>> +
>> +static const int ltc2990_attrs_ena[] = {
>> +    LTC2990_IN1 | LTC2990_IN2 | LTC2990_TEMP3,
>> +    LTC2990_CURR1 | LTC2990_TEMP3,
>> +    LTC2990_CURR1 | LTC2990_IN3 | LTC2990_IN4,
>> +    LTC2990_TEMP2 | LTC2990_IN3 | LTC2990_IN4,
>> +    LTC2990_TEMP2 | LTC2990_CURR2,
>> +    LTC2990_TEMP2 | LTC2990_TEMP3,
>> +    LTC2990_CURR1 | LTC2990_CURR2,
>> +    LTC2990_IN1 | LTC2990_IN2 | LTC2990_IN3 | LTC2990_IN4
>> +};
>> +
>> +struct ltc2990_data {
>> +    struct i2c_client *i2c;
>> +    u32 mode;
>> +};
>>
>>  /* Return the converted value from the given register in uV or mC */
>> -static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32
>> *result)
>> +static int ltc2990_get_value(struct i2c_client *i2c, int index, s32
>> *result)
>>  {
>>      s32 val;
>> +    u8 reg;
>> +
>> +    switch (index) {
>> +    case LTC2990_IN0:
>> +        reg = LTC2990_VCC_MSB;
>> +        break;
>> +    case LTC2990_IN1:
>> +    case LTC2990_CURR1:
>> +    case LTC2990_TEMP2:
>> +        reg = LTC2990_V1_MSB;
>> +        break;
>> +    case LTC2990_IN2:
>> +        reg = LTC2990_V2_MSB;
>> +        break;
>> +    case LTC2990_IN3:
>> +    case LTC2990_CURR2:
>> +    case LTC2990_TEMP3:
>> +        reg = LTC2990_V3_MSB;
>> +        break;
>> +    case LTC2990_IN4:
>> +        reg = LTC2990_V4_MSB;
>> +        break;
>> +    case LTC2990_TEMP1:
>> +        reg = LTC2990_TINT_MSB;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>>
>>      val = i2c_smbus_read_word_swapped(i2c, reg);
>>      if (unlikely(val < 0))
>>          return val;
>>
>> -    switch (reg) {
>> -    case LTC2990_TINT_MSB:
>> -        /* internal temp, 0.0625 degrees/LSB, 13-bit  */
>> +    switch (index) {
>> +    case LTC2990_TEMP1:
>> +    case LTC2990_TEMP2:
>> +    case LTC2990_TEMP3:
>> +        /* temp, 0.0625 degrees/LSB, 13-bit  */
>>          *result = sign_extend32(val, 12) * 1000 / 16;
>>          break;
>> -    case LTC2990_V1_MSB:
>> -    case LTC2990_V3_MSB:
>> -         /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
>> +    case LTC2990_CURR1:
>> +    case LTC2990_CURR2:
>> +         /* Vx-Vy, 19.42uV/LSB */
>>          *result = sign_extend32(val, 14) * 1942 / 100;
>>          break;
>> -    case LTC2990_VCC_MSB:
>> -        /* Vcc, 305.18μV/LSB, 2.5V offset */
>> +    case LTC2990_IN0:
>> +        /* Vcc, 305.18uV/LSB, 2.5V offset */
>>          *result = sign_extend32(val, 14) * 30518 / (100 * 1000) + 2500;
>>          break;
>> +    case LTC2990_IN1:
>> +    case LTC2990_IN2:
>> +    case LTC2990_IN3:
>> +    case LTC2990_IN4:
>> +        /* Vx: 305.18uV/LSB */
>> +        *result = sign_extend32(val, 14) * 30518 / (100 * 1000);
>> +        break;
>>      default:
>>          return -EINVAL; /* won't happen, keep compiler happy */
>>      }
>> @@ -72,48 +133,104 @@ static ssize_t ltc2990_show_value(struct device
>> *dev,
>>                    struct device_attribute *da, char *buf)
>>  {
>>      struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +    struct ltc2990_data *data = dev_get_drvdata(dev);
>>      s32 value;
>>      int ret;
>>
>> -    ret = ltc2990_get_value(dev_get_drvdata(dev), attr->index, &value);
>> +    ret = ltc2990_get_value(data->i2c, attr->index, &value);
>>      if (unlikely(ret < 0))
>>          return ret;
>>
>>      return snprintf(buf, PAGE_SIZE, "%d\n", value);
>>  }
>>
>> +static umode_t ltc2990_attrs_visible(struct kobject *kobj,
>> +                     struct attribute *a, int n)
>> +{
>> +    struct device *dev = container_of(kobj, struct device, kobj);
>> +    struct ltc2990_data *data = dev_get_drvdata(dev);
>> +    struct device_attribute *da =
>> +            container_of(a, struct device_attribute, attr);
>> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +
>> +    if (attr->index == LTC2990_TEMP1 ||
>> +        attr->index == LTC2990_IN0 ||
>> +        attr->index & ltc2990_attrs_ena[data->mode])
>> +        return a->mode;
>> +    else
>
> Unnecessary else
>
>> +        return 0;
>> +}
>> +
>>  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value,
>> NULL,
>> -              LTC2990_TINT_MSB);
>> +              LTC2990_TEMP1);
>> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, ltc2990_show_value,
>> NULL,
>> +              LTC2990_TEMP2);
>> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, ltc2990_show_value,
>> NULL,
>> +              LTC2990_TEMP3);
>>  static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value,
>> NULL,
>> -              LTC2990_V1_MSB);
>> +              LTC2990_CURR1);
>>  static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value,
>> NULL,
>> -              LTC2990_V3_MSB);
>> +              LTC2990_CURR2);
>>  static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
>> -              LTC2990_VCC_MSB);
>> +              LTC2990_IN0);
>> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ltc2990_show_value, NULL,
>> +              LTC2990_IN1);
>> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, ltc2990_show_value, NULL,
>> +              LTC2990_IN2);
>> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, ltc2990_show_value, NULL,
>> +              LTC2990_IN3);
>> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, ltc2990_show_value, NULL,
>> +              LTC2990_IN4);
>>
>>  static struct attribute *ltc2990_attrs[] = {
>>      &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +    &sensor_dev_attr_temp2_input.dev_attr.attr,
>> +    &sensor_dev_attr_temp3_input.dev_attr.attr,
>>      &sensor_dev_attr_curr1_input.dev_attr.attr,
>>      &sensor_dev_attr_curr2_input.dev_attr.attr,
>>      &sensor_dev_attr_in0_input.dev_attr.attr,
>> +    &sensor_dev_attr_in1_input.dev_attr.attr,
>> +    &sensor_dev_attr_in2_input.dev_attr.attr,
>> +    &sensor_dev_attr_in3_input.dev_attr.attr,
>> +    &sensor_dev_attr_in4_input.dev_attr.attr,
>>      NULL,
>>  };
>> -ATTRIBUTE_GROUPS(ltc2990);
>> +
>> +static const struct attribute_group ltc2990_group = {
>> +    .attrs = ltc2990_attrs,
>> +    .is_visible = ltc2990_attrs_visible,
>> +};
>> +__ATTRIBUTE_GROUPS(ltc2990);
>>
>>  static int ltc2990_i2c_probe(struct i2c_client *i2c,
>>                   const struct i2c_device_id *id)
>>  {
>>      int ret;
>>      struct device *hwmon_dev;
>> +    struct ltc2990_data *data;
>> +    struct device_node *of_node = i2c->dev.of_node;
>>
>>      if (!i2c_check_functionality(i2c->adapter,
>> I2C_FUNC_SMBUS_BYTE_DATA |
>>                       I2C_FUNC_SMBUS_WORD_DATA))
>>          return -ENODEV;
>>
>> -    /* Setup continuous mode, current monitor */
>> +    data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2990_data),
>> GFP_KERNEL);
>> +    if (unlikely(!data))
>> +        return -ENOMEM;
>> +    data->i2c = i2c;
>> +
>> +    if (!of_node || of_property_read_u32(of_node, "lltc,mode",
>> &data->mode))
>> +        data->mode = LTC2990_CONTROL_MODE_DEFAULT;
>
> Iam arguing with myself if we should still do this or if we should read
> the mode
> from the chip instead if it isn't provided (after all, it may have been
> initialized
> by the BIOS/ROMMON).

I think the mode should be explicitly set, without default. There's no 
way to tell whether the BIOS or bootloader has really set it up or 
whether the chip is just reporting whatever it happened to default to. 
And given the chip's function, it's unlikely a bootloader would want to 
initialize it.

My advice would be to make it a required property. If not set, display 
an error and bail out.

> Mike, would that break your application, or can you specify the mode in
> devicetree ?

I'm fine with specifying this in the devicetree. It will break things 
for me, but I've been warned and willing to bow for the greater good :)

>
> Thanks,
> Guenter
>
>> +
>> +    if (data->mode > LTC2990_CONTROL_MODE_MAX) {
>> +        dev_err(&i2c->dev, "Error: Invalid mode %d.\n", data->mode);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* Setup continuous mode */
>>      ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
>>                      LTC2990_CONTROL_MEASURE_ALL |
>> -                    LTC2990_CONTROL_MODE_CURRENT);
>> +                    data->mode);
>>      if (ret < 0) {
>>          dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
>>          return ret;
>> @@ -127,7 +244,7 @@ static int ltc2990_i2c_probe(struct i2c_client *i2c,
>>
>>      hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
>>                                 i2c->name,
>> -                               i2c,
>> +                               data,
>>                                 ltc2990_groups);
>>
>>      return PTR_ERR_OR_ZERO(hwmon_dev);
>>
>


-- 
Mike Looijmans


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

^ permalink raw reply

* Re: [PATCH] soc/tegra: Implement Tegra186 PMC support
From: Thierry Reding @ 2016-11-17 17:31 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, Mark Rutland, Stephen Warren, Alexandre Courbot,
	Jon Hunter, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <0068ebe4-09f3-4434-fc38-071cf2d553bc-5wv7dgnIgG8@public.gmane.org>

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

On Thu, Nov 17, 2016 at 05:28:53PM +0000, Sudeep Holla wrote:
> 
> 
> On 17/11/16 17:16, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > The power management controller on Tegra186 has changed in backwards-
> > incompatible ways with respect to earlier generations. This implements a
> > new driver that supports inversion of the PMU interrupt as well as the
> > "recovery", "bootloader" and "forced-recovery" reboot commands.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../bindings/arm/tegra/nvidia,tegra186-pmc.txt     |  34 +++++
> >  drivers/soc/tegra/Makefile                         |   2 +-
> >  drivers/soc/tegra/pmc-tegra186.c                   | 169 +++++++++++++++++++++
> >  3 files changed, 204 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
> >  create mode 100644 drivers/soc/tegra/pmc-tegra186.c
> > 
> 
> [...]
> 
> > diff --git a/drivers/soc/tegra/pmc-tegra186.c b/drivers/soc/tegra/pmc-tegra186.c
> > new file mode 100644
> > index 000000000000..ee28eddd8e3c
> > --- /dev/null
> > +++ b/drivers/soc/tegra/pmc-tegra186.c
> 
> [...]
> 
> > +
> > +	/*
> > +	 * If available, call the system restart implementation that was
> > +	 * registered earlier (typically PSCI).
> > +	 */
> > +	if (pmc->system_restart) {
> > +		pmc->system_restart(reboot_mode, cmd);
> > +		return NOTIFY_DONE;
> > +	}
> > +
> 
> IIUC, Tegra186 implements PSCI v1.0 and it always takes above path.
> So what other platforms does this driver support ? The name is
> pmc-tegra186.c, hence the confusion.

It's technically possible to run Tegra186 without PSCI enabled, or even
boot it with firmware that doesn't implement PSCI. In such cases it
would be nice to still be able to reboot via the PMC code above.

Thierry

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

^ permalink raw reply

* Re: [PATCH] soc/tegra: Implement Tegra186 PMC support
From: Sudeep Holla @ 2016-11-17 17:28 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Sudeep Holla, Rob Herring, Mark Rutland, Stephen Warren,
	Alexandre Courbot, Jon Hunter, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161117171636.20580-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>



On 17/11/16 17:16, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> The power management controller on Tegra186 has changed in backwards-
> incompatible ways with respect to earlier generations. This implements a
> new driver that supports inversion of the PMU interrupt as well as the
> "recovery", "bootloader" and "forced-recovery" reboot commands.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/arm/tegra/nvidia,tegra186-pmc.txt     |  34 +++++
>  drivers/soc/tegra/Makefile                         |   2 +-
>  drivers/soc/tegra/pmc-tegra186.c                   | 169 +++++++++++++++++++++
>  3 files changed, 204 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
>  create mode 100644 drivers/soc/tegra/pmc-tegra186.c
>

[...]

> diff --git a/drivers/soc/tegra/pmc-tegra186.c b/drivers/soc/tegra/pmc-tegra186.c
> new file mode 100644
> index 000000000000..ee28eddd8e3c
> --- /dev/null
> +++ b/drivers/soc/tegra/pmc-tegra186.c

[...]

> +
> +	/*
> +	 * If available, call the system restart implementation that was
> +	 * registered earlier (typically PSCI).
> +	 */
> +	if (pmc->system_restart) {
> +		pmc->system_restart(reboot_mode, cmd);
> +		return NOTIFY_DONE;
> +	}
> +

IIUC, Tegra186 implements PSCI v1.0 and it always takes above path.
So what other platforms does this driver support ? The name is
pmc-tegra186.c, hence the confusion.

-- 
Regards,
Sudeep

^ permalink raw reply

* Re: [PATCH v3 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver
From: Laurent Pinchart @ 2016-11-17 17:20 UTC (permalink / raw)
  To: Jyri Sarha
  Cc: devicetree, bcousson, khilman, dri-devel, bgolaszewski,
	tomi.valkeinen
In-Reply-To: <5969991d-c4cd-4ac5-9025-dfcd2b6b914d@ti.com>

Hi Jyri,

On Thursday 17 Nov 2016 15:39:26 Jyri Sarha wrote:
> On 11/17/16 15:28, Jyri Sarha wrote:
> > Add very basic ti-ftp410 DVI transmitter driver. The only feature
> > separating this from a completely dummy bridge is the EDID read
> > support trough DDC I2C. Even that functionality should be in a
> > separate generic connector driver. However, because of missing DRM
> > infrastructure support the connector is implemented within the bridge
> > driver. Some tfp410 HW specific features may be added later if needed,
> > because there is a set of registers behind i2c if it is connected.
> > 
> > This implementation is tested against my new tilcdc bridge support
> > and it works with BeagleBone DVI-D Cape Rev A3. A DT binding document
> > is also added.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > ---
> > 
> >  .../bindings/display/bridge/ti,tfp410.txt          |  40 +++
> >  drivers/gpu/drm/bridge/Kconfig                     |   7 +
> >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >  drivers/gpu/drm/bridge/ti-tfp410.c                 | 287 ++++++++++++++++
> >  4 files changed, 335 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt create
> >  mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> > b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt new file
> > mode 100644
> > index 0000000..6174b18
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> > @@ -0,0 +1,40 @@
> > +TFP410 DVI bridge bindings
> > +
> > +Required properties:
> > +	- compatible: "ti,tfp410"
> > +
> > +Optional properties
> > +	- reg: I2C address. If and only if present the device node
> > +	  should be placed into the i2c controller node where the
> > +	  tfp410 i2c is connected to.
> > +
> > +Required subnodes:
> > +	- port@0: Video input port node to connect the bridge to a
> > +	  display controller output [1].
> > +	- port@1: Video output port node to connect the bridge to a
> > +	  connector input [1].
> > +
> > +[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +Example:
> > +	hdmi-bridge {
> > +		compatible = "ti,tfp410";
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +				bridge_in: endpoint {
> > +					remote-endpoint = <&dc_out>;
> > +				};
> > +			};
> > +
> > +			port@1 {
> > +				reg = <1>;
> > +				bridge_out: endpoint {
> > +					remote-endpoint = <&hdmi_in>;
> > +				};
> > +			};
> > +		};
> > +	};
> > diff --git a/drivers/gpu/drm/bridge/Kconfig
> > b/drivers/gpu/drm/bridge/Kconfig index bd6acc8..a424e03 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -81,6 +81,13 @@ config DRM_TOSHIBA_TC358767
> > 
> >  	---help---
> >  	
> >  	  Toshiba TC358767 eDP bridge chip driver.
> > 
> > +config DRM_TI_TFP410
> > +	tristate "TI TFP410 DVI/HDMI bridge"
> > +	depends on OF
> > +	select DRM_KMS_HELPER
> > +	---help---
> > +	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
> > +
> > 
> >  source "drivers/gpu/drm/bridge/analogix/Kconfig"
> >  
> >  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> > 
> > diff --git a/drivers/gpu/drm/bridge/Makefile
> > b/drivers/gpu/drm/bridge/Makefile index 97ed1a5..8b065d9 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -11,3 +11,4 @@ obj-$(CONFIG_DRM_SII902X) += sii902x.o
> > 
> >  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> >  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> >  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> > 
> > +obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> > b/drivers/gpu/drm/bridge/ti-tfp410.c new file mode 100644
> > index 0000000..64f54e4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > @@ -0,0 +1,287 @@
> > +/*
> > + * Copyright (C) 2016 Texas Instruments
> > + * Author: Jyri Sarha <jsarha@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/module.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/i2c.h>
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +struct tfp410 {
> > +	struct drm_bridge	bridge;
> > +	struct drm_connector	connector;
> > +
> > +	struct i2c_adapter	*ddc;
> > +
> > +	struct device *dev;
> > +};
> > +
> > +static inline struct tfp410 *
> > +drm_bridge_to_tfp410(struct drm_bridge *bridge)
> > +{
> > +	return container_of(bridge, struct tfp410, bridge);
> > +}
> > +
> > +static inline struct tfp410 *
> > +drm_connector_to_tfp410(struct drm_connector *connector)
> > +{
> > +	return container_of(connector, struct tfp410, connector);
> > +}
> > +
> > +static int tfp410_get_modes(struct drm_connector *connector)
> > +{
> > +	struct tfp410 *dvi = drm_connector_to_tfp410(connector);
> > +	struct edid *edid;
> > +	int ret;
> > +
> > +	if (!dvi->ddc)
> > +		goto fallback;
> > +
> > +	edid = drm_get_edid(connector, dvi->ddc);
> > +	if (!edid) {
> > +		DRM_INFO("EDID read failed. Fallback to standard modes\n");
> > +		goto fallback;
> > +	}
> > +
> > +	drm_mode_connector_update_edid_property(connector, edid);
> > +
> > +	return drm_add_edid_modes(connector, edid);
> > +fallback:
> > +	/* No EDID, fallback on the XGA standard modes */
> > +	ret = drm_add_modes_noedid(connector, 1920, 1200);
> > +
> > +	/* And prefer a mode pretty much anything can handle */
> > +	drm_set_preferred_mode(connector, 1024, 768);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct drm_connector_helper_funcs tfp410_con_helper_funcs =
> > {
> > +	.get_modes	= tfp410_get_modes,
> > +};
> > +
> > +static enum drm_connector_status
> > +tfp410_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +	struct tfp410 *dvi = drm_connector_to_tfp410(connector);
> > +
> > +	if (dvi->ddc) {
> > +		if (drm_probe_ddc(dvi->ddc))
> > +			return connector_status_connected;
> > +		else
> > +			return connector_status_disconnected;
> > +	}
> > +
> > +	return connector_status_unknown;
> > +}
> > +
> > +static const struct drm_connector_funcs tfp410_con_funcs = {
> > +	.dpms			= drm_atomic_helper_connector_dpms,
> > +	.detect			= tfp410_connector_detect,
> > +	.fill_modes		= drm_helper_probe_single_connector_modes,
> > +	.destroy		= drm_connector_cleanup,
> > +	.reset			= drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int tfp410_attach(struct drm_bridge *bridge)
> > +{
> > +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> > +	int ret;
> > +
> > +	if (!bridge->encoder) {
> > +		dev_err(dvi->dev, "Missing encoder\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	drm_connector_helper_add(&dvi->connector,
> > +				 &tfp410_con_helper_funcs);
> > +	ret = drm_connector_init(bridge->dev, &dvi->connector,
> > +				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
> > +	if (ret) {
> > +		dev_err(dvi->dev, "drm_connector_init() failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	drm_mode_connector_attach_encoder(&dvi->connector,
> > +					  bridge->encoder);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_bridge_funcs tfp410_bridge_funcs = {
> > +	.attach		= tfp410_attach,
> > +};
> > +
> > +static int tfp410_get_connector_ddc(struct tfp410 *dvi)
> > +{
> > +	struct device_node *ep = NULL, *connector_node = NULL;
> > +	struct device_node *ddc_phandle = NULL;
> > +	int ret = 0;
> > +
> > +	/* port@1 is the connector node */
> > +	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 1, -1);
> > +	if (!ep)
> > +		goto fail;
> > +
> > +	connector_node = of_graph_get_remote_port_parent(ep);
> > +	if (!connector_node)
> > +		goto fail;
> > +
> > +	ddc_phandle = of_parse_phandle(connector_node, "ddc-i2c-bus", 0);
> > +	if (!ddc_phandle)
> > +		goto fail;
> > +
> > +	dvi->ddc = of_get_i2c_adapter_by_node(ddc_phandle);
> > +	if (dvi->ddc)
> > +		dev_info(dvi->dev, "Connector's ddc i2c bus found\n");
> > +	else
> > +		ret = -EPROBE_DEFER;
> > +
> > +fail:
> > +	of_node_put(ep);
> > +	of_node_put(connector_node);
> > +	of_node_put(ddc_phandle);
> > +	return ret;
> > +}
> > +
> > +static int tfp410_init(struct device *dev)
> > +{
> > +	struct tfp410 *dvi;
> > +	int ret;
> > +
> > +	if (!dev->of_node) {
> > +		dev_err(dev, "device-tree data is missing\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	dvi = devm_kzalloc(dev, sizeof(*dvi), GFP_KERNEL);
> > +	if (!dvi)
> > +		return -ENOMEM;
> > +	dev_set_drvdata(dev, dvi);
> > +
> > +	dvi->bridge.funcs = &tfp410_bridge_funcs;
> > +	dvi->bridge.of_node = dev->of_node;
> > +	dvi->dev = dev;
> > +
> > +	ret = tfp410_get_connector_ddc(dvi);
> > +	if (ret)
> > +		goto fail;
> > +
> > +	ret = drm_bridge_add(&dvi->bridge);
> > +	if (ret) {
> > +		dev_err(dev, "drm_bridge_add() failed: %d\n", ret);
> > +		goto fail;
> > +	}
> > +
> > +	return 0;
> > +fail:
> > +	i2c_put_adapter(dvi->ddc);
> > +	return ret;
> > +}
> > +
> > +static int tfp410_fini(struct device *dev)
> > +{
> > +	struct tfp410 *dvi = dev_get_drvdata(dev);
> > +
> > +	drm_bridge_remove(&dvi->bridge);
> > +
> > +	if (dvi->ddc)
> > +		i2c_put_adapter(dvi->ddc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tfp410_probe(struct platform_device *pdev)
> > +{
> > +	return tfp410_init(&pdev->dev);
> > +}
> > +
> > +static int tfp410_remove(struct platform_device *pdev)
> > +{
> > +	return tfp410_fini(&pdev->dev);
> > +}
> > +
> > +/* There is currently no i2c functionality. */
> > +static int tfp410_i2c_probe(struct i2c_client *client,
> > +			    const struct i2c_device_id *id)
> > +{
> > +	int reg;
> > +
> > +	if (!client->dev.of_node ||
> > +	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
> > +		dev_err(&client->dev,
> > +			"Can't get i2c reg property from device-tree\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	return tfp410_init(&client->dev);
> > +}
> > +
> > +static int tfp410_i2c_remove(struct i2c_client *client)
> > +{
> > +	return tfp410_fini(&client->dev);
> > +}
> > +
> > +static const struct of_device_id tfp410_match[] = {
> > +	{ .compatible = "ti,tfp410" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, tfp410_match);
> > +
> > +struct platform_driver tfp410_platform_driver = {
> > +	.probe	= tfp410_probe,
> > +	.remove	= tfp410_remove,
> > +	.driver	= {
> > +		.name		= "tfp410-bridge",
> > +		.of_match_table	= tfp410_match,
> > +	},
> > +};
> > +
> > +static const struct i2c_device_id tfp410_i2c_ids[] = {
> > +	{ "tfp410", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tfp410_i2c_ids);
> > +
> > +static struct i2c_driver tfp410_i2c_driver = {
> > +	.driver = {
> > +		.name	= "tfp410",
> > +		.of_match_table = of_match_ptr(tfp410_match),
> > +	},
> > +	.id_table	= tfp410_i2c_ids,
> > +	.probe		= tfp410_i2c_probe,
> > +	.remove		= tfp410_i2c_remove,
> > +};
> > +
> > +static int __init tfp410_module_init(void)
> > +{
> > +	i2c_add_driver(&tfp410_i2c_driver);
> > +	platform_driver_register(&tfp410_platform_driver);
> 
> Oops, forgot to handle the return values. This is how I fixed it:
> static int __init tfp410_module_init(void)
> {
> 	int ret;
> 
> 	ret = i2c_add_driver(&tfp410_i2c_driver);
> 	if (ret)
> 		return ret;
> 
> 	ret = platform_driver_register(&tfp410_platform_driver);
> 	if (ret)
> 		i2c_del_driver(&tfp410_i2c_driver);
> 
> 	return ret;
> }

If registration of one of the two drivers fail, wouldn't it make sense to 
still continue (probably with an error message) to avoid breaking the other 
one ?

> > +
> > +	return 0;
> > +}
> > +module_init(tfp410_module_init);
> > +
> > +static void __exit tfp410_module_exit(void)
> > +{
> > +	i2c_del_driver(&tfp410_i2c_driver);
> > +	platform_driver_unregister(&tfp410_platform_driver);
> > +}
> > +module_exit(tfp410_module_exit);
> > +
> > +MODULE_AUTHOR("Jyri Sarha <jsarha@ti.com>");
> > +MODULE_DESCRIPTION("TI TFP410 DVI bridge driver");
> > +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

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

^ permalink raw reply

* [PATCH] soc/tegra: Implement Tegra186 PMC support
From: Thierry Reding @ 2016-11-17 17:16 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rob Herring, Mark Rutland, Stephen Warren, Alexandre Courbot,
	Jon Hunter, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The power management controller on Tegra186 has changed in backwards-
incompatible ways with respect to earlier generations. This implements a
new driver that supports inversion of the PMU interrupt as well as the
"recovery", "bootloader" and "forced-recovery" reboot commands.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/arm/tegra/nvidia,tegra186-pmc.txt     |  34 +++++
 drivers/soc/tegra/Makefile                         |   2 +-
 drivers/soc/tegra/pmc-tegra186.c                   | 169 +++++++++++++++++++++
 3 files changed, 204 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
 create mode 100644 drivers/soc/tegra/pmc-tegra186.c

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
new file mode 100644
index 000000000000..078a58b0302f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
@@ -0,0 +1,34 @@
+NVIDIA Tegra Power Management Controller (PMC)
+
+Required properties:
+- compatible: Should contain one of the following:
+  - "nvidia,tegra186-pmc": for Tegra186
+- reg: Must contain an (offset, length) pair of the register set for each
+  entry in reg-names.
+- reg-names: Must include the following entries:
+  - "pmc"
+  - "wake"
+  - "aotag"
+  - "scratch"
+
+Optional properties:
+- nvidia,invert-interrupt: If present, inverts the PMU interrupt signal.
+
+Example:
+
+SoC DTSI:
+
+	pmc@c3600000 {
+		compatible = "nvidia,tegra186-pmc";
+		reg = <0 0x0c360000 0 0x10000>,
+		      <0 0x0c370000 0 0x10000>,
+		      <0 0x0c380000 0 0x10000>,
+		      <0 0x0c390000 0 0x10000>;
+		reg-names = "pmc", "wake", "aotag", "scratch";
+	};
+
+Board DTS:
+
+	pmc@c360000 {
+		nvidia,invert-interrupt;
+	};
diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile
index ae857ff7d53d..9976a0de1927 100644
--- a/drivers/soc/tegra/Makefile
+++ b/drivers/soc/tegra/Makefile
@@ -1,4 +1,4 @@
 obj-y += fuse/
 
 obj-y += common.o
-obj-y += pmc.o
+obj-y += pmc.o pmc-tegra186.o
diff --git a/drivers/soc/tegra/pmc-tegra186.c b/drivers/soc/tegra/pmc-tegra186.c
new file mode 100644
index 000000000000..ee28eddd8e3c
--- /dev/null
+++ b/drivers/soc/tegra/pmc-tegra186.c
@@ -0,0 +1,169 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+
+#include <asm/system_misc.h>
+
+#define PMC_CNTRL 0x000
+#define  PMC_CNTRL_MAIN_RST BIT(4)
+
+#define PMC_RST_STATUS 0x070
+
+#define WAKE_AOWAKE_CTRL 0x4f4
+#define  WAKE_AOWAKE_CTRL_INTR_POLARITY BIT(0)
+
+#define SCRATCH_SCRATCH0 0x2000
+#define  SCRATCH_SCRATCH0_MODE_RECOVERY BIT(31)
+#define  SCRATCH_SCRATCH0_MODE_BOOTLOADER BIT(30)
+#define  SCRATCH_SCRATCH0_MODE_RCM BIT(1)
+#define  SCRATCH_SCRATCH0_MODE_MASK (SCRATCH_SCRATCH0_MODE_RECOVERY | \
+				     SCRATCH_SCRATCH0_MODE_BOOTLOADER | \
+				     SCRATCH_SCRATCH0_MODE_RCM)
+
+struct tegra_pmc {
+	struct device *dev;
+	void __iomem *regs;
+	void __iomem *wake;
+	void __iomem *aotag;
+	void __iomem *scratch;
+
+	void (*system_restart)(enum reboot_mode mode, const char *cmd);
+	struct notifier_block restart;
+};
+
+static int tegra186_pmc_restart_notify(struct notifier_block *nb,
+				       unsigned long action,
+				       void *data)
+{
+	struct tegra_pmc *pmc = container_of(nb, struct tegra_pmc, restart);
+	const char *cmd = data;
+	u32 value;
+
+	value = readl(pmc->scratch + SCRATCH_SCRATCH0);
+	value &= ~SCRATCH_SCRATCH0_MODE_MASK;
+
+	if (cmd) {
+		if (strcmp(cmd, "recovery") == 0)
+			value |= SCRATCH_SCRATCH0_MODE_RECOVERY;
+
+		if (strcmp(cmd, "bootloader") == 0)
+			value |= SCRATCH_SCRATCH0_MODE_BOOTLOADER;
+
+		if (strcmp(cmd, "forced-recovery") == 0)
+			value |= SCRATCH_SCRATCH0_MODE_RCM;
+	}
+
+	writel(value, pmc->scratch + SCRATCH_SCRATCH0);
+
+	/*
+	 * If available, call the system restart implementation that was
+	 * registered earlier (typically PSCI).
+	 */
+	if (pmc->system_restart) {
+		pmc->system_restart(reboot_mode, cmd);
+		return NOTIFY_DONE;
+	}
+
+	/* reset everything but SCRATCH0_SCRATCH0 and PMC_RST_STATUS */
+	value = readl(pmc->regs + PMC_CNTRL);
+	value |= PMC_CNTRL_MAIN_RST;
+	writel(value, pmc->regs + PMC_CNTRL);
+
+	return NOTIFY_DONE;
+}
+
+static int tegra186_pmc_setup(struct tegra_pmc *pmc)
+{
+	struct device_node *np = pmc->dev->of_node;
+	bool invert;
+	u32 value;
+
+	invert = of_property_read_bool(np, "nvidia,invert-interrupt");
+
+	value = readl(pmc->wake + WAKE_AOWAKE_CTRL);
+
+	if (invert)
+		value |= WAKE_AOWAKE_CTRL_INTR_POLARITY;
+	else
+		value &= ~WAKE_AOWAKE_CTRL_INTR_POLARITY;
+
+	writel(value, pmc->wake + WAKE_AOWAKE_CTRL);
+
+	/*
+	 * We need to hook any system restart implementation registered
+	 * previously so we can write SCRATCH_SCRATCH0 before reset.
+	 */
+	pmc->system_restart = arm_pm_restart;
+	arm_pm_restart = NULL;
+
+	pmc->restart.notifier_call = tegra186_pmc_restart_notify;
+	pmc->restart.priority = 128;
+
+	return register_restart_handler(&pmc->restart);
+}
+
+static int tegra186_pmc_probe(struct platform_device *pdev)
+{
+	struct tegra_pmc *pmc;
+	struct resource *res;
+
+	pmc = devm_kzalloc(&pdev->dev, sizeof(*pmc), GFP_KERNEL);
+	if (!pmc)
+		return -ENOMEM;
+
+	pmc->dev = &pdev->dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pmc");
+	pmc->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pmc->regs))
+		return PTR_ERR(pmc->regs);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "wake");
+	pmc->wake = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pmc->wake))
+		return PTR_ERR(pmc->wake);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aotag");
+	pmc->aotag = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pmc->aotag))
+		return PTR_ERR(pmc->aotag);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "scratch");
+	pmc->scratch = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pmc->scratch))
+		return PTR_ERR(pmc->scratch);
+
+	tegra186_pmc_setup(pmc);
+
+	return 0;
+}
+
+static const struct of_device_id tegra186_pmc_of_match[] = {
+	{ .compatible = "nvidia,tegra186-pmc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tegra186_pmc_of_match);
+
+static struct platform_driver tegra186_pmc_driver = {
+	.driver = {
+		.name = "tegra186-pmc",
+		.of_match_table = tegra186_pmc_of_match,
+	},
+	.probe = tegra186_pmc_probe,
+};
+builtin_platform_driver(tegra186_pmc_driver);
-- 
2.10.2

^ permalink raw reply related

* Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes
From: Guenter Roeck @ 2016-11-17 16:56 UTC (permalink / raw)
  To: Tom Levens
  Cc: jdelvare-IBi9RG/b67k, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mike Looijmans
In-Reply-To: <1479384616-12479-3-git-send-email-tom.levens-vJEk5272eHo@public.gmane.org>

On 11/17/2016 04:10 AM, Tom Levens wrote:
> Updated version of the ltc2990 driver which supports all measurement
> modes available in the chip. The mode can be set through a devicetree
> attribute.

property

>
> Signed-off-by: Tom Levens <tom.levens-vJEk5272eHo@public.gmane.org>
> ---
>
> Changes since v1:
>   * Refactored value conversion (patch 1/3)
>   * Split the devicetree binding into separate patch (patch 2/3)
>   * Specifying an invalid mode now returns -EINVAL, previously this
>     only issued a warning and used the default value
>   * Removed the "mode" sysfs attribute, as the mode of the chip is
>     hardware specific and should not be user configurable. This allows much
>     simpler code as a result.
>
>  Documentation/hwmon/ltc2990 |   24 ++++---
>  drivers/hwmon/Kconfig       |    7 +--
>  drivers/hwmon/ltc2990.c     |  167 ++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 159 insertions(+), 39 deletions(-)
>
> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
> index c25211e..3ed68f6 100644
> --- a/Documentation/hwmon/ltc2990
> +++ b/Documentation/hwmon/ltc2990
> @@ -8,6 +8,7 @@ Supported chips:
>      Datasheet: http://www.linear.com/product/ltc2990
>
>  Author: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>
> +        Tom Levens <tom.levens-vJEk5272eHo@public.gmane.org>
>
>
>  Description
> @@ -16,10 +17,8 @@ Description
>  LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
>  The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
>  can be combined to measure a differential voltage, which is typically used to
> -measure current through a series resistor, or a temperature.
> -
> -This driver currently uses the 2x differential mode only. In order to support
> -other modes, the driver will need to be expanded.
> +measure current through a series resistor, or a temperature with an external
> +diode.
>
>
>  Usage Notes
> @@ -32,12 +31,19 @@ devices explicitly.
>  Sysfs attributes
>  ----------------
>
> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
> +temp1_input   Internal chip temperature in millidegrees Celcius
> +
> +A subset of the following attributes are visible, depending on the measurement
> +mode of the chip.
> +
> +in[1-4]_input Voltage at V[1-4] pin in millivolt
> +temp2_input   External temperature sensor TR1 in millidegrees Celcius
> +temp3_input   External temperature sensor TR2 in millidegrees Celcius
> +curr1_input   Current in mA across V1-V2 assuming a 1mOhm sense resistor
> +curr2_input   Current in mA across V3-V4 assuming a 1mOhm sense resistor
> +
>  The "curr*_input" measurements actually report the voltage drop across the
>  input pins in microvolts. This is equivalent to the current through a 1mOhm
>  sense resistor. Divide the reported value by the actual sense resistor value
>  in mOhm to get the actual value.
> -
> -in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
> -temp1_input   Internal chip temperature in millidegrees Celcius
> -curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
> -curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 45cef3d..f7096ca 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -699,15 +699,12 @@ config SENSORS_LTC2945
>  	  be called ltc2945.
>
>  config SENSORS_LTC2990
> -	tristate "Linear Technology LTC2990 (current monitoring mode only)"
> +	tristate "Linear Technology LTC2990"
>  	depends on I2C
>  	help
>  	  If you say yes here you get support for Linear Technology LTC2990
>  	  I2C System Monitor. The LTC2990 supports a combination of voltage,
> -	  current and temperature monitoring, but in addition to the Vcc supply
> -	  voltage and chip temperature, this driver currently only supports
> -	  reading two currents by measuring two differential voltages across
> -	  series resistors.
> +	  current and temperature monitoring.
>
>  	  This driver can also be built as a module. If so, the module will
>  	  be called ltc2990.
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> index 0ec4102..e8d36f5 100644
> --- a/drivers/hwmon/ltc2990.c
> +++ b/drivers/hwmon/ltc2990.c
> @@ -6,11 +6,7 @@
>   *
>   * License: GPLv2
>   *
> - * This driver assumes the chip is wired as a dual current monitor, and
> - * reports the voltage drop across two series resistors. It also reports
> - * the chip's internal temperature and Vcc power supply voltage.
> - *
> - * Value conversion refactored
> + * Value conversion refactored and support for all measurement modes added
>   * by Tom Levens <tom.levens-vJEk5272eHo@public.gmane.org>
>   */
>
> @@ -21,6 +17,7 @@
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>
>  #define LTC2990_STATUS	0x00
>  #define LTC2990_CONTROL	0x01
> @@ -35,32 +32,96 @@
>  #define LTC2990_CONTROL_KELVIN		BIT(7)
>  #define LTC2990_CONTROL_SINGLE		BIT(6)
>  #define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
> -#define LTC2990_CONTROL_MODE_CURRENT	0x06
> -#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
> +#define LTC2990_CONTROL_MODE_DEFAULT	0x06

I think LTC2990_CONTROL_MODE_CURRENT was better - it describes the actual mode.
Changing the define to _DEFAULT does not really improve code readability.

> +#define LTC2990_CONTROL_MODE_MAX	0x07
> +
> +#define LTC2990_IN0	BIT(0)
> +#define LTC2990_IN1	BIT(1)
> +#define LTC2990_IN2	BIT(2)
> +#define LTC2990_IN3	BIT(3)
> +#define LTC2990_IN4	BIT(4)
> +#define LTC2990_CURR1	BIT(5)
> +#define LTC2990_CURR2	BIT(6)
> +#define LTC2990_TEMP1	BIT(7)
> +#define LTC2990_TEMP2	BIT(8)
> +#define LTC2990_TEMP3	BIT(9)
> +
> +static const int ltc2990_attrs_ena[] = {
> +	LTC2990_IN1 | LTC2990_IN2 | LTC2990_TEMP3,
> +	LTC2990_CURR1 | LTC2990_TEMP3,
> +	LTC2990_CURR1 | LTC2990_IN3 | LTC2990_IN4,
> +	LTC2990_TEMP2 | LTC2990_IN3 | LTC2990_IN4,
> +	LTC2990_TEMP2 | LTC2990_CURR2,
> +	LTC2990_TEMP2 | LTC2990_TEMP3,
> +	LTC2990_CURR1 | LTC2990_CURR2,
> +	LTC2990_IN1 | LTC2990_IN2 | LTC2990_IN3 | LTC2990_IN4
> +};
> +
> +struct ltc2990_data {
> +	struct i2c_client *i2c;
> +	u32 mode;
> +};
>
>  /* Return the converted value from the given register in uV or mC */
> -static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 *result)
> +static int ltc2990_get_value(struct i2c_client *i2c, int index, s32 *result)
>  {
>  	s32 val;
> +	u8 reg;
> +
> +	switch (index) {
> +	case LTC2990_IN0:
> +		reg = LTC2990_VCC_MSB;
> +		break;
> +	case LTC2990_IN1:
> +	case LTC2990_CURR1:
> +	case LTC2990_TEMP2:
> +		reg = LTC2990_V1_MSB;
> +		break;
> +	case LTC2990_IN2:
> +		reg = LTC2990_V2_MSB;
> +		break;
> +	case LTC2990_IN3:
> +	case LTC2990_CURR2:
> +	case LTC2990_TEMP3:
> +		reg = LTC2990_V3_MSB;
> +		break;
> +	case LTC2990_IN4:
> +		reg = LTC2990_V4_MSB;
> +		break;
> +	case LTC2990_TEMP1:
> +		reg = LTC2990_TINT_MSB;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>
>  	val = i2c_smbus_read_word_swapped(i2c, reg);
>  	if (unlikely(val < 0))
>  		return val;
>
> -	switch (reg) {
> -	case LTC2990_TINT_MSB:
> -		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
> +	switch (index) {
> +	case LTC2990_TEMP1:
> +	case LTC2990_TEMP2:
> +	case LTC2990_TEMP3:
> +		/* temp, 0.0625 degrees/LSB, 13-bit  */
>  		*result = sign_extend32(val, 12) * 1000 / 16;
>  		break;
> -	case LTC2990_V1_MSB:
> -	case LTC2990_V3_MSB:
> -		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
> +	case LTC2990_CURR1:
> +	case LTC2990_CURR2:
> +		 /* Vx-Vy, 19.42uV/LSB */
>  		*result = sign_extend32(val, 14) * 1942 / 100;
>  		break;
> -	case LTC2990_VCC_MSB:
> -		/* Vcc, 305.18μV/LSB, 2.5V offset */
> +	case LTC2990_IN0:
> +		/* Vcc, 305.18uV/LSB, 2.5V offset */
>  		*result = sign_extend32(val, 14) * 30518 / (100 * 1000) + 2500;
>  		break;
> +	case LTC2990_IN1:
> +	case LTC2990_IN2:
> +	case LTC2990_IN3:
> +	case LTC2990_IN4:
> +		/* Vx: 305.18uV/LSB */
> +		*result = sign_extend32(val, 14) * 30518 / (100 * 1000);
> +		break;
>  	default:
>  		return -EINVAL; /* won't happen, keep compiler happy */
>  	}
> @@ -72,48 +133,104 @@ static ssize_t ltc2990_show_value(struct device *dev,
>  				  struct device_attribute *da, char *buf)
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	struct ltc2990_data *data = dev_get_drvdata(dev);
>  	s32 value;
>  	int ret;
>
> -	ret = ltc2990_get_value(dev_get_drvdata(dev), attr->index, &value);
> +	ret = ltc2990_get_value(data->i2c, attr->index, &value);
>  	if (unlikely(ret < 0))
>  		return ret;
>
>  	return snprintf(buf, PAGE_SIZE, "%d\n", value);
>  }
>
> +static umode_t ltc2990_attrs_visible(struct kobject *kobj,
> +				     struct attribute *a, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct ltc2990_data *data = dev_get_drvdata(dev);
> +	struct device_attribute *da =
> +			container_of(a, struct device_attribute, attr);
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +
> +	if (attr->index == LTC2990_TEMP1 ||
> +	    attr->index == LTC2990_IN0 ||
> +	    attr->index & ltc2990_attrs_ena[data->mode])
> +		return a->mode;
> +	else

Unnecessary else

> +		return 0;
> +}
> +
>  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
> -			  LTC2990_TINT_MSB);
> +			  LTC2990_TEMP1);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_TEMP2);
> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_TEMP3);
>  static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL,
> -			  LTC2990_V1_MSB);
> +			  LTC2990_CURR1);
>  static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL,
> -			  LTC2990_V3_MSB);
> +			  LTC2990_CURR2);
>  static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
> -			  LTC2990_VCC_MSB);
> +			  LTC2990_IN0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_IN1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_IN2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_IN3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_IN4);
>
>  static struct attribute *ltc2990_attrs[] = {
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
>  	&sensor_dev_attr_curr1_input.dev_attr.attr,
>  	&sensor_dev_attr_curr2_input.dev_attr.attr,
>  	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	&sensor_dev_attr_in2_input.dev_attr.attr,
> +	&sensor_dev_attr_in3_input.dev_attr.attr,
> +	&sensor_dev_attr_in4_input.dev_attr.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(ltc2990);
> +
> +static const struct attribute_group ltc2990_group = {
> +	.attrs = ltc2990_attrs,
> +	.is_visible = ltc2990_attrs_visible,
> +};
> +__ATTRIBUTE_GROUPS(ltc2990);
>
>  static int ltc2990_i2c_probe(struct i2c_client *i2c,
>  			     const struct i2c_device_id *id)
>  {
>  	int ret;
>  	struct device *hwmon_dev;
> +	struct ltc2990_data *data;
> +	struct device_node *of_node = i2c->dev.of_node;
>
>  	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>  				     I2C_FUNC_SMBUS_WORD_DATA))
>  		return -ENODEV;
>
> -	/* Setup continuous mode, current monitor */
> +	data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2990_data), GFP_KERNEL);
> +	if (unlikely(!data))
> +		return -ENOMEM;
> +	data->i2c = i2c;
> +
> +	if (!of_node || of_property_read_u32(of_node, "lltc,mode", &data->mode))
> +		data->mode = LTC2990_CONTROL_MODE_DEFAULT;

Iam arguing with myself if we should still do this or if we should read the mode
from the chip instead if it isn't provided (after all, it may have been initialized
by the BIOS/ROMMON).

Mike, would that break your application, or can you specify the mode in devicetree ?

Thanks,
Guenter

> +
> +	if (data->mode > LTC2990_CONTROL_MODE_MAX) {
> +		dev_err(&i2c->dev, "Error: Invalid mode %d.\n", data->mode);
> +		return -EINVAL;
> +	}
> +
> +	/* Setup continuous mode */
>  	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
>  					LTC2990_CONTROL_MEASURE_ALL |
> -					LTC2990_CONTROL_MODE_CURRENT);
> +					data->mode);
>  	if (ret < 0) {
>  		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
>  		return ret;
> @@ -127,7 +244,7 @@ static int ltc2990_i2c_probe(struct i2c_client *i2c,
>
>  	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
>  							   i2c->name,
> -							   i2c,
> +							   data,
>  							   ltc2990_groups);
>
>  	return PTR_ERR_OR_ZERO(hwmon_dev);
>

--
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 v2 1/3] hwmon: ltc2990: refactor value conversion
From: Guenter Roeck @ 2016-11-17 16:36 UTC (permalink / raw)
  To: Tom Levens
  Cc: jdelvare, robh+dt, mark.rutland, linux-kernel, linux-hwmon,
	devicetree
In-Reply-To: <alpine.LRH.2.20.1611171613250.12237@pcbe13573-vm.dyndns.cern.ch>

On 11/17/2016 08:23 AM, Tom Levens wrote:
> Hi Guenter,
>
> Thanks for taking the time to review the patch.
>
> On Thu, 17 Nov 2016, Guenter Roeck wrote:
>
>> Hi Tom,
>>
>> On 11/17/2016 04:10 AM, Tom Levens wrote:
>>>  Conversion from raw values to signed integers has been refactored using
>>>  the macros in bitops.h.
>>>
>> Please also mention that this fixes a bug in negative temperature conversions.
>
> Yes, of course, I will include the information in v3.
>
>>
>>>  Signed-off-by: Tom Levens <tom.levens@cern.ch>
>>>  ---
>>>   drivers/hwmon/ltc2990.c |   27 ++++++++++-----------------
>>>   1 files changed, 10 insertions(+), 17 deletions(-)
>>>
>>>  diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>>>  index 8f8fe05..0ec4102 100644
>>>  --- a/drivers/hwmon/ltc2990.c
>>>  +++ b/drivers/hwmon/ltc2990.c
>>>  @@ -9,8 +9,12 @@
>>>    * This driver assumes the chip is wired as a dual current monitor, and
>>>    * reports the voltage drop across two series resistors. It also reports
>>>    * the chip's internal temperature and Vcc power supply voltage.
>>>  + *
>>>  + * Value conversion refactored
>>>  + * by Tom Levens <tom.levens@cern.ch>
>>
>> Kind of unusual to do that for minor changes like this. Imagine if everyone would do that.
>> The commit log is what gives you credit.
>
> Good point, thanks for the hint. I will remove it from v3.
>
>>>    */
>>>
>>>  +#include <linux/bitops.h>
>>>   #include <linux/err.h>
>>>   #include <linux/hwmon.h>
>>>   #include <linux/hwmon-sysfs.h>
>>>  @@ -34,19 +38,10 @@
>>>   #define LTC2990_CONTROL_MODE_CURRENT    0x06
>>>   #define LTC2990_CONTROL_MODE_VOLTAGE    0x07
>>>
>>>  -/* convert raw register value to sign-extended integer in 16-bit range */
>>>  -static int ltc2990_voltage_to_int(int raw)
>>>  -{
>>>  -    if (raw & BIT(14))
>>>  -        return -(0x4000 - (raw & 0x3FFF)) << 2;
>>>  -    else
>>>  -        return (raw & 0x3FFF) << 2;
>>>  -}
>>>  -
>>>  /* Return the converted value from the given register in uV or mC */
>>>  -static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>>>  +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 *result)
>>>  {
>>>  -    int val;
>>>  +    s32 val;
>>
>> Please just leave the variable type alone. it is also used for the return value
>> from i2c_smbus_read_word_swapped(), which is an int, and changing it to s32 doesn't really make the code better.
>
> According to i2c.h the return type for i2c_smbus_read_word_swapped() is s32, which is why I modified it here. But it could be changed back if you think it is better to leave it as an int.
>
Ah, ok. Good to know. Please leave it anyway, reason being that there is no real
reason to change it. Effectively those are just whitespace changes (unlike the rest,
which is part bug fix, part cleanup).

>> Can you send me a register map for the chip ? I would like to write a module test.
>
> Here is an example register dump:

I meant the output of i2cdump (something like "i2cdump -y -f <bus> <i2c-address> w").

Thanks,
Guenter

>
> 00 00 00 00
> 01 90 07 d0
> 2c cd 7d 80
> 7c 29 20 00
>
> The expected values in this case are:
>
> in0_input     5000
> in1_input    610
> in2_input    3500
> in3_input    -195
> in4_input    -299
> temp1_input     25000
> temp2_input     125000
> temp3_input    -40000
> curr1_input    38840
> curr2_input    -12428
>
> Testing with lltc,mode set to <5>, <6> and <7> should give you all measurements.
>
>> Thanks,
>> Guenter
>
> Cheers,
>

^ permalink raw reply

* Re: [PATCH 2/2] Add basic support for DW CSI-2 Host IPK
From: Sakari Ailus @ 2016-11-17 16:34 UTC (permalink / raw)
  To: Ramiro Oliveira, g
  Cc: robh+dt, mark.rutland, mchehab, devicetree, linux-kernel,
	linux-media, davem, gregkh, geert+renesas, akpm, linux, hverkuil,
	laurent.pinchart+renesas, arnd, sudipm.mukherjee, tiffany.lin,
	minghsiu.tsai, jean-christophe.trotin, andrew-ct.chen,
	simon.horman, songjun.wu, bparrot, CARLOS.PALMINHA
In-Reply-To: <c1699c43562aaae69ab851ff3955086131119c51.1479132355.git.roliveir@synopsys.com>

Hi Ramiro,

Thank you for the patchset. Please see my comments below.

If you've got a CSI-2 receiver, why do you use the DV timing presets? What
do you use those for?

On Mon, Nov 14, 2016 at 02:20:23PM +0000, Ramiro Oliveira wrote:
> Add support for basic configuration of the DW CSI-2 Host IPK
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
> ---
>  MAINTAINERS                                 |   7 +
>  drivers/media/platform/Kconfig              |   1 +
>  drivers/media/platform/Makefile             |   2 +
>  drivers/media/platform/dwc/Kconfig          |  36 ++
>  drivers/media/platform/dwc/Makefile         |   3 +
>  drivers/media/platform/dwc/dw_mipi_csi.c    | 687 +++++++++++++++++++++++
>  drivers/media/platform/dwc/dw_mipi_csi.h    | 179 ++++++
>  drivers/media/platform/dwc/plat_ipk.c       | 835 ++++++++++++++++++++++++++++
>  drivers/media/platform/dwc/plat_ipk.h       |  97 ++++
>  drivers/media/platform/dwc/plat_ipk_video.h |  97 ++++
>  drivers/media/platform/dwc/video_device.c   | 741 ++++++++++++++++++++++++
>  drivers/media/platform/dwc/video_device.h   | 101 ++++
>  12 files changed, 2786 insertions(+)
>  create mode 100644 drivers/media/platform/dwc/Kconfig
>  create mode 100644 drivers/media/platform/dwc/Makefile
>  create mode 100644 drivers/media/platform/dwc/dw_mipi_csi.c
>  create mode 100644 drivers/media/platform/dwc/dw_mipi_csi.h
>  create mode 100644 drivers/media/platform/dwc/plat_ipk.c
>  create mode 100644 drivers/media/platform/dwc/plat_ipk.h
>  create mode 100644 drivers/media/platform/dwc/plat_ipk_video.h
>  create mode 100644 drivers/media/platform/dwc/video_device.c
>  create mode 100644 drivers/media/platform/dwc/video_device.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6a71422..f54dfd8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10618,6 +10618,13 @@ S:	Maintained
>  F:	drivers/staging/media/st-cec/
>  F:	Documentation/devicetree/bindings/media/stih-cec.txt
>  
> +SYNOPSYS DESIGNWARE CSI-2 HOST IPK DRIVER
> +M:	Ramiro Oliveira <roliveir@synopsys.com>
> +L:	linux-media@vger.kernel.org
> +T:	git git://linuxtv.org/media_tree.git
> +S:	Maintained
> +F:	drivers/media/platform/dwc/
> +
>  SYNOPSYS DESIGNWARE DMAC DRIVER
>  M:	Viresh Kumar <vireshk@kernel.org>
>  M:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 754edbf1..6fef760f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -120,6 +120,7 @@ source "drivers/media/platform/am437x/Kconfig"
>  source "drivers/media/platform/xilinx/Kconfig"
>  source "drivers/media/platform/rcar-vin/Kconfig"
>  source "drivers/media/platform/atmel/Kconfig"
> +source "drivers/media/platform/dwc/Kconfig"
>  
>  config VIDEO_TI_CAL
>  	tristate "TI CAL (Camera Adaptation Layer) driver"
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index f842933..623f5d2 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -61,6 +61,8 @@ obj-$(CONFIG_VIDEO_RCAR_VIN)		+= rcar-vin/
>  
>  obj-$(CONFIG_VIDEO_ATMEL_ISC)		+= atmel/
>  
> +obj-$(CONFIG_VIDEO_DWC)			+= dwc/
> +
>  ccflags-y += -I$(srctree)/drivers/media/i2c
>  
>  obj-$(CONFIG_VIDEO_MEDIATEK_VPU)	+= mtk-vpu/
> diff --git a/drivers/media/platform/dwc/Kconfig b/drivers/media/platform/dwc/Kconfig
> new file mode 100644
> index 0000000..fb8533b
> --- /dev/null
> +++ b/drivers/media/platform/dwc/Kconfig
> @@ -0,0 +1,36 @@
> +config VIDEO_DWC
> +	bool "Designware Cores CSI-2 IPK (EXPERIMENTAL)"
> +	depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA
> +	help
> +	  Say Y here to enable support for the DesignWare Cores CSI-2 Host IP
> +	  Prototyping Kit.
> +
> +if VIDEO_DWC
> +config DWC_PLATFORM
> +	tristate "SNPS DWC MIPI CSI2 Host"
> +	depends on VIDEO_DWC
> +	help
> +	  This is the V4L2 plaftorm driver driver for the DWC CSI-2 HOST IPK
> +
> +	  To compile this driver as a module, choose M here
> +
> +
> +config DWC_MIPI_CSI2_HOST
> +	tristate "SNPS DWC MIPI CSI2 Host"
> +	select GENERIC_PHY
> +	depends on VIDEO_DWC
> +	help
> +	  This is a V4L2 driver for SNPS DWC MIPI-CSI2
> +
> +	  To compile this driver as a module, choose M here
> +
> +config DWC_VIDEO_DEVICE
> +	tristate "DWC VIDEO DEVICE"
> +	select VIDEOBUF2_VMALLOC
> +	select VIDEOBUF2_DMA_CONTIG
> +	depends on VIDEO_DWC
> +	help
> +	  This is a V4L2 driver for SNPS Video device
> +	  To compile this driver as a module, choose M here
> +
> +endif # VIDEO_DWC
> diff --git a/drivers/media/platform/dwc/Makefile b/drivers/media/platform/dwc/Makefile
> new file mode 100644
> index 0000000..75c74b7
> --- /dev/null
> +++ b/drivers/media/platform/dwc/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_DWC_PLATFORM)		+= plat_ipk.o
> +obj-$(CONFIG_DWC_MIPI_CSI2_HOST)	+= dw_mipi_csi.o
> +obj-$(CONFIG_DWC_VIDEO_DEVICE)		+= video_device.o
> diff --git a/drivers/media/platform/dwc/dw_mipi_csi.c b/drivers/media/platform/dwc/dw_mipi_csi.c
> new file mode 100644
> index 0000000..1337a5e
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw_mipi_csi.c
> @@ -0,0 +1,687 @@
> +/*
> + * DWC MIPI CSI-2 Host device driver
> + *
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + * Author: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation, either version 2 of the License,
> + * or (at your option) any later version.
> + */
> +
> +
> +#include "dw_mipi_csi.h"
> +
> +/** @short Driver args: debug */
> +static unsigned int debug;
> +module_param(debug, uint, 0644);
> +MODULE_PARM_DESC(debug, "Activates debug info");

There should be no need for module specific debug parameters. Please use
dynamic debug instead.

> +
> +/*
> + * Basic IO read and write operations
> + */
> +
> +/**
> + * @short Video formats supported by the MIPI CSI-2
> + */
> +static const struct mipi_fmt dw_mipi_csi_formats[] = {
> +	{
> +	 .name = "RAW BGGR 8",

Do you use the name field for something? If not, I'd suggest to drop it.

> +	 .code = MEDIA_BUS_FMT_SBGGR8_1X8,
> +	 .depth = 8,
> +	 },
> +	{
> +	 .name = "RAW10",
> +	 .code = MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE,
> +	 .depth = 10,
> +	 },
> +	{
> +	 .name = "RGB565",
> +	 .code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> +	 .depth = 16,
> +	 },
> +	{
> +	 .name = "BGR565",
> +	 .code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> +	 .depth = 16,
> +	 },
> +	{
> +	 .name = "RGB888",
> +	 .code = MEDIA_BUS_FMT_RGB888_2X12_LE,
> +	 .depth = 24,
> +	 },
> +	{
> +	 .name = "BGR888",
> +	 .code = MEDIA_BUS_FMT_RGB888_2X12_BE,
> +	 .depth = 24,
> +	 },
> +};
> +
> +static struct mipi_csi_dev *
> +sd_to_mipi_csi_dev(struct v4l2_subdev *sdev)
> +{
> +	return container_of(sdev, struct mipi_csi_dev, sd);
> +}
> +
> +void
> +dw_mipi_csi_write(struct mipi_csi_dev *dev,
> +		  unsigned int address, unsigned int data)
> +{
> +	iowrite32(data, dev->base_address + address);
> +}
> +
> +u32
> +dw_mipi_csi_read(struct mipi_csi_dev *dev, unsigned long address)
> +{
> +	return ioread32(dev->base_address + address);
> +}
> +
> +void
> +dw_mipi_csi_write_part(struct mipi_csi_dev *dev,
> +		       unsigned long address, unsigned long data,
> +		       unsigned char shift, unsigned char width)
> +{
> +	u32 mask = (1 << width) - 1;
> +	u32 temp = dw_mipi_csi_read(dev, address);
> +
> +	temp &= ~(mask << shift);
> +	temp |= (data & mask) << shift;
> +	dw_mipi_csi_write(dev, address, temp);
> +}
> +
> +static const struct mipi_fmt *
> +find_dw_mipi_csi_format(struct v4l2_mbus_framefmt *mf)
> +{
> +	int i;

unsigned int

> +
> +	for (i = 0; i < ARRAY_SIZE(dw_mipi_csi_formats); i++)
> +		if (mf->code == dw_mipi_csi_formats[i].code)
> +			return &dw_mipi_csi_formats[i];
> +	return NULL;
> +}
> +
> +int
> +enable_video_output(struct mipi_csi_dev *dev)
> +{
> +	return 0;
> +}

This one is unused.

> +
> +int
> +disable_video_output(struct mipi_csi_dev *dev)
> +{
> +	phy_power_off(dev->phy);
> +	return 0;
> +}

This one as well.

> +
> +static void
> +dw_mipi_csi_reset(struct mipi_csi_dev *dev)
> +{
> +	dw_mipi_csi_write(dev, R_CSI2_CTRL_RESETN, 0);
> +	/* mdelay(1); */
> +	dw_mipi_csi_write(dev, R_CSI2_CTRL_RESETN, 1);
> +}
> +
> +static int
> +dw_mipi_csi_mask_irq_power_off(struct mipi_csi_dev *dev)
> +{
> +	/* set only one lane (lane 0) as active (ON) */
> +	dw_mipi_csi_write(dev, R_CSI2_N_LANES, 0);
> +
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PHY_FATAL, 0);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PKT_FATAL, 0);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_FRAME_FATAL, 0);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PHY, 0);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PKT, 0);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_LINE, 0);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_IPI, 0);
> +
> +	dw_mipi_csi_write(dev, R_CSI2_CTRL_RESETN, 0);
> +
> +	return 0;
> +
> +}
> +
> +static int
> +dw_mipi_csi_hw_stdby(struct mipi_csi_dev *dev)
> +{
> +	/* set only one lane (lane 0) as active (ON) */
> +	dw_mipi_csi_reset(dev);
> +
> +	dw_mipi_csi_write(dev, R_CSI2_N_LANES, 0);
> +
> +	phy_init(dev->phy);
> +
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PHY_FATAL, 0xFFFFFFFF);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PKT_FATAL, 0xFFFFFFFF);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_FRAME_FATAL, 0xFFFFFFFF);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PHY, 0xFFFFFFFF);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_PKT, 0xFFFFFFFF);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_LINE, 0xFFFFFFFF);
> +	dw_mipi_csi_write(dev, R_CSI2_MASK_INT_IPI, 0xFFFFFFFF);
> +
> +	return 0;
> +
> +}
> +
> +void

Shouldn't this be static? The same applies to many other symbols as well.

> +dw_mipi_csi_set_ipi_fmt(struct mipi_csi_dev *dev)
> +{
> +	switch (dev->fmt->code) {
> +	case MEDIA_BUS_FMT_RGB565_2X8_BE:
> +	case MEDIA_BUS_FMT_RGB565_2X8_LE:
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_DATA_TYPE, CSI_2_RGB565);
> +		v4l2_dbg(1, debug, &dev->sd, "DT: RGB 565");
> +		break;
> +
> +	case MEDIA_BUS_FMT_RGB888_2X12_LE:
> +	case MEDIA_BUS_FMT_RGB888_2X12_BE:
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_DATA_TYPE, CSI_2_RGB888);
> +		v4l2_dbg(1, debug, &dev->sd, "DT: RGB 888");
> +		break;
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_DATA_TYPE, CSI_2_RAW10);
> +		v4l2_dbg(1, debug, &dev->sd, "DT: RAW 10");
> +		break;
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_DATA_TYPE, CSI_2_RAW8);
> +		v4l2_dbg(1, debug, &dev->sd, "DT: RAW 8");
> +		break;
> +	default:
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_DATA_TYPE, CSI_2_RGB565);
> +		v4l2_dbg(1, debug, &dev->sd, "Error");
> +		break;
> +	}
> +}
> +
> +void
> +__dw_mipi_csi_fill_timings(struct mipi_csi_dev *dev,
> +			   const struct v4l2_bt_timings *bt)
> +{
> +
> +	if (bt == NULL)
> +		return;
> +
> +	dev->hw.hsa = bt->hsync;
> +	dev->hw.hbp = bt->hbackporch;
> +	dev->hw.hsd = bt->hsync;
> +	dev->hw.htotal = bt->height + bt->vfrontporch +
> +	    bt->vsync + bt->vbackporch;
> +	dev->hw.vsa = bt->vsync;
> +	dev->hw.vbp = bt->vbackporch;
> +	dev->hw.vfp = bt->vfrontporch;
> +	dev->hw.vactive = bt->height;
> +}

There seem to be quite a few unused functions here.

> +
> +void
> +dw_mipi_csi_start(struct mipi_csi_dev *dev)
> +{
> +	const struct v4l2_bt_timings *bt = &v4l2_dv_timings_presets[0].bt;
> +
> +	__dw_mipi_csi_fill_timings(dev, bt);
> +
> +	dw_mipi_csi_write(dev, R_CSI2_N_LANES, (dev->hw.num_lanes - 1));
> +	v4l2_dbg(1, debug, &dev->sd, "N Lanes: %d\n", dev->hw.num_lanes);
> +
> +	/*IPI Related Configuration */
> +	if ((dev->hw.output_type == IPI_OUT)
> +	    || (dev->hw.output_type == BOTH_OUT)) {
> +
> +		dw_mipi_csi_write_part(dev, R_CSI2_IPI_MODE, dev->hw.ipi_mode,
> +				       0, 1);
> +		v4l2_dbg(1, debug, &dev->sd, "IPI MODE: %d\n",
> +			 dev->hw.ipi_mode);
> +
> +		dw_mipi_csi_write_part(dev, R_CSI2_IPI_MODE,
> +				       dev->hw.ipi_color_mode, 8, 1);
> +		v4l2_dbg(1, debug, &dev->sd, "Color Mode: %d\n",
> +			 dev->hw.ipi_color_mode);
> +
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_VCID, dev->hw.virtual_ch);
> +		v4l2_dbg(1, debug, &dev->sd, "Virtual Channel: %d\n",
> +			 dev->hw.virtual_ch);
> +
> +		dw_mipi_csi_write_part(dev, R_CSI2_IPI_MEM_FLUSH,
> +				       dev->hw.ipi_auto_flush, 8, 1);
> +		v4l2_dbg(1, debug, &dev->sd, "Auto-flush: %d\n",
> +			 dev->hw.ipi_auto_flush);
> +
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_HSA_TIME, dev->hw.hsa);
> +		v4l2_dbg(1, debug, &dev->sd, "HSA: %d\n", dev->hw.hsa);
> +
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_HBP_TIME, dev->hw.hbp);
> +		v4l2_dbg(1, debug, &dev->sd, "HBP: %d\n", dev->hw.hbp);
> +
> +		dw_mipi_csi_write(dev, R_CSI2_IPI_HSD_TIME, dev->hw.hsd);
> +		v4l2_dbg(1, debug, &dev->sd, "HSD: %d\n", dev->hw.hsd);
> +
> +		if (dev->hw.ipi_mode == AUTO_TIMING) {
> +			dw_mipi_csi_write(dev, R_CSI2_IPI_HLINE_TIME,
> +					  dev->hw.htotal);
> +			v4l2_dbg(1, debug, &dev->sd, "H total: %d\n",
> +				 dev->hw.htotal);
> +
> +			dw_mipi_csi_write(dev, R_CSI2_IPI_VSA_LINES,
> +					  dev->hw.vsa);
> +			v4l2_dbg(1, debug, &dev->sd, "VSA: %d\n", dev->hw.vsa);
> +
> +			dw_mipi_csi_write(dev, R_CSI2_IPI_VBP_LINES,
> +					  dev->hw.vbp);
> +			v4l2_dbg(1, debug, &dev->sd, "VBP: %d\n", dev->hw.vbp);
> +
> +			dw_mipi_csi_write(dev, R_CSI2_IPI_VFP_LINES,
> +					  dev->hw.vfp);
> +			v4l2_dbg(1, debug, &dev->sd, "VFP: %d\n", dev->hw.vfp);
> +
> +			dw_mipi_csi_write(dev, R_CSI2_IPI_VACTIVE_LINES,
> +					  dev->hw.vactive);
> +			v4l2_dbg(1, debug, &dev->sd, "V Active: %d\n",
> +				 dev->hw.vactive);
> +		}
> +	}
> +
> +	phy_power_on(dev->phy);
> +}
> +
> +static int
> +dw_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index >= ARRAY_SIZE(dw_mipi_csi_formats))
> +		return -EINVAL;
> +
> +	code->code = dw_mipi_csi_formats[code->index].code;
> +	return 0;
> +}
> +
> +static struct mipi_fmt const *
> +dw_mipi_csi_try_format(struct v4l2_mbus_framefmt *mf)
> +{
> +	struct mipi_fmt const *fmt;
> +
> +	fmt = find_dw_mipi_csi_format(mf);
> +	if (fmt == NULL)
> +		fmt = &dw_mipi_csi_formats[0];
> +
> +	mf->code = fmt->code;
> +	return fmt;
> +}
> +
> +static struct v4l2_mbus_framefmt *
> +__dw_mipi_csi_get_format(struct mipi_csi_dev *dev,
> +			 struct v4l2_subdev_pad_config *cfg,
> +			 enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return cfg ? v4l2_subdev_get_try_format(&dev->sd, cfg,
> +							0) : NULL;
> +
> +	return &dev->format;
> +}
> +
> +static int
> +dw_mipi_csi_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg,
> +		    struct v4l2_subdev_format *fmt)
> +{
> +	struct mipi_csi_dev *dev = sd_to_mipi_csi_dev(sd);
> +	struct mipi_fmt const *dev_fmt;
> +	struct v4l2_mbus_framefmt *mf;
> +	int i = 0;

unsigned int

> +	const struct v4l2_bt_timings *bt_r = &v4l2_dv_timings_presets[0].bt;
> +
> +	mf = __dw_mipi_csi_get_format(dev, cfg, fmt->which);
> +
> +	dev_fmt = dw_mipi_csi_try_format(&fmt->format);
> +	if (dev_fmt) {
> +		*mf = fmt->format;
> +		if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +			dev->fmt = dev_fmt;
> +		dw_mipi_csi_set_ipi_fmt(dev);
> +	}
> +	while (v4l2_dv_timings_presets[i].bt.width) {
> +		const struct v4l2_bt_timings *bt =
> +		    &v4l2_dv_timings_presets[i].bt;
> +		if (mf->width == bt->width && mf->height == bt->width) {
> +			__dw_mipi_csi_fill_timings(dev, bt);
> +			return 0;
> +		}
> +		i++;
> +	}
> +
> +	__dw_mipi_csi_fill_timings(dev, bt_r);
> +	return 0;
> +
> +}
> +
> +static int
> +dw_mipi_csi_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg,
> +		    struct v4l2_subdev_format *fmt)
> +{
> +	struct mipi_csi_dev *dev = sd_to_mipi_csi_dev(sd);
> +	struct v4l2_mbus_framefmt *mf;
> +
> +	mf = __dw_mipi_csi_get_format(dev, cfg, fmt->which);
> +	if (!mf)
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->lock);
> +	fmt->format = *mf;
> +	mutex_unlock(&dev->lock);
> +	return 0;
> +}
> +
> +static int
> +dw_mipi_csi_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct mipi_csi_dev *dev = sd_to_mipi_csi_dev(sd);
> +
> +	if (on) {
> +		dw_mipi_csi_hw_stdby(dev);
> +		dw_mipi_csi_start(dev);
> +	} else {
> +		dw_mipi_csi_mask_irq_power_off(dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +dw_mipi_csi_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	struct v4l2_mbus_framefmt *format =
> +	    v4l2_subdev_get_try_format(sd, fh->pad, 0);
> +
> +	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->code = dw_mipi_csi_formats[0].code;
> +	format->width = MIN_WIDTH;
> +	format->height = MIN_HEIGHT;
> +	format->field = V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw_mipi_csi_sd_internal_ops = {
> +	.open = dw_mipi_csi_open,
> +};
> +
> +static struct v4l2_subdev_core_ops dw_mipi_csi_core_ops = {
> +	.s_power = dw_mipi_csi_s_power,
> +};
> +
> +static struct v4l2_subdev_pad_ops dw_mipi_csi_pad_ops = {
> +	.enum_mbus_code = dw_mipi_csi_enum_mbus_code,
> +	.get_fmt = dw_mipi_csi_get_fmt,
> +	.set_fmt = dw_mipi_csi_set_fmt,
> +};
> +
> +static struct v4l2_subdev_ops dw_mipi_csi_subdev_ops = {
> +	.core = &dw_mipi_csi_core_ops,
> +	.pad = &dw_mipi_csi_pad_ops,
> +};
> +
> +static irqreturn_t
> +dw_mipi_csi_irq1(int irq, void *dev_id)
> +{
> +
> +	struct mipi_csi_dev *dev = dev_id;
> +	u32 int_status, ind_status;
> +	unsigned long flags;
> +
> +	int_status = dw_mipi_csi_read(dev, R_CSI2_INTERRUPT);
> +	spin_lock_irqsave(&dev->slock, flags);
> +
> +	if (int_status & CSI2_INT_PHY_FATAL) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_PHY_FATAL);
> +		pr_info_ratelimited("%08X CSI INT PHY FATAL: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);

I don't think you should use pr_info() for interrupts. This is basically
debug information, isn't it?

> +	}
> +
> +	if (int_status & CSI2_INT_PKT_FATAL) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_PKT_FATAL);
> +		pr_info_ratelimited("%08X CSI INT PKT FATAL: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);
> +	}
> +
> +	if (int_status & CSI2_INT_FRAME_FATAL) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_FRAME_FATAL);
> +		pr_info_ratelimited("%08X CSI INT FRAME FATAL: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);
> +	}
> +
> +	if (int_status & CSI2_INT_PHY) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_PHY);
> +		pr_info_ratelimited("%08X CSI INT PHY: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);
> +	}
> +
> +	if (int_status & CSI2_INT_PKT) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_PKT);
> +		pr_info_ratelimited("%08X CSI INT PKT: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);
> +	}
> +
> +	if (int_status & CSI2_INT_LINE) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_LINE);
> +		pr_info_ratelimited("%08X CSI INT LINE: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);
> +	}
> +
> +	if (int_status & CSI2_INT_IPI) {
> +		ind_status = dw_mipi_csi_read(dev, R_CSI2_INT_IPI);
> +		pr_info_ratelimited("%08X CSI INT IPI: %08X\n",
> +				    (uint32_t) dev->base_address, ind_status);
> +	}
> +	spin_unlock_irqrestore(&dev->slock, flags);
> +	return IRQ_HANDLED;
> +}
> +
> +static int
> +dw_mipi_csi_parse_dt(struct platform_device *pdev, struct mipi_csi_dev *dev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	int reg;
> +	int ret = 0;
> +
> +	/* Device tree information */
> +	ret = of_property_read_u32(node, "data-lanes", &dev->hw.num_lanes);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read data-lanes\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "output-type", &dev->hw.output_type);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read output-type\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "ipi-mode", &dev->hw.ipi_mode);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read ipi-mode\n");
> +		return ret;
> +	}
> +
> +	ret =
> +	    of_property_read_u32(node, "ipi-auto-flush",
> +				 &dev->hw.ipi_auto_flush);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read ipi-auto-flush\n");
> +		return ret;
> +	}
> +
> +	ret =
> +	    of_property_read_u32(node, "ipi-color-mode",
> +				 &dev->hw.ipi_color_mode);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read ipi-color-mode\n");
> +		return ret;
> +	}
> +
> +	ret =
> +	    of_property_read_u32(node, "virtual-channel", &dev->hw.virtual_ch);

Is this some thing that should come from DT? Sure, we don't really have
proper support for virtual channels right now, but could you use zero for
now?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read virtual-channel\n");
> +		return ret;
> +	}
> +
> +	node = of_get_child_by_name(node, "port");
> +	if (!node)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32(node, "reg", &reg);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't read reg value\n");
> +		return ret;
> +	}
> +	dev->index = reg - 1;
> +
> +	if (dev->index >= CSI_MAX_ENTITIES)
> +		return -ENXIO;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dw_mipi_csi_of_match[];
> +
> +/**
> + * @short Initialization routine - Entry point of the driver
> + * @param[in] pdev pointer to the platform device structure
> + * @return 0 on success and a negative number on failure
> + * Refer to Linux errors.
> + */
> +static int mipi_csi_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_id;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res = NULL;
> +	struct mipi_csi_dev *mipi_csi;
> +	int ret = -ENOMEM;
> +
> +	dev_info(&pdev->dev, "Installing MIPI CSI-2 module\n");
> +
> +	dev_dbg(&pdev->dev, "Device registration\n");

At least one of these is redundant. I'd just remove both.

> +	mipi_csi = devm_kzalloc(dev, sizeof(*mipi_csi), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	mutex_init(&mipi_csi->lock);
> +	spin_lock_init(&mipi_csi->slock);
> +	mipi_csi->pdev = pdev;
> +
> +	of_id = of_match_node(dw_mipi_csi_of_match, dev->of_node);
> +	if (WARN_ON(of_id == NULL))
> +		return -EINVAL;
> +
> +	ret = dw_mipi_csi_parse_dt(pdev, mipi_csi);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev, "Request DPHY\n");
> +	mipi_csi->phy = devm_phy_get(dev, "csi2-dphy");
> +	if (IS_ERR(mipi_csi->phy))
> +		return PTR_ERR(mipi_csi->phy);

It'd be better to tell what's wrong when there's an error instead of
printing a series of messages even when all is well.

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mipi_csi->base_address = devm_ioremap_resource(dev, res);
> +
> +	if (IS_ERR(mipi_csi->base_address))
> +		return PTR_ERR(mipi_csi->base_address);
> +
> +	mipi_csi->ctrl_irq_number = platform_get_irq(pdev, 0);
> +	if (mipi_csi->ctrl_irq_number <= 0) {
> +		dev_err(dev, "IRQ number not set.\n");
> +		return mipi_csi->ctrl_irq_number;
> +	}
> +
> +	ret = devm_request_irq(dev, mipi_csi->ctrl_irq_number,
> +			       dw_mipi_csi_irq1, IRQF_SHARED,
> +			       dev_name(dev), mipi_csi);
> +	if (ret) {
> +		dev_err(dev, "IRQ failed\n");
> +		goto end;
> +	}
> +
> +	v4l2_subdev_init(&mipi_csi->sd, &dw_mipi_csi_subdev_ops);
> +	mipi_csi->sd.owner = THIS_MODULE;
> +	snprintf(mipi_csi->sd.name, sizeof(mipi_csi->sd.name), "%s.%d",
> +		 CSI_DEVICE_NAME, mipi_csi->index);
> +	mipi_csi->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	mipi_csi->fmt = &dw_mipi_csi_formats[0];
> +
> +	mipi_csi->format.code = dw_mipi_csi_formats[0].code;
> +	mipi_csi->format.width = MIN_WIDTH;
> +	mipi_csi->format.height = MIN_HEIGHT;
> +
> +	mipi_csi->pads[CSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +	mipi_csi->pads[CSI_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&mipi_csi->sd.entity,
> +				     CSI_PADS_NUM, mipi_csi->pads);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "Media Entity init failed\n");
> +		goto entity_cleanup;
> +	}
> +
> +	/* This allows to retrieve the platform device id by the host driver */
> +	v4l2_set_subdevdata(&mipi_csi->sd, pdev);
> +
> +	/* .. and a pointer to the subdev. */
> +	platform_set_drvdata(pdev, &mipi_csi->sd);
> +
> +	dw_mipi_csi_mask_irq_power_off(mipi_csi);
> +	dev_info(dev, "DW MIPI CSI-2 Host registered successfully\n");
> +	return 0;
> +
> +entity_cleanup:
> +	media_entity_cleanup(&mipi_csi->sd.entity);
> +end:
> +	return ret;
> +}
> +
> +/**
> + * @short Exit routine - Exit point of the driver
> + * @param[in] pdev pointer to the platform device structure
> + * @return 0 on success and a negative number on failure
> + * Refer to Linux errors.
> + */
> +static int mipi_csi_remove(struct platform_device *pdev)
> +{
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct mipi_csi_dev *mipi_csi = sd_to_mipi_csi_dev(sd);
> +
> +	dev_dbg(&pdev->dev, "Removing MIPI CSI-2 module\n");
> +	media_entity_cleanup(&mipi_csi->sd.entity);
> +
> +	return 0;
> +}
> +
> +/**
> + * @short of_device_id structure
> + */
> +static const struct of_device_id dw_mipi_csi_of_match[] = {
> +	{
> +	 .compatible = "snps,dw-mipi-csi"},
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, dw_mipi_csi_of_match);
> +
> +/**
> + * @short Platform driver structure
> + */
> +static struct platform_driver __refdata dw_mipi_csi_pdrv = {
> +	.remove = mipi_csi_remove,
> +	.probe = mipi_csi_probe,
> +	.driver = {
> +		   .name = CSI_DEVICE_NAME,
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = dw_mipi_csi_of_match,
> +		   },
> +};
> +
> +module_platform_driver(dw_mipi_csi_pdrv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Ramiro Oliveira <roliveir@synopsys.com>");
> +MODULE_DESCRIPTION("Synopsys DW MIPI CSI-2 Host driver");
> diff --git a/drivers/media/platform/dwc/dw_mipi_csi.h b/drivers/media/platform/dwc/dw_mipi_csi.h
> new file mode 100644
> index 0000000..5e4c48c
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw_mipi_csi.h
> @@ -0,0 +1,179 @@
> +/*
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef DW_MIPI_CSI_H_
> +#define DW_MIPI_CSI_H_
> +
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_graph.h>
> +#include <linux/delay.h>
> +#include <linux/wait.h>
> +#include <linux/string.h>
> +#include <linux/phy/phy.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-dv-timings.h>
> +#include <linux/videodev2.h>
> +#include <linux/io.h>

Please arrange alphabetically. Same for other files.

> +
> +#include "plat_ipk_video.h"
> +
> +#define CSI_DEVICE_NAME	"dw-mipi-csi"
> +
> +/** @short DWC MIPI CSI-2 register addresses*/
> +enum register_addresses {
> +	R_CSI2_VERSION = 0x00,
> +	R_CSI2_N_LANES = 0x04,
> +	R_CSI2_CTRL_RESETN = 0x08,
> +	R_CSI2_INTERRUPT = 0x0C,
> +	R_CSI2_DATA_IDS_1 = 0x10,
> +	R_CSI2_DATA_IDS_2 = 0x14,
> +	R_CSI2_IPI_MODE = 0x80,
> +	R_CSI2_IPI_VCID = 0x84,
> +	R_CSI2_IPI_DATA_TYPE = 0x88,
> +	R_CSI2_IPI_MEM_FLUSH = 0x8C,
> +	R_CSI2_IPI_HSA_TIME = 0x90,
> +	R_CSI2_IPI_HBP_TIME = 0x94,
> +	R_CSI2_IPI_HSD_TIME = 0x98,
> +	R_CSI2_IPI_HLINE_TIME = 0x9C,
> +	R_CSI2_IPI_VSA_LINES = 0xB0,
> +	R_CSI2_IPI_VBP_LINES = 0xB4,
> +	R_CSI2_IPI_VFP_LINES = 0xB8,
> +	R_CSI2_IPI_VACTIVE_LINES = 0xBC,
> +	R_CSI2_INT_PHY_FATAL = 0xe0,
> +	R_CSI2_MASK_INT_PHY_FATAL = 0xe4,
> +	R_CSI2_FORCE_INT_PHY_FATAL = 0xe8,
> +	R_CSI2_INT_PKT_FATAL = 0xf0,
> +	R_CSI2_MASK_INT_PKT_FATAL = 0xf4,
> +	R_CSI2_FORCE_INT_PKT_FATAL = 0xf8,
> +	R_CSI2_INT_FRAME_FATAL = 0x100,
> +	R_CSI2_MASK_INT_FRAME_FATAL = 0x104,
> +	R_CSI2_FORCE_INT_FRAME_FATAL = 0x108,
> +	R_CSI2_INT_PHY = 0x110,
> +	R_CSI2_MASK_INT_PHY = 0x114,
> +	R_CSI2_FORCE_INT_PHY = 0x118,
> +	R_CSI2_INT_PKT = 0x120,
> +	R_CSI2_MASK_INT_PKT = 0x124,
> +	R_CSI2_FORCE_INT_PKT = 0x128,
> +	R_CSI2_INT_LINE = 0x130,
> +	R_CSI2_MASK_INT_LINE = 0x134,
> +	R_CSI2_FORCE_INT_LINE = 0x138,
> +	R_CSI2_INT_IPI = 0x140,
> +	R_CSI2_MASK_INT_IPI = 0x144,
> +	R_CSI2_FORCE_INT_IPI = 0x148
> +};
> +
> +/** @short IPI Data Types */
> +enum data_type {
> +	CSI_2_YUV420_8 = 0x18,
> +	CSI_2_YUV420_10 = 0x19,
> +	CSI_2_YUV420_8_LEG = 0x1A,
> +	CSI_2_YUV420_8_SHIFT = 0x1C,
> +	CSI_2_YUV420_10_SHIFT = 0x1D,
> +	CSI_2_YUV422_8 = 0x1E,
> +	CSI_2_YUV422_10 = 0x1F,
> +	CSI_2_RGB444 = 0x20,
> +	CSI_2_RGB555 = 0x21,
> +	CSI_2_RGB565 = 0x22,
> +	CSI_2_RGB666 = 0x23,
> +	CSI_2_RGB888 = 0x24,
> +	CSI_2_RAW6 = 0x28,
> +	CSI_2_RAW7 = 0x29,
> +	CSI_2_RAW8 = 0x2A,
> +	CSI_2_RAW10 = 0x2B,
> +	CSI_2_RAW12 = 0x2C,
> +	CSI_2_RAW14 = 0x2D,
> +};
> +
> +/** @short Interrupt Masks */
> +enum interrupt_type {
> +	CSI2_INT_PHY_FATAL = 1 << 0,
> +	CSI2_INT_PKT_FATAL = 1 << 1,
> +	CSI2_INT_FRAME_FATAL = 1 << 2,
> +	CSI2_INT_PHY = 1 << 16,
> +	CSI2_INT_PKT = 1 << 17,
> +	CSI2_INT_LINE = 1 << 18,
> +	CSI2_INT_IPI = 1 << 19,
> +
> +};
> +
> +/** @short DWC MIPI CSI-2 output types*/
> +enum output_type {
> +	IPI_OUT = 0,
> +	IDI_OUT = 1,
> +	BOTH_OUT = 2
> +};
> +
> +/** @short IPI output types*/
> +enum ipi_output_type {
> +	CAMERA_TIMING = 0,
> +	AUTO_TIMING = 1
> +};
> +
> +/**
> + * @short Format template
> + */
> +struct mipi_fmt {
> +	const char *name;
> +	u32 code;
> +	u8 depth;
> +};
> +
> +struct csi_hw {
> +
> +	uint32_t num_lanes;
> +	uint32_t output_type;
> +
> +	/*IPI Info */
> +	uint32_t ipi_mode;
> +	uint32_t ipi_color_mode;
> +	uint32_t ipi_auto_flush;
> +	uint32_t virtual_ch;
> +
> +	uint32_t hsa;
> +	uint32_t hbp;
> +	uint32_t hsd;
> +	uint32_t htotal;
> +
> +	uint32_t vsa;
> +	uint32_t vbp;
> +	uint32_t vfp;
> +	uint32_t vactive;
> +};
> +
> +/**
> + * @short Structure to embed device driver information
> + */
> +struct mipi_csi_dev {
> +	struct v4l2_subdev sd;
> +	struct video_device vdev;
> +
> +	struct mutex lock;
> +	spinlock_t slock;
> +	struct media_pad pads[CSI_PADS_NUM];
> +	struct platform_device *pdev;
> +	u8 index;
> +
> +	/** Store current format */
> +	const struct mipi_fmt *fmt;
> +	struct v4l2_mbus_framefmt format;
> +
> +	/** Device Tree Information */
> +	void __iomem *base_address;
> +	uint32_t ctrl_irq_number;
> +
> +	struct csi_hw hw;
> +
> +	struct phy *phy;
> +};
> +
> +#endif				/* DW_MIPI_CSI */
> diff --git a/drivers/media/platform/dwc/plat_ipk.c b/drivers/media/platform/dwc/plat_ipk.c
> new file mode 100644
> index 0000000..48b9e36
> --- /dev/null
> +++ b/drivers/media/platform/dwc/plat_ipk.c
> @@ -0,0 +1,835 @@
> +/**
> + * DWC MIPI CSI-2 Host IPK platform device driver
> + *
> + * Based on Omnivision OV7670 Camera Driver
> + * Copyright (C) 2011 - 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + * Author: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation, either version 2 of the License,
> + * or (at your option) any later version.
> + */
> +
> +#include "plat_ipk.h"
> +#include "video_device.h"
> +#include "dw_mipi_csi.h"
> +#include <media/v4l2-of.h>
> +#include <media/v4l2-subdev.h>
> +
> +static int
> +__plat_ipk_pipeline_s_format(struct plat_ipk_media_pipeline *ep,
> +			     struct v4l2_subdev_format *fmt)
> +{
> +
> +	struct plat_ipk_pipeline *p = to_plat_ipk_pipeline(ep);
> +	static const u8 seq[IDX_MAX] = {IDX_SENSOR, IDX_CSI, IDX_VDEV};
> +
> +	fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	v4l2_subdev_call(p->subdevs[seq[IDX_CSI]], pad, set_fmt, NULL, fmt);
> +
> +	return 0;
> +}
> +
> +static void
> +plat_ipk_pipeline_prepare(struct plat_ipk_pipeline *p, struct media_entity *me)
> +{
> +	struct v4l2_subdev *sd;
> +	int i = 0;

unsigned int?

> +
> +	for (i = 0; i < IDX_MAX; i++)
> +		p->subdevs[i] = NULL;
> +
> +	while (1) {
> +		struct media_pad *pad = NULL;
> +
> +		for (i = 0; i < me->num_pads; i++) {
> +			struct media_pad *spad = &me->pads[i];
> +
> +			if (!(spad->flags & MEDIA_PAD_FL_SINK))
> +				continue;
> +
> +			pad = media_entity_remote_pad(spad);
> +			if (pad)
> +				break;
> +		}
> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> +			break;
> +
> +		sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> +		switch (sd->grp_id) {
> +		case GRP_ID_SENSOR:
> +			p->subdevs[IDX_SENSOR] = sd;
> +			break;
> +		case GRP_ID_CSI:
> +			p->subdevs[IDX_CSI] = sd;
> +			break;
> +		case GRP_ID_VIDEODEV:
> +			p->subdevs[IDX_VDEV] = sd;
> +			break;
> +		default:
> +			break;
> +		}
> +		me = &sd->entity;
> +		if (me->num_pads == 1)
> +			break;
> +	}
> +}
> +
> +static int __subdev_set_power(struct v4l2_subdev *sd, int on)
> +{
> +	int *use_count;
> +	int ret;
> +
> +	if (sd == NULL) {
> +		pr_info("null subdev\n");
> +		return -ENXIO;
> +	}
> +	use_count = &sd->entity.use_count;
> +	if (on && (*use_count)++ > 0)
> +		return 0;
> +	else if (!on && (*use_count == 0 || --(*use_count) > 0))
> +		return 0;
> +
> +	pr_debug("%d %s !\n", on, sd->entity.name);

Please do use dev_*() family of functions instead.

> +	ret = v4l2_subdev_call(sd, core, s_power, on);
> +
> +	return ret != -ENOIOCTLCMD ? ret : 0;
> +}
> +
> +static int plat_ipk_pipeline_s_power(struct plat_ipk_pipeline *p, bool on)
> +{
> +	static const u8 seq[IDX_MAX] = {IDX_CSI, IDX_SENSOR, IDX_VDEV};
> +	int i, ret = 0;
> +
> +	for (i = 0; i < IDX_MAX; i++) {
> +		unsigned int idx = seq[i];
> +
> +		if (p->subdevs[idx] == NULL)
> +			pr_info("No device registered on %d\n", idx);
> +		else {
> +			ret = __subdev_set_power(p->subdevs[idx], on);
> +			if (ret < 0 && ret != -ENXIO)
> +				goto error;
> +		}
> +	}
> +	return 0;
> +error:
> +	for (; i >= 0; i--) {
> +		unsigned int idx = seq[i];
> +
> +		__subdev_set_power(p->subdevs[idx], !on);
> +	}
> +	return ret;
> +}
> +
> +static int
> +__plat_ipk_pipeline_open(struct plat_ipk_media_pipeline *ep,
> +			 struct media_entity *me, bool prepare)
> +{
> +	struct plat_ipk_pipeline *p = to_plat_ipk_pipeline(ep);
> +	int ret;
> +
> +	if (WARN_ON(p == NULL || me == NULL))
> +		return -EINVAL;
> +
> +	if (prepare)
> +		plat_ipk_pipeline_prepare(p, me);
> +
> +	ret = plat_ipk_pipeline_s_power(p, 1);
> +	if (!ret)
> +		return 0;
> +
> +	return ret;
> +}
> +
> +static int __plat_ipk_pipeline_close(struct plat_ipk_media_pipeline *ep)
> +{
> +	struct plat_ipk_pipeline *p = to_plat_ipk_pipeline(ep);
> +	int ret;
> +
> +	ret = plat_ipk_pipeline_s_power(p, 0);
> +
> +	return ret == -ENXIO ? 0 : ret;
> +}
> +
> +static int
> +__plat_ipk_pipeline_s_stream(struct plat_ipk_media_pipeline *ep, bool on)
> +{
> +	static const u8 seq[IDX_MAX] = {IDX_SENSOR, IDX_CSI, IDX_VDEV};
> +	struct plat_ipk_pipeline *p = to_plat_ipk_pipeline(ep);
> +	int i, ret = 0;
> +
> +	for (i = 0; i < IDX_MAX; i++) {
> +		unsigned int idx = seq[i];
> +
> +		if (p->subdevs[idx] == NULL)
> +			pr_debug("No device registered on %d\n", idx);
> +		else {
> +			ret =
> +			    v4l2_subdev_call(p->subdevs[idx], video, s_stream,
> +					     on);
> +
> +			if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> +				goto error;
> +		}
> +	}
> +	return 0;
> +error:
> +	for (; i >= 0; i--) {
> +		unsigned int idx = seq[i];
> +
> +		v4l2_subdev_call(p->subdevs[idx], video, s_stream, !on);
> +	}
> +	return ret;
> +}
> +
> +static const struct plat_ipk_media_pipeline_ops plat_ipk_pipeline_ops = {
> +	.open = __plat_ipk_pipeline_open,
> +	.close = __plat_ipk_pipeline_close,
> +	.set_format = __plat_ipk_pipeline_s_format,
> +	.set_stream = __plat_ipk_pipeline_s_stream,
> +};
> +
> +static struct plat_ipk_media_pipeline *
> +plat_ipk_pipeline_create(struct plat_ipk_dev *plat_ipk)
> +{
> +	struct plat_ipk_pipeline *p;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return NULL;
> +
> +	list_add_tail(&p->list, &plat_ipk->pipelines);
> +
> +	p->ep.ops = &plat_ipk_pipeline_ops;
> +	return &p->ep;
> +}
> +
> +static void
> +plat_ipk_pipelines_free(struct plat_ipk_dev *plat_ipk)
> +{
> +	while (!list_empty(&plat_ipk->pipelines)) {
> +		struct plat_ipk_pipeline *p;
> +
> +		p = list_entry(plat_ipk->pipelines.next, typeof(*p), list);
> +		list_del(&p->list);
> +		kfree(p);
> +	}
> +}
> +
> +static int
> +plat_ipk_parse_port_node(struct plat_ipk_dev *plat_ipk,
> +			 struct device_node *port, unsigned int index)
> +{
> +	struct device_node *rem, *ep;
> +	struct v4l2_of_endpoint endpoint;
> +	struct plat_ipk_source_info *pd = &plat_ipk->sensor[index].pdata;
> +
> +	/* Assume here a port node can have only one endpoint node. */
> +	ep = of_get_next_child(port, NULL);
> +	if (!ep)
> +		return 0;
> +
> +	v4l2_of_parse_endpoint(ep, &endpoint);
> +	if (WARN_ON(endpoint.base.port == 0) || index >= PLAT_MAX_SENSORS)
> +		return -EINVAL;
> +
> +	pd->mux_id = endpoint.base.port - 1;
> +
> +	rem = of_graph_get_remote_port_parent(ep);
> +	of_node_put(ep);
> +	if (rem == NULL) {
> +		v4l2_info(&plat_ipk->v4l2_dev,
> +			  "Remote device at %s not found\n", ep->full_name);
> +		return 0;
> +	}
> +
> +	if (WARN_ON(index >= ARRAY_SIZE(plat_ipk->sensor)))
> +		return -EINVAL;
> +
> +	plat_ipk->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_OF;
> +	plat_ipk->sensor[index].asd.match.of.node = rem;
> +	plat_ipk->async_subdevs[index] = &plat_ipk->sensor[index].asd;
> +
> +	plat_ipk->num_sensors++;
> +
> +	of_node_put(rem);
> +	return 0;
> +}
> +
> +
> +static int plat_ipk_register_sensor_entities(struct plat_ipk_dev *plat_ipk)
> +{
> +	struct device_node *parent = plat_ipk->pdev->dev.of_node;
> +	struct device_node *node;
> +	int index = 0;
> +	int ret;
> +
> +	plat_ipk->num_sensors = 0;
> +
> +	for_each_available_child_of_node(parent, node) {
> +		struct device_node *port;
> +
> +		if (of_node_cmp(node->name, "csi2"))
> +			continue;
> +		port = of_get_next_child(node, NULL);
> +		if (!port)
> +			continue;
> +
> +		ret = plat_ipk_parse_port_node(plat_ipk, port, index);
> +		if (ret < 0)
> +			return ret;
> +		index++;
> +	}
> +	return 0;
> +}
> +
> +static int
> +__of_get_port_id(struct device_node *np)
> +{
> +	u32 reg = 0;
> +
> +	np = of_get_child_by_name(np, "port");
> +	if (!np)
> +		return -EINVAL;
> +	of_property_read_u32(np, "reg", &reg);
> +
> +	return reg - 1;
> +}
> +
> +static int register_videodev_entity(struct plat_ipk_dev *plat_ipk,
> +			 struct video_device_dev *vid_dev)
> +{
> +	struct v4l2_subdev *sd;
> +	struct plat_ipk_media_pipeline *ep;
> +	int ret;
> +
> +	sd = &vid_dev->subdev;
> +	sd->grp_id = GRP_ID_VIDEODEV;
> +
> +	ep = plat_ipk_pipeline_create(plat_ipk);
> +	if (!ep)
> +		return -ENOMEM;
> +
> +	v4l2_set_subdev_hostdata(sd, ep);
> +
> +	ret = v4l2_device_register_subdev(&plat_ipk->v4l2_dev, sd);
> +	if (!ret)
> +		plat_ipk->vid_dev = vid_dev;
> +	else
> +		v4l2_err(&plat_ipk->v4l2_dev,
> +			 "Failed to register Video Device\n");
> +	return ret;
> +}
> +
> +static int register_mipi_csi_entity(struct plat_ipk_dev *plat_ipk,
> +			 struct platform_device *pdev, struct v4l2_subdev *sd)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	int id, ret;
> +
> +	id = node ? __of_get_port_id(node) : max(0, pdev->id);
> +
> +	if (WARN_ON(id < 0 || id >= CSI_MAX_ENTITIES))
> +		return -ENOENT;
> +
> +	if (WARN_ON(plat_ipk->mipi_csi[id].sd))
> +		return -EBUSY;
> +
> +	sd->grp_id = GRP_ID_CSI;
> +	ret = v4l2_device_register_subdev(&plat_ipk->v4l2_dev, sd);
> +
> +	if (!ret)
> +		plat_ipk->mipi_csi[id].sd = sd;
> +	else
> +		v4l2_err(&plat_ipk->v4l2_dev,
> +			 "Failed to register MIPI-CSI.%d (%d)\n", id, ret);
> +	return ret;
> +}
> +
> +static int plat_ipk_register_platform_entity(struct plat_ipk_dev *plat_ipk,
> +				struct platform_device *pdev, int plat_entity)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret = -EPROBE_DEFER;
> +	void *drvdata;
> +
> +

Extra newline.

> +	device_lock(dev);
> +	if (!dev->driver || !try_module_get(dev->driver->owner))
> +		goto dev_unlock;
> +
> +	drvdata = dev_get_drvdata(dev);
> +
> +	if (drvdata) {
> +		switch (plat_entity) {
> +		case IDX_VDEV:
> +			ret = register_videodev_entity(plat_ipk, drvdata);
> +			break;
> +		case IDX_CSI:
> +			ret = register_mipi_csi_entity(plat_ipk, pdev, drvdata);
> +			break;
> +		default:
> +			ret = -ENODEV;
> +		}
> +	} else
> +		dev_err(&plat_ipk->pdev->dev, "%s no drvdata\n", dev_name(dev));
> +	module_put(dev->driver->owner);
> +dev_unlock:
> +	device_unlock(dev);
> +	if (ret == -EPROBE_DEFER)
> +		dev_info(&plat_ipk->pdev->dev,
> +			 "deferring %s device registration\n", dev_name(dev));
> +	else if (ret < 0)
> +		dev_err(&plat_ipk->pdev->dev,
> +			"%s device registration failed (%d)\n", dev_name(dev),
> +			ret);
> +	return ret;
> +}
> +
> +static int
> +plat_ipk_register_platform_entities(struct plat_ipk_dev *plat_ipk,
> +				    struct device_node *parent)
> +{
> +	struct device_node *node;
> +	int ret = 0;
> +
> +	for_each_available_child_of_node(parent, node) {
> +		struct platform_device *pdev;
> +		int plat_entity = -1;
> +
> +		pdev = of_find_device_by_node(node);
> +		if (!pdev)
> +			continue;
> +
> +		if (!strcmp(node->name, VIDEODEV_OF_NODE_NAME))
> +			plat_entity = IDX_VDEV;
> +		else if (!strcmp(node->name, CSI_OF_NODE_NAME))
> +			plat_entity = IDX_CSI;
> +
> +		if (plat_entity >= 0)
> +			ret = plat_ipk_register_platform_entity(plat_ipk, pdev,
> +								plat_entity);
> +		put_device(&pdev->dev);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static void
> +plat_ipk_unregister_entities(struct plat_ipk_dev *plat_ipk)
> +{
> +	int i;
> +	struct video_device_dev *dev = plat_ipk->vid_dev;
> +
> +	if (dev == NULL)
> +		return;
> +	v4l2_device_unregister_subdev(&dev->subdev);
> +	dev->ve.pipe = NULL;
> +	plat_ipk->vid_dev = NULL;
> +
> +	for (i = 0; i < CSI_MAX_ENTITIES; i++) {
> +		if (plat_ipk->mipi_csi[i].sd == NULL)
> +			continue;
> +		v4l2_device_unregister_subdev(plat_ipk->mipi_csi[i].sd);
> +		plat_ipk->mipi_csi[i].sd = NULL;
> +	}
> +
> +	v4l2_info(&plat_ipk->v4l2_dev, "Unregistered all entities\n");
> +}
> +
> +static int
> +__plat_ipk_create_videodev_sink_links(struct plat_ipk_dev *plat_ipk,
> +				      struct media_entity *source,
> +				      int pad)
> +{
> +	struct media_entity *sink;
> +	int ret = 0;
> +
> +	if (!plat_ipk->vid_dev)
> +		return 0;
> +
> +	sink = &plat_ipk->vid_dev->subdev.entity;
> +	ret = media_create_pad_link(source, pad, sink,
> +				    CSI_PAD_SOURCE, MEDIA_LNK_FL_ENABLED);
> +	if (ret)
> +		return ret;
> +
> +	ret = media_entity_call(sink, link_setup, &sink->pads[0],
> +				&source->pads[pad], 0);

What's the purpose of calling link_setup() here?

> +	if (ret)
> +		return 0;
> +
> +	v4l2_info(&plat_ipk->v4l2_dev, "created link [%s] -> [%s]\n",
> +		  source->name, sink->name);
> +
> +	return 0;
> +}
> +
> +
> +static int
> +__plat_ipk_create_videodev_source_links(struct plat_ipk_dev *plat_ipk)
> +{
> +	struct media_entity *source, *sink;
> +	int ret = 0;
> +
> +	struct video_device_dev *vid_dev = plat_ipk->vid_dev;
> +
> +	if (vid_dev == NULL)
> +		return -ENODEV;
> +
> +	source = &vid_dev->subdev.entity;
> +	sink = &vid_dev->ve.vdev.entity;
> +
> +	ret = media_create_pad_link(source, VIDEO_DEV_SD_PAD_SOURCE_DMA,
> +				    sink, 0, MEDIA_LNK_FL_ENABLED);
> +
> +	v4l2_info(&plat_ipk->v4l2_dev, "created link [%s] -> [%s]\n",
> +		  source->name, sink->name);
> +	return ret;
> +}
> +
> +static int
> +plat_ipk_create_links(struct plat_ipk_dev *plat_ipk)
> +{
> +	struct v4l2_subdev *csi_sensor[CSI_MAX_ENTITIES] = { NULL };
> +	struct v4l2_subdev *sensor, *csi;
> +	struct media_entity *source;
> +	struct plat_ipk_source_info *pdata;
> +	int i, pad, ret = 0;
> +
> +	for (i = 0; i < plat_ipk->num_sensors; i++) {
> +		if (plat_ipk->sensor[i].subdev == NULL)
> +			continue;
> +
> +		sensor = plat_ipk->sensor[i].subdev;
> +		pdata = v4l2_get_subdev_hostdata(sensor);
> +		if (!pdata)
> +			continue;
> +
> +		source = NULL;
> +
> +		csi = plat_ipk->mipi_csi[pdata->mux_id].sd;
> +		if (WARN(csi == NULL, "MIPI-CSI interface specified but	dw-mipi-csi module is not loaded!\n"))
> +			return -EINVAL;
> +
> +		pad = sensor->entity.num_pads - 1;
> +		ret = media_create_pad_link(&sensor->entity, pad,
> +					    &csi->entity, CSI_PAD_SINK,
> +					    MEDIA_LNK_FL_IMMUTABLE |
> +					    MEDIA_LNK_FL_ENABLED);
> +
> +		if (ret)
> +			return ret;
> +		v4l2_info(&plat_ipk->v4l2_dev, "created link [%s] -> [%s]\n",
> +			  sensor->entity.name, csi->entity.name);

dev_dbg() ?

> +
> +		csi_sensor[pdata->mux_id] = sensor;
> +	}
> +
> +	for (i = 0; i < CSI_MAX_ENTITIES; i++) {
> +		if (plat_ipk->mipi_csi[i].sd == NULL) {
> +			pr_info("no link\n");
> +			continue;
> +		}
> +
> +		source = &plat_ipk->mipi_csi[i].sd->entity;
> +		pad = VIDEO_DEV_SD_PAD_SINK_CSI;
> +
> +		ret = __plat_ipk_create_videodev_sink_links(plat_ipk, source,
> +								pad);
> +	}
> +
> +	ret = __plat_ipk_create_videodev_source_links(plat_ipk);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret;
> +}
> +
> +static int __plat_ipk_modify_pipeline(struct media_entity *entity, bool enable)
> +{
> +	struct plat_ipk_video_entity *ve;
> +	struct plat_ipk_pipeline *p;
> +	struct video_device *vdev;
> +	int ret;
> +
> +	vdev = media_entity_to_video_device(entity);
> +
> +	if (vdev->entity.use_count == 0)
> +		return 0;
> +
> +	ve = vdev_to_plat_ipk_video_entity(vdev);
> +	p = to_plat_ipk_pipeline(ve->pipe);
> +
> +	if (enable)
> +		ret = __plat_ipk_pipeline_open(ve->pipe, entity, true);
> +	else
> +		ret = __plat_ipk_pipeline_close(ve->pipe);
> +
> +	if (ret == 0 && !enable)
> +		memset(p->subdevs, 0, sizeof(p->subdevs));
> +
> +	return ret;
> +}
> +
> +
> +static int
> +__plat_ipk_modify_pipelines(struct media_entity *entity, bool enable,
> +			    struct media_entity_graph *graph)
> +{
> +	struct media_entity *entity_err = entity;
> +	int ret;
> +
> +	media_entity_graph_walk_start(graph, entity);
> +
> +	while ((entity = media_entity_graph_walk_next(graph))) {
> +		if (!is_media_entity_v4l2_video_device(entity))
> +			continue;
> +
> +		ret = __plat_ipk_modify_pipeline(entity, enable);
> +
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	media_entity_graph_walk_start(graph, entity_err);
> +
> +	while ((entity_err = media_entity_graph_walk_next(graph))) {
> +		if (!is_media_entity_v4l2_video_device(entity_err))
> +			continue;
> +
> +		__plat_ipk_modify_pipeline(entity_err, !enable);
> +
> +		if (entity_err == entity)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +plat_ipk_link_notify(struct media_link *link, unsigned int flags,
> +		     unsigned int notification)
> +{
> +	struct media_entity_graph *graph =
> +	    &container_of(link->graph_obj.mdev, struct plat_ipk_dev,
> +			  media_dev)->link_setup_graph;
> +	struct media_entity *sink = link->sink->entity;
> +	int ret = 0;
> +
> +	pr_debug("Link notify\n");

This probably made sense at development time but should be removed now.

> +
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH) {
> +		ret = media_entity_graph_walk_init(graph, link->graph_obj.mdev);
> +		if (ret)
> +			return ret;
> +		if (!(flags & MEDIA_LNK_FL_ENABLED))
> +			ret = __plat_ipk_modify_pipelines(sink, false, graph);
> +
> +	} else if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH) {
> +		if (link->flags & MEDIA_LNK_FL_ENABLED)
> +			ret = __plat_ipk_modify_pipelines(sink, true, graph);
> +		media_entity_graph_walk_cleanup(graph);
> +	}
> +
> +	return ret ? -EPIPE : 0;
> +}

Missing newline.

> +static const struct media_device_ops plat_ipk_media_ops = {
> +	.link_notify = plat_ipk_link_notify,
> +};
> +
> +
> +static int
> +subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> +		      struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd)
> +{
> +	struct plat_ipk_dev *plat_ipk = notifier_to_plat_ipk(notifier);
> +	struct plat_ipk_sensor_info *si = NULL;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(plat_ipk->sensor); i++)
> +		if (plat_ipk->sensor[i].asd.match.of.node ==
> +		    subdev->dev->of_node)
> +			si = &plat_ipk->sensor[i];
> +
> +	if (si == NULL)
> +		return -EINVAL;
> +
> +	v4l2_set_subdev_hostdata(subdev, &si->pdata);
> +
> +	subdev->grp_id = GRP_ID_SENSOR;
> +
> +	si->subdev = subdev;
> +
> +	v4l2_info(&plat_ipk->v4l2_dev, "Registered sensor subdevice: %s (%d)\n",
> +		  subdev->name, plat_ipk->num_sensors);
> +
> +	plat_ipk->num_sensors++;
> +
> +	return 0;
> +}
> +
> +static int
> +subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct plat_ipk_dev *plat_ipk = notifier_to_plat_ipk(notifier);
> +	int ret;
> +
> +	mutex_lock(&plat_ipk->media_dev.graph_mutex);
> +
> +	ret = plat_ipk_create_links(plat_ipk);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	ret = v4l2_device_register_subdev_nodes(&plat_ipk->v4l2_dev);
> +unlock:
> +	mutex_unlock(&plat_ipk->media_dev.graph_mutex);
> +	if (ret < 0)
> +		return ret;
> +
> +	return media_device_register(&plat_ipk->media_dev);
> +}
> +
> +static const struct of_device_id plat_ipk_of_match[];

This is redundant.

> +
> +static int plat_ipk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_device *v4l2_dev;
> +	struct plat_ipk_dev *plat_ipk;
> +	int ret;
> +
> +	dev_info(dev, "Installing DW MIPI CSI-2 IPK Platform module\n");
> +
> +	plat_ipk = devm_kzalloc(dev, sizeof(*plat_ipk), GFP_KERNEL);
> +	if (!plat_ipk)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&plat_ipk->slock);
> +	INIT_LIST_HEAD(&plat_ipk->pipelines);
> +	plat_ipk->pdev = pdev;
> +
> +	strlcpy(plat_ipk->media_dev.model, "SNPS IPK Platform",
> +		sizeof(plat_ipk->media_dev.model));
> +	plat_ipk->media_dev.ops = &plat_ipk_media_ops;
> +	plat_ipk->media_dev.dev = dev;
> +
> +	v4l2_dev = &plat_ipk->v4l2_dev;
> +	v4l2_dev->mdev = &plat_ipk->media_dev;
> +	strlcpy(v4l2_dev->name, "plat-ipk", sizeof(v4l2_dev->name));
> +
> +	media_device_init(&plat_ipk->media_dev);
> +
> +	ret = v4l2_device_register(dev, &plat_ipk->v4l2_dev);
> +	if (ret < 0) {
> +		v4l2_err(v4l2_dev, "Failed to register v4l2_device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, plat_ipk);
> +
> +	ret = plat_ipk_register_platform_entities(plat_ipk, dev->of_node);
> +	if (ret)
> +		goto err_m_ent;
> +
> +	ret = plat_ipk_register_sensor_entities(plat_ipk);
> +	if (ret)
> +		goto err_m_ent;
> +
> +	if (plat_ipk->num_sensors > 0) {
> +		plat_ipk->subdev_notifier.subdevs = plat_ipk->async_subdevs;
> +		plat_ipk->subdev_notifier.num_subdevs = plat_ipk->num_sensors;
> +		plat_ipk->subdev_notifier.bound = subdev_notifier_bound;
> +		plat_ipk->subdev_notifier.complete = subdev_notifier_complete;
> +		plat_ipk->num_sensors = 0;
> +
> +		ret = v4l2_async_notifier_register(&plat_ipk->v4l2_dev,
> +						   &plat_ipk->subdev_notifier);
> +		if (ret)
> +			goto err_m_ent;
> +	}
> +
> +	return 0;
> +
> +err_m_ent:
> +	plat_ipk_unregister_entities(plat_ipk);
> +	media_device_unregister(&plat_ipk->media_dev);
> +	media_device_cleanup(&plat_ipk->media_dev);
> +	v4l2_device_unregister(&plat_ipk->v4l2_dev);
> +	return ret;
> +}
> +
> +static int plat_ipk_remove(struct platform_device *pdev)
> +{
> +	struct plat_ipk_dev *dev = platform_get_drvdata(pdev);
> +
> +	if (!dev)
> +		return 0;

Wouldn't !dev here require a driver bug? If probe() succeeds, then dev is
non-zero, right?

> +
> +	v4l2_async_notifier_unregister(&dev->subdev_notifier);
> +
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +	plat_ipk_unregister_entities(dev);
> +	plat_ipk_pipelines_free(dev);
> +	media_device_unregister(&dev->media_dev);
> +	media_device_cleanup(&dev->media_dev);
> +
> +	dev_info(&pdev->dev, "Driver removed\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * @short of_device_id structure
> + */
> +static const struct of_device_id plat_ipk_of_match[] = {
> +	{.compatible = "snps,plat-ipk"},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, plat_ipk_of_match);
> +
> +/**
> + * @short Platform driver structure
> + */
> +static struct platform_driver plat_ipk_pdrv = {
> +	.remove = plat_ipk_remove,
> +	.probe = plat_ipk_probe,
> +	.driver = {
> +		   .name = "snps,plat-ipk",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = plat_ipk_of_match,
> +		   },
> +};
> +
> +static int __init
> +plat_ipk_init(void)
> +{
> +	request_module("dw-mipi-csi");

Why do you need this?

> +
> +	return platform_driver_register(&plat_ipk_pdrv);
> +}
> +
> +static void __exit
> +plat_ipk_exit(void)
> +{
> +	platform_driver_unregister(&plat_ipk_pdrv);
> +}
> +
> +module_init(plat_ipk_init);
> +module_exit(plat_ipk_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Ramiro Oliveira <roliveir@synopsys.com>");
> +MODULE_DESCRIPTION("Platform driver for MIPI CSI-2 Host IPK");
> diff --git a/drivers/media/platform/dwc/plat_ipk.h b/drivers/media/platform/dwc/plat_ipk.h
> new file mode 100644
> index 0000000..ef569eb
> --- /dev/null
> +++ b/drivers/media/platform/dwc/plat_ipk.h
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef PLAT_IPK_H_
> +#define PLAT_IPK_H_
> +
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/list.h>
> +#include <linux/string.h>
> +#include <media/v4l2-device.h>
> +#include <linux/videodev2.h>
> +#include <media/media-entity.h>
> +
> +#include "plat_ipk_video.h"
> +
> +#define VIDEODEV_OF_NODE_NAME	"video-device"
> +#define CSI_OF_NODE_NAME	"csi2"
> +
> +enum plat_ipk_subdev_index {
> +	IDX_SENSOR,
> +	IDX_CSI,
> +	IDX_VDEV,
> +	IDX_MAX,
> +};
> +
> +struct plat_ipk_sensor_info {
> +	struct plat_ipk_source_info pdata;
> +	struct v4l2_async_subdev asd;
> +	struct v4l2_subdev *subdev;
> +	struct mipi_csi_dev *host;
> +};
> +
> +struct plat_ipk_pipeline {
> +	struct plat_ipk_media_pipeline ep;
> +	struct list_head list;
> +	struct media_entity *vdev_entity;
> +	struct v4l2_subdev *subdevs[IDX_MAX];
> +};
> +
> +#define to_plat_ipk_pipeline(_ep)\
> +	 container_of(_ep, struct plat_ipk_pipeline, ep)
> +
> +struct mipi_csi_info {
> +	struct v4l2_subdev *sd;
> +	int id;
> +};
> +
> +/**
> + * @short Structure to embed device driver information
> + */
> +struct plat_ipk_dev {
> +	struct mipi_csi_info		mipi_csi[CSI_MAX_ENTITIES];
> +	struct video_device_dev		*vid_dev;
> +	struct device			*dev;
> +	struct media_device		media_dev;
> +	struct v4l2_device		v4l2_dev;
> +	struct platform_device		*pdev;
> +	struct plat_ipk_sensor_info	sensor[PLAT_MAX_SENSORS];
> +	struct v4l2_async_notifier	subdev_notifier;
> +	struct v4l2_async_subdev	*async_subdevs[PLAT_MAX_SENSORS];
> +	spinlock_t			slock;
> +	struct list_head		pipelines;
> +	int				num_sensors;
> +	struct media_entity_graph	link_setup_graph;
> +};
> +
> +static inline struct plat_ipk_dev *
> +entity_to_plat_ipk_mdev(struct media_entity *me)
> +{
> +	return me->graph_obj.mdev == NULL ? NULL :
> +	    container_of(me->graph_obj.mdev, struct plat_ipk_dev, media_dev);
> +}
> +
> +static inline struct plat_ipk_dev *
> +notifier_to_plat_ipk(struct v4l2_async_notifier *n)
> +{
> +	return container_of(n, struct plat_ipk_dev, subdev_notifier);
> +}
> +
> +static inline void
> +plat_ipk_graph_unlock(struct plat_ipk_video_entity *ve)
> +{
> +	mutex_unlock(&ve->vdev.entity.graph_obj.mdev->graph_mutex);
> +}
> +
> +#endif				/* PLAT_IPK_H_ */
> diff --git a/drivers/media/platform/dwc/plat_ipk_video.h b/drivers/media/platform/dwc/plat_ipk_video.h
> new file mode 100644
> index 0000000..6bfc9f8
> --- /dev/null
> +++ b/drivers/media/platform/dwc/plat_ipk_video.h
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef PLAT_IPK_INCLUDES_H_
> +#define PLAT_IPK_INCLUDES_H_
> +
> +#include <media/media-entity.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
> +
> +/*
> + * The subdevices' group IDs.
> + */
> +
> +#define MAX_WIDTH	3280
> +#define MAX_HEIGHT	1852
> +
> +#define MIN_WIDTH	640
> +#define MIN_HEIGHT	480
> +
> +#define GRP_ID_SENSOR		(10)
> +#define GRP_ID_CSI		(20)
> +#define GRP_ID_VIDEODEV		(30)
> +
> +#define CSI_MAX_ENTITIES	1
> +#define PLAT_MAX_SENSORS	1
> +
> +enum video_dev_pads {
> +	VIDEO_DEV_SD_PAD_SINK_CSI = 0,
> +	VIDEO_DEV_SD_PAD_SOURCE_DMA = 1,
> +	VIDEO_DEV_SD_PADS_NUM = 2,
> +};
> +
> +enum mipi_csi_pads {
> +	CSI_PAD_SINK = 0,
> +	CSI_PAD_SOURCE = 1,
> +	CSI_PADS_NUM = 2,
> +};
> +
> +struct plat_ipk_source_info {
> +	u16 flags;
> +	u16 mux_id;
> +};
> +
> +struct plat_ipk_fmt {
> +	u32 mbus_code;
> +	char *name;
> +	u32 fourcc;
> +	u8 depth;
> +};
> +
> +struct plat_ipk_media_pipeline;
> +
> +/*
> + * Media pipeline operations to be called from within a video node,  i.e. the
> + * last entity within the pipeline. Implemented by related media device driver.
> + */
> +struct plat_ipk_media_pipeline_ops {
> +	int (*prepare)(struct plat_ipk_media_pipeline *p,
> +			struct media_entity *me);
> +	int (*unprepare)(struct plat_ipk_media_pipeline *p);
> +	int (*open)(struct plat_ipk_media_pipeline *p,
> +			struct media_entity *me, bool resume);
> +	int (*close)(struct plat_ipk_media_pipeline *p);
> +	int (*set_stream)(struct plat_ipk_media_pipeline *p, bool state);
> +	int (*set_format)(struct plat_ipk_media_pipeline *p,
> +			struct v4l2_subdev_format *fmt);
> +};
> +
> +struct plat_ipk_video_entity {
> +	struct video_device vdev;
> +	struct plat_ipk_media_pipeline *pipe;
> +};
> +
> +struct plat_ipk_media_pipeline {
> +	struct media_pipeline mp;
> +	const struct plat_ipk_media_pipeline_ops *ops;
> +};
> +
> +static inline struct plat_ipk_video_entity *
> +vdev_to_plat_ipk_video_entity(struct video_device *vdev)
> +{
> +	return container_of(vdev, struct plat_ipk_video_entity, vdev);
> +}
> +
> +#define plat_ipk_pipeline_call(ent, op, args...)\
> +	(!(ent) ? -ENOENT : (((ent)->pipe->ops && (ent)->pipe->ops->op) ? \
> +	(ent)->pipe->ops->op(((ent)->pipe), ##args) : -ENOIOCTLCMD))	  \
> +
> +
> +#endif				/* PLAT_IPK_INCLUDES_H_ */
> diff --git a/drivers/media/platform/dwc/video_device.c b/drivers/media/platform/dwc/video_device.c
> new file mode 100644
> index 0000000..d827426
> --- /dev/null
> +++ b/drivers/media/platform/dwc/video_device.c
> @@ -0,0 +1,741 @@
> +/*
> + * DWC MIPI CSI-2 Host IPK video device device driver
> + *
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + * Author: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation, either version 2 of the License,
> + * or (at your option) any later version.
> + */
> +
> +#include "video_device.h"
> +
> +const struct plat_ipk_fmt vid_dev_formats[] = {
> +	{
> +		.name = "RGB888",
> +		.fourcc = V4L2_PIX_FMT_BGR24,
> +		.depth = 24,
> +		.mbus_code = MEDIA_BUS_FMT_RGB888_2X12_LE,
> +	}, {
> +		.name = "RGB565",
> +		.fourcc = V4L2_PIX_FMT_RGB565,
> +		.depth = 16,
> +		.mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> +	},
> +};
> +
> +const struct plat_ipk_fmt *
> +vid_dev_find_format(struct v4l2_format *f, int index)
> +{
> +	const struct plat_ipk_fmt *fmt = NULL;
> +	unsigned int i;
> +
> +	if (index >= (int) ARRAY_SIZE(vid_dev_formats))
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vid_dev_formats); ++i) {
> +		fmt = &vid_dev_formats[i];
> +		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +			return fmt;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Video node ioctl operations
> + */
> +static int
> +vidioc_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
> +{
> +	struct video_device_dev *vid_dev = video_drvdata(file);
> +
> +	strlcpy(cap->driver, VIDEO_DEVICE_NAME, sizeof(cap->driver));
> +	strlcpy(cap->card, VIDEO_DEVICE_NAME, sizeof(cap->card));
> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> +		 dev_name(&vid_dev->pdev->dev));
> +
> +	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +	return 0;
> +}
> +
> +int
> +vidioc_enum_fmt_vid_cap(struct file *file, void *priv, struct v4l2_fmtdesc *f)
> +{
> +	const struct plat_ipk_fmt *p_fmt;
> +
> +	if (f->index >= ARRAY_SIZE(vid_dev_formats))
> +		return -EINVAL;
> +
> +	p_fmt = &vid_dev_formats[f->index];
> +
> +	strlcpy(f->description, p_fmt->name, sizeof(f->description));
> +	f->pixelformat = p_fmt->fourcc;
> +
> +	return 0;
> +}
> +
> +int vidioc_g_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f)
> +{
> +	struct video_device_dev *dev = video_drvdata(file);
> +
> +	memcpy(&f->fmt.pix, &dev->format.fmt.pix,
> +	       sizeof(struct v4l2_pix_format));
> +
> +	return 0;
> +}
> +
> +static int
> +vidioc_try_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f)
> +{
> +	const struct plat_ipk_fmt *fmt;
> +
> +	fmt = vid_dev_find_format(f, -1);
> +	if (!fmt) {
> +		f->fmt.pix.pixelformat = V4L2_PIX_FMT_RGB565;
> +		fmt = vid_dev_find_format(f, -1);
> +	}
> +
> +	f->fmt.pix.field = V4L2_FIELD_NONE;
> +	v4l_bound_align_image(&f->fmt.pix.width, 48, MAX_WIDTH, 2,
> +			      &f->fmt.pix.height, 32, MAX_HEIGHT, 0, 0);
> +
> +	f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3;
> +	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> +	return 0;
> +}
> +
> +int vidioc_s_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f)
> +{
> +	struct video_device_dev *dev = video_drvdata(file);
> +	int ret;
> +	struct v4l2_subdev_format fmt;
> +
> +	if (vb2_is_busy(&dev->vb_queue))
> +		return -EBUSY;
> +
> +	ret = vidioc_try_fmt_vid_cap(file, dev, f);
> +	if (ret)
> +		return ret;
> +
> +	dev->fmt = vid_dev_find_format(f, -1);
> +	pixel_format(dev) = f->fmt.pix.pixelformat;
> +	width(dev) = f->fmt.pix.width;
> +	height(dev) = f->fmt.pix.height;
> +	bytes_per_line(dev) = width(dev) * dev->fmt->depth / 8;
> +	size_image(dev) = height(dev) * bytes_per_line(dev);
> +
> +	fmt.format.colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt.format.code = dev->fmt->mbus_code;
> +
> +	fmt.format.width = width(dev);
> +	fmt.format.height = height(dev);
> +
> +	ret = plat_ipk_pipeline_call(&dev->ve, set_format, &fmt);
> +
> +	return 0;
> +}
> +
> +int vidioc_enum_framesizes(struct file *file, void *fh,
> +		       struct v4l2_frmsizeenum *fsize)
> +{
> +	static const struct v4l2_frmsize_stepwise sizes = {
> +		48, MAX_WIDTH, 4,
> +		32, MAX_HEIGHT, 1
> +	};
> +	int i;
> +
> +	if (fsize->index)
> +		return -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(vid_dev_formats); i++)
> +		if (vid_dev_formats[i].fourcc == fsize->pixel_format)
> +			break;
> +	if (i == ARRAY_SIZE(vid_dev_formats))
> +		return -EINVAL;
> +	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> +	fsize->stepwise = sizes;
> +	return 0;
> +}
> +
> +int vidioc_enum_input(struct file *file, void *priv, struct v4l2_input *input)
> +{
> +	if (input->index != 0)
> +		return -EINVAL;
> +
> +	input->type = V4L2_INPUT_TYPE_CAMERA;
> +	input->std = V4L2_STD_ALL;	/* Not sure what should go here */
> +	strcpy(input->name, "Camera");
> +	return 0;
> +}
> +
> +int vidioc_g_input(struct file *file, void *priv, unsigned int *i)
> +{
> +	*i = 0;
> +	return 0;
> +}
> +
> +int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> +{
> +	if (i != 0)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +int vidioc_g_std(struct file *file, void *fh, v4l2_std_id *norm)
> +{
> +	*norm = V4L2_STD_NTSC_M;
> +	return 0;
> +}
> +
> +int vidioc_s_std(struct file *file, void *fh, v4l2_std_id a)
> +{
> +	return 0;
> +}
> +
> +static int
> +vid_dev_pipeline_validate(struct video_device_dev *vid_dev)
> +{
> +	return 0;
> +}
> +
> +static int
> +vid_dev_streamon(struct file *file, void *priv, enum v4l2_buf_type type)
> +{
> +	struct video_device_dev *vid_dev = video_drvdata(file);
> +	struct media_entity *entity = &vid_dev->ve.vdev.entity;
> +	int ret;
> +
> +	ret = media_entity_pipeline_start(entity, &vid_dev->ve.pipe->mp);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = vid_dev_pipeline_validate(vid_dev);
> +	if (ret < 0)
> +		goto err_p_stop;
> +
> +	vb2_ioctl_streamon(file, priv, type);
> +	if (!ret)
> +		return ret;
> +
> +err_p_stop:
> +	media_entity_pipeline_stop(entity);
> +	return 0;
> +}
> +
> +static int
> +vid_dev_streamoff(struct file *file, void *priv, enum v4l2_buf_type type)
> +{
> +	struct video_device_dev *vid_dev = video_drvdata(file);
> +	int ret;
> +
> +	ret = vb2_ioctl_streamoff(file, priv, type);
> +	if (ret < 0)
> +		return ret;
> +
> +	media_entity_pipeline_stop(&vid_dev->ve.vdev.entity);
> +	return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops vid_dev_ioctl_ops = {
> +	.vidioc_querycap = vidioc_querycap,
> +	.vidioc_enum_fmt_vid_cap = vidioc_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap = vidioc_g_fmt_vid_cap,
> +	.vidioc_s_fmt_vid_cap = vidioc_s_fmt_vid_cap,
> +	.vidioc_enum_framesizes = vidioc_enum_framesizes,
> +	.vidioc_enum_input = vidioc_enum_input,
> +	.vidioc_g_input = vidioc_g_input,
> +	.vidioc_s_input = vidioc_s_input,
> +
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_streamon = vid_dev_streamon,
> +	.vidioc_streamoff = vid_dev_streamoff,
> +};
> +
> +/* Capture subdev media entity operations */
> +static int
> +vid_dev_link_setup(struct media_entity *entity,
> +		   const struct media_pad *local,
> +		   const struct media_pad *remote, u32 flags)
> +{
> +	return 0;
> +}
> +
> +static const struct media_entity_operations vid_dev_subdev_media_ops = {
> +	.link_setup = vid_dev_link_setup,
> +};
> +
> +static int
> +vid_dev_open(struct file *file)
> +{
> +	struct video_device_dev *vid_dev = video_drvdata(file);
> +	struct media_entity *me = &vid_dev->ve.vdev.entity;
> +	int ret;
> +
> +	mutex_lock(&vid_dev->lock);
> +
> +	ret = v4l2_fh_open(file);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	if (!v4l2_fh_is_singular_file(file))
> +		goto unlock;
> +
> +	mutex_lock(&me->graph_obj.mdev->graph_mutex);
> +
> +	ret = plat_ipk_pipeline_call(&vid_dev->ve, open, me, true);
> +	if (ret == 0)
> +		me->use_count++;
> +
> +	mutex_unlock(&me->graph_obj.mdev->graph_mutex);
> +
> +	if (!ret)
> +		goto unlock;
> +
> +	v4l2_fh_release(file);
> +unlock:
> +	mutex_unlock(&vid_dev->lock);
> +	return ret;
> +}
> +
> +static int
> +vid_dev_release(struct file *file)
> +{
> +	struct video_device_dev *vid_dev = video_drvdata(file);
> +	struct media_entity *entity = &vid_dev->ve.vdev.entity;
> +
> +	mutex_lock(&vid_dev->lock);
> +
> +	if (v4l2_fh_is_singular_file(file)) {
> +		plat_ipk_pipeline_call(&vid_dev->ve, close);
> +		mutex_lock(&entity->graph_obj.mdev->graph_mutex);
> +		entity->use_count--;
> +		mutex_unlock(&entity->graph_obj.mdev->graph_mutex);
> +	}
> +
> +	_vb2_fop_release(file, NULL);
> +
> +	mutex_unlock(&vid_dev->lock);
> +	return 0;
> +}
> +
> +static const struct v4l2_file_operations vid_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = vid_dev_open,
> +	.release = vid_dev_release,
> +	.write = vb2_fop_write,
> +	.read = vb2_fop_read,
> +	.poll = vb2_fop_poll,
> +	.unlocked_ioctl = video_ioctl2,
> +	.mmap = vb2_fop_mmap,
> +};
> +
> +/*
> + * VideoBuffer2 operations
> + */
> +
> +void fill_buffer(struct video_device_dev *dev, struct rx_buffer *buf,
> +			int buf_num, unsigned long flags)
> +{
> +	int size = 0;
> +	void *vbuf = NULL;
> +
> +	if (&buf->vb == NULL)
> +		return;
> +
> +	size = vb2_plane_size(&buf->vb.vb2_buf, 0);
> +	vbuf = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
> +
> +	if (vbuf) {
> +		spin_unlock_irqrestore(&dev->slock, flags);
> +
> +		memcpy(vbuf, dev->dma_buf[buf_num].cpu_addr, size);
> +
> +		spin_lock_irqsave(&dev->slock, flags);
> +
> +		buf->vb.field = dev->format.fmt.pix.field;
> +		buf->vb.sequence++;
> +		buf->vb.vb2_buf.timestamp = ktime_get_ns();
> +	}
> +	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +static void buffer_copy_process(void *param)
> +{
> +	struct video_device_dev *dev = (struct video_device_dev *) param;
> +	unsigned long flags;
> +	struct dmaqueue *dma_q = &dev->vidq;
> +	struct rx_buffer *buf = NULL;
> +
> +	spin_lock_irqsave(&dev->slock, flags);
> +
> +	if (!list_empty(&dma_q->active)) {
> +		buf = list_entry(dma_q->active.next, struct rx_buffer, list);
> +		list_del(&buf->list);
> +		fill_buffer(dev, buf, dev->last_idx, flags);
> +	}
> +
> +	spin_unlock_irqrestore(&dev->slock, flags);
> +}
> +
> +static inline struct rx_buffer *to_rx_buffer(struct vb2_v4l2_buffer *vb2)
> +{
> +	return container_of(vb2, struct rx_buffer, vb);
> +}
> +
> +int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> +			unsigned int *nplanes, unsigned int sizes[],
> +			struct device *alloc_devs[])
> +{
> +	struct video_device_dev *dev = vb2_get_drv_priv(vq);
> +	unsigned long size = 0;
> +	int i;
> +
> +	size = size_image(dev);
> +	if (size == 0)
> +		return -EINVAL;
> +
> +	*nbuffers = N_BUFFERS;
> +
> +	for (i = 0; i < N_BUFFERS; i++) {
> +		dev->dma_buf[i].cpu_addr = dma_alloc_coherent(&dev->pdev->dev,
> +							      size_image(dev),
> +							      &dev->
> +							      dma_buf
> +							      [i].dma_addr,
> +							      GFP_KERNEL);
> +	}
> +
> +	*nplanes = 1;
> +	sizes[0] = size;
> +
> +	return 0;
> +}
> +
> +int buffer_prepare(struct vb2_buffer *vb)
> +{
> +	struct rx_buffer *buf = NULL;
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	int size = 0;
> +
> +	if (vb == NULL) {
> +		pr_warn("%s:vb2_buffer is null\n", FUNC_NAME);
> +		return 0;
> +	}
> +
> +	buf = to_rx_buffer(vbuf);
> +
> +	size = vb2_plane_size(&buf->vb.vb2_buf, 0);
> +	vb2_set_plane_payload(&buf->vb.vb2_buf, 0, size);
> +
> +	INIT_LIST_HEAD(&buf->list);
> +	return 0;
> +}
> +
> +void buffer_queue(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct video_device_dev *dev = NULL;
> +	struct rx_buffer *buf = NULL;
> +	struct dmaqueue *vidq = NULL;
> +	struct dma_async_tx_descriptor *desc;
> +	u32 flags;
> +
> +	if (vb == NULL) {
> +		pr_warn("%s:vb2_buffer is null\n", FUNC_NAME);
> +		return;
> +	}
> +
> +	dev = vb2_get_drv_priv(vb->vb2_queue);
> +	buf = to_rx_buffer(vbuf);
> +	vidq = &dev->vidq;
> +
> +	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +	dev->xt.dir = DMA_DEV_TO_MEM;
> +	dev->xt.src_sgl = false;
> +	dev->xt.dst_inc = false;
> +	dev->xt.dst_sgl = true;
> +	dev->xt.dst_start = dev->dma_buf[dev->idx].dma_addr;
> +
> +	dev->last_idx = dev->idx;
> +	dev->idx++;
> +	if (dev->idx >= N_BUFFERS)
> +		dev->idx = 0;
> +
> +	dev->xt.frame_size = 1;
> +	dev->sgl[0].size = bytes_per_line(dev);
> +	dev->sgl[0].icg = 0;
> +	dev->xt.numf = height(dev);
> +
> +	desc = dmaengine_prep_interleaved_dma(dev->dma, &dev->xt, flags);
> +	if (!desc) {
> +		pr_err("Failed to prepare DMA transfer\n");
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +		return;
> +	}
> +
> +	desc->callback = buffer_copy_process;
> +	desc->callback_param = dev;
> +
> +	spin_lock(&dev->slock);
> +	list_add_tail(&buf->list, &vidq->active);
> +	spin_unlock(&dev->slock);
> +
> +	dmaengine_submit(desc);
> +
> +	if (vb2_is_streaming(&dev->vb_queue))
> +		dma_async_issue_pending(dev->dma);
> +}
> +
> +int start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> +	struct video_device_dev *dev = vb2_get_drv_priv(vq);
> +
> +	dma_async_issue_pending(dev->dma);
> +
> +	return 0;
> +}
> +
> +void stop_streaming(struct vb2_queue *vq)
> +{
> +	struct video_device_dev *dev = vb2_get_drv_priv(vq);
> +	struct dmaqueue *dma_q = &dev->vidq;
> +
> +	/* Stop and reset the DMA engine. */
> +	dmaengine_terminate_all(dev->dma);
> +
> +	while (!list_empty(&dma_q->active)) {
> +		struct rx_buffer *buf;
> +
> +		buf = list_entry(dma_q->active.next, struct rx_buffer, list);
> +		if (buf) {
> +			list_del(&buf->list);
> +			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +		}
> +	}
> +	list_del_init(&dev->vidq.active);
> +}
> +
> +static const struct vb2_ops vb2_video_qops = {
> +	.queue_setup = queue_setup,
> +	.buf_prepare = buffer_prepare,
> +	.buf_queue = buffer_queue,
> +	.start_streaming = start_streaming,
> +	.stop_streaming = stop_streaming,
> +	.wait_prepare = vb2_ops_wait_prepare,
> +	.wait_finish = vb2_ops_wait_finish,
> +};
> +
> +static int vid_dev_subdev_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	return 0;

I think you could just omit defining the function if it does nothing. The
same goes for core ops.

> +}
> +
> +static int vid_dev_subdev_registered(struct v4l2_subdev *sd)
> +{
> +	struct video_device_dev *vid_dev = v4l2_get_subdevdata(sd);
> +	struct vb2_queue *q = &vid_dev->vb_queue;
> +	struct video_device *vfd = &vid_dev->ve.vdev;
> +	int ret;
> +
> +	memset(vfd, 0, sizeof(*vfd));
> +
> +	snprintf(vfd->name, sizeof(vfd->name), VIDEO_DEVICE_NAME);

How about strlcpy()?

> +
> +	vfd->fops = &vid_dev_fops;
> +	vfd->ioctl_ops = &vid_dev_ioctl_ops;
> +	vfd->v4l2_dev = sd->v4l2_dev;
> +	vfd->minor = -1;
> +	vfd->release = video_device_release_empty;
> +	vfd->queue = q;
> +
> +	INIT_LIST_HEAD(&vid_dev->vidq.active);
> +	init_waitqueue_head(&vid_dev->vidq.wq);
> +	memset(q, 0, sizeof(*q));
> +	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	q->io_modes = VB2_MMAP | VB2_USERPTR;
> +	q->ops = &vb2_video_qops;
> +	q->mem_ops = &vb2_vmalloc_memops;
> +	q->buf_struct_size = sizeof(struct rx_buffer);
> +	q->drv_priv = vid_dev;
> +	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +	q->lock = &vid_dev->lock;
> +
> +	ret = vb2_queue_init(q);
> +	if (ret < 0)
> +		return ret;
> +
> +	vid_dev->vd_pad.flags = MEDIA_PAD_FL_SINK;
> +	ret = media_entity_pads_init(&vfd->entity, 1, &vid_dev->vd_pad);
> +	if (ret < 0)
> +		return ret;
> +
> +	video_set_drvdata(vfd, vid_dev);
> +	vid_dev->ve.pipe = v4l2_get_subdev_hostdata(sd);
> +
> +	ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
> +	if (ret < 0) {
> +		media_entity_cleanup(&vfd->entity);
> +		vid_dev->ve.pipe = NULL;
> +		return ret;
> +	}
> +
> +	v4l2_info(sd->v4l2_dev, "Registered %s as /dev/%s\n",
> +		  vfd->name, video_device_node_name(vfd));
> +	return 0;
> +}
> +
> +static void vid_dev_subdev_unregistered(struct v4l2_subdev *sd)
> +{
> +	struct video_device_dev *vid_dev = v4l2_get_subdevdata(sd);
> +
> +	if (vid_dev == NULL)
> +		return;

Can this happen?

> +
> +	mutex_lock(&vid_dev->lock);
> +
> +	if (video_is_registered(&vid_dev->ve.vdev)) {
> +		video_unregister_device(&vid_dev->ve.vdev);
> +		media_entity_cleanup(&vid_dev->ve.vdev.entity);
> +		vid_dev->ve.pipe = NULL;
> +	}
> +
> +	mutex_unlock(&vid_dev->lock);
> +}
> +
> +static const struct v4l2_subdev_internal_ops vid_dev_subdev_internal_ops = {
> +	.registered = vid_dev_subdev_registered,
> +	.unregistered = vid_dev_subdev_unregistered,
> +};
> +
> +static const struct v4l2_subdev_core_ops vid_dev_subdev_core_ops = {
> +	.s_power = vid_dev_subdev_s_power,
> +};
> +
> +static struct v4l2_subdev_ops vid_dev_subdev_ops = {
> +	.core = &vid_dev_subdev_core_ops,
> +};
> +
> +static int vid_dev_create_capture_subdev(struct video_device_dev *vid_dev)
> +{
> +	struct v4l2_subdev *sd = &vid_dev->subdev;
> +	int ret;
> +
> +	v4l2_subdev_init(sd, &vid_dev_subdev_ops);
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	snprintf(sd->name, sizeof(sd->name), "Capture device");
> +
> +	vid_dev->subdev_pads[VIDEO_DEV_SD_PAD_SINK_CSI].flags =
> +		MEDIA_PAD_FL_SOURCE;
> +	vid_dev->subdev_pads[VIDEO_DEV_SD_PAD_SOURCE_DMA].flags =
> +		MEDIA_PAD_FL_SINK;
> +	ret =

No need for a line break.

> +	    media_entity_pads_init(&sd->entity, VIDEO_DEV_SD_PADS_NUM,
> +				   vid_dev->subdev_pads);
> +	if (ret)
> +		return ret;
> +
> +	sd->internal_ops = &vid_dev_subdev_internal_ops;
> +	sd->entity.ops = &vid_dev_subdev_media_ops;
> +	sd->owner = THIS_MODULE;
> +	v4l2_set_subdevdata(sd, vid_dev);
> +
> +	return 0;
> +}
> +
> +static void vid_dev_unregister_subdev(struct video_device_dev *vid_dev)
> +{
> +	struct v4l2_subdev *sd = &vid_dev->subdev;
> +
> +	v4l2_device_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_set_subdevdata(sd, NULL);
> +}
> +
> +static const struct of_device_id vid_dev_of_match[];
> +
> +static int vid_dev_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *of_id;
> +	int ret = 0;
> +	struct video_device_dev *vid_dev;
> +
> +	dev_info(dev, "Installing IPK Video Device module\n");
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	vid_dev = devm_kzalloc(dev, sizeof(*vid_dev), GFP_KERNEL);
> +	if (!vid_dev)
> +		return -ENOMEM;
> +
> +	of_id = of_match_node(vid_dev_of_match, dev->of_node);
> +	if (WARN_ON(of_id == NULL))
> +		return -EINVAL;
> +
> +	vid_dev->pdev = pdev;
> +
> +	spin_lock_init(&vid_dev->slock);
> +	mutex_init(&vid_dev->lock);
> +
> +	dev_info(&pdev->dev, "Requesting DMA\n");
> +	vid_dev->dma = dma_request_slave_channel(&pdev->dev, "vdma0");
> +	if (vid_dev->dma == NULL) {
> +		dev_err(&pdev->dev, "no VDMA channel found\n");
> +		ret = -ENODEV;
> +		goto end;
> +	}
> +
> +	ret = vid_dev_create_capture_subdev(vid_dev);
> +	if (ret)
> +		goto end;
> +
> +	platform_set_drvdata(pdev, vid_dev);
> +
> +	dev_info(dev, "Video Device registered successfully\n");
> +	return 0;
> +end:
> +	dev_err(dev, "Video Device not registered!!\n");
> +	return ret;
> +}
> +
> +static int vid_dev_remove(struct platform_device *pdev)
> +{
> +	struct video_device_dev *dev = platform_get_drvdata(pdev);
> +
> +	vid_dev_unregister_subdev(dev);
> +	dev_info(&pdev->dev, "Driver removed\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id vid_dev_of_match[] = {
> +	{.compatible = "snps,video-device"},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, vid_dev_of_match);
> +
> +static struct platform_driver __refdata vid_dev_pdrv = {
> +	.remove = vid_dev_remove,
> +	.probe = vid_dev_probe,
> +	.driver = {
> +		   .name = VIDEO_DEVICE_NAME,
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = vid_dev_of_match,
> +		   },
> +};
> +
> +module_platform_driver(vid_dev_pdrv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Ramiro Oliveira <roliveir@synopsys.com>");
> +MODULE_DESCRIPTION("Driver for configuring DMA and Video Device");
> diff --git a/drivers/media/platform/dwc/video_device.h b/drivers/media/platform/dwc/video_device.h
> new file mode 100644
> index 0000000..e828d4b
> --- /dev/null
> +++ b/drivers/media/platform/dwc/video_device.h
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef VIDEO_DEVICE_H_
> +#define VIDEO_DEVICE_H_
> +
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/delay.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +#include <linux/string.h>
> +#include <linux/videodev2.h>
> +#include <linux/dma/xilinx_dma.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-common.h>
> +#include <media/videobuf2-vmalloc.h>
> +#include <media/media-entity.h>
> +#include <linux/io.h>
> +
> +#include "plat_ipk_video.h"
> +
> +#define N_BUFFERS 3
> +
> +#define VIDEO_DEVICE_NAME	"video-device"
> +
> +#define FUNC_NAME __func__
> +
> +struct rx_buffer {
> +	/** @short Buffer for video frames */
> +	struct vb2_v4l2_buffer vb;
> +	struct list_head list;
> +
> +	dma_addr_t dma_addr;
> +	void *cpu_addr;
> +};
> +
> +struct dmaqueue {
> +	struct list_head active;
> +	wait_queue_head_t wq;
> +};
> +
> +/**
> + * @short Structure to embed device driver information
> + */
> +struct video_device_dev {
> +	struct platform_device *pdev;
> +	struct v4l2_device *v4l2_dev;
> +	struct v4l2_subdev subdev;
> +	struct media_pad vd_pad;
> +	struct media_pad subdev_pads[VIDEO_DEV_SD_PADS_NUM];
> +	struct mutex lock;
> +	spinlock_t slock;
> +	struct plat_ipk_video_entity ve;
> +	struct v4l2_format format;
> +	struct v4l2_pix_format pix_format;
> +	const struct plat_ipk_fmt *fmt;
> +	unsigned long *alloc_ctx;
> +
> +	/* Buffer and DMA */
> +	struct vb2_queue vb_queue;
> +	int idx;
> +	int last_idx;
> +	struct dmaqueue vidq;
> +	struct rx_buffer dma_buf[N_BUFFERS];
> +	struct dma_chan *dma;
> +	struct dma_interleaved_template xt;
> +	struct data_chunk sgl[1];
> +};
> +
> +/**
> + * @short Defines to simplify the code reading
> + */
> +
> +#define pixel_format(dev)	\
> +	dev->format.fmt.pix.pixelformat
> +#define bytes_per_line(dev)	\
> +	dev->format.fmt.pix.bytesperline
> +#define width(dev)		\
> +	dev->format.fmt.pix.width
> +#define height(dev)		\
> +	dev->format.fmt.pix.height
> +#define size_image(dev)		\
> +	dev->format.fmt.pix.sizeimage

How about referring to the field directly, or define a local variable for a
pointer to dev->format.fmt.fix in functions it's needed.

> +
> +const struct plat_ipk_fmt *vid_dev_find_format(struct v4l2_format *f,
> +					       int index);
> +
> +#endif				/* VIDEO_DEVICE_H_ */

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply

* Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion
From: Tom Levens @ 2016-11-17 16:23 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Tom Levens, jdelvare, robh+dt, mark.rutland, linux-kernel,
	linux-hwmon, devicetree
In-Reply-To: <86c28b4f-c78b-8d3e-5c48-fa42ca614ba4@roeck-us.net>

Hi Guenter,

Thanks for taking the time to review the patch.

On Thu, 17 Nov 2016, Guenter Roeck wrote:

> Hi Tom,
>
> On 11/17/2016 04:10 AM, Tom Levens wrote:
>>  Conversion from raw values to signed integers has been refactored using
>>  the macros in bitops.h.
>> 
> Please also mention that this fixes a bug in negative temperature 
> conversions.

Yes, of course, I will include the information in v3.

>
>>  Signed-off-by: Tom Levens <tom.levens@cern.ch>
>>  ---
>>   drivers/hwmon/ltc2990.c |   27 ++++++++++-----------------
>>   1 files changed, 10 insertions(+), 17 deletions(-)
>>
>>  diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>>  index 8f8fe05..0ec4102 100644
>>  --- a/drivers/hwmon/ltc2990.c
>>  +++ b/drivers/hwmon/ltc2990.c
>>  @@ -9,8 +9,12 @@
>>    * This driver assumes the chip is wired as a dual current monitor, and
>>    * reports the voltage drop across two series resistors. It also reports
>>    * the chip's internal temperature and Vcc power supply voltage.
>>  + *
>>  + * Value conversion refactored
>>  + * by Tom Levens <tom.levens@cern.ch>
>
> Kind of unusual to do that for minor changes like this. Imagine if everyone would do that.
> The commit log is what gives you credit.

Good point, thanks for the hint. I will remove it from v3.

>>    */
>>
>>  +#include <linux/bitops.h>
>>   #include <linux/err.h>
>>   #include <linux/hwmon.h>
>>   #include <linux/hwmon-sysfs.h>
>>  @@ -34,19 +38,10 @@
>>   #define LTC2990_CONTROL_MODE_CURRENT	0x06
>>   #define LTC2990_CONTROL_MODE_VOLTAGE	0x07
>>
>>  -/* convert raw register value to sign-extended integer in 16-bit range */
>>  -static int ltc2990_voltage_to_int(int raw)
>>  -{
>>  -	if (raw & BIT(14))
>>  -		return -(0x4000 - (raw & 0x3FFF)) << 2;
>>  -	else
>>  -		return (raw & 0x3FFF) << 2;
>>  -}
>>  -
>>  /* Return the converted value from the given register in uV or mC */
>>  -static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>>  +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 *result)
>>  {
>>  -	int val;
>>  +	s32 val;
>
> Please just leave the variable type alone. it is also used for the return value
> from i2c_smbus_read_word_swapped(), which is an int, and changing it to s32 
> doesn't really make the code better.

According to i2c.h the return type for i2c_smbus_read_word_swapped() is 
s32, which is why I modified it here. But it could be changed back if you 
think it is better to leave it as an int.

> Can you send me a register map for the chip ? I would like to write a module test.

Here is an example register dump:

00 00 00 00
01 90 07 d0
2c cd 7d 80
7c 29 20 00

The expected values in this case are:

in0_input 	5000
in1_input	610
in2_input	3500
in3_input	-195
in4_input	-299
temp1_input 	25000
temp2_input 	125000
temp3_input	-40000
curr1_input	38840
curr2_input	-12428

Testing with lltc,mode set to <5>, <6> and <7> should give you all 
measurements.

> Thanks,
> Guenter

Cheers,

^ permalink raw reply

* Re: [PATCH v2 2/4] usb: dwc2: Add binding for AHB burst
From: John Youn @ 2016-11-17 16:07 UTC (permalink / raw)
  To: Stefan Wahren, John Youn, Felipe Balbi,
	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
	Mark Rutland
  Cc: Christian Lamparter
In-Reply-To: <633e5a10-1ea0-48c7-a5b7-a5ff2625e759-eS4NqCHxEME@public.gmane.org>

On 11/17/2016 7:35 AM, Stefan Wahren wrote:
> Hi John,
> 
> Am 17.11.2016 um 00:47 schrieb John Youn:
>> Add the "snps,ahb-burst" binding and read it in.
>>
>> This property controls which burst type to perform on the AHB bus as a
>> master in internal DMA mode. This overrides the legacy param value,
>> which we need to keep around for now since several platforms use it.
>>
>> Some platforms may see better or worse performance based on this
>> value. The HAPS platform is one example where all INCRx have worse
>> performance than INCR.
>>
>> Other platforms (such as the Canyonlands board) report that the default
>> value causes system hangs.
>>
>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> Cc: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
>>  drivers/usb/dwc2/core.h                        |  9 +++++
>>  drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
>>  3 files changed, 67 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
>> index 6c7c2bce..9e7b4b4 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> 
> according to Documentation/devicetree/bindings/submitting-patches.txt
> this change should be a separate patch.

Ok

> 
>> @@ -26,6 +26,8 @@ Optional properties:
>>  Refer to phy/phy-bindings.txt for generic phy consumer properties
>>  - dr_mode: shall be one of "host", "peripheral" and "otg"
>>    Refer to usb/generic.txt
>> +- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
>> +  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".
> 
> This doesn't apply in case of the bcm2835. I would prefer this option is
> ignored in that case with a dev_warn("snps,ahb-burst is not supported on
> this platform").

Sure I'll add this.

Regards,
John


--
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] clk: qcom: smd-rpm: Add msm8974 clocks
From: Georgi Djakov @ 2016-11-17 15:53 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Stephen Boyd
  Cc: linux-clk, devicetree, linux-kernel, linux-arm-msm, linux-soc
In-Reply-To: <1479192844-1281-1-git-send-email-bjorn.andersson@linaro.org>

On 11/15/2016 08:54 AM, Bjorn Andersson wrote:
> This adds all RPM based clocks for msm8974 except cxo and gfx3d_clk_src.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  .../devicetree/bindings/clock/qcom,rpmcc.txt       |  1 +
>  drivers/clk/qcom/clk-smd-rpm.c                     | 71 ++++++++++++++++++++++
>  include/dt-bindings/clock/qcom,rpmcc.h             | 40 +++++++++++-
>  3 files changed, 110 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> index 87d3714b956a..a7235e9e1c97 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> @@ -11,6 +11,7 @@ Required properties :
>                 compatible "qcom,rpmcc" should be also included.
>
>  			"qcom,rpmcc-msm8916", "qcom,rpmcc"
> +			"qcom,rpmcc-msm8974", "qcom,rpmcc"
>  			"qcom,rpmcc-apq8064", "qcom,rpmcc"
>
>  - #clock-cells : shall contain 1
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index a27013dbc0aa..b8fcac6f2f87 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -462,8 +462,79 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8916 = {
>  	.num_clks = ARRAY_SIZE(msm8916_clks),
>  };
>
> +/* msm8974 */
> +DEFINE_CLK_SMD_RPM(msm8974, pnoc_clk, pnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
> +DEFINE_CLK_SMD_RPM(msm8974, snoc_clk, snoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 1);
> +DEFINE_CLK_SMD_RPM(msm8974, cnoc_clk, cnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 2);
> +DEFINE_CLK_SMD_RPM(msm8974, mmssnoc_ahb_clk, mmssnoc_ahb_a_clk, QCOM_SMD_RPM_BUS_CLK, 3);
> +DEFINE_CLK_SMD_RPM(msm8974, bimc_clk, bimc_a_clk, QCOM_SMD_RPM_MEM_CLK, 0);
> +DEFINE_CLK_SMD_RPM(msm8974, gfx3d_clk_src, gfx3d_a_clk_src, QCOM_SMD_RPM_MEM_CLK, 1);
> +DEFINE_CLK_SMD_RPM(msm8974, ocmemgx_clk, ocmemgx_a_clk, QCOM_SMD_RPM_MEM_CLK, 2);
> +DEFINE_CLK_SMD_RPM_QDSS(msm8974, qdss_clk, qdss_a_clk, QCOM_SMD_RPM_MISC_CLK, 1);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, cxo_d0, cxo_d0_a, 1);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, cxo_d1, cxo_d1_a, 2);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, cxo_a0, cxo_a0_a, 4);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, cxo_a1, cxo_a1_a, 5);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, cxo_a2, cxo_a2_a, 6);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, diff_clk, diff_a_clk, 7);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, div_clk1, div_a_clk1, 11);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8974, div_clk2, div_a_clk2, 12);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8974, cxo_d0_pin, cxo_d0_a_pin, 1);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8974, cxo_d1_pin, cxo_d1_a_pin, 2);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8974, cxo_a0_pin, cxo_a0_a_pin, 4);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8974, cxo_a1_pin, cxo_a1_a_pin, 5);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8974, cxo_a2_pin, cxo_a2_a_pin, 6);
> +
> +static struct clk_smd_rpm *msm8974_clks[] = {
> +	[RPM_SMD_PNOC_CLK]		= &msm8974_pnoc_clk,
> +	[RPM_SMD_PNOC_A_CLK]		= &msm8974_pnoc_a_clk,
> +	[RPM_SMD_SNOC_CLK]		= &msm8974_snoc_clk,
> +	[RPM_SMD_SNOC_A_CLK]		= &msm8974_snoc_a_clk,
> +	[RPM_SMD_CNOC_CLK]		= &msm8974_cnoc_clk,
> +	[RPM_SMD_CNOC_A_CLK]		= &msm8974_cnoc_a_clk,
> +	[RPM_SMD_MMSSNOC_AHB_CLK]	= &msm8974_mmssnoc_ahb_clk,
> +	[RPM_SMD_MMSSNOC_AHB_A_CLK]	= &msm8974_mmssnoc_ahb_a_clk,
> +	[RPM_SMD_BIMC_CLK]		= &msm8974_bimc_clk,
> +	[RPM_SMD_BIMC_A_CLK]		= &msm8974_bimc_a_clk,
> +	[RPM_SMD_OCMEMGX_CLK]		= &msm8974_ocmemgx_clk,
> +	[RPM_SMD_OCMEMGX_A_CLK]		= &msm8974_ocmemgx_a_clk,
> +	[RPM_SMD_QDSS_CLK]		= &msm8974_qdss_clk,
> +	[RPM_SMD_QDSS_A_CLK]		= &msm8974_qdss_a_clk,
> +	[RPM_SMD_CXO_D0]			= &msm8974_cxo_d0,
> +	[RPM_SMD_CXO_D0_A]		= &msm8974_cxo_d0_a,
> +	[RPM_SMD_CXO_D1]			= &msm8974_cxo_d1,
> +	[RPM_SMD_CXO_D1_A]		= &msm8974_cxo_d1_a,
> +	[RPM_SMD_CXO_A0]			= &msm8974_cxo_a0,
> +	[RPM_SMD_CXO_A0_A]		= &msm8974_cxo_a0_a,
> +	[RPM_SMD_CXO_A1]			= &msm8974_cxo_a1,
> +	[RPM_SMD_CXO_A1_A]		= &msm8974_cxo_a1_a,
> +	[RPM_SMD_CXO_A2]			= &msm8974_cxo_a2,

There are some extra tabs above. Otherwise looks fine:
Tested-by: Georgi Djakov <georgi.djakov@linaro.org>

BR,
Georgi

^ permalink raw reply

* Re: [PATCH -next] ARM: dts: omap5: replace gpio-key,wakeup with wakeup-source property
From: Tony Lindgren @ 2016-11-17 15:47 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: devicetree, linux-omap, Benoît Cousson, linux-arm-kernel
In-Reply-To: <8d06724a-98af-05ab-f5a8-07c552722fca@arm.com>

* Sudeep Holla <sudeep.holla@arm.com> [161117 07:32]:
> 
> 
> On 17/11/16 15:27, Tony Lindgren wrote:
> > * Sudeep Holla <sudeep.holla@arm.com> [161114 07:44]:
> > > Though the keyboard driver for GPIO buttons(gpio-keys) will continue to
> > > check for/support the legacy "gpio-key,wakeup" boolean property to
> > > enable gpio buttons as wakeup source, "wakeup-source" is the new
> > > standard binding.
> > > 
> > > This patch replaces the legacy "gpio-key,wakeup" with the unified
> > > "wakeup-source" property in order to avoid any further copy-paste
> > > duplication.
> > > 
> > > Cc: "Benoît Cousson" <bcousson@baylibre.com>
> > > Cc: Tony Lindgren <tony@atomide.com>
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  arch/arm/boot/dts/omap5-uevm.dts | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Hi,
> > > 
> > > Inspite of getting rid of most of the legacy property almost a year ago,
> > > addition of new platforms have brought this back and over time it's
> > > now found again in few places. Just get rid of them *again*
> > 
> > Thanks for the annual check up :)
> > 
> > Acked-by: Tony Lindgren <tony@atomide.com>
> > 
> > Please let me know if you want me to pick this up instead.
> 
> Yes that would be better. Also it's present only in your -next,
> looks like something newly added via commit 2d46c0c60725 ("ARM:
> dts: omap5 uevm: add USR1 button")

OK applied into omap-for-v4.10/dt:omap-for-v4.10/dt thanks.

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH] ARM: dts: qcom: Add apq8064 CoreSight components
From: Georgi Djakov @ 2016-11-17 15:36 UTC (permalink / raw)
  To: andy.gross
  Cc: devicetree, mathieu.poirier, linux-arm-msm, linux-kernel,
	zhang.chunyan, robh+dt, iivanov.xz, georgi.djakov,
	linux-arm-kernel

From: "Ivan T. Ivanov" <ivan.ivanov@linaro.org>

Add initial set of CoreSight components found on Qualcomm's
8064 chipset.

Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8064-coresight.dtsi | 196 ++++++++++++++++++++++++++
 arch/arm/boot/dts/qcom-apq8064.dtsi           |  11 +-
 2 files changed, 203 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/boot/dts/qcom-apq8064-coresight.dtsi

diff --git a/arch/arm/boot/dts/qcom-apq8064-coresight.dtsi b/arch/arm/boot/dts/qcom-apq8064-coresight.dtsi
new file mode 100644
index 000000000000..9395fddb1bf0
--- /dev/null
+++ b/arch/arm/boot/dts/qcom-apq8064-coresight.dtsi
@@ -0,0 +1,196 @@
+/*
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+&soc {
+
+	etb@1a01000 {
+		compatible = "coresight-etb10", "arm,primecell";
+		reg = <0x1a01000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		port {
+			etb_in: endpoint {
+				slave-mode;
+				remote-endpoint = <&replicator_out0>;
+			};
+		};
+	};
+
+	tpiu@1a03000 {
+		compatible = "arm,coresight-tpiu", "arm,primecell";
+		reg = <0x1a03000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		port {
+			tpiu_in: endpoint {
+				slave-mode;
+				remote-endpoint = <&replicator_out1>;
+			};
+		};
+	};
+
+	replicator {
+		compatible = "arm,coresight-replicator";
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				replicator_out0: endpoint {
+					remote-endpoint = <&etb_in>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				replicator_out1: endpoint {
+					remote-endpoint = <&tpiu_in>;
+				};
+			};
+			port@2 {
+				reg = <0>;
+				replicator_in: endpoint {
+					slave-mode;
+					remote-endpoint = <&funnel_out>;
+				};
+			};
+		};
+	};
+
+	funnel@1a04000 {
+		compatible = "arm,coresight-funnel", "arm,primecell";
+		reg = <0x1a04000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/*
+			 * Not described input ports:
+			 * 2 - connected to STM component
+			 * 3 - not-connected
+			 * 6 - not-connected
+			 * 7 - not-connected
+			 */
+			port@0 {
+				reg = <0>;
+				funnel_in0: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm0_out>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				funnel_in1: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm1_out>;
+				};
+			};
+			port@4 {
+				reg = <4>;
+				funnel_in4: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm2_out>;
+				};
+			};
+			port@5 {
+				reg = <5>;
+				funnel_in5: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm3_out>;
+				};
+			};
+			port@8 {
+				reg = <0>;
+				funnel_out: endpoint {
+					remote-endpoint = <&replicator_in>;
+				};
+			};
+		};
+	};
+
+	etm@1a1c000 {
+		compatible = "arm,coresight-etm3x", "arm,primecell";
+		reg = <0x1a1c000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		cpu = <&CPU0>;
+
+		port {
+			etm0_out: endpoint {
+				remote-endpoint = <&funnel_in0>;
+			};
+		};
+	};
+
+	etm@1a1d000 {
+		compatible = "arm,coresight-etm3x", "arm,primecell";
+		reg = <0x1a1d000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		cpu = <&CPU1>;
+
+		port {
+			etm1_out: endpoint {
+				remote-endpoint = <&funnel_in1>;
+			};
+		};
+	};
+
+	etm@1a1e000 {
+		compatible = "arm,coresight-etm3x", "arm,primecell";
+		reg = <0x1a1e000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		cpu = <&CPU2>;
+
+		port {
+			etm2_out: endpoint {
+				remote-endpoint = <&funnel_in4>;
+			};
+		};
+	};
+
+	etm@1a1f000 {
+		compatible = "arm,coresight-etm3x", "arm,primecell";
+		reg = <0x1a1f000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>;
+		clock-names = "apb_pclk";
+
+		cpu = <&CPU3>;
+
+		port {
+			etm3_out: endpoint {
+				remote-endpoint = <&funnel_in5>;
+			};
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 268bd470c865..18469c632e2f 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -4,6 +4,7 @@
 #include <dt-bindings/clock/qcom,gcc-msm8960.h>
 #include <dt-bindings/reset/qcom,gcc-msm8960.h>
 #include <dt-bindings/clock/qcom,mmcc-msm8960.h>
+#include <dt-bindings/clock/qcom,rpmcc.h>
 #include <dt-bindings/soc/qcom,gsbi.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -27,7 +28,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		cpu@0 {
+		CPU0: cpu@0 {
 			compatible = "qcom,krait";
 			enable-method = "qcom,kpss-acc-v1";
 			device_type = "cpu";
@@ -38,7 +39,7 @@
 			cpu-idle-states = <&CPU_SPC>;
 		};
 
-		cpu@1 {
+		CPU1: cpu@1 {
 			compatible = "qcom,krait";
 			enable-method = "qcom,kpss-acc-v1";
 			device_type = "cpu";
@@ -49,7 +50,7 @@
 			cpu-idle-states = <&CPU_SPC>;
 		};
 
-		cpu@2 {
+		CPU2: cpu@2 {
 			compatible = "qcom,krait";
 			enable-method = "qcom,kpss-acc-v1";
 			device_type = "cpu";
@@ -60,7 +61,7 @@
 			cpu-idle-states = <&CPU_SPC>;
 		};
 
-		cpu@3 {
+		CPU3: cpu@3 {
 			compatible = "qcom,krait";
 			enable-method = "qcom,kpss-acc-v1";
 			device_type = "cpu";
@@ -1418,4 +1419,6 @@
 		};
 	};
 };
+
+#include "qcom-apq8064-coresight.dtsi"
 #include "qcom-apq8064-pins.dtsi"

^ permalink raw reply related

* [PATCH v4] arm64: dts: qcom: Add msm8916 CoreSight components
From: Georgi Djakov @ 2016-11-17 15:35 UTC (permalink / raw)
  To: andy.gross
  Cc: devicetree, mathieu.poirier, linux-arm-msm, linux-kernel,
	zhang.chunyan, robh+dt, iivanov.xz, georgi.djakov,
	linux-arm-kernel

From: "Ivan T. Ivanov" <ivan.ivanov@linaro.org>

Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.

Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---

This patch was on hold for some time, as it has a dependency on RPM clocks,
which is now merged into clk-next.

Changes since v3: (https://lkml.org/lkml/2015/5/11/134)
 * Include msm8916-coresight.dtsi into msm8916.dtsi

Changes since v2: (https://lkml.org/lkml/2015/4/29/242)
 * Added "1x" to "qcom,coresight-replicator" compatible string, to match what
   devicetree bindings documentations says.


 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi | 254 ++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/msm8916.dtsi           |   2 +
 2 files changed, 256 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi

diff --git a/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi b/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
new file mode 100644
index 000000000000..900f1f484a0a
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
@@ -0,0 +1,254 @@
+/*
+ * Copyright (c) 2013 - 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+&soc {
+
+	tpiu@820000 {
+		compatible = "arm,coresight-tpiu", "arm,primecell";
+		reg = <0x820000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		port {
+			tpiu_in: endpoint {
+				slave-mode;
+				remote-endpoint = <&replicator_out1>;
+			};
+		};
+	};
+
+	funnel@821000 {
+		compatible = "arm,coresight-funnel", "arm,primecell";
+		reg = <0x821000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/*
+			 * Not described input ports:
+			 * 0 - connected to Resource and Power Manger CPU ETM
+			 * 1 - not-connected
+			 * 2 - connected to Modem CPU ETM
+			 * 3 - not-connected
+			 * 5 - not-connected
+			 * 6 - connected trought funnel to Wireless CPU ETM
+			 * 7 - connected to STM component
+			 */
+			port@4 {
+				reg = <4>;
+				funnel0_in4: endpoint {
+					slave-mode;
+					remote-endpoint = <&funnel1_out>;
+				};
+			};
+			port@8 {
+				reg = <0>;
+				funnel0_out: endpoint {
+					remote-endpoint = <&etf_in>;
+				};
+			};
+		};
+	};
+
+	replicator@824000 {
+		compatible = "qcom,coresight-replicator1x", "arm,primecell";
+		reg = <0x824000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				replicator_out0: endpoint {
+					remote-endpoint = <&etr_in>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				replicator_out1: endpoint {
+					remote-endpoint = <&tpiu_in>;
+				};
+			};
+			port@2 {
+				reg = <0>;
+				replicator_in: endpoint {
+					slave-mode;
+					remote-endpoint = <&etf_out>;
+				};
+			};
+		};
+	};
+
+	etf@825000 {
+		compatible = "arm,coresight-tmc", "arm,primecell";
+		reg = <0x825000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				etf_out: endpoint {
+					slave-mode;
+					remote-endpoint = <&funnel0_out>;
+				};
+			};
+			port@1 {
+				reg = <0>;
+				etf_in: endpoint {
+					remote-endpoint = <&replicator_in>;
+				};
+			};
+		};
+	};
+
+	etr@826000 {
+		compatible = "arm,coresight-tmc", "arm,primecell";
+		reg = <0x826000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		port {
+			etr_in: endpoint {
+				slave-mode;
+				remote-endpoint = <&replicator_out0>;
+			};
+		};
+	};
+
+	funnel@841000 {	/* APSS funnel only 4 inputs are used */
+		compatible = "arm,coresight-funnel", "arm,primecell";
+		reg = <0x841000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				funnel1_in0: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm0_out>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				funnel1_in1: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm1_out>;
+				};
+			};
+			port@2 {
+				reg = <2>;
+				funnel1_in2: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm2_out>;
+				};
+			};
+			port@3 {
+				reg = <3>;
+				funnel1_in3: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm3_out>;
+				};
+			};
+			port@4 {
+				reg = <0>;
+				funnel1_out: endpoint {
+					remote-endpoint = <&funnel0_in4>;
+				};
+			};
+		};
+	};
+
+	etm@85c000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85c000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU0>;
+
+		port {
+			etm0_out: endpoint {
+				remote-endpoint = <&funnel1_in0>;
+			};
+		};
+	};
+
+	etm@85d000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85d000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU1>;
+
+		port {
+			etm1_out: endpoint {
+				remote-endpoint = <&funnel1_in1>;
+			};
+		};
+	};
+
+	etm@85e000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85e000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU2>;
+
+		port {
+			etm2_out: endpoint {
+				remote-endpoint = <&funnel1_in2>;
+			};
+		};
+	};
+
+	etm@85f000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85f000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU3>;
+
+		port {
+			etm3_out: endpoint {
+				remote-endpoint = <&funnel1_in3>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 4221b7d2c0ce..bfaeb9364190 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -14,6 +14,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,gcc-msm8916.h>
 #include <dt-bindings/reset/qcom,gcc-msm8916.h>
+#include <dt-bindings/clock/qcom,rpmcc.h>
 
 / {
 	model = "Qualcomm Technologies, Inc. MSM8916";
@@ -993,4 +994,5 @@
 	};
 };
 
+#include "msm8916-coresight.dtsi"
 #include "msm8916-pins.dtsi"

^ permalink raw reply related

* Re: [PATCH v2 2/4] usb: dwc2: Add binding for AHB burst
From: Stefan Wahren @ 2016-11-17 15:35 UTC (permalink / raw)
  To: John Youn, Felipe Balbi, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Mark Rutland
  Cc: Christian Lamparter
In-Reply-To: <7fa1c1c4d703c435d698cdf140c9d43163347f1d.1479339900.git.johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:
> Add the "snps,ahb-burst" binding and read it in.
>
> This property controls which burst type to perform on the AHB bus as a
> master in internal DMA mode. This overrides the legacy param value,
> which we need to keep around for now since several platforms use it.
>
> Some platforms may see better or worse performance based on this
> value. The HAPS platform is one example where all INCRx have worse
> performance than INCR.
>
> Other platforms (such as the Canyonlands board) report that the default
> value causes system hangs.
>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> Cc: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
>  drivers/usb/dwc2/core.h                        |  9 +++++
>  drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
> index 6c7c2bce..9e7b4b4 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt

according to Documentation/devicetree/bindings/submitting-patches.txt
this change should be a separate patch.

> @@ -26,6 +26,8 @@ Optional properties:
>  Refer to phy/phy-bindings.txt for generic phy consumer properties
>  - dr_mode: shall be one of "host", "peripheral" and "otg"
>    Refer to usb/generic.txt
> +- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
> +  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is
ignored in that case with a dev_warn("snps,ahb-burst is not supported on
this platform").

Stefan
--
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

* [PATCH 2/2] of: base: replace all duplicate code with of_machine_get_model_name
From: Sudeep Holla @ 2016-11-17 15:32 UTC (permalink / raw)
  To: linux-kernel; +Cc: Sudeep Holla, Rob Herring, Arnd Bergmann, devicetree
In-Reply-To: <1479396775-32033-1-git-send-email-sudeep.holla@arm.com>

This patch replaces all the code snippets that reads the model name
from the device tree with the newly added helper function:
of_machine_get_model_name

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm/mach-imx/cpu.c           |  5 +----
 arch/arm/mach-mxs/mach-mxs.c      |  4 +---
 arch/mips/cavium-octeon/setup.c   | 12 ++----------
 arch/mips/generic/proc.c          | 15 +++------------
 arch/sh/boards/of-generic.c       |  8 +-------
 drivers/soc/fsl/guts.c            |  8 ++------
 drivers/soc/renesas/renesas-soc.c |  4 +---
 7 files changed, 11 insertions(+), 45 deletions(-)

Hi,

This patch is just to show the use of PATCH 1/2. It needs to go after
1/2 is merged for sake of simplicity and avoid cross dependencies
(targetting v4.11)

Regards,
Sudeep

diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c
index b3347d32349f..dc3ebc1560dd 100644
--- a/arch/arm/mach-imx/cpu.c
+++ b/arch/arm/mach-imx/cpu.c
@@ -75,7 +75,6 @@ struct device * __init imx_soc_device_init(void)
 {
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
-	struct device_node *root;
 	const char *soc_id;
 	int ret;

@@ -85,9 +84,7 @@ struct device * __init imx_soc_device_init(void)

 	soc_dev_attr->family = "Freescale i.MX";

-	root = of_find_node_by_path("/");
-	ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
-	of_node_put(root);
+	ret = of_machine_get_model_name(&soc_dev_attr->machine);
 	if (ret)
 		goto free_soc;

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index e4f21086b42b..871c65f02fca 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -381,7 +381,6 @@ static void __init eukrea_mbmx283lc_init(void)

 static void __init mxs_machine_init(void)
 {
-	struct device_node *root;
 	struct device *parent;
 	struct soc_device *soc_dev;
 	struct soc_device_attribute *soc_dev_attr;
@@ -391,8 +390,7 @@ static void __init mxs_machine_init(void)
 	if (!soc_dev_attr)
 		return;

-	root = of_find_node_by_path("/");
-	ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
+	ret = of_machine_get_model_name(&soc_dev_attr->machine);
 	if (ret)
 		return;

diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
index 9a2db1c013d9..2e2b1b5befa4 100644
--- a/arch/mips/cavium-octeon/setup.c
+++ b/arch/mips/cavium-octeon/setup.c
@@ -498,16 +498,8 @@ static void __init init_octeon_system_type(void)
 	char const *board_type;

 	board_type = cvmx_board_type_to_string(octeon_bootinfo->board_type);
-	if (board_type == NULL) {
-		struct device_node *root;
-		int ret;
-
-		root = of_find_node_by_path("/");
-		ret = of_property_read_string(root, "model", &board_type);
-		of_node_put(root);
-		if (ret)
-			board_type = "Unsupported Board";
-	}
+	if (!board_type && of_machine_get_model_name(&board_type))
+		board_type = "Unsupported Board";

 	snprintf(octeon_system_type, sizeof(octeon_system_type), "%s (%s)",
 		 board_type, octeon_model_get_string(read_c0_prid()));
diff --git a/arch/mips/generic/proc.c b/arch/mips/generic/proc.c
index 42b33250a4a2..f7fc067bf908 100644
--- a/arch/mips/generic/proc.c
+++ b/arch/mips/generic/proc.c
@@ -10,20 +10,11 @@

 #include <linux/of.h>

-#include <asm/bootinfo.h>
-
 const char *get_system_type(void)
 {
 	const char *str;
-	int err;
-
-	err = of_property_read_string(of_root, "model", &str);
-	if (!err)
-		return str;
-
-	err = of_property_read_string_index(of_root, "compatible", 0, &str);
-	if (!err)
-		return str;

-	return "Unknown";
+	if (of_machine_get_model_name(&str))
+		return "Unknown";
+	return str;
 }
diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c
index 1fb6d5714bae..9de3f81b2d22 100644
--- a/arch/sh/boards/of-generic.c
+++ b/arch/sh/boards/of-generic.c
@@ -124,8 +124,6 @@ static void __init sh_of_time_init(void)

 static void __init sh_of_setup(char **cmdline_p)
 {
-	struct device_node *root;
-
 #ifdef CONFIG_USE_BUILTIN_DTB
 	unflatten_and_copy_device_tree();
 #else
@@ -135,11 +133,7 @@ static void __init sh_of_setup(char **cmdline_p)
 	board_time_init = sh_of_time_init;

 	sh_mv.mv_name = "Unknown SH model";
-	root = of_find_node_by_path("/");
-	if (root) {
-		of_property_read_string(root, "model", &sh_mv.mv_name);
-		of_node_put(root);
-	}
+	of_machine_get_model_name(&sh_mv.mv_name);

 	sh_of_smp_probe();
 }
diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index 6af7a11f09a5..94aef0465451 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -132,7 +132,7 @@ EXPORT_SYMBOL(fsl_guts_get_svr);

 static int fsl_guts_probe(struct platform_device *pdev)
 {
-	struct device_node *root, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	const struct fsl_soc_die_attr *soc_die;
@@ -152,11 +152,7 @@ static int fsl_guts_probe(struct platform_device *pdev)
 		return PTR_ERR(guts->regs);

 	/* Register soc device */
-	root = of_find_node_by_path("/");
-	if (of_property_read_string(root, "model", &machine))
-		of_property_read_string_index(root, "compatible", 0, &machine);
-	of_node_put(root);
-	if (machine)
+	if (!of_machine_get_model_name(&machine))
 		soc_dev_attr.machine = devm_kstrdup(dev, machine, GFP_KERNEL);

 	svr = fsl_guts_get_svr();
diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
index 330960312296..d9a119073de5 100644
--- a/drivers/soc/renesas/renesas-soc.c
+++ b/drivers/soc/renesas/renesas-soc.c
@@ -228,9 +228,7 @@ static int __init renesas_soc_init(void)
 	if (!soc_dev_attr)
 		return -ENOMEM;

-	np = of_find_node_by_path("/");
-	of_property_read_string(np, "model", &soc_dev_attr->machine);
-	of_node_put(np);
+	of_machine_get_model_name(&soc_dev_attr->machine);

 	soc_dev_attr->family = kstrdup_const(family->name, GFP_KERNEL);
 	soc_dev_attr->soc_id = kstrdup_const(strchr(match->compatible, ',') + 1,
--
2.7.4

^ permalink raw reply related

* [PATCH 1/2] of: base: add support to get machine model name
From: Sudeep Holla @ 2016-11-17 15:32 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Sudeep Holla, Rob Herring, Arnd Bergmann,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Frank Rowand

Currently platforms/drivers needing to get the machine model name are
replicating the same snippet of code. In some case, the OF reference
counting is either missing or incorrect.

This patch adds support to read the machine model name either using
the "model" or the "compatible" property in the device tree root node
to the core OF/DT code.

This can be used to remove all the duplicate code snippets doing exactly
same thing later.

Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
 drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
 include/linux/of.h |  6 ++++++
 2 files changed, 38 insertions(+)

Hi Rob,

It would be good if we can target this for v4.10, so that we have no
dependencies to push PATCH 2/2 in v4.11

Regards,
Sudeep

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a0bccb54a9bd..0810c5ecf1aa 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat)
 EXPORT_SYMBOL(of_machine_is_compatible);

 /**
+ * of_machine_get_model_name - Find and read the model name or the compatible
+ *		value for the machine.
+ * @model:	pointer to null terminated return string, modified only if
+ *		return value is 0.
+ *
+ * Returns a string containing either the model name or the compatible value
+ * of the machine if found, else return error.
+ *
+ * Search for a machine model name or the compatible if model name is missing
+ * in a device tree node and retrieve a null terminated string value (pointer
+ * to data, not a copy). Returns 0 on success, -EINVAL if root of the device
+ * tree is not found and other error returned by of_property_read_string on
+ * failure.
+ */
+int of_machine_get_model_name(const char **model)
+{
+	int error;
+
+	if (!of_node_get(of_root))
+		return -EINVAL;
+
+	error = of_property_read_string(of_root, "model", model);
+	if (error)
+		error = of_property_read_string_index(of_root, "compatible",
+						      0, model);
+	of_node_put(of_root);
+
+	return error;
+}
+EXPORT_SYMBOL(of_machine_get_model_name);
+
+/**
  *  __of_device_is_available - check if a device is available for use
  *
  *  @device: Node to check for availability, with locks already held
diff --git a/include/linux/of.h b/include/linux/of.h
index d72f01009297..13fc66531f1b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);

 extern int of_machine_is_compatible(const char *compat);
+extern int of_machine_get_model_name(const char **model);

 extern int of_add_property(struct device_node *np, struct property *prop);
 extern int of_remove_property(struct device_node *np, struct property *prop);
@@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat)
 	return 0;
 }

+static inline int of_machine_get_model_name(const char **model)
+{
+	return -EINVAL;
+}
+
 static inline bool of_console_check(const struct device_node *dn, const char *name, int index)
 {
 	return false;
--
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 -next] ARM: dts: omap5: replace gpio-key,wakeup with wakeup-source property
From: Sudeep Holla @ 2016-11-17 15:32 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: devicetree, linux-omap, Benoît Cousson, linux-arm-kernel,
	Sudeep Holla
In-Reply-To: <20161117152717.GS4082@atomide.com>



On 17/11/16 15:27, Tony Lindgren wrote:
> * Sudeep Holla <sudeep.holla@arm.com> [161114 07:44]:
>> Though the keyboard driver for GPIO buttons(gpio-keys) will continue to
>> check for/support the legacy "gpio-key,wakeup" boolean property to
>> enable gpio buttons as wakeup source, "wakeup-source" is the new
>> standard binding.
>>
>> This patch replaces the legacy "gpio-key,wakeup" with the unified
>> "wakeup-source" property in order to avoid any further copy-paste
>> duplication.
>>
>> Cc: "Benoît Cousson" <bcousson@baylibre.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  arch/arm/boot/dts/omap5-uevm.dts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Hi,
>>
>> Inspite of getting rid of most of the legacy property almost a year ago,
>> addition of new platforms have brought this back and over time it's
>> now found again in few places. Just get rid of them *again*
>
> Thanks for the annual check up :)
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
> Please let me know if you want me to pick this up instead.

Yes that would be better. Also it's present only in your -next,
looks like something newly added via commit 2d46c0c60725 ("ARM:
dts: omap5 uevm: add USR1 button")

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH -next] ARM: dts: omap5: replace gpio-key,wakeup with wakeup-source property
From: Tony Lindgren @ 2016-11-17 15:27 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: devicetree, linux-omap, Benoît Cousson, linux-arm-kernel
In-Reply-To: <1479138250-17780-1-git-send-email-sudeep.holla@arm.com>

* Sudeep Holla <sudeep.holla@arm.com> [161114 07:44]:
> Though the keyboard driver for GPIO buttons(gpio-keys) will continue to
> check for/support the legacy "gpio-key,wakeup" boolean property to
> enable gpio buttons as wakeup source, "wakeup-source" is the new
> standard binding.
> 
> This patch replaces the legacy "gpio-key,wakeup" with the unified
> "wakeup-source" property in order to avoid any further copy-paste
> duplication.
> 
> Cc: "Benoît Cousson" <bcousson@baylibre.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm/boot/dts/omap5-uevm.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Hi,
> 
> Inspite of getting rid of most of the legacy property almost a year ago,
> addition of new platforms have brought this back and over time it's
> now found again in few places. Just get rid of them *again*

Thanks for the annual check up :)

Acked-by: Tony Lindgren <tony@atomide.com>

Please let me know if you want me to pick this up instead.

> diff --git a/arch/arm/boot/dts/omap5-uevm.dts b/arch/arm/boot/dts/omap5-uevm.dts
> index 2fcdc516da45..a8c72611fbe3 100644
> --- a/arch/arm/boot/dts/omap5-uevm.dts
> +++ b/arch/arm/boot/dts/omap5-uevm.dts
> @@ -41,7 +41,7 @@
>  			label = "BTN1";
>  			linux,code = <169>;
>  			gpios = <&gpio3 19 GPIO_ACTIVE_LOW>;	/* gpio3_83 */
> -			gpio-key,wakeup;
> +			wakeup-source;
>  			autorepeat;
>  			debounce_interval = <50>;
>  		};
> --
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH V8 2/6] thermal: bcm2835: add thermal driver for bcm2835 soc
From: Eduardo Valentin @ 2016-11-17 15:10 UTC (permalink / raw)
  To: Martin Sperl
  Cc: Mark Rutland, devicetree, Florian Fainelli, Russell King,
	Pawel Moll, Stephen Warren, Catalin Marinas, linux-pm, Lee Jones,
	Will Deacon, Eric Anholt, Rob Herring, linux-rpi-kernel,
	Zhang Rui, linux-arm-kernel
In-Reply-To: <766e1b70-d83a-eb52-fa2b-aec435e85673@martin.sperl.org>

Hello Martin,

On Thu, Nov 17, 2016 at 10:51:33AM +0100, Martin Sperl wrote:
> 
> 
> On 17.11.2016 03:11, Eduardo Valentin wrote:
> >Hey Martin,
> >
> >Very sorry for the late feedback. Not so sure if this one got queued
> >already or not. Anyways, just minor questions as follows:
> >
> >On Wed, Nov 02, 2016 at 10:18:22AM +0000, kernel@martin.sperl.org wrote:
> >>From: Martin Sperl <kernel@martin.sperl.org>
> >>
> >>Add basic thermal driver for bcm2835 SOC.
> >>
> >>This driver currently relies on the firmware setting up the
> >>tsense HW block and does not set it up itself.
> >>
> >>Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> >>Acked-by: Eric Anholt <eric@anholt.net>
> >>Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>
> ...
> >>+static int bcm2835_thermal_adc2temp(
> >>+	const struct bcm2835_thermal_info *info, u32 adc)
> >>+{
> >>+	return info->offset + (adc * info->slope);
> >
> >Any specific reason we cannot use thermal_zone_params->slope and
> >thermal_zone_params->offset?
> 
> You could - the patch was just rebased to 4.9 and those slope and
> offset just got merged during this cycle.
> 
> Do we really need to modify it - the patch has been around since 4.6.
> 
> >>+
> >>+static int bcm2835_thermal_get_trip_temp(
> >>+	struct thermal_zone_device *tz, int trip, int *temp)
> >>+{
> >>+	struct bcm2835_thermal_data *data = tz->devdata;
> >>+	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
> >>+
> >>+	/* get the THOLD bits */
> >>+	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
> >>+	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
> >>+
> >>+	/* if it is zero then use the info value */
> >>+	if (val)
> >
> >Is this a read only register or is this driver supposed to program it?
> >In which scenario it would be 0? Can this be added as comments?
> 
> It is RW, but the Firmware typically sets up the thermal device with the
> correct values already - this is just a fallback.

OK, but how do you differentiate from a buggy firmware or misconfigured
hardware? How do you know if the firmware is supposed to be loaded and
ready? There is no firmware loading in this driver. Also, there is no
dependency with a driver that loads firmware, or at least, I missed it.

> 
> >>+static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
> >>+				    int *temp)
> >>+{
> >>+	struct bcm2835_thermal_data *data = tz->devdata;
> >>+	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
> >>+
> >>+	if (!(val & BCM2835_TS_TSENSSTAT_VALID))
> >
> >What cases you would get the valid bit not set? Do you need to wait for
> >the conversion to finish?
> 
> I guess: if you have just enabled the HW-block (which the FW does much
> in advance) and start to read the value immediately (before the first sample
> period has finished), then this will not be valid.
> 

Then again, how does this driver make sure the initialization procedure
is correct, and that by the time it is using the hardware, the hardware
is in a sane state?

Back to the original question, does it mean the hardware is in
some sort of continuous ADC conversion mode or reading the temperature
requires specific steps to get the conversion done, and therefore you
are checking the valid bit here?



> So do you need another version of the patchset that uses that new API?

I think the API usage is change that can be done together with
clarification for the above questions too: on hardware state,
firmware loading, maybe a master driver dependency, and the ADC
conversion sequence, which are not well clear to me on this driver. As long as
this is clarified and documented in the code (can be simple comments so
it is clear to whoever reads in the future), then I would be OK with
this driver. 

> 
> Thanks,
> 	Martin

^ permalink raw reply

* Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion
From: Guenter Roeck @ 2016-11-17 15:06 UTC (permalink / raw)
  To: Tom Levens
  Cc: jdelvare, robh+dt, mark.rutland, linux-kernel, linux-hwmon,
	devicetree
In-Reply-To: <1479384616-12479-1-git-send-email-tom.levens@cern.ch>

Hi Tom,

On 11/17/2016 04:10 AM, Tom Levens wrote:
> Conversion from raw values to signed integers has been refactored using
> the macros in bitops.h.
>
Please also mention that this fixes a bug in negative temperature conversions.

> Signed-off-by: Tom Levens <tom.levens@cern.ch>
> ---
>  drivers/hwmon/ltc2990.c |   27 ++++++++++-----------------
>  1 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> index 8f8fe05..0ec4102 100644
> --- a/drivers/hwmon/ltc2990.c
> +++ b/drivers/hwmon/ltc2990.c
> @@ -9,8 +9,12 @@
>   * This driver assumes the chip is wired as a dual current monitor, and
>   * reports the voltage drop across two series resistors. It also reports
>   * the chip's internal temperature and Vcc power supply voltage.
> + *
> + * Value conversion refactored
> + * by Tom Levens <tom.levens@cern.ch>

Kind of unusual to do that for minor changes like this. Imagine if everyone would do that.
The commit log is what gives you credit.

>   */
>
> +#include <linux/bitops.h>
>  #include <linux/err.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -34,19 +38,10 @@
>  #define LTC2990_CONTROL_MODE_CURRENT	0x06
>  #define LTC2990_CONTROL_MODE_VOLTAGE	0x07
>
> -/* convert raw register value to sign-extended integer in 16-bit range */
> -static int ltc2990_voltage_to_int(int raw)
> -{
> -	if (raw & BIT(14))
> -		return -(0x4000 - (raw & 0x3FFF)) << 2;
> -	else
> -		return (raw & 0x3FFF) << 2;
> -}
> -
>  /* Return the converted value from the given register in uV or mC */
> -static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
> +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 *result)
>  {
> -	int val;
> +	s32 val;

Please just leave the variable type alone. it is also used for the return value
from i2c_smbus_read_word_swapped(), which is an int, and changing it to s32 doesn't
really make the code better.

Can you send me a register map for the chip ? I would like to write a module test.

Thanks,
Guenter

^ permalink raw reply

* Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2
From: Jean-Francois Moine @ 2016-11-17 14:57 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Dave Airlie, Liam Girdwood, Mark Brown, Rob Herring,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20161116213306.s5o5zi7pgppwuq7t@lukather>

On Wed, 16 Nov 2016 22:33:06 +0100
Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> > > > The Device Engine just handles the planes of the LCDs, but, indeed,
> > > > the LCDs must know about the DE and the DE must know about the LCDs.
> > > > There are 2 ways to realize this knowledge in the DT:
> > > > 1) either the DE has one or two phandle's to the LCDs,
> > > > 2) or the LCDs have a phandle to the DE.
> > > > 
> > > > I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
> > > > which is part of the video link (OF-graph LCD <-> connector).
> > > > It would be possible to have phandles to the LCDs themselves, but this
> > > > asks for more code.
> > > > 
> > > > The second way is also possible, but it also complexifies a bit the
> > > > exchanges DE <-> LCD.
> > > 
> > > I'm still not sure how it would complexify anything, and why you can't
> > > use the display graph to model the relation between the display engine
> > > and the TCON (and why you want to use a generic property that refers
> > > to the of-graph while it really isn't).
> > 
> > Complexification:
> > 1- my solution:
> >   At startup time, the DE device is the DRM device.
> 
> How do you deal with SoCs with multiple display engines then?

In the H3, A83T and A64, there is only one DE.
If many DEs in a SoC, there may be either one DRM device per DE or one
DRM device for the whole system. In this last case, the (global) DE
would have many resources (many I/O memory maps / IRQs..) and the
physical DE of each endpoint would be indicated by the position of its
phandle in the 'ports' array (DT documentation).

> > It has to know the devices entering in the video chains.
> >   The CRTCs (LCD/TCON) are found by
> > 	ports[i] -> parent
> >   The connectors are found by
> > 	ports[i] -> endpoint -> remote_endpoint -> parent
> > 2- with ports pointing to the LCDs:
> >   The CRTCs (LCD/TCON) are simply
> > 	ports[i]
> >   The connectors are found by
> > 	loop on all ports of ports[i]
> > 		ports[i][j] -> endpoint -> remote_endpoint -> parent
> > 3- with a phandle to the DE in the LCDs:
> 
> What do you mean with LCD? Panels? Why would panels have a phandle to
> the display engine?

LCD is the same as CRTC. I don't think people will connect old CRT's to
their new ARM boards. LCD's may be panels, modern TV sets, or any
digital display. The word LCD seems clearer to me in this context, even
if there may a DAC as an ancoder.

> >   The DE cannot be the DRM device because there is no information about
> >   the video devices in the DT. Then, the DRM devices are the LCDs.
> >   These LCDs must give their indices to the DE. So, the DE must implement
> >   some callback function to accept a LCD definition, and there must be
> >   a list of DEs in the driver to make the association DE <-> LCD[i]
> >   Some more problem may be raised if a user wants to have the same frame
> >   buffer on the 2 LCDs of a DE.
> 
> I have no idea what you're talking about, sorry.

Here is the DT (I am using back 'CRTC'):

	de: display-controller@xxx {
		...
	};
	crtc0: crt-controller@xxx{
		...
		display-controller = <&de>;
		ports {
			...		/* to the crtc0 connectors */
		}
	};
	crtc1: crt-controller@xxx{
		...
		display-controller = <&de>;
		ports {
			...		/* to the crtc1 connectors */
		};
	};

There are 2 DRM devices: one on crtc0, the other one on crtc1.
The DE device is isolated. But, to treat the planes, it must receive
information about the CRTCs. How?

> > Anyway, my solution is already used in the IMX Soc.
> > See 'display-subsystem' in arch/arm/boot/dts/imx6q.dtsi for an example.
> 
> Pointing out random example in the tree doesn't make a compelling
> argument.

This is not a random example. There was a thread about the 'ports'
phandle in the DT definition of the IMX video subsystem, and what kind
of OF function should be used (see one of my previous mails). In the DRI
list, nobody objected about the phandle by itself.

> > > > > > > Panel functions? In the CRTC driver?
> > > > > > 
> > > > > > Yes, dumb panel.
> > > > > 
> > > > > What do you mean by that? Using a Parallel/RGB interface?
> > > > 
> > > > Sorry, I though this was a well-known name. The 'dump panel' was used
> > > > in the documentation of my previous ARM machine as the video frame sent
> > > > to the HDMI controller. 'video_frame' is OK for you?
> > > 
> > > If it's the frame sent to the encoder, then it would be the CRTC by
> > > DRM's nomenclature.
> > 
> > The CRTC is a software entity. The frame buffer is a hardware entity.
> 
> Why are you about framebuffer now, this is nowhere in that
> discussion. Any way, the framebuffer is also what is put in a plane,
> so there's a name collision here, and you'll probably want to change
> it.
> 
> Judging by the previous discussion, this should really be called
> encoder if it encodes the frames on a bus format, or CRTC if it
> composes the planes.

I think that we will end in agreeing on the words. We just need some
time!
Here is how I understand the Allwinner's DE2:

- the TCON is the piece of hardware which gets some memory area and
  sends it to a bus according to some configuation (screen resolution,
  timings...). The bus data are encoded and transmitted to the connector.
  At the end, the display device receives frames. So, going back to the
  TCON, the memory are is the frame buffer.

- this frame buffer is virtual, empty, 'dumb', it is a dumb panel!
  It is filled by planes. This job is done by a specific image
  processor, the one contained in the DE2.

- the DE2 processor gets the plane source images from the SoC memory.
  It adjusts the images according to many configuration parameters and
  includes the result into the frame buffer.

So:
	LCD = CRTC = frame buffer = dumb panel = TCON

- LCD = hardware piece of a display device (terminal side) which renders
  the colored pixels in a digital way. By extension, the hardware part
  (computer side) of a display engine which handles the definitions of
  a digital or analog physical display device. By extension also,
  the software structure (driver side) which defines a physical screen.

- CRTC = DRM software entity which handles the definitions of a screen,
  but not just a CRT.

- frame buffer = piece of memory which contains the images which are
  sent to a screen.

- dumb panel = abstract entity which defines the characteristics of a
  physical screen.

You may note that, in the DE2 scheme, the TCON and LCD are not in the
same (software) device while they are part of the same DRM software
entity, the CTRC.

> > > > > If it is similar, I think hardcoding the pipe number is pretty bad,
> > > > > because that would restrict the combination of planes and formats,
> > > > > while some other might have worked.
> > > > 
> > > > From what I understood about the DE2, the pipes just define the priority
> > > > of the overlay channels (one pipe for one channel).
> > > > With the cursor constraint, there must be at least 2 channels in
> > > > order (primary, cursor). Then, with these 2 channels/pipes, there can be
> > > > 6 so-called overlay planes (3 RGB/YUV and 3 RGB only).
> > > > Enabling the pipes 2 and 3 (LCD 0 only) would offer 8 more planes, but
> > > > RGB only. Then, it might be useful to have dynamic pipes.
> > > 
> > > That's very valuable (and definitely should go into a comment),
> > > thanks!
> > > 
> > > I still believe that's it should be into a (simple at first)
> > > atomic_check. That would be easier to extend and quite easy to
> > > document and get simply by looking at the code.
> > 
> > Sorry for I don't understand what you mean.
> 
> You can check all the constraints you exposed above in atomic_check
> instead of hardcoding it.

Sorry, but I don't like to run useless code for pure static definition.

> > The DE and the LCDs are different devices on different drivers.
> > A DE must be only one device because it has to handle concurent
> > accesses from its 2 LCDs. Then 2 drivers.
> 
> If it's different drivers, why are they in the same module?
> 
> > But only one module. Why? Because there cannot be double direction
> > calls from one module to an other one, and, in our case, for example,
> > - the DRM (DE) device must call vblank functions which are handled in
> >   the CRTC (TCON) device, and
> > - the CRTC device must call DE initialization functions at startup time.
> 
> I'm sorry, but that doesn't make any sense. The crtc device should
> take care of the CRTC functions. Your DRM CRTC and encoders can
> definitely be shared across different devices, you can have a look at
> how we're doing it for sun4i.
> 
> We basically have 3 drivers to create most of the display pipeline:
>   - One for the DRM device
>   - One for the display engine
>   - One for the TCON

Your DRM device is useless. It is simpler to have the DRM device as the
display engine.
Also, maybe, you have not the constraint the DE being shared between
2 CRTCs.

> Once they have all loaded and registered in the component framework,
> their bind callback is called, and it's when the DRM entities are
> created using exported functions to manipulate what needs to be setup.
> 
> It's definitely doable, it just takes some effort.

It seems you did not look at what I have coded...

> > On the other side, removing the cursor would just let one more plane.
> > Do we really need this one? In other words, I'd be pleased to know how
> > you run 7 applications doing video overlay.
> 
> You can use those planes to do composition too, with each application
> having one or more plane. Android uses that.
> 
> There's no argument to have a cursor plane, really. Even regular
> graphic stack like X can use a regular overlay to have its cursor on
> it. It doesn't *remove* anything, it just allows to use the plane for
> what it was supposed to be used for.

I'd be glad to know how you can have a hardware cursor without defining
it in drm_crtc_init_with_planes().

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* Re: [PATCHv2 2/4] ath10k: Add support to update btcoex priority value via nl80211
From: kbuild test robot @ 2016-11-17 14:36 UTC (permalink / raw)
  Cc: devicetree, robh, linux-wireless, linux-kernel, ath10k,
	Tamizh chelvam, tamizhchelvam, kbuild-all
In-Reply-To: <1479383064-25718-3-git-send-email-c_traja@qti.qualcomm.com>

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

Hi Tamizh,

[auto build test ERROR on ath6kl/ath-next]
[also build test ERROR on next-20161117]
[cannot apply to v4.9-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/c_traja-qti-qualcomm-com/ath10k-Add-support-for-BTCOEX-feature/20161117-200322
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git ath-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/net/wireless/ath/ath10k/mac.c:7506:35: warning: 'struct cfg80211_btcoex_priority' declared inside parameter list will not be visible outside of this definition or declaration
    ath10k_mac_get_btcoex_prio(struct cfg80211_btcoex_priority *btcoex_priority)
                                      ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_get_btcoex_prio':
>> drivers/net/wireless/ath/ath10k/mac.c:7510:21: error: dereferencing pointer to incomplete type 'struct cfg80211_btcoex_priority'
     if (btcoex_priority->wlan_be_preferred)
                        ^~
>> drivers/net/wireless/ath/ath10k/mac.c:7511:18: error: 'WIPHY_WLAN_BE_PREFERRED' undeclared (first use in this function)
      btcoex_prio |= WIPHY_WLAN_BE_PREFERRED;
                     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:7511:18: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/net/wireless/ath/ath10k/mac.c:7514:18: error: 'WIPHY_WLAN_BK_PREFERRED' undeclared (first use in this function)
      btcoex_prio |= WIPHY_WLAN_BK_PREFERRED;
                     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/ath/ath10k/mac.c:7517:18: error: 'WIPHY_WLAN_VI_PREFERRED' undeclared (first use in this function)
      btcoex_prio |= WIPHY_WLAN_VI_PREFERRED;
                     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/ath/ath10k/mac.c:7520:18: error: 'WIPHY_WLAN_VO_PREFERRED' undeclared (first use in this function)
      btcoex_prio |= WIPHY_WLAN_VO_PREFERRED;
                     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/ath/ath10k/mac.c:7523:18: error: 'WIPHY_WLAN_BEACON_PREFERRED' undeclared (first use in this function)
      btcoex_prio |= WIPHY_WLAN_BEACON_PREFERRED;
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/ath/ath10k/mac.c:7526:18: error: 'WIPHY_WLAN_MGMT_PREFERRED' undeclared (first use in this function)
      btcoex_prio |= WIPHY_WLAN_MGMT_PREFERRED;
                     ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c: At top level:
   drivers/net/wireless/ath/ath10k/mac.c:7532:11: warning: 'struct cfg80211_btcoex_priority' declared inside parameter list will not be visible outside of this definition or declaration
       struct cfg80211_btcoex_priority *btcoex_priority)
              ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_op_set_btcoex_priority':
>> drivers/net/wireless/ath/ath10k/mac.c:7551:43: error: passing argument 1 of 'ath10k_mac_get_btcoex_prio' from incompatible pointer type [-Werror=incompatible-pointer-types]
     btcoex_prio = ath10k_mac_get_btcoex_prio(btcoex_priority);
                                              ^~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:7506:1: note: expected 'struct cfg80211_btcoex_priority *' but argument is of type 'struct cfg80211_btcoex_priority *'
    ath10k_mac_get_btcoex_prio(struct cfg80211_btcoex_priority *btcoex_priority)
    ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c: At top level:
   drivers/net/wireless/ath/ath10k/mac.c:7611:2: error: unknown field 'set_btcoex' specified in initializer
     .set_btcoex                     = ath10k_mac_op_set_btcoex,
     ^
   drivers/net/wireless/ath/ath10k/mac.c:7611:36: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .set_btcoex                     = ath10k_mac_op_set_btcoex,
                                       ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:7611:36: note: (near initialization for 'ath10k_ops.reconfig_complete')
>> drivers/net/wireless/ath/ath10k/mac.c:7612:2: error: unknown field 'set_btcoex_priority' specified in initializer
     .set_btcoex_priority  = ath10k_mac_op_set_btcoex_priority,
     ^
   drivers/net/wireless/ath/ath10k/mac.c:7612:26: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .set_btcoex_priority  = ath10k_mac_op_set_btcoex_priority,
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:7612:26: note: (near initialization for 'ath10k_ops.ipv6_addr_change')
   drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_register':
>> drivers/net/wireless/ath/ath10k/mac.c:8203:16: error: 'struct wiphy' has no member named 'btcoex_support_flags'
      ar->hw->wiphy->btcoex_support_flags =
                   ^~
   drivers/net/wireless/ath/ath10k/mac.c:8204:4: error: 'WIPHY_WLAN_BE_PREFERRED' undeclared (first use in this function)
       WIPHY_WLAN_BE_PREFERRED |
       ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:8205:4: error: 'WIPHY_WLAN_BK_PREFERRED' undeclared (first use in this function)
       WIPHY_WLAN_BK_PREFERRED |
       ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:8206:4: error: 'WIPHY_WLAN_VI_PREFERRED' undeclared (first use in this function)
       WIPHY_WLAN_VI_PREFERRED |
       ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:8207:4: error: 'WIPHY_WLAN_VO_PREFERRED' undeclared (first use in this function)
       WIPHY_WLAN_VO_PREFERRED |
       ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:8208:4: error: 'WIPHY_WLAN_BEACON_PREFERRED' undeclared (first use in this function)
       WIPHY_WLAN_BEACON_PREFERRED |
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:8209:4: error: 'WIPHY_WLAN_MGMT_PREFERRED' undeclared (first use in this function)
       WIPHY_WLAN_MGMT_PREFERRED;
       ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireless/ath/ath10k/mac.c:8211:20: error: 'struct wiphy' has no member named 'btcoex_support_flags'
          ar->hw->wiphy->btcoex_support_flags);
                       ^~
   cc1: some warnings being treated as errors

vim +7510 drivers/net/wireless/ath/ath10k/mac.c

  7500		mutex_unlock(&ar->conf_mutex);
  7501	
  7502		return ret;
  7503	}
  7504	
  7505	u32
> 7506	ath10k_mac_get_btcoex_prio(struct cfg80211_btcoex_priority *btcoex_priority)
  7507	{
  7508		u32 btcoex_prio = 0;
  7509	
> 7510		if (btcoex_priority->wlan_be_preferred)
> 7511			btcoex_prio |= WIPHY_WLAN_BE_PREFERRED;
  7512	
  7513		if (btcoex_priority->wlan_bk_preferred)
> 7514			btcoex_prio |= WIPHY_WLAN_BK_PREFERRED;
  7515	
  7516		if (btcoex_priority->wlan_vi_preferred)
> 7517			btcoex_prio |= WIPHY_WLAN_VI_PREFERRED;
  7518	
  7519		if (btcoex_priority->wlan_vo_preferred)
> 7520			btcoex_prio |= WIPHY_WLAN_VO_PREFERRED;
  7521	
  7522		if (btcoex_priority->wlan_beacon_preferred)
> 7523			btcoex_prio |= WIPHY_WLAN_BEACON_PREFERRED;
  7524	
  7525		if (btcoex_priority->wlan_mgmt_preferred)
> 7526			btcoex_prio |= WIPHY_WLAN_MGMT_PREFERRED;
  7527	
  7528		return btcoex_prio;
  7529	}
  7530	
  7531	static int ath10k_mac_op_set_btcoex_priority(struct ieee80211_hw *hw,
  7532				struct cfg80211_btcoex_priority *btcoex_priority)
  7533	{
  7534		u32 btcoex_prio;
  7535		struct ath10k *ar = hw->priv;
  7536		int ret;
  7537	
  7538		if (!(test_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags))) {
  7539			ret = -EINVAL;
  7540			goto exit;
  7541		}
  7542	
  7543		mutex_lock(&ar->conf_mutex);
  7544	
  7545		if (ar->state != ATH10K_STATE_ON &&
  7546		    ar->state != ATH10K_STATE_RESTARTED) {
  7547			ret = -ENETDOWN;
  7548			goto exit;
  7549		}
  7550	
> 7551		btcoex_prio = ath10k_mac_get_btcoex_prio(btcoex_priority);
  7552	
  7553		if (btcoex_prio > 0x3f) {
  7554			ret = -E2BIG;
  7555			goto exit;
  7556		}
  7557	
  7558		ret = ath10k_wmi_set_coex_param(ar, btcoex_prio);
  7559	
  7560		if (ret) {
  7561			ath10k_warn(ar, "failed to set btcoex priority: %d\n", ret);
  7562			goto exit;
  7563		}
  7564	
  7565	exit:
  7566		mutex_unlock(&ar->conf_mutex);
  7567		return ret;
  7568	}
  7569	
  7570	static const struct ieee80211_ops ath10k_ops = {
  7571		.tx				= ath10k_mac_op_tx,
  7572		.wake_tx_queue			= ath10k_mac_op_wake_tx_queue,
  7573		.start				= ath10k_start,
  7574		.stop				= ath10k_stop,
  7575		.config				= ath10k_config,
  7576		.add_interface			= ath10k_add_interface,
  7577		.remove_interface		= ath10k_remove_interface,
  7578		.configure_filter		= ath10k_configure_filter,
  7579		.bss_info_changed		= ath10k_bss_info_changed,
  7580		.set_coverage_class		= ath10k_mac_op_set_coverage_class,
  7581		.hw_scan			= ath10k_hw_scan,
  7582		.cancel_hw_scan			= ath10k_cancel_hw_scan,
  7583		.set_key			= ath10k_set_key,
  7584		.set_default_unicast_key        = ath10k_set_default_unicast_key,
  7585		.sta_state			= ath10k_sta_state,
  7586		.conf_tx			= ath10k_conf_tx,
  7587		.remain_on_channel		= ath10k_remain_on_channel,
  7588		.cancel_remain_on_channel	= ath10k_cancel_remain_on_channel,
  7589		.set_rts_threshold		= ath10k_set_rts_threshold,
  7590		.set_frag_threshold		= ath10k_mac_op_set_frag_threshold,
  7591		.flush				= ath10k_flush,
  7592		.tx_last_beacon			= ath10k_tx_last_beacon,
  7593		.set_antenna			= ath10k_set_antenna,
  7594		.get_antenna			= ath10k_get_antenna,
  7595		.reconfig_complete		= ath10k_reconfig_complete,
  7596		.get_survey			= ath10k_get_survey,
  7597		.set_bitrate_mask		= ath10k_mac_op_set_bitrate_mask,
  7598		.sta_rc_update			= ath10k_sta_rc_update,
  7599		.get_tsf			= ath10k_get_tsf,
  7600		.set_tsf			= ath10k_set_tsf,
  7601		.ampdu_action			= ath10k_ampdu_action,
  7602		.get_et_sset_count		= ath10k_debug_get_et_sset_count,
  7603		.get_et_stats			= ath10k_debug_get_et_stats,
  7604		.get_et_strings			= ath10k_debug_get_et_strings,
  7605		.add_chanctx			= ath10k_mac_op_add_chanctx,
  7606		.remove_chanctx			= ath10k_mac_op_remove_chanctx,
  7607		.change_chanctx			= ath10k_mac_op_change_chanctx,
  7608		.assign_vif_chanctx		= ath10k_mac_op_assign_vif_chanctx,
  7609		.unassign_vif_chanctx		= ath10k_mac_op_unassign_vif_chanctx,
  7610		.switch_vif_chanctx		= ath10k_mac_op_switch_vif_chanctx,
> 7611		.set_btcoex                     = ath10k_mac_op_set_btcoex,
> 7612		.set_btcoex_priority		= ath10k_mac_op_set_btcoex_priority,
  7613	
  7614		CFG80211_TESTMODE_CMD(ath10k_tm_cmd)
  7615	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56822 bytes --]

[-- Attachment #3: Type: text/plain, Size: 146 bytes --]

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ 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