* Re: [PATCH v3 3/5] ARM: dts: stm32: enable ltdc binding with ili9341 on stm32429-disco board
From: Alexandre Torgue @ 2020-05-14 12:52 UTC (permalink / raw)
To: Linus Walleij, dillon.minfei
Cc: Rob Herring, Maxime Coquelin, thierry.reding@gmail.com,
Sam Ravnborg, Dave Airlie, Daniel Vetter, Michael Turquette,
Stephen Boyd,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-stm32, Linux ARM, linux-kernel@vger.kernel.org,
open list:DRM PANEL DRIVERS, linux-clk
In-Reply-To: <CACRpkda5VjjBdbruXTi33QBNb=VU6vK2zDE8yyQXoWw7=NQFeg@mail.gmail.com>
On 5/14/20 10:24 AM, Linus Walleij wrote:
> On Tue, May 12, 2020 at 9:04 AM <dillon.minfei@gmail.com> wrote:
>
>> From: dillon min <dillon.minfei@gmail.com>
>>
>> Enable the ltdc & ili9341 on stm32429-disco board.
>>
>> Signed-off-by: dillon min <dillon.minfei@gmail.com>
>
> This mostly looks good but...
>
>> +&spi5 {
>> + status = "okay";
>> + pinctrl-0 = <&spi5_pins>;
>> + pinctrl-names = "default";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + cs-gpios = <&gpioc 2 GPIO_ACTIVE_LOW>;
>> + dmas = <&dma2 3 2 0x400 0x0>,
>> + <&dma2 4 2 0x400 0x0>;
>> + dma-names = "rx", "tx";
>
> These DMA assignments seem to be SoC things and should
> rather be in the DTS(I) file where &spi5 is defined, right?
> stm32f429.dtsi I suppose?
I agree with Linus, DMA have to be defined in SoC dtsi. And if a board
doesn't want to use it, we use the "delete-property".
>
> It is likely the same no matter which device is using spi5.
>
> Yours,
> Linus Walleij
>
^ permalink raw reply
* [PATCHv3 1/7] dt-bindings: crypto: Add TI SA2UL crypto accelerator documentation
From: Tero Kristo @ 2020-05-14 12:50 UTC (permalink / raw)
To: herbert, davem, linux-crypto; +Cc: Keerthy, Rob Herring, devicetree
In-Reply-To: <20200511215343.GA10123@bogus>
From: Keerthy <j-keerthy@ti.com>
The Security Accelerator Ultra Lite (SA2UL) subsystem provides hardware
cryptographic acceleration for the following use cases:
* Encryption and authentication for secure boot
* Encryption and authentication of content in applications
requiring DRM (digital rights management) and
content/asset protection
SA2UL provides support for number of different cryptographic algorithms
including SHA1, SHA256, SHA512, AES, 3DES, and various combinations of
the previous for AEAD use.
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Keerthy <j-keerthy@ti.com>
[t-kristo@ti.com: converted documentation to yaml]
Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
v3:
- fixed a typo in rng child node regex
.../devicetree/bindings/crypto/ti,sa2ul.yaml | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
create mode 100644 Documentation/devicetree/bindings/crypto/ti,sa2ul.yaml
diff --git a/Documentation/devicetree/bindings/crypto/ti,sa2ul.yaml b/Documentation/devicetree/bindings/crypto/ti,sa2ul.yaml
new file mode 100644
index 000000000000..27bb3a7e2b87
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/ti,sa2ul.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/crypto/ti,sa2ul.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: K3 SoC SA2UL crypto module
+
+maintainers:
+ - Tero Kristo <t-kristo@ti.com>
+
+properties:
+ compatible:
+ enum:
+ - ti,j721e-sa2ul
+ - ti,am654-sa2ul
+
+ reg:
+ maxItems: 1
+
+ power-domains:
+ maxItems: 1
+
+ dmas:
+ items:
+ - description: TX DMA Channel
+ - description: RX DMA Channel #1
+ - description: RX DMA Channel #2
+
+ dma-names:
+ items:
+ - const: tx
+ - const: rx1
+ - const: rx2
+
+ dma-coherent: true
+
+ "#address-cells":
+ const: 2
+
+ "#size-cells":
+ const: 2
+
+ ranges:
+ description:
+ Address translation for the possible RNG child node for SA2UL
+
+patternProperties:
+ "^rng@[a-f0-9]+$":
+ type: object
+ description:
+ Child RNG node for SA2UL
+
+required:
+ - compatible
+ - reg
+ - power-domains
+ - dmas
+ - dma-names
+ - dma-coherent
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+ main_crypto: crypto@4e00000 {
+ compatible = "ti,j721-sa2ul";
+ reg = <0x0 0x4e00000 0x0 0x1200>;
+ power-domains = <&k3_pds 264 TI_SCI_PD_EXCLUSIVE>;
+ dmas = <&main_udmap 0xc000>, <&main_udmap 0x4000>,
+ <&main_udmap 0x4001>;
+ dma-names = "tx", "rx1", "rx2";
+ dma-coherent;
+ };
--
2.17.1
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply related
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Manivannan Sadhasivam @ 2020-05-14 12:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Kieran Bingham, Linux-Renesas, Linux Media Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Mauro Carvalho Chehab, Kieran Bingham,
Laurent Pinchart, Jacopo Mondi, Niklas Söderlund,
Sakari Ailus, Hans Verkuil, Hyun Kwon, Rob Herring, Jacopo Mondi,
Laurent Pinchart, Niklas Söderlund
In-Reply-To: <CAMuHMdWSEY2q1f1iobrSXHYWzweV9yjk4i1ROj2Yde7DJMiabQ@mail.gmail.com>
On Thu, May 14, 2020 at 01:59:35PM +0200, Geert Uytterhoeven wrote:
> Hi Kieran,
>
> On Thu, May 14, 2020 at 1:47 PM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> > On 14/05/2020 11:13, Manivannan Sadhasivam wrote:
> > > On Thu, May 14, 2020 at 11:02:53AM +0100, Kieran Bingham wrote:
> > >> On 12/05/2020 19:17, Manivannan Sadhasivam wrote:
> > >>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
> > >>>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
> > >>>> CSI-2 output. The device supports multicamera streaming applications,
> > >>>> and features the ability to synchronise the attached cameras.
> > >>>>
> > >>>> CSI-2 output can be configured with 1 to 4 lanes, and a control channel
> > >>>> is supported over I2C, which implements an I2C mux to facilitate
> > >>>> communications with connected cameras across the reverse control
> > >>>> channel.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> > >>>> --- /dev/null
> > >>>> +++ b/drivers/media/i2c/max9286.c
>
> > >>>> +static int max9286_register_gpio(struct max9286_priv *priv)
> > >>>> +{
> > >>>> + struct device *dev = &priv->client->dev;
> > >>>> + struct gpio_chip *gpio = &priv->gpio;
> > >>>> + int ret;
> > >>>> +
> > >>>> + static const char * const names[] = {
> > >>>> + "GPIO0OUT",
> > >>>> + "GPIO1OUT",
> > >>>> + };
> > >>>> +
> > >>>> + /* Configure the GPIO */
> > >>>> + gpio->label = dev_name(dev);
> > >>>
> > >>> So if you have more than one MAX9286 in a system, all gpiochips will appear
> > >>> with the same name. I'd recommend to append the index to distinguish properly.
> > >>
> > >> Ah yes, that's a good point, and I think I've even seen that.
> > >>
> > >> I'll fix it now.
> >
> > Oh, in fact actually this doesn't.
> >
> > gpiodetect prints:
> >
> > gpiochip10 [4-004c] (2 lines)
> > gpiochip11 [4-006c] (2 lines)
> >
> > and mostly references them as gpiochip10 and gpiochip11.
>
> Indeed, dev_name() should be different for each instance.
>
Ah, my bad! Somehow I got confused that this delivers static name... Sorry for
the noise, Kieran.
> > However,
> >
> > > [ 2.318533] gpio gpiochip11: Detected name collision for GPIO name 'GPIO0OUT'
> > > [ 2.325739] gpio gpiochip11: Detected name collision for GPIO name 'GPIO1OUT'
> >
> > That seems to be more of a problem for the gpio library, so I think I'll
> > just drop the const names. I don't think they add much value.
>
Well, I should've pointed this instead of above...
(lack of coffee)
> These are the line names. If they're not unique, a warning is printed,
> but they are still registered.
> So probably you want to use kasprintf("%s.%s", dev_name(dev), names[i]) to
> generate names.
>
Ack.
I think you should CC Linus W for next iteration to get review for gpiolib
implementation.
Thanks,
Mani
> See "[PATCH] gpiolib: Document that GPIO line names are not globally unique"
> (https://lore.kernel.org/linux-gpio/20200511101828.30046-1-geert+renesas@glider.be/)
> to clear up the details.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* [PATCH v3 3/3] ASoC: tlv320adcx140: Configure PDM sampling edge
From: Dan Murphy @ 2020-05-14 12:33 UTC (permalink / raw)
To: lgirdwood, broonie, perex, tiwai
Cc: alsa-devel, linux-kernel, devicetree, Dan Murphy
In-Reply-To: <20200514123338.20392-1-dmurphy@ti.com>
Configure the PDM sampling edges based on the values from the firmware.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
sound/soc/codecs/tlv320adcx140.c | 22 ++++++++++++++++++++++
sound/soc/codecs/tlv320adcx140.h | 3 +++
2 files changed, 25 insertions(+)
diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
index 97634e0ca0ba..140a5802a9a9 100644
--- a/sound/soc/codecs/tlv320adcx140.c
+++ b/sound/soc/codecs/tlv320adcx140.c
@@ -760,6 +760,10 @@ static int adcx140_codec_probe(struct snd_soc_component *component)
int sleep_cfg_val = ADCX140_WAKE_DEV;
u8 bias_source;
u8 vref_source;
+ int pdm_count;
+ u32 pdm_edges[ADCX140_NUM_PDM_EDGES];
+ u32 pdm_edge_val = 0;
+ int i;
int ret;
ret = device_property_read_u8(adcx140->dev, "ti,mic-bias-source",
@@ -786,6 +790,24 @@ static int adcx140_codec_probe(struct snd_soc_component *component)
bias_source |= vref_source;
+ pdm_count = device_property_count_u32(adcx140->dev,
+ "ti,pdm-edge-select");
+ if (pdm_count <= ADCX140_NUM_PDM_EDGES && pdm_count > 0) {
+ ret = device_property_read_u32_array(adcx140->dev,
+ "ti,pdm-edge-select",
+ pdm_edges, pdm_count);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < pdm_count; i++)
+ pdm_edge_val |= pdm_edges[i] << (ADCX140_PDM_EDGE_SHIFT - i);
+
+ ret = regmap_write(adcx140->regmap, ADCX140_PDM_CFG,
+ pdm_edge_val);
+ if (ret)
+ return ret;
+ }
+
ret = adcx140_reset(adcx140);
if (ret)
goto out;
diff --git a/sound/soc/codecs/tlv320adcx140.h b/sound/soc/codecs/tlv320adcx140.h
index 6d055e55909e..432eaf25d1a7 100644
--- a/sound/soc/codecs/tlv320adcx140.h
+++ b/sound/soc/codecs/tlv320adcx140.h
@@ -128,4 +128,7 @@
#define ADCX140_TX_OFFSET_MASK GENMASK(4, 0)
+#define ADCX140_NUM_PDM_EDGES 4
+#define ADCX140_PDM_EDGE_SHIFT 7
+
#endif /* _TLV320ADCX140_ */
--
2.26.2
^ permalink raw reply related
* [PATCH v3 2/3] ASoC: tlv320adcx140: Add device tree property for PDM edges
From: Dan Murphy @ 2020-05-14 12:33 UTC (permalink / raw)
To: lgirdwood, broonie, perex, tiwai
Cc: alsa-devel, linux-kernel, devicetree, Dan Murphy, Rob Herring
In-Reply-To: <20200514123338.20392-1-dmurphy@ti.com>
Add a device tree property to configure the PDM sampling edge for each
digital microphone.
CC: Rob Herring <robh@kernel.org>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
v3 - Updated description to be more descriptive and fixed example.
.../bindings/sound/tlv320adcx140.yaml | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml b/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
index ab2268c0ee67..daa6cc0e031b 100644
--- a/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
+++ b/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
@@ -63,6 +63,29 @@ properties:
- $ref: /schemas/types.yaml#/definitions/uint32
- enum: [0, 1, 2]
+ ti,pdm-edge-select:
+ description: |
+ Defines the PDMCLK sampling edge configuration for the PDM inputs. This
+ array is defined as <PDMIN1 PDMIN2 PDMIN3 PDMIN4>.
+
+ 0 - (default) Odd channel is latched on the negative edge and even
+ channel is latched on the the positive edge.
+ 1 - Odd channel is latched on the positive edge and even channel is
+ latched on the the negative edge.
+
+ PDMIN1 - PDMCLK latching edge used for channel 1 and 2 data
+ PDMIN2 - PDMCLK latching edge used for channel 3 and 4 data
+ PDMIN3 - PDMCLK latching edge used for channel 5 and 6 data
+ PDMIN4 - PDMCLK latching edge used for channel 7 and 8 data
+
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint32-array
+ - minItems: 1
+ maxItems: 4
+ items:
+ maximum: 1
+ default: [0, 0, 0, 0]
+
required:
- compatible
- reg
@@ -77,6 +100,7 @@ examples:
compatible = "ti,tlv320adc5140";
reg = <0x4c>;
ti,mic-bias-source = <6>;
+ ti,pdm-edge-select = <0 1 0 1>;
reset-gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>;
};
};
--
2.26.2
^ permalink raw reply related
* [PATCH v3 1/3] ASoC: tlv320adcx140: Add controls for PDM clk
From: Dan Murphy @ 2020-05-14 12:33 UTC (permalink / raw)
To: lgirdwood, broonie, perex, tiwai
Cc: alsa-devel, linux-kernel, devicetree, Dan Murphy
Add ALSA controls to configure the PDM clocks.
The clocks need to be configurable to accommodate various microphones
that use clocks for low power/low resolution modes to high power/high
resolution modes.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
sound/soc/codecs/tlv320adcx140.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
index 0f713efde046..97634e0ca0ba 100644
--- a/sound/soc/codecs/tlv320adcx140.c
+++ b/sound/soc/codecs/tlv320adcx140.c
@@ -180,6 +180,17 @@ static const struct snd_kcontrol_new decimation_filter_controls[] = {
SOC_DAPM_ENUM("Decimation Filter", decimation_filter_enum),
};
+static const char * const pdmclk_text[] = {
+ "2.8224 MHz", "1.4112 MHz", "705.6 kHz", "5.6448 MHz"
+};
+
+static SOC_ENUM_SINGLE_DECL(pdmclk_select_enum, ADCX140_PDMCLK_CFG, 0,
+ pdmclk_text);
+
+static const struct snd_kcontrol_new pdmclk_div_controls[] = {
+ SOC_DAPM_ENUM("PDM Clk Divider Select", pdmclk_select_enum),
+};
+
static const char * const resistor_text[] = {
"2.5 kOhm", "10 kOhm", "20 kOhm"
};
@@ -416,6 +427,9 @@ static const struct snd_soc_dapm_widget adcx140_dapm_widgets[] = {
SND_SOC_DAPM_MUX("IN4 Analog Mic Resistor", SND_SOC_NOPM, 0, 0,
in4_resistor_controls),
+ SND_SOC_DAPM_MUX("PDM Clk Div Select", SND_SOC_NOPM, 0, 0,
+ pdmclk_div_controls),
+
SND_SOC_DAPM_MUX("Decimation Filter", SND_SOC_NOPM, 0, 0,
decimation_filter_controls),
};
@@ -493,6 +507,11 @@ static const struct snd_soc_dapm_route adcx140_audio_map[] = {
{"IN4 Analog Mic Resistor", "10 kOhm", "MIC4M Input Mux"},
{"IN4 Analog Mic Resistor", "20 kOhm", "MIC4M Input Mux"},
+ {"PDM Clk Div Select", "2.8224 MHz", "MIC1P Input Mux"},
+ {"PDM Clk Div Select", "1.4112 MHz", "MIC1P Input Mux"},
+ {"PDM Clk Div Select", "705.6 kHz", "MIC1P Input Mux"},
+ {"PDM Clk Div Select", "5.6448 MHz", "MIC1P Input Mux"},
+
{"MIC1 Analog Mux", "Line In", "MIC1P"},
{"MIC2 Analog Mux", "Line In", "MIC2P"},
{"MIC3 Analog Mux", "Line In", "MIC3P"},
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v2 0/6] Add Stinger96 and IoT Box board support
From: Alexandre Torgue @ 2020-05-14 12:38 UTC (permalink / raw)
To: mani, robh+dt, mcoquelin.stm32
Cc: devicetree, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <20200503154215.23654-1-mani@kernel.org>
Hi Mani
On 5/3/20 5:42 PM, mani@kernel.org wrote:
> From: Manivannan Sadhasivam <mani@kernel.org>
>
> Hello,
>
> This series adds Stinger96 and IoT Box board support. These boards are
> based on STM32MP157A SoC, designed and manufactured by Shiratech solutions.
>
> The Stinger96 is a base board (96Boards IoT Extended edition) while IoT Box
> adds one mezzanine on top of it and sold as a gateway device.
>
> This series depends on below patchsets already submitted and gone through
> reviews:
>
> [PATCH v3 0/3] Add Reset and Wakeup support for CCS811
> [PATCH v3 0/2] Add CTS/RTS gpio support to STM32 UART
>
> More information about these boards can be found in below links:
>
> https://www.shiratech-solutions.com/products/stinger96/
> https://www.shiratech-solutions.com/products/iot-box/
>
> Thanks,
> Mani
>
Series applied on stm32-next.
Regards
Alex
> Changes in v2:
>
> * Used "stm32" prefix for all DT commits
> * Dropped custom sdmmc2 pinctrl node since existing node itself has pullup
> enabled and works fine.
>
> Manivannan Sadhasivam (6):
> dt-bindings: Add vendor prefix for Shiratech Solutions
> ARM: dts: stm32: Add missing pinctrl entries for STM32MP15
> dt-bindings: arm: stm32: Document Stinger96 compatible
> ARM: dts: stm32: Add Stinger96 board support
> dt-bindings: arm: stm32: Document IoT Box compatible
> ARM: dts: stm32: Add IoT Box board support
>
> .../devicetree/bindings/arm/stm32/stm32.yaml | 2 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> arch/arm/boot/dts/Makefile | 2 +
> arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 64 ++++
> arch/arm/boot/dts/stm32mp157a-iot-box.dts | 68 ++++
> arch/arm/boot/dts/stm32mp157a-stinger96.dts | 12 +
> arch/arm/boot/dts/stm32mp157a-stinger96.dtsi | 342 ++++++++++++++++++
> 7 files changed, 492 insertions(+)
> create mode 100644 arch/arm/boot/dts/stm32mp157a-iot-box.dts
> create mode 100644 arch/arm/boot/dts/stm32mp157a-stinger96.dts
> create mode 100644 arch/arm/boot/dts/stm32mp157a-stinger96.dtsi
>
^ permalink raw reply
* Re: [PATCH v4 0/5] iommu: Add Allwinner H6 IOMMU driver
From: Joerg Roedel @ 2020-05-14 12:38 UTC (permalink / raw)
To: Maxime Ripard
Cc: Chen-Yu Tsai, Maxime Ripard, Mark Rutland, Rob Herring,
Frank Rowand, devicetree, linux-arm-kernel, iommu
In-Reply-To: <cover.b27dedd61e008ffcf55a028ccddda3bb4d21dfc8.1589378833.git-series.maxime@cerno.tech>
On Wed, May 13, 2020 at 04:07:19PM +0200, Maxime Ripard wrote:
> Maxime Ripard (5):
> dt-bindings: iommu: Add Allwinner H6 IOMMU bindings
> dt-bindings: display: sun8i-mixer: Allow for an iommu property
> iommu: Add Allwinner H6 IOMMU driver
> arm64: dts: allwinner: h6: Add IOMMU
> drm/sun4i: mixer: Call of_dma_configure if there's an IOMMU
Applied all to the IOMMU tree, thanks. The code lives in the
arm/allwinner branch. Please use 'iommu/sun50i: <Capital Letter>...' as
the pattern for your future subject lines when changing the driver.
I also have two fixes on-top which I will send out shortly and add to
that branch.
Regards,
Joerg
^ permalink raw reply
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Kieran Bingham @ 2020-05-14 12:27 UTC (permalink / raw)
To: kieran.bingham+renesas, Manivannan Sadhasivam
Cc: linux-renesas-soc, linux-media, devicetree, linux-kernel,
Mauro Carvalho Chehab, Laurent Pinchart, Jacopo Mondi,
Niklas Söderlund, sakari.ailus, Hans Verkuil, Hyun Kwon,
Rob Herring, Jacopo Mondi, Laurent Pinchart,
Niklas Söderlund
In-Reply-To: <f46ed5fc-4eb0-8841-25b8-ef6c45e7ac87@ideasonboard.com>
Hi Mani,
On 14/05/2020 12:47, Kieran Bingham wrote:
> On 14/05/2020 11:13, Manivannan Sadhasivam wrote:
>> Hi Kieran,
<snip>
>>>>> +static int max9286_parse_dt(struct max9286_priv *priv)
>>>>> +{
>>>>> + struct device *dev = &priv->client->dev;
>>>>> + struct device_node *i2c_mux;
>>>>> + struct device_node *node = NULL;
>>>>> + unsigned int i2c_mux_mask = 0;
>>>>> +
>>>>> + of_node_get(dev->of_node);
>>>>
>>>> Why this is needed?
>>>
>>> Hrm .. I recall adding it to solve dt reference balancing.
>>>
>>> I wish I'd added a comment at the time ... as I can't recall the details
>>> now.
>>>
>>
>> I understand that it is for the refcount balancing but I certainly don't see
>> a need for it.
>
> I'll go through and try to validate this again now.
Aha, that's why:
* of_find_node_by_name - Find a node by its "name" property
* @from: The node to start searching from or NULL; the node
* you pass will not be searched, only the next one
* will. Typically, you pass what the previous call
* returned. of_node_put() will be called on @from.
* @name: The name string to match against
I'll add a comment to state that it is to balance the of_node_put during
of_find_node_by_name().
--
Kieran
>>>>> + i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>>>> + if (!i2c_mux) {
>>>>> + dev_err(dev, "Failed to find i2c-mux node\n");
>>>>> + of_node_put(dev->of_node);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>> [...]
>>>>
--
Regards
--
Kieran
^ permalink raw reply
* Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support
From: Ravi Kumar Bokka (Temp) @ 2020-05-14 12:26 UTC (permalink / raw)
To: Srinivas Kandagatla, Rob Herring
Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp,
mturney, sparate, c_rbokka, mkurumel, dianders
In-Reply-To: <ffaccce7-95c0-2f95-ad3b-55f1da42eaee@linaro.org>
Hi Srinivas,
Thanks for your feedback by giving review comments. Please find my
inline comments.
Regards,
Ravi Kumar.B
On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>
>
> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>> This patch adds new driver for QTI qfprom-efuse controller. This
>> driver can
>> access the raw qfprom regions for fuse blowing.
>
> QTI?
guidance I have received from internal Legal/LOST team is that the QCOM
prefix needs to be changed to QTI everywhere it is used
>
>>
>> The current existed qfprom driver is only supports for cpufreq,
>> thermal sensors
>> drivers by read out calibration data, speed bins..etc which is stored
>> by qfprom efuses.
>
> Can you explain bit more about this QFPROM instance, Is this QFPROM part
> of secure controller address space?
> Is this closely tied to SoC or Secure controller version?
>
> Any reason why this can not be integrated into qfprom driver with
> specific compatible.
>
QFPROM driver communicates with sec_controller address space however
scope and functionalities of this driver is different and not limited as
existing qfprom fuse Read-Only driver for specific “fuse buckets’ like
cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch
requires specific sequence to write/blow fuses unlike other driver.
Scope/functionalities are different and this is separate driver.
>>
>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>> ---
>> drivers/nvmem/Kconfig | 10 +
>> drivers/nvmem/Makefile | 2 +
>> drivers/nvmem/qfprom-efuse.c | 476
>> +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 488 insertions(+)
>> create mode 100644 drivers/nvmem/qfprom-efuse.c
>>
> ...
>
>> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
>> new file mode 100644
>> index 0000000..2e3c275
>> --- /dev/null
>> +++ b/drivers/nvmem/qfprom-efuse.c
>> @@ -0,0 +1,476 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>> +#define QFPROM_BLOW_STATUS_READY 0x0
>> +
>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>> +
>> +/* Amount of time required to hold charge to blow fuse in
>> micro-seconds */
>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>> +
>> +#define QFPROM_ACCEL_OFFSET 0x044
>> +
>> +/**
>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>> + * platform data
>> + *
>> + * @name: qfprom-efuse compatible name
>
> ??
Thanks for your feedback. I will address this change
>> + * @fuse_blow_time_in_us: Should contain the wait time when doing the
>> fuse blow
>> + * @accel_value: Should contain qfprom accel value
>> + * @accel_reset_value: The reset value of qfprom accel value
>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
>> efuse blow
>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
>> blowing
>> + * is done
>> + * @qfprom_blow_set_freq: The frequency required to set when we start
>> the
>> + * fuse blowing
>> + * @qfprom_max_vol: max voltage required to set fuse blow
>> + * @qfprom_min_vol: min voltage required to set fuse blow
>
> How specific are these values per SoC?
>
This voltage level may change based on SoC and/or fuse-hardware
technology, it would change for SoC with different technology, hence we
have kept it in SOC specific settings.
>
>> + */
>> +struct qfprom_efuse_platform_data {
>> + const char *name;
>> + u8 fuse_blow_time_in_us;
>> + u32 accel_value;
>> + u32 accel_reset_value;
>> + u32 qfprom_blow_timer_value;
>> + u32 qfprom_blow_reset_freq;
>> + u32 qfprom_blow_set_freq;
>> + u32 qfprom_max_vol;
>> + u32 qfprom_min_vol;
>> +};
>> +
>> +/**
>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
>> + *
>> + * @qfpbase: iomapped memory space for qfprom base
>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>> +
>> + * @dev: qfprom device structure
>> + * @secclk: clock supply
>> + * @vcc: regulator supply
>> +
>> + * @qfpraw_start: qfprom raw fuse start region
>> + * @qfpraw_end: qfprom raw fuse end region
>> + * @qfprom_efuse_platform_data: qfprom platform data
>> + */
>> +struct qfprom_efuse_priv {
>> + void __iomem *qfpbase;
>> + void __iomem *qfpraw;
>> + void __iomem *qfpmap;
>
> Why are these memory regions split? Can't you just have complete qfprom
> area and add fixed offset for qfpraw within the driver?
>
Thanks for your feedback. I will address this change.
I have separated this memory regions because to identify raw fuse
regions separately and compare these raw fuse regions from the user
given input.
>> + struct device *dev;
>> + struct clk *secclk;
>> + struct regulator *vcc;
>> + resource_size_t qfpraw_start;
>> + resource_size_t qfpraw_end;
> Why do we need to check this range? as long as we set the nvmem_config
> with correct range then you should not need this check.
>
There is no harm in this explicit check in QFPROM-fuse driver and based
on internal review with our security team, this check is important to
avoid dependency on other upper layer.
>
>> + struct qfprom_efuse_platform_data efuse;
> A pointer here should be good enough?
>> +};
>> +
>
Thanks for your feedback. I will address this change
> ...
>
>> +/*
>> + * sets the value of the blow timer, accel register and the clock
>> + * and voltage settings
>> + */
>> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv
>> *priv)
>> +{
>> + int ret;
>> +
>> + ret = qfprom_disable_fuse_blowing(priv);
>> + if (ret) {
>> + dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>> + return ret;
>> + }
>
> Why do we need to qfprom_disable_fuse_blowing() for every call to enable
> it?
>
> Or are we missing some error handling in the caller?
>
We must disable/vote-off this QFPROM fuse power rail after blowing fuse,
it is the safe and right approach as per hardware programming guide for
fuse blowing process. Caller here is user space, can’t control
fuse-power-rail or can’t be relied to follow the required process. There
could also be unnecessary risk of leaving the vote/power-rail configured
at specific level after blowing the fuse. As per hardware requirement,
right after fuse blowing, we need to disable power rail.
>> +
>> + writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>> + QFPROM_BLOW_TIMER_OFFSET);
>> + writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
>> +
>> + ret = qfprom_set_clock_settings(priv);
>> + if (ret) {
>> + dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>> + return ret;
>> + }
>> +
>> + ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
>> + priv->efuse.qfprom_max_vol);
>> + if (ret) {
>> + dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> <<
>> +/*
>> + * verifying to make sure address being written or read is from qfprom
>> + * raw address range
>> + */
>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
>> + size_t bytes)
>> +{
>> + if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>> + ((reg + bytes) <= priv->qfpraw_end)) {
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
> >>
> Above function is totally redundant, nvmem core already has checks for
> this.
>
There is no harm in this explicit check in QFPROM-fuse driver and based
on internal review with our security team, this check is important to
avoid dependency on other upper layer.
>
>
>> +
>> +/*
>> + * API for reading from raw qfprom region
>> + */
>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
>> void *_val,
>> + size_t bytes)
>> +{
>> + struct qfprom_efuse_priv *priv = context;
>> + u32 *value = _val;
>> + u32 align_check;
>> + int i = 0, words = bytes / 4;
>> +
>> + dev_info(priv->dev,
>> + "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>> + reg, bytes);
>
> In general there is lot of debug info across the code, do you really
> need all this? Consider removing these!
>
Thanks for your feedback. I will address this change.
>> +
>> + if (bytes % 4 != 0x00) {
>> + dev_err(priv->dev,
>> + "Bytes: %zd to read should be word align\n",
>> + bytes);
>> + return -EINVAL;
>> + }
>
> This word align check is also redundant once you set nvmem_config with
> correct word_size.
>
I understand that there may be different approach to handle this. We
have used this approach and tested this driver thoroughly. Unless there
is technical limitation, changing this word_size would end up requiring
re-writing write/read APIs and going through testing again, there is not
much difference in either approach, we would like to keep this approach
unless there is technical concern.
>
>> +
>> + if (!addr_in_qfprom_range(priv, reg, bytes)) {
>> + dev_err(priv->dev,
>> + "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>> + reg, bytes);
>> + return -EINVAL;
>> + }
>> +
>> + align_check = (reg & 0xF);
>> +
>> + if (((align_check & ~3) == align_check) && value != NULL)
>> + while (words--)
>> + *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>> +
>> + else
>> + dev_err(priv->dev,
>> + "Invalid input parameter 0x%08x fuse blow address\n",
>> + reg);
>> +
>> + return 0;
>> +}
> ...
>
>> +
>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *qfpbase, *qfpraw, *qfpmap;
>> + struct nvmem_device *nvmem;
>> + struct nvmem_config *econfig;
>> + struct qfprom_efuse_priv *priv;
>> + const struct qfprom_efuse_platform_data *drvdata;
>> + int ret;
>> +
>> + dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>> +
>
> too much debug!
>
Thanks for your feedback. I will address this change.
>> + drvdata = of_device_get_match_data(&pdev->dev);
>> + if (!drvdata)
>> + return -EINVAL;
> Unnecessary check as this driver will not be probed unless there is a
> compatible match.
>
Thanks for your feedback. I will address this change.
>
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>> + priv->efuse.accel_value = drvdata->accel_value;
>> + priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>> + priv->efuse.qfprom_blow_timer_value =
>> drvdata->qfprom_blow_timer_value;
>> + priv->efuse.qfprom_blow_reset_freq =
>> drvdata->qfprom_blow_reset_freq;
>> + priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>> + priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>> + priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>> + priv->dev = dev;
>> +
>> + qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>> + if (IS_ERR(priv->qfpbase)) {
>> + ret = PTR_ERR(priv->qfpbase);
>> + goto err;
>> + }
>> +
>> + qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +
>> + priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>> + if (IS_ERR(priv->qfpraw)) {
>> + ret = PTR_ERR(priv->qfpraw);
>> + goto err;
>> + }
>> +
>> + priv->qfpraw_start = qfpraw->start - qfpbase->start;
>> + priv->qfpraw_end = qfpraw->end - qfpbase->start;
>> +
>> + qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> +
>> + priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>> + if (IS_ERR(priv->qfpmap)) {
>> + ret = PTR_ERR(priv->qfpmap);
>> + goto err;
>> + }
>> +
>> + priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>
> I see no reference to this regulator in dt bindings.
This perameter kept in board specific file i.e., sc7180-idp.dts file
>> + if (IS_ERR(priv->vcc)) {
>> + ret = PTR_ERR(priv->vcc);
>> + if (ret == -ENODEV)
>> + ret = -EPROBE_DEFER;
> Can you explain what is going on here?
>
As i took other drivers reference, i have kept this check.
>> +
>> + goto err;
>> + }
>> +
>> + priv->secclk = devm_clk_get(dev, "secclk");
>> + if (IS_ERR(priv->secclk)) {
>> + ret = PTR_ERR(priv->secclk);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "secclk error getting : %d\n", ret);
>> + goto err;
>> + }
>> +
>> + ret = clk_prepare_enable(priv->secclk);
>> + if (ret) {
>> + dev_err(dev, "clk_prepare_enable() failed\n");
>> + goto err;
>> + }
>> +
>> + econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>> + if (!econfig)
> Why not disabling the clk here?
>> + return -ENOMEM;
>
Thanks for your feedback. I will address this change.
>> +
>> + econfig->dev = dev;
>> + econfig->name = "qfprom-efuse";
>> + econfig->stride = 1;
>> + econfig->word_size = 1;
>> + econfig->reg_read = qfprom_efuse_reg_read;
>> + econfig->reg_write = qfprom_efuse_reg_write;
>> + econfig->size = resource_size(qfpraw);
>> + econfig->priv = priv;
>> +
>> + nvmem = devm_nvmem_register(dev, econfig);
>> +
>> + return PTR_ERR_OR_ZERO(nvmem);
> probably you should check the nvmem here before returning to disable the
> clk properly.
>
Thanks for your feedback. I will address this change.
>> +
>> +err:
>> + clk_disable_unprepare(priv->secclk);
>> + return ret;
>> +}
>> +
>> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
>> + .name = "sc7180-qfprom-efuse",
> Redundant.
>
Thanks for your feedback. I will address this change.
>> + .fuse_blow_time_in_us = 10,
>> + .accel_value = 0xD10,
>> + .accel_reset_value = 0x800,
>> + .qfprom_blow_timer_value = 25,
>> + .qfprom_blow_reset_freq = 19200000,
>> + .qfprom_blow_set_freq = 4800000,
>> + .qfprom_max_vol = 1904000,
>> + .qfprom_min_vol = 1800000,
>> +};
>> +
>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>> + {
>> + .compatible = "qcom,sc7180-qfprom-efuse",
>> + .data = &sc7180_qfp_efuse_data
>> + },
>> + {/* sentinel */},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>> +
>> +static struct platform_driver qfprom_efuse_driver = {
>> + .probe = qfprom_efuse_probe,
>> + .driver = {
>> + .name = "sc7180-qfprom-efuse",
>> + .of_match_table = qfprom_efuse_of_match,
>> + },
>> +};
>> +
>> +module_platform_driver(qfprom_efuse_driver);
>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>> +MODULE_LICENSE("GPL v2");
>>
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by the Linux Foundation.
^ permalink raw reply
* Re: [PATCH 11/17] spi: dw: Fix native CS being unset
From: Andy Shevchenko @ 2020-05-14 12:22 UTC (permalink / raw)
To: Serge Semin
Cc: Linus Walleij, Serge Semin, Gregory Clement, Mark Brown,
Charles Keepax, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
Allison Randal, Gareth Williams, Rob Herring, linux-mips,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Phil Edworthy, Thomas Gleixner, Alexios Zavras, Thor Thayer,
wuxu.wu, Xinwei Kong, Jarkko Nikula, linux-spi,
linux-kernel@vger.kernel.org
In-Reply-To: <20200514115558.e6cqnuxqyqkysfn7@mobilestation>
On Thu, May 14, 2020 at 02:55:58PM +0300, Serge Semin wrote:
> On Thu, May 14, 2020 at 10:31:13AM +0200, Linus Walleij wrote:
> > On Wed, May 13, 2020 at 2:13 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> BTW I experience a problem with vger.kernel.org. For some reason a few days ago
> it started bouncing my emails back sent to the GPIO/MIPS/SPI/kernel mailing lists.
> I've sent multiple backward messages to the postmaster (postmaster (dog) vger.kernel.org)
> with the bounce text, but still with no response. Could you tell me who should I
> bother with this problem to get a help with its solution?
Perhaps, helpdesk@kernel.org ?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [V4 PATCH 1/2] dt-bindings: Added device tree binding for max98390
From: Steve Lee @ 2020-05-14 12:16 UTC (permalink / raw)
To: lgirdwood, broonie, robh+dt, alsa-devel, devicetree, linux-kernel
Cc: ryan.lee.maxim, ryans.lee, steves.lee, steves.lee.maxim
Add documentation for DT binding of max98390 amplifier driver.
Signed-off-by: Steve Lee <steves.lee@maximintegrated.com>
---
Changed since V3:
* No changes.
Changed since V2:
* No changes.
Changed since V1:
* Modified sample text in example
.../devicetree/bindings/sound/max98390.txt | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/max98390.txt
diff --git a/Documentation/devicetree/bindings/sound/max98390.txt b/Documentation/devicetree/bindings/sound/max98390.txt
new file mode 100644
index 000000000000..0ddd4c6ae55e
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/max98390.txt
@@ -0,0 +1,26 @@
+Maxim Integrated MAX98390 Speaker Amplifier
+
+This device supports I2C.
+
+Required properties:
+
+ - compatible : "maxim,max98390"
+
+ - reg : the I2C address of the device.
+
+Optional properties:
+
+- maxim,temperature_calib
+ u32. The calculated temperature data was measured while doing the calibration. Data : Temp / 100 * 2^12
+
+- maxim,r0_calib
+ u32. This is r0 calibration data which was measured in factory mode.
+
+Example:
+
+codec: max98390@38 {
+ compatible = "maxim,max98390";
+ reg = <0x38>;
+ maxim,temperature_calib = <1024>;
+ maxim,r0_calib = <100232>;
+};
--
2.17.1
^ permalink raw reply related
* [PATCH v4 2/2] PCI: uniphier: Add Socionext UniPhier Pro5 PCIe endpoint controller driver
From: Kunihiko Hayashi @ 2020-05-14 12:03 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Masahiro Yamada
Cc: linux-pci, devicetree, linux-arm-kernel, linux-kernel,
Masami Hiramatsu, Jassi Brar, Kunihiko Hayashi
In-Reply-To: <1589457801-12796-1-git-send-email-hayashi.kunihiko@socionext.com>
Add driver for the Socionext UniPhier Pro5 SoC endpoint controller.
This controller is based on the DesignWare PCIe core.
And add "host" to existing controller descriontions for the host controller
in Kconfig.
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
MAINTAINERS | 2 +-
drivers/pci/controller/dwc/Kconfig | 13 +-
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-uniphier-ep.c | 383 ++++++++++++++++++++++++++
4 files changed, 396 insertions(+), 3 deletions(-)
create mode 100644 drivers/pci/controller/dwc/pcie-uniphier-ep.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f26748..b5ef0e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13212,7 +13212,7 @@ M: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
L: linux-pci@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/pci/uniphier-pcie*
-F: drivers/pci/controller/dwc/pcie-uniphier.c
+F: drivers/pci/controller/dwc/pcie-uniphier*
PCIE DRIVER FOR ST SPEAR13XX
M: Pratyush Anand <pratyush.anand@gmail.com>
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index ce13fd6..43a29f7 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -280,15 +280,24 @@ config PCIE_TEGRA194_EP
selected. This uses the DesignWare core.
config PCIE_UNIPHIER
- bool "Socionext UniPhier PCIe controllers"
+ bool "Socionext UniPhier PCIe host controllers"
depends on ARCH_UNIPHIER || COMPILE_TEST
depends on OF && HAS_IOMEM
depends on PCI_MSI_IRQ_DOMAIN
select PCIE_DW_HOST
help
- Say Y here if you want PCIe controller support on UniPhier SoCs.
+ Say Y here if you want PCIe host controller support on UniPhier SoCs.
This driver supports LD20 and PXs3 SoCs.
+config PCIE_UNIPHIER_EP
+ bool "Socionext UniPhier PCIe endpoint controllers"
+ depends on ARCH_UNIPHIER || COMPILE_TEST
+ depends on OF && HAS_IOMEM
+ select PCIE_DW_EP
+ help
+ Say Y here if you want PCIe endpoint controller support on
+ UniPhier SoCs. This driver supports Pro5 SoC.
+
config PCIE_AL
bool "Amazon Annapurna Labs PCIe controller"
depends on OF && (ARM64 || COMPILE_TEST)
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 8a637cf..a751553 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
obj-$(CONFIG_PCI_MESON) += pci-meson.o
obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
+obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
# The following drivers are for devices that use the generic ACPI
# pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-uniphier-ep.c b/drivers/pci/controller/dwc/pcie-uniphier-ep.c
new file mode 100644
index 0000000..0f36aa3
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-uniphier-ep.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe endpoint controller driver for UniPhier SoCs
+ * Copyright 2018 Socionext Inc.
+ * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#include "pcie-designware.h"
+
+/* Link Glue registers */
+#define PCL_RSTCTRL0 0x0010
+#define PCL_RSTCTRL_AXI_REG BIT(3)
+#define PCL_RSTCTRL_AXI_SLAVE BIT(2)
+#define PCL_RSTCTRL_AXI_MASTER BIT(1)
+#define PCL_RSTCTRL_PIPE3 BIT(0)
+
+#define PCL_RSTCTRL1 0x0020
+#define PCL_RSTCTRL_PERST BIT(0)
+
+#define PCL_RSTCTRL2 0x0024
+#define PCL_RSTCTRL_PHY_RESET BIT(0)
+
+#define PCL_MODE 0x8000
+#define PCL_MODE_REGEN BIT(8)
+#define PCL_MODE_REGVAL BIT(0)
+
+#define PCL_APP_CLK_CTRL 0x8004
+#define PCL_APP_CLK_REQ BIT(0)
+
+#define PCL_APP_READY_CTRL 0x8008
+#define PCL_APP_LTSSM_ENABLE BIT(0)
+
+#define PCL_APP_MSI0 0x8040
+#define PCL_APP_VEN_MSI_TC_MASK GENMASK(10, 8)
+#define PCL_APP_VEN_MSI_VECTOR_MASK GENMASK(4, 0)
+
+#define PCL_APP_MSI1 0x8044
+#define PCL_APP_MSI_REQ BIT(0)
+
+#define PCL_APP_INTX 0x8074
+#define PCL_APP_INTX_SYS_INT BIT(0)
+
+/* assertion time of INTx in usec */
+#define PCL_INTX_WIDTH_USEC 30
+
+struct uniphier_pcie_ep_priv {
+ void __iomem *base;
+ struct dw_pcie pci;
+ struct clk *clk, *clk_gio;
+ struct reset_control *rst, *rst_gio;
+ struct phy *phy;
+ const struct pci_epc_features *features;
+};
+
+#define to_uniphier_pcie(x) dev_get_drvdata((x)->dev)
+
+static void uniphier_pcie_ltssm_enable(struct uniphier_pcie_ep_priv *priv,
+ bool enable)
+{
+ u32 val;
+
+ val = readl(priv->base + PCL_APP_READY_CTRL);
+ if (enable)
+ val |= PCL_APP_LTSSM_ENABLE;
+ else
+ val &= ~PCL_APP_LTSSM_ENABLE;
+ writel(val, priv->base + PCL_APP_READY_CTRL);
+}
+
+static void uniphier_pcie_phy_reset(struct uniphier_pcie_ep_priv *priv,
+ bool assert)
+{
+ u32 val;
+
+ val = readl(priv->base + PCL_RSTCTRL2);
+ if (assert)
+ val |= PCL_RSTCTRL_PHY_RESET;
+ else
+ val &= ~PCL_RSTCTRL_PHY_RESET;
+ writel(val, priv->base + PCL_RSTCTRL2);
+}
+
+static void uniphier_pcie_init_ep(struct uniphier_pcie_ep_priv *priv)
+{
+ u32 val;
+
+ /* set EP mode */
+ val = readl(priv->base + PCL_MODE);
+ val |= PCL_MODE_REGEN | PCL_MODE_REGVAL;
+ writel(val, priv->base + PCL_MODE);
+
+ /* clock request */
+ val = readl(priv->base + PCL_APP_CLK_CTRL);
+ val &= ~PCL_APP_CLK_REQ;
+ writel(val, priv->base + PCL_APP_CLK_CTRL);
+
+ /* deassert PIPE3 and AXI reset */
+ val = readl(priv->base + PCL_RSTCTRL0);
+ val |= PCL_RSTCTRL_AXI_REG | PCL_RSTCTRL_AXI_SLAVE
+ | PCL_RSTCTRL_AXI_MASTER | PCL_RSTCTRL_PIPE3;
+ writel(val, priv->base + PCL_RSTCTRL0);
+
+ uniphier_pcie_ltssm_enable(priv, false);
+
+ msleep(100);
+}
+
+static int uniphier_pcie_start_link(struct dw_pcie *pci)
+{
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+
+ uniphier_pcie_ltssm_enable(priv, true);
+
+ return 0;
+}
+
+static void uniphier_pcie_stop_link(struct dw_pcie *pci)
+{
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+
+ uniphier_pcie_ltssm_enable(priv, false);
+}
+
+static void uniphier_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ enum pci_barno bar;
+
+ for (bar = BAR_0; bar <= BAR_5; bar++)
+ dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int uniphier_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+ u32 val;
+
+ /*
+ * This makes pulse signal to send INTx to the RC, so this should
+ * be cleared as soon as possible. This sequence is covered with
+ * mutex in pci_epc_raise_irq().
+ */
+ /* assert INTx */
+ val = readl(priv->base + PCL_APP_INTX);
+ val |= PCL_APP_INTX_SYS_INT;
+ writel(val, priv->base + PCL_APP_INTX);
+
+ udelay(PCL_INTX_WIDTH_USEC);
+
+ /* deassert INTx */
+ val &= ~PCL_APP_INTX_SYS_INT;
+ writel(val, priv->base + PCL_APP_INTX);
+
+ return 0;
+}
+
+static int uniphier_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
+ u8 func_no, u16 interrupt_num)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+ u32 val;
+
+ val = FIELD_PREP(PCL_APP_VEN_MSI_TC_MASK, func_no)
+ | FIELD_PREP(PCL_APP_VEN_MSI_VECTOR_MASK, interrupt_num - 1);
+ writel(val, priv->base + PCL_APP_MSI0);
+
+ val = readl(priv->base + PCL_APP_MSI1);
+ val |= PCL_APP_MSI_REQ;
+ writel(val, priv->base + PCL_APP_MSI1);
+
+ return 0;
+}
+
+static int uniphier_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+ enum pci_epc_irq_type type,
+ u16 interrupt_num)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+ switch (type) {
+ case PCI_EPC_IRQ_LEGACY:
+ return uniphier_pcie_ep_raise_legacy_irq(ep);
+ case PCI_EPC_IRQ_MSI:
+ return uniphier_pcie_ep_raise_msi_irq(ep, func_no,
+ interrupt_num);
+ default:
+ dev_err(pci->dev, "UNKNOWN IRQ type (%d)\n", type);
+ }
+
+ return 0;
+}
+
+static const struct pci_epc_features*
+uniphier_pcie_get_features(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+
+ return priv->features;
+}
+
+static const struct dw_pcie_ep_ops uniphier_pcie_ep_ops = {
+ .ep_init = uniphier_pcie_ep_init,
+ .raise_irq = uniphier_pcie_ep_raise_irq,
+ .get_features = uniphier_pcie_get_features,
+};
+
+static int uniphier_add_pcie_ep(struct uniphier_pcie_ep_priv *priv,
+ struct platform_device *pdev)
+{
+ struct dw_pcie *pci = &priv->pci;
+ struct dw_pcie_ep *ep = &pci->ep;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int ret;
+
+ ep->ops = &uniphier_pcie_ep_ops;
+
+ pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
+ if (IS_ERR(pci->dbi_base2))
+ return PTR_ERR(pci->dbi_base2);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+ if (!res)
+ return -EINVAL;
+
+ ep->phys_base = res->start;
+ ep->addr_size = resource_size(res);
+
+ ret = dw_pcie_ep_init(ep);
+ if (ret)
+ dev_err(dev, "Failed to initialize endpoint (%d)\n", ret);
+
+ return ret;
+}
+
+static int uniphier_pcie_ep_enable(struct uniphier_pcie_ep_priv *priv)
+{
+ int ret;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(priv->clk_gio);
+ if (ret)
+ goto out_clk_disable;
+
+ ret = reset_control_deassert(priv->rst);
+ if (ret)
+ goto out_clk_gio_disable;
+
+ ret = reset_control_deassert(priv->rst_gio);
+ if (ret)
+ goto out_rst_assert;
+
+ uniphier_pcie_init_ep(priv);
+
+ uniphier_pcie_phy_reset(priv, true);
+
+ ret = phy_init(priv->phy);
+ if (ret)
+ goto out_rst_gio_assert;
+
+ uniphier_pcie_phy_reset(priv, false);
+
+ return 0;
+
+out_rst_gio_assert:
+ reset_control_assert(priv->rst_gio);
+out_rst_assert:
+ reset_control_assert(priv->rst);
+out_clk_gio_disable:
+ clk_disable_unprepare(priv->clk_gio);
+out_clk_disable:
+ clk_disable_unprepare(priv->clk);
+
+ return ret;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+ .start_link = uniphier_pcie_start_link,
+ .stop_link = uniphier_pcie_stop_link,
+};
+
+static int uniphier_pcie_ep_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct uniphier_pcie_ep_priv *priv;
+ struct resource *res;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->features = of_device_get_match_data(dev);
+ if (WARN_ON(!priv->features))
+ return -EINVAL;
+
+ priv->pci.dev = dev;
+ priv->pci.ops = &dw_pcie_ops;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+ priv->pci.dbi_base = devm_pci_remap_cfg_resource(dev, res);
+ if (IS_ERR(priv->pci.dbi_base))
+ return PTR_ERR(priv->pci.dbi_base);
+
+ priv->base = devm_platform_ioremap_resource_byname(pdev, "link");
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->clk_gio = devm_clk_get(dev, "gio");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ priv->rst_gio = devm_reset_control_get_shared(dev, "gio");
+ if (IS_ERR(priv->rst_gio))
+ return PTR_ERR(priv->rst_gio);
+
+ priv->clk = devm_clk_get(dev, "link");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ priv->rst = devm_reset_control_get_shared(dev, "link");
+ if (IS_ERR(priv->rst))
+ return PTR_ERR(priv->rst);
+
+ priv->phy = devm_phy_optional_get(dev, "pcie-phy");
+ if (IS_ERR(priv->phy)) {
+ ret = PTR_ERR(priv->phy);
+ dev_err(dev, "Failed to get phy (%d)\n", ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, priv);
+
+ ret = uniphier_pcie_ep_enable(priv);
+ if (ret)
+ return ret;
+
+ return uniphier_add_pcie_ep(priv, pdev);
+}
+
+static const struct pci_epc_features uniphier_pro5_data = {
+ .linkup_notifier = false,
+ .msi_capable = true,
+ .msix_capable = false,
+ .align = 1 << 16,
+ .bar_fixed_64bit = BIT(BAR_0) | BIT(BAR_2) | BIT(BAR_4),
+ .reserved_bar = BIT(BAR_4),
+};
+
+static const struct of_device_id uniphier_pcie_ep_match[] = {
+ {
+ .compatible = "socionext,uniphier-pro5-pcie-ep",
+ .data = &uniphier_pro5_data,
+ },
+ { /* sentinel */ },
+};
+
+static struct platform_driver uniphier_pcie_ep_driver = {
+ .probe = uniphier_pcie_ep_probe,
+ .driver = {
+ .name = "uniphier-pcie-ep",
+ .of_match_table = uniphier_pcie_ep_match,
+ .suppress_bind_attrs = true,
+ },
+};
+builtin_platform_driver(uniphier_pcie_ep_driver);
--
2.7.4
^ permalink raw reply related
* [PATCH v4 0/2] PCI: Add new UniPhier PCIe endpoint driver
From: Kunihiko Hayashi @ 2020-05-14 12:03 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Masahiro Yamada
Cc: linux-pci, devicetree, linux-arm-kernel, linux-kernel,
Masami Hiramatsu, Jassi Brar, Kunihiko Hayashi
This series adds PCIe endpoint controller driver for Socionext UniPhier
SoCs. This controller is based on the DesignWare PCIe core.
This driver supports Pro5 SoC only, so Pro5 needs multiple clocks and
resets in devicetree node.
Changes since v3:
- dt-bindings: Convert with dt-schema
- Replace with devm_platform_ioremap_resource()
- Add a commnet that mutex covers raising legacy IRQ
Changes since v2:
- dt-bindings: Add clock-names, reset-names, and fix example for Pro5
- Remove 'is_legacy' indicating that the compatible is for legacy SoC
- Use pci_epc_features instead of defining uniphier_soc_data
- Remove redundant register read access
- Clean up return code on uniphier_add_pcie_ep()
- typo: intx -> INTx
Changes since v1:
- dt-bindings: Add Reviewed-by line
- Fix register value to set EP mode
- Add error message when failed to get phy
- Replace INTx assertion time with macro
Kunihiko Hayashi (2):
dt-bindings: PCI: Add UniPhier PCIe endpoint controller description
PCI: uniphier: Add Socionext UniPhier Pro5 PCIe endpoint controller
driver
.../bindings/pci/socionext,uniphier-pcie-ep.yaml | 92 +++++
MAINTAINERS | 4 +-
drivers/pci/controller/dwc/Kconfig | 13 +-
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-uniphier-ep.c | 383 +++++++++++++++++++++
5 files changed, 489 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
create mode 100644 drivers/pci/controller/dwc/pcie-uniphier-ep.c
--
2.7.4
^ permalink raw reply
* [PATCH v4 1/2] dt-bindings: PCI: Add UniPhier PCIe endpoint controller description
From: Kunihiko Hayashi @ 2020-05-14 12:03 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Masahiro Yamada
Cc: linux-pci, devicetree, linux-arm-kernel, linux-kernel,
Masami Hiramatsu, Jassi Brar, Kunihiko Hayashi
In-Reply-To: <1589457801-12796-1-git-send-email-hayashi.kunihiko@socionext.com>
Add DT bindings for PCIe controller implemented in UniPhier SoCs
when configured in endpoint mode. This controller is based on
the DesignWare PCIe core.
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
.../bindings/pci/socionext,uniphier-pcie-ep.yaml | 92 ++++++++++++++++++++++
MAINTAINERS | 2 +-
2 files changed, 93 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
diff --git a/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
new file mode 100644
index 0000000..f0558b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/socionext,uniphier-pcie-ep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Socionext UniPhier PCIe endpoint controller
+
+description: |
+ UniPhier PCIe endpoint controller is based on the Synopsys DesignWare
+ PCI core. It shares common features with the PCIe DesignWare core and
+ inherits common properties defined in
+ Documentation/devicetree/bindings/pci/designware-pcie.txt.
+
+maintainers:
+ - Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
+
+allOf:
+ - $ref: "pci-ep.yaml#"
+
+properties:
+ compatible:
+ const: socionext,uniphier-pro5-pcie-ep
+
+ reg:
+ maxItems: 4
+
+ reg-names:
+ items:
+ - const: dbi
+ - const: dbi2
+ - const: link
+ - const: addr_space
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: gio
+ - const: link
+
+ resets:
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: gio
+ - const: link
+
+ num-ib-windows:
+ const: 16
+
+ num-ob-windows:
+ const: 16
+
+ num-lanes: true
+
+ phys:
+ maxItems: 1
+
+ phy-names:
+ const: pcie-phy
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+
+additionalProperties: false
+
+examples:
+ - |
+ pcie_ep: pcie-ep@66000000 {
+ compatible = "socionext,uniphier-pro5-pcie-ep";
+ reg-names = "dbi", "dbi2", "link", "addr_space";
+ reg = <0x66000000 0x1000>, <0x66001000 0x1000>,
+ <0x66010000 0x10000>, <0x67000000 0x400000>;
+ clock-names = "gio", "link";
+ clocks = <&sys_clk 12>, <&sys_clk 24>;
+ reset-names = "gio", "link";
+ resets = <&sys_rst 12>, <&sys_rst 24>;
+ num-ib-windows = <16>;
+ num-ob-windows = <16>;
+ num-lanes = <4>;
+ phy-names = "pcie-phy";
+ phys = <&pcie_phy>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 92657a1..7f26748 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13211,7 +13211,7 @@ PCIE DRIVER FOR SOCIONEXT UNIPHIER
M: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
L: linux-pci@vger.kernel.org
S: Maintained
-F: Documentation/devicetree/bindings/pci/uniphier-pcie.txt
+F: Documentation/devicetree/bindings/pci/uniphier-pcie*
F: drivers/pci/controller/dwc/pcie-uniphier.c
PCIE DRIVER FOR ST SPEAR13XX
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Geert Uytterhoeven @ 2020-05-14 11:59 UTC (permalink / raw)
To: Kieran Bingham
Cc: Manivannan Sadhasivam, Linux-Renesas, Linux Media Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Mauro Carvalho Chehab, Kieran Bingham,
Laurent Pinchart, Jacopo Mondi, Niklas Söderlund,
Sakari Ailus, Hans Verkuil, Hyun Kwon, Rob Herring, Jacopo Mondi,
Laurent Pinchart, Niklas Söderlund
In-Reply-To: <d492cbcb-3b13-82b8-8e5d-0f49320170a2@ideasonboard.com>
Hi Kieran,
On Thu, May 14, 2020 at 1:47 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> On 14/05/2020 11:13, Manivannan Sadhasivam wrote:
> > On Thu, May 14, 2020 at 11:02:53AM +0100, Kieran Bingham wrote:
> >> On 12/05/2020 19:17, Manivannan Sadhasivam wrote:
> >>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
> >>>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
> >>>> CSI-2 output. The device supports multicamera streaming applications,
> >>>> and features the ability to synchronise the attached cameras.
> >>>>
> >>>> CSI-2 output can be configured with 1 to 4 lanes, and a control channel
> >>>> is supported over I2C, which implements an I2C mux to facilitate
> >>>> communications with connected cameras across the reverse control
> >>>> channel.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>> --- /dev/null
> >>>> +++ b/drivers/media/i2c/max9286.c
> >>>> +static int max9286_register_gpio(struct max9286_priv *priv)
> >>>> +{
> >>>> + struct device *dev = &priv->client->dev;
> >>>> + struct gpio_chip *gpio = &priv->gpio;
> >>>> + int ret;
> >>>> +
> >>>> + static const char * const names[] = {
> >>>> + "GPIO0OUT",
> >>>> + "GPIO1OUT",
> >>>> + };
> >>>> +
> >>>> + /* Configure the GPIO */
> >>>> + gpio->label = dev_name(dev);
> >>>
> >>> So if you have more than one MAX9286 in a system, all gpiochips will appear
> >>> with the same name. I'd recommend to append the index to distinguish properly.
> >>
> >> Ah yes, that's a good point, and I think I've even seen that.
> >>
> >> I'll fix it now.
>
> Oh, in fact actually this doesn't.
>
> gpiodetect prints:
>
> gpiochip10 [4-004c] (2 lines)
> gpiochip11 [4-006c] (2 lines)
>
> and mostly references them as gpiochip10 and gpiochip11.
Indeed, dev_name() should be different for each instance.
> However,
>
> > [ 2.318533] gpio gpiochip11: Detected name collision for GPIO name 'GPIO0OUT'
> > [ 2.325739] gpio gpiochip11: Detected name collision for GPIO name 'GPIO1OUT'
>
> That seems to be more of a problem for the gpio library, so I think I'll
> just drop the const names. I don't think they add much value.
These are the line names. If they're not unique, a warning is printed,
but they are still registered.
So probably you want to use kasprintf("%s.%s", dev_name(dev), names[i]) to
generate names.
See "[PATCH] gpiolib: Document that GPIO line names are not globally unique"
(https://lore.kernel.org/linux-gpio/20200511101828.30046-1-geert+renesas@glider.be/)
to clear up the details.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
From: Sumit Semwal @ 2020-05-14 11:58 UTC (permalink / raw)
To: Bjorn Andersson
Cc: agross, lgirdwood, Mark Brown, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
In-Reply-To: <20200512052541.GF1302550@yoga>
Hello Bjorn,
Thanks for the review comments.
On Tue, 12 May 2020 at 10:55, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 08 May 13:42 PDT 2020, Sumit Semwal wrote:
>
> > From: Nisha Kumari <nishakumari@codeaurora.org>
> >
> > Add Short circuit interrupt handling and recovery for the lab and
> > ibb regulators on qcom platforms.
> >
> > The client panel drivers need to register for REGULATOR_EVENT_OVER_CURRENT
> > notification which will be triggered on short circuit. They should
> > try to enable the regulator once, and if it doesn't get enabled,
> > handle shutting down the panel accordingly.
> >
> > Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> >
> > --
> > v2: sumits: reworked handling to user regmap_read_poll_timeout, and handle it
> > per-regulator instead of clearing both lab and ibb errors on either irq
> > triggering. Also added REGULATOR_EVENT_OVER_CURRENT handling and
> > notification to clients.
> > ---
> > drivers/regulator/qcom-labibb-regulator.c | 103 +++++++++++++++++++++-
> > 1 file changed, 100 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> > index a9dc7c060375..3539631c9f96 100644
> > --- a/drivers/regulator/qcom-labibb-regulator.c
> > +++ b/drivers/regulator/qcom-labibb-regulator.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> > // Copyright (c) 2019, The Linux Foundation. All rights reserved.
> >
> > +#include <linux/interrupt.h>
> > #include <linux/module.h>
> > #include <linux/of_irq.h>
> > #include <linux/of.h>
> > @@ -18,11 +19,15 @@
> > #define REG_LABIBB_ENABLE_CTL 0x46
> > #define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
> > #define LABIBB_CONTROL_ENABLE BIT(7)
> > +#define LABIBB_STATUS1_SC_DETECT_BIT BIT(6)
> >
> > #define LAB_ENABLE_CTL_MASK BIT(7)
> > #define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6))
> >
> > #define POWER_DELAY 8000
> > +#define POLLING_SCP_DONE_INTERVAL_US 5000
> > +#define POLLING_SCP_TIMEOUT 16000
> > +
> >
> > struct labibb_regulator {
> > struct regulator_desc desc;
> > @@ -30,6 +35,8 @@ struct labibb_regulator {
> > struct regmap *regmap;
> > struct regulator_dev *rdev;
> > u16 base;
> > + int sc_irq;
> > + int vreg_enabled;
> > u8 type;
> > };
> >
> > @@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > return ret;
> > }
> >
> > - if (ret)
> > + if (ret) {
> > + reg->vreg_enabled = 1;
> > return 0;
> > -
> > + }
> >
> > dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> > return -EINVAL;
> > @@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> > return ret;
> > }
> >
> > - if (!ret)
> > + if (!ret) {
> > + reg->vreg_enabled = 0;
> > return 0;
> > + }
> >
> > dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> > return -EINVAL;
> > @@ -153,6 +163,70 @@ static struct regulator_ops qcom_labibb_ops = {
> > .is_enabled = qcom_labibb_regulator_is_enabled,
> > };
> >
> > +
> > +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> > +{
> > + int ret, count;
> > + u16 reg;
> > + u8 sc_err_mask;
> > + unsigned int val;
> > + struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
>
> No need to explicitly typecast a void *.
ok.
>
> > + bool in_sc_err, reg_en, scp_done = false;
>
> reg_en is unused.
>
Yes, will remove.
> > +
> > + if (irq == labibb_reg->sc_irq)
>
> When is this false?
Shouldn't be; will remove the check.
>
> > + reg = labibb_reg->base + REG_LABIBB_STATUS1;
> > + else
> > + return IRQ_HANDLED;
> > +
> > + sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
> > +
> > + ret = regmap_bulk_read(labibb_reg->regmap, reg, &val, 1);
>
> Just inline reg->base + REG_LABIBB_STATUS1 in this call.
Ok.
>
> > + if (ret < 0) {
> > + dev_err(labibb_reg->dev, "Read failed, ret=%d\n", ret);
> > + return IRQ_HANDLED;
> > + }
> > + dev_dbg(labibb_reg->dev, "%s SC error triggered! STATUS1 = %d\n",
> > + labibb_reg->desc.name, val);
> > +
> > + in_sc_err = !!(val & sc_err_mask);
> > +
> > + /*
> > + * The SC(short circuit) fault would trigger PBS(Portable Batch
> > + * System) to disable regulators for protection. This would
> > + * cause the SC_DETECT status being cleared so that it's not
> > + * able to get the SC fault status.
> > + * Check if the regulator is enabled in the driver but
> > + * disabled in hardware, this means a SC fault had happened
> > + * and SCP handling is completed by PBS.
> > + */
> > + if (!in_sc_err) {
>
> if (!(val & LABIBB_STATUS1_SC_DETECT_BIT)) {
>
> > +
> > + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> > +
> > + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> > + reg, val,
> > + !(val & LABIBB_CONTROL_ENABLE),
> > + POLLING_SCP_DONE_INTERVAL_US,
> > + POLLING_SCP_TIMEOUT);
> > +
> > + if (!ret && labibb_reg->vreg_enabled) {
>
> Wouldn't be fine to assume that if you get a short circuit IRQ the
> regulator is enabled?
So this is from my understanding of the previous patchset by the
author, as documented in the comment: I understood it to mean that if
we aren't able to read back the 'ENABLE' bit from the hardware (so
it's disabled in hw?) but the driver has it's vreg->enabled set, that
means the SC fault had happened but was handled by the PBS?
>
> If you are worried about racing with a disable this won't work anyways,
> and you better enable_irq()/disable_irq() in regulator enable/disable,
> respectively.
>
It didn't seem to be about the disable race, but being handled by the PBS.
> > + dev_dbg(labibb_reg->dev,
> > + "%s has been disabled by SCP\n",
> > + labibb_reg->desc.name);
> > + scp_done = true;
> > + }
>
> If you flip the poll check around you will get here by not being in an
> short-circuit condition and you conclude that the regulator is still on;
> in which case you can just return here.
>
> That way you can drop in_sc_err and scp_done and flatten below
> conditional section.
Yeah I think that's related to the PBS v/s regulator's handling of the
short-circuit, afaiu.
>
> > + }
> > +
> > + if (in_sc_err || scp_done) {
> > + regulator_lock(labibb_reg->rdev);
> > + regulator_notifier_call_chain(labibb_reg->rdev,
> > + REGULATOR_EVENT_OVER_CURRENT,
> > + NULL);
> > + regulator_unlock(labibb_reg->rdev);
> > + }
> > + return IRQ_HANDLED;
> > +}
> > +
> > static int register_labibb_regulator(struct qcom_labibb *labibb,
> > const struct labibb_regulator_data *reg_data,
> > struct device_node *of_node)
> > @@ -181,6 +255,29 @@ static int register_labibb_regulator(struct qcom_labibb *labibb,
> > reg->desc.type = REGULATOR_VOLTAGE;
> > reg->desc.ops = &qcom_labibb_ops;
> >
> > + reg->sc_irq = -EINVAL;
> > + ret = of_irq_get_byname(of_node, reg_data->irq_name);
> > + if (ret < 0)
> > + dev_dbg(labibb->dev,
>
> Isn't this an error?
yes, will change.
>
> > + "Unable to get %s, ret = %d\n",
> > + reg_data->irq_name, ret);
> > + else
> > + reg->sc_irq = ret;
> > +
> > + if (reg->sc_irq > 0) {
> > + ret = devm_request_threaded_irq(labibb->dev,
> > + reg->sc_irq,
> > + NULL, labibb_sc_err_handler,
> > + IRQF_ONESHOT |
> > + IRQF_TRIGGER_RISING,
>
> Omit IRQF_TRIGGER_RISING and let that come from DT.
Ok. That's anyways coming from the DT.
>
> > + reg_data->irq_name, labibb);
> > + if (ret) {
> > + dev_err(labibb->dev, "Failed to register '%s' irq ret=%d\n",
> > + reg_data->irq_name, ret);
> > + return ret;
> > + }
> > + }
> > +
>
> Regards,
> Bjorn
>
> > cfg.dev = labibb->dev;
> > cfg.driver_data = reg;
> > cfg.regmap = labibb->regmap;
> > --
> > 2.26.2
> >
Best,
Sumit.
^ permalink raw reply
* Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
From: Sumit Semwal @ 2020-05-14 11:50 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
In-Reply-To: <20200511104922.GD8216@sirena.org.uk>
Hello Mark,
Thanks for the review comments.
On Mon, 11 May 2020 at 16:19, Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, May 09, 2020 at 02:12:00AM +0530, Sumit Semwal wrote:
>
> > +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> > +{
> > + int ret, count;
> > + u16 reg;
> > + u8 sc_err_mask;
> > + unsigned int val;
> > + struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
> > + bool in_sc_err, reg_en, scp_done = false;
> > +
> > + if (irq == labibb_reg->sc_irq)
> > + reg = labibb_reg->base + REG_LABIBB_STATUS1;
> > + else
> > + return IRQ_HANDLED;
>
> Why would we be registering the interrupt handler when it's not valid?
Agreed, will correct.
Best,
Sumit.
^ permalink raw reply
* [PATCH] dt-bindings: pwm: Convert mxs pwm to json-schema
From: Anson Huang @ 2020-05-14 11:41 UTC (permalink / raw)
To: thierry.reding, u.kleine-koenig, robh+dt, shawnguo, s.hauer,
kernel, festevam, linux-pwm, devicetree, linux-arm-kernel,
linux-kernel
Cc: Linux-imx
Convert the mxs pwm binding to DT schema format using json-schema.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Documentation/devicetree/bindings/pwm/mxs-pwm.txt | 17 ---------
Documentation/devicetree/bindings/pwm/mxs-pwm.yaml | 43 ++++++++++++++++++++++
2 files changed, 43 insertions(+), 17 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/pwm/mxs-pwm.txt
create mode 100644 Documentation/devicetree/bindings/pwm/mxs-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/mxs-pwm.txt b/Documentation/devicetree/bindings/pwm/mxs-pwm.txt
deleted file mode 100644
index a1b8a48..0000000
--- a/Documentation/devicetree/bindings/pwm/mxs-pwm.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-Freescale MXS PWM controller
-
-Required properties:
-- compatible: should be "fsl,imx23-pwm"
-- reg: physical base address and length of the controller's registers
-- #pwm-cells: should be 3. See pwm.yaml in this directory for a description of
- the cells format.
-- fsl,pwm-number: the number of PWM devices
-
-Example:
-
-pwm: pwm@80064000 {
- compatible = "fsl,imx28-pwm", "fsl,imx23-pwm";
- reg = <0x80064000 0x2000>;
- #pwm-cells = <3>;
- fsl,pwm-number = <8>;
-};
diff --git a/Documentation/devicetree/bindings/pwm/mxs-pwm.yaml b/Documentation/devicetree/bindings/pwm/mxs-pwm.yaml
new file mode 100644
index 0000000..da68f4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/mxs-pwm.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/mxs-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale MXS PWM controller
+
+maintainers:
+ - Shawn Guo <shawn.guo@linaro.org>
+ - Anson Huang <anson.huang@nxp.com>
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx23-pwm
+
+ reg:
+ maxItems: 1
+
+ "#pwm-cells":
+ const: 3
+
+ fsl,pwm-number:
+ $ref: '/schemas/types.yaml#/definitions/uint32'
+ description: u32 value representing the number of PWM devices
+
+required:
+ - compatible
+ - reg
+ - "#pwm-cells"
+ - fsl,pwm-number
+
+additionalProperties: false
+
+examples:
+ - |
+ pwm@80064000 {
+ compatible = "fsl,imx23-pwm";
+ reg = <0x80064000 0x2000>;
+ #pwm-cells = <3>;
+ fsl,pwm-number = <8>;
+ };
--
2.7.4
^ permalink raw reply related
* Re: [v2 1/4] dt-bindings: regulator: Add labibb regulator
From: Sumit Semwal @ 2020-05-14 11:50 UTC (permalink / raw)
To: Bjorn Andersson
Cc: agross, lgirdwood, Mark Brown, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
In-Reply-To: <20200512014339.GE57962@builder.lan>
Hello Bjorn,
Thanks for the comments!
On Tue, 12 May 2020 at 07:15, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 08 May 13:41 PDT 2020, Sumit Semwal wrote:
>
> > From: Nisha Kumari <nishakumari@codeaurora.org>
> >
> > Adding the devicetree binding for labibb regulator.
> >
> > Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
>
> Sorry, I missed this when we talked about it. But please rewrite this in
> yaml.
Sure, I will.
>
> >
> > --
> > v2: updated for better compatible string and names.
> > ---
> > .../regulator/qcom-labibb-regulator.txt | 47 +++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> > new file mode 100644
> > index 000000000000..6e639d69f780
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> > @@ -0,0 +1,47 @@
> > +Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
> > +
> > +LAB can be used as a positive boost power supply and IBB can be used as a negative
> > +boost power supply for display panels. Currently implemented for pmi8998.
> > +
> > +Main node required properties:
> > +
> > +- compatible: Must be:
> > + "qcom,pmi8998-lab-ibb"
> > +- #address-cells: Must be 1
> > +- #size-cells: Must be 0
>
> But the children doesn't have reg properties...
Oh, right. Let me correct that.
>
> > +
> > +LAB subnode required properties:
> > +
> > +- interrupts: Specify the interrupts as per the interrupt
> > + encoding.
> > +- interrupt-names: Interrupt names to match up 1-to-1 with
> > + the interrupts specified in 'interrupts'
> > + property.
>
> Do specify the expected string (and given that you already have the
> lab & ibb subnodes, you don't need to include this in the string).
Yes, agreed. Keeping it as 'sc-err' makes the code easier too.
>
> Regards,
> Bjorn
Best,
Sumit.
>
> > +
> > +IBB subnode required properties:
> > +
> > +- interrupts: Specify the interrupts as per the interrupt
> > + encoding.
> > +- interrupt-names: Interrupt names to match up 1-to-1 with
> > + the interrupts specified in 'interrupts'
> > + property.
> > +
> > +Example:
> > + pmi8998_lsid1: pmic@3 {
> > + labibb {
> > + compatible = "qcom,pmi8998-lab-ibb";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + lab: lab {
> > + interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
> > + interrupt-names = "lab-sc-err";
> > + };
> > +
> > + ibb: ibb {
> > + interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
> > + interrupt-names = "ibb-sc-err";
> > + };
> > +
> > + };
> > + };
> > --
> > 2.26.2
> >
^ permalink raw reply
* Re: [v2 3/4] regulator: qcom: Add labibb driver
From: Sumit Semwal @ 2020-05-14 11:47 UTC (permalink / raw)
To: Bjorn Andersson
Cc: agross, lgirdwood, Mark Brown, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
In-Reply-To: <20200512021509.GF57962@builder.lan>
Hello Bjorn,
Thanks very much for the review.
On Tue, 12 May 2020 at 07:46, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 08 May 13:41 PDT 2020, Sumit Semwal wrote:
>
> > From: Nisha Kumari <nishakumari@codeaurora.org>
> >
> > Qualcomm platforms have LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost)
> > Regulators, labibb for short, which are used as power supply for
>
> lowercase Regulators
ok.
>
> > LCD Mode displays.
> >
> > This patch adds labibb regulator driver for pmi8998 pmic, found on
>
> Uppercase PMIC
>
ok.
> > SDM845 platforms.
> >
> > Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> >
> > --
> > v2: sumits: reworked the driver for more common code, and addressed
> > review comments from v1. This includes merging regulator_ops into
> > one, and allowing for future labibb variations.
> > ---
> > drivers/regulator/Kconfig | 10 +
> > drivers/regulator/Makefile | 1 +
> > drivers/regulator/qcom-labibb-regulator.c | 288 ++++++++++++++++++++++
> > 3 files changed, 299 insertions(+)
> > create mode 100644 drivers/regulator/qcom-labibb-regulator.c
> >
> > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> > index f4b72cb098ef..58704a9fd05d 100644
> > --- a/drivers/regulator/Kconfig
> > +++ b/drivers/regulator/Kconfig
> > @@ -1167,5 +1167,15 @@ config REGULATOR_WM8994
> > This driver provides support for the voltage regulators on the
> > WM8994 CODEC.
> >
> > +config REGULATOR_QCOM_LABIBB
> > + tristate "QCOM LAB/IBB regulator support"
> > + depends on SPMI || COMPILE_TEST
> > + help
> > + This driver supports Qualcomm's LAB/IBB regulators present on the
> > + Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI
> > + based PMIC implementations. LAB can be used as positive
> > + boost regulator and IBB can be used as a negative boost regulator
> > + for LCD display panel.
> > +
> > endif
> >
> > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> > index 6610ee001d9a..5b313786c0e8 100644
> > --- a/drivers/regulator/Makefile
> > +++ b/drivers/regulator/Makefile
> > @@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
> > obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o
> > obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
> > obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
> > +obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
> > obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
> > obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
> > obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> > diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> > new file mode 100644
> > index 000000000000..a9dc7c060375
> > --- /dev/null
> > +++ b/drivers/regulator/qcom-labibb-regulator.c
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019, The Linux Foundation. All rights reserved.
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/of_regulator.h>
> > +
> > +#define REG_PERPH_TYPE 0x04
> > +#define QCOM_LAB_TYPE 0x24
> > +#define QCOM_IBB_TYPE 0x20
> > +
> > +#define REG_LABIBB_STATUS1 0x08
> > +#define REG_LABIBB_ENABLE_CTL 0x46
> > +#define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
> > +#define LABIBB_CONTROL_ENABLE BIT(7)
> > +
> > +#define LAB_ENABLE_CTL_MASK BIT(7)
> > +#define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6))
> > +
> > +#define POWER_DELAY 8000
> > +
> > +struct labibb_regulator {
> > + struct regulator_desc desc;
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct regulator_dev *rdev;
> > + u16 base;
> > + u8 type;
> > +};
> > +
> > +struct qcom_labibb {
>
> You pretty much use this as a local variable within probe, and then you
> use labibb_regulator in runtime. Perhaps you can just drop it?
>
Yes, you're right. It's probably a leftover from the re-design. I will drop it.
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct labibb_regulator lab;
> > + struct labibb_regulator ibb;
> > +};
> > +
> > +struct labibb_regulator_data {
> > + u16 base;
> > + const char *name;
> > + const char *irq_name;
> > + u8 type;
> > +};
> > +
> > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
> > +{
> > + int ret;
> > + u8 val;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + ret = regmap_bulk_read(reg->regmap, reg->base +
> > + REG_LABIBB_STATUS1, &val, 1);
> > + if (ret < 0) {
> > + dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> > + return ret;
> > + }
> > +
> > + if (val & LABIBB_STATUS1_VREG_OK_BIT)
> > + return 1;
> > + else
> > + return 0;
>
> return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
>
> > +}
> > +
> > +static int _check_enabled_with_retries(struct regulator_dev *rdev,
> > + int retries, int enabled)
> > +{
> > + int ret;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + while (retries--) {
>
> Mark's suggestion of extending _regulator_enable_delay() to support
> polling is_enable() seems reasonable.
>
> The only complication I can see is that code path currently doesn't have
> any expectations of the regulator not being operational at the end -
> this seems to offer that possibility. So some care needs to be taken
> there.
>
I think when I try Mark's suggestion, there will be a need to
communicate an error if we don't get a successful check of status
after the requested number of retries. So yes, I will try and take
care in the next iteration.
>
> But doing that would allow you to use regulator_enable_regmap() as your
> .enable function directly and you can drop
> qcom_labibb_regulator_enable()
>
Agreed.
> > + /* Wait for a small period before checking REG_LABIBB_STATUS1 */
> > + usleep_range(POWER_DELAY, POWER_DELAY + 200);
> > +
> > + ret = qcom_labibb_regulator_is_enabled(rdev);
> > +
> > + if (ret < 0) {
> > + dev_err(reg->dev, "Can't read %s regulator status\n",
> > + reg->desc.name);
> > + return ret;
> > + }
> > +
> > + if (ret == enabled)
> > + return ret;
> > +
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > +{
> > + int ret, retries = 10;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + ret = regulator_enable_regmap(rdev);
> > +
> > + if (ret < 0) {
> > + dev_err(reg->dev, "Write failed: enable %s regulator\n",
> > + reg->desc.name);
> > + return ret;
> > + }
> > +
> > + ret = _check_enabled_with_retries(rdev, retries, 1);
> > + if (ret < 0) {
> > + dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
> > + reg->desc.name);
> > + return ret;
> > + }
> > +
> > + if (ret)
> > + return 0;
>
> This looks weird; check for timeout, check for errors and then return
> success.
Yes, it does look weird - that's because the
_check_enabled_with_retries returns 1 if it matches what you asked to
check (enable or disable via last argument). That should've been coded
differently.
It should go away with the above mentioned rework.
>
> > +
> > +
> > + dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> > + return -EINVAL;
> > +}
> > +
> > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> > +{
> > + int ret, retries = 2;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + ret = regulator_disable_regmap(rdev);
> > +
> > + if (ret < 0) {
> > + dev_err(reg->dev, "Write failed: disable %s regulator\n",
> > + reg->desc.name);
> > + return ret;
> > + }
> > +
> > + ret = _check_enabled_with_retries(rdev, retries, 0);
>
> I don't think we care about waiting for the regulator to turning off,
> might be nice to ensure that an off/on cycle really is allowed to take
> its time though. So specifying an desc->off_on_delay of 8200 * 2 (the
> worst case usleep_range() * retries) should take care of this.
Oh, that's neat. Will update.
>
> And then you should be able to just use regulator_disable_regmap() as
> your .disable function.
+1.
>
> > + if (ret < 0) {
> > + dev_err(reg->dev, "retries exhausted: disable %s regulator\n",
> > + reg->desc.name);
> > + return ret;
> > + }
> > +
> > + if (!ret)
> > + return 0;
> > +
> > + dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> > + return -EINVAL;
> > +}
> > +
> > +static struct regulator_ops qcom_labibb_ops = {
> > + .enable = qcom_labibb_regulator_enable,
> > + .disable = qcom_labibb_regulator_disable,
> > + .is_enabled = qcom_labibb_regulator_is_enabled,
> > +};
> > +
> > +static int register_labibb_regulator(struct qcom_labibb *labibb,
> > + const struct labibb_regulator_data *reg_data,
> > + struct device_node *of_node)
> > +{
> > + int ret;
> > + struct labibb_regulator *reg;
> > + struct regulator_config cfg = {};
> > +
> > + if (reg_data->type == QCOM_LAB_TYPE) {
> > + reg = &labibb->lab;
> > + reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
> > + } else {
> > + reg = &labibb->ibb;
> > + reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
> > + }
> > +
> > + reg->dev = labibb->dev;
> > + reg->base = reg_data->base;
> > + reg->type = reg_data->type;
> > + reg->regmap = labibb->regmap;
> > + reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL;
> > + reg->desc.enable_val = LABIBB_CONTROL_ENABLE;
> > + reg->desc.of_match = reg_data->name;
> > + reg->desc.name = reg_data->name;
> > + reg->desc.owner = THIS_MODULE;
> > + reg->desc.type = REGULATOR_VOLTAGE;
> > + reg->desc.ops = &qcom_labibb_ops;
> > +
> > + cfg.dev = labibb->dev;
> > + cfg.driver_data = reg;
> > + cfg.regmap = labibb->regmap;
> > + cfg.of_node = of_node;
> > +
> > + reg->rdev = devm_regulator_register(labibb->dev, ®->desc,
> > + &cfg);
>
> Do you really need to wrap this line?
No :) - rework leftover. Will correct.
>
> > + if (IS_ERR(reg->rdev)) {
> > + ret = PTR_ERR(reg->rdev);
> > + dev_err(labibb->dev,
> > + "unable to register %s regulator\n", reg_data->name);
> > + return ret;
> > + }
> > + return 0;
>
> return PTR_ERR_OR_ZERO(reg->rdev);
>
> > +}
> > +
> > +static const struct labibb_regulator_data pmi8998_labibb_data[] = {
> > + {0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE},
>
> Please shorten the interrupt to just "sc-err".
Ok. And actually then I can remove it off from the
labibb_regulotor_data too, since it will be the same for both the
regulators.
>
> > + {0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE},
> > + { },
> > +};
> > +
> > +static const struct of_device_id qcom_labibb_match[] = {
> > + { .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data},
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, qcom_labibb_match);
> > +
> > +static int qcom_labibb_regulator_probe(struct platform_device *pdev)
> > +{
> > + struct qcom_labibb *labibb;
> > + struct device_node *child;
> > + const struct of_device_id *match;
> > + const struct labibb_regulator_data *reg;
> > + u8 type;
> > + int ret;
> > +
> > + labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL);
> > + if (!labibb)
> > + return -ENOMEM;
> > +
> > + labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > + if (!labibb->regmap) {
> > + dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
> > + return -ENODEV;
> > + }
> > +
> > + labibb->dev = &pdev->dev;
> > +
> > + match = of_match_device(qcom_labibb_match, &pdev->dev);
> > + if (!match)
> > + return -ENODEV;
> > +
> > + for (reg = match->data; reg->name; reg++) {
> > + child = of_get_child_by_name(pdev->dev.of_node, reg->name);
> > +
> > + /* TODO: This validates if the type of regulator is indeed
> > + * what's mentioned in DT.
> > + * I'm not sure if this is needed, but we'll keep it for now.
> > + */
> > + ret = regmap_bulk_read(labibb->regmap,
> > + reg->base + REG_PERPH_TYPE,
> > + &type, 1);
> > + if (ret < 0) {
> > + dev_err(labibb->dev,
> > + "Peripheral type read failed ret=%d\n",
> > + ret);
> > + return -EINVAL;
> > + }
> > +
> > + if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) {
> > + dev_err(labibb->dev,
> > + "qcom_labibb: unknown peripheral type\n");
> > + return -EINVAL;
> > + } else if (type != reg->type) {
> > + dev_err(labibb->dev,
> > + "qcom_labibb: type read %x doesn't match DT %x\n",
> > + type, reg->type);
> > + return -EINVAL;
> > + }
> > +
> > + ret = register_labibb_regulator(labibb, reg, child);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev,
> > + "qcom_labibb: error registering %s regulator: %d\n",
> > + child->full_name, ret);
>
> You already printed in register_labibb_regulator() so no need to print
> again.
Got it.
>
> > + return ret;
> > + }
> > + }
> > +
> > + dev_set_drvdata(&pdev->dev, labibb);
>
> rdev_get_drvdata() returns cfg.driver_data, so you don't need this one.
Got it, and it ties to your other comment about struct qcom_labibb. Will update.
>
> Regards,
> Bjorn
>
> > + return 0;
> > +}
> > +
> > +static struct platform_driver qcom_labibb_regulator_driver = {
> > + .driver = {
> > + .name = "qcom-lab-ibb-regulator",
> > + .of_match_table = qcom_labibb_match,
> > + },
> > + .probe = qcom_labibb_regulator_probe,
> > +};
> > +module_platform_driver(qcom_labibb_regulator_driver);
> > +
> > +MODULE_DESCRIPTION("Qualcomm labibb driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.26.2
> >
Best,
Sumit.
^ permalink raw reply
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Kieran Bingham @ 2020-05-14 11:47 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: linux-renesas-soc, linux-media, devicetree, linux-kernel,
Mauro Carvalho Chehab, Kieran Bingham, Laurent Pinchart,
Jacopo Mondi, Niklas Söderlund, sakari.ailus, Hans Verkuil,
Hyun Kwon, Rob Herring, Jacopo Mondi, Laurent Pinchart,
Niklas Söderlund
In-Reply-To: <20200514101356.GF2877@Mani-XPS-13-9360>
On 14/05/2020 11:13, Manivannan Sadhasivam wrote:
> Hi Kieran,
>
> On Thu, May 14, 2020 at 11:02:53AM +0100, Kieran Bingham wrote:
>> Hi Mani,
>>
>> On 12/05/2020 19:17, Manivannan Sadhasivam wrote:
>>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
>>>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
>>>> CSI-2 output. The device supports multicamera streaming applications,
>>>> and features the ability to synchronise the attached cameras.
>>>>
>>>> CSI-2 output can be configured with 1 to 4 lanes, and a control channel
>>>> is supported over I2C, which implements an I2C mux to facilitate
>>>> communications with connected cameras across the reverse control
>>>> channel.
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>
>>>> --
>>>> v2:
>>>> - Fix MAINTAINERS entry
>>>>
>>>> This posting is released with the following modifications to work
>>>> without Sakari's VC developments:
>>>> - max9286_g_mbus_config() re-instated
>>>> - max9286_get_frame_desc() is not bus/csi aware
>>>> - max9286_{get,set}_routing() removed
>>>>
>>>> v3:
>>>> - Initialise notifier with v4l2_async_notifier_init
>>>> - Update for new mbus csi2 format V4L2_MBUS_CSI2_DPHY
>>>>
>>>> v4: - Re-introduce required code to function with the VC series.
>>>>
>>>> - Implement max9286_get_routing, max9286_set_routing
>>>> - Remove max9286_g_mbus_config
>>>>
>>>> v5: (internal release)
>>>> - Fix printk formatting for hex value
>>>> - max9286->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE (add |)
>>>> - MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER -> MEDIA_ENT_F_VID_IF_BRIDGE
>>>> - Remove 'device is bound' workaround
>>>>
>>>> v6:
>>>> - v4l2_subdev_krouting instead of v4l2_subdev_routing separated
>>>> to allow integration without the VC/V4L2-Mux series.
>>>> - convert sd_to_max9286 to inline function
>>>> - rename max9286_device to max9286_priv
>>>> - Cleanup the v4l2_async_notifier
>>>> - Extend MODULE_AUTHOR
>>>> - Replace of_graph_get_endpoint_by_regs with fwnode_graph_get_endpoint_by_id
>>>> - Pass default bus type when parsing fwnode endpoint (Manivannan Sadhasivam)
>>>> - Use new YAML file reference in MAINTAINERS
>>>> - Parse new i2c-mux node in max9286_get_i2c_by_id
>>>> (This could/should be refactored to parse these separately first)
>>>> - Spelling and calculation fixes in the FSYNC_LOCKED check comments
>>>> - Identify each enabled i2c-mux channel in a single pass
>>>> - max9286: Improve mux-state readbility [v2]
>>>> - Fix frame sync lock durations
>>>> - Add comment to describe /why/ we must open the mux in s_stream
>>>> - use -EXDEV as return code for failed link synchronisation.
>>>> - Fix reference counting of the dt nodeS
>>>> - Convert to probe_new for I2C
>>>> - Remove redundant max9286_i2c_mux_state
>>>> - Provide optional enable-gpio (max9286-pwdn)
>>>>
>>>> v7:
>>>> [Kieran]
>>>> - Ensure powerdown lines are optional
>>>> - Add a 4ms power-up delay
>>>> - Add max9286_check_config_link() to core
>>>> - Add GPIO chip controller for GPIO0OUT and GPIO1OUT
>>>> - Fix GPIO registration
>>>> - max9286: Split out async registration
>>>> (fixes regulator -EPROBE_DEFERs failures)
>>>> - Collect all V4L2 registrations
>>>> - balance v4l2_async refcnting
>>>> - Rename max9286_v4l2_async_ => max9286_v4l2_notifier_
>>>>
>>>> [Jacopo]
>>>> - Remove redundanct MAXIM_I2C_SPEED macros
>>>> - Move notifiers operations
>>>> - Add delay after reverse channel reconfiguration
>>>> - Move link setup to completion
>>>> - Fix up max9286_check_config_link() implementation
>>>> - Remove redundant dual configuration of reverse channel
>>>>
>>>> v8:
>>>>
>>>> [Kieran]
>>>> - Update the bound_sources mask on unbind
>>>> - Convert probe kzalloc usage to devm_ variant
>>>> - Fix up cleanup path from GPIO PowerDown registration
>>>> - cleanup GPIO device registration fail path
>>>> - Convert to use devm_regulator_get()
>>>> - Fit max9286_parse_dt print on one line
>>>> - Move multi-device workarounds out of upstream driver
>>>> - Remove I2C mod-table
>>>> - Lock format changes
>>>> - Describe pad index usage
>>>> - Remove poc_enabled workaround
>>>> - Rename the max9286_gpio to be more explicit on it's actions.
>>>> - Move max9286_init_format call
>>>> - Rework probe sequence and simplify error paths.
>>>> - Simplify i2c comments
>>>> - Implement Pixelrate control
>>>> - Disable overlap window
>>>>
>>>> [Jacopo]
>>>> - Adapt Kconfig to latest upstream changes
>>>> - Put of node on error
>>>> - Calculate pixel rate
>>>> - Simplify overlap window disablement
>>>>
>>>> v9:
>>>>
>>>> [Kieran]
>>>> - Kconfig: Depend on OF
>>>> - Re-sort addition to Makefile
>>>> ---
>>>> MAINTAINERS | 10 +
>>>> drivers/media/i2c/Kconfig | 13 +
>>>> drivers/media/i2c/Makefile | 1 +
>>>> drivers/media/i2c/max9286.c | 1332 +++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 1356 insertions(+)
>>>> create mode 100644 drivers/media/i2c/max9286.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index a7bb6e22d5da..99e3bf7760fd 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -10274,6 +10274,16 @@ F: Documentation/hwmon/max6697.rst
>>>> F: drivers/hwmon/max6697.c
>>>> F: include/linux/platform_data/max6697.h
>>>>
>>>> +MAX9286 QUAD GMSL DESERIALIZER DRIVER
>>>> +M: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>> +M: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> +M: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> +M: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>> +L: linux-media@vger.kernel.org
>>>> +S: Maintained
>>>> +F: Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>>>> +F: drivers/media/i2c/max9286.c
>>>> +
>>>> MAX9860 MONO AUDIO VOICE CODEC DRIVER
>>>> M: Peter Rosin <peda@axentia.se>
>>>> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
>>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>>> index 3abc80373ec0..2e390f41f6da 100644
>>>> --- a/drivers/media/i2c/Kconfig
>>>> +++ b/drivers/media/i2c/Kconfig
>>>> @@ -464,6 +464,19 @@ config VIDEO_VPX3220
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called vpx3220.
>>>>
>>>> +config VIDEO_MAX9286
>>>> + tristate "Maxim MAX9286 GMSL deserializer support"
>>>> + depends on I2C && I2C_MUX
>>>> + depends on OF
>>>> + select V4L2_FWNODE
>>>> + select VIDEO_V4L2_SUBDEV_API
>>>> + select MEDIA_CONTROLLER
>>>> + help
>>>> + This driver supports the Maxim MAX9286 GMSL deserializer.
>>>> +
>>>> + To compile this driver as a module, choose M here: the
>>>> + module will be called max9286.
>>>> +
>>>> comment "Video and audio decoders"
>>>>
>>>> config VIDEO_SAA717X
>>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>>> index 77bf7d0b691f..f0b001ee4b05 100644
>>>> --- a/drivers/media/i2c/Makefile
>>>> +++ b/drivers/media/i2c/Makefile
>>>> @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o
>>>> obj-$(CONFIG_VIDEO_IMX290) += imx290.o
>>>> obj-$(CONFIG_VIDEO_IMX319) += imx319.o
>>>> obj-$(CONFIG_VIDEO_IMX355) += imx355.o
>>>> +obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
>>>> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>>>>
>>>> obj-$(CONFIG_SDR_MAX2175) += max2175.o
>>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>>> new file mode 100644
>>>> index 000000000000..481d65f2b51d
>>>> --- /dev/null
>>>> +++ b/drivers/media/i2c/max9286.c
>>>> @@ -0,0 +1,1332 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Maxim MAX9286 GMSL Deserializer Driver
>>>> + *
>>>> + * Copyright (C) 2017-2019 Jacopo Mondi
>>>> + * Copyright (C) 2017-2019 Kieran Bingham
>>>> + * Copyright (C) 2017-2019 Laurent Pinchart
>>>> + * Copyright (C) 2017-2019 Niklas Söderlund
>>>> + * Copyright (C) 2016 Renesas Electronics Corporation
>>>> + * Copyright (C) 2015 Cogent Embedded, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/fwnode.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/gpio/driver.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/i2c-mux.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include <media/v4l2-async.h>
>>>> +#include <media/v4l2-ctrls.h>
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-fwnode.h>
>>>> +#include <media/v4l2-subdev.h>
>>>> +
>>>> +/* Register 0x00 */
>>>> +#define MAX9286_MSTLINKSEL_AUTO (7 << 5)
>>>> +#define MAX9286_MSTLINKSEL(n) ((n) << 5)
>>>> +#define MAX9286_EN_VS_GEN BIT(4)
>>>> +#define MAX9286_LINKEN(n) (1 << (n))
>>>> +/* Register 0x01 */
>>>> +#define MAX9286_FSYNCMODE_ECU (3 << 6)
>>>> +#define MAX9286_FSYNCMODE_EXT (2 << 6)
>>>> +#define MAX9286_FSYNCMODE_INT_OUT (1 << 6)
>>>> +#define MAX9286_FSYNCMODE_INT_HIZ (0 << 6)
>>>> +#define MAX9286_GPIEN BIT(5)
>>>> +#define MAX9286_ENLMO_RSTFSYNC BIT(2)
>>>> +#define MAX9286_FSYNCMETH_AUTO (2 << 0)
>>>> +#define MAX9286_FSYNCMETH_SEMI_AUTO (1 << 0)
>>>> +#define MAX9286_FSYNCMETH_MANUAL (0 << 0)
>>>> +#define MAX9286_REG_FSYNC_PERIOD_L 0x06
>>>> +#define MAX9286_REG_FSYNC_PERIOD_M 0x07
>>>> +#define MAX9286_REG_FSYNC_PERIOD_H 0x08
>>>> +/* Register 0x0a */
>>>> +#define MAX9286_FWDCCEN(n) (1 << ((n) + 4))
>>>> +#define MAX9286_REVCCEN(n) (1 << (n))
>>>> +/* Register 0x0c */
>>>> +#define MAX9286_HVEN BIT(7)
>>>> +#define MAX9286_EDC_6BIT_HAMMING (2 << 5)
>>>> +#define MAX9286_EDC_6BIT_CRC (1 << 5)
>>>> +#define MAX9286_EDC_1BIT_PARITY (0 << 5)
>>>> +#define MAX9286_DESEL BIT(4)
>>>> +#define MAX9286_INVVS BIT(3)
>>>> +#define MAX9286_INVHS BIT(2)
>>>> +#define MAX9286_HVSRC_D0 (2 << 0)
>>>> +#define MAX9286_HVSRC_D14 (1 << 0)
>>>> +#define MAX9286_HVSRC_D18 (0 << 0)
>>>> +/* Register 0x0f */
>>>> +#define MAX9286_0X0F_RESERVED BIT(3)
>>>> +/* Register 0x12 */
>>>> +#define MAX9286_CSILANECNT(n) (((n) - 1) << 6)
>>>> +#define MAX9286_CSIDBL BIT(5)
>>>> +#define MAX9286_DBL BIT(4)
>>>> +#define MAX9286_DATATYPE_USER_8BIT (11 << 0)
>>>> +#define MAX9286_DATATYPE_USER_YUV_12BIT (10 << 0)
>>>> +#define MAX9286_DATATYPE_USER_24BIT (9 << 0)
>>>> +#define MAX9286_DATATYPE_RAW14 (8 << 0)
>>>> +#define MAX9286_DATATYPE_RAW11 (7 << 0)
>>>> +#define MAX9286_DATATYPE_RAW10 (6 << 0)
>>>> +#define MAX9286_DATATYPE_RAW8 (5 << 0)
>>>> +#define MAX9286_DATATYPE_YUV422_10BIT (4 << 0)
>>>> +#define MAX9286_DATATYPE_YUV422_8BIT (3 << 0)
>>>> +#define MAX9286_DATATYPE_RGB555 (2 << 0)
>>>> +#define MAX9286_DATATYPE_RGB565 (1 << 0)
>>>> +#define MAX9286_DATATYPE_RGB888 (0 << 0)
>>>> +/* Register 0x15 */
>>>> +#define MAX9286_VC(n) ((n) << 5)
>>>> +#define MAX9286_VCTYPE BIT(4)
>>>> +#define MAX9286_CSIOUTEN BIT(3)
>>>> +#define MAX9286_0X15_RESV (3 << 0)
>>>> +/* Register 0x1b */
>>>> +#define MAX9286_SWITCHIN(n) (1 << ((n) + 4))
>>>> +#define MAX9286_ENEQ(n) (1 << (n))
>>>> +/* Register 0x27 */
>>>> +#define MAX9286_LOCKED BIT(7)
>>>> +/* Register 0x31 */
>>>> +#define MAX9286_FSYNC_LOCKED BIT(6)
>>>> +/* Register 0x34 */
>>>> +#define MAX9286_I2CLOCACK BIT(7)
>>>> +#define MAX9286_I2CSLVSH_1046NS_469NS (3 << 5)
>>>> +#define MAX9286_I2CSLVSH_938NS_352NS (2 << 5)
>>>> +#define MAX9286_I2CSLVSH_469NS_234NS (1 << 5)
>>>> +#define MAX9286_I2CSLVSH_352NS_117NS (0 << 5)
>>>> +#define MAX9286_I2CMSTBT_837KBPS (7 << 2)
>>>> +#define MAX9286_I2CMSTBT_533KBPS (6 << 2)
>>>> +#define MAX9286_I2CMSTBT_339KBPS (5 << 2)
>>>> +#define MAX9286_I2CMSTBT_173KBPS (4 << 2)
>>>> +#define MAX9286_I2CMSTBT_105KBPS (3 << 2)
>>>> +#define MAX9286_I2CMSTBT_84KBPS (2 << 2)
>>>> +#define MAX9286_I2CMSTBT_28KBPS (1 << 2)
>>>> +#define MAX9286_I2CMSTBT_8KBPS (0 << 2)
>>>> +#define MAX9286_I2CSLVTO_NONE (3 << 0)
>>>> +#define MAX9286_I2CSLVTO_1024US (2 << 0)
>>>> +#define MAX9286_I2CSLVTO_256US (1 << 0)
>>>> +#define MAX9286_I2CSLVTO_64US (0 << 0)
>>>> +/* Register 0x3b */
>>>> +#define MAX9286_REV_TRF(n) ((n) << 4)
>>>> +#define MAX9286_REV_AMP(n) ((((n) - 30) / 10) << 1) /* in mV */
>>>> +#define MAX9286_REV_AMP_X BIT(0)
>>>> +/* Register 0x3f */
>>>> +#define MAX9286_EN_REV_CFG BIT(6)
>>>> +#define MAX9286_REV_FLEN(n) ((n) - 20)
>>>> +/* Register 0x49 */
>>>> +#define MAX9286_VIDEO_DETECT_MASK 0x0f
>>>> +/* Register 0x69 */
>>>> +#define MAX9286_LFLTBMONMASKED BIT(7)
>>>> +#define MAX9286_LOCKMONMASKED BIT(6)
>>>> +#define MAX9286_AUTOCOMBACKEN BIT(5)
>>>> +#define MAX9286_AUTOMASKEN BIT(4)
>>>> +#define MAX9286_MASKLINK(n) ((n) << 0)
>>>> +
>>>> +/*
>>>> + * The sink and source pads are created to match the OF graph port numbers so
>>>> + * that their indexes can be used interchangeably.
>>>> + */
>>>> +#define MAX9286_NUM_GMSL 4
>>>> +#define MAX9286_N_SINKS 4
>>>> +#define MAX9286_N_PADS 5
>>>> +#define MAX9286_SRC_PAD 4
>>>> +
>>>> +struct max9286_source {
>>>> + struct v4l2_async_subdev asd;
>>>> + struct v4l2_subdev *sd;
>>>> + struct fwnode_handle *fwnode;
>>>> +};
>>>> +
>>>> +#define asd_to_max9286_source(_asd) \
>>>> + container_of(_asd, struct max9286_source, asd)
>>>> +
>>>> +struct max9286_priv {
>>>> + struct i2c_client *client;
>>>> + struct gpio_desc *gpiod_pwdn;
>>>> + struct v4l2_subdev sd;
>>>> + struct media_pad pads[MAX9286_N_PADS];
>>>> + struct regulator *regulator;
>>>> +
>>>> + struct gpio_chip gpio;
>>>> + u8 gpio_state;
>>>> +
>>>> + struct i2c_mux_core *mux;
>>>> + unsigned int mux_channel;
>>>> + bool mux_open;
>>>> +
>>>> + struct v4l2_ctrl_handler ctrls;
>>>> + struct v4l2_ctrl *pixelrate;
>>>> +
>>>> + struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>>>> +
>>>> + /* Protects controls and fmt structures */
>>>> + struct mutex mutex;
>>>> +
>>>> + unsigned int nsources;
>>>> + unsigned int source_mask;
>>>> + unsigned int route_mask;
>>>> + unsigned int bound_sources;
>>>> + unsigned int csi2_data_lanes;
>>>> + struct max9286_source sources[MAX9286_NUM_GMSL];
>>>> + struct v4l2_async_notifier notifier;
>>>> +};
>>>> +
>>>
>>> [...]
>>>
>>>> +static int max9286_register_gpio(struct max9286_priv *priv)
>>>> +{
>>>> + struct device *dev = &priv->client->dev;
>>>> + struct gpio_chip *gpio = &priv->gpio;
>>>> + int ret;
>>>> +
>>>> + static const char * const names[] = {
>>>> + "GPIO0OUT",
>>>> + "GPIO1OUT",
>>>> + };
>>>> +
>>>> + /* Configure the GPIO */
>>>> + gpio->label = dev_name(dev);
>>>
>>> So if you have more than one MAX9286 in a system, all gpiochips will appear
>>> with the same name. I'd recommend to append the index to distinguish properly.
>>
>> Ah yes, that's a good point, and I think I've even seen that.
>>
>> I'll fix it now.
>>
>>>
>>>> + gpio->parent = dev;
>>>> + gpio->owner = THIS_MODULE;
>>>> + gpio->of_node = dev->of_node;
>>>> + gpio->ngpio = 2;
>>>> + gpio->base = -1;
>>>> + gpio->set = max9286_gpio_set;
>>>> + gpio->get = max9286_gpio_get;
>>>> + gpio->can_sleep = true;
>>>> + gpio->names = names;
>>>> +
>>>> + /* GPIO values default to high */
>>>> + priv->gpio_state = BIT(0) | BIT(1);
>>>> +
>>>> + ret = devm_gpiochip_add_data(dev, gpio, priv);
>>>> + if (ret)
>>>> + dev_err(dev, "Unable to create gpio_chip\n");
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>> +static int max9286_parse_dt(struct max9286_priv *priv)
>>>> +{
>>>> + struct device *dev = &priv->client->dev;
>>>> + struct device_node *i2c_mux;
>>>> + struct device_node *node = NULL;
>>>> + unsigned int i2c_mux_mask = 0;
>>>> +
>>>> + of_node_get(dev->of_node);
>>>
>>> Why this is needed?
>>
>> Hrm .. I recall adding it to solve dt reference balancing.
>>
>> I wish I'd added a comment at the time ... as I can't recall the details
>> now.
>>
>
> I understand that it is for the refcount balancing but I certainly don't see
> a need for it.
I'll go through and try to validate this again now.
>
>>>> + i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>>> + if (!i2c_mux) {
>>>> + dev_err(dev, "Failed to find i2c-mux node\n");
>>>> + of_node_put(dev->of_node);
>>>> + return -EINVAL;
>>>> + }
>>>> +
> [...]
>>>
>>> [...]
>>>
>>>> +static int max9286_remove(struct i2c_client *client)
>>>> +{
>>>> + struct max9286_priv *priv = i2c_get_clientdata(client);
>>>> +
>>>> + i2c_mux_del_adapters(priv->mux);
>>>> +
>>>> + max9286_v4l2_unregister(priv);
>>>> +
>>>> + regulator_disable(priv->regulator);
>>>> +
>>>> + gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>>>
>>> Usual power down sequence is to pull the power down gpio low and then turn off
>>> the regulators. This helps in clearing up the internal state machine properly.
>>
>> Do you mean usual, among drivers using regulators? or usual for the max9286?
>>
>
> Usual for devices exposing shutdown gpios and powered by an external regulator.
>
>> I have a platform which controls the regulators for the cameras
>> (priv->regulator) through one of the GPIOs provided by the MAX9286.
>>
>> If we powerdown the max9286 first, then we will not be able to change
>> the GPIO value on the gpio controlled regulator.
>>
>
> Hmm, I didn't think of this usecase. Let's keep it as it is.
Aha indeed it's not clear (especially as the variable is just
'regulator'). The regulator being controlled here does not power the
max9286, but the cameras attached to the max9286 which can be powered up
/ down separately to the max9286.
--
Kieran
>
> Thanks,
> Mani
>
>> (Currently it doesn't expose as a gpio regulator, but just a gpio-hog -
>> but thats a different matter)
>>
>> --
>> Kieran
>>
>>
>>>
>>> Thanks,
>>> Mani
>>>
>>>> +
>>>> + max9286_cleanup_dt(priv);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id max9286_dt_ids[] = {
>>>> + { .compatible = "maxim,max9286" },
>>>> + {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, max9286_dt_ids);
>>>> +
>>>> +static struct i2c_driver max9286_i2c_driver = {
>>>> + .driver = {
>>>> + .name = "max9286",
>>>> + .of_match_table = of_match_ptr(max9286_dt_ids),
>>>> + },
>>>> + .probe_new = max9286_probe,
>>>> + .remove = max9286_remove,
>>>> +};
>>>> +
>>>> +module_i2c_driver(max9286_i2c_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("Maxim MAX9286 GMSL Deserializer Driver");
>>>> +MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.25.1
>>>>
>>
^ permalink raw reply
* Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
From: Kieran Bingham @ 2020-05-14 11:47 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: linux-renesas-soc, linux-media, devicetree, linux-kernel,
Mauro Carvalho Chehab, Kieran Bingham, Laurent Pinchart,
Jacopo Mondi, Niklas Söderlund, sakari.ailus, Hans Verkuil,
Hyun Kwon, Rob Herring, Jacopo Mondi, Laurent Pinchart,
Niklas Söderlund
In-Reply-To: <20200514101356.GF2877@Mani-XPS-13-9360>
Hi Mani,
On 14/05/2020 11:13, Manivannan Sadhasivam wrote:
> Hi Kieran,
>
> On Thu, May 14, 2020 at 11:02:53AM +0100, Kieran Bingham wrote:
>> Hi Mani,
>>
>> On 12/05/2020 19:17, Manivannan Sadhasivam wrote:
>>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
>>>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
>>>> CSI-2 output. The device supports multicamera streaming applications,
>>>> and features the ability to synchronise the attached cameras.
>>>>
>>>> CSI-2 output can be configured with 1 to 4 lanes, and a control channel
>>>> is supported over I2C, which implements an I2C mux to facilitate
>>>> communications with connected cameras across the reverse control
>>>> channel.
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>
>>>> --
>>>> v2:
>>>> - Fix MAINTAINERS entry
>>>>
>>>> This posting is released with the following modifications to work
>>>> without Sakari's VC developments:
>>>> - max9286_g_mbus_config() re-instated
>>>> - max9286_get_frame_desc() is not bus/csi aware
>>>> - max9286_{get,set}_routing() removed
>>>>
>>>> v3:
>>>> - Initialise notifier with v4l2_async_notifier_init
>>>> - Update for new mbus csi2 format V4L2_MBUS_CSI2_DPHY
>>>>
>>>> v4: - Re-introduce required code to function with the VC series.
>>>>
>>>> - Implement max9286_get_routing, max9286_set_routing
>>>> - Remove max9286_g_mbus_config
>>>>
>>>> v5: (internal release)
>>>> - Fix printk formatting for hex value
>>>> - max9286->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE (add |)
>>>> - MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER -> MEDIA_ENT_F_VID_IF_BRIDGE
>>>> - Remove 'device is bound' workaround
>>>>
>>>> v6:
>>>> - v4l2_subdev_krouting instead of v4l2_subdev_routing separated
>>>> to allow integration without the VC/V4L2-Mux series.
>>>> - convert sd_to_max9286 to inline function
>>>> - rename max9286_device to max9286_priv
>>>> - Cleanup the v4l2_async_notifier
>>>> - Extend MODULE_AUTHOR
>>>> - Replace of_graph_get_endpoint_by_regs with fwnode_graph_get_endpoint_by_id
>>>> - Pass default bus type when parsing fwnode endpoint (Manivannan Sadhasivam)
>>>> - Use new YAML file reference in MAINTAINERS
>>>> - Parse new i2c-mux node in max9286_get_i2c_by_id
>>>> (This could/should be refactored to parse these separately first)
>>>> - Spelling and calculation fixes in the FSYNC_LOCKED check comments
>>>> - Identify each enabled i2c-mux channel in a single pass
>>>> - max9286: Improve mux-state readbility [v2]
>>>> - Fix frame sync lock durations
>>>> - Add comment to describe /why/ we must open the mux in s_stream
>>>> - use -EXDEV as return code for failed link synchronisation.
>>>> - Fix reference counting of the dt nodeS
>>>> - Convert to probe_new for I2C
>>>> - Remove redundant max9286_i2c_mux_state
>>>> - Provide optional enable-gpio (max9286-pwdn)
>>>>
>>>> v7:
>>>> [Kieran]
>>>> - Ensure powerdown lines are optional
>>>> - Add a 4ms power-up delay
>>>> - Add max9286_check_config_link() to core
>>>> - Add GPIO chip controller for GPIO0OUT and GPIO1OUT
>>>> - Fix GPIO registration
>>>> - max9286: Split out async registration
>>>> (fixes regulator -EPROBE_DEFERs failures)
>>>> - Collect all V4L2 registrations
>>>> - balance v4l2_async refcnting
>>>> - Rename max9286_v4l2_async_ => max9286_v4l2_notifier_
>>>>
>>>> [Jacopo]
>>>> - Remove redundanct MAXIM_I2C_SPEED macros
>>>> - Move notifiers operations
>>>> - Add delay after reverse channel reconfiguration
>>>> - Move link setup to completion
>>>> - Fix up max9286_check_config_link() implementation
>>>> - Remove redundant dual configuration of reverse channel
>>>>
>>>> v8:
>>>>
>>>> [Kieran]
>>>> - Update the bound_sources mask on unbind
>>>> - Convert probe kzalloc usage to devm_ variant
>>>> - Fix up cleanup path from GPIO PowerDown registration
>>>> - cleanup GPIO device registration fail path
>>>> - Convert to use devm_regulator_get()
>>>> - Fit max9286_parse_dt print on one line
>>>> - Move multi-device workarounds out of upstream driver
>>>> - Remove I2C mod-table
>>>> - Lock format changes
>>>> - Describe pad index usage
>>>> - Remove poc_enabled workaround
>>>> - Rename the max9286_gpio to be more explicit on it's actions.
>>>> - Move max9286_init_format call
>>>> - Rework probe sequence and simplify error paths.
>>>> - Simplify i2c comments
>>>> - Implement Pixelrate control
>>>> - Disable overlap window
>>>>
>>>> [Jacopo]
>>>> - Adapt Kconfig to latest upstream changes
>>>> - Put of node on error
>>>> - Calculate pixel rate
>>>> - Simplify overlap window disablement
>>>>
>>>> v9:
>>>>
>>>> [Kieran]
>>>> - Kconfig: Depend on OF
>>>> - Re-sort addition to Makefile
>>>> ---
>>>> MAINTAINERS | 10 +
>>>> drivers/media/i2c/Kconfig | 13 +
>>>> drivers/media/i2c/Makefile | 1 +
>>>> drivers/media/i2c/max9286.c | 1332 +++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 1356 insertions(+)
>>>> create mode 100644 drivers/media/i2c/max9286.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index a7bb6e22d5da..99e3bf7760fd 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -10274,6 +10274,16 @@ F: Documentation/hwmon/max6697.rst
>>>> F: drivers/hwmon/max6697.c
>>>> F: include/linux/platform_data/max6697.h
>>>>
>>>> +MAX9286 QUAD GMSL DESERIALIZER DRIVER
>>>> +M: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>> +M: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> +M: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> +M: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>> +L: linux-media@vger.kernel.org
>>>> +S: Maintained
>>>> +F: Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>>>> +F: drivers/media/i2c/max9286.c
>>>> +
>>>> MAX9860 MONO AUDIO VOICE CODEC DRIVER
>>>> M: Peter Rosin <peda@axentia.se>
>>>> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
>>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>>> index 3abc80373ec0..2e390f41f6da 100644
>>>> --- a/drivers/media/i2c/Kconfig
>>>> +++ b/drivers/media/i2c/Kconfig
>>>> @@ -464,6 +464,19 @@ config VIDEO_VPX3220
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called vpx3220.
>>>>
>>>> +config VIDEO_MAX9286
>>>> + tristate "Maxim MAX9286 GMSL deserializer support"
>>>> + depends on I2C && I2C_MUX
>>>> + depends on OF
>>>> + select V4L2_FWNODE
>>>> + select VIDEO_V4L2_SUBDEV_API
>>>> + select MEDIA_CONTROLLER
>>>> + help
>>>> + This driver supports the Maxim MAX9286 GMSL deserializer.
>>>> +
>>>> + To compile this driver as a module, choose M here: the
>>>> + module will be called max9286.
>>>> +
>>>> comment "Video and audio decoders"
>>>>
>>>> config VIDEO_SAA717X
>>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>>> index 77bf7d0b691f..f0b001ee4b05 100644
>>>> --- a/drivers/media/i2c/Makefile
>>>> +++ b/drivers/media/i2c/Makefile
>>>> @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o
>>>> obj-$(CONFIG_VIDEO_IMX290) += imx290.o
>>>> obj-$(CONFIG_VIDEO_IMX319) += imx319.o
>>>> obj-$(CONFIG_VIDEO_IMX355) += imx355.o
>>>> +obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
>>>> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>>>>
>>>> obj-$(CONFIG_SDR_MAX2175) += max2175.o
>>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>>> new file mode 100644
>>>> index 000000000000..481d65f2b51d
>>>> --- /dev/null
>>>> +++ b/drivers/media/i2c/max9286.c
>>>> @@ -0,0 +1,1332 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Maxim MAX9286 GMSL Deserializer Driver
>>>> + *
>>>> + * Copyright (C) 2017-2019 Jacopo Mondi
>>>> + * Copyright (C) 2017-2019 Kieran Bingham
>>>> + * Copyright (C) 2017-2019 Laurent Pinchart
>>>> + * Copyright (C) 2017-2019 Niklas Söderlund
>>>> + * Copyright (C) 2016 Renesas Electronics Corporation
>>>> + * Copyright (C) 2015 Cogent Embedded, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/fwnode.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/gpio/driver.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/i2c-mux.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include <media/v4l2-async.h>
>>>> +#include <media/v4l2-ctrls.h>
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-fwnode.h>
>>>> +#include <media/v4l2-subdev.h>
>>>> +
>>>> +/* Register 0x00 */
>>>> +#define MAX9286_MSTLINKSEL_AUTO (7 << 5)
>>>> +#define MAX9286_MSTLINKSEL(n) ((n) << 5)
>>>> +#define MAX9286_EN_VS_GEN BIT(4)
>>>> +#define MAX9286_LINKEN(n) (1 << (n))
>>>> +/* Register 0x01 */
>>>> +#define MAX9286_FSYNCMODE_ECU (3 << 6)
>>>> +#define MAX9286_FSYNCMODE_EXT (2 << 6)
>>>> +#define MAX9286_FSYNCMODE_INT_OUT (1 << 6)
>>>> +#define MAX9286_FSYNCMODE_INT_HIZ (0 << 6)
>>>> +#define MAX9286_GPIEN BIT(5)
>>>> +#define MAX9286_ENLMO_RSTFSYNC BIT(2)
>>>> +#define MAX9286_FSYNCMETH_AUTO (2 << 0)
>>>> +#define MAX9286_FSYNCMETH_SEMI_AUTO (1 << 0)
>>>> +#define MAX9286_FSYNCMETH_MANUAL (0 << 0)
>>>> +#define MAX9286_REG_FSYNC_PERIOD_L 0x06
>>>> +#define MAX9286_REG_FSYNC_PERIOD_M 0x07
>>>> +#define MAX9286_REG_FSYNC_PERIOD_H 0x08
>>>> +/* Register 0x0a */
>>>> +#define MAX9286_FWDCCEN(n) (1 << ((n) + 4))
>>>> +#define MAX9286_REVCCEN(n) (1 << (n))
>>>> +/* Register 0x0c */
>>>> +#define MAX9286_HVEN BIT(7)
>>>> +#define MAX9286_EDC_6BIT_HAMMING (2 << 5)
>>>> +#define MAX9286_EDC_6BIT_CRC (1 << 5)
>>>> +#define MAX9286_EDC_1BIT_PARITY (0 << 5)
>>>> +#define MAX9286_DESEL BIT(4)
>>>> +#define MAX9286_INVVS BIT(3)
>>>> +#define MAX9286_INVHS BIT(2)
>>>> +#define MAX9286_HVSRC_D0 (2 << 0)
>>>> +#define MAX9286_HVSRC_D14 (1 << 0)
>>>> +#define MAX9286_HVSRC_D18 (0 << 0)
>>>> +/* Register 0x0f */
>>>> +#define MAX9286_0X0F_RESERVED BIT(3)
>>>> +/* Register 0x12 */
>>>> +#define MAX9286_CSILANECNT(n) (((n) - 1) << 6)
>>>> +#define MAX9286_CSIDBL BIT(5)
>>>> +#define MAX9286_DBL BIT(4)
>>>> +#define MAX9286_DATATYPE_USER_8BIT (11 << 0)
>>>> +#define MAX9286_DATATYPE_USER_YUV_12BIT (10 << 0)
>>>> +#define MAX9286_DATATYPE_USER_24BIT (9 << 0)
>>>> +#define MAX9286_DATATYPE_RAW14 (8 << 0)
>>>> +#define MAX9286_DATATYPE_RAW11 (7 << 0)
>>>> +#define MAX9286_DATATYPE_RAW10 (6 << 0)
>>>> +#define MAX9286_DATATYPE_RAW8 (5 << 0)
>>>> +#define MAX9286_DATATYPE_YUV422_10BIT (4 << 0)
>>>> +#define MAX9286_DATATYPE_YUV422_8BIT (3 << 0)
>>>> +#define MAX9286_DATATYPE_RGB555 (2 << 0)
>>>> +#define MAX9286_DATATYPE_RGB565 (1 << 0)
>>>> +#define MAX9286_DATATYPE_RGB888 (0 << 0)
>>>> +/* Register 0x15 */
>>>> +#define MAX9286_VC(n) ((n) << 5)
>>>> +#define MAX9286_VCTYPE BIT(4)
>>>> +#define MAX9286_CSIOUTEN BIT(3)
>>>> +#define MAX9286_0X15_RESV (3 << 0)
>>>> +/* Register 0x1b */
>>>> +#define MAX9286_SWITCHIN(n) (1 << ((n) + 4))
>>>> +#define MAX9286_ENEQ(n) (1 << (n))
>>>> +/* Register 0x27 */
>>>> +#define MAX9286_LOCKED BIT(7)
>>>> +/* Register 0x31 */
>>>> +#define MAX9286_FSYNC_LOCKED BIT(6)
>>>> +/* Register 0x34 */
>>>> +#define MAX9286_I2CLOCACK BIT(7)
>>>> +#define MAX9286_I2CSLVSH_1046NS_469NS (3 << 5)
>>>> +#define MAX9286_I2CSLVSH_938NS_352NS (2 << 5)
>>>> +#define MAX9286_I2CSLVSH_469NS_234NS (1 << 5)
>>>> +#define MAX9286_I2CSLVSH_352NS_117NS (0 << 5)
>>>> +#define MAX9286_I2CMSTBT_837KBPS (7 << 2)
>>>> +#define MAX9286_I2CMSTBT_533KBPS (6 << 2)
>>>> +#define MAX9286_I2CMSTBT_339KBPS (5 << 2)
>>>> +#define MAX9286_I2CMSTBT_173KBPS (4 << 2)
>>>> +#define MAX9286_I2CMSTBT_105KBPS (3 << 2)
>>>> +#define MAX9286_I2CMSTBT_84KBPS (2 << 2)
>>>> +#define MAX9286_I2CMSTBT_28KBPS (1 << 2)
>>>> +#define MAX9286_I2CMSTBT_8KBPS (0 << 2)
>>>> +#define MAX9286_I2CSLVTO_NONE (3 << 0)
>>>> +#define MAX9286_I2CSLVTO_1024US (2 << 0)
>>>> +#define MAX9286_I2CSLVTO_256US (1 << 0)
>>>> +#define MAX9286_I2CSLVTO_64US (0 << 0)
>>>> +/* Register 0x3b */
>>>> +#define MAX9286_REV_TRF(n) ((n) << 4)
>>>> +#define MAX9286_REV_AMP(n) ((((n) - 30) / 10) << 1) /* in mV */
>>>> +#define MAX9286_REV_AMP_X BIT(0)
>>>> +/* Register 0x3f */
>>>> +#define MAX9286_EN_REV_CFG BIT(6)
>>>> +#define MAX9286_REV_FLEN(n) ((n) - 20)
>>>> +/* Register 0x49 */
>>>> +#define MAX9286_VIDEO_DETECT_MASK 0x0f
>>>> +/* Register 0x69 */
>>>> +#define MAX9286_LFLTBMONMASKED BIT(7)
>>>> +#define MAX9286_LOCKMONMASKED BIT(6)
>>>> +#define MAX9286_AUTOCOMBACKEN BIT(5)
>>>> +#define MAX9286_AUTOMASKEN BIT(4)
>>>> +#define MAX9286_MASKLINK(n) ((n) << 0)
>>>> +
>>>> +/*
>>>> + * The sink and source pads are created to match the OF graph port numbers so
>>>> + * that their indexes can be used interchangeably.
>>>> + */
>>>> +#define MAX9286_NUM_GMSL 4
>>>> +#define MAX9286_N_SINKS 4
>>>> +#define MAX9286_N_PADS 5
>>>> +#define MAX9286_SRC_PAD 4
>>>> +
>>>> +struct max9286_source {
>>>> + struct v4l2_async_subdev asd;
>>>> + struct v4l2_subdev *sd;
>>>> + struct fwnode_handle *fwnode;
>>>> +};
>>>> +
>>>> +#define asd_to_max9286_source(_asd) \
>>>> + container_of(_asd, struct max9286_source, asd)
>>>> +
>>>> +struct max9286_priv {
>>>> + struct i2c_client *client;
>>>> + struct gpio_desc *gpiod_pwdn;
>>>> + struct v4l2_subdev sd;
>>>> + struct media_pad pads[MAX9286_N_PADS];
>>>> + struct regulator *regulator;
>>>> +
>>>> + struct gpio_chip gpio;
>>>> + u8 gpio_state;
>>>> +
>>>> + struct i2c_mux_core *mux;
>>>> + unsigned int mux_channel;
>>>> + bool mux_open;
>>>> +
>>>> + struct v4l2_ctrl_handler ctrls;
>>>> + struct v4l2_ctrl *pixelrate;
>>>> +
>>>> + struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>>>> +
>>>> + /* Protects controls and fmt structures */
>>>> + struct mutex mutex;
>>>> +
>>>> + unsigned int nsources;
>>>> + unsigned int source_mask;
>>>> + unsigned int route_mask;
>>>> + unsigned int bound_sources;
>>>> + unsigned int csi2_data_lanes;
>>>> + struct max9286_source sources[MAX9286_NUM_GMSL];
>>>> + struct v4l2_async_notifier notifier;
>>>> +};
>>>> +
>>>
>>> [...]
>>>
>>>> +static int max9286_register_gpio(struct max9286_priv *priv)
>>>> +{
>>>> + struct device *dev = &priv->client->dev;
>>>> + struct gpio_chip *gpio = &priv->gpio;
>>>> + int ret;
>>>> +
>>>> + static const char * const names[] = {
>>>> + "GPIO0OUT",
>>>> + "GPIO1OUT",
>>>> + };
>>>> +
>>>> + /* Configure the GPIO */
>>>> + gpio->label = dev_name(dev);
>>>
>>> So if you have more than one MAX9286 in a system, all gpiochips will appear
>>> with the same name. I'd recommend to append the index to distinguish properly.
>>
>> Ah yes, that's a good point, and I think I've even seen that.
>>
>> I'll fix it now.
Oh, in fact actually this doesn't.
gpiodetect prints:
gpiochip10 [4-004c] (2 lines)
gpiochip11 [4-006c] (2 lines)
and mostly references them as gpiochip10 and gpiochip11.
However,
> [ 2.318533] gpio gpiochip11: Detected name collision for GPIO name 'GPIO0OUT'
> [ 2.325739] gpio gpiochip11: Detected name collision for GPIO name 'GPIO1OUT'
That seems to be more of a problem for the gpio library, so I think I'll
just drop the const names. I don't think they add much value.
>>>> + gpio->parent = dev;
>>>> + gpio->owner = THIS_MODULE;
>>>> + gpio->of_node = dev->of_node;
>>>> + gpio->ngpio = 2;
>>>> + gpio->base = -1;
>>>> + gpio->set = max9286_gpio_set;
>>>> + gpio->get = max9286_gpio_get;
>>>> + gpio->can_sleep = true;
>>>> + gpio->names = names;
>>>> +
>>>> + /* GPIO values default to high */
>>>> + priv->gpio_state = BIT(0) | BIT(1);
>>>> +
>>>> + ret = devm_gpiochip_add_data(dev, gpio, priv);
>>>> + if (ret)
>>>> + dev_err(dev, "Unable to create gpio_chip\n");
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>> +static int max9286_parse_dt(struct max9286_priv *priv)
>>>> +{
>>>> + struct device *dev = &priv->client->dev;
>>>> + struct device_node *i2c_mux;
>>>> + struct device_node *node = NULL;
>>>> + unsigned int i2c_mux_mask = 0;
>>>> +
>>>> + of_node_get(dev->of_node);
>>>
>>> Why this is needed?
>>
>> Hrm .. I recall adding it to solve dt reference balancing.
>>
>> I wish I'd added a comment at the time ... as I can't recall the details
>> now.
>>
>
> I understand that it is for the refcount balancing but I certainly don't see
> a need for it.
>
>>>> + i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>>> + if (!i2c_mux) {
>>>> + dev_err(dev, "Failed to find i2c-mux node\n");
>>>> + of_node_put(dev->of_node);
>>>> + return -EINVAL;
>>>> + }
>>>> +
> [...]
>>>
>>> [...]
>>>
>>>> +static int max9286_remove(struct i2c_client *client)
>>>> +{
>>>> + struct max9286_priv *priv = i2c_get_clientdata(client);
>>>> +
>>>> + i2c_mux_del_adapters(priv->mux);
>>>> +
>>>> + max9286_v4l2_unregister(priv);
>>>> +
>>>> + regulator_disable(priv->regulator);
>>>> +
>>>> + gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>>>
>>> Usual power down sequence is to pull the power down gpio low and then turn off
>>> the regulators. This helps in clearing up the internal state machine properly.
>>
>> Do you mean usual, among drivers using regulators? or usual for the max9286?
>>
>
> Usual for devices exposing shutdown gpios and powered by an external regulator.
>
>> I have a platform which controls the regulators for the cameras
>> (priv->regulator) through one of the GPIOs provided by the MAX9286.
>>
>> If we powerdown the max9286 first, then we will not be able to change
>> the GPIO value on the gpio controlled regulator.
>>
>
> Hmm, I didn't think of this usecase. Let's keep it as it is.
>
> Thanks,
> Mani
>
>> (Currently it doesn't expose as a gpio regulator, but just a gpio-hog -
>> but thats a different matter)
>>
>> --
>> Kieran
>>
>>
>>>
>>> Thanks,
>>> Mani
>>>
>>>> +
>>>> + max9286_cleanup_dt(priv);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id max9286_dt_ids[] = {
>>>> + { .compatible = "maxim,max9286" },
>>>> + {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, max9286_dt_ids);
>>>> +
>>>> +static struct i2c_driver max9286_i2c_driver = {
>>>> + .driver = {
>>>> + .name = "max9286",
>>>> + .of_match_table = of_match_ptr(max9286_dt_ids),
>>>> + },
>>>> + .probe_new = max9286_probe,
>>>> + .remove = max9286_remove,
>>>> +};
>>>> +
>>>> +module_i2c_driver(max9286_i2c_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("Maxim MAX9286 GMSL Deserializer Driver");
>>>> +MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.25.1
>>>>
>>
^ permalink raw reply
* Re: [PATCH] dt-bindings: auxdisplay: hd44780: Convert to json-schema
From: Miguel Ojeda @ 2020-05-14 11:36 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Rob Herring, devicetree, linux-kernel
In-Reply-To: <20200514113003.19067-1-geert@linux-m68k.org>
Hi Rob,
On Thu, May 14, 2020 at 1:30 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Convert the Hitachi HD44780 Character LCD Controller Device Tree binding
> documentation to json-schema.
Do you usually take these ones or should I?
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
From: Felipe Balbi @ 2020-05-14 11:30 UTC (permalink / raw)
To: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
Doug Anderson, Matthias Kaehlcke
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
Chandana Kishori Chiluveru, Sandeep Maheswaram
In-Reply-To: <878shu4uwk.fsf@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 458 bytes --]
Felipe Balbi <balbi@kernel.org> writes:
> Hi,
>
> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
>> +{
>> + struct device *dev = qcom->dev;
>> + int ret;
>> +
>> + if (!device_is_bound(&qcom->dwc3->dev))
>> + return -EPROBE_DEFER;
>
> this breaks allmodconfig. I'm dropping this series from my queue for
> this merge window.
Sorry, I meant this patch ;-)
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox