Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v3 10/12] arm64: allwinner: a64: add DTSI file for AXP803 PMIC
From: Maxime Ripard @ 2017-04-18  7:08 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Liam Girdwood, Mark Brown,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170417115747.7300-11-icenowy-h8G6r0blFSE@public.gmane.org>

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

On Mon, Apr 17, 2017 at 07:57:45PM +0800, Icenowy Zheng wrote:
> As nearly all A64 boards are using AXP803 PMIC, add a DTSI file for it,
> like the old DTSI files for AXP20x/22x, for the common parts of the
> PMIC.
> 
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
>  arch/arm64/boot/dts/allwinner/axp803.dtsi | 150 ++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/axp803.dtsi
> 
> diff --git a/arch/arm64/boot/dts/allwinner/axp803.dtsi b/arch/arm64/boot/dts/allwinner/axp803.dtsi
> new file mode 100644
> index 000000000000..f0e53a7fffbd
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/axp803.dtsi
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright 2017 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2 of the
> + *     License, or (at your option) any later version.
> + *
> + *     This file 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.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/*
> + * AXP803 Integrated Power Management Chip
> + * http://files.pine64.org/doc/datasheet/pine64/AXP803_Datasheet_V1.0.pdf
> + */
> +
> +&axp803 {
> +	interrupt-controller;
> +	#interrupt-cells = <1>;
> +
> +	regulators {
> +		/* Default work frequency for buck regulators */
> +		x-powers,dcdc-freq = <3000>;
> +
> +		reg_dcdc1: dcdc1 {
> +			regulator-name = "dcdc1";
> +		};
> +
> +		reg_dcdc2: dcdc2 {
> +			regulator-name = "dcdc2";
> +		};
> +
> +		reg_dcdc3: dcdc3 {
> +			regulator-name = "dcdc3";
> +		};
> +
> +		reg_dcdc4: dcdc4 {
> +			regulator-name = "dcdc4";
> +		};
> +
> +		reg_dcdc5: dcdc5 {
> +			regulator-name = "dcdc5";
> +		};
> +
> +		reg_dcdc6: dcdc6 {
> +			regulator-name = "dcdc6";
> +		};
> +
> +		reg_dc1sw: dc1sw {
> +			regulator-name = "dc1sw";
> +		};
> +
> +		reg_aldo1: aldo1 {
> +			regulator-name = "aldo1";
> +		};
> +
> +		reg_aldo2: aldo2 {
> +			regulator-name = "aldo2";
> +		};
> +
> +		reg_aldo3: aldo3 {
> +			regulator-name = "aldo3";
> +		};

Please sort them by alphabetical order.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

^ permalink raw reply

* Re: [PATCH v3 10/12] arm64: allwinner: a64: add DTSI file for AXP803 PMIC
From: Chen-Yu Tsai @ 2017-04-18  7:20 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Maxime Ripard,
	Liam Girdwood, Mark Brown, devicetree, linux-kernel,
	linux-arm-kernel, linux-sunxi
In-Reply-To: <20170417115747.7300-11-icenowy-h8G6r0blFSE@public.gmane.org>

Hi,

On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> As nearly all A64 boards are using AXP803 PMIC, add a DTSI file for it,
> like the old DTSI files for AXP20x/22x, for the common parts of the
> PMIC.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
>  arch/arm64/boot/dts/allwinner/axp803.dtsi | 150 ++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/axp803.dtsi
>
> diff --git a/arch/arm64/boot/dts/allwinner/axp803.dtsi b/arch/arm64/boot/dts/allwinner/axp803.dtsi
> new file mode 100644
> index 000000000000..f0e53a7fffbd
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/axp803.dtsi
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright 2017 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2 of the
> + *     License, or (at your option) any later version.
> + *
> + *     This file 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.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/*
> + * AXP803 Integrated Power Management Chip
> + * http://files.pine64.org/doc/datasheet/pine64/AXP803_Datasheet_V1.0.pdf
> + */
> +
> +&axp803 {
> +       interrupt-controller;
> +       #interrupt-cells = <1>;
> +
> +       regulators {
> +               /* Default work frequency for buck regulators */
> +               x-powers,dcdc-freq = <3000>;
> +
> +               reg_dcdc1: dcdc1 {
> +                       regulator-name = "dcdc1";
> +               };
> +
> +               reg_dcdc2: dcdc2 {
> +                       regulator-name = "dcdc2";
> +               };
> +
> +               reg_dcdc3: dcdc3 {
> +                       regulator-name = "dcdc3";
> +               };
> +
> +               reg_dcdc4: dcdc4 {
> +                       regulator-name = "dcdc4";
> +               };
> +
> +               reg_dcdc5: dcdc5 {
> +                       regulator-name = "dcdc5";
> +               };
> +
> +               reg_dcdc6: dcdc6 {
> +                       regulator-name = "dcdc6";
> +               };
> +
> +               reg_dc1sw: dc1sw {
> +                       regulator-name = "dc1sw";
> +               };
> +
> +               reg_aldo1: aldo1 {
> +                       regulator-name = "aldo1";
> +               };
> +
> +               reg_aldo2: aldo2 {
> +                       regulator-name = "aldo2";
> +               };
> +
> +               reg_aldo3: aldo3 {
> +                       regulator-name = "aldo3";
> +               };
> +
> +               reg_dldo1: dldo1 {
> +                       regulator-name = "dldo1";
> +               };
> +
> +               reg_dldo2: dldo2 {
> +                       regulator-name = "dldo2";
> +               };
> +
> +               reg_dldo3: dldo3 {
> +                       regulator-name = "dldo3";
> +               };
> +
> +               reg_dldo4: dldo4 {
> +                       regulator-name = "dldo4";
> +               };
> +
> +               reg_eldo1: eldo1 {
> +                       regulator-name = "eldo1";
> +               };
> +
> +               reg_eldo2: eldo2 {
> +                       regulator-name = "eldo2";
> +               };
> +
> +               reg_eldo3: eldo3 {
> +                       regulator-name = "eldo3";
> +               };
> +
> +               reg_fldo1: fldo1 {
> +                       regulator-name = "fldo1";
> +               };
> +
> +               reg_fldo2: fldo2 {
> +                       regulator-name = "fldo2";
> +               };
> +
> +               reg_ldo_io0: ldo_io0 {
> +                       regulator-name = "ldo_io0";

Please use hyphens instead of underscores for the node name,
and preferably, for the regulator name as well.

ChenYu

^ permalink raw reply

* [PATCH v6 0/6] add support for AXP20X and AXP22X battery power supply driver
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

This is a series for AXP20X and AXP22X battery power supply without the
support for changing constant charge current from the DT. The patches for
supporting the constant charge current from DT are ready and will be sent
once the battery framework required to make this work has been merged.

v6:
  - removed mention to fixed battery in DT-binding documentation,

Quote from v4 cover letter:

The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose
information and data of the various power supplies they support such as
ACIN, battery and VBUS. For example, they expose the current battery
voltage, charge or discharge, as well as ACIN and VBUS current voltages
and currents, internal PMIC temperature and ADC on 2 different GPIOs
when in the right mode (for the AXP209 only).

The ACIN power supply driver is added by this patch. The AXP20X and
AXP22X can both read the status and the "usability" of the power supply
but only the AXP209 will be able to tell the current current and voltage
of the power supply by reading ADC channels. It is simply not supported
by the AXP22X PMICs.

The battery power supply driver is also added by this patch. The AXP20X
and AXP22X share most of their behaviour but have slight variations. The
allowed target voltages for battery charging are not the same, the
AXP22X PMIC are able to tell if the battery percentage computed by the
PMIC is trustworthy and they have different formulas for computing max
current for battery power supply. The driver is able to give the current
voltage and current of the battery (be it charging or discharging), the
maximal and minimal voltage and maximal current allowed for the battery,
whether the battery is present and usable and its capacity. It will get
the battery current current and voltage by reading the ADC channels. The
PMIC allows maximal voltages (4.36V for AXP20X and 4.22V and 4.24V for
AXP22X) that should not be used with Lithium-based batteries and since
this PMIC is supposed to be used with Lithium-based batteries, they have
been disabled. The values returned by the ADC driver are multipled by
1000 to scale from the mV returned by the ADC to the uV expected by the
power supply framework.

This series of patch adds DT bindings for ACIN power supply, ADC and
battery power supply drivers for AXP20X and AXP22X PMICs and their
documentation. It also enables the supported power supplies for the
Nextthing Co. CHIP and Sinlinx SinA33 boards.

The different drivers are also added to the MFD cells of the AXP20X and
AXP22X cells and the writeable and volatile regs updated to work with
the newly added drivers.

This series of patch is based on a previous upstreaming attempt done by
Bruno Prémont few months ago. It differs in three points: the ADC
driver does not tell the battery temperature (TS_IN) as I do not have a
board to test it with, it does not tell the instantaneous battery power
as it returns crazy values for me and finally no support for OCV curves
for the battery.

You can test these patches from this repo and branch:
https://github.com/QSchulz/linux/tree/axp2xx_adc_batt_ac_v4

v4:
 - added the ability to set maximum constant charge current from sysfs,
 - added a warning when setting a higher than current maximum constant charge
 current,
 - set default to minimum possible value for current and maximum constant charge
 current when no battery DT is present or invalid battery DT,
 - fixed a forgotten custom formula to compute maximum constant charge current
 for AXP22X,
 - automatically lower the current constant charge current when it is higher
 than the maximum constant charge current about to be set,

v3:
 - Removed DT property for constant charge current in favor of the WIP
 battery framework as requested by Sebastian Reichel,
 - Using a simple if condition instead of a switch in the ADC driver,
 - Fixed error handling in ADC driver's probe,
 - Fixed missing call to iio_map_array_unregister in the ADC driver's
 remove,
 - Removed ADC driver's DT node and documentation,
 - Merged IIO channel mapping patches into the original ADC driver
 patch,
 - Removed `adding V-OFF to writeable reg' patch as it's already in
 writeable reg range,

v2:
 - Some registers' name have been changed to better reflect their
 purpose,
 - Make VBUS power supply driver use IIO channels when AXP ADC driver is
 enabled, but fall back on previous behavior when disabled. This is made
 to avoid the ADC driver overwritting registers for VBUS power supply
 ADC when removed,
 - Removed useless adding of data registers to volatile registers,
 - Reordered IIO channels, now grouped by same part of the PMIC (e.g.
 voltage and current of the battery have the same index in different
 IIO types),
 - Added structures for specific data instead of matching on IDs,
 - Switched from DT IIO channels mapping to iio_map structures IIO
 channels mapping,

Quentin

Quentin Schulz (6):
  dt-bindings: power: supply: add AXP20X/AXP22X battery DT binding
  power: supply: add battery driver for AXP20X and AXP22X PMICs
  ARM: dtsi: axp209: add battery power supply subnode
  ARM: dtsi: axp22x: add battery power supply subnode
  ARM: dts: sun8i: sina33: enable battery power supply subnode
  ARM: sun5i: chip: enable battery power supply subnode

 .../bindings/power/supply/axp20x_battery.txt       |  20 +
 arch/arm/boot/dts/axp209.dtsi                      |   5 +
 arch/arm/boot/dts/axp22x.dtsi                      |   5 +
 arch/arm/boot/dts/sun5i-r8-chip.dts                |   4 +
 arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts     |   4 +
 drivers/power/supply/Kconfig                       |  12 +
 drivers/power/supply/Makefile                      |   1 +
 drivers/power/supply/axp20x_battery.c              | 502 +++++++++++++++++++++
 8 files changed, 553 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
 create mode 100644 drivers/power/supply/axp20x_battery.c

-- 
2.11.0

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

* [PATCH v6 1/6] dt-bindings: power: supply: add AXP20X/AXP22X battery DT binding
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418073421.31351-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The X-Powers AXP20X and AXP22X PMICs can have a battery as power supply.

This patch adds the DT binding documentation for the battery power
supply which gets various data from the PMIC, such as the battery status
(charging, discharging, full, dead), current max limit, current current,
battery capacity (in percentage), voltage max and min limits, current
voltage and battery capacity (in Ah).

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
v6:
  - removed mention to monitored-battery, will be sent when the battery
  framework has been merged,

v5:
  - removed DT property example from monitored-battery,

v4:
 - added monitored-battery optional property,
 - added example with battery,

v3:
 - removed constant charge current property, now should use the WIP
 battery framework,

v2:
 - changed DT node name from ac_power_supply to ac-power-supply,
 - removed io-channels and io-channel-names from DT (the IIO mapping is
 done in the IIO ADC driver now),
 - added x-powers,constant-charge-current property to set the maximal
 default constant current charge of the battery,

 .../bindings/power/supply/axp20x_battery.txt         | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/axp20x_battery.txt

diff --git a/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
new file mode 100644
index 000000000000..c24886676a60
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
@@ -0,0 +1,20 @@
+AXP20x and AXP22x battery power supply
+
+Required Properties:
+ - compatible, one of:
+			"x-powers,axp209-battery-power-supply"
+			"x-powers,axp221-battery-power-supply"
+
+This node is a subnode of the axp20x/axp22x PMIC.
+
+The AXP20X and AXP22X can read the battery voltage, charge and discharge
+currents of the battery by reading ADC channels from the AXP20X/AXP22X
+ADC.
+
+Example:
+
+&axp209 {
+	battery_power_supply: battery-power-supply {
+		compatible = "x-powers,axp209-battery-power-supply";
+	}
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v6 2/6] power: supply: add battery driver for AXP20X and AXP22X PMICs
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418073421.31351-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The X-Powers AXP20X and AXP22X PMICs can have a battery as power supply.

This patch adds the battery power supply driver to get various data from
the PMIC, such as the battery status (charging, discharging, full,
dead), current max limit, current current, battery capacity (in
percentage), voltage max and min limits, current voltage and battery
capacity (in Ah).

This battery driver uses the AXP20X/AXP22X ADC driver as PMIC data
provider.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
Acked-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---

v5:
 - removed support for constant charge current setting from DT,
 - added a function to compute the constant charge current depending on the
 PMIC variant,

v4:
 - removed useless axp20x_dev variable in struct axp20x_batt_ps,
 - added struct device and maximum constant current charge variables in struct
 axp20x_batt_ps,
 - fixed wrong maximum constant current charge formula for AXP22X (forgot to add
 a custom formula so it was calculated like the AXP209),
 - when a battery node in DT does not have a valid constant current charge or
 there is no battery, the maximum current constant current charge are set to the
 lowest possible value,
 - it is possible to set maximum constant current charge now but a warn message
 is printed when trying to increase it to inform people it is risky,
 - added a check to verify the constant current charge to be set is under the
 maximum allowed,
 - lower the current constant charge current if it is higher than the maximum
 current value to be set,

v3:
 - added axp20x_set_voltage_min_design function so it can be reused,
 - used power_supply_get_battery_info for setting constant charge current
 instead of x-powers,constant-charge-current introduced in v2,
 - used power_supply_get_battery_info for setting voltage min design of
 the battery,

v2:
 - changed BIT(x) to 1 << x when describing bits purpose for which 2 <<
 x or 3 << x exists, to be consistent,
 - switched from POWER_SUPPLY_PROP_CURRENT_MAX to
 POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
 - added POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX to the list of
 readable properties,
 - replaced µ character by a common u for micro units to make checkpatch
 happy,
 - factorized code in axp20x_battery_set_max_voltage,
 - added a axp20x_set_constant_charge_current function to be used when
 setting the value from sysfs and from the DT,
 - removed some dead code,
 - added a DT property to set constant current charge of the battery
 (x-powers,constant-charge-current),
 - migrated to dev_get_regmap instead of manually looking for the regmap
 in the drvdata of the parent,
 - switched from int to uintptr_t cast to make sure the cast is always
 for the same size type (make build on 64bits platforms happy mainly),

 drivers/power/supply/Kconfig          |  12 +
 drivers/power/supply/Makefile         |   1 +
 drivers/power/supply/axp20x_battery.c | 502 ++++++++++++++++++++++++++++++++++
 3 files changed, 515 insertions(+)
 create mode 100644 drivers/power/supply/axp20x_battery.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index da922756149f..4b718e90b3ca 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -238,6 +238,18 @@ config CHARGER_AXP20X
 	  This driver can also be built as a module. If so, the module will be
 	  called axp20x_ac_power.
 
+config BATTERY_AXP20X
+	tristate "X-Powers AXP20X battery driver"
+	depends on MFD_AXP20X
+	depends on AXP20X_ADC
+	depends on IIO
+	help
+	  Say Y here to enable support for X-Powers AXP20X PMICs' battery power
+	  supply.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called axp20x_battery.
+
 config AXP288_CHARGER
 	tristate "X-Powers AXP288 Charger"
 	depends on MFD_AXP20X && EXTCON_AXP288
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 39fc733e6cc4..a39126d7a6ce 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_TEST_POWER)	+= test_power.o
 
 obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
 obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
+obj-$(CONFIG_BATTERY_AXP20X)	+= axp20x_battery.o
 obj-$(CONFIG_CHARGER_AXP20X)	+= axp20x_ac_power.o
 obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
 obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
new file mode 100644
index 000000000000..5d29b2eab8fc
--- /dev/null
+++ b/drivers/power/supply/axp20x_battery.c
@@ -0,0 +1,502 @@
+/*
+ * Battery power supply driver for X-Powers AXP20X and AXP22X PMICs
+ *
+ * Copyright 2016 Free Electrons NextThing Co.
+ *	Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This driver is based on a previous upstreaming attempt by:
+ *	Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/consumer.h>
+#include <linux/mfd/axp20x.h>
+
+#define AXP20X_PWR_STATUS_BAT_CHARGING	BIT(2)
+
+#define AXP20X_PWR_OP_BATT_PRESENT	BIT(5)
+#define AXP20X_PWR_OP_BATT_ACTIVATED	BIT(3)
+
+#define AXP209_FG_PERCENT		GENMASK(6, 0)
+#define AXP22X_FG_VALID			BIT(7)
+
+#define AXP20X_CHRG_CTRL1_TGT_VOLT	GENMASK(6, 5)
+#define AXP20X_CHRG_CTRL1_TGT_4_1V	(0 << 5)
+#define AXP20X_CHRG_CTRL1_TGT_4_15V	(1 << 5)
+#define AXP20X_CHRG_CTRL1_TGT_4_2V	(2 << 5)
+#define AXP20X_CHRG_CTRL1_TGT_4_36V	(3 << 5)
+
+#define AXP22X_CHRG_CTRL1_TGT_4_22V	(1 << 5)
+#define AXP22X_CHRG_CTRL1_TGT_4_24V	(3 << 5)
+
+#define AXP20X_CHRG_CTRL1_TGT_CURR	GENMASK(3, 0)
+
+#define AXP20X_V_OFF_MASK		GENMASK(2, 0)
+
+struct axp20x_batt_ps {
+	struct regmap *regmap;
+	struct power_supply *batt;
+	struct device *dev;
+	struct iio_channel *batt_chrg_i;
+	struct iio_channel *batt_dischrg_i;
+	struct iio_channel *batt_v;
+	u8 axp_id;
+};
+
+static int axp20x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
+					  int *val)
+{
+	int ret, reg;
+
+	ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &reg);
+	if (ret)
+		return ret;
+
+	switch (reg & AXP20X_CHRG_CTRL1_TGT_VOLT) {
+	case AXP20X_CHRG_CTRL1_TGT_4_1V:
+		*val = 4100000;
+		break;
+	case AXP20X_CHRG_CTRL1_TGT_4_15V:
+		*val = 4150000;
+		break;
+	case AXP20X_CHRG_CTRL1_TGT_4_2V:
+		*val = 4200000;
+		break;
+	case AXP20X_CHRG_CTRL1_TGT_4_36V:
+		*val = 4360000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
+					  int *val)
+{
+	int ret, reg;
+
+	ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &reg);
+	if (ret)
+		return ret;
+
+	switch (reg & AXP20X_CHRG_CTRL1_TGT_VOLT) {
+	case AXP20X_CHRG_CTRL1_TGT_4_1V:
+		*val = 4100000;
+		break;
+	case AXP20X_CHRG_CTRL1_TGT_4_2V:
+		*val = 4200000;
+		break;
+	case AXP22X_CHRG_CTRL1_TGT_4_22V:
+		*val = 4220000;
+		break;
+	case AXP22X_CHRG_CTRL1_TGT_4_24V:
+		*val = 4240000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val)
+{
+	if (axp->axp_id == AXP209_ID)
+		*val = *val * 100000 + 300000;
+	else
+		*val = *val * 150000 + 300000;
+}
+
+static int axp20x_get_constant_charge_current(struct axp20x_batt_ps *axp,
+					      int *val)
+{
+	int ret;
+
+	ret = regmap_read(axp->regmap, AXP20X_CHRG_CTRL1, val);
+	if (ret)
+		return ret;
+
+	*val &= AXP20X_CHRG_CTRL1_TGT_CURR;
+
+	raw_to_constant_charge_current(axp, val);
+
+	return 0;
+}
+
+static int axp20x_battery_get_prop(struct power_supply *psy,
+				   enum power_supply_property psp,
+				   union power_supply_propval *val)
+{
+	struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy);
+	struct iio_channel *chan;
+	int ret = 0, reg, val1;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+	case POWER_SUPPLY_PROP_ONLINE:
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_PWR_OP_MODE,
+				  &reg);
+		if (ret)
+			return ret;
+
+		val->intval = !!(reg & AXP20X_PWR_OP_BATT_PRESENT);
+		break;
+
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_PWR_INPUT_STATUS,
+				  &reg);
+		if (ret)
+			return ret;
+
+		if (reg & AXP20X_PWR_STATUS_BAT_CHARGING) {
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+			return 0;
+		}
+
+		ret = iio_read_channel_processed(axp20x_batt->batt_dischrg_i,
+						 &val1);
+		if (ret)
+			return ret;
+
+		if (val1) {
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+			return 0;
+		}
+
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_FG_RES, &val1);
+		if (ret)
+			return ret;
+
+		/*
+		 * Fuel Gauge data takes 7 bits but the stored value seems to be
+		 * directly the raw percentage without any scaling to 7 bits.
+		 */
+		if ((val1 & AXP209_FG_PERCENT) == 100)
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+
+	case POWER_SUPPLY_PROP_HEALTH:
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_PWR_OP_MODE,
+				  &val1);
+		if (ret)
+			return ret;
+
+		if (val1 & AXP20X_PWR_OP_BATT_ACTIVATED) {
+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
+			return 0;
+		}
+
+		val->intval = POWER_SUPPLY_HEALTH_GOOD;
+		break;
+
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		ret = axp20x_get_constant_charge_current(axp20x_batt,
+							 &val->intval);
+		if (ret)
+			return ret;
+		break;
+
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		val->intval = AXP20X_CHRG_CTRL1_TGT_CURR;
+		raw_to_constant_charge_current(axp20x_batt, &val->intval);
+
+		break;
+
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_PWR_INPUT_STATUS,
+				  &reg);
+		if (ret)
+			return ret;
+
+		if (reg & AXP20X_PWR_STATUS_BAT_CHARGING)
+			chan = axp20x_batt->batt_chrg_i;
+		else
+			chan = axp20x_batt->batt_dischrg_i;
+
+		ret = iio_read_channel_processed(chan, &val->intval);
+		if (ret)
+			return ret;
+
+		/* IIO framework gives mA but Power Supply framework gives uA */
+		val->intval *= 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_CAPACITY:
+		/* When no battery is present, return capacity is 100% */
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_PWR_OP_MODE,
+				  &reg);
+		if (ret)
+			return ret;
+
+		if (!(reg & AXP20X_PWR_OP_BATT_PRESENT)) {
+			val->intval = 100;
+			return 0;
+		}
+
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_FG_RES, &reg);
+		if (ret)
+			return ret;
+
+		if (axp20x_batt->axp_id == AXP221_ID &&
+		    !(reg & AXP22X_FG_VALID))
+			return -EINVAL;
+
+		/*
+		 * Fuel Gauge data takes 7 bits but the stored value seems to be
+		 * directly the raw percentage without any scaling to 7 bits.
+		 */
+		val->intval = reg & AXP209_FG_PERCENT;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		if (axp20x_batt->axp_id == AXP209_ID)
+			return axp20x_battery_get_max_voltage(axp20x_batt,
+							      &val->intval);
+		return axp22x_battery_get_max_voltage(axp20x_batt,
+						      &val->intval);
+
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_V_OFF, &reg);
+		if (ret)
+			return ret;
+
+		val->intval = 2600000 + 100000 * (reg & AXP20X_V_OFF_MASK);
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		ret = iio_read_channel_processed(axp20x_batt->batt_v,
+						 &val->intval);
+		if (ret)
+			return ret;
+
+		/* IIO framework gives mV but Power Supply framework gives uV */
+		val->intval *= 1000;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int axp20x_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
+					  int val)
+{
+	switch (val) {
+	case 4100000:
+		val = AXP20X_CHRG_CTRL1_TGT_4_1V;
+		break;
+
+	case 4150000:
+		if (axp20x_batt->axp_id == AXP221_ID)
+			return -EINVAL;
+
+		val = AXP20X_CHRG_CTRL1_TGT_4_15V;
+		break;
+
+	case 4200000:
+		val = AXP20X_CHRG_CTRL1_TGT_4_2V;
+		break;
+
+	default:
+		/*
+		 * AXP20x max voltage can be set to 4.36V and AXP22X max voltage
+		 * can be set to 4.22V and 4.24V, but these voltages are too
+		 * high for Lithium based batteries (AXP PMICs are supposed to
+		 * be used with these kinds of battery).
+		 */
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				  AXP20X_CHRG_CTRL1_TGT_VOLT, val);
+}
+
+static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
+					      int charge_current)
+{
+	if (axp_batt->axp_id == AXP209_ID)
+		charge_current = (charge_current - 300000) / 100000;
+	else
+		charge_current = (charge_current - 300000) / 150000;
+
+	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
+		return -EINVAL;
+
+	return regmap_update_bits(axp_batt->regmap, AXP20X_CHRG_CTRL1,
+				  AXP20X_CHRG_CTRL1_TGT_CURR, charge_current);
+}
+
+static int axp20x_set_voltage_min_design(struct axp20x_batt_ps *axp_batt,
+					 int min_voltage)
+{
+	int val1 = (min_voltage - 2600000) / 100000;
+
+	if (val1 < 0 || val1 > AXP20X_V_OFF_MASK)
+		return -EINVAL;
+
+	return regmap_update_bits(axp_batt->regmap, AXP20X_V_OFF,
+				  AXP20X_V_OFF_MASK, val1);
+}
+
+static int axp20x_battery_set_prop(struct power_supply *psy,
+				   enum power_supply_property psp,
+				   const union power_supply_propval *val)
+{
+	struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		return axp20x_set_voltage_min_design(axp20x_batt, val->intval);
+
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		return axp20x_battery_set_max_voltage(axp20x_batt, val->intval);
+
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		return axp20x_set_constant_charge_current(axp20x_batt,
+							  val->intval);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static enum power_supply_property axp20x_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int axp20x_battery_prop_writeable(struct power_supply *psy,
+					 enum power_supply_property psp)
+{
+	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
+	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT;
+}
+
+static const struct power_supply_desc axp20x_batt_ps_desc = {
+	.name = "axp20x-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = axp20x_battery_props,
+	.num_properties = ARRAY_SIZE(axp20x_battery_props),
+	.property_is_writeable = axp20x_battery_prop_writeable,
+	.get_property = axp20x_battery_get_prop,
+	.set_property = axp20x_battery_set_prop,
+};
+
+static const struct of_device_id axp20x_battery_ps_id[] = {
+	{
+		.compatible = "x-powers,axp209-battery-power-supply",
+		.data = (void *)AXP209_ID,
+	}, {
+		.compatible = "x-powers,axp221-battery-power-supply",
+		.data = (void *)AXP221_ID,
+	}, { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, axp20x_battery_ps_id);
+
+static int axp20x_power_probe(struct platform_device *pdev)
+{
+	struct axp20x_batt_ps *axp20x_batt;
+	struct power_supply_config psy_cfg = {};
+
+	if (!of_device_is_available(pdev->dev.of_node))
+		return -ENODEV;
+
+	axp20x_batt = devm_kzalloc(&pdev->dev, sizeof(*axp20x_batt),
+				   GFP_KERNEL);
+	if (!axp20x_batt)
+		return -ENOMEM;
+
+	axp20x_batt->dev = &pdev->dev;
+
+	axp20x_batt->batt_v = devm_iio_channel_get(&pdev->dev, "batt_v");
+	if (IS_ERR(axp20x_batt->batt_v)) {
+		if (PTR_ERR(axp20x_batt->batt_v) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(axp20x_batt->batt_v);
+	}
+
+	axp20x_batt->batt_chrg_i = devm_iio_channel_get(&pdev->dev,
+							"batt_chrg_i");
+	if (IS_ERR(axp20x_batt->batt_chrg_i)) {
+		if (PTR_ERR(axp20x_batt->batt_chrg_i) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(axp20x_batt->batt_chrg_i);
+	}
+
+	axp20x_batt->batt_dischrg_i = devm_iio_channel_get(&pdev->dev,
+							   "batt_dischrg_i");
+	if (IS_ERR(axp20x_batt->batt_dischrg_i)) {
+		if (PTR_ERR(axp20x_batt->batt_dischrg_i) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(axp20x_batt->batt_dischrg_i);
+	}
+
+	axp20x_batt->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	platform_set_drvdata(pdev, axp20x_batt);
+
+	psy_cfg.drv_data = axp20x_batt;
+	psy_cfg.of_node = pdev->dev.of_node;
+
+	axp20x_batt->axp_id = (uintptr_t)of_device_get_match_data(&pdev->dev);
+
+	axp20x_batt->batt = devm_power_supply_register(&pdev->dev,
+						       &axp20x_batt_ps_desc,
+						       &psy_cfg);
+	if (IS_ERR(axp20x_batt->batt)) {
+		dev_err(&pdev->dev, "failed to register power supply: %ld\n",
+			PTR_ERR(axp20x_batt->batt));
+		return PTR_ERR(axp20x_batt->batt);
+	}
+
+	return 0;
+}
+
+static struct platform_driver axp20x_batt_driver = {
+	.probe    = axp20x_power_probe,
+	.driver   = {
+		.name  = "axp20x-battery-power-supply",
+		.of_match_table = axp20x_battery_ps_id,
+	},
+};
+
+module_platform_driver(axp20x_batt_driver);
+
+MODULE_DESCRIPTION("Battery power supply driver for AXP20X and AXP22X PMICs");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
2.11.0

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

* [PATCH v6 3/6] ARM: dtsi: axp209: add battery power supply subnode
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418073421.31351-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The X-Powers AXP209 PMIC exposes battery supply various data such as
the battery status (charging, discharging, full, dead), current max
limit, current current, battery capacity (in percentage), voltage max
and min limits, current voltage, and battery capacity (in Ah).

This adds the battery power supply subnode for AXP20X PMIC.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---

v2:
 - changed DT node name from battery_power_supply to
 battery-power-supply,
 - removed io-channels and io-channel-names from DT (the IIO mapping is
 done in the IIO ADC driver now),

 arch/arm/boot/dts/axp209.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/axp209.dtsi b/arch/arm/boot/dts/axp209.dtsi
index 9677dd5cf6b6..3c8fa26e87b7 100644
--- a/arch/arm/boot/dts/axp209.dtsi
+++ b/arch/arm/boot/dts/axp209.dtsi
@@ -64,6 +64,11 @@
 		#gpio-cells = <2>;
 	};
 
+	battery_power_supply: battery-power-supply {
+		compatible = "x-powers,axp209-battery-power-supply";
+		status = "disabled";
+	};
+
 	regulators {
 		/* Default work frequency for buck regulators */
 		x-powers,dcdc-freq = <1500>;
-- 
2.11.0

^ permalink raw reply related

* [PATCH v6 4/6] ARM: dtsi: axp22x: add battery power supply subnode
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418073421.31351-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The X-Powers AXP22X PMIC exposes battery supply various data such as
the battery status (charging, discharging, full, dead), current max
limit, current current, battery capacity (in percentage), voltage max
limit, current voltage, and battery capacity (in Ah).

This adds the battery power supply subnode for AXP22X PMIC.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---

v2:
 - changed DT node name from battery_power_supply to
 battery-power-supply,
 - removed io-channels and io-channel-names from DT (the IIO mapping is
 done in the IIO ADC driver now),

 arch/arm/boot/dts/axp22x.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/axp22x.dtsi b/arch/arm/boot/dts/axp22x.dtsi
index 67331c5f1714..87fb08e812ec 100644
--- a/arch/arm/boot/dts/axp22x.dtsi
+++ b/arch/arm/boot/dts/axp22x.dtsi
@@ -57,6 +57,11 @@
 		status = "disabled";
 	};
 
+	battery_power_supply: battery-power-supply {
+		compatible = "x-powers,axp221-battery-power-supply";
+		status = "disabled";
+	};
+
 	regulators {
 		/* Default work frequency for buck regulators */
 		x-powers,dcdc-freq = <3000>;
-- 
2.11.0

^ permalink raw reply related

* [PATCH v6 5/6] ARM: dts: sun8i: sina33: enable battery power supply subnode
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418073421.31351-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The Sinlinx SinA33 has an AXP223 PMIC and a battery connector, thus, we
enable the battery power supply subnode in its Device Tree.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
index 9b620cc1d5f1..edc68984fbc8 100644
--- a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
+++ b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
@@ -196,6 +196,10 @@
 	status = "okay";
 };
 
+&battery_power_supply {
+	status = "okay";
+};
+
 &reg_aldo1 {
 	regulator-always-on;
 	regulator-min-microvolt = <3000000>;
-- 
2.11.0

^ permalink raw reply related

* [PATCH v6 6/6] ARM: sun5i: chip: enable battery power supply subnode
From: Quentin Schulz @ 2017-04-18  7:34 UTC (permalink / raw)
  To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: Quentin Schulz, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, icenowy-ymACFijhrKM,
	liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418073421.31351-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

The NextThing Co. CHIP has an AXP209 PMIC with battery connector.

This enables the battery power supply subnode.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
index d0785602663b..879a4b0f3bd5 100644
--- a/arch/arm/boot/dts/sun5i-r8-chip.dts
+++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
@@ -132,6 +132,10 @@
 	status = "okay";
 };
 
+&battery_power_supply {
+	status = "okay";
+};
+
 &i2c1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c1_pins_a>;
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH v2 3/3] arm64: dts: exynos: Add support for s6e3hf2 panel device on TM2e board
From: Hoegeun Kwon @ 2017-04-18  7:44 UTC (permalink / raw)
  To: Andrzej Hajda, thierry.reding, airlied, robh+dt, mark.rutland,
	catalin.marinas, will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, Hoegeun Kwon, linux-arm-kernel
In-Reply-To: <b9bfcdd9-5576-34c3-6c7d-556b6c81946d@samsung.com>

On 04/18/2017 03:00 PM, Andrzej Hajda wrote:
> On 17.04.2017 08:02, Hoegeun Kwon wrote:
>> This patch add the panel device tree node for s6e3hf2 display
>> controller to TM2e dts.
>>
>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Maybe it would be good to remove te-gpios property - tm2/tm2e uses
> hardware trigger, so it is not necessary and generates useless interrupts.

Thanks for your review.

I agree, but I thinks remove the te-gpio property after your patch [1] 
has been merged.


[1] https://patchwork.kernel.org/patch/9625383/

Best regards,
Hoegeun

>
> Beside this:
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>   --
> Regards
> Andrzej
>> ---
>>   arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
>> index 694717a..98ad094 100644
>> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
>> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
>> @@ -52,6 +52,18 @@
>>   	assigned-clock-rates = <278000000>, <400000000>;
>>   };
>>   
>> +&dsi {
>> +	panel@0 {
>> +		compatible = "samsung,s6e3hf2";
>> +		reg = <0>;
>> +		vdd3-supply = <&ldo27_reg>;
>> +		vci-supply = <&ldo28_reg>;
>> +		reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
>> +		enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
>> +		te-gpios = <&gpf1 3 GPIO_ACTIVE_HIGH>;
>> +	};
>> +};
>> +
>>   &ldo31_reg {
>>   	regulator-name = "TSP_VDD_1.8V_AP";
>>   	regulator-min-microvolt = <1800000>;
>
>
>

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

^ permalink raw reply

* Re: [PATCH v2 3/3] arm64: dts: exynos: Add support for s6e3hf2 panel device on TM2e board
From: Andrzej Hajda @ 2017-04-18  7:56 UTC (permalink / raw)
  To: Hoegeun Kwon, thierry.reding, airlied, robh+dt, mark.rutland,
	catalin.marinas, will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, linux-arm-kernel
In-Reply-To: <f2acb724-ba41-175c-061a-7be765cc1939@samsung.com>

On 18.04.2017 09:44, Hoegeun Kwon wrote:
> On 04/18/2017 03:00 PM, Andrzej Hajda wrote:
>> On 17.04.2017 08:02, Hoegeun Kwon wrote:
>>> This patch add the panel device tree node for s6e3hf2 display
>>> controller to TM2e dts.
>>>
>>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>> Maybe it would be good to remove te-gpios property - tm2/tm2e uses
>> hardware trigger, so it is not necessary and generates useless interrupts.
> Thanks for your review.
>
> I agree, but I thinks remove the te-gpio property after your patch [1] 
> has been merged.

Patch [1] has been merged about month ago :)

Regards
Andrzej

>
>
> [1] https://patchwork.kernel.org/patch/9625383/
>
> Best regards,
> Hoegeun
>
>> Beside this:
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>   --
>> Regards
>> Andrzej
>>> ---
>>>   arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
>>> index 694717a..98ad094 100644
>>> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
>>> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
>>> @@ -52,6 +52,18 @@
>>>   	assigned-clock-rates = <278000000>, <400000000>;
>>>   };
>>>   
>>> +&dsi {
>>> +	panel@0 {
>>> +		compatible = "samsung,s6e3hf2";
>>> +		reg = <0>;
>>> +		vdd3-supply = <&ldo27_reg>;
>>> +		vci-supply = <&ldo28_reg>;
>>> +		reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
>>> +		enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
>>> +		te-gpios = <&gpf1 3 GPIO_ACTIVE_HIGH>;
>>> +	};
>>> +};
>>> +
>>>   &ldo31_reg {
>>>   	regulator-name = "TSP_VDD_1.8V_AP";
>>>   	regulator-min-microvolt = <1800000>;
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

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

^ permalink raw reply

* Re: [PATCH v2 2/3] drm/panel: s6e3ha2: Add support for s6e3hf2 panel on TM2e board
From: Inki Dae @ 2017-04-18  8:03 UTC (permalink / raw)
  To: Andrzej Hajda, Hoegeun Kwon, thierry.reding, airlied, robh+dt,
	mark.rutland, catalin.marinas, will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, linux-arm-kernel
In-Reply-To: <5b3031ca-2847-fbc0-36f3-7e0a6cd3e0b3@samsung.com>



2017년 04월 18일 14:55에 Andrzej Hajda 이(가) 쓴 글:
> On 17.04.2017 08:02, Hoegeun Kwon wrote:
>> This patch supports TM2e panel and the panel has 1600x2560 resolution
>> in 5.65" physical.
>>
>> This identify panel type with compatibility string, also invoke
>> display mode that matches the type. So add the check code for s6e3ha2
>> compatibility and s6e3hf2 type and select the drm_display_mode of
>> default and edge type.
>>
>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Inki Dae <inki.dae@samsung.com>

Thanks,
Inki Dae

> 
>  --
> Regards
> Andrzej
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver
From: Philipp Zabel @ 2017-04-18  8:09 UTC (permalink / raw)
  To: Pavel Machek
  Cc: mark.rutland, andrew-ct.chen, minghsiu.tsai, nick, songjun.wu,
	hverkuil, Steve Longerbeam, robert.jarzmik, devel, markus.heiser,
	laurent.pinchart+renesas, shuah, linux, geert, Steve Longerbeam,
	Sascha Hauer, linux-media, devicetree, sakari.ailus, arnd,
	mchehab, bparrot, robh+dt, horms+renesas, tiffany.lin,
	linux-arm-kernel, niklas.soderlund+renesas, gregkh, linux-kernel,
	Sakari Ailus, jean-christophe.trotin, kernel
In-Reply-To: <20170414203216.GA10920@amd>

Hi Pavel,

On Fri, 2017-04-14 at 22:32 +0200, Pavel Machek wrote:
> Hi!
> 
> > > The MUX framework is already in linux-next. Could you use that instead of
> > > adding new driver + bindings that are not compliant with the MUX framework?
> > > I don't think it'd be much of a change in terms of code, using the MUX
> > > framework appears quite simple.
> > 
> > It is not quite clear to me how to design the DT bindings for this. Just
> > splitting the video-multiplexer driver from the mux-mmio / mux-gpio
> > would make it necessary to keep the video-multiplexer node to describe
> > the of-graph bindings. But then we have two different nodes in the DT
> > that describe the same hardware:
> > 
> > 	mux: mux {
> > 		compatible = "mux-gpio";
> > 		mux-gpios = <&gpio 0>, <&gpio 1>;
> > 		#mux-control-cells = <0>;
> > 	}
> > 
> > 	video-multiplexer {
> > 		compatible = "video-multiplexer"
> > 		mux-controls = <&mux>;
> > 
> > 		ports {
> > 			/* ... */
> > 		}
> > 	}
> > 
> > It would feel more natural to have the ports in the mux node, but then
> > how would the video-multiplexer driver be instanciated, and how would it
> > get to the of-graph nodes?
> 
> Device tree representation and code used to implement the muxing
> driver should be pretty independend, no? Yes, one piece of hardware
> should have one entry in the device tree,

I agree.

>  so it should be something like:
> 
>  	video-multiplexer {
>  		compatible = "video-multiplexer-gpio"	
>  		mux-gpios = <&gpio 0>, <&gpio 1>;
>  		#mux-control-cells = <0>;
> 
>  		mux-controls = <&mux>;
>  
>  		ports {
>  			/* ... */
>  		}
>  	}

That self-referencing mux-controls property looks a bit superfluous:

	mux: video-multiplexer {
		mux-controls = <&mux>;
	};

Other than that, I'm completely fine with splitting the compatible into
something like video-mux-gpio and video-mux-mmio and reusing the
mux-gpios property for video-mux-gpio.

> You should be able to use code in drivers/mux as a library...

This is a good idea in principle, but this requires some rework of the
mux subsystem, and that subsystem hasn't even landed yet. For now I'd
like to focus on getting the DT bindings right.

I'd honestly prefer to not add this rework as a requirement for the i.MX
media drivers to get into staging.

regards
Philipp

^ permalink raw reply

* Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings
From: Philipp Zabel @ 2017-04-18  8:19 UTC (permalink / raw)
  To: Peter Rosin, Pavel Machek
  Cc: Rob Herring, Mark Rutland, Sakari Ailus, Steve Longerbeam,
	devicetree, linux-kernel, kernel
In-Reply-To: <20170413154812.19597-1-p.zabel@pengutronix.de>

On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent
> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"
> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;
> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {
> +		compatible = "video-mux";
> +		mux-controls = <&mux>;
> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};

So Pavel (added to Cc:) suggested to merge these into one node for the
video mux, as really we are describing a single hardware entity that
happens to be multiplexing multiple video buses into one:

	syscon {
		compatible = "syscon";

		/* video multiplexer */
		mux: mux-controller@3 {
			compatible = "video-mmio-mux";
			reg = <0x3>;
			bit-mask = <0x1>;
			bit-shift = <5>;
			#mux-control-cells = <0>;
 
			mux-controls = <&mux>;

			ports {
				/* input 0 */
				port@0 {
					reg = <0>;
				};

				/* input 1 */
				port@1 {
					reg = <1>;
				};

				/* output */
				port@2 {
					reg = <2>;
				};
			};
		};
	};

That would not touch on this "general purpose" mmio-mux binding itself,
but would make it necessary to add a separate "video-mmio-mux" and a
"video-gpio-mux" binding that mirror the "mmio-mux" and "gpio-mux"
bindings but add the OF-graph connections.

Also I think in this case the self-referencing mux-controls property
would be superfluous, as the driver binding to this node is expected to
control the mux according to activation of the links described by the
OF-graph bindings.

regards
Philipp

^ permalink raw reply

* Re: [PATCH v2 3/3] arm64: dts: exynos: Add support for s6e3hf2 panel device on TM2e board
From: Hoegeun Kwon @ 2017-04-18  8:27 UTC (permalink / raw)
  To: Andrzej Hajda, thierry.reding, airlied, robh+dt, mark.rutland,
	catalin.marinas, will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, Hoegeun Kwon, linux-arm-kernel
In-Reply-To: <94152d85-5fb0-f5dc-045d-e6ee6c009f42@samsung.com>

On 04/18/2017 04:56 PM, Andrzej Hajda wrote:
> On 18.04.2017 09:44, Hoegeun Kwon wrote:
>> On 04/18/2017 03:00 PM, Andrzej Hajda wrote:
>>> On 17.04.2017 08:02, Hoegeun Kwon wrote:
>>>> This patch add the panel device tree node for s6e3hf2 display
>>>> controller to TM2e dts.
>>>>
>>>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>>> Maybe it would be good to remove te-gpios property - tm2/tm2e uses
>>> hardware trigger, so it is not necessary and generates useless interrupts.
>> Thanks for your review.
>>
>> I agree, but I thinks remove the te-gpio property after your patch [1]
>> has been merged.
> Patch [1] has been merged about month ago :)

Ah..., My mistake.... :)

I will send ver3 patchset without te-gpio property.

Best regards,
Hoegeun


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

^ permalink raw reply

* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
From: Philipp Zabel @ 2017-04-18  8:34 UTC (permalink / raw)
  To: Peter Rosin
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman,
	Wolfram Sang, Rob Herring, Mark Rutland, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Jonathan Corbet, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Colin Ian King,
	Paul Gortmaker, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <1492101794-13444-4-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>

Hi Peter,

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index 000000000000..febdde4246df
> --- /dev/null
> +++ b/include/linux/mux.h
[...]

Consider separating mux.h into a consumer header and a driver header.
Right now there is no separation between the consumer API and the
framework internals.

regards
Philipp

--
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 v3 0/3] Add support for the S6E3HF2 panel on TM2e board
From: Hoegeun Kwon @ 2017-04-18  8:40 UTC (permalink / raw)
  To: thierry.reding, airlied, robh+dt, mark.rutland, catalin.marinas,
	will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, Hoegeun Kwon, linux-arm-kernel
In-Reply-To: <CGME20170418084047epcas5p493261a4cb9204e682f60cde8d659565c@epcas5p4.samsung.com>

Hi all,

The purpose of this patch is add support for s6e3hf2 AMOLED panel on
the TM2e board. The panel has 1600x2560 resolution in 5.65" physical
panel in the TM2e device.

The s6e3hf2 panel(5.65") is simliar to the previous s6e3ha2 panel(5.7"),
but resolution and some command message are different. So it can be
distinguished as a compatiblitiy string.

Best regards,
Hoegeun

Changes for V3:
- Remove te-gpios property in dts.
- Added Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> on all patches.
- Added Reviewed-by: Inki Dae <inki.dae@samsung.com> on patch (2/3).

Changes for V2:
- Add new compatible string to "samsung,s6e3ha2.txt binding with comments.
- Fix the panel name from s6e3ha2-e to s6e3hf2

Hoegeun Kwon (3):
  dt-bindings: Add support for samsung s6e3hf2 panel
  drm/panel: s6e3ha2: Add support for s6e3hf2 panel on TM2e board
  arm64: dts: exynos: Add support for s6e3hf2 panel device on TM2e board

 .../bindings/display/panel/samsung,s6e3ha2.txt     |  5 +-
 arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts     | 11 ++++
 drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c      | 64 +++++++++++++++++++---
 3 files changed, 72 insertions(+), 8 deletions(-)

-- 
1.9.1

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

^ permalink raw reply

* [PATCH v3 1/3] dt-bindings: Add support for samsung s6e3hf2 panel
From: Hoegeun Kwon @ 2017-04-18  8:40 UTC (permalink / raw)
  To: thierry.reding, airlied, robh+dt, mark.rutland, catalin.marinas,
	will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, Hoegeun Kwon, linux-arm-kernel
In-Reply-To: <1492504836-19225-1-git-send-email-hoegeun.kwon@samsung.com>

The samsung s6e3hf2 panel is a 5.65" 1600x2560 AMOLED panel connected
using MIPI-DSI interfaces.

The s6e3hf2 is add to samsung,s6e3ha2.txt binding because it is a
panel similar to the s6e3ha2. So add the compatible string and
comments.

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 Documentation/devicetree/bindings/display/panel/samsung,s6e3ha2.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha2.txt b/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha2.txt
index 18854f4..4acea25 100644
--- a/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha2.txt
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha2.txt
@@ -1,7 +1,10 @@
 Samsung S6E3HA2 5.7" 1440x2560 AMOLED panel
+Samsung S6E3HF2 5.65" 1600x2560 AMOLED panel
 
 Required properties:
-  - compatible: "samsung,s6e3ha2"
+  - compatible: should be one of:
+    "samsung,s6e3ha2",
+    "samsung,s6e3hf2".
   - reg: the virtual channel number of a DSI peripheral
   - vdd3-supply: I/O voltage supply
   - vci-supply: voltage supply for analog circuits
-- 
1.9.1

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

^ permalink raw reply related

* [PATCH v3 2/3] drm/panel: s6e3ha2: Add support for s6e3hf2 panel on TM2e board
From: Hoegeun Kwon @ 2017-04-18  8:40 UTC (permalink / raw)
  To: thierry.reding, airlied, robh+dt, mark.rutland, catalin.marinas,
	will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, Hoegeun Kwon, linux-arm-kernel
In-Reply-To: <1492504836-19225-1-git-send-email-hoegeun.kwon@samsung.com>

This patch supports TM2e panel and the panel has 1600x2560 resolution
in 5.65" physical.

This identify panel type with compatibility string, also invoke
display mode that matches the type. So add the check code for s6e3ha2
compatibility and s6e3hf2 type and select the drm_display_mode of
default and edge type.

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c | 64 ++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c b/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
index 4cc08d7..c7b418b 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
@@ -16,6 +16,7 @@
 #include <drm/drm_panel.h>
 #include <linux/backlight.h>
 #include <linux/gpio/consumer.h>
+#include <linux/of_device.h>
 #include <linux/regulator/consumer.h>
 
 #define S6E3HA2_MIN_BRIGHTNESS		0
@@ -218,6 +219,16 @@
 	0x1d, 0x1e, 0x1f, 0x20, 0x21
 };
 
+enum s6e3ha2_type {
+	HA2_TYPE,
+	HF2_TYPE,
+};
+
+struct s6e3ha2_panel_desc {
+	const struct drm_display_mode *mode;
+	enum s6e3ha2_type type;
+};
+
 struct s6e3ha2 {
 	struct device *dev;
 	struct drm_panel panel;
@@ -226,6 +237,8 @@ struct s6e3ha2 {
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *enable_gpio;
+
+	const struct s6e3ha2_panel_desc *desc;
 };
 
 static int s6e3ha2_dcs_write(struct s6e3ha2 *ctx, const void *data, size_t len)
@@ -283,11 +296,19 @@ static int s6e3ha2_single_dsi_set(struct s6e3ha2 *ctx)
 static int s6e3ha2_freq_calibration(struct s6e3ha2 *ctx)
 {
 	s6e3ha2_dcs_write_seq_static(ctx, 0xfd, 0x1c);
+	if (ctx->desc->type == HF2_TYPE)
+		s6e3ha2_dcs_write_seq_static(ctx, 0xf2, 0x67, 0x40, 0xc5);
 	s6e3ha2_dcs_write_seq_static(ctx, 0xfe, 0x20, 0x39);
 	s6e3ha2_dcs_write_seq_static(ctx, 0xfe, 0xa0);
 	s6e3ha2_dcs_write_seq_static(ctx, 0xfe, 0x20);
-	s6e3ha2_dcs_write_seq_static(ctx, 0xce, 0x03, 0x3b, 0x12, 0x62, 0x40,
-				0x80, 0xc0, 0x28, 0x28, 0x28, 0x28, 0x39, 0xc5);
+
+	if (ctx->desc->type == HA2_TYPE)
+		s6e3ha2_dcs_write_seq_static(ctx, 0xce, 0x03, 0x3b, 0x12, 0x62,
+			0x40, 0x80, 0xc0, 0x28, 0x28, 0x28, 0x28, 0x39, 0xc5);
+	else
+		s6e3ha2_dcs_write_seq_static(ctx, 0xce, 0x03, 0x3b, 0x14, 0x6d,
+			0x40, 0x80, 0xc0, 0x28, 0x28, 0x28, 0x28, 0x39, 0xc5);
+
 	return 0;
 }
 
@@ -583,7 +604,7 @@ static int s6e3ha2_enable(struct drm_panel *panel)
 	return 0;
 }
 
-static const struct drm_display_mode default_mode = {
+static const struct drm_display_mode s6e3ha2_mode = {
 	.clock = 222372,
 	.hdisplay = 1440,
 	.hsync_start = 1440 + 1,
@@ -597,16 +618,41 @@ static int s6e3ha2_enable(struct drm_panel *panel)
 	.flags = 0,
 };
 
+static const struct s6e3ha2_panel_desc samsung_s6e3ha2 = {
+	.mode = &s6e3ha2_mode,
+	.type = HA2_TYPE,
+};
+
+static const struct drm_display_mode s6e3hf2_mode = {
+	.clock = 247856,
+	.hdisplay = 1600,
+	.hsync_start = 1600 + 1,
+	.hsync_end = 1600 + 1 + 1,
+	.htotal = 1600 + 1 + 1 + 1,
+	.vdisplay = 2560,
+	.vsync_start = 2560 + 1,
+	.vsync_end = 2560 + 1 + 1,
+	.vtotal = 2560 + 1 + 1 + 15,
+	.vrefresh = 60,
+	.flags = 0,
+};
+
+static const struct s6e3ha2_panel_desc samsung_s6e3hf2 = {
+	.mode = &s6e3hf2_mode,
+	.type = HF2_TYPE,
+};
+
 static int s6e3ha2_get_modes(struct drm_panel *panel)
 {
 	struct drm_connector *connector = panel->connector;
+	struct s6e3ha2 *ctx = container_of(panel, struct s6e3ha2, panel);
 	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(panel->drm, &default_mode);
+	mode = drm_mode_duplicate(panel->drm, ctx->desc->mode);
 	if (!mode) {
 		DRM_ERROR("failed to add mode %ux%ux@%u\n",
-				default_mode.hdisplay, default_mode.vdisplay,
-				default_mode.vrefresh);
+			ctx->desc->mode->hdisplay, ctx->desc->mode->vdisplay,
+			ctx->desc->mode->vrefresh);
 		return -ENOMEM;
 	}
 
@@ -642,6 +688,7 @@ static int s6e3ha2_probe(struct mipi_dsi_device *dsi)
 	mipi_dsi_set_drvdata(dsi, ctx);
 
 	ctx->dev = dev;
+	ctx->desc = of_device_get_match_data(dev);
 
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
@@ -717,7 +764,10 @@ static int s6e3ha2_remove(struct mipi_dsi_device *dsi)
 }
 
 static const struct of_device_id s6e3ha2_of_match[] = {
-	{ .compatible = "samsung,s6e3ha2" },
+	{ .compatible = "samsung,s6e3ha2",
+	  .data = &samsung_s6e3ha2 },
+	{ .compatible = "samsung,s6e3hf2",
+	  .data = &samsung_s6e3hf2 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, s6e3ha2_of_match);
-- 
1.9.1

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

^ permalink raw reply related

* [PATCH v3 3/3] arm64: dts: exynos: Add support for s6e3hf2 panel device on TM2e board
From: Hoegeun Kwon @ 2017-04-18  8:40 UTC (permalink / raw)
  To: thierry.reding, airlied, robh+dt, mark.rutland, catalin.marinas,
	will.deacon, kgene, krzk
  Cc: devicetree, linux-samsung-soc, linux-kernel, dri-devel, javier,
	andi.shyti, Hoegeun Kwon, linux-arm-kernel
In-Reply-To: <1492504836-19225-1-git-send-email-hoegeun.kwon@samsung.com>

This patch add the panel device tree node for s6e3hf2 display
controller to TM2e dts.

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
index 694717a..b73e123 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
@@ -52,6 +52,17 @@
 	assigned-clock-rates = <278000000>, <400000000>;
 };
 
+&dsi {
+	panel@0 {
+		compatible = "samsung,s6e3hf2";
+		reg = <0>;
+		vdd3-supply = <&ldo27_reg>;
+		vci-supply = <&ldo28_reg>;
+		reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
+		enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
+	};
+};
+
 &ldo31_reg {
 	regulator-name = "TSP_VDD_1.8V_AP";
 	regulator-min-microvolt = <1800000>;
-- 
1.9.1

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

^ permalink raw reply related

* [PATCH v6 3/4] net: dsa: LAN9303: add I2C managed mode support
From: Juergen Borleis @ 2017-04-18  8:48 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem,
	devicetree, robh+dt, mark.rutland
In-Reply-To: <20170418084827.19613-1-jbe@pengutronix.de>

In this mode the switch device and the internal phys will be managed via
I2C interface. The MDIO interface is still supported, but for the
(emulated) CPU port only.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
CC: devicetree@vger.kernel.org
CC: robh+dt@kernel.org
CC: mark.rutland@arm.com
---
 .../devicetree/bindings/net/dsa/lan9303.txt        |  62 +++++++++++
 drivers/net/dsa/Kconfig                            |  16 +++
 drivers/net/dsa/Makefile                           |   2 +
 drivers/net/dsa/lan9303_i2c.c                      | 113 +++++++++++++++++++++
 4 files changed, 193 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/lan9303.txt
 create mode 100644 drivers/net/dsa/lan9303_i2c.c

diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
new file mode 100644
index 0000000000000..2edc2561467a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
@@ -0,0 +1,62 @@
+SMSC/MicroChip LAN9303 three port ethernet switch
+-------------------------------------------------
+
+Required properties:
+
+- compatible: should be "smsc,lan9303-i2c"
+
+Optional properties:
+
+- reset-gpios: GPIO to be used to reset the whole device
+- reset-duration: reset duration in milliseconds, defaults to 200 ms
+
+Subnodes:
+
+The integrated switch subnode should be specified according to the binding
+described in dsa/dsa.txt. The CPU port of this switch is always port 0.
+
+Note: always use 'reg = <0/1/2>;' for the three DSA ports, even if the device is
+configured to use 1/2/3 instead. This hardware configuration will be
+auto-detected and mapped accordingly.
+
+Example:
+
+I2C managed mode:
+
+	master: masterdevice@X {
+		status = "okay";
+
+		fixed-link { /* RMII fixed link to LAN9303 */
+			speed = <100>;
+			full-duplex;
+		};
+	};
+
+	switch: switch@a {
+		compatible = "smsc,lan9303-i2c";
+		reg = <0xa>;
+		status = "okay";
+		reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
+		reset-duration = <200>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 { /* RMII fixed link to master */
+				reg = <0>;
+				label = "cpu";
+				ethernet = <&master>;
+			};
+
+			port@1 { /* external port 1 */
+				reg = <1>;
+				label = "lan1;
+			};
+
+			port@2 { /* external port 2 */
+				reg = <2>;
+				label = "lan2";
+			};
+		};
+	};
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 31a2b229106dd..c56533bffc8ce 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -50,4 +50,20 @@ config NET_DSA_MT7530
 	  This enables support for the Mediatek MT7530 Ethernet switch
 	  chip.
 
+config NET_DSA_SMSC_LAN9303
+	tristate
+	select NET_DSA_TAG_LAN9303
+	---help---
+	  This enables support for the SMSC/Microchip LAN9303 3 port ethernet
+	  switch chips.
+
+config NET_DSA_SMSC_LAN9303_I2C
+	tristate "SMSC/Microchip LAN9303 3-ports 10/100 ethernet switch in I2C managed mode"
+	depends on NET_DSA
+	select NET_DSA_SMSC_LAN9303
+	select REGMAP_I2C
+	---help---
+	  Enable access functions if the SMSC/Microchip LAN9303 is configured
+	  for I2C managed mode.
+
 endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 2ae07f4fbf635..c1981ba18963f 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -3,6 +3,8 @@ obj-$(CONFIG_NET_DSA_BCM_SF2)	+= bcm-sf2.o
 bcm-sf2-objs			:= bcm_sf2.o bcm_sf2_cfp.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
 obj-$(CONFIG_NET_DSA_MT7530)	+= mt7530.o
+obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
+obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
 obj-y				+= b53/
 obj-y				+= mv88e6xxx/
 obj-$(CONFIG_NET_DSA_LOOP)	+= dsa_loop.o dsa_loop_bdinfo.o
diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
new file mode 100644
index 0000000000000..ab3ce0da5071a
--- /dev/null
+++ b/drivers/net/dsa/lan9303_i2c.c
@@ -0,0 +1,113 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Juergen Borleis <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+
+#include "lan9303.h"
+
+struct lan9303_i2c {
+	struct i2c_client *device;
+	struct lan9303 chip;
+};
+
+static const struct regmap_config lan9303_i2c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 1,
+	.can_multi_write = true,
+	.max_register = 0x0ff, /* address bits 0..1 are not used */
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+
+	.volatile_table = &lan9303_register_set,
+	.wr_table = &lan9303_register_set,
+	.rd_table = &lan9303_register_set,
+
+	.cache_type = REGCACHE_NONE,
+};
+
+static int lan9303_i2c_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct lan9303_i2c *sw_dev;
+	int ret;
+
+	sw_dev = devm_kzalloc(&client->dev, sizeof(struct lan9303_i2c),
+			      GFP_KERNEL);
+	if (!sw_dev)
+		return -ENOMEM;
+
+	sw_dev->chip.regmap = devm_regmap_init_i2c(client,
+						&lan9303_i2c_regmap_config);
+	if (IS_ERR(sw_dev->chip.regmap)) {
+		ret = PTR_ERR(sw_dev->chip.regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	/* link forward and backward */
+	sw_dev->device = client;
+	i2c_set_clientdata(client, sw_dev);
+	sw_dev->chip.dev = &client->dev;
+
+	ret = lan9303_probe(&sw_dev->chip, client->dev.of_node);
+	if (ret != 0)
+		return ret;
+
+	dev_info(&client->dev, "LAN9303 I2C driver loaded successfully\n");
+
+	return 0;
+}
+
+static int lan9303_i2c_remove(struct i2c_client *client)
+{
+	struct lan9303_i2c *sw_dev;
+
+	sw_dev = i2c_get_clientdata(client);
+	if (!sw_dev)
+		return -ENODEV;
+
+	return lan9303_remove(&sw_dev->chip);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct i2c_device_id lan9303_i2c_id[] = {
+	{ "lan9303", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, lan9303_i2c_id);
+
+static const struct of_device_id lan9303_i2c_of_match[] = {
+	{ .compatible = "smsc,lan9303-i2c", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, lan9303_i2c_of_match);
+
+static struct i2c_driver lan9303_i2c_driver = {
+	.driver = {
+		.name = "LAN9303_I2C",
+		.of_match_table = of_match_ptr(lan9303_i2c_of_match),
+	},
+	.probe = lan9303_i2c_probe,
+	.remove = lan9303_i2c_remove,
+	.id_table = lan9303_i2c_id,
+};
+module_i2c_driver(lan9303_i2c_driver);
+
+MODULE_AUTHOR("Juergen Borleis <kernel@pengutronix.de>");
+MODULE_DESCRIPTION("Driver for SMSC/Microchip LAN9303 three port ethernet switch in I2C managed mode");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

^ permalink raw reply related

* [PATCH v6 4/4] net: dsa: LAN9303: add MDIO managed mode support
From: Juergen Borleis @ 2017-04-18  8:48 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem,
	devicetree, robh+dt, mark.rutland, sr
In-Reply-To: <20170418084827.19613-1-jbe@pengutronix.de>

When the LAN9303 device is in MDIO manged mode, all register accesses must
be done via MDIO.

Please note: this code is compile time tested only due to the absence of such
configured hardware. It is based on a patch from Stefan Roese from 2014.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
CC: devicetree@vger.kernel.org
CC: robh+dt@kernel.org
CC: mark.rutland@arm.com
CC: sr@denx.de
---
 .../devicetree/bindings/net/dsa/lan9303.txt        |  45 ++++++-
 drivers/net/dsa/Kconfig                            |   8 ++
 drivers/net/dsa/Makefile                           |   1 +
 drivers/net/dsa/lan9303_mdio.c                     | 148 +++++++++++++++++++++
 4 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/dsa/lan9303_mdio.c

diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
index 2edc2561467a7..04f2965a44676 100644
--- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt
+++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
@@ -3,7 +3,10 @@ SMSC/MicroChip LAN9303 three port ethernet switch
 
 Required properties:
 
-- compatible: should be "smsc,lan9303-i2c"
+- compatible: should be
+  - "smsc,lan9303-i2c" for I2C managed mode
+    or
+  - "smsc,lan9303-mdio" for mdio managed mode
 
 Optional properties:
 
@@ -60,3 +63,43 @@ I2C managed mode:
 			};
 		};
 	};
+
+MDIO managed mode:
+
+	master: masterdevice@X {
+		status = "okay";
+		phy-handle = <&switch>;
+
+		mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			switch: switch-phy@0 {
+				compatible = "smsc,lan9303-mdio";
+				reg = <0>;
+				reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
+				reset-duration = <100>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						label = "cpu";
+						ethernet = <&master>;
+					};
+
+					port@1 { /* external port 1 */
+						reg = <1>;
+						label = "lan1;
+					};
+
+					port@2 { /* external port 2 */
+						reg = <2>;
+						label = "lan2";
+					};
+				};
+			};
+		};
+	};
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index c56533bffc8ce..131a5b1cbfc83 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -66,4 +66,12 @@ config NET_DSA_SMSC_LAN9303_I2C
 	  Enable access functions if the SMSC/Microchip LAN9303 is configured
 	  for I2C managed mode.
 
+config NET_DSA_SMSC_LAN9303_MDIO
+	tristate "SMSC/Microchip LAN9303 3-ports 10/100 ethernet switch in MDIO managed mode"
+	depends on NET_DSA
+	select NET_DSA_SMSC_LAN9303
+	---help---
+	  Enable access functions if the SMSC/Microchip LAN9303 is configured
+	  for MDIO managed mode.
+
 endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index c1981ba18963f..edd6303617365 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
 obj-$(CONFIG_NET_DSA_MT7530)	+= mt7530.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
+obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
 obj-y				+= b53/
 obj-y				+= mv88e6xxx/
 obj-$(CONFIG_NET_DSA_LOOP)	+= dsa_loop.o dsa_loop_bdinfo.o
diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
new file mode 100644
index 0000000000000..93c36c0541cf9
--- /dev/null
+++ b/drivers/net/dsa/lan9303_mdio.c
@@ -0,0 +1,148 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Juergen Borleis <kernel@pengutronix.de>
+ *
+ * Partially based on a patch from
+ * Copyright (c) 2014 Stefan Roese <sr@denx.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+
+#include "lan9303.h"
+
+/* Generate phy-addr and -reg from the input address */
+#define PHY_ADDR(x) ((((x) >> 6) + 0x10) & 0x1f)
+#define PHY_REG(x) (((x) >> 1) & 0x1f)
+
+struct lan9303_mdio {
+	struct mdio_device *device;
+	struct lan9303 chip;
+};
+
+static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val)
+{
+	mdio->bus->write(mdio->bus, PHY_ADDR(reg), PHY_REG(reg), val);
+}
+
+static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val)
+{
+	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+
+	mutex_lock(&sw_dev->device->bus->mdio_lock);
+	lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff);
+	lan9303_mdio_real_write(sw_dev->device, reg + 2, (val >> 16) & 0xffff);
+	mutex_unlock(&sw_dev->device->bus->mdio_lock);
+
+	return 0;
+}
+
+static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg)
+{
+	return mdio->bus->read(mdio->bus, PHY_ADDR(reg), PHY_REG(reg));
+}
+
+static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
+{
+	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+
+	mutex_lock(&sw_dev->device->bus->mdio_lock);
+	*val = lan9303_mdio_real_read(sw_dev->device, reg);
+	*val |= (lan9303_mdio_real_read(sw_dev->device, reg + 2) << 16);
+	mutex_unlock(&sw_dev->device->bus->mdio_lock);
+
+	return 0;
+}
+
+static const struct regmap_config lan9303_mdio_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 1,
+	.can_multi_write = true,
+	.max_register = 0x0ff, /* address bits 0..1 are not used */
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+
+	.volatile_table = &lan9303_register_set,
+	.wr_table = &lan9303_register_set,
+	.rd_table = &lan9303_register_set,
+
+	.reg_read = lan9303_mdio_read,
+	.reg_write = lan9303_mdio_write,
+
+	.cache_type = REGCACHE_NONE,
+};
+
+static int lan9303_mdio_probe(struct mdio_device *mdiodev)
+{
+	struct lan9303_mdio *sw_dev;
+	int ret;
+
+	sw_dev = devm_kzalloc(&mdiodev->dev, sizeof(struct lan9303_mdio),
+			      GFP_KERNEL);
+	if (!sw_dev)
+		return -ENOMEM;
+
+	sw_dev->chip.regmap = devm_regmap_init(&mdiodev->dev, NULL, sw_dev,
+						&lan9303_mdio_regmap_config);
+	if (IS_ERR(sw_dev->chip.regmap)) {
+		ret = PTR_ERR(sw_dev->chip.regmap);
+		dev_err(&mdiodev->dev, "regmap init failed: %d\n", ret);
+		return ret;
+	}
+
+	/* link forward and backward */
+	sw_dev->device = mdiodev;
+	dev_set_drvdata(&mdiodev->dev, sw_dev);
+	sw_dev->chip.dev = &mdiodev->dev;
+
+	ret = lan9303_probe(&sw_dev->chip, mdiodev->dev.of_node);
+	if (ret != 0)
+		return ret;
+
+	dev_info(&mdiodev->dev, "LAN9303 MDIO driver loaded successfully\n");
+
+	return 0;
+}
+
+static void lan9303_mdio_remove(struct mdio_device *mdiodev)
+{
+	struct lan9303_mdio *sw_dev = dev_get_drvdata(&mdiodev->dev);
+
+	if (!sw_dev)
+		return;
+
+	lan9303_remove(&sw_dev->chip);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct of_device_id lan9303_mdio_of_match[] = {
+	{ .compatible = "smsc,lan9303-mdio" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, lan9303_mdio_of_match);
+
+static struct mdio_driver lan9303_mdio_driver = {
+	.mdiodrv.driver = {
+		.name = "LAN9303_MDIO",
+		.of_match_table = of_match_ptr(lan9303_mdio_of_match),
+	},
+	.probe  = lan9303_mdio_probe,
+	.remove = lan9303_mdio_remove,
+};
+mdio_module_driver(lan9303_mdio_driver);
+
+MODULE_AUTHOR("Stefan Roese <sr@denx.de>, Juergen Borleis <kernel@pengutronix.de>");
+MODULE_DESCRIPTION("Driver for SMSC/Microchip LAN9303 three port ethernet switch in MDIO managed mode");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
From: Greg Kroah-Hartman @ 2017-04-18  8:51 UTC (permalink / raw)
  To: Peter Rosin
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang, Rob Herring,
	Mark Rutland, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Colin Ian King,
	Paul Gortmaker, Philipp Zabel, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <1492101794-13444-4-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>

On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> +config MUX_GPIO
> +	tristate "GPIO-controlled Multiplexer"
> +	depends on OF && GPIOLIB

Why have the gpio and mux core in the same patch?

And why does this depend on OF?


> +	help
> +	  GPIO-controlled Multiplexer controller.
> +
> +	  The driver builds a single multiplexer controller using a number
> +	  of gpio pins. For N pins, there will be 2^N possible multiplexer
> +	  states. The GPIO pins can be connected (by the hardware) to several
> +	  multiplexers, which in that case will be operated in parallel.
> +
> +	  To compile the driver as a module, choose M here: the module will
> +	  be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index 000000000000..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
> +obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> new file mode 100644
> index 000000000000..66a8bccfc3d7
> --- /dev/null
> +++ b/drivers/mux/mux-core.c
> @@ -0,0 +1,422 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +/*
> + * The idle-as-is "state" is not an actual state that may be selected, it
> + * only implies that the state should not be changed. So, use that state
> + * as indication that the cached state of the multiplexer is unknown.
> + */
> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
> +
> +static struct class mux_class = {
> +	.name = "mux",
> +	.owner = THIS_MODULE,
> +};

No Documentation/ABI/ update for your sysfs files?  Please do so.

> +
> +static int __init mux_init(void)
> +{
> +	return class_register(&mux_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);

When your module is unloaded, you forgot to clean this structure up with
what was done with it.

> +
> +static void mux_chip_release(struct device *dev)
> +{
> +	struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> +	ida_simple_remove(&mux_ida, mux_chip->id);
> +	kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> +	.name = "mux-chip",
> +	.release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> +				unsigned int controllers, size_t sizeof_priv)
> +{
> +	struct mux_chip *mux_chip;
> +	int i;
> +
> +	if (WARN_ON(!dev || !controllers))
> +		return NULL;
> +
> +	mux_chip = kzalloc(sizeof(*mux_chip) +
> +			   controllers * sizeof(*mux_chip->mux) +
> +			   sizeof_priv, GFP_KERNEL);
> +	if (!mux_chip)
> +		return NULL;

You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
it, just curious...)

> +
> +	mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> +	mux_chip->dev.class = &mux_class;
> +	mux_chip->dev.type = &mux_type;
> +	mux_chip->dev.parent = dev;
> +	mux_chip->dev.of_node = dev->of_node;
> +	dev_set_drvdata(&mux_chip->dev, mux_chip);
> +
> +	mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
> +	if (mux_chip->id < 0) {
> +		pr_err("muxchipX failed to get a device id\n");
> +		kfree(mux_chip);
> +		return NULL;
> +	}
> +	dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
> +
> +	mux_chip->controllers = controllers;
> +	for (i = 0; i < controllers; ++i) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +
> +		mux->chip = mux_chip;
> +		init_rwsem(&mux->lock);
> +		mux->cached_state = MUX_CACHE_UNKNOWN;
> +		mux->idle_state = MUX_IDLE_AS_IS;
> +	}
> +
> +	device_initialize(&mux_chip->dev);

Why are you not registering the device here as well?  Why have this be a
two step process?

> +
> +	return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> +	int ret = mux->chip->ops->set(mux, state);
> +
> +	mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
> +
> +	return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < mux_chip->controllers; ++i) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +
> +		if (mux->idle_state == mux->cached_state)
> +			continue;
> +
> +		ret = mux_control_set(mux, mux->idle_state);
> +		if (ret < 0) {
> +			dev_err(&mux_chip->dev, "unable to set idle state\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = device_add(&mux_chip->dev);
> +	if (ret < 0)
> +		dev_err(&mux_chip->dev,
> +			"device_add failed in mux_chip_register: %d\n", ret);

Did you run checkpatch.pl in strict mode on this new file?  Please do so :)

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);
> +
> +void mux_chip_unregister(struct mux_chip *mux_chip)
> +{
> +	device_del(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
> +
> +void mux_chip_free(struct mux_chip *mux_chip)
> +{
> +	if (!mux_chip)
> +		return;
> +
> +	put_device(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_free);
> +
> +static void devm_mux_chip_release(struct device *dev, void *res)
> +{
> +	struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> +	mux_chip_free(mux_chip);
> +}
> +
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> +				     unsigned int controllers,
> +				     size_t sizeof_priv)
> +{
> +	struct mux_chip **ptr, *mux_chip;
> +
> +	ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
> +	if (!mux_chip) {
> +		devres_free(ptr);
> +		return NULL;
> +	}
> +
> +	*ptr = mux_chip;
> +	devres_add(dev, ptr);
> +
> +	return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);


Having devm functions that create/destroy other struct devices worries
me, do we have other examples of this happening today?  Are you sure you
got the reference counting all correct?

> +
> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> +{
> +	struct mux_chip **r = res;
> +
> +	if (WARN_ON(!r || !*r))

How can this happen?

> +		return 0;
> +
> +	return *r == data;
> +}
> +
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> +{
> +	WARN_ON(devres_release(dev, devm_mux_chip_release,
> +			       devm_mux_chip_match, mux_chip));

What can someone do with these WARN_ON() splats in the kernel log?


> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_free);
> +
> +static void devm_mux_chip_reg_release(struct device *dev, void *res)
> +{
> +	struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> +	mux_chip_unregister(mux_chip);
> +}
> +
> +int devm_mux_chip_register(struct device *dev,
> +			   struct mux_chip *mux_chip)
> +{
> +	struct mux_chip **ptr;
> +	int res;
> +
> +	ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	res = mux_chip_register(mux_chip);
> +	if (res) {
> +		devres_free(ptr);
> +		return res;
> +	}
> +
> +	*ptr = mux_chip;
> +	devres_add(dev, ptr);
> +
> +	return res;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> +
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
> +{
> +	WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
> +			       devm_mux_chip_match, mux_chip));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
> +
> +int mux_control_select(struct mux_control *mux, int state)
> +{
> +	int ret;
> +
> +	if (down_read_trylock(&mux->lock)) {
> +		if (mux->cached_state == state)
> +			return 0;
> +
> +		/* Sigh, the mux needs updating... */
> +		up_read(&mux->lock);
> +	}
> +
> +	/* ...or it's just contended. */
> +	down_write(&mux->lock);

Why use a read/write lock at all?  Have you tested this to verify it
really is faster and needed?

> +
> +	if (mux->cached_state == state) {
> +		/*
> +		 * Hmmm, someone else changed the mux to my liking.
> +		 * That makes me wonder how long I waited for nothing?
> +		 */
> +		downgrade_write(&mux->lock);

Oh that always scares me...  Are you _sure_ this is correct?  And
needed?

> +		return 0;
> +	}
> +
> +	ret = mux_control_set(mux, state);
> +	if (ret < 0) {
> +		if (mux->idle_state != MUX_IDLE_AS_IS)
> +			mux_control_set(mux, mux->idle_state);
> +
> +		up_write(&mux->lock);
> +		return ret;
> +	}
> +
> +	downgrade_write(&mux->lock);
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_select);
> +
> +int mux_control_deselect(struct mux_control *mux)
> +{
> +	int ret = 0;
> +
> +	if (mux->idle_state != MUX_IDLE_AS_IS &&
> +	    mux->idle_state != mux->cached_state)
> +		ret = mux_control_set(mux, mux->idle_state);
> +
> +	up_read(&mux->lock);

You require a lock to be held for a "global" function?  Without
documentation?  Or even a sparse marking?  That's asking for trouble...

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_deselect);
> +
> +static int of_dev_node_match(struct device *dev, const void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> +{
> +	struct device *dev;
> +
> +	dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
> +
> +	return dev ? to_mux_chip(dev) : NULL;
> +}
> +
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct of_phandle_args args;
> +	struct mux_chip *mux_chip;
> +	unsigned int controller;
> +	int index = 0;
> +	int ret;
> +
> +	if (mux_name) {
> +		index = of_property_match_string(np, "mux-control-names",
> +						 mux_name);
> +		if (index < 0) {
> +			dev_err(dev, "mux controller '%s' not found\n",
> +				mux_name);
> +			return ERR_PTR(index);
> +		}
> +	}
> +
> +	ret = of_parse_phandle_with_args(np,
> +					 "mux-controls", "#mux-control-cells",
> +					 index, &args);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
> +			np->full_name, mux_name ?: "", index);
> +		return ERR_PTR(ret);
> +	}
> +
> +	mux_chip = of_find_mux_chip_by_node(args.np);
> +	of_node_put(args.np);
> +	if (!mux_chip)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (args.args_count > 1 ||
> +	    (!args.args_count && (mux_chip->controllers > 1))) {
> +		dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
> +			np->full_name, args.np->full_name);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	controller = 0;
> +	if (args.args_count)
> +		controller = args.args[0];
> +
> +	if (controller >= mux_chip->controllers) {
> +		dev_err(dev, "%s: bad mux controller %u specified in %s\n",
> +			np->full_name, controller, args.np->full_name);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	get_device(&mux_chip->dev);
> +	return &mux_chip->mux[controller];
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get);
> +
> +void mux_control_put(struct mux_control *mux)
> +{
> +	put_device(&mux->chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_put);
> +
> +static void devm_mux_control_release(struct device *dev, void *res)
> +{
> +	struct mux_control *mux = *(struct mux_control **)res;
> +
> +	mux_control_put(mux);
> +}
> +
> +struct mux_control *devm_mux_control_get(struct device *dev,
> +					 const char *mux_name)
> +{
> +	struct mux_control **ptr, *mux;
> +
> +	ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux = mux_control_get(dev, mux_name);
> +	if (IS_ERR(mux)) {
> +		devres_free(ptr);
> +		return mux;
> +	}
> +
> +	*ptr = mux;
> +	devres_add(dev, ptr);
> +
> +	return mux;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get);
> +
> +static int devm_mux_control_match(struct device *dev, void *res, void *data)
> +{
> +	struct mux_control **r = res;
> +
> +	if (WARN_ON(!r || !*r))
> +		return 0;

Same here, how can this happen?

> +
> +	return *r == data;
> +}
> +
> +void devm_mux_control_put(struct device *dev, struct mux_control *mux)
> +{
> +	WARN_ON(devres_release(dev, devm_mux_control_release,
> +			       devm_mux_control_match, mux));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_put);
> +
> +/*
> + * Using subsys_initcall instead of module_init here to ensure - for the
> + * non-modular case - that the subsystem is initialized when mux consumers
> + * and mux controllers start to use it /without/ relying on link order.
> + * For the modular case, the ordering is ensured with module dependencies.
> + */
> +subsys_initcall(mux_init);

Even with subsys_initcall you are relying on link order, you do realize
that?  What about other subsystems that rely on this?  :)


> +
> +MODULE_DESCRIPTION("Multiplexer subsystem");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
> new file mode 100644
> index 000000000000..227d3572e6db
> --- /dev/null
> +++ b/drivers/mux/mux-gpio.c
> @@ -0,0 +1,114 @@
> +/*
> + * GPIO-controlled multiplexer driver
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +struct mux_gpio {
> +	struct gpio_descs *gpios;
> +	int *val;
> +};
> +
> +static int mux_gpio_set(struct mux_control *mux, int state)
> +{
> +	struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> +	int i;
> +
> +	for (i = 0; i < mux_gpio->gpios->ndescs; i++)
> +		mux_gpio->val[i] = (state >> i) & 1;
> +
> +	gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> +				       mux_gpio->gpios->desc,
> +				       mux_gpio->val);
> +
> +	return 0;
> +}
> +
> +static const struct mux_control_ops mux_gpio_ops = {
> +	.set = mux_gpio_set,
> +};
> +
> +static const struct of_device_id mux_gpio_dt_ids[] = {
> +	{ .compatible = "gpio-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
> +
> +static int mux_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mux_chip *mux_chip;
> +	struct mux_gpio *mux_gpio;
> +	int pins;
> +	s32 idle_state;
> +	int ret;
> +
> +	pins = gpiod_count(dev, "mux");
> +	if (pins < 0)
> +		return pins;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
> +				       pins * sizeof(*mux_gpio->val));
> +	if (!mux_chip)
> +		return -ENOMEM;
> +
> +	mux_gpio = mux_chip_priv(mux_chip);
> +	mux_gpio->val = (int *)(mux_gpio + 1);
> +	mux_chip->ops = &mux_gpio_ops;
> +
> +	mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
> +	if (IS_ERR(mux_gpio->gpios)) {
> +		ret = PTR_ERR(mux_gpio->gpios);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get gpios\n");
> +		return ret;
> +	}
> +	WARN_ON(pins != mux_gpio->gpios->ndescs);
> +	mux_chip->mux->states = 1 << pins;
> +
> +	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
> +	if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
> +		if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
> +			dev_err(dev, "invalid idle-state %u\n", idle_state);
> +			return -EINVAL;
> +		}
> +
> +		mux_chip->mux->idle_state = idle_state;
> +	}
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev, "%u-way mux-controller registered\n",
> +		 mux_chip->mux->states);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mux_gpio_driver = {
> +	.driver = {
> +		.name = "gpio-mux",
> +		.of_match_table	= of_match_ptr(mux_gpio_dt_ids),
> +	},
> +	.probe = mux_gpio_probe,
> +};
> +module_platform_driver(mux_gpio_driver);
> +
> +MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index 000000000000..febdde4246df
> --- /dev/null
> +++ b/include/linux/mux.h
> @@ -0,0 +1,252 @@
> +/*
> + * mux.h - definitions for the multiplexer interface
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _LINUX_MUX_H
> +#define _LINUX_MUX_H
> +
> +#include <linux/device.h>
> +#include <linux/rwsem.h>
> +
> +struct mux_chip;
> +struct mux_control;
> +struct platform_device;
> +
> +/**
> + * struct mux_control_ops -	Mux controller operations for a mux chip.
> + * @set:			Set the state of the given mux controller.
> + */
> +struct mux_control_ops {
> +	int (*set)(struct mux_control *mux, int state);
> +};
> +
> +/* These defines match the constants from the dt-bindings. On purpose. */

Why on purpose?

> +#define MUX_IDLE_AS_IS      (-1)
> +#define MUX_IDLE_DISCONNECT (-2)
> +
> +/**
> + * struct mux_control -	Represents a mux controller.
> + * @lock:		Protects the mux controller state.
> + * @chip:		The mux chip that is handling this mux controller.
> + * @states:		The number of mux controller states.
> + * @cached_state:	The current mux controller state, or -1 if none.
> + * @idle_state:		The mux controller state to use when inactive, or one
> + *			of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
> + */
> +struct mux_control {
> +	struct rw_semaphore lock; /* protects the state of the mux */
> +
> +	struct mux_chip *chip;
> +
> +	unsigned int states;
> +	int cached_state;
> +	int idle_state;
> +};
> +
> +/**
> + * struct mux_chip -	Represents a chip holding mux controllers.
> + * @controllers:	Number of mux controllers handled by the chip.
> + * @mux:		Array of mux controllers that are handled.
> + * @dev:		Device structure.
> + * @id:			Used to identify the device internally.
> + * @ops:		Mux controller operations.
> + */
> +struct mux_chip {
> +	unsigned int controllers;
> +	struct mux_control *mux;
> +	struct device dev;
> +	int id;
> +
> +	const struct mux_control_ops *ops;
> +};
> +
> +#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
> +
> +/**
> + * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
> + * @mux_chip: The mux-chip to get the private memory from.
> + *
> + * Return: Pointer to the private memory reserved by the allocator.
> + */
> +static inline void *mux_chip_priv(struct mux_chip *mux_chip)
> +{
> +	return &mux_chip->mux[mux_chip->controllers];
> +}
> +
> +/**
> + * mux_chip_alloc() - Allocate a mux-chip.
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> +				unsigned int controllers, size_t sizeof_priv);
> +

Don't put kernel doc comments in a .h file, they normally go into the .c
file, next to the code itself.  That makes it easier to fix up and
realise when they need to be changed when the code changes.  The .h file
rarely changes.


> +/**
> + * mux_chip_register() - Register a mux-chip, thus readying the controllers
> + *			 for use.
> + * @mux_chip: The mux-chip to register.
> + *
> + * Do not retry registration of the same mux-chip on failure. You should
> + * instead put it away with mux_chip_free() and allocate a new one, if you
> + * for some reason would like to retry registration.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int mux_chip_register(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_unregister() - Take the mux-chip off-line.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * mux_chip_unregister() reverses the effects of mux_chip_register().
> + * But not completely, you should not try to call mux_chip_register()
> + * on a mux-chip that has been registered before.
> + */
> +void mux_chip_unregister(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_free() - Free the mux-chip for good.
> + * @mux_chip: The mux-chip to free.
> + *
> + * mux_chip_free() reverses the effects of mux_chip_alloc().
> + */
> +void mux_chip_free(struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * See mux_chip_alloc() for more details.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> +				     unsigned int controllers,
> +				     size_t sizeof_priv);
> +
> +/**
> + * devm_mux_chip_register() - Resource-managed version mux_chip_register().
> + * @dev: The parent device implementing the mux interface.
> + * @mux_chip: The mux-chip to register.
> + *
> + * See mux_chip_register() for more details.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
> + * @dev: The device that originally registered the mux-chip.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * See mux_chip_unregister() for more details.
> + *
> + * Note that you do not normally need to call this function.

Odd, then why is it exported???


> + */
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_free() - Resource-managed version mux_chip_free().
> + * @dev: The device that originally got the mux-chip.
> + * @mux_chip: The mux-chip to free.
> + *
> + * See mux_chip_free() for more details.
> + *
> + * Note that you do not normally need to call this function.
> + */
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * mux_control_select() - Select the given multiplexer state.
> + * @mux: The mux-control to request a change of state from.
> + * @state: The new requested state.
> + *
> + * Make sure to call mux_control_deselect() when the operation is complete and
> + * the mux-control is free for others to use, but do not call
> + * mux_control_deselect() if mux_control_select() fails.
> + *
> + * Return: 0 if the requested state was already active, or 1 it the
> + * mux-control state was changed to the requested state. Or a negative
> + * errno on error.
> + *
> + * Note that the difference in return value of zero or one is of
> + * questionable value; especially if the mux-control has several independent
> + * consumers, which is something the consumers should perhaps not be making
> + * assumptions about.

I don't understand this note, what is a user of this api supposed to do
differently between 1 and 0?  Why make the difference at all?

And I agree with the comment to split this up into 2 different .h files,
if possible.

thanks,

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

^ permalink raw reply

* Re: [PATCH v4 05/11] drm/sun4i: abstract a engine type
From: Maxime Ripard @ 2017-04-18  8:55 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Rob Herring, Chen-Yu Tsai, David Airlie, Jernej Skrabec,
	linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170416120849.54542-6-icenowy-h8G6r0blFSE@public.gmane.org>

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

Hi,

Thanks for that rework.

On Sun, Apr 16, 2017 at 08:08:43PM +0800, Icenowy Zheng wrote:
> As we are going to add support for the Allwinner DE2 engine in sun4i-drm
> driver, we will finally have two types of display engines -- the DE1
> backend and the DE2 mixer. They both do some display blending and feed
> graphics data to TCON, so I choose to call them both "engine" here.
> 
> Abstract the engine type to void * and a ops struct, which contains
> functions that should be called outside the engine-specified code (in
> TCON, CRTC or TV Encoder code).
> 
> A dedicated Kconfig option is also added to control whether
> sun4i-backend-specified code (sun4i_backend.c and sun4i_layer.c) should
> be built. As we removed the codes in CRTC code that directly call the
> layer code, we can now extract the layer part and combine it with the
> backend part into a new module, sun4i-backend.ko.
> 
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> Changes in v4:
> - Comments to tag the color correction functions as optional.
> - Check before calling the optional functions.
> - Change layers_init to satisfy new PATCH v4 04/11.
> 
>  drivers/gpu/drm/sun4i/Kconfig         | 10 ++++++++++
>  drivers/gpu/drm/sun4i/Makefile        |  6 ++++--
>  drivers/gpu/drm/sun4i/sun4i_backend.c | 26 +++++++++++++++++++-------
>  drivers/gpu/drm/sun4i/sun4i_backend.h |  5 -----
>  drivers/gpu/drm/sun4i/sun4i_crtc.c    | 14 +++++++-------
>  drivers/gpu/drm/sun4i/sun4i_crtc.h    |  7 ++++---
>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  3 ++-
>  drivers/gpu/drm/sun4i/sun4i_layer.c   |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_layer.h   |  3 ++-
>  drivers/gpu/drm/sun4i/sun4i_tcon.c    |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_tv.c      | 11 ++++++-----
>  drivers/gpu/drm/sun4i/sunxi_engine.h  | 35 +++++++++++++++++++++++++++++++++++
>  12 files changed, 91 insertions(+), 33 deletions(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sunxi_engine.h
> 
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index a4b357db8856..5a8227f37cc4 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -12,3 +12,13 @@ config DRM_SUN4I
>  	  Choose this option if you have an Allwinner SoC with a
>  	  Display Engine. If M is selected the module will be called
>  	  sun4i-drm.
> +
> +config DRM_SUN4I_BACKEND
> +	tristate "Support for Allwinner A10 Display Engine Backend"
> +	depends on DRM_SUN4I
> +	default DRM_SUN4I
> +	help
> +	  Choose this option if you have an Allwinner SoC with the
> +	  original Allwinner Display Engine, which has a backend to
> +	  do some alpha blending and feed graphics to TCON. If M is
> +	  selected the module will be called sun4i-backend.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 59b757350a1f..1db1068b9be1 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,9 +5,11 @@ sun4i-tcon-y += sun4i_tcon.o
>  sun4i-tcon-y += sun4i_rgb.o
>  sun4i-tcon-y += sun4i_dotclock.o
>  sun4i-tcon-y += sun4i_crtc.o
> -sun4i-tcon-y += sun4i_layer.o
> +
> +sun4i-backend-y += sun4i_layer.o
> +sun4i-backend-y += sun4i_backend.o
>  
>  obj-$(CONFIG_DRM_SUN4I)		+= sun4i-drm.o sun4i-tcon.o
> -obj-$(CONFIG_DRM_SUN4I)		+= sun4i_backend.o
> +obj-$(CONFIG_DRM_SUN4I_BACKEND)	+= sun4i-backend.o
>  obj-$(CONFIG_DRM_SUN4I)		+= sun6i_drc.o
>  obj-$(CONFIG_DRM_SUN4I)		+= sun4i_tv.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index d660741ba475..a16c96a002a4 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -23,6 +23,8 @@
>  
>  #include "sun4i_backend.h"
>  #include "sun4i_drv.h"
> +#include "sun4i_layer.h"
> +#include "sunxi_engine.h"
>  
>  static const u32 sunxi_rgb2yuv_coef[12] = {
>  	0x00000107, 0x00000204, 0x00000064, 0x00000108,
> @@ -30,9 +32,10 @@ static const u32 sunxi_rgb2yuv_coef[12] = {
>  	0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
>  };
>  
> -void sun4i_backend_apply_color_correction(struct sun4i_backend *backend)
> +static void sun4i_backend_apply_color_correction(void *engine)
>  {
>  	int i;
> +	struct sun4i_backend *backend = engine;

I'm not really fond of passing an opaque pointer here, and hope that
things will work out.

I think having a common structure, holding the common thingsand a more
specific structure for that one would work better.

Something like

struct sunxi_engine {
       struct regmap	*regs;
};

struct sun4i_backend {
       struct sunxi_engine	engine;

	struct clk		*sat_clk;
	struct reset_control	*sat_reset;
	
};

static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
       struct sun4i_backend *backend = container_of(engine, struct sun4i_backend, engine);

...

> +static const struct sunxi_engine_ops sun4i_backend_engine_ops = {
> +	.commit = sun4i_backend_commit,
> +	.layers_init = sun4i_layers_init,
> +	.apply_color_correction = sun4i_backend_apply_color_correction,
> +	.disable_color_correction = sun4i_backend_disable_color_correction,

Please align the values with tabs, like done in the other structures
created that way in this driver.

>  static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -56,7 +55,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	DRM_DEBUG_DRIVER("Committing plane changes\n");
>  
> -	sun4i_backend_commit(scrtc->backend);
> +	scrtc->engine_ops->commit(scrtc->engine);

You rely on the backend having setup things properly, which is pretty
fragile. Ideally, you should have a function to check that engine_ops
and commit is !NULL, and call it, and the consumers would use that
function...

> @@ -362,7 +361,9 @@ static void sun4i_tv_disable(struct drm_encoder *encoder)
>  	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
>  			   SUN4I_TVE_EN_ENABLE,
>  			   0);
> -	sun4i_backend_disable_color_correction(backend);
> +
> +	if (crtc->engine_ops->disable_color_correction)
> +		crtc->engine_ops->disable_color_correction(crtc->engine);
>  }

... Instead of having to do it in some cases, but not always ...

>  static void sun4i_tv_enable(struct drm_encoder *encoder)
> @@ -370,11 +371,11 @@ static void sun4i_tv_enable(struct drm_encoder *encoder)
>  	struct sun4i_tv *tv = drm_encoder_to_sun4i_tv(encoder);
>  	struct sun4i_crtc *crtc = drm_crtc_to_sun4i_crtc(encoder->crtc);
>  	struct sun4i_tcon *tcon = crtc->tcon;
> -	struct sun4i_backend *backend = crtc->backend;
>  
>  	DRM_DEBUG_DRIVER("Enabling the TV Output\n");
>  
> -	sun4i_backend_apply_color_correction(backend);
> +	if (crtc->engine_ops->apply_color_correction)
> +		crtc->engine_ops->apply_color_correction(crtc->engine);
>  
>  	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
>  			   SUN4I_TVE_EN_ENABLE,
> diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h b/drivers/gpu/drm/sun4i/sunxi_engine.h
> new file mode 100644
> index 000000000000..a9128abda66f
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef _SUNXI_ENGINE_H_
> +#define _SUNXI_ENGINE_H_
> +
> +struct sun4i_crtc;
> +
> +struct sunxi_engine_ops {
> +	/* Commit the changes to the engine */
> +	void (*commit)(void *engine);
> +	/* Initialize layers (planes) for this engine */
> +	struct drm_plane **(*layers_init)(struct drm_device *drm,
> +					  struct sun4i_crtc *crtc);
> +
> +	/*
> +	 * These are optional functions for the TV Encoder. Please check
> +	 * their presence before calling them.
> +	 *
> +	 * The first function applies the color space correction needed
> +	 * for outputing correct TV signal.
> +	 *
> +	 * The second function disabled the correction.
> +	 */
> +	void (*apply_color_correction)(void *engine);
> +	void (*disable_color_correction)(void *engine);

... And have to document it.

Please also use kerneldoc for those comments.

Thanks again!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH v4 06/11] drm/sun4i: add support for Allwinner DE2 mixers
From: Maxime Ripard @ 2017-04-18  9:00 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Rob Herring, Chen-Yu Tsai, David Airlie, Jernej Skrabec,
	linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170416120849.54542-7-icenowy-h8G6r0blFSE@public.gmane.org>

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

On Sun, Apr 16, 2017 at 08:08:44PM +0800, Icenowy Zheng wrote:
> Allwinner have a new "Display Engine 2.0" in their new SoCs, which comes
> with mixers to do graphic processing and feed data to TCON, like the old
> backends and frontends.
> 
> Add support for the mixer on Allwinner V3s SoC; it's the simplest one.
> 
> Currently a lot of functions are still missing -- more investigations
> are needed to gain enough information for them.
> 
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> Changes in v4:
> - Killed some dead code according to Jernej.
> 
>  drivers/gpu/drm/sun4i/Kconfig       |  10 +
>  drivers/gpu/drm/sun4i/Makefile      |   4 +
>  drivers/gpu/drm/sun4i/sun8i_layer.c | 142 ++++++++++++++
>  drivers/gpu/drm/sun4i/sun8i_layer.h |  36 ++++
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 381 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/sun4i/sun8i_mixer.h | 131 +++++++++++++
>  6 files changed, 704 insertions(+)
>  create mode 100644 drivers/gpu/drm/sun4i/sun8i_layer.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun8i_layer.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun8i_mixer.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun8i_mixer.h
> 
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 5a8227f37cc4..15557484520d 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -22,3 +22,13 @@ config DRM_SUN4I_BACKEND
>  	  original Allwinner Display Engine, which has a backend to
>  	  do some alpha blending and feed graphics to TCON. If M is
>  	  selected the module will be called sun4i-backend.
> +
> +config DRM_SUN4I_SUN8I_MIXER
> +	tristate "Support for Allwinner Display Engine 2.0 Mixer"
> +	depends on DRM_SUN4I
> +	default MACH_SUN8I
> +	help
> +	  Choose this option if you have an Allwinner SoC with the
> +	  Allwinner Display Engine 2.0, which has a mixer to do some
> +	  graphics mixture and feed graphics to TCON, If M is
> +	  selected the module will be called sun8i-mixer.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 1db1068b9be1..7625c2dad1bb 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -9,7 +9,11 @@ sun4i-tcon-y += sun4i_crtc.o
>  sun4i-backend-y += sun4i_layer.o
>  sun4i-backend-y += sun4i_backend.o
>  
> +sun8i-mixer-y += sun8i_layer.o
> +sun8i-mixer-y += sun8i_mixer.o
> +
>  obj-$(CONFIG_DRM_SUN4I)		+= sun4i-drm.o sun4i-tcon.o
>  obj-$(CONFIG_DRM_SUN4I_BACKEND)	+= sun4i-backend.o
> +obj-$(CONFIG_DRM_SUN4I_SUN8I_MIXER) += sun8i-mixer.o

Please align the value using tabs.

>  obj-$(CONFIG_DRM_SUN4I)		+= sun6i_drc.o
>  obj-$(CONFIG_DRM_SUN4I)		+= sun4i_tv.o
> diff --git a/drivers/gpu/drm/sun4i/sun8i_layer.c b/drivers/gpu/drm/sun4i/sun8i_layer.c
> new file mode 100644
> index 000000000000..d70a90d963b0
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun8i_layer.c
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (C) Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + *
> + * Based on sun4i_layer.h, which is:
> + *   Copyright (C) 2015 Free Electrons
> + *   Copyright (C) 2015 NextThing Co
> + *
> + *   Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drmP.h>
> +
> +#include "sun4i_crtc.h"
> +#include "sun8i_layer.h"
> +#include "sun8i_mixer.h"
> +
> +struct sun8i_plane_desc {
> +	       enum drm_plane_type     type;
> +	       const uint32_t          *formats;
> +	       uint32_t                nformats;
> +};
> +
> +static int sun8i_mixer_layer_atomic_check(struct drm_plane *plane,
> +					    struct drm_plane_state *state)
> +{
> +	return 0;
> +}
> +
> +static void sun8i_mixer_layer_atomic_disable(struct drm_plane *plane,
> +					       struct drm_plane_state *old_state)
> +{
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
> +	struct sun8i_mixer *mixer = layer->mixer;
> +
> +	sun8i_mixer_layer_enable(mixer, layer->id, false);
> +}
> +
> +static void sun8i_mixer_layer_atomic_update(struct drm_plane *plane,
> +					      struct drm_plane_state *old_state)
> +{
> +	struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
> +	struct sun8i_mixer *mixer = layer->mixer;
> +
> +	sun8i_mixer_update_layer_coord(mixer, layer->id, plane);
> +	sun8i_mixer_update_layer_formats(mixer, layer->id, plane);
> +	sun8i_mixer_update_layer_buffer(mixer, layer->id, plane);
> +	sun8i_mixer_layer_enable(mixer, layer->id, true);
> +}
> +
> +static struct drm_plane_helper_funcs sun8i_mixer_layer_helper_funcs = {
> +	.atomic_check	= sun8i_mixer_layer_atomic_check,
> +	.atomic_disable	= sun8i_mixer_layer_atomic_disable,
> +	.atomic_update	= sun8i_mixer_layer_atomic_update,
> +};
> +
> +static const struct drm_plane_funcs sun8i_mixer_layer_funcs = {
> +	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
> +	.destroy		= drm_plane_cleanup,
> +	.disable_plane		= drm_atomic_helper_disable_plane,
> +	.reset			= drm_atomic_helper_plane_reset,
> +	.update_plane		= drm_atomic_helper_update_plane,
> +};
> +
> +static const uint32_t sun8i_mixer_layer_formats[] = {
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static const struct sun8i_plane_desc sun8i_mixer_planes[] = {
> +	{
> +		.type = DRM_PLANE_TYPE_PRIMARY,
> +		.formats = sun8i_mixer_layer_formats,
> +		.nformats = ARRAY_SIZE(sun8i_mixer_layer_formats),
> +	},
> +};
> +
> +static struct sun8i_layer *sun8i_layer_init_one(struct drm_device *drm,
> +						struct sun8i_mixer *mixer,
> +						const struct sun8i_plane_desc *plane)
> +{
> +	struct sun8i_layer *layer;
> +	int ret;
> +
> +	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> +	if (!layer)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* possible crtcs are set later */
> +	ret = drm_universal_plane_init(drm, &layer->plane, 0,
> +				       &sun8i_mixer_layer_funcs,
> +				       plane->formats, plane->nformats,
> +				       plane->type, NULL);
> +	if (ret) {
> +		dev_err(drm->dev, "Couldn't initialize layer\n");
> +		return ERR_PTR(ret);
> +	}
> +
> +	drm_plane_helper_add(&layer->plane,
> +			     &sun8i_mixer_layer_helper_funcs);
> +	layer->mixer = mixer;
> +
> +	return layer;
> +}
> +
> +struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> +				     struct sun4i_crtc *crtc)
> +{
> +	struct drm_plane **planes;
> +	struct sun8i_mixer *mixer = crtc->engine;
> +	int i;
> +
> +	planes = devm_kcalloc(drm->dev, ARRAY_SIZE(sun8i_mixer_planes) + 1,
> +			      sizeof(*planes), GFP_KERNEL);
> +	if (!planes)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < ARRAY_SIZE(sun8i_mixer_planes); i++) {
> +		const struct sun8i_plane_desc *plane = &sun8i_mixer_planes[i];
> +		struct sun8i_layer *layer;
> +
> +		layer = sun8i_layer_init_one(drm, mixer, plane);
> +		if (IS_ERR(layer)) {
> +			dev_err(drm->dev, "Couldn't initialize %s plane\n",
> +				i ? "overlay" : "primary");
> +			return ERR_CAST(layer);
> +		};
> +
> +		layer->id = i;
> +		planes[i] = &layer->plane;
> +	};
> +
> +	return planes;
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun8i_layer.h b/drivers/gpu/drm/sun4i/sun8i_layer.h
> new file mode 100644
> index 000000000000..fe7e8a069d71
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun8i_layer.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (C) Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + *
> + * Based on sun4i_layer.h, which is:
> + *   Copyright (C) 2015 Free Electrons
> + *   Copyright (C) 2015 NextThing Co
> + *
> + *   Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef _SUN8I_LAYER_H_
> +#define _SUN8I_LAYER_H_
> +
> +struct sun4i_crtc;
> +
> +struct sun8i_layer {
> +	struct drm_plane	plane;
> +	struct sun4i_drv	*drv;
> +	struct sun8i_mixer	*mixer;
> +	int			id;
> +};
> +
> +static inline struct sun8i_layer *
> +plane_to_sun8i_layer(struct drm_plane *plane)
> +{
> +	return container_of(plane, struct sun8i_layer, plane);
> +}
> +
> +struct drm_plane **sun8i_layers_init(struct drm_device *drm,
> +				     struct sun4i_crtc *crtc);
> +#endif /* _SUN8I_LAYER_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> new file mode 100644
> index 000000000000..5cff3f3833a7
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -0,0 +1,381 @@
> +/*
> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + *
> + * Based on sun4i_backend.c, which is:
> + *   Copyright (C) 2015 Free Electrons
> + *   Copyright (C) 2015 NextThing Co
> + *
> + * 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 <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_plane_helper.h>
> +
> +#include <linux/component.h>
> +#include <linux/reset.h>
> +#include <linux/of_device.h>
> +
> +#include "sun4i_drv.h"
> +#include "sun8i_mixer.h"
> +#include "sun8i_layer.h"
> +#include "sunxi_engine.h"
> +
> +void sun8i_mixer_commit(void *mixer)
> +{
> +	struct sun8i_mixer *sun8i_mixer = mixer;
> +
> +	DRM_DEBUG_DRIVER("Committing changes\n");
> +
> +	regmap_write(sun8i_mixer->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> +		     SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> +}
> +
> +void sun8i_mixer_layer_enable(struct sun8i_mixer *mixer,
> +				int layer, bool enable)
> +{
> +	u32 val;
> +	/* Currently the first UI channel is used */
> +	int chan = mixer->cfg->vi_num;
> +
> +	DRM_DEBUG_DRIVER("Enabling layer %d in channel %d\n", layer, chan);
> +
> +	if (enable)
> +		val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
> +	else
> +		val = 0;
> +
> +	regmap_update_bits(mixer->regs,
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
> +
> +	/* Set the alpha configuration */
> +	regmap_update_bits(mixer->regs,
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK,
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_DEF);
> +	regmap_update_bits(mixer->regs,
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK,
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_DEF);
> +}
> +EXPORT_SYMBOL(sun8i_mixer_layer_enable);

Why do you need to export it?

> +
> +static int sun8i_mixer_drm_format_to_layer(struct drm_plane *plane,
> +					     u32 format, u32 *mode)
> +{
> +	switch (format) {
> +	case DRM_FORMAT_XRGB8888:
> +		*mode = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_XRGB8888;
> +		break;
> +
> +	case DRM_FORMAT_RGB888:
> +		*mode = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_RGB888;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int sun8i_mixer_update_layer_coord(struct sun8i_mixer *mixer,
> +				     int layer, struct drm_plane *plane)
> +{
> +	struct drm_plane_state *state = plane->state;
> +	struct drm_framebuffer *fb = state->fb;
> +	/* Currently the first UI channel is used */
> +	int chan = mixer->cfg->vi_num;
> +
> +	DRM_DEBUG_DRIVER("Updating layer %d\n", layer);
> +
> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> +		DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
> +				 state->crtc_w, state->crtc_h);
> +		regmap_write(mixer->regs, SUN8I_MIXER_GLOBAL_SIZE,
> +			     SUN8I_MIXER_SIZE(state->crtc_w,
> +					      state->crtc_h));
> +		DRM_DEBUG_DRIVER("Updating blender size\n");
> +		regmap_write(mixer->regs,
> +			     SUN8I_MIXER_BLEND_ATTR_INSIZE(0),
> +			     SUN8I_MIXER_SIZE(state->crtc_w,
> +					      state->crtc_h));
> +		regmap_write(mixer->regs, SUN8I_MIXER_BLEND_OUTSIZE,
> +			     SUN8I_MIXER_SIZE(state->crtc_w,
> +					      state->crtc_h));
> +		DRM_DEBUG_DRIVER("Updating channel size\n");
> +		regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_OVL_SIZE(chan),
> +			     SUN8I_MIXER_SIZE(state->crtc_w,
> +					      state->crtc_h));
> +	}
> +
> +	/* Set the line width */
> +	DRM_DEBUG_DRIVER("Layer line width: %d bytes\n", fb->pitches[0]);
> +	regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_LAYER_PITCH(chan, layer),
> +		     fb->pitches[0]);
> +
> +	/* Set height and width */
> +	DRM_DEBUG_DRIVER("Layer size W: %u H: %u\n",
> +			 state->crtc_w, state->crtc_h);
> +	regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_LAYER_SIZE(chan, layer),
> +		     SUN8I_MIXER_SIZE(state->crtc_w, state->crtc_h));
> +
> +	/* Set base coordinates */
> +	DRM_DEBUG_DRIVER("Layer coordinates X: %d Y: %d\n",
> +			 state->crtc_x, state->crtc_y);
> +	regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_LAYER_COORD(chan, layer),
> +		     SUN8I_MIXER_COORD(state->crtc_x, state->crtc_y));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(sun8i_mixer_update_layer_coord);
> +
> +int sun8i_mixer_update_layer_formats(struct sun8i_mixer *mixer,
> +				       int layer, struct drm_plane *plane)
> +{
> +	struct drm_plane_state *state = plane->state;
> +	struct drm_framebuffer *fb = state->fb;
> +	bool interlaced = false;
> +	u32 val;
> +	/* Currently the first UI channel is used */
> +	int chan = mixer->cfg->vi_num;
> +	int ret;
> +
> +	if (plane->state->crtc)
> +		interlaced = plane->state->crtc->state->adjusted_mode.flags
> +			& DRM_MODE_FLAG_INTERLACE;
> +
> +	regmap_update_bits(mixer->regs, SUN8I_MIXER_BLEND_OUTCTL,
> +			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
> +			   interlaced ?
> +			   SUN8I_MIXER_BLEND_OUTCTL_INTERLACED : 0);
> +
> +	DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
> +			 interlaced ? "on" : "off");

You're not using interlaced anywhere.

> +
> +	ret = sun8i_mixer_drm_format_to_layer(plane, fb->format->format,
> +						&val);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("Invalid format\n");
> +		return ret;
> +	}
> +
> +	regmap_update_bits(mixer->regs,
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
> +			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK, val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(sun8i_mixer_update_layer_formats);
> +
> +int sun8i_mixer_update_layer_buffer(struct sun8i_mixer *mixer,
> +				      int layer, struct drm_plane *plane)
> +{
> +	struct drm_plane_state *state = plane->state;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct drm_gem_cma_object *gem;
> +	dma_addr_t paddr;
> +	uint32_t paddr_u32;
> +	/* Currently the first UI channel is used */
> +	int chan = mixer->cfg->vi_num;
> +	int bpp;
> +
> +	/* Get the physical address of the buffer in memory */
> +	gem = drm_fb_cma_get_gem_obj(fb, 0);
> +
> +	DRM_DEBUG_DRIVER("Using GEM @ %pad\n", &gem->paddr);
> +
> +	/* Compute the start of the displayed memory */
> +	bpp = fb->format->cpp[0];
> +	paddr = gem->paddr + fb->offsets[0];
> +	paddr += (state->src_x >> 16) * bpp;
> +	paddr += (state->src_y >> 16) * fb->pitches[0];
> +
> +	DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
> +
> +	paddr_u32 = (uint32_t) paddr;

How does that work on 64-bits systems ?

> +
> +	regmap_write(mixer->regs,
> +		     SUN8I_MIXER_CHAN_UI_LAYER_TOP_LADDR(chan, layer),
> +		     paddr_u32);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(sun8i_mixer_update_layer_buffer);
> +
> +static const struct sunxi_engine_ops sun8i_engine_ops = {
> +	.commit = sun8i_mixer_commit,
> +	.layers_init = sun8i_layers_init,

Align with tabs.

> +};
> +
> +static struct regmap_config sun8i_mixer_regmap_config = {
> +	.reg_bits	= 32,
> +	.val_bits	= 32,
> +	.reg_stride	= 4,
> +	.max_register	= 0xbfffc, /* guessed */
> +};
> +
> +static int sun8i_mixer_bind(struct device *dev, struct device *master,
> +			      void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct sun4i_drv *drv = drm->dev_private;
> +	struct sun8i_mixer *mixer;
> +	struct resource *res;
> +	void __iomem *regs;
> +	int i, ret;
> +
> +	mixer = devm_kzalloc(dev, sizeof(*mixer), GFP_KERNEL);
> +	if (!mixer)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, mixer);
> +	drv->engine = mixer;
> +	drv->engine_ops = &sun8i_engine_ops;
> +
> +	mixer->cfg = of_device_get_match_data(dev);
> +	if (!mixer->cfg)
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	mixer->regs = devm_regmap_init_mmio(dev, regs,
> +					      &sun8i_mixer_regmap_config);
> +	if (IS_ERR(mixer->regs)) {
> +		dev_err(dev, "Couldn't create the mixer regmap\n");
> +		return PTR_ERR(mixer->regs);
> +	}
> +
> +	mixer->reset = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(mixer->reset)) {
> +		dev_err(dev, "Couldn't get our reset line\n");
> +		return PTR_ERR(mixer->reset);
> +	}
> +
> +	ret = reset_control_deassert(mixer->reset);
> +	if (ret) {
> +		dev_err(dev, "Couldn't deassert our reset line\n");
> +		return ret;
> +	}
> +
> +	mixer->bus_clk = devm_clk_get(dev, "bus");
> +	if (IS_ERR(mixer->bus_clk)) {
> +		dev_err(dev, "Couldn't get the mixer bus clock\n");
> +		ret = PTR_ERR(mixer->bus_clk);
> +		goto err_assert_reset;
> +	}
> +	clk_prepare_enable(mixer->bus_clk);
> +
> +	mixer->mod_clk = devm_clk_get(dev, "mod");
> +	if (IS_ERR(mixer->mod_clk)) {
> +		dev_err(dev, "Couldn't get the mixer module clock\n");
> +		ret = PTR_ERR(mixer->mod_clk);
> +		goto err_disable_bus_clk;
> +	}
> +	clk_prepare_enable(mixer->mod_clk);
> +
> +	/* Reset the registers */
> +	for (i = 0x0; i < 0x20000; i += 4)
> +		regmap_write(mixer->regs, i, 0);
> +
> +	/* Enable the mixer */
> +	regmap_write(mixer->regs, SUN8I_MIXER_GLOBAL_CTL,
> +		     SUN8I_MIXER_GLOBAL_CTL_RT_EN);
> +
> +	/* Initialize blender */
> +	regmap_write(mixer->regs, SUN8I_MIXER_BLEND_FCOLOR_CTL,
> +		     SUN8I_MIXER_BLEND_FCOLOR_CTL_DEF);
> +	regmap_write(mixer->regs, SUN8I_MIXER_BLEND_PREMULTIPLY,
> +		     SUN8I_MIXER_BLEND_PREMULTIPLY_DEF);
> +	regmap_write(mixer->regs, SUN8I_MIXER_BLEND_BKCOLOR,
> +		     SUN8I_MIXER_BLEND_BKCOLOR_DEF);
> +	regmap_write(mixer->regs, SUN8I_MIXER_BLEND_MODE(0),
> +		     SUN8I_MIXER_BLEND_MODE_DEF);
> +	regmap_write(mixer->regs, SUN8I_MIXER_BLEND_CK_CTL,
> +		     SUN8I_MIXER_BLEND_CK_CTL_DEF);
> +
> +	regmap_write(mixer->regs,
> +		     SUN8I_MIXER_BLEND_ATTR_FCOLOR(0),
> +		     SUN8I_MIXER_BLEND_ATTR_FCOLOR_DEF);
> +
> +	/* Select the first UI channel */
> +	DRM_DEBUG_DRIVER("Selecting channel %d (first UI channel)\n",
> +			 mixer->cfg->vi_num);
> +	regmap_write(mixer->regs, SUN8I_MIXER_BLEND_ROUTE,
> +		     mixer->cfg->vi_num);
> +
> +	return 0;
> +
> +	clk_disable_unprepare(mixer->mod_clk);
> +err_disable_bus_clk:
> +	clk_disable_unprepare(mixer->bus_clk);
> +err_assert_reset:
> +	reset_control_assert(mixer->reset);
> +	return ret;
> +}
> +
> +static void sun8i_mixer_unbind(struct device *dev, struct device *master,
> +				 void *data)
> +{
> +	struct sun8i_mixer *mixer = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(mixer->mod_clk);
> +	clk_disable_unprepare(mixer->bus_clk);
> +	reset_control_assert(mixer->reset);
> +}
> +
> +static const struct component_ops sun8i_mixer_ops = {
> +	.bind	= sun8i_mixer_bind,
> +	.unbind	= sun8i_mixer_unbind,
> +};
> +
> +static int sun8i_mixer_probe(struct platform_device *pdev)
> +{
> +	return component_add(&pdev->dev, &sun8i_mixer_ops);
> +}
> +
> +static int sun8i_mixer_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &sun8i_mixer_ops);
> +
> +	return 0;
> +}
> +
> +static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = {
> +	.vi_num = 2,
> +	.ui_num = 1,
> +};
> +
> +static const struct of_device_id sun8i_mixer_of_table[] = {
> +	{
> +		.compatible = "allwinner,sun8i-v3s-de2-mixer",
> +		.data = &sun8i_v3s_mixer_cfg,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_mixer_of_table);
> +
> +static struct platform_driver sun8i_mixer_platform_driver = {
> +	.probe		= sun8i_mixer_probe,
> +	.remove		= sun8i_mixer_remove,
> +	.driver		= {
> +		.name		= "sun8i-mixer",
> +		.of_match_table	= sun8i_mixer_of_table,
> +	},
> +};
> +module_platform_driver(sun8i_mixer_platform_driver);
> +
> +MODULE_AUTHOR("Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>");
> +MODULE_DESCRIPTION("Allwinner DE2 Mixer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> new file mode 100644
> index 000000000000..a4a365ae44c9
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef _SUN8I_MIXER_H_
> +#define _SUN8I_MIXER_H_
> +
> +#include <linux/clk.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#include "sun4i_layer.h"
> +
> +#define SUN8I_MIXER_MAX_CHAN_COUNT		4
> +
> +#define SUN8I_MIXER_SIZE(w, h)			(((h) - 1) << 16 | ((w) - 1))
> +#define SUN8I_MIXER_COORD(x, y)			((y) << 16 | (x))
> +
> +#define SUN8I_MIXER_GLOBAL_CTL			0x0
> +#define SUN8I_MIXER_GLOBAL_STATUS		0x4
> +#define SUN8I_MIXER_GLOBAL_DBUFF		0x8
> +#define SUN8I_MIXER_GLOBAL_SIZE			0xc
> +
> +#define SUN8I_MIXER_GLOBAL_CTL_RT_EN		0x1
> +
> +#define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE		0x1
> +
> +#define SUN8I_MIXER_BLEND_FCOLOR_CTL		0x1000
> +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(x)	(0x1004 + 0x10 * (x) + 0x0)
> +#define SUN8I_MIXER_BLEND_ATTR_INSIZE(x)	(0x1004 + 0x10 * (x) + 0x4)
> +#define SUN8I_MIXER_BLEND_ATTR_OFFSET(x)	(0x1004 + 0x10 * (x) + 0x8)
> +#define SUN8I_MIXER_BLEND_ROUTE			0x1080
> +#define SUN8I_MIXER_BLEND_PREMULTIPLY		0x1084
> +#define SUN8I_MIXER_BLEND_BKCOLOR		0x1088
> +#define SUN8I_MIXER_BLEND_OUTSIZE		0x108c
> +#define SUN8I_MIXER_BLEND_MODE(x)		(0x1090 + 0x04 * (x))
> +#define SUN8I_MIXER_BLEND_CK_CTL		0x10b0
> +#define SUN8I_MIXER_BLEND_CK_CFG		0x10b4
> +#define SUN8I_MIXER_BLEND_CK_MAX(x)		(0x10c0 + 0x04 * (x))
> +#define SUN8I_MIXER_BLEND_CK_MIN(x)		(0x10e0 + 0x04 * (x))
> +#define SUN8I_MIXER_BLEND_OUTCTL		0x10fc
> +
> +/* The following numbers are some still unknown magic numbers */
> +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR_DEF	0xff000000
> +#define SUN8I_MIXER_BLEND_FCOLOR_CTL_DEF	0x00000101
> +#define SUN8I_MIXER_BLEND_PREMULTIPLY_DEF	0x0
> +#define SUN8I_MIXER_BLEND_BKCOLOR_DEF		0xff000000
> +#define SUN8I_MIXER_BLEND_MODE_DEF		0x03010301
> +#define SUN8I_MIXER_BLEND_CK_CTL_DEF		0x0
> +
> +#define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
> +
> +/*
> + * VI channels are not used now, but the support of them may be introduced in
> + * the future.
> + */
> +
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x0)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_SIZE(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x4)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_COORD(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x8)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_PITCH(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0xc)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_TOP_LADDR(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x10)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_BOT_LADDR(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x14)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_FCOLOR(ch, layer) \
> +			(0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x18)
> +#define SUN8I_MIXER_CHAN_UI_TOP_HADDR(ch)	(0x2000 + 0x1000 * (ch) + 0x80)
> +#define SUN8I_MIXER_CHAN_UI_BOT_HADDR(ch)	(0x2000 + 0x1000 * (ch) + 0x84)
> +#define SUN8I_MIXER_CHAN_UI_OVL_SIZE(ch)	(0x2000 + 0x1000 * (ch) + 0x88)
> +
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN		BIT(0)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK	GENMASK(2, 1)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK	GENMASK(11, 8)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK	GENMASK(31, 24)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_DEF	(1 << 1)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_ARGB8888	(0 << 8)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_XRGB8888	(4 << 8)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_RGB888	(8 << 8)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_DEF	(0xff << 24)
> +
> +/*
> + * These sub-engines are still unknown now, the EN registers are here only to
> + * be used to disable these sub-engines.
> + */
> +#define SUN8I_MIXER_VSU_EN			0x20000
> +#define SUN8I_MIXER_GSU1_EN			0x30000
> +#define SUN8I_MIXER_GSU2_EN			0x40000
> +#define SUN8I_MIXER_GSU3_EN			0x50000
> +#define SUN8I_MIXER_FCE_EN			0xa0000
> +#define SUN8I_MIXER_BWS_EN			0xa2000
> +#define SUN8I_MIXER_LTI_EN			0xa4000
> +#define SUN8I_MIXER_PEAK_EN			0xa6000
> +#define SUN8I_MIXER_ASE_EN			0xa8000
> +#define SUN8I_MIXER_FCC_EN			0xaa000
> +#define SUN8I_MIXER_DCSC_EN			0xb0000
> +
> +struct sun8i_mixer_cfg {
> +	int		vi_num;
> +	int		ui_num;
> +};
> +
> +struct sun8i_mixer {
> +	struct regmap			*regs;
> +
> +	const struct sun8i_mixer_cfg	*cfg;
> +
> +	struct reset_control		*reset;
> +
> +	struct clk			*bus_clk;
> +	struct clk			*mod_clk;
> +};
> +
> +void sun8i_mixer_layer_enable(struct sun8i_mixer *mixer,
> +				int layer, bool enable);
> +int sun8i_mixer_update_layer_coord(struct sun8i_mixer *mixer,
> +				     int layer, struct drm_plane *plane);
> +int sun8i_mixer_update_layer_formats(struct sun8i_mixer *mixer,
> +				       int layer, struct drm_plane *plane);
> +int sun8i_mixer_update_layer_buffer(struct sun8i_mixer *mixer,
> +				      int layer, struct drm_plane *plane);
> +#endif /* _SUN8I_MIXER_H_ */

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ 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